Stop the sync engine inheriting descriptors it never uses - #575
Merged
Conversation
A hung macOS shard was captured mid-hang: the engine and three bats processes all held the same pipe, 0xc9aea28a590ca110, at fd 144. bats was alive and waiting for an EOF the engine was keeping from arriving, and the job ran to its 25-minute cap with every test already reported ok. The engine had been reparented to init, so no teardown could reach it, and the number was 144 -- which is why closing 3 and 4 by name at the spawn sites never helped. That norm, made uniform only yesterday, is hereby superseded: the close is by range and lives at the engine's own entry, so it covers every caller rather than each one separately. Two tests, each failing for a different mistake: dropping the close lets 144 through, and letting the range reach stdin/stdout/stderr silences the engine. The second is asserted by using the streams, not by listing them -- a closed 0/1/2 is reissued to whatever opens next, so a listing cannot tell the two apart, and an earlier draft of the test passed while the code closed all three.
One of the descriptors above stderr belongs to the shell itself: bash holds the script it is reading at 255, and the enumeration could not tell that one from a descriptor bats had left open. Closing it from a statement left the shell to read the rest of its own script through a descriptor it had just given up. Bash turns out to defend against exactly this -- given `exec 255>&-` it moves the script to another number, observed here as 11 under both 3.2.57 and 5.3.15, and a script padded well past any read buffer still ran to its last line. So the hazard did not bite. Attaching the closes to the exec retires the question rather than resting on that defence: the redirections are applied and the execve follows with no statement of this script in between. The two tests are unchanged and still fail one apiece -- dropping the closes lets 144 through, widening the range past stderr silences the engine.
Moving the closes onto the exec line was a regression, and CI said so: the head before it passed every macOS shard, the head after it hung 4/4 twice. The dump from the hang has the engine holding bats's pipe at fd 13 while bats holds it at 143. bash 3.2 -- /bin/bash on macOS, and what starts the engine on the macOS runner -- relocates descriptors into the range at and above 10 while it processes an exec's redirections, and the child inherits the relocated copies. Same script, same enumeration (`10>&- 143>&- 255>&- 3>&-`): bash 5.3.15 engine sees 0 1 2 bash 3.2.57 engine sees 0 1 2 10 11 <- 143 came back as 11 The hazard that argued for the exec form does not bite: given `exec 255>&-` bash moves the script it is reading to another number, and a script padded past any read buffer still runs to its last line on both. That was measured before and is recorded next to the code. The regression test now runs under every bash present and compares against a baseline instead of naming numbers -- what 3.2 leaves behind is renumbered, so "144 is absent" cannot see it. One shell and named numbers is what let this through.
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
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.
Shards have been reporting every test
okand then sitting silent until the job's 25-minute cap. Three explanations were tried and each was refuted by observation. This one was not guessed — it was photographed.What was holding the job open
From
#566's diagnostics, taken while a macOS shard was hung, after 262/262ok:A surviving sync engine holding the same pipe as bats, at the same descriptor. bats had not exited — all three of its processes were alive, waiting for an EOF that could not arrive while the engine held the write end.
Two things follow, and they explain the two fixes that did not work:
ppid 1. The engine had already been reparented, so no teardown reaching for a pidfile could have caught it. That is why#559's teardown fix did not stop this.fd 144. Not 3, not 4. The engine's spawn site already closed 3 and 4 by name. A descriptor whose number the harness picks cannot be closed by naming it, which is why yesterday's uniform3>&- 4>&-rule did not stop this either.The change
Close everything above stderr at the engine's own entry, by range rather than by name, enumerated from
/dev/fdwhere it exists and swept otherwise. It lives at the entry, not at the spawn sites, so it covers every way the engine is started rather than each one separately.The engine speaks only over stdin, stdout and stderr, and the spawn sites already point those at a log.
The closes are attached to the
execrather than performed before it:One descriptor above stderr belongs to the shell — bash holds the script it is reading at 255, and an enumeration cannot tell that one from a descriptor bats left open. Closing it from a statement would leave the shell reading the rest of its own script through a descriptor it had just given up. As a redirection on the
execthe question does not arise: the redirections are applied and theexecvefollows, with no statement of this script in between.Bash does defend against the statement form, as it turns out. Given
exec 255>&-it moves the script to another number — observed as 11 under both 3.2.57 (macOS/bin/bash) and 5.3.15 — and a script padded to 291 KB, far past any read buffer, still ran to its last line under both. The hazard was right in shape and did not bite. Attaching the closes to theexecretires it rather than resting on that defence.That is also why no test is added for it. A regression exercising a long script passes against the statement form too, so it could only ever be green; the measurement above is the evidence instead.
This supersedes the norm added in
#565. That change made3>&- 4>&-uniform acrossscripts/and enforced it with a test; the norm was right in shape and too narrow in reach. The history is recorded in the commit body so it lives in one place.Tests
Two, and each fails for a different mistake:
Test 2 asserts by using the streams — a marker on stdout, one on stderr, a line through stdin — rather than listing them. A closed 0/1/2 is immediately reissued to whatever opens next, so a listing shows all three either way. An earlier draft asserted exactly that and passed while the code closed all three; that draft is why the mutation table above exists.
Locally: the descriptors reaching the engine are
0 1 2with this change and0 1 144 2 3 4 77without it.What this does not establish
That it ends the hangs. The mechanism is captured and the fix demonstrably closes it, but whether these shards stop hanging is three consecutive clean runs of the affected shard, not this description.
Unrelated, found on the way
connect: a keyed team fails closed when the remote disallows age-v1intest_remote.batsfails onintegration/remoteitself, with or without this change — but only whereageis installed. CI skips it (# skip age/age-keygen not installed), so CI has never run it. Reported separately; not touched here.