Re: Is t5516 somehow flakey only on macOS?
- From
Jeff King <peff@peff.net>
- Date
- Jan 9, 2021, 10:34 UTC
- Message-ID
- <X/mGnY3wR1fGoxcf@coredump.intra.peff.net>
- In-Reply-To
- <CAPig+cQ3U4s0qmzoLL=ZBeSyCZm=QqjWz2P36ZUxMzNAyYn-WQ@mail.gmail.com>
On Sat, Jan 09, 2021 at 04:11:09AM -0500, Eric Sunshine wrote:
Show 12 quoted lines
> In a successful run, the content of `err` is: > > fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886 > fatal: remote error: upload-pack: not our ref > 64ea4c133d59fa98e86a771eda009872d6ab2886 > > However, in the unsuccessful run from CI, it is: > > fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886 > fatal: unable to write to remote: Broken pipe > > Perhaps this means something to someone familiar with this area of the code.
This is the same issue discussed in this sub-thread:
https://lore.kernel.org/git/20190828153944.GA29715@sigill.intra.peff.net/
It's not macOS-specific, but for whatever reason it's much easier to trigger there. There's a reliable reproduction with a sleep() in:
https://lore.kernel.org/git/20190829220630.GH8571@szeder.dev/
The solution is something like the "after a write() error, pump the read channel to see if we got an ERR packet" patch Gábor showed in the reply to that message.
But an interesting thing to note: this particular race only happens with protocol v0 (and the test is forcing v0, because v2 would not produce a "not our ref" error in the first place). In general I'd suspect that v2 is a bit less susceptible to these types of races, because it has more of a request/response flow. But it's still possible (e.g., if the server side ever aborts before reading the whole request).
For this _particular_ test, since we know that it is testing a v0-only behavior, we might want to just loosen the test. This goes against the point of adding it in 014ade7484 (upload-pack: send ERR packet for non-tip objects, 2019-04-13), but it's the best we can do for now. Something like this:
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 01004ff680..8726d80250 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1260,7 +1260,20 @@ do git cat-file commit $SHA1_2 && test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \ git fetch ../testrepo/.git $SHA1_3 2>err && - test_i18ngrep "remote error:.*not our ref.*$SHA1_3\$" err + # ideally we would insist this be on a "remote error:" + # line to make sure an ERR packet was sent. But + # checking that is racy, because the client may see the + # pipe close while they are still writing their "done" + # line. So check only that it appears in stderr at all, + # which will generally pick up the copy that + # upload-pack sends to stderr. This is a hack, because + # in the real world the upload-pack stderr would often + # not be shown to the client at all (e.g., over git:// + # or over an ssh channel to a non-vanilla server). So + # this is papering over a real racy bug that users + # might see in the wild, but this is the best we can do + # for now to make our tests robust. + test_i18ngrep "not our ref.*$SHA1_3\$" err ) ' done -Peff