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

[fujibee]

Three windows-latest jobs failed with

rm: cannot remove '/tmp/tmp.XXXXXXXXXX': Directory not empty

on runs where every test passed. The message names the directory and not the
holder, so each red cost a rerun and left the next one no better informed.

What this establishes — and what it does not

It records the pids this teardown called kill and a wait on — not the
pids it 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 that column; it is an intent, and the STILL ALIVE
marker printed beside each pid is the outcome. It also records a process
inventory taken at the moment the removal failed.

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.

That boundary is deliberate. An earlier draft of this PR said the next failure
would answer the question outright, which was more than the dump can support.

The pid set is captured before the removal

A recursive delete can unlink part of the tree before it fails. On the arm where
the pid files go first and the directory survives, a reporter that re-scanned
them afterwards would print nothing — while its heading still said "the pids
teardown knew about". A label with no content reads as an answer.

The file-level teardown also hands over the pids it called kill on, which is
more than is inferable from what remains on disk — and the two sets are printed
in separate columns, the second marked "NOT known to have been signalled",
because a pid FILE that merely existed is not a pid anything acted on.

Why the teardown and not a workflow step

The suspected holder exits on its own 60-second timer. By the time a post-job
step runs, the photograph is of an empty room. The failing cleanup is the only
moment it is still there.

Reports, never repairs

• rm's status is returned unchanged — a failure stays a failure
• the probe body is a subshell, so its set +e cannot leak into the state
  the framework runs in afterwards
• every probe may fail without consequence
• no head in the pipelines — it SIGPIPEs the writer, the same reason the
  workflow's forensics step avoids it
• tasklist is included because a native Windows process is absent from the
  MSYS ps entirely

Verified with controls

case	result
normal teardown	returns 0, prints zero bytes
unremovable dir whose pid file the rm already deleted	returns non-zero, still names that pid and reports it alive
errexit set by the caller	still set after the probe returns

Only Windows can confirm the diagnosis this enables; POSIX unlinks a directory
whose files are open, which is why this has never reproduced locally.

What was wrong before, recorded so nobody retraces it

Two earlier diagnoses of mine were wrong: I read a comment present on one of two
identical fakes as evidence of a mechanism absent from the other, and I proposed
adding a pid-file teardown loop that was already there.

The controls are enforceable, and each was broken to prove it

tests/test_teardown_forensics.bats pins the properties above. A first draft of
it made three of its central claims with a non-last [[ ]], which on bash 3.2 —
what CI's macOS leg runs — reports ok with a false claim inside it (#670).
check-enforced-assertions.sh names them: it read 638 against a baseline of 635
and exited 1. Every claim is now a plain command or a [ ], and the checker is
back at the baseline.

The partial-delete arm no longer depends on rm walking the tree in name order
or on chmod 500 refusing a delete — two assumptions about the platform, and
neither is what the file is asking about. A seam replaces rm for that one
call: it unlinks the pid file, then returns non-zero.

Six mutations, one per property, each red:

mutation	result
scan the pid files after the rm instead of before	1 failing
merge both sets into the acted column	1 failing
drop the dedupe	1 failing
let the probe leak set +e	1 failing
report on a successful removal too	1 failing
drop the STILL ALIVE liveness marker	1 failing

---

Root cause of the CI red (codex-monitor 357/359), and the fix at b5aedd5

The Directory not empty red on codex-monitor: never kills a non-codex process recorded under a reused pid / reuses a live app-server when tasklist cannot see it (#567) is a teardown race whose writer is now named:

• codex-monitor.sh:240 spawns codex-bridge-launcher.sh detached (… &, comment: "the launcher is detached on purpose and outlives this script").
• The launcher resolves its own SKILL_DIR from its script path (codex-bridge-launcher.sh:31), which under test is TEST_SKILL_DIR, so it writes into TEST_SKILL_DIR/run — including mkdir -p "$RUN_DIR" (recreates the dir), an atomic-rename temp ${prefix}${now}.$$.$RANDOM, a bridge pidfile, and an identity-cache marker.
• The test teardown kills only the codex-app-server.*.pid set — never this launcher — so it outlives the test and, under shard load, is still writing run/ when teardown_test_env removes the tree → rm: … Directory not empty. Its temp is gone by the time the windows runtime (#567): the job fails in teardown while every test passes — a file is still held under the temp dir #1036 forensic reporter runs, which is why the report shows an empty run/.
• Confirmed the launcher fires: stubbing it to a probe showed 3 invocations in 357/359 (codex <proj> ws://127.0.0.1:PORT <ppid>).

Why #1049 and not main: #1049 adds ~3k lines across 39 test files; the shard split (weighted by @test count) rebalances and the codex-monitor shard gets heavier, so the launcher is slower under that load and overlaps the rm. No single line of #1049 introduces the race — it exposes a pre-existing latent race in the codex-monitor teardown via shard load.

Fix (b5aedd5): stub AGMSG_CODEX_BRIDGE_LAUNCHER_CMD to a no-op in test_codex_monitor.bats setup, so the detached launcher never spawns. These tests assert app-server pid handling and the codex handoff in CALL_LOG, not the bridge, so coverage is unchanged. Verified: 10/10 green, no bridge process lingers.

Reproduction status (honest)

The natural timing race did not reproduce locally — isolation (2/2), 4× CPU load (3/3), and bats --jobs 4 (4/4) were all clean. A fast machine finishes the launcher before teardown. "Did not reproduce" is a result, not proof of absence; the mechanism is established by the code path plus the confirmed launcher spawn.

Deterministic reproduction the next person can add (seam, not sleep)

Rather than chase the timing, pin the timepoint with a barrier seam like inbox.sh:82's AGMSG_TEST_MARK_BARRIER (.reached / .release, no-op when unset; siblings in check-inbox.sh, remote.sh). Concretely: point AGMSG_CODEX_BRIDGE_LAUNCHER_CMD at a stub that (given the run dir via env) writes a run/ file, touches <barrier>.reached, and blocks on <barrier>.release; have the test wait for .reached, then run the teardown rm while the launcher's file is present — a deterministic Directory not empty. The b5aedd5 fix (no-op launcher) then turns it green. That converts this from a load-flake into a controlled regression test.

Priority note: base is main, not on the 1.3.0 path; left at b5aedd5 with the above so the next person need not redo the investigation.