smart: ignore shallow/unshallow packets during ACK processing #6973
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In RPC mode (https), the client sends its list of shallow commits at the beginning of each request during packfile negotiation, so that the remote endpoint keeps context. This causes the remote to prepend appropriate shallow/unshallow packets to each response sent back to the client. However, the store_common() helper function (used in multi_ack mode) does not cater for this, returning as soon as it encounters any packet different than an ACK packet and therefore leaving the rest of the HTTP buffer unprocessed. This in turn causes subsequent iterations of the while loop processing ACK packets to process data returned by older HTTP requests instead of the current one, messing up the packfile negotiation process. Given that the wait_while_ack() helper function (called after the client signals to the remote that it is ready to receive packfile data) correctly skips over shallow/unshallow packets, packfile contents can still be received successfully in some cases (depending on message framing); in some other ones, though (particularly when git_smart__download_pack() processes an HTTP buffer starting with shallow/unshallow packets), the fetching process fails with an "incomplete pack header" error due to the flush packet terminating a set of shallow/unshallow packets being incorrectly interpreted as the flush packet indicating the end of the packfile (making the code behave as if no packfile data was sent by the remote). Fix by ignoring shallow/unshallow packets in the store_common() helper function, therefore making the ACK processing logic work on the correct HTTP buffers and ensuring that git_smart__download_pack() is not called until packfile negotiation is actually finished.
|
Wow.
What an understatement. Thanks for the deep investigation and the fix. |
|
I had another read through - this makes sense, and I really appreciate the detailed explanation that you gave in the PR. 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In RPC mode (https), the client sends its list of shallow commits at the beginning of each request during packfile negotiation, so that the remote endpoint keeps context. This causes the remote to prepend appropriate shallow/unshallow packets to each response sent back to the client.
However, the store_common() helper function (used in multi_ack mode) does not cater for this, returning as soon as it encounters any packet different than an ACK packet and therefore leaving the rest of the HTTP buffer unprocessed. This in turn causes subsequent iterations of the while loop processing ACK packets to process data returned by older HTTP requests instead of the current one, messing up the packfile negotiation process. Given that the wait_while_ack() helper function (called after the client signals to the remote that it is ready to receive packfile data) correctly skips over shallow/unshallow packets, packfile contents can still be received successfully in some cases (depending on message framing); in some other ones, though (particularly when git_smart__download_pack() processes an HTTP buffer starting with shallow/unshallow packets), the fetching process fails with an "incomplete pack header" error due to the flush packet terminating a set of shallow/unshallow packets being incorrectly interpreted as the flush packet indicating the end of the packfile (making the code behave as if no packfile data was sent by the remote).
Fix by ignoring shallow/unshallow packets in the store_common() helper function, therefore making the ACK processing logic work on the correct HTTP buffers and ensuring that git_smart__download_pack() is not called until packfile negotiation is actually finished.
This bug was fairly nasty to pinpoint, so it was also challenging to come up with a test triggering it, particularly given that the test repository used in
tests/libgit2/online/shallow.conly contains 21 commits, i.e. barely enough to exceed the 20-packethaveline split point. In fact, the test included in this PR only demonstrates a case in which the code is able to (mostly accidentally) "resync" the packfile negotiation process and fetch packfile data after all:To make this test trigger the
incomplete pack headererror using the test repository, an extra modification lowering the maximum count ofhavelines per request must be applied (which does not affect the way the ACK processing logic works on a higher level), e.g.:With the above modification in place, the test fails:
Adding HTTP tracing to the test code hopefully further clarifies what happens behind the scenes, e.g.:
With these two modifications applied (
i % 5and tracing), the test triggers the following behavior without the fix proposed in this PR:Note in particular how the code does not read new data from the wire between some of the later POST requests - that's the "out of sync" HTTP buffer processing in action.
With the fix proposed in this PR applied, the above becomes:
Looking at the code, I believe a similar issue affects non-multi_ack packfile negotiation (I would expect the
unexpected pkt typeerror to be raised), but I did not have the heart to test and/or fix against a server that does support shallow clones while not supporting multi_ack mode...