diff --git a/tests/test_helper.bash b/tests/test_helper.bash index 1e80586dd..f7e828f2b 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -34,7 +34,95 @@ setup_test_env() { mkdir -p "$HOME" } +# PIDs (one per line, this shell excluded) whose command line references . +# The detached codex children — codex-bridge-launcher.sh and the codex-bridge.js it +# starts (codex-monitor.sh spawns the launcher with `… &`, "outlives this script") — +# resolve their SKILL_DIR from their own script path, so their argv carries +# TEST_SKILL_DIR. The launcher records no pidfile of its own, so a pidfile sweep cannot +# reach it; the command line is what names it. Unix uses ps; on Git Bash ps enumerates +# MSYS processes, which the launcher/bridge are, so it reaches them there too. +# +# LIMIT (named deliberately, not a defect): this matches only processes that carry +# $dir IN THEIR ARGV. A process whose CWD is inside $dir but whose argv does not name +# it would NOT be found. The two known holders are argv-visible today — the launcher +# resolves SKILL_DIR from its own script path (argv[0]), and the bridge receives +# --workspace-root — so they are caught; but that is a property of THOSE two, not +# a guarantee about any future holder. A cwd/open-fd sweep (lsof) would close the gap; +# it is deliberately NOT used because lsof is slow and this runs in EVERY test's +# teardown — too heavy for the ~all tests that hold nothing. If a future detached child +# holds $dir without naming it in argv, revisit (add an lsof pass gated on the rm +# actually failing, so the cost is paid only when it is needed). +_pids_referencing_dir() { # + ps -eo pid=,args= 2>/dev/null | awk -v d="$1" -v me="$$" 'index($0, d) { if ($1+0 != me+0) print $1 }' +} + +# Reap any process still holding $TEST_SKILL_DIR, then let handles release, BEFORE the +# rm. Those detached children keep writing $TEST_SKILL_DIR/run after the test body +# returns and are in no pidset the tests kill, so the bare rm below races them and fails +# `rm: Directory not empty` (or, on Windows, `Device or resource busy` on the bridge's +# open messages.db). #662 == #1036 == #1049. +# +# Scope is $TEST_SKILL_DIR ITSELF — a unique mktemp path — so matching it in process +# args cannot reach a developer's live bridge or another test's processes; this is never +# a blanket `pkill codex-bridge.js`. Guarded to a temp path so a mis-set variable can +# never turn the scan loose on a short/rooty prefix. A single `ps` for the ~all tests +# that spawn nothing. +# +# The SIGTERM→wait→SIGKILL sequence is EXERCISED by tests/test_teardown_reap.bats (kill, +# scope-safety, no-op, guard); whether the wait budget is long enough on a load-3-digit +# host, and whether killing a holder RELEASES the Windows file handle before the rm, are +# both timing/OS facts this repo cannot measure on the author's loaded machine — CI +# (dedicated runners, Windows leg) measures them. Written as designed-and-static-checked, +# NOT as "measured", per the day's rule that a claim states how it was verified (#1036). +_reap_test_skill_dir_procs() { + local dir="${TEST_SKILL_DIR:-}" + case "$dir" in + ""|/|/tmp|/var|/private|/usr|"$HOME") return 0 ;; + esac + case "$dir" in + /tmp/*|/private/*|/var/folders/*|/private/var/folders/*) : ;; + *) + # Outside the well-known temp roots, allow ONLY under a TMPDIR that is set AND a + # real path — never unset, "", or "/". Resolve and VALIDATE the prefix before using + # it as a pattern: a pattern assembled from an empty prefix ("${TMPDIR:+…}" with + # TMPDIR unset, or "${TMPDIR%/}" with TMPDIR="/") degenerates to match ANY non-empty + # dir. This guards a KILL, so the loose failure kills EXTRA processes, not nothing + # (co2 BLOCKING). Strip the trailing slash first, then require the result non-empty, + # so unset / "" / "/" all fail closed. Only then is "$_tmp" safe as a pattern prefix. + local _tmp="${TMPDIR:-}"; _tmp="${_tmp%/}" + [ -n "$_tmp" ] || return 0 + case "$dir" in "$_tmp"/?*) : ;; *) return 0 ;; esac + ;; + esac + local pids tries=0 sig p + while :; do + pids="$(_pids_referencing_dir "$dir")" + [ -n "$pids" ] || return 0 + # Escalate to SIGKILL quickly (after ~0.3s of SIGTERM): a detached launcher may not + # act on SIGTERM, and this is a teardown, not a graceful shutdown. SIGKILL is + # uncatchable, so once sent the process WILL die — the only remaining wait is for ps + # to stop listing it, which a heavily loaded host can slow. So keep re-checking up + # to ~6s (a bound only ever reached when something is genuinely stuck; the ~all tests + # that hold nothing return on the first check above), then return and let the rm + # surface anything still there. The 6s headroom is what covers a load-3-digit host. + sig=TERM; [ "$tries" -ge 3 ] && sig=KILL + for p in $pids; do kill "-$sig" "$p" 2>/dev/null || true; done + [ "$tries" -ge 60 ] && return 0 + sleep 0.1 2>/dev/null || true + tries=$((tries + 1)) + done +} + teardown_test_env() { + # Try the plain rm FIRST, and only reap when it actually fails. The reaper's scan is a + # full `ps -eo pid=,args=`; running it in EVERY teardown would add that cost to all of + # the (vast majority of) tests that hold nothing — across the suite's hundreds of tests + # that dominates the runtime and pushes CI shards over their timeout. The race it fixes + # is rare (only the codex tests spawn the detached launcher), and it announces itself + # as a non-zero rm ("Directory not empty" / "Device or resource busy"), so pay the cost + # exactly there: on failure, reap the TEST_SKILL_DIR-scoped holders and retry. + rm -rf "$TEST_SKILL_DIR" 2>/dev/null && return 0 + _reap_test_skill_dir_procs rm -rf "$TEST_SKILL_DIR" } diff --git a/tests/test_teardown_reap.bats b/tests/test_teardown_reap.bats new file mode 100644 index 000000000..3090ea24e --- /dev/null +++ b/tests/test_teardown_reap.bats @@ -0,0 +1,98 @@ +#!/usr/bin/env bats +# +# teardown_test_env must kill a detached process still holding $TEST_SKILL_DIR before +# it removes the tree. The detached codex children (codex-bridge-launcher.sh and the +# codex-bridge.js it starts) outlive the test body and keep writing $TEST_SKILL_DIR/run, +# so the bare rm races them and fails "Directory not empty" (#662 == #1036 == #1049). +# The reaper is scoped to the unique mktemp $TEST_SKILL_DIR so it can never reach a real +# bridge or another test — that scope safety is asserted here, not just the kill. +# +# Assertions use plain commands / `refute`, never a non-last `[[ ]]` or `! cmd` (#670). + +load test_helper + +setup() { setup_test_env; } +teardown() { teardown_test_env; } + +# Spawn a detached holder whose argv carries $TEST_SKILL_DIR (as the launcher's does), +# keeping run/ busy. Echoes its pid once it is actually writing run/. +_start_holder() { + mkdir -p "$TEST_SKILL_DIR/run" + local holder="$TEST_SKILL_DIR/scripts/holder.sh" + cat > "$holder" <<'H' +#!/usr/bin/env bash +d="$1"; while :; do : > "$d/run/held.$$"; sleep 0.05; done +H + chmod +x "$holder" + # ORPHAN it in a subshell (the subshell exits, the holder reparents to init) so it + # matches the real detached launcher — and so that after the reaper's SIGKILL it is + # reaped by init rather than lingering as a zombie of this test shell, which would + # still answer `kill -0` and defeat the "it is dead" assertion. It records its own pid. + ( "$holder" "$TEST_SKILL_DIR" & printf '%s\n' "$!" > "$TEST_SKILL_DIR/holder.pid" ) + local hp n=0; hp="$(cat "$TEST_SKILL_DIR/holder.pid" 2>/dev/null)" + while [ ! -e "$TEST_SKILL_DIR/run/held.$hp" ] && [ "$n" -lt 100 ]; do + sleep 0.05; n=$((n + 1)) + done + printf '%s\n' "$hp" +} + +@test "teardown reaper kills a detached process holding TEST_SKILL_DIR (#662)" { + local hp; hp="$(_start_holder)" + kill -0 "$hp" # holder is alive and holding run/ + _reap_test_skill_dir_procs # THE FIX — a no-op reaper leaves it alive (mutation) + refute kill -0 "$hp" # reaper killed it, so run/ is free for the rm +} + +@test "teardown reaper leaves a process that does NOT reference TEST_SKILL_DIR alone (scope safety)" { + # The reaper must never reach a developer's live bridge or another test's process. + # A plain `sleep` whose argv does not contain this test's mktemp dir must survive. + sleep 30 & + local other=$! + _reap_test_skill_dir_procs + kill -0 "$other" # untouched + kill "$other" 2>/dev/null || true +} + +@test "teardown reaper is a no-op when nothing holds TEST_SKILL_DIR" { + run _reap_test_skill_dir_procs + [ "$status" -eq 0 ] +} + +@test "teardown reaper refuses to scan when TEST_SKILL_DIR is not a temp path (guard)" { + # A mis-set TEST_SKILL_DIR must never turn the scan loose on a short/rooty prefix + # (which would match — and kill — nearly every process). + sleep 30 & + local other=$! + TEST_SKILL_DIR="/" _reap_test_skill_dir_procs + TEST_SKILL_DIR="/usr" _reap_test_skill_dir_procs + kill -0 "$other" # nothing was scanned or killed for a rooty dir + kill "$other" 2>/dev/null || true +} + +@test "teardown reaper fails closed on a NON-temp TEST_SKILL_DIR for a degenerate TMPDIR: unset / empty / root (#662, co2 BLOCKING)" { + # utildev measured that ubuntu-latest's `mktemp -d` uses /tmp with TMPDIR UNSET, so a + # guard pattern assembled from $TMPDIR degenerates to ?* there and would turn this KILL + # loose on CI Linux. Prove fail-closed: with a NON-temp TEST_SKILL_DIR and TMPDIR unset, + # "", or "/", a probe whose argv carries that dir must SURVIVE. A bare `sleep` cannot + # show this — the degenerate scan would find nothing to kill and pass vacuously — so the + # probe gives the scan a real target, and its survival is what distinguishes refuse from + # scan-and-kill. (Mutation: restore the old `"${TMPDIR:+${TMPDIR%/}/}"?*` guard and this + # reds — the probe gets killed under the non-temp dir.) + local marker="/agmsg-nontemp-probe-$$" + local probe="$TEST_SKILL_DIR/scripts/probe.sh" + mkdir -p "$(dirname "$probe")" + printf '%s\n' '#!/usr/bin/env bash' 'while :; do sleep 0.1; done' > "$probe" + chmod +x "$probe" + local pp + _spawn_probe() { + # Orphaned (subshell) so it reparents to init like the real detached launcher. Its + # argv carries $marker (so the reaper scoped to $marker would hit it if it degenerated) + # AND $TEST_SKILL_DIR (the probe.sh path), so the real teardown reaps it afterwards. + ( "$probe" "$marker" & printf '%s\n' "$!" > "$TEST_SKILL_DIR/probe.pid" ) + pp="$(cat "$TEST_SKILL_DIR/probe.pid")" + kill -0 "$pp" # running, so "survives" below is meaningful rather than a race + } + _spawn_probe; ( unset TMPDIR; TEST_SKILL_DIR="$marker" _reap_test_skill_dir_procs ); kill -0 "$pp"; kill "$pp" 2>/dev/null || true + _spawn_probe; TMPDIR="" TEST_SKILL_DIR="$marker" _reap_test_skill_dir_procs; kill -0 "$pp"; kill "$pp" 2>/dev/null || true + _spawn_probe; TMPDIR="/" TEST_SKILL_DIR="$marker" _reap_test_skill_dir_procs; kill -0 "$pp"; kill "$pp" 2>/dev/null || true +}