From 036c23a42f77931fd5af90f3323cffc9000c402f Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 5 Sep 2026 09:12:40 -0700 Subject: [PATCH 1/5] test(teardown): when the cleanup cannot remove the dir, say who is holding it (#1036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three windows-latest jobs failed with rm: cannot remove '/tmp/tmp.XXXXXXXXXX': 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 left the next one no better informed. This does not fix the failure. It makes the next one answer the question three reds could not: is the pid the teardown killed and waited for the same pid that is still holding the directory? The pid files record the process codex-monitor.sh backgrounded; the handle may belong to a CHILD of it, which no wait on the parent covers. Until that is observed, any fix is a guess — an earlier one of mine was, and it was wrong twice over: the kill-forwarding I proposed to add is already in both fakes, and the pid-file teardown that was supposed to solve exactly this is already there too. Placed in the teardown rather than a workflow step because the holder exits on its own 60-second timer: by the time a post-job step runs, the photograph is of an empty room. The failure itself is the only moment the holder is still there. Reports, never repairs. `rm`'s status is returned unchanged, so a failure stays a failure and nothing is swallowed; the probes are all allowed to fail, so a dump cannot become a second thing to debug. `tasklist` is included because a native Windows process does not appear in the MSYS `ps` at all — and the suspected holder, a node started by a wrapper, is exactly that kind. Verified both directions, since a probe that only ever runs on red is easy to get backwards: a normal teardown returns 0 and prints zero bytes, and a deliberately unremovable directory returns non-zero AND names the pid the teardown knew about. --- tests/test_helper.bash | 53 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/tests/test_helper.bash b/tests/test_helper.bash index 1e80586dd..6f5b1dc56 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -34,8 +34,59 @@ setup_test_env() { mkdir -p "$HOME" } +# When the removal fails, say WHO is still holding the directory (#1036). +# +# Three windows-latest failures reported nothing but `rm: cannot remove +# '/tmp/tmp.XXXXXXXXXX': Directory not empty`, on a job whose every test passed. +# That message names the directory and not the holder, so each red cost a rerun +# and taught nobody anything. POSIX unlinks a directory whose files are open, so +# this only ever fires on Windows -- which is also the only place the answer is +# needed. +# +# The probe reports and does NOT repair: the removal's own status is returned +# unchanged, so a failure stays a failure. Every probe is allowed to fail and +# none of them can turn a green teardown red -- a dump is worth nothing if it +# becomes a second thing to debug. `head` is avoided in the pipelines for the +# reason the workflow's forensics step gives: it closes the pipe early and +# SIGPIPEs whatever was writing. +# +# What it prints is chosen to answer ONE question, the one nobody could answer +# from three reds: is the pid teardown killed and waited for the same pid that +# is still holding the directory? The pid files record the wrapper that +# codex-monitor backgrounded; the handle may belong to a CHILD of that wrapper, +# which no wait on the parent covers. +_teardown_forensics() { + local dir="$1" pf pid + set +e + { + echo "##### teardown could not remove $dir (#1036)" + echo "##### what is still there:" + ls -laR "$dir" 2>/dev/null + echo "##### pids teardown knew about, and whether they are still alive:" + for pf in "$dir"/run/*.pid; do + [ -f "$pf" ] || continue + pid="$(cat "$pf" 2>/dev/null)" + [ -n "$pid" ] || continue + if kill -0 "$pid" 2>/dev/null; then + echo " $pf -> $pid STILL ALIVE (teardown waited for this one)" + else + echo " $pf -> $pid exited" + fi + done + echo "##### every process this runner can see (the holder is in here):" + ps -ef 2>/dev/null || ps 2>/dev/null + # Native Windows processes do not appear in the MSYS ps at all, and the + # holder is exactly the kind that would not: a node started by a wrapper. + command -v tasklist >/dev/null 2>&1 && tasklist 2>/dev/null + } >&2 + return 0 +} + teardown_test_env() { - rm -rf "$TEST_SKILL_DIR" + local rc=0 + rm -rf "$TEST_SKILL_DIR" || rc=$? + [ "$rc" -eq 0 ] || _teardown_forensics "$TEST_SKILL_DIR" + return "$rc" } # Skip a test on native Windows / Git Bash (MSYS/MINGW/Cygwin). Use ONLY for From 8343c681e61dae545ed93ff5a8bf5aaba19aefd5 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 5 Sep 2026 09:22:42 -0700 Subject: [PATCH 2/5] test(teardown): capture the pids before the removal, and stop claiming the holder (#1036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blocking points from review, all of them right. 1. The pid set was re-scanned AFTER the failed `rm -rf`. A recursive delete can unlink part of the tree before it fails, so on the arm where the pid files go first and the directory survives, the report would have printed nothing — while its heading still said "the pids teardown knew about". A label with no content reads as an answer. The set is now captured BEFORE the removal, and the file-level teardown hands over the pids it actually signalled, which is not recoverable from disk afterwards. 2. `ps -ef` / `tasklist` is a whole-machine inventory; nothing here binds an open handle to the remaining path. The old wording ("the holder is in here") and the PR body's "the next failure answers the question" claimed an identification the dump cannot make. Both are narrowed to what is true: the waited pids' liveness plus an inventory taken at the same moment, offered as correlation, with the holder explicitly unproven. 3. `set +e` leaked into the caller and was never restored — a report-only probe must not change the state the framework runs in afterwards. The body is now a subshell. Verified with controls rather than by reading: a normal teardown still returns 0 and prints zero bytes; an unremovable directory whose pid file the rm has already deleted still names that pid and reports it alive; and errexit survives the probe. (One of those controls first reported an overclaim that was not in the file — the `ps` dump had captured the fixture's own command line, grep pattern and all.) --- tests/test_codex_monitor.bats | 6 +++ tests/test_helper.bash | 93 +++++++++++++++++++++-------------- 2 files changed, 61 insertions(+), 38 deletions(-) diff --git a/tests/test_codex_monitor.bats b/tests/test_codex_monitor.bats index ebf4be667..8fca8eac3 100644 --- a/tests/test_codex_monitor.bats +++ b/tests/test_codex_monitor.bats @@ -71,7 +71,13 @@ 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 ACTED on, which is not + # recoverable from disk afterwards: the recursive delete can unlink these + # very files before it fails. + 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 6f5b1dc56..7fdeac4c5 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -34,61 +34,78 @@ setup_test_env() { mkdir -p "$HOME" } -# When the removal fails, say WHO is still holding the directory (#1036). +# When the removal fails, report what is known about it (#1036). # -# Three windows-latest failures reported nothing but `rm: cannot remove -# '/tmp/tmp.XXXXXXXXXX': Directory not empty`, on a job whose every test passed. -# That message names the directory and not the holder, so each red cost a rerun -# and taught nobody anything. POSIX unlinks a directory whose files are open, so -# this only ever fires on Windows -- which is also the only place the answer is -# needed. +# 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. # -# The probe reports and does NOT repair: the removal's own status is returned -# unchanged, so a failure stays a failure. Every probe is allowed to fail and -# none of them can turn a green teardown red -- a dump is worth nothing if it -# becomes a second thing to debug. `head` is avoided in the pipelines for the -# reason the workflow's forensics step gives: it closes the pipe early and -# SIGPIPEs whatever was writing. +# WHAT THIS ESTABLISHES, and what it does not. It records the pids this teardown +# actually killed and waited for, and whether each is still alive 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. The pids and the +# inventory are correlation material for whoever reads the next red — the +# holder's identity stays unproven until something can name it. # -# What it prints is chosen to answer ONE question, the one nobody could answer -# from three reds: is the pid teardown killed and waited for the same pid that -# is still holding the directory? The pid files record the wrapper that -# codex-monitor backgrounded; the handle may belong to a CHILD of that wrapper, -# which no wait on the parent covers. +# The pid set is captured BEFORE the removal, not scanned after it. A recursive +# delete can unlink some of the tree before it fails, so the pid files may be +# gone by the time the report runs; a reporter that re-scanned them would then +# print nothing at exactly the moment its output was wanted. +# +# Reports, never repairs: the removal's own status is returned unchanged, so a +# failure stays a failure. The probe body runs in a SUBSHELL so its `set +e` +# cannot leak into the caller — a report-only probe must not alter 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" pf pid - set +e - { + local dir="$1" waited="$2" + ( + 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 - echo "##### pids teardown knew about, and whether they are still alive:" - for pf in "$dir"/run/*.pid; do - [ -f "$pf" ] || continue - pid="$(cat "$pf" 2>/dev/null)" - [ -n "$pid" ] || continue - if kill -0 "$pid" 2>/dev/null; then - echo " $pf -> $pid STILL ALIVE (teardown waited for this one)" - else - echo " $pf -> $pid exited" - fi - done - echo "##### every process this runner can see (the holder is in here):" + echo "##### pids this teardown killed and waited for, captured BEFORE the rm:" + if [ -z "$waited" ]; then + echo " (none — no pid file existed when the removal started)" + else + local pid + for pid in $waited; 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 + fi + 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 - # Native Windows processes do not appear in the MSYS ps at all, and the - # holder is exactly the kind that would not: a node started by a wrapper. + # 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 + ) >&2 return 0 } teardown_test_env() { - local rc=0 + local rc=0 waited="" pf pid + # Snapshot BEFORE the removal (see above). AGMSG_TEARDOWN_WAITED_PIDS lets a + # file-level teardown hand over the pids it actually signalled, which is + # stronger than anything inferable from the files still on disk. + for pf in "$TEST_SKILL_DIR"/run/*.pid; do + [ -f "$pf" ] || continue + pid="$(cat "$pf" 2>/dev/null)" + [ -n "$pid" ] && waited="$waited $pid" + done + waited="${AGMSG_TEARDOWN_WAITED_PIDS:-}$waited" rm -rf "$TEST_SKILL_DIR" || rc=$? - [ "$rc" -eq 0 ] || _teardown_forensics "$TEST_SKILL_DIR" + [ "$rc" -eq 0 ] || _teardown_forensics "$TEST_SKILL_DIR" "$waited" 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 From fec21ab26288a604c4365f58eb17a08345cd667d Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 5 Sep 2026 09:31:54 -0700 Subject: [PATCH 3/5] test(teardown): keep the two pid sets apart, and pin the arm that broke (#1036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review, second round, both points right. The handover of the pids a file-level teardown actually signalled was sound, but `teardown_test_env` then re-scanned `run/*.pid` and merged the result into the same list — so a pid file nobody ever waited on could be reported under a heading that said "killed and waited for". That is the same defect the previous commit fixed, one layer along: a label wider than the thing it names. The two sets now print separately, with the second explicitly marked as "NOT known to have been waited on", and duplicates removed. The reporter only runs on a red that appears about once a day on one platform, so nothing else in the suite would notice it rotting. tests/test_teardown_ forensics.bats pins what makes it worth having: silence on green, the partial- delete arm (the removal unlinks the pid file and THEN fails, and the pid is still named), provenance kept apart, no duplicates, and no shell-option leak. Each was checked by mutation rather than by reading — scanning after the rm, merging the sets, dropping the dedupe, and un-subshelling the probe each turn one or more of them red. The last of those did NOT go red at first. `set -e` leaking out of the probe is invisible inside a bats @test, because bats manages that option around every command it runs — verified by watching the same mutation leak for real in a plain bash and pass in here. The control now asks a plain bash instead, which is where the property actually matters, and it fails on the mutation as it should. --- tests/test_helper.bash | 88 +++++++++++++++----------- tests/test_teardown_forensics.bats | 99 ++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 35 deletions(-) create mode 100644 tests/test_teardown_forensics.bats diff --git a/tests/test_helper.bash b/tests/test_helper.bash index 7fdeac4c5..1e0d995d0 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -40,45 +40,39 @@ setup_test_env() { # 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 the pids this teardown -# actually killed and waited for, and whether each is still alive 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. The pids and the -# inventory are correlation material for whoever reads the next red — the -# holder's identity stays unproven until something can name it. +# 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. # -# The pid set is captured BEFORE the removal, not scanned after it. A recursive -# delete can unlink some of the tree before it fails, so the pid files may be -# gone by the time the report runs; a reporter that re-scanned them would then -# print nothing at exactly the moment its output was wanted. +# TWO SETS, KEPT APART, because they are not the same claim: # -# Reports, never repairs: the removal's own status is returned unchanged, so a -# failure stays a failure. The probe body runs in a SUBSHELL so its `set +e` -# cannot leak into the caller — a report-only probe must not alter the state the +# acted pids a file-level teardown actually signalled and waited for, handed +# over before the removal ran. This is the set the question is about. +# seen pid FILES that merely existed before the removal. Nobody may have +# waited on these. Printing them under the first heading would say +# something untrue about them — the same label-without-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" waited="$2" + 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 - echo "##### pids this teardown killed and waited for, captured BEFORE the rm:" - if [ -z "$waited" ]; then - echo " (none — no pid file existed when the removal started)" - else - local pid - for pid in $waited; 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 - fi + _teardown_report_pids "pids a teardown SIGNALLED and WAITED FOR (captured before the rm)" "$acted" + _teardown_report_pids "pid FILES present before the rm — NOT known to have been waited on" "$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 @@ -89,19 +83,43 @@ _teardown_forensics() { 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() { - local rc=0 waited="" pf pid - # Snapshot BEFORE the removal (see above). AGMSG_TEARDOWN_WAITED_PIDS lets a - # file-level teardown hand over the pids it actually signalled, which is - # stronger than anything inferable from the files still on disk. + 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" ] && waited="$waited $pid" + [ -n "$pid" ] && case " $acted " in *" $pid "*) : ;; *) seen="$seen $pid" ;; esac done - waited="${AGMSG_TEARDOWN_WAITED_PIDS:-}$waited" + seen="$(_teardown_uniq_pids "$seen")" rm -rf "$TEST_SKILL_DIR" || rc=$? - [ "$rc" -eq 0 ] || _teardown_forensics "$TEST_SKILL_DIR" "$waited" + [ "$rc" -eq 0 ] || _teardown_forensics "$TEST_SKILL_DIR" "$acted" "$seen" return "$rc" } diff --git a/tests/test_teardown_forensics.bats b/tests/test_teardown_forensics.bats new file mode 100644 index 000000000..1df13ca5b --- /dev/null +++ b/tests/test_teardown_forensics.bats @@ -0,0 +1,99 @@ +#!/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 that 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. + +load test_helper + +setup() { + PROBE_DIR="$(mktemp -d)" + unset AGMSG_TEARDOWN_WAITED_PIDS +} + +teardown() { + [ -n "${LIVE_PID:-}" ] && kill "$LIVE_PID" 2>/dev/null + [ -d "${PROBE_DIR:-}/zlocked" ] && chmod 700 "$PROBE_DIR/zlocked" 2>/dev/null + [ -n "${PROBE_DIR:-}" ] && rm -rf "$PROBE_DIR" 2>/dev/null + return 0 +} + +# Build a directory whose `rm -rf` removes run/ (and the pid file in it) and +# THEN fails on an unwritable subdirectory. `zlocked` sorts after `run`, which is +# what puts the deletion first — the partial-delete arm. +_unremovable_with_pidfile() { # + mkdir -p "$PROBE_DIR/run" "$PROBE_DIR/zlocked" + printf '%s\n' "$1" > "$PROBE_DIR/run/server.pid" + : > "$PROBE_DIR/zlocked/inner" + chmod 500 "$PROBE_DIR/zlocked" +} + +@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" + _unremovable_with_pidfile "$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 + [[ "$output" == *"$LIVE_PID STILL ALIVE"* ]] # …and the pid survived anyway + [[ "$output" == *"SIGNALLED and WAITED FOR"* ]] +} + +@test "teardown forensics: a pid file nobody waited on is NOT reported as waited (#1036)" { + # Provenance: the generic pre-rm scan finds pid FILES. Reporting those under + # the "signalled and waited for" heading would say something untrue about + # them, which is the defect this file exists to keep out. + export TEST_SKILL_DIR="$PROBE_DIR" + _unremovable_with_pidfile 999001 # written, never signalled + + run teardown_test_env + [ "$status" -ne 0 ] + local waited_block="${output#*SIGNALLED and WAITED FOR}" + waited_block="${waited_block%%#####*}" + [[ "$waited_block" != *999001* ]] + [[ "$output" == *"NOT known to have been waited on"* ]] + [[ "$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" + _unremovable_with_pidfile 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 — checked, and + # the mutation that removes the subshell passes in here while leaking for real + # outside. The property belongs to whatever sources the helper, so that is + # what gets asked. + _unremovable_with_pidfile 999003 + run bash -c ' + . "$1/test_helper.bash" + export TEST_SKILL_DIR="$2" + 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" + [[ "$output" == *OPTIONS_INTACT* ]] +} From 407025b32153843459ce13e5ab3035517166684f Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 5 Sep 2026 11:02:42 -0700 Subject: [PATCH 4/5] test(teardown): narrow the label to what the producer promises, and make the controls enforceable (#1036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review, third round, both points right again. The column heading said the pids had been SIGNALLED AND WAITED FOR. The producer does `kill "$pid" || true` and `wait_for_pid_exit "$pid" || true`, discarding both statuses — so a pid already gone, and one whose wait timed out, are in that set too. The heading, the source comment and the PR body now say what is true of every member: the teardown CALLED kill and a wait on it. The outcome is the STILL ALIVE marker printed beside each pid, and that one is measured. Third round on the same axis, one layer further in each time: sets mixed -> columns split but the label wider than its content -> the label matched to what the producer actually guarantees. The controls added last round to fix "a claim that is never checked" were themselves not checking. Three of their central claims were non-last `[[ ]]`, which on bash 3.2 — CI's macOS leg — reports ok with a false claim inside it (#670). check-enforced-assertions.sh names exactly those three: 638 against a baseline of 635, exit 1. It was in the tree the whole time and nobody ran it. Every claim is now a plain command or a `[ ]`; the checker is back at 635. The partial-delete arm no longer leans on `rm` walking the tree in name order or on chmod 500 refusing a delete. Neither is what this file asks about, and both can produce a red that means nothing. A seam replaces `rm` for that one call: it unlinks the pid file, then returns non-zero. Six mutations, one per property, each red: scanning after the rm, merging the sets, dropping the dedupe, leaking set +e, reporting on success, and dropping the liveness marker. --- tests/test_codex_monitor.bats | 8 ++- tests/test_helper.bash | 20 ++++--- tests/test_teardown_forensics.bats | 91 +++++++++++++++++++----------- 3 files changed, 75 insertions(+), 44 deletions(-) diff --git a/tests/test_codex_monitor.bats b/tests/test_codex_monitor.bats index 8fca8eac3..e4690074a 100644 --- a/tests/test_codex_monitor.bats +++ b/tests/test_codex_monitor.bats @@ -72,9 +72,11 @@ teardown() { 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 ACTED on, which is not - # recoverable from disk afterwards: the recursive delete can unlink these - # very files before it fails. + # 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 diff --git a/tests/test_helper.bash b/tests/test_helper.bash index 1e0d995d0..f12c1c455 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -48,12 +48,16 @@ setup_test_env() { # # TWO SETS, KEPT APART, because they are not the same claim: # -# acted pids a file-level teardown actually signalled and waited for, handed -# over before the removal ran. This is the set the question is about. -# seen pid FILES that merely existed before the removal. Nobody may have -# waited on these. Printing them under the first heading would say -# something untrue about them — the same label-without-content defect -# that an earlier draft of this had, one layer along. +# 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 @@ -71,8 +75,8 @@ _teardown_forensics() { echo "##### teardown could not remove $dir (#1036)" echo "##### what is still there:" ls -laR "$dir" 2>/dev/null - _teardown_report_pids "pids a teardown SIGNALLED and WAITED FOR (captured before the rm)" "$acted" - _teardown_report_pids "pid FILES present before the rm — NOT known to have been waited on" "$seen" + _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 diff --git a/tests/test_teardown_forensics.bats b/tests/test_teardown_forensics.bats index 1df13ca5b..24bd0c2c5 100644 --- a/tests/test_teardown_forensics.bats +++ b/tests/test_teardown_forensics.bats @@ -4,9 +4,14 @@ # # 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 that 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. +# 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 @@ -17,19 +22,36 @@ setup() { teardown() { [ -n "${LIVE_PID:-}" ] && kill "$LIVE_PID" 2>/dev/null - [ -d "${PROBE_DIR:-}/zlocked" ] && chmod 700 "$PROBE_DIR/zlocked" 2>/dev/null - [ -n "${PROBE_DIR:-}" ] && rm -rf "$PROBE_DIR" 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 } -# Build a directory whose `rm -rf` removes run/ (and the pid file in it) and -# THEN fails on an unwritable subdirectory. `zlocked` sorts after `run`, which is -# what puts the deletion first — the partial-delete arm. -_unremovable_with_pidfile() { # - mkdir -p "$PROBE_DIR/run" "$PROBE_DIR/zlocked" +# 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" - : > "$PROBE_DIR/zlocked/inner" - chmod 500 "$PROBE_DIR/zlocked" + 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)" { @@ -46,35 +68,35 @@ _unremovable_with_pidfile() { # sleep 30 & LIVE_PID=$! export TEST_SKILL_DIR="$PROBE_DIR" export AGMSG_TEARDOWN_WAITED_PIDS="$LIVE_PID" - _unremovable_with_pidfile "$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 - [[ "$output" == *"$LIVE_PID STILL ALIVE"* ]] # …and the pid survived anyway - [[ "$output" == *"SIGNALLED and WAITED FOR"* ]] + [ "$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 waited on is NOT reported as waited (#1036)" { - # Provenance: the generic pre-rm scan finds pid FILES. Reporting those under - # the "signalled and waited for" heading would say something untrue about +@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" - _unremovable_with_pidfile 999001 # written, never signalled + _arm_partial_delete 999001 # written, never signalled run teardown_test_env [ "$status" -ne 0 ] - local waited_block="${output#*SIGNALLED and WAITED FOR}" - waited_block="${waited_block%%#####*}" - [[ "$waited_block" != *999001* ]] - [[ "$output" == *"NOT known to have been waited on"* ]] - [[ "$output" == *999001* ]] + 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" - _unremovable_with_pidfile 999002 + _arm_partial_delete 999002 run teardown_test_env [ "$status" -ne 0 ] @@ -83,17 +105,20 @@ _unremovable_with_pidfile() { # @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 — checked, and - # the mutation that removes the subshell passes in here while leaking for real - # outside. The property belongs to whatever sources the helper, so that is - # what gets asked. - _unremovable_with_pidfile 999003 + # 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" - [[ "$output" == *OPTIONS_INTACT* ]] + [ "$status" -eq 0 ] + _says "$output" OPTIONS_INTACT } From b5aedd52d124eacb03aa4038942ec4fb810dafa3 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 5 Sep 2026 17:11:06 -0700 Subject: [PATCH 5/5] test(codex-monitor): stub the detached bridge launcher so it cannot race teardown (#1049 CI) codex-monitor.sh spawns codex-bridge-launcher.sh DETACHED ("outlives this script"), and it resolves its own SKILL_DIR from its script path -> TEST_SKILL_DIR under test, so it writes into TEST_SKILL_DIR/run (temp files, a bridge pidfile, and a `mkdir -p run/` that recreates the dir). The test teardown kills only the codex-app-server.*.pid set, never this launcher, so under shard load it is still writing run/ when teardown_test_env removes the tree -> "rm: cannot remove ...: Directory not empty" -- and its atomic-rename temp is gone by the time the #1036 forensic reporter runs, which is why the report saw an empty run/. It surfaced with #1049 only because #1049 added ~3k lines of tests, shifting this shard's load enough to lose a race that main's lighter shard did not; no single line of #1049 introduces it. None of these tests exercise the bridge (they assert app-server pid handling and the codex handoff in CALL_LOG), so a no-op launcher removes the racer without changing coverage. Confirmed: the real launcher fires in tests 357/359 (three invocations, each with a real ws:// url and the app-server ppid); with the stub all 10 tests pass and no bridge process lingers. The natural timing race did not reproduce locally (isolation, 4x CPU load, and --jobs 4 all clean): a fast machine finishes the launcher before teardown. The fix targets the confirmed mechanism, not a reproduced timing. --- tests/test_codex_monitor.bats | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_codex_monitor.bats b/tests/test_codex_monitor.bats index e4690074a..feeb27c06 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() {