test: reap leaked background processes on every suite ending

[sbracewell64]

Intent

Goal: stop crewmate worktrees of the firstmate repo from self-arming phantom watchers. Phantom fm-watch-arm.sh --restart plus fm-watch.sh pairs were observed live in several treehouse task worktrees; effects were an inflated shell count, polluted liveness reads, and a test suite that can hang.

Important context the diff does not show. The task as originally briefed asked me to make the Claude Stop auto-arm eligibility check refuse to arm inside a linked git worktree. That premise was investigated and DISPROVEN: bin/fm-primary-scope-lib.sh already refuses when git rev-parse --git-dir differs from --git-common-dir, with a .fm-secondmate-home marker as the secondmate exemption, and tests/fm-claude-stop-autoarm.test.sh already covers all three cases (primary arms, marked secondmate home arms, linked child worktree stays inert) and passes. So no change was made to the auto-arm or to the scope predicate, deliberately - that would have been a no-op.

The real cause was traced to a leaking test. Every live phantom carried test-harness env (FM_HOME=/tmp/fm-watcher-lock-tests./restart-healthy-peer), and the only producer of that case is tests/fm-watcher-lock.test.sh test_watch_restart_attaches_to_healthy_peer. It backgrounds a real arm and only kills it on its happy path, so when an assertion aborts the suite via fail(), or when timeout(1) kills a hung case, the arm and its child watcher are orphaned and keep polling an already-deleted temp home. Reproduced directly.

The supervising firstmate ruled to fix exactly this leak and explicitly NOT to expand into fixing the attach failure.

Decisions made, all deliberate:

• tests/lib.sh gains the reaper because it already owns the temp-root cleanup registry, keeping one owner for teardown.
• SIGKILL rather than SIGTERM is required, not sloppy: bin/fm-watch-arm.sh deliberately launches a successor watcher when its child cycle ends, so a catchable signal to the parent hands back a fresh process that was never registered. This is exactly why the original phantom respawned after being killed once.
• The process tree is snapshotted BEFORE the first kill because a dead parent leaves its children reparented and unreachable by a ppid walk.
• Cleanup is trapped on HUP, INT and TERM as well as EXIT because a bare EXIT trap does not run for a signalled shell, which is precisely why the hang path leaked.
• All roots share one ps read so registering many short-lived helpers stays cheap.
• Every background launch in tests/fm-watcher-lock.test.sh registers its pid at launch, including the short-lived lock contenders, for uniformity.
• The new regression case runs FIRST in the suite so it still executes even when a later case aborts, and it covers all three endings: a clean pass, a fail() abort, and a TERM. It was verified to FAIL against a deliberately neutered reaper, so it is not a tautology.

Known and deliberate non-goals, please do not flag these as omissions:

• test_watch_restart_attaches_to_healthy_peer still fails on this machine. That failure is pre-existing on main, upstream CI is green, and it was classified as a load-sensitive flaky fixture rather than a runtime defect: the identical scenario reproduced standalone against the same bin/fm-watch-arm.sh passes and prints the intended watcher: attached line. Fixing it was explicitly ruled out of scope for this change.
• fm_test_tmproot has a separate pre-existing quirk where a call inside command substitution loses its registration to the parent, so temp dirs accumulate. That is disk-only, affects every test file, and was intentionally left alone and reported instead.
• Nothing was added to AGENTS.md on purpose, to respect its size discipline; the durable convention went to the firstmate-coding-guidelines skill and to the tests/lib.sh header, which are the classified owners.

What Changed

• tests/lib.sh now owns a process reaper alongside its temp-root registry: background pids are registered at launch, the process tree is snapshotted before killing so reparented children stay reachable, kills use SIGKILL (a catchable signal lets fm-watch-arm.sh respawn a successor watcher), each kill is gated by pid start-time identity to survive pid reuse and ps failures, and cleanup is trapped on HUP/INT/TERM as well as EXIT so a signalled or timeout(1)-killed shell still tears down. This closes the leak in tests/fm-watcher-lock.test.sh where test_watch_restart_attaches_to_healthy_peer orphaned a real fm-watch-arm.sh --restart + fm-watch.sh pair whenever the suite aborted or hung — the source of the phantom watchers seen in crewmate worktrees.
• tests/fm-watcher-lock.test.sh registers every background launch (including short-lived lock contenders) and adds a regression case that runs first in the suite, covering all three suite endings (clean pass, fail() abort, and TERM); it was verified to fail against a deliberately neutered reaper. The auto-arm eligibility check itself was left untouched — bin/fm-primary-scope-lib.sh already refuses to arm in linked worktrees and its existing tests pass.
• The reaper convention is documented in the firstmate-coding-guidelines skill and the tests/lib.sh header, with tests/wake-helpers.sh adjusted to match.

