Skip to content

fix(wake): harden test cleanup and wake-queue locking against tmp leaks and filesystem failures#6

Merged
Freudator86 merged 4 commits into
mainfrom
fm/fm-tmp-leak-cleanup
Jul 21, 2026
Merged

fix(wake): harden test cleanup and wake-queue locking against tmp leaks and filesystem failures#6
Freudator86 merged 4 commits into
mainfrom
fm/fm-tmp-leak-cleanup

Conversation

@Freudator86

Copy link
Copy Markdown
Owner

Intent

Fix the fleet-wide /tmp quota exhaustion hazards reported from repeated firstmate test runs. Ensure tests/lib.sh temp roots are registered in the actual caller shell and cleaned on normal exit plus catchable HUP, INT, and TERM interruption without breaking test-owned EXIT cleanup; document that SIGKILL is structurally untrappable, and migrate every shared-helper caller rather than narrowly patching the surfaced suites. Make wake-queue locking distinguish genuine live/stale contention from operational filesystem failures such as ENOSPC or EDQUOT, return promptly and loudly on those failures, impose a hard steal-recursion bound, preserve legitimate stale-lock stealing, and keep all production callers fail-closed. Add deterministic regressions for interrupted cleanup, failed lock publication, bounded recursion, and queue-write refusal, while preserving all existing tests and the pinned lint gate.

What Changed

  • Reworked tests/lib.sh temp-root handling (fm_test_tmproot, cleanup traps) so temp roots register in the actual caller shell and are removed on normal exit and catchable HUP/INT/TERM (documenting that SIGKILL cannot be trapped), without clobbering test-owned EXIT traps; namespaced its internal locals to avoid colliding with caller-chosen variable names, and migrated every test/helper file (tests/*.test.sh, tests/wake-helpers.sh) from ad hoc temp-dir handling onto the shared helper.
  • Hardened bin/fm-wake-lib.sh locking: fm_lock_try_create/fm_lock_try_acquire/fm_lock_acquire_wait now distinguish genuine live/stale lock contention (rc 1) from operational filesystem failures such as ENOSPC/EDQUOT (rc 2, surfaced via FM_LOCK_ERROR), added a new FM_LOCK_STEAL_MAX_DEPTH hard bound on steal recursion, and made fm_wake_append return promptly on filesystem failure instead of looping.
  • Updated production lock callers (bin/fm-guard.sh, bin/fm-supervise-daemon.sh, bin/fm-watch.sh, bin/fm-afk-return.sh, bin/fm-wake-drain.sh, bin/fm-pr-check-migrate.sh) to stay fail-closed on the new failure return codes.
  • Added regression coverage for interrupted cleanup, output-var name collisions, failed lock publication, bounded steal recursion, and queue-write refusal on lock failure (tests/fm-test-lib.test.sh, tests/fm-watcher-lock.test.sh, tests/fm-wake-queue.test.sh), and documented FM_LOCK_STEAL_MAX_DEPTH in docs/configuration.md.

Risk Assessment

✅ Low: The fix is a minimal, targeted change confined to one test function that removes the leaking subshell and instead declares a plain local variable in the caller's own shell, verified by direct execution to pass all three tests in the file with zero leftover temp directories, and it does not touch any other file or behavior.

Testing

Baseline full e2e suite already passed per harness; I additionally ran the three test files most relevant to the stated intent (tests/fm-test-lib.test.sh, tests/fm-wake-queue.test.sh, tests/fm-watcher-lock.test.sh) plus tests/fm-lint.test.sh directly and captured their full pass/fail transcripts as evidence — every case passed, including the new deterministic regressions for interrupted cleanup (TERM), failed lock publication (ENOSPC/EDQUOT-style fakebin failure), bounded steal recursion, and queue-write refusal on lock failure. No findings.

Evidence: Targeted regression test transcript (tmproot cleanup, wake-queue lock failure, watcher-lock fail-closed/steal-depth-bound)
== tests/fm-test-lib.test.sh (new regression suite for tests/lib.sh cleanup hardening) ==
ok - caller-shell temp-root registration cleans on normal exit
ok - TERM preserves a test-owned EXIT trap and cleans its registered root
ok - fm_test_tmproot assigns a caller variable named 'root' without collision

== tests/fm-wake-queue.test.sh (includes new lock-publication-failure regression) ==
ok - wake append returns promptly and writes nothing when lock publication fails
ok - concurrent append plus drain preserves queue records
ok - signal written while no watcher runs is caught on next run
ok - stale wake is queued before suppressor state is advanced
ok - a not-provably-working stale wake is queued before its suppressor is advanced
ok - registered custom check output is queued before cadence suppression
ok - two atomic drains cannot consume the same records twice
ok - drain collapses obvious duplicate heartbeat and signal records
ok - drain asserts watcher liveness: warns on a lapse, stays silent right after a fire

== tests/fm-watcher-lock.test.sh (includes new fail-closed + steal-depth-bound regressions) ==
ok - simultaneous watcher starts leave exactly one live process
ok - fm_pid_identity is locale-invariant across LC_ALL/LC_TIME
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 (re-arm-after-drain) and stays silent when fresh
ok - lock publication failure returns promptly without entering steal recursion
ok - lock steal recursion stops at its configured hard depth bound
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 reports a healthy peer without attaching to it
ok - watcher self-evicts when the lock pid no longer names it
ok - arm attaches to a live fresh watcher and exits only when that cycle ends
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 exits when peer dies
watcher: lock held by live pid 1069091 but heartbeat is stale for 837911610s (>300s); inspect or stop that watcher before re-arming.
ok - arm reports FAILED and exits non-zero when no fresh watcher can be confirmed

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (2) ✅
  • ⚠️ tests/lib.sh:88 - fm_test_tmproot() in tests/lib.sh declares its own locals output_var, prefix, and root in the same call frame it writes to via printf -v "$output_var". Bash resolves that name dynamically against the innermost active scope, which is fm_test_tmproot's own frame. If a caller ever names its output variable root, prefix, or output_var (e.g. fm_test_tmproot root fm-foo), printf -v silently reassigns fm_test_tmproot's own local root instead of the caller's variable, leaving the caller's variable unset. Verified locally under set -u this raises "root: unbound variable" at the caller's next use. No current caller uses these names (all use TMP_ROOT/FM_ROOT_OVERRIDE/tmp/dir/scratch), so this is latent, but it is a natural name choice for future test authors and the failure mode is a confusing indirect error far from the real cause. Fix by renaming the function's internal locals to a namespaced form so they cannot collide with a caller-chosen output variable name.

🔧 Fix: fix: namespace fm_test_tmproot locals to avoid caller-var collisions
1 warning still open:

  • ⚠️ tests/fm-test-lib.test.sh:78 - The new regression test test_output_var_named_root_is_assigned wraps fm_test_tmproot in an explicit ( ... ) subshell to get a local variable literally named 'root'. Verified empirically: bash does not run a parent-installed EXIT trap when an explicit subshell exits, so fm_test_install_cleanup_traps's 'trap fm_test_cleanup EXIT' never fires for the subshell, and the FM_TEST_CLEANUP_DIRS entry added inside the subshell only exists in the subshell's forked copy of the array -- it is discarded when the subshell exits and is never seen by the outer test process's own cleanup. Net effect: every run of this test permanently leaks one 'fm-test-lib-collide.XXXXXX' temp directory in $TMPDIR, reproduced directly (mkdir'd dir survives both the subshell's exit and the outer script's own EXIT-trap cleanup). This is exactly the class of recurring /tmp leak from repeated test runs that this entire branch sets out to fix, introduced by the fix's own new test. Since the underlying local-name collision this subshell was working around is already fixed by namespacing fm_test_tmproot's internals to fm_test_tmproot*, the subshell is no longer needed: dropping it (declare local root in the test function and call fm_test_tmproot directly, letting it register in the already-established outer FM_TEST_CLEANUP_DIRS/trap) would satisfy the same assertions without leaking.

🔧 Fix: fix: drop leaking subshell in fm_test_tmproot collision regression
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"
  • bash tests/fm-test-lib.test.sh — new caller-shell tmproot registration, TERM-preserves-owned-EXIT-trap, and output-var-collision regressions, all pass
  • bash tests/fm-wake-queue.test.sh — includes new test_append_returns_on_lock_filesystem_failure regression (fakebin ln forced to fail with rc 74), verifies fm_wake_append returns 2 promptly and writes nothing; all 9 cases pass
  • bash tests/fm-watcher-lock.test.sh — includes new test_lock_create_filesystem_failure_returns_without_steal_recursion and test_lock_steal_recursion_has_a_hard_depth_bound regressions; all 24 cases pass, including pre-existing stale-lock steal/no-steal and concurrency tests
  • bash tests/fm-lint.test.sh — confirms the pinned ShellCheck lint gate is preserved and still catches real defects
  • git status --porcelain before and after — confirmed no leftover artifacts in the working tree from test runs
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@Freudator86
Freudator86 merged commit e3ad719 into main Jul 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants