fix(supervision): make signal handling deadlock-safe in watcher/daemon - #7
Merged
Merged
Conversation
…the watcher Firstmate's locks are not reentrant, and the long-running supervision processes trap HUP/INT/TERM into an exit whose cleanup re-enters those same locks. A signal delivered mid-section unwound the shell out of a critical section and straight back into it, producing a watcher that held .watch.lock and .wake-queue.lock with a frozen beacon until the session was restarted. Make the section uninterruptible rather than the unwind survivable: record the signal, finish the short section, release, then re-raise it against the caller's own disposition. Deferral is opt-in per section, so a long-held ownership lock cannot turn into a TERM-immune process, and a nested acquisition inside a deferred section becomes bounded once a signal is pending so shutdown never waits on another process's lifetime. Back it with a liveness bound: watcher cleanup and the away-mode daemon's child reap now terminate loudly instead of blocking forever.
…nd footgun Restoring the caller's signal disposition by clearing first left a window, hit on every section exit, where HUP/INT/TERM carried their default action; a signal landing there killed the process with no EXIT trap and its locks still held. Overwrite each trap instead, and clear only signals the caller had no handler for. Stop seeding the acquire bound from the environment: an exported value would bound every lock wait in the process, and the callers that ignore the return value would then proceed without the lock they believe they hold. Record the signal-safety contract and its regression pointer in the watcher continuity owner.
An audit of every trap-registered handler in bin/ - rather than a reading pass - turned up two more instances of the same shape the watcher had: - fm-pr-check-migrate.sh traps into a cleanup that re-enters the recovery-marker lock while holding the watch lock, so it gets the same bound and keeps its existing stale-lock-evidence outcome. - fm-watch-arm.sh's signal handler stopped its child with an unbounded wait, so the handler that exists to stop the arm could itself hang on a child that was slow to go. The bounded child stop now has one owner in fm-wake-lib.sh instead of a copy per caller. Everything else the audit listed only releases locks in cleanup, which never blocks, and is genuinely unaffected.
…stop_bounded liveness check"}
…masked by log()"}
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
Redesign watcher signal handling so a signal landing mid-critical-section can never self-deadlock the watcher.
This is a firstmate-repo task on supervision-core code, so the firstmate-coding-guidelines apply throughout, including their deterministic-enforcement and harness-dependent-check sections in full.
ROOT CAUSE. Fleet locks are non-reentrant, and bin/fm-watch.sh traps HUP/INT/TERM as
exit 1, which unwinds into watcher_cleanup, which re-acquires locks the interrupted flow still holds. Reachable on EVERY poll through _fm_recovery_marker_arm_check (resurface_after_downtime), not only through fm_wake_append. Observed in the wild as a watcher whose own pid held .wake-queue.lock and .watcher-down.lock with a frozen beacon for 64 minutes. Upstream kunchenguid#2251 reports the same symptom with no fix; kunchenguid#2270 (afk daemon SIGTERM shutdown hang) is the same root cause on the daemon path, so the fix had to cover both surfaces.REQUIRED DELIVERABLES. (1) The redesign, applied to fm-watch.sh AND every other surface sharing the trap-unwind pattern, explicitly auditing the afk daemon bin/fm-supervise-daemon.sh and any other loop that traps into cleanup while holding locks - no tunnel vision on the watcher alone. (2) A portable regression in tests/ that drives a real signal into the critical section deterministically and proves no deadlock, plus TERM-still-stops-the-watcher coverage, extending existing runner patterns with .test.sh naming. (3) Removal of the quarantine skip the earlier stale-reads change placed on the hanging watch-triage case (tracking name watcher-signal-deadlock-redesign), with its full assertion restored and proven passing. (4) bin/fm-lint.sh clean.
DESIGN CHOICE AND WHY. Two options were pre-scoped and neither was to be inherited blindly: (1) a global deferred-signal trap in fm-watch.sh with per-poll safe points, correct in shape but risking a TERM-immune watcher if a safe point is missed; (2) an ownership-aware fm_lock_acquire_wait, small but requiring an audit of all ~65 callers because ~20 ignore its return value and would silently unlock spawn/teardown/drain critical sections. A hybrid or third design was acceptable if shown to dominate both.
I chose a hybrid and it must be judged as one: make the critical SECTION uninterruptible rather than make the unwind survivable, plus a separate liveness bound so shutdown can never block.
DELIBERATE DECISIONS A REVIEWER CANNOT SEE IN THE DIFF.
KNOWN PRE-EXISTING ISSUES DELIBERATELY LEFT TO FOLLOW-UP, NOT REGRESSIONS FROM THIS CHANGE: tests/fm-watch-triage.test.sh's absorb gates are fixed 3-second wall-clock slices (wait_live), which the harness's own wait_watcher_beat comment warns against; under heavy load they flake, measured alternating in isolated checkouts at load ~20 as 2/2 failures on this branch AND 2/2 on origin/main. Converting ~26 gates changes what those cases assert and belongs in its own task rather than inside a supervision-core signal fix.
DELIVERY. PRs go to origin zeeshaanahmad/firstmate (a fork); kunchenguid/firstmate is fetch-only upstream and must never be pushed to. The ci step is skipped because the fork has zero Actions runs and checks never register: bin/fm-ci-probe.sh with the fork named explicitly returns "none". Note its no-argument path returns "present" here because
gh repo viewresolves to the upstream parent rather than origin - a separate defect being reported, not a reason to let ci poll for a check that can never arrive. Local verification stands in for CI and is recorded in the PR body.The PR body must include a Local verification section with exact commands, the deterministic red-then-green deadlock-reproduction evidence, the TERM-stops proof, the restored-quarantine proof, all measured against the pushed head, and an explicit list of what was not run and why.
What Changed
bin/fm-wake-lib.sh(fm_signal_defer_begin/end,fm_lock_section_enter/leave) that holds HUP/INT/TERM during a recovery-marker or wake-queue critical section and re-raises the signal against the caller's own disposition once the section closes, so a signal can no longer unwind intowatcher_cleanupmid-section and re-acquire a lock the interrupted flow still holds._fm_recovery_marker_arm_check(reachable on every watcher poll) and the other recovery-marker helpers infm-wake-lib.shnow use these deferred sections instead of rawfm_lock_acquire_wait/fm_lock_release.FM_LOCK_ACQUIRE_WAIT_TICKS) onfm_lock_acquire_wait, and a sharedfm_child_stop_bounded/_fm_child_is_livehelper for bounded TERM-then-KILL child shutdown. Applied the bound towatcher_cleanupinbin/fm-watch.shand to the recovery-marker release inbin/fm-pr-check-migrate.sh'smigration_cleanup(both trap-unwind-into-cleanup surfaces), and applied the bounded child stop tobin/fm-supervise-daemon.sh's newstop_watcher_childand tobin/fm-watch-arm.sh'shandle_arm_signal, replacing their unboundedkill; waitshutdown of the watcher child.FM_SIGNAL_DEADLOCK_SKIPquarantine intests/wake-helpers.shwith anassert_reaped_on_termhelper that turns a TERM-ignoring reap into a named test failure instead of a silent skip, updatedtests/fm-liveness-source.test.shandtests/fm-watch-triage.test.sh(restoring its previously-skipped assertions) to use it, gated the subshell-reclaim half oftests/fm-wake-queue.test.shonBASHPIDavailability, and addedtests/fm-watcher-signal-safety.test.shas a new regression suite (registered inbin/fm-test-run.sh) covering deferred-section signal handling, bounded watcher/daemon shutdown, and TERM-still-stops-the-watcher behavior with real processes and signals.docs/watcher-continuity.md.Risk Assessment
✅ Low: Both prior-round findings (zombie/kill-0 liveness race, and stop_watcher_child's return code being masked by log()) are correctly and completely fixed on independent re-inspection; the core signal-deferral mechanism (fm_signal_defer_begin/end, fm_lock_section_enter/leave, the caller-scoped fm_lock_acquire_wait bound) is internally consistent across every call site with no depth-counter leaks or unpaired enter/leave, all four required deliverables from the user intent are present and verifiable in source (redesign applied to all four trap-unwind surfaces, a real-process/real-signal regression suite, the quarantine skip fully replaced by named assert_reaped_on_term failures, and a clean bin/fm-lint.sh run), and no forbidden behavior was introduced.
Testing
Ran the new tests/fm-watcher-signal-safety.test.sh (5/5 pass) which drives real signals into a real watcher pinned inside its critical section by an external lock holder, and separately reproduced the pre-fix deadlock (not ok) against the base commit's code in an isolated scratch checkout to establish red-then-green evidence; also ran fm-liveness-source, fm-watch-triage (including both restored-quarantine cases), fm-wake-queue, fm-watch-arm, and fm-daemon suites, all passing with no failures, orphaned processes, or worktree artifacts.
Evidence: Red-then-green deadlock reproduction transcript
Evidence: Full fm-watch-triage.test.sh run showing both restored-quarantine assertions passing
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
bin/fm-wake-lib.sh:489- fm_child_stop_bounded's poll loop (and its post-loop escalation check) treats an unreaped zombie child as still alive, becausekill -0 "$pid"succeeds for a zombie (its process-table entry persists until the parent calls wait(), which this function only does at the very end, after both the poll loop and the escalation decision). A child that exits immediately on TERM but hasn't been reaped yet will make the loop spin the fulllimitticks and then incorrectly set escalated=1 and send a needless KILL. The codebase already has a dedicated workaround for exactly this pattern - tests/wake-helpers.sh:402'sis_live_non_zombie(checksps -o stat=forZ) - used precisely because a plain kill -0 poll-for-child-exit loop is known to misreport zombies as live. This new production helper (shared by bin/fm-supervise-daemon.sh's stop_watcher_child and bin/fm-watch-arm.sh's handle_arm_signal) reintroduces the naive check instead of reusing that convention.🔧 Fix: {"summary": "Fix zombie/kill-0 race in fm_child_stop_bounded liveness check"}
1 warning still open:
bin/fm-supervise-daemon.sh:1368- stop_watcher_child'sfm_child_stop_bounded ... || log "warn: ..."makes the wrapper's own return code equal to log()'s return code whenever an escalation happens, not fm_child_stop_bounded's. log() is[ -n "${LOG:-}" ] && printf ... >> "$LOG"(line 1343), which returns 0 whenever $LOG is set (the normal daemon runtime) - so stop_watcher_child silently reports success (rc=0) even after a KILL escalation, breaking the '0 when TERM alone sufficed, 1 when escalation was needed' contract that fm_child_stop_bounded's own header documents for exactly this kind of caller ('a caller with somewhere to log can say which happened'). It only happens to return 1 today when $LOG is unset, which is coincidental, not the contract working. Currently harmless - fm_super_main (line 1498) never inspects stop_watcher_child's return value, and the warn log line itself still fires correctly since that happens inside the || regardless - but it is a latent trap for any future caller that does check the return value. The new daemon test in tests/fm-watcher-signal-safety.test.sh (test_daemon_child_stop_is_bounded_against_a_term_ignoring_child, added in this fix round) only asserts rc==0 for the non-escalating/ordinary-child branch; it never asserts what rc is for the TERM-ignoring/escalating branch, so this masking would not be caught by the new regression coverage. Mechanical fix: capture fm_child_stop_bounded's status explicitly (e.g.local rc=0; fm_child_stop_bounded "$pid" "..." || { rc=1; log "warn: ..."; }; return "$rc") instead of letting log()'s own status leak through.🔧 Fix: {"summary": "Fix stop_watcher_child return code masked by log()"}
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-watcher-signal-safety.test.sh(target commit) — all 5 cases pass, including the real-process deadlock reproduction and TERM-from-every-position sweepbash tests/fm-watcher-signal-safety.test.shre-run against base commit 6d074eca5c7dc5b148db1db9df595a5115d2c78b's bin/fm-watch.sh and bin/fm-wake-lib.sh in an isolated /tmp scratch checkout — fails withnot ok - a watcher signalled inside a contended critical section never exited (self-deadlock), reproducing the reported wild symptom (red side of red/green)bash tests/fm-liveness-source.test.sh— confirms assert_reaped_on_term fires as a named failure over a real TERM-ignoring processbash tests/fm-watch-triage.test.sh— full suite passes, including both restored-quarantine cases (test_nonterminal_stale_paused_absorbed_then_resurfaced,test_exited_declared_pause_is_bounded_but_live_gate_surfaces)bash tests/fm-wake-queue.test.sh— passes, including the BASHPID-gated subshell reclaim case on this machine's Bash 3.2bash tests/fm-watch-arm.test.sh— passes (covers the arm-signal handler's shared fm_child_stop_bounded path)bash tests/fm-daemon.test.sh— passes (covers daemon shutdown surfaces)Manual grep confirming no remaining references to the removed FM_SIGNAL_DEADLOCK_SKIP quarantine anywhere in the treeps auxchecks and worktreegit statusafter each run confirming no orphaned processes or transient artifacts were left behind✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Local verification
All results below are measured against the pushed head
12484d739da833dc1a18c117a176a0eb353e3c8c, on macOS 26.4.1 (arm64),/bin/bash3.2.57, ShellCheck 0.11.0 (pinned).Commands and results
232 assertions, 0 failures.
fm-watch-triagewas confirmed green on two independent runs.fm-watcher-signal-safety,fm-daemonandfm-watch-armwere re-run against the pushed head after the pipeline's two fix commits landed, since those commits changed the shared bounded-stop helper.Deterministic red-then-green
Each case was run with
bin/at the merge base6d074ecandtests/from this branch, in an isolated clone, then against this branch. Determinism comes from an external process holding the recovery-marker lock, which pins the watcher inside the contended section rather than racing a signal against a sub-millisecond section.6d074ecnot ok - ...unwound the process instead of being heldoknot ok - ...never exited (self-deadlock)okok(control)oknot ok - shutdown blocked indefinitely on a marker lock held by another live processoknot ok - daemon shutdown left a TERM-ignoring child aliveokThe third row is the row that judges the design: it passes on both sides. That is the guard against the mirror-image defect - the deferral did not buy correctness by making the watcher harder to stop.
TERM-stops proof, specifically
test_term_stops_the_watcher_from_every_loop_positionstarts a fresh watcher per round on its own state (a stopped watcher leaves a downtime marker that makes the next one resurface and exit early, which would quietly hollow out the sweep), waits for a completed poll so the offset is measured from inside the loop with the exit path installed, then delivers TERM at ten offsets that do not divide evenly into the 0.2s poll. Every round must exit within the bound and leave no.watch.lockbehind, so a re-arm can always take over.Restored-quarantine proof
FM_SIGNAL_DEADLOCK_SKIPand both skip sites are gone.tests/fm-watch-triage.test.shruns 49 ok, 0 not ok with the previously-skipped assertions restored, and both formerly-quarantined cases now callassert_reaped_on_term, which turns the old skip condition into a named failure rather than a silent step-aside.What was not run, and why
bin/fm-ci-probe.sh zeeshaanahmad/firstmate→none; PR fix(bin): show registered decision key and fold every stated key #3 shows "no CI checks configured"), so the ci step has no reachable end condition. Local verification above stands in for it.bin/fm-test-run.shsuite. The pipeline's own test step covers it; the sweep above targets every suite that touches the changed files.live-harness-optinguards. Nothing here is harness-dependent - the verdict comes from signal disposition and lock ownership, not from anything a vendor emits - so no live-harness guard or per-harness verification record is owed.Pre-existing issues found, not caused by this change
bin/fm-ci-probe.shresolves the wrong repository. With no argument it usesgh repo view, which returns the fork's parent (kunchenguid/firstmate) rather thanorigin, so it answerspresenton a fork where checks never register - the opposite of the answer it exists to give, and it would let the ci step poll for a check that can never arrive.bash bin/fm-ci-probe.sh→present;bash bin/fm-ci-probe.sh zeeshaanahmad/firstmate→none;gh repo view --json nameWithOwner→kunchenguid/firstmatewhileoriginiszeeshaanahmad/firstmate. Reported separately; not fixed here.tests/fm-wake-queue.test.sh's subshell half cannot hold on stock macOS Bash 3.2. Lock ownership is keyed on${BASHPID:-$$};BASHPIDis Bash 4.0+, so on 3.2 a subshell reports its parent's pid. Confirmed failing identically atorigin/main. Now gated on the capability and says so, rather than failing for the wrong reason.tests/fm-watch-triage.test.shflakes under heavy load, on both sides. Its absorb gates arewait_live "$pid" 30- a fixed 3-second wall-clock slice, which is what the harness's ownwait_watcher_beatcomment warns against. Measured alternating in isolated checkouts at load ~20: this branch failed 2/2 andorigin/mainfailed 2/2, each at a different case. At load ~5.7 the suite is 49/49 green here. Converting ~26 gates changes what those cases assert and belongs in its own task.A hardening that was written, proved wrong, and removed
While chasing an unrelated symptom I saw
fm_lock_try_acquirerecurse into.wake-queue.lock.steal.steal.steal...until the filename exceeded the OS limit, and added a recursion depth bound. Its own red/green proof showed the bound was harmful: an unbounded walk of a fully abandoned steal chain legitimately reclaims the lock, and capping it turns slow, noisy recovery into a permanent refusal thatfm_lock_acquire_waitwould then spin on forever - a deadlock this PR would have introduced. Reverted. The observation is real and needs a fix that cleans up an abandoned chain rather than refusing it; that is its own task, deliberately not smuggled in here.Merge reasoning (firstmate)