Typing upload-pack ERR responses

[kim]

I have a test case which tries to fetch a ref the remote end doesn't know about:

15:49:04.445735 pkt-line.c:80           packet:  upload-pack< command=fetch
15:49:04.445784 pkt-line.c:80           packet:  upload-pack< agent=git/oxide-0.10.4
15:49:04.445787 pkt-line.c:80           packet:  upload-pack< 0001
15:49:04.445796 pkt-line.c:80           packet:  upload-pack< thin-pack
15:49:04.445800 pkt-line.c:80           packet:  upload-pack< include-tag
15:49:04.445803 pkt-line.c:80           packet:  upload-pack< ofs-delta
15:49:04.446266 pkt-line.c:80           packet:  upload-pack< want-ref refs/namespaces/hnrkez5kbhsrh97mimd8xgxf7w4cimcnc9wdo/refs/remotes/hyn68wpms9eognsszkrsikgbwkr9trotu8etrknwdw178z18bbgj3g/rad/id
15:49:04.446297 pkt-line.c:80           packet:  upload-pack> ERR unknown ref refs/namespaces/hnrkez5kbhsrh97mimd8xgxf7w4cimcnc9wdo/refs/remotes/hyn68wpms9eognsszkrsikgbwkr9trotu8etrknwdw178z18bbgj3g/rad/id

The Displayed error message from this is "The server response could not be parsed". Debugging the error shows that it is two nested io::Errors.

I couldn't quite figure out where to look, but it seems to me like the pktline reader should recognise this as a protocol message, and return a dedicated error variant for it.

"unknown ref" is of course still an arbitrary string, but it would still be helpful to be able to pattern match on those kinds of errors.

[Byron]

Thanks for reporting, [rant] error handling around these pseudo-streaming-iterator line readers wrapped in impls of std::io::Read is definitely not nice and begging for some sort of rewrite once GATs are there or… a version that just copies the line around to make the borrow check issues go away right now. Maybe a case of premature optimization[/rant].

Looking into this more, even with GATs or without these borrow-check workarounds error handling might be difficult to do on the top-level there is this implementation of Read/AsyncRead which needs an std::io::Error. For code which knows what to expect, would dynamically casting the contained error even be possible?

Understanding what's happening

Following the chain of errors bubbling up, it all starts with reading the packet line, where the server error is treated as string:

gitoxide/git-packetline/src/read/blocking_io.rs

Lines 54 to 60 in d4c16a9

	} else if fail_on_err_lines {
	if let Some(err) = line.check_error() {
	let err = err.0.as_bstr().to_string();
	buf.clear();
	return (true, None, Some(Err(io::Error::new(io::ErrorKind::Other, err))));
	}
	}

There one could add a strongly typed error based on some well-known server messages. This should be fine as they are seemingly stable and not internationalized.

Going up a level, the actual std::io::Read impl seems to ? the underlying IO error, but would wrap the interior error if needed. This doesn't happen here though, so I'd expect a single io error with a string inside to bubble up.

gitoxide/git-packetline/src/read/sidebands/blocking_io.rs

Lines 105 to 108 in 23438fd

	let line = match self.parent.read_line() {
	Some(line) => line?.map_err(|err| io::Error::new(io::ErrorKind::Other, err))?,
	None => break (0, 0),
	};

Ultimately, this makes for a fetch::Error::Response(response::Error::Io(std::io::Error(Kind::Other, "unknown ref <ref>")))

Since server errors are certainly common, providing better support for that would be nice to allow retries or more resilient clients in general.

Possible changes

What' I could imagine doing would be to change the inner-most error contained in the std::io::Error to become fetch::Error::Response(response::Error::UploadPack(UploadPackError::UnkownRef{ref: BString})). The protocol level would then unpeel the io error, parse the string inside and try to make sense of it based on a few known errors along with a catch-all string version. That way the client side code wouldn't have to deal with the underlying io error.

Does that sound viable to you? If so quick confirmation is enough for me to implement this.

[kim]

