test(watcher): de-flake and leak-proof the watcher-lock suite - #904
Open
mattwwatson wants to merge 3 commits into
Open
test(watcher): de-flake and leak-proof the watcher-lock suite#904mattwwatson wants to merge 3 commits into
mattwwatson wants to merge 3 commits into
Conversation
The watcher-lock suite raced its peer processes (peers were assumed started without confirmation) and leaked background children on failure exits, which could hang the suite's output pipe and hold a temp home's watcher lock. De-flake it at the root: exec-confirmed peer starts, durable-file synchronization instead of sleeps, a self-checking bash peer, and an injectable FM_ARM_CONFIRM_TIMEOUT. Leak-proof the harness with file-backed pid and temp registries in tests/lib.sh plus a home-scoped reaper (fm_test_track_pid), and document the convention in CONTRIBUTING.md: nothing is ever reaped by process-name pattern.
mattwwatson
force-pushed
the
up/watcher-lock-deflake
branch
from
July 24, 2026 19:32
0a0d42c to
de7c31a
Compare
Owner
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for firstmate#904 at |
kunchenguid
added
the
wheelhouse:pending-contributor-action
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Upstream contribution from the mattwwatson/firstmate fork: de-flake and leak-proof the watcher-lock test suite at the root. Peer watcher starts are now exec-confirmed with durable-file synchronization instead of sleeps, a self-checking bash peer, and an injectable FM_ARM_CONFIRM_TIMEOUT; tests/lib.sh gains file-backed pid and temp registries with a home-scoped reaper (fm_test_track_pid) so no failure path leaks background processes or holds a temp home's watcher lock, and CONTRIBUTING.md documents the convention (nothing reaped by process-name pattern). The CONTRIBUTING.md paragraph was hand-merged onto the current upstream testing-docs rewrite, keeping upstream's fm-test-run.sh section intact and appending only the two new convention sentences. Suite passes 28/28 locally on the upstream base.
What Changed
FM_ARM_CONFIRM_TIMEOUT, and anfm-watch-arm.shtweak that surfaces the finished child's stand-down output before the successor-confirmation wait so tests can synchronize on it.tests/lib.sh(stored in a privatemktemp -ddirectory) with a home-scoped reaper viafm_test_track_pid, so failing test paths no longer leak background processes or hold a temp home's watcher lock — nothing is reaped by process-name pattern. Suites that install custom EXIT traps now drain these registries in their own cleanup.fm-test-run.shtesting section. Suite verified 28/28 across three consecutive runs in the pipeline, including its built-in no-leaked-process/no-held-lock audit.Risk Assessment
✅ Low: The final commit cleanly closes the round-2 registry-dir leak by adding idempotent, exit-status-preserving fm_test_cleanup calls to the exact three non-conforming suites, and the full branch now satisfies every stated intent criterion with no outstanding source findings.
Testing
Ran the watcher-lock suite three times through the canonical runner (28/28 each, no flakes), ran the three other touched suites (pass/clean self-skip), and demonstrated the leak-proofing end-to-end with a fail()-aborted real-arm demo (children reaped, lock released, 5s pipe EOF) against a pre-change contrast that hung the pipe 76s; before/after audits show zero leaked processes or temp dirs and the user's real watcher untouched. No UI surface — this is CLI/test infrastructure, so transcripts are the reviewer-visible evidence.
Evidence: watcher-lock suite run 1 transcript (28/28 ok)
Evidence: watcher-lock suite run 2 transcript (28/28 ok)
Evidence: watcher-lock suite run 3 transcript (28/28 ok)
Evidence: Fail-path leak demo transcript — tracked arm+watcher reaped after mid-case fail(), lock released, 5s pipe EOF
demo exit=1 (expected nonzero: fail() aborted the case) seconds-to-EOF=5 (a leaked child would hold the pipe open and hang this read) arm pid 65278: gone (reaped) watcher grandchild pid 65293: gone (reaped) temp root .../fm-leak-demo.lDnZPj: removed (watcher lock released with it)Evidence: Pre-change contrast transcript — untracked children held the output pipe 76s past the abort
Evidence: Fail-path demo script (replicates a watcher-lock case with the new registry convention)
Evidence: Other touched suites transcript (tmux 4/4 ok; herdr e2e clean opt-in skips)
Evidence: Leak audit: TMPDIR fm-* dirs and watcher processes before/after all runs (zero suite leftovers)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (2) ✅
tests/lib.sh:85- The file-backed registries use predictable world-tmp paths (${TMPDIR:-/tmp}/fm-test-cleanup.$$andfm-test-pids.$$), and fm_test_cleanup laterrm -rfs every path read back from the cleanup log (guarded only against empty and/). On a multi-user machine another user can pre-create or symlink those names — sticky-bit /tmp even makes the source-timerm -fof a foreign-owned planted file fail silently — letting planted content steer therm -rf. The in-code rationale for rejecting mktemp (that an assignment would die inside a command-substitution subshell) does not apply at source time: these variables are assigned when lib.sh is sourced in the main shell, where a mktemp'd path would be equally visible to every subshell; the only real trade-off is the documented lazy-creation/no-stray-file property. Consider a mktemp'd per-process registry directory created at source time (fixed filenames inside, created lazily), which keeps laziness for the files while removing name predictability. Flagged ask-user because the current scheme is explicitly documented as deliberate.bin/fm-watch-arm.sh:418- A commit scopedtest(watcher):changes production behavior in bin/fm-watch-arm.sh: the arm now prints its finished child's output (and deletes the temp file) before spending the successor-confirmation window, so the child's stand-down explanation surfaces up to FM_ARM_CONFIRM_TIMEOUT seconds earlier. I verified the change is safe (wait_for_healthy_successor/healthy_watcher never read child/child_out; the signal handler behaves identically since the child is already reaped on this path; relative stdout order and cycle-ledger timing are unchanged) and it is load-bearing for the new durable-file synchronization in test_arm_waits_for_peer_beacon_after_child_stands_down, which greps the arm's own output for the stand-down line before the beacon exists. Only noting that the commit-type label understates a production output-timing change.🔧 Fix: harden test registries with private mktemp dir
1 info still open:
tests/lib.sh:85- The round-1 hardening makes the registry directory eager: sourcing tests/lib.sh now always runsmktemp -d(tests/lib.sh:85), so any suite that sources lib.sh but overrides the EXIT trap without calling fm_test_cleanup leaks one small empty registry dir per run. Exactly three suites do this: tests/fm-tmux-submit-busy.test.sh (trap 'rm -rf "$TMP_ROOT"' EXIT), tests/fm-afk-pi-herdr-return-e2e.test.sh (custom cleanup that rm -rfs its own TMP_ROOT but never drains the registries — it already leaked the lazy cleanup-log file under the previous commit since it uses fm_test_tmproot), and tests/fm-send-secondmate-marker-herdr-e2e.test.sh. All three violate the trap convention this branch documents in lib.sh and CONTRIBUTING.md, and the leak is bounded to a tiny OS-reaped tmp dir. Making their traps call fm_test_cleanup is a mechanical follow-up, but it would expand this focused upstream-contribution branch to unrelated suites — asking rather than auto-fixing; deferring entirely is also reasonable.🔧 Fix: drain test registries in suites with custom EXIT traps
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-watcher-lock.test.sh×3 consecutive runs — 28/28 ok each, exit=0, including the suite's built-in no-leaked-process/no-held-lock audit casebin/fm-test-run.sh tests/fm-tmux-submit-busy.test.sh tests/fm-afk-pi-herdr-return-e2e.test.sh tests/fm-send-secondmate-marker-herdr-e2e.test.sh— tmux suite 4/4 ok; both herdr e2e suites self-skip cleanly behind their opt-in env gates with the new fm_test_cleanup trap line in placeFail-path leak demo: real fm-watch-arm.sh + watcher grandchild registered via fm_test_track_pid, aborted mid-case with fail() — verified both pids reaped, temp home watcher lock released, and suite output pipe reached EOF in 5sPre-change contrast demo (untracked pid, dirs-only EXIT trap) — reproduced the old symptom: output pipe held 76s past the abort by lingering arm/watcher before driver cleanupLeak audit: diffed TMPDIR fm-* dirs and fm-watch/fm-watch-arm processes before vs after all runs — zero suite leftovers, TMPDIR restored to exact baseline, user's real-home watcher untouchedReviewed CONTRIBUTING.md diff — two convention sentences appended after upstream's intact fm-test-run.sh testing-docs section✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.