Risk Assessment

✅ Low: The follow-up commit is tests-only and implements all three requested fixes exactly as instructed (interruptible hang fixture, identity-gated root kills via the production fm_pid_identity primitive through a subshell, and a roots-emitting ps-failure fallback) while preserving every declared design constraint, with the only residual behaviors being rare, fail-open identity-read races that match the chosen safety direction.

Testing

Ran the changed watcher-lock suite via bin/fm-test-run.sh (all cases pass, new reaper regression case first, including the author's reportedly flaky peer-attach case which passed here), then demonstrated the fix end-to-end with a base-vs-branch comparison: a TERM'd wedged suite on the base harness orphans the exact phantom fm-watch-arm.sh + fm-watch.sh pair with the live-incident FM_HOME signature, while the fixed harness reaps every process and exits 128+TERM; a neutered-reaper run confirmed the regression test genuinely fails without the fix. No orphaned processes or temp scaffolding remain. No visual artifacts because this is a test-harness/CLI change with no rendered UI surface — CLI transcripts are the evidence.

Evidence: Full watcher-lock suite run on the branch (exit 0, reaper regression case first)

FM_TEST_BEGIN 2026-07-30T03:18:54Z tests/fm-watcher-lock.test.sh family=watcher-wake-lock expected_gate_skip=none
ok - harness reaps a registered process and its child on a pass, an abort, and a signal
ok - simultaneous watcher starts leave exactly one live process
ok - fm_pid_identity real ps fallback is locale-invariant
ok - fm_pid_identity is locale-invariant across LC_ALL/LC_TIME
ok - /proc process identity ignores simulated btime changes
ok - /proc process identity detects pid reuse
ok - MSYS /proc process identity regression skipped on non-Windows host
ok - killed watcher stale lock is reclaimed
ok - live watcher lock with stale heartbeat is actionable
ok - guard banner leads when down with pending wakes (repair-after-drain) and stays silent when fresh
ok - concurrent fm_lock_try_acquire yields exactly one winner
ok - dead-pid stale lock is reclaimed by a single acquirer
ok - concurrent stale-lock steal yields exactly one winner
ok - live steal mutex is not reclaimed
ok - live-held lock is not stolen
ok - empty mid-acquire lock keeps a minimum grace
ok - late original claimant cannot claim a recreated lock
ok - paused mid-acquire claimant backs off to active stealer
ok - watch restart refuses to signal a reused pid
ok - watch restart attaches to a verified healthy peer and later surfaces a successor gap
ok - watcher self-evicts when the lock pid no longer names it
ok - arm turns clean self-eviction without a successor into a typed failure
ok - arm attaches to a live fresh watcher and fails loudly when that cycle has no successor
ok - attached arm signals record a classified lifecycle entry
ok - arm starts+confirms a fresh watcher on a clean lock and self-heals a dead-pid lock (never healthy off a dead pid)
ok - arm cleans child watcher and temp output on HUP
ok - arm propagates an immediate watcher wake before confirmation
ok - arm attaches to a peer watcher after child stands down and surfaces a missing successor
watcher: lock held by live pid 2493486 but heartbeat is stale for 838678793s (>300s); inspect or stop that watcher before re-arming.
ok - arm reports FAILED and exits non-zero when no fresh watcher can be confirmed
ok - cycle-exit ledger links a verified successor and remains size-capped
ok - SIGSTOP distinguishes live PID from stale beacon and termination records the exit class
FM_TEST_END 2026-07-30T03:20:38Z tests/fm-watcher-lock.test.sh exit=0 duration_ms=103299 gate_skip=false
FM_TEST_SUMMARY total=1 failed=0 skipped_gate=0 duration_ms=103333
FM_TEST_SUMMARY_FAMILY family=watcher-wake-lock count=1 duration_ms=103299 failed=0
FM_TEST_SLOWEST rank=1 script=tests/fm-watcher-lock.test.sh duration_ms=103299

Evidence: Base-commit harness: TERM'd wedged suite orphans the phantom watcher pair

== baseline: suite shell pid=2568598 backgrounded real arm pid=2568614, now wedged == -- sending SIGTERM to the suite shell (what timeout(1) does to a hung suite) -- suite shell exited with status 143 -- processes still alive from this run (tagged) -- 2568614 34 bash /tmp/fm-baseline-harness.yqMgjn/bin/fm-watch-arm.sh 2568626 2568614 bash /tmp/fm-baseline-harness.yqMgjn/bin/fm-watch.sh ORPHANS: 4 process(es) survived the suite -- FM_HOME carried by the orphaned arm (matches the live-incident signature) -- FM_HOME=/tmp/fm-watcher-lock-tests.fAKcHr/restart-healthy-peer

== baseline: suite shell pid=2568598 backgrounded real arm pid=2568614, now wedged ==
-- sending SIGTERM to the suite shell (what timeout(1) does to a hung suite) --
suite shell exited with status 143
-- processes still alive from this run (tagged FM_LEAK_DEMO_MARKER=leakdemo-base-01KYRF4B) --
2568614      34 bash /tmp/fm-baseline-harness.yqMgjn/bin/fm-watch-arm.sh
2568626 2568614 bash /tmp/fm-baseline-harness.yqMgjn/bin/fm-watch.sh
2568741 2568626 sleep 5
2568780      34 sleep 300
ORPHANS: 4 process(es) survived the suite, still polling FM_HOME under /tmp/fm-watcher-lock-tests.*
-- FM_HOME carried by the orphaned arm (matches the live-incident signature) --
FM_HOME=/tmp/fm-watcher-lock-tests.fAKcHr/restart-healthy-peer

Evidence: Fixed harness: identical TERM'd wedged suite is fully reaped

== fixed: suite shell pid=2604013 backgrounded real arm pid=2604061, now wedged == -- sending SIGTERM to the suite shell (what timeout(1) does to a hung suite) -- suite shell exited with status 143 -- processes still alive from this run (tagged) -- (none - every process from the aborted suite was reaped)

== fixed: suite shell pid=2604013 backgrounded real arm pid=2604061, now wedged ==
-- sending SIGTERM to the suite shell (what timeout(1) does to a hung suite) --
suite shell exited with status 143
-- processes still alive from this run (tagged FM_LEAK_DEMO_MARKER=leakdemo-fixed2-01KYRF4B) --
(none - every process from the aborted suite was reaped)

Evidence: Neutered-reaper run proving the regression test is not a tautology (fails immediately)

not ok - clean: registered process (pid 2628696) survived the harness teardown

not ok - clean: registered process (pid 2628696) survived the harness teardown

Evidence: Reproduction scripts for the before/after leak demo

#!/usr/bin/env bash
# Faithful reduction of test_watch_restart_attaches_to_healthy_peer's leak path:
# background a REAL bin/fm-watch-arm.sh, then wedge until timeout(1)-style TERM.
set -u
. "$TESTS_DIR/wake-helpers.sh"
TMP_ROOT=$(fm_test_tmproot fm-watcher-lock-tests)
dir="$TMP_ROOT/restart-healthy-peer"
fakebin="$dir/fakebin"
mkdir -p "$dir/state" "$fakebin"
printf '#!/usr/bin/env bash\nexit 0\n' > "$fakebin/tmux"; chmod +x "$fakebin/tmux"
state="$dir/state"
printf '%s\n' fm-pr-check-migration-scan-v1 > "$state/.pr-check-migration-scan-v1"
printf '%s\n' fm-pr-check-migration-v1 > "$state/.pr-check-migration-v1"
chmod 0600 "$state/.pr-check-migration-scan-v1" "$state/.pr-check-migration-v1"
PATH="$fakebin:$PATH" FM_HOME="$dir" FM_POLL=5 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 \
  "$ROOT/bin/fm-watch-arm.sh" > "$dir/arm.out" 2>&1 &
armpid=$!
# The fixed harness registers the pid at launch; the base harness has no reaper.
if type fm_test_reap >/dev/null 2>&1; then fm_test_reap "$armpid"; fi
i=0
while [ "$i" -lt 100 ]; do
  grep -qE 'watcher: (started|attached) pid=' "$dir/arm.out" 2>/dev/null && break
  sleep 0.1; i=$((i + 1))
done
printf '%s\n' "$armpid" > "$FIXTURE_READY_FILE"
# Wedge, exactly like a hung case a runner stops with TERM.
sleep 300 &
if type fm_test_reap >/dev/null 2>&1; then fm_test_reap "$!"; fi
wait

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info

• 🚨 tests/fm-watcher-lock.test.sh:528 - The hang-mode leg of the new regression test simulates a wedged suite with a foreground sleep 300, but bash defers a trapped TERM until the current foreground command completes (verified empirically: a bash -c 'trap "exit 143" TERM; sleep 300' process stays alive after TERM). The leg only passes if the outer kill -TERM lands while the fixture is still inside its 0.1s poll loop; if the fixture has already dropped into sleep 300 — which happens whenever the kid pidfile is written before the fixture's first poll check, and is more likely under full-suite load — the trap is deferred ~300s, wait_for_exit (tests/wake-helpers.sh:260) polls 8s then blocks in wait for the remaining ~292s, and returns 124 instead of 143, failing the assertion at line 553. Because this test deliberately runs first, the losing side of the race stalls the suite ~5 minutes and then aborts every subsequent case. Fix: make the wedge interruptible, e.g. hang) sleep 300 & fm_test_reap "$!"; wait ;; — the wait builtin returns immediately on a trapped signal, making the TERM leg deterministic (and the extra sleep is reaped by the fixture's own cleanup).
• ⚠️ tests/lib.sh:138 - FM_TEST_CLEANUP_PIDS accumulates every registered pid for the suite's whole lifetime and pids are never deregistered, even after a test explicitly waits them (e.g. the lock-contender loops). At teardown fm_test_kill_tree unconditionally SIGKILLs each stale pid and also treats it as a tree root, so after pid wraparound (macOS pid space is ~99999; Linux pid_max is often 32768) a recycled pid can belong to an unrelated same-user process, which then gets SIGKILLed along with its descendants. The repo already has a pid-identity primitive (fm_pid_identity, used by test_watch_restart_rejects_reused_pid) that could gate the kill, or fm_test_kill_tree could skip pids whose start-time identity changed since registration. Flagging for the author because the intent declares registering short-lived contenders "for uniformity" a deliberate choice.
• ⚠️ tests/lib.sh:113 - fm_test_process_tree bails out with empty output when ps -axo pid=,ppid= fails (|| return 0), so fm_test_kill_tree kills nothing at all — not even the registered root pids that the caller knows are valid. On platforms whose ps lacks BSD -o output selection (MSYS/Cygwin ps, some busybox builds — and this very suite carries an MSYS-specific case, test_msys_pid_identity_uses_proc), the reaper silently no-ops, reintroducing the leak there, and the new first-position regression test would then fail the entire suite on those platforms because assert_reaped finds the fixture parent still alive. Degrade gracefully by emitting the given root pids when the ps read fails, so at least the registered processes are killed.
• ℹ️ tests/lib.sh:85 - fm_test_cleanup_on_signal runs trap - EXIT before exiting, so a test file that follows the pattern documented in this same header — define its own EXIT trap that does extra teardown (e.g. cleanup_all killing tmux sessions in the e2e suites) and calls fm_test_cleanup from inside — gets that extra teardown silently skipped on the signal path; only lib-registered dirs/pids are torn down. This is not a regression (previously nothing ran on a signalled shell) and fm_test_cleanup is idempotent per its own comment, so leaving the EXIT trap installed and simply exiting would run the file's custom teardown too at the cost of a harmless double cleanup. Noting as a design tradeoff.

