From 0e5c0726c3cec010bcdc2fafe431cdf9f1791242 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 31 Jul 2026 00:43:16 -0700 Subject: [PATCH 1/3] fix(sync): stop the engine inheriting descriptors it never uses 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. --- scripts/remote-sync.sh | 33 +++++++++++++ tests/test_engine_inherited_fds.bats | 74 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 tests/test_engine_inherited_fds.bats diff --git a/scripts/remote-sync.sh b/scripts/remote-sync.sh index c7420788..d377cccc 100755 --- a/scripts/remote-sync.sh +++ b/scripts/remote-sync.sh @@ -15,4 +15,37 @@ export AGMSG_SYNC_DRIVER="$SCRIPT_DIR/internal/storage-sync-driver.sh" NODE_BIN="$(agmsg_resolve_node)" export AGMSG_SYNC_NODE_BIN="$NODE_BIN" export AGMSG_SYNC_CIPHER_HELPER="$SCRIPT_DIR/internal/sync-cipher.mjs" +# The engine outlives the command that starts it, so whatever it inherits it +# holds for as long as it runs. Under bats that included fd 144 -- a descriptor +# internal to the harness -- and the shard then ran to the CI job's cap with +# every test already reported ok, because bats was waiting for an EOF the engine +# was keeping from arriving. Captured directly: the engine and three bats +# processes all held the same pipe, 0xc9aea28a590ca110, at fd 144. +# +# Closing 3 and 4 by name at the spawn sites, which is what this repo did until +# now, cannot reach a descriptor whose number the harness chooses. So the close +# is by range, not by name, and it lives here -- the one place every engine +# invocation passes through -- rather than at each caller. +# +# The engine speaks only over stdin, stdout and stderr; the spawn sites already +# point those at a log. Nothing above stderr is anything it should keep. +_close_inherited_fds() { + local fd + if [ -d /dev/fd ]; then + for fd in /dev/fd/*; do + fd="${fd##*/}" + case "$fd" in ''|*[!0-9]*) continue ;; esac + [ "$fd" -gt 2 ] || continue + eval "exec ${fd}>&-" 2>/dev/null || true + done + else + # No /dev/fd to enumerate: sweep a range instead. Bounded rather than exact, + # which is why it is the fallback and not the method. + for fd in $(seq 3 255); do + eval "exec ${fd}>&-" 2>/dev/null || true + done + fi +} +_close_inherited_fds + exec "$NODE_BIN" "$SCRIPT_DIR/internal/remote-sync.mjs" "$@" diff --git a/tests/test_engine_inherited_fds.bats b/tests/test_engine_inherited_fds.bats new file mode 100644 index 00000000..1b9cc1af --- /dev/null +++ b/tests/test_engine_inherited_fds.bats @@ -0,0 +1,74 @@ +#!/usr/bin/env bats + +# The sync engine outlives the command that starts it, so every descriptor it +# inherits it holds for as long as it runs. Under bats that included fd 144 -- +# a descriptor internal to the harness -- and the shard then ran to the CI job's +# cap with every test already reported ok, because bats was waiting for an EOF +# the engine was keeping from arriving. +# +# Captured from a hung macOS shard: the engine and three bats processes all held +# the same pipe, 0xc9aea28a590ca110, at fd 144. +# +# node .../remote-sync.mjs fd 144 PIPE 0xc9aea28a590ca110 (ppid 1) +# bats ... fd 144 PIPE 0xc9aea28a590ca110 +# bats ... fd 144 PIPE 0xc9aea28a590ca110 +# bats-format-cat fd 144 PIPE 0xc9aea28a590ca110 + +setup() { + SCRIPTS="$BATS_TEST_DIRNAME/../scripts" + WORK="$(mktemp -d)" + FD_REPORT="$WORK/fds.txt" + export FD_REPORT + # Stands in for node. It reports its descriptors to a file, because command + # substitution rearranges descriptors itself and that is the thing being + # measured, and it also speaks over all three standard streams so a close that + # went too far shows up as silence rather than as a passing test. + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'ls /dev/fd > "$FD_REPORT" 2>/dev/null' + printf '%s\n' 'echo MARKER-OUT' + printf '%s\n' 'echo MARKER-ERR >&2' + printf '%s\n' 'cat > "$FD_REPORT.stdin"' + } > "$WORK/fake-node" + chmod +x "$WORK/fake-node" + export AGMSG_NODE="$WORK/fake-node" + export AGMSG_STORAGE_PATH="$WORK/store" + mkdir -p "$AGMSG_STORAGE_PATH" +} + +teardown() { + [ -n "${WORK:-}" ] && rm -rf "$WORK" +} + +@test "the engine does not inherit descriptors the harness opened" { + # 144 is the number seen in the captured hang; 77 is arbitrary, so passing + # cannot come from something particular about 144. A closed descriptor is not + # reissued at its old number -- reuse takes the lowest free one -- so absence + # here is meaningful. + bash "$SCRIPTS/remote-sync.sh" probe 144>/dev/null 77>/dev/null \ + /dev/null 2>/dev/null || true + + # That the stand-in ran at all: with an empty report every "absent" assertion + # below would hold for the wrong reason. + [ -s "$FD_REPORT" ] + + run grep -qx 144 "$FD_REPORT" + [ "$status" -ne 0 ] + run grep -qx 77 "$FD_REPORT" + [ "$status" -ne 0 ] +} + +@test "the three standard streams still reach the engine" { + # The close is by range above stderr, and this is what stops that range from + # creeping downwards. Asserted by using the streams, not by listing them: a + # closed 0/1/2 is immediately reissued to whatever opens next, so a listing + # shows all three either way and cannot tell the two cases apart. An earlier + # revision of this file asserted exactly that and passed while the code + # closed all three. + printf 'ping\n' | bash "$SCRIPTS/remote-sync.sh" probe \ + > "$WORK/out.txt" 2> "$WORK/err.txt" || true + + grep -qx MARKER-OUT "$WORK/out.txt" + grep -qx MARKER-ERR "$WORK/err.txt" + grep -qx ping "$FD_REPORT.stdin" +} From 332c9043153cdc97dbe3ed06bac6dfb7a9cfa080 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 31 Jul 2026 18:12:19 -0700 Subject: [PATCH 2/3] fix(sync): close the inherited descriptors as part of the exec 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. --- scripts/remote-sync.sh | 43 +++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/scripts/remote-sync.sh b/scripts/remote-sync.sh index d377cccc..2d5b8a7b 100755 --- a/scripts/remote-sync.sh +++ b/scripts/remote-sync.sh @@ -29,23 +29,28 @@ export AGMSG_SYNC_CIPHER_HELPER="$SCRIPT_DIR/internal/sync-cipher.mjs" # # The engine speaks only over stdin, stdout and stderr; the spawn sites already # point those at a log. Nothing above stderr is anything it should keep. -_close_inherited_fds() { - local fd - if [ -d /dev/fd ]; then - for fd in /dev/fd/*; do - fd="${fd##*/}" - case "$fd" in ''|*[!0-9]*) continue ;; esac - [ "$fd" -gt 2 ] || continue - eval "exec ${fd}>&-" 2>/dev/null || true - done - else - # No /dev/fd to enumerate: sweep a range instead. Bounded rather than exact, - # which is why it is the fallback and not the method. - for fd in $(seq 3 255); do - eval "exec ${fd}>&-" 2>/dev/null || true - done - fi -} -_close_inherited_fds +# +# The closes are attached to the exec rather than performed before it. One of the +# descriptors above stderr belongs to the shell itself -- bash keeps the script it +# is reading open, at 255 -- and closing it from a statement would leave the shell +# to carry on reading a script through a descriptor it no longer holds. Bash does +# defend against that (observed: it moves the script to another number), but as a +# redirection on the exec the question does not arise: nothing of this script runs +# between the close and the execve. +_fd_closes="" +if [ -d /dev/fd ]; then + for _fd in /dev/fd/*; do + _fd="${_fd##*/}" + case "$_fd" in ''|*[!0-9]*) continue ;; esac + [ "$_fd" -gt 2 ] || continue + _fd_closes="$_fd_closes ${_fd}>&-" + done +else + # Nothing to enumerate, so name a range instead. Closing a descriptor that was + # never open is not an error, which is what makes the blind form safe. + for _fd in $(seq 3 255); do + _fd_closes="$_fd_closes ${_fd}>&-" + done +fi -exec "$NODE_BIN" "$SCRIPT_DIR/internal/remote-sync.mjs" "$@" +eval "exec$_fd_closes \"\$NODE_BIN\" \"\$SCRIPT_DIR/internal/remote-sync.mjs\" \"\$@\"" From 06e939492e3aa2640e756e12089b535666de411d Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 1 Aug 2026 01:02:17 -0700 Subject: [PATCH 3/3] fix(sync): close each descriptor as its own statement again 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. --- scripts/remote-sync.sh | 62 +++++++++++++++--------- tests/test_engine_inherited_fds.bats | 70 ++++++++++++++++++++++------ 2 files changed, 96 insertions(+), 36 deletions(-) diff --git a/scripts/remote-sync.sh b/scripts/remote-sync.sh index 2d5b8a7b..c2bb52c5 100755 --- a/scripts/remote-sync.sh +++ b/scripts/remote-sync.sh @@ -30,27 +30,43 @@ export AGMSG_SYNC_CIPHER_HELPER="$SCRIPT_DIR/internal/sync-cipher.mjs" # The engine speaks only over stdin, stdout and stderr; the spawn sites already # point those at a log. Nothing above stderr is anything it should keep. # -# The closes are attached to the exec rather than performed before it. One of the -# descriptors above stderr belongs to the shell itself -- bash keeps the script it -# is reading open, at 255 -- and closing it from a statement would leave the shell -# to carry on reading a script through a descriptor it no longer holds. Bash does -# defend against that (observed: it moves the script to another number), but as a -# redirection on the exec the question does not arise: nothing of this script runs -# between the close and the execve. -_fd_closes="" -if [ -d /dev/fd ]; then - for _fd in /dev/fd/*; do - _fd="${_fd##*/}" - case "$_fd" in ''|*[!0-9]*) continue ;; esac - [ "$_fd" -gt 2 ] || continue - _fd_closes="$_fd_closes ${_fd}>&-" - done -else - # Nothing to enumerate, so name a range instead. Closing a descriptor that was - # never open is not an error, which is what makes the blind form safe. - for _fd in $(seq 3 255); do - _fd_closes="$_fd_closes ${_fd}>&-" - done -fi +# Each close is its own statement. Collecting them into redirections on the exec +# reads better and is wrong: bash 3.2 -- which is /bin/bash on macOS, and what +# runs this on the macOS runner -- relocates descriptors into the range at and +# above 10 while it processes an exec's redirections, and the child then inherits +# those relocated copies. Measured, same script, same enumeration +# (`10>&- 143>&- 255>&- 3>&-`) both times: +# +# 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 +# +# A CI shard hung on exactly that: the engine held bats's pipe at fd 13 while +# bats held it at 143, and bats sat waiting for an EOF that could not arrive. +# +# The hazard that argued for the exec form does not bite. One descriptor above +# stderr belongs to the shell -- bash keeps the script it is reading open, at 255 +# -- and closing it from a statement might have left the shell reading through a +# descriptor it had given up. Bash defends itself: given `exec 255>&-` it moves +# the script to another number (observed as 11), and a script padded to 291 KB, +# far past any read buffer, still ran to its last line under both 3.2.57 and +# 5.3.15. Do not "simplify" this back onto the exec line. +_close_inherited_fds() { + local fd + if [ -d /dev/fd ]; then + for fd in /dev/fd/*; do + fd="${fd##*/}" + case "$fd" in ''|*[!0-9]*) continue ;; esac + [ "$fd" -gt 2 ] || continue + eval "exec ${fd}>&-" 2>/dev/null || true + done + else + # Nothing to enumerate, so sweep a range instead. Closing a descriptor that + # was never open is not an error, which is what makes the blind form safe. + for fd in $(seq 3 255); do + eval "exec ${fd}>&-" 2>/dev/null || true + done + fi +} +_close_inherited_fds -eval "exec$_fd_closes \"\$NODE_BIN\" \"\$SCRIPT_DIR/internal/remote-sync.mjs\" \"\$@\"" +exec "$NODE_BIN" "$SCRIPT_DIR/internal/remote-sync.mjs" "$@" diff --git a/tests/test_engine_inherited_fds.bats b/tests/test_engine_inherited_fds.bats index 1b9cc1af..bf39999e 100644 --- a/tests/test_engine_inherited_fds.bats +++ b/tests/test_engine_inherited_fds.bats @@ -40,22 +40,66 @@ teardown() { [ -n "${WORK:-}" ] && rm -rf "$WORK" } +# Reports its OWN descriptors above stderr. A stand-in that shells out to `ls` +# would add that child's directory descriptor to the count; this one adds +# nothing the baseline below does not also see. +_write_self_reporter() { + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' ': > "$FD_REPORT"' + printf '%s\n' 'for f in /dev/fd/*; do' + printf '%s\n' ' n="${f##*/}"' + printf '%s\n' ' case "$n" in ""|*[!0-9]*) continue ;; esac' + printf '%s\n' ' [ "$n" -gt 2 ] && printf "%s " "$n" >> "$FD_REPORT"' + printf '%s\n' 'done' + } > "$1" + chmod +x "$1" +} + @test "the engine does not inherit descriptors the harness opened" { - # 144 is the number seen in the captured hang; 77 is arbitrary, so passing - # cannot come from something particular about 144. A closed descriptor is not - # reissued at its old number -- reuse takes the lowest free one -- so absence - # here is meaningful. - bash "$SCRIPTS/remote-sync.sh" probe 144>/dev/null 77>/dev/null \ - /dev/null 2>/dev/null || true + # Run under EVERY bash on this machine, because they disagree. bash 3.2 -- + # /bin/bash on macOS, and what runs this on the macOS runner -- relocates + # descriptors into the range at and above 10 while processing an exec's + # redirections, so a close 5.x honours can come back to the child at a new + # number. A single-shell version of this test stayed green through exactly + # that regression while a CI shard hung. + # + # Compared against a BASELINE rather than against named numbers: what 3.2 + # leaves behind is renumbered, so asserting "144 and 77 are absent" cannot + # see it. The baseline is the stand-in's own descriptors, measured with + # nothing inherited to lose; anything beyond that came through the close. + local reporter="$WORK/self-report" + _write_self_reporter "$reporter" + export AGMSG_NODE="$reporter" + + local sh found=0 + for sh in /bin/bash /usr/local/bin/bash /opt/homebrew/bin/bash "$(command -v bash)"; do + [ -x "$sh" ] || continue + found=$((found + 1)) + + : > "$FD_REPORT" + "$sh" "$SCRIPTS/remote-sync.sh" probe /dev/null 2>/dev/null || true + local baseline; baseline="$(cat "$FD_REPORT")" - # That the stand-in ran at all: with an empty report every "absent" assertion - # below would hold for the wrong reason. - [ -s "$FD_REPORT" ] + # Pipes, not files: the relocation only shows up on pipes, which is what a + # harness holds. 143 is the shape seen in the captured hang; 10 is low and + # arbitrary, so a pass cannot come from something particular about 143. + : > "$FD_REPORT" + ( + exec 143> >(sleep 20) + exec 10> >(sleep 20) + "$sh" "$SCRIPTS/remote-sync.sh" probe /dev/null 2>/dev/null || true + ) + local measured; measured="$(cat "$FD_REPORT")" - run grep -qx 144 "$FD_REPORT" - [ "$status" -ne 0 ] - run grep -qx 77 "$FD_REPORT" - [ "$status" -ne 0 ] + [ -n "$baseline$measured" ] || { echo "$sh: the stand-in never ran"; false; } + [ "$baseline" = "$measured" ] || { + echo "$sh ($("$sh" --version 2>/dev/null | head -1))" + echo " baseline [$baseline] measured [$measured]" + false + } + done + [ "$found" -gt 0 ] } @test "the three standard streams still reach the engine" {