test(teardown): reap detached processes holding TEST_SKILL_DIR before rm (#662)

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 <dir>.
+# 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 <dir> — 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() {   # <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"
 }
 

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
+}