Ah I see, thanks. That’s a bit tricky indeed - I was hoping that ERR responses would be handled one layer up. Because, technically an ERR is a valid pktline, but an error on the protocol level. If that’s not a feasible refactor at this point, maybe a stopgap would be to wrap the err string in a `struct ErrResponse(BString)`? This would disambiguate any future uses of `io::ErrorKind::Other` + stringly value, but also avoid to commit on accidentally stable error strings in git. I mean, my use of this is a bit unorthodox — I’m just short cutting ls-refs because the remote is supposed to serve a particular ref by convention. The response to an unknown want-ref is, well, “unknown ref”, which would normally be a client error. In our case, however, it means that this is a “bad” peer, and so I need to be able to distinguish this error from others. Iow, I think ERR responses are semantically equivalent to panics, and I’m matching on it on my own risk. Most likely gitoxide doesn’t want to commit on returning the exact same error strings in the same cases as `git upload-pack`, so I think just distinguishing ERR responses from decoding or IO errors is what we’d want to end up with. Wdyt?

[Byron]

Ah I see, thanks. That’s a bit tricky indeed - I was hoping that ERR responses would be handled one layer up. Because, technically an ERR is a valid pktline, but an error on the protocol level.

That's true, it's just so much easier to have the option of transforming this into an error on transport packetline level. That said…

If that’s not a feasible refactor at this point, maybe a stopgap would be to wrap the err string in a struct ErrResponse(BString)? This would disambiguate any future uses of io::ErrorKind::Other + stringly value, but also avoid to commit on accidentally stable error strings in git.

…wrapping it into a structured error and putting that into the std::io::Error would be an immediate change.

I mean, my use of this is a bit unorthodox — I’m just short cutting ls-refs because the remote is supposed to serve a particular ref by convention. The response to an unknown want-ref is, well, “unknown ref”, which would normally be a client error. In our case, however, it means that this is a “bad” peer, and so I need to be able to distinguish this error from others.

Super interesting to know :)!

Iow, I think ERR responses are semantically equivalent to panics, and I’m matching on it on my own risk. Most likely gitoxide doesn’t want to commit on returning the exact same error strings in the same cases as git upload-pack, so I think just distinguishing ERR responses from decoding or IO errors is what we’d want to end up with.

Great, that's easier to implement for sure. So the anticipated change would be something like this: fetch::Error::Response(response::Error::UploadPack(Box<dyn Error + Send + Sync>), where the box is the ErrResponse(BString). If you have any clue how to get that out of the box, I'd happily do that, too. It looks like https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_inner could be used for unboxing, but casting the error would be as good as it gets right now, turning the above into: fetch::Error::Response(response::Error::UploadPack(Box<ErrResponse>).

[Byron]

By the way, are you already dealing with error source()es? These nested errors set the source so it should be relatively straigtforward to see what's happening on application level, here is what it looks for me when reproducing the issue:

$ /Users/byron/dev/github.com/Byron/gitoxide/tests/../target/debug/gixp pack-receive -p 1 git://localhost/
Expected actual status 1 to be 0
Error: The server response could not be parsed

Caused by:
    0: Failed to read from line reader
    1: upload-pack: not our ref 0000000000000000000000000000000000000000
make[1]: *** [Makefile:182: journey-tests-async] Error 1

[kim]

By the way, are you already dealing with error source()es?

We’re using anyhow further up the stack, and often in tests, which mostly does the right thing. When the only choice is to log out an error, we’re using the Debug impl — that’s a bit suboptimal: at low verbosity, one would want to simply concatenate the Display strings along the chain, while at debug level the actual Debug is more useful. Not sure how to deal with this. The tracing crate has a Value trait, but that is sealed for object safety reasons. They had announced a separate crate to deal with this problem, but I’m not sure if it’s stable yet.

[kim]

worksforme

Thanks! 🙏

[Byron]

We’re using anyhow further up the stack, and often in tests, which mostly does the right thing. When the only choice is to log out an error, we’re using the Debug impl — that’s a bit suboptimal: at low verbosity, one would want to simply concatenate the Display strings along the chain, while at debug level the actual Debug is more useful.

It would indeed be nice to have a new formatter or utility to automatically walks the source chain of errors when generating a display string. Thanks to my none-existing experience with tracing I wouldn't be able to help, but would also hope that there is a way to present the entire error chain. It's in their nature that the leaf of the chain is the most informative, whereas the one we actually get to see is just 'meh'.

worksforme

Neat! Please let me know if you need a release right away, and if that should be only git-protocol or if git-repository would be better.