diff --git a/tests/test_codex_monitor.bats b/tests/test_codex_monitor.bats index ebf4be66..feeb27c0 100644 --- a/tests/test_codex_monitor.bats +++ b/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 } diff --git a/tests/test_helper.bash b/tests/test_helper.bash index 1e80586d..f12c1c45 100644 --- a/tests/test_helper.bash +++ b/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 diff --git a/tests/test_teardown_forensics.bats b/tests/test_teardown_forensics.bats new file mode 100644 index 00000000..24bd0c2c --- /dev/null +++ b/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() { # + 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() { # + 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 +}