🔧 Fix: gate reaper kills by identity and survives ps loss
1 info still open:

• ℹ️ tests/lib.sh:96 - Residual fail-open race in the identity gate: fm_pid_identity's identity includes the full /proc cmdline (or ps command=), and fm_test_reap reads it immediately after fork - if the identity read (itself ~10ms: fork bash, source bin/fm-wake-lib.sh) ever completes before the just-forked child execs its command, the recorded pre-exec bash cmdline can never match the teardown re-read, so that process is skipped and leaks. The same design makes the reaper a full no-op on a platform with neither a readable compatible /proc nor a BSD-capable ps (identity reads fail, all roots skipped). Both are deliberate consequences of the instructed governing property (never kill what cannot be proven ours), fail in the safe direction, and the realistic platform set (Linux, macOS, MSYS/Cygwin with /proc) is covered - noting the tradeoff only.

✅ **Test** - passed

✅ No issues found.

• bin/fm-test-run.sh tests/fm-watcher-lock.test.sh — full changed suite on the branch: exit 0, new reaper regression case runs first and passes (clean/abort/TERM endings), and a post-run /proc environ scan with a unique marker env var confirmed zero surviving background processes
• Manual leak reproduction on base commit a2d5f26: materialized base tests/ with shared bin/, ran a faithful reduction of test_watch_restart_attaches_to_healthy_peer (real backgrounded bin/fm-watch-arm.sh, then wedged), sent SIGTERM like timeout(1) — orphaned fm-watch-arm.sh + fm-watch.sh survived, carrying the incident's FM_HOME=/tmp/fm-watcher-lock-tests.*/restart-healthy-peer signature
• Same manual scenario against the fixed harness at HEAD: suite shell exited 143 (128+TERM per the new signal cleanup contract) and zero tagged processes survived
• Non-tautology check: neutered fm_test_kill_tree to a no-op in a temp copy of HEAD's tests and re-ran the suite — the new regression case immediately failed with 'registered process survived the harness teardown' (exit 1)
• Post-test hygiene: killed all marker-tagged demo/neutered-run leftovers without touching other lanes' live fm-watch processes, removed temp harness dirs, verified worktree clean

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.