fix(wake): harden test cleanup and wake-queue locking against tmp leaks and filesystem failures#6
Merged
Merged
Conversation
…aller-var collisions
…llision regression
…var to configuration reference
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
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
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
tests/lib.shtemp-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.bin/fm-wake-lib.shlocking:fm_lock_try_create/fm_lock_try_acquire/fm_lock_acquire_waitnow distinguish genuine live/stale lock contention (rc 1) from operational filesystem failures such as ENOSPC/EDQUOT (rc 2, surfaced viaFM_LOCK_ERROR), added a newFM_LOCK_STEAL_MAX_DEPTHhard bound on steal recursion, and madefm_wake_appendreturn promptly on filesystem failure instead of looping.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.tests/fm-test-lib.test.sh,tests/fm-watcher-lock.test.sh,tests/fm-wake-queue.test.sh), and documentedFM_LOCK_STEAL_MAX_DEPTHindocs/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)
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 (declarelocal rootin 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 passbash tests/fm-wake-queue.test.sh — includes newtest_append_returns_on_lock_filesystem_failureregression (fakebin ln forced to fail with rc 74), verifies fm_wake_append returns 2 promptly and writes nothing; all 9 cases passbash tests/fm-watcher-lock.test.sh — includes newtest_lock_create_filesystem_failure_returns_without_steal_recursionandtest_lock_steal_recursion_has_a_hard_depth_boundregressions; all 24 cases pass, including pre-existing stale-lock steal/no-steal and concurrency testsbash tests/fm-lint.test.sh — confirms the pinned ShellCheck lint gate is preserved and still catches real defectsgit 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.