test(teardown): report pid and process state when cleanup fails, without naming the holder (#1036)

tests/test_codex_monitor.bats

@@ -56,6 +56,21 @@ PY
 esac
 EOF
   chmod +x "$FAKE_CODEX"
+
+  # Stub the bridge launcher (#1049 CI race). codex-monitor.sh spawns
+  # codex-bridge-launcher.sh DETACHED, and it "outlives this script" -- it
+  # resolves its own SKILL_DIR from its script path, which under test is
+  # TEST_SKILL_DIR, so it writes into TEST_SKILL_DIR/run (temp files, a bridge
+  # pidfile, and mkdir -p run/ that RECREATES the dir). teardown kills only the
+  # codex-app-server.*.pid set, never this launcher, so under load it is still
+  # writing run/ when teardown_test_env removes the tree -> "rm: Directory not
+  # empty". None of these tests exercise the bridge (they assert on app-server
+  # pid handling and the codex handoff in CALL_LOG), so a no-op launcher removes
+  # the racer without changing what is tested.
+  local noop="$TEST_PROJECT/noop-bridge-launcher"
+  printf '%s\n' '#!/usr/bin/env bash' 'exit 0' > "$noop"
+  chmod +x "$noop"
+  export AGMSG_CODEX_BRIDGE_LAUNCHER_CMD="$noop"
 }
 
 teardown() {
@@ -71,7 +86,15 @@ teardown() {
     [ -n "$pid" ] || continue
     kill "$pid" 2>/dev/null || true
     wait_for_pid_exit "$pid" || true
+    # Hand the pid to the shared teardown's reporter (#1036). What matters when
+    # the removal then fails is the set this teardown called kill and wait on,
+    # which is not recoverable from disk afterwards: the recursive delete can
+    # unlink these very files before it fails. Both statuses above are
+    # discarded, so this records the attempt, not a confirmed exit — the
+    # reporter labels it that way.
+    AGMSG_TEARDOWN_WAITED_PIDS="${AGMSG_TEARDOWN_WAITED_PIDS:-} $pid"
   done
+  export AGMSG_TEARDOWN_WAITED_PIDS
   rm -rf "$TEST_PROJECT"
   teardown_test_env
 }

tests/test_helper.bash

@@ -34,10 +34,100 @@ setup_test_env() {
   mkdir -p "$HOME"
 }
 
+# When the removal fails, report what is known about it (#1036).
+#
+# Three windows-latest jobs failed with `rm: cannot remove …: Directory not
+# empty` on runs where every test passed. That message names the directory and
+# not the holder, so each red cost a rerun and taught the next one nothing.
+#
+# WHAT THIS ESTABLISHES, and what it does not. It records pids and their
+# liveness at the moment the removal failed, alongside a process inventory taken
+# at that same moment. It does NOT identify the holder: nothing here binds an
+# open handle to the remaining path, and neither `ps` nor `tasklist` can. All of
+# it is correlation material for whoever reads the next red.
+#
+# TWO SETS, KEPT APART, because they are not the same claim:
+#
+#   acted   pids a file-level teardown CALLED `kill` and a wait on, handed over
+#           before the removal ran. Not "killed": the producer discards both
+#           statuses (`kill … || true`, `wait_for_pid_exit … || true`), so a pid
+#           that was already gone, and one whose wait timed out, are both in
+#           here. Read as intent, not as outcome — the STILL ALIVE marker
+#           beside each pid is the outcome, and it is measured here.
+#   seen    pid FILES that merely existed before the removal. Nothing here
+#           signalled them. Printing them in the first column would say
+#           something untrue about them — the same label-wider-than-its-content
+#           defect that an earlier draft of this had, one layer along.
+#
+# Both are captured BEFORE the removal: a recursive delete can unlink part of
+# the tree before it fails, so a reporter that scanned afterwards would print
+# nothing at exactly the moment its output was wanted.
+#
+# Reports, never repairs: the removal's own status is returned unchanged. The
+# probe body is a SUBSHELL so its `set +e` cannot leak into the state the
+# framework runs in afterwards. `head` is avoided in the pipelines, as the
+# workflow's forensics step does, because it SIGPIPEs whatever was writing.
+_teardown_forensics() {
+  local dir="$1" acted="$2" seen="$3"
+  (
+    set +e
+    set +o pipefail 2>/dev/null || true
+    echo "##### teardown could not remove $dir (#1036)"
+    echo "##### what is still there:"
+    ls -laR "$dir" 2>/dev/null
+    _teardown_report_pids "pids a teardown CALLED kill AND wait ON (captured before the rm)" "$acted"
+    _teardown_report_pids "pid FILES present before the rm — NOT known to have been signalled" "$seen"
+    echo "##### process inventory at that same moment (correlation only —"
+    echo "##### this does NOT say which of these holds the directory):"
+    ps -ef 2>/dev/null || ps 2>/dev/null
+    # A native Windows process is absent from the MSYS ps entirely, and the
+    # suspected holder — a node started by a wrapper — is exactly that kind.
+    command -v tasklist >/dev/null 2>&1 && tasklist 2>/dev/null
+  ) >&2
+  return 0
+}
+
+_teardown_report_pids() {
+  local heading="$1" pids="$2" pid
+  echo "##### $heading:"
+  if [ -z "${pids// /}" ]; then
+    echo "  (none)"
+    return 0
+  fi
+  for pid in $pids; do
+    if kill -0 "$pid" 2>/dev/null; then
+      echo "  $pid STILL ALIVE at the moment the removal failed"
+    else
+      echo "  $pid exited"
+    fi
+  done
+}
+
+# Deduplicate a whitespace-separated pid list, order preserved.
+_teardown_uniq_pids() {
+  local seen=" " out="" p
+  for p in $1; do
+    case "$seen" in *" $p "*) continue ;; esac
+    seen="$seen$p "; out="$out $p"
+  done
+  printf '%s' "${out# }"
+}
+
 teardown_test_env() {
-  rm -rf "$TEST_SKILL_DIR"
+  local rc=0 acted seen="" pf pid
+  acted="$(_teardown_uniq_pids "${AGMSG_TEARDOWN_WAITED_PIDS:-}")"
+  for pf in "$TEST_SKILL_DIR"/run/*.pid; do
+    [ -f "$pf" ] || continue
+    pid="$(cat "$pf" 2>/dev/null)"
+    [ -n "$pid" ] && case " $acted " in *" $pid "*) : ;; *) seen="$seen $pid" ;; esac
+  done
+  seen="$(_teardown_uniq_pids "$seen")"
+  rm -rf "$TEST_SKILL_DIR" || rc=$?
+  [ "$rc" -eq 0 ] || _teardown_forensics "$TEST_SKILL_DIR" "$acted" "$seen"
+  return "$rc"
 }
 
+
 # Skip a test on native Windows / Git Bash (MSYS/MINGW/Cygwin). Use ONLY for
 # behaviour that depends on POSIX process semantics agmsg does not yet support
 # there — watcher discovery/kill via ps/pgrep, and session liveness via kill -0

tests/test_teardown_forensics.bats

@@ -0,0 +1,124 @@
+#!/usr/bin/env bats
+#
+# Controls for the teardown's failure reporter (#1036).
+#
+# The reporter only ever runs on a red that appears roughly once a day on one
+# platform, so nothing else in the suite would notice it rotting. These pin the
+# properties that make it worth having at all — including the arm an earlier
+# draft got wrong, where the removal deletes the pid file BEFORE it fails and a
+# reporter that scanned afterwards had nothing left to say.
+#
+# Every claim here is a plain command or a `[ ]`, never a non-last `[[ ]]`: on
+# bash 3.2, which is what CI's macOS leg runs, a non-last `[[ ]]` reports `ok`
+# with a false claim inside it (#670). A first draft of this file had three of
+# them, and check-enforced-assertions.sh is what named them.
+
+load test_helper
+
+setup() {
+  PROBE_DIR="$(mktemp -d)"
+  unset AGMSG_TEARDOWN_WAITED_PIDS
+}
+
+teardown() {
+  [ -n "${LIVE_PID:-}" ] && kill "$LIVE_PID" 2>/dev/null
+  # `command`, because a test may have replaced `rm` with the seam below.
+  [ -n "${PROBE_DIR:-}" ] && command rm -rf "$PROBE_DIR" 2>/dev/null
+  return 0
+}
+
+# Says so, so a red names which claim fired rather than a bare line number.
+_says() {          # <haystack> <needle>
+  case "$1" in *"$2"*) return 0 ;; esac
+  echo "expected the report to contain: $2" >&2
+  return 1
+}
+
+# The removal fails, and the pid file is already gone when it does.
+#
+# A seam, not a filesystem trick. An earlier draft built an unwritable
+# subdirectory sorting after `run/` and leaned on `rm` walking it in name order
+# and on chmod 500 refusing the delete — two assumptions about the platform,
+# neither of which is what this file is asking about. The question is what the
+# reporter can still say after a partial delete, so the partial delete is
+# stated outright.
+_arm_partial_delete() {   # <pid to write into the pid file>
+  mkdir -p "$PROBE_DIR/run"
+  printf '%s\n' "$1" > "$PROBE_DIR/run/server.pid"
+  rm() {
+    if [ "${1:-}" = "-rf" ] && [ "${2:-}" = "$PROBE_DIR" ]; then
+      command rm -f "$PROBE_DIR/run/server.pid"
+      return 1
+    fi
+    command rm "$@"
+  }
+}
+
+@test "teardown forensics: a successful removal says nothing at all (#1036)" {
+  export TEST_SKILL_DIR="$PROBE_DIR"
+  mkdir -p "$TEST_SKILL_DIR/run"
+  run teardown_test_env
+  [ "$status" -eq 0 ]
+  # Not "few bytes" — none. A probe that chatters on green trains people to
+  # ignore it on red.
+  [ -z "$output" ]
+}
+
+@test "teardown forensics: the rm deletes the pid file first, and the report still names it (#1036)" {
+  sleep 30 & LIVE_PID=$!
+  export TEST_SKILL_DIR="$PROBE_DIR"
+  export AGMSG_TEARDOWN_WAITED_PIDS="$LIVE_PID"
+  _arm_partial_delete "$LIVE_PID"
+
+  run teardown_test_env
+  [ "$status" -ne 0 ]                          # the failure is not swallowed
+  [ ! -f "$PROBE_DIR/run/server.pid" ]         # the arm under test: it is gone
+  _says "$output" "$LIVE_PID STILL ALIVE"      # …and the pid survived anyway
+  _says "$output" "CALLED kill AND wait ON"
+}
+
+@test "teardown forensics: a pid file nobody signalled is NOT reported as signalled (#1036)" {
+  # Provenance: the generic pre-rm scan finds pid FILES. Reporting those in the
+  # column for pids the teardown acted on would say something untrue about
+  # them, which is the defect this file exists to keep out.
+  export TEST_SKILL_DIR="$PROBE_DIR"
+  _arm_partial_delete 999001                   # written, never signalled
+
+  run teardown_test_env
+  [ "$status" -ne 0 ]
+  local acted_block="${output#*CALLED kill AND wait ON}"
+  acted_block="${acted_block%%#####*}"
+  refute _says "$acted_block" 999001
+  _says "$output" "NOT known to have been signalled"
+  _says "$output" 999001
+}
+
+@test "teardown forensics: a pid is not reported twice when both sources carry it (#1036)" {
+  export TEST_SKILL_DIR="$PROBE_DIR"
+  export AGMSG_TEARDOWN_WAITED_PIDS="999002 999002"
+  _arm_partial_delete 999002
+
+  run teardown_test_env
+  [ "$status" -ne 0 ]
+  [ "$(printf '%s\n' "$output" | grep -c '^  999002 ')" -eq 1 ]
+}
+
+@test "teardown forensics: the probe does not change the caller's shell options (#1036)" {
+  # Asked of a plain bash, not of this test body: bats manages `set -e` around
+  # every command it runs, so a leak inside a @test is invisible — measured,
+  # and the mutation that removes the subshell passed in here while leaking for
+  # real outside. The property belongs to whatever sources the helper, so that
+  # is what gets asked.
+  mkdir -p "$PROBE_DIR/run"
+  printf '999003\n' > "$PROBE_DIR/run/server.pid"
+  run bash -c '
+    . "$1/test_helper.bash"
+    export TEST_SKILL_DIR="$2"
+    rm() { if [ "${1:-}" = "-rf" ]; then return 1; fi; command rm "$@"; }
+    set -e
+    teardown_test_env >/dev/null 2>&1 || true
+    case "$-" in *e*) echo OPTIONS_INTACT ;; *) echo OPTIONS_LEAKED ;; esac
+  ' _ "$BATS_TEST_DIRNAME" "$PROBE_DIR"
+  [ "$status" -eq 0 ]
+  _says "$output" OPTIONS_INTACT
+}