fix(bin): stop false watcher cycle-failure alarms and supervision churn - #1416
Open
Moshik21 wants to merge 4 commits into
Open
fix(bin): stop false watcher cycle-failure alarms and supervision churn#1416Moshik21 wants to merge 4 commits into
Moshik21 wants to merge 4 commits into
Conversation
An attached arm can never see the owner arm's stdout, so every ordinary actionable close read as "cycle ended without an actionable reason" to a duplicate observer, and the failure banner then told the model to arm manually, recreating the duplicate that caused the next false alarm. - The attached arm now consults the lifecycle ledger before failing: an owner-delivered actionable close reports a typed benign line and exits 0, while a close with no delivered reason keeps the typed failure. - The Stop hook's failure banner defers to its own next-Stop re-arm and gates manual repair on the following turn still showing no watcher. - The watcher's poll and signal-grace waits are trap-interruptible, so a TERM acts at once instead of after a full wait, and --restart refuses loudly when a predecessor outlives its bounded reap wait rather than forking a child that would attach to the dying holder. - A live-agent declared pause caches its authoritative verdict for the recheck window, so the costly crew-state and backend probes run once per window instead of once per poll. - Secondmate windows are exempt from the stale loop unconditionally, per the AGENTS.md section 8 routed-status contract. - A declared pause whose endpoint is gone with a confirmed-dead agent surfaces once and keeps its bounded recheck instead of dropping out of supervision silently.
The bounded wait for a TERM'd predecessor was a fixed 5 seconds, shorter than the 15 second default poll budget. On an event-capable backend the terminal wait of a cycle is a foreground command substitution budgeted at FM_POLL, and bash runs no trap while a foreground command is outstanding, so a TERM'd watcher parked there cannot exit before that budget elapses. The loud refusal added for a genuinely wedged predecessor therefore fired on ordinary event-path restarts, which then needed a retry. - Derive the reap bound from the watcher's own poll budget plus a margin, never below the historical 5 seconds, with FM_ARM_REAP_WAIT to override it. The refusal keeps its shape, typed line, ledger row, and exit code; only the bound moves. - Correct the arm header's ledger contract: the lifecycle ledger is load-bearing for exactly one classification, the attached arm's owner-delivered-close check, and a row dropped under lock contention degrades that check to the typed failure line, costing a spurious failure report but never a missed wake. - Remove the secondmate stale-triage branch and the pause-path kind guard that the unconditional secondmate skip left unreachable.
…re and reap-bound fixes
5 tasks
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
1 participant
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
Stop the firstmate primary watcher from repeatedly reporting 'watcher: FAILED - cycle ended without an actionable reason' and needing a manual --restart, and stop the related supervision churn found while diagnosing it.
Investigation (report at data/fm-watcher-stale-churn/report.md) established that the alarm was almost entirely FALSE. Real wakes were being delivered the whole time; what failed was an ATTACHED duplicate arm's classification of an ordinary close. An attached arm can never see the owner arm's stdout, so every ordinary actionable close read as a reasonless death to the duplicate observer, and the failure banner then told the model to arm manually, creating the next duplicate and the next false alarm.
Fixes accepted for this change, F1-F6, already committed in the first commit:
F1 - the attached arm consults the lifecycle ledger before failing: an owner-delivered actionable close reports a typed benign line and exits 0, while a close with no delivered reason keeps the typed failure.
F2 - the Claude Stop hook's failure banner defers to its own next-Stop re-arm and gates manual repair on the following turn still showing no watcher, breaking the duplicate-creating feedback loop.
F3 - the watcher's poll and signal-grace waits are trap-interruptible (backgrounded sleep plus wait), so a TERM acts at once, and --restart refuses loudly when a predecessor outlives its bounded reap wait rather than forking a child that would attach to the dying holder.
F4 - a live-agent declared pause caches its authoritative verdict for the recheck window, so the costly crew-state and backend probes run once per window instead of once per ~20s poll. This is the pause-path churn the triage log showed.
F5 - secondmate windows are exempt from the stale loop unconditionally, per the AGENTS.md section 8 contract that a secondmate's idle endpoint is healthy and parent supervision relies on its routed status, not a pane-stale cadence.
F6 - a declared pause whose endpoint is gone with a confirmed-dead agent surfaces once and keeps its bounded recheck instead of dropping out of supervision silently.
A review gate on that first commit raised four findings; the captain decided all four and this second commit applies exactly that decision:
A (FIX, applied) - F3's loud refusal was a regression on event-capable (herdr) homes. The reap wait was a fixed 5s, but on an event-capable backend the terminal wait of a cycle is a foreground command substitution budgeted at FM_POLL (default 15s), and bash runs no trap while a foreground command is outstanding, so an ordinary event-path restart would routinely hit the refusal. The bound is now derived from the poll budget plus a 5s margin, floored at the historical 5s, with FM_ARM_REAP_WAIT to override. Deliberately narrow: only the bound moved; the refusal keeps its shape, typed line, ledger row and exit code. Option A2 (making the event-path wait interruptible) was rejected as reaching into backend wait semantics shared with the push path.
B (DEFER, deliberately untouched) - the changed-hash branch still calls clear_pause_tracking, so F4's throttle is defeated on a pane that redraws between polls. Open PR #1389 owns exactly that case in another lane; touching those lines here would conflict. F4 still delivers its win on a static parked pane. Do not flag this as an incomplete fix.
C (DEFER, deliberately out of scope) - F1's typed benign close line is interpreted by the Claude Stop hook; the OpenCode and Pi arm extensions still fall through to a generic failure for a clean close with no recognized wake line. Extending those two adapters is new harness surface, filed as its own backlog item, not part of this change.
D (DOCUMENT, applied) - F1's classification now depends on a ledger row whose append is non-blocking and gives up under lock contention. The arm header previously claimed the ledger was never a supervision dependency. It is corrected to say the ledger is load-bearing for exactly that one classification, and the fallback is stated: a dropped row degrades to the typed failure line, costing a spurious failure report but never a missed wake, because wakes are enqueued durably before the watcher prints and exits. Making the append blocking was rejected as introducing lock waiting on the close path for a smaller cost than the risk.
Also applied: mechanical removal of the secondmate stale-triage branch and the pause-path kind guard that F5's unconditional skip left unreachable.
Constraints honored: no behavior change to the non-blocking ledger append; the never-exits refusal test still passes; the new reap-bound regression test was verified to FAIL against the old fixed 5s bound and pass with the fix; docs/watcher-continuity.md is updated for both the derived bound and the ledger dependency.
What Changed
bin/fm-watch-arm.sh: an attached arm whose watched cycle ends with no successor now consults the lifecycle ledger before failing — an owner-delivered actionable close prints the typed benignwatcher: cycle closed actionably (reason delivered by its owner arm)and exits 0, while a close with no delivered reason keeps the typed failure.--restartrefuses loudly (typed FAILED, exit 1, no fresh fork) when a TERM'd predecessor outlives its reap wait, and that wait is now derived asmax(FM_POLL, 2 * FM_CREW_STATE_NM_TIMEOUT) + 5seconds, floored at the historical 5s, with a newFM_ARM_REAP_WAIToverride, so it covers both non-interruptible foreground spans a stopping watcher can be parked in.bin/fm-watch.sh: poll and signal-grace waits run through a backgroundedsleeppluswaitso a TERM acts immediately; a declared pause caches its authoritative verdict in the recheck marker so crew-state and backend probes run once per recheck window instead of once per poll; secondmate windows skip the stale loop unconditionally; and a declared pause whose endpoint no longer captures with a confirmed-dead agent surfaces once and keeps its bounded recheck instead of dropping out of supervision. The now-unreachable secondmate stale branch and pause-path kind guards were removed.bin/fm-claude-stop-autoarm.sh's failure banner now defers to the hook's own next-Stop re-arm and gates manual repair on the following turn still showing no watcher.docs/watcher-continuity.md,docs/architecture.md,docs/configuration.md,docs/supervision-protocols/claude.md,AGENTS.md) record the derived reap bound, the new benign close line, the unconditional secondmate exemption, the gone-endpoint recheck, and the correction that the non-blocking ledger is load-bearing for exactly that one classification (a dropped row degrades to the typed failure, never a missed wake). Tests added acrosstests/fm-watcher-lock.test.sh,tests/fm-watch-triage.test.sh, andtests/fm-claude-stop-autoarm.test.sh; the reap-bound regression test was verified to fail against the previous fixed bound.Risk Assessment
✅ Low: The follow-up commit applies exactly the three accepted round-1 fixes within their authorized scope — a widened but shape-preserving reap bound, an equivalence-preserving dead-branch collapse, and a verified-safe removal of a redundant key recompute — with the matching doc sentence updated and both named reap-bound tests still satisfied by construction.
Testing
Ran the three targeted suites the change touches — watcher lock (34 cases), watch triage (43), Claude Stop auto-arm (17) — all green, then proved each accepted fix at the product level by driving the real scripts at the base, both intermediate, and target commits and diffing what an operator actually sees: the attached duplicate arm's false
watcher: FAILED - cycle ended without an actionable reasonis replaced by the typed benign owner-delivered close with exit 0, the Stop-hook alarm no longer tells the model to arm immediately, an event-path--restartnow reaps and starts a successor instead of refusing, steady-state crew-state reads for one parked crewmate fall from 16 per 20s to 0, and a paused secondmate pane goes silent while its routed status still wakes. The second commit's widening of the reap bound to cover the crew-state span shipped without coverage, so I added a focused regression test and confirmed it fails against the pre-widening commit with the exact refusal line and passes at HEAD; it adds a case rather than altering any existing assertion. No visual artifact applies — every surface here is a CLI/daemon line, so the transcripts are the end-user view. Worktree is clean apart from that intentional test addition.Evidence: Before/after transcripts: duplicate-arm false failure, Stop-hook alarm, restart reap bound, pause churn, secondmate pane
Evidence: The reported symptom, before vs after (duplicate arm on an ordinary crewmate finish)
Evidence: The Stop-hook alarm the model reads on a typed failure
Evidence: Restart reap bound: finding A's regression, and this commit's crew-state widening
Evidence: Supervision churn and secondmate pane noise
Evidence: New regression test verified failing against the pre-widening commit 66f8df0
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed ✅
bin/fm-watch-arm.sh:107- REAP_WAIT is derived as FM_POLL + 5 to cover the event-capable terminal wait, but that is not the only non-interruptible foreground span in a watcher cycle. crew_absorb_class (bin/fm-classify-lib.sh:337) invokes fm-crew-state.sh in a command substitution bounded by FM_CREW_STATE_NM_TIMEOUT (default 10s, bin/fm-crew-state.sh:73) and can chain a second bounded no-mistakes call; bash defers the TERM trap for that whole span, and its duration does not scale with FM_POLL. At the default FM_POLL=15 the 20s bound covers it, but FM_POLL is documented as user-tunable (docs/configuration.md:435) and any value below ~6 puts the derived bound under the crew-state budget, reintroducing exactly the spurious 'predecessor still stopping' refusal that decision A was applied to remove. Secondary: the bound is computed from the arm's FM_POLL, which need not match the environment the running predecessor was launched with. Consider flooring the derivation at the crew-state read budget as well (e.g. max(FM_POLL, FM_CREW_STATE_NM_TIMEOUT) + margin) rather than FM_POLL alone.bin/fm-watch.sh:455- The*) rm -f "$recheck_file"arm of the case at 453-456 is now unreachable. crew_absorb_class only ever returns working|paused|none;workingalready returned at 445-448, and lines 451-452 force the remainder into exactly {paused, none}, sopaused|none)always matches. The case can collapse to a single unconditionalprintf '%s' "$class" > "$recheck_file".bin/fm-watch.sh:939- The newrm -f "$STATE/.paused-gone-$key"at line 937 consumes $key, which is only set by the bash-expansion block at 913-915; line 939 then recomputes the identical value viatr ':/.' '___'. The two forms are equivalent, so the code is correct, but reading a variable one line before it is reassigned is misleading and makes the new line look order-dependent on a value that has not been computed yet. Drop the recompute at 939 (or move it above 937).🔧 Fix: widen reap bound to cover crew-state span; drop dead code
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-watcher-lock.test.sh(34 cases, exit 0; includes the never-exits refusal test and the FM_POLL-derived reap-bound regression)bin/fm-test-run.sh tests/fm-watch-triage.test.sh(43 cases, exit 0; covers the F4 pause throttle, F5 secondmate exemption, F6 gone-endpoint recheck)bin/fm-test-run.sh tests/fm-claude-stop-autoarm.test.sh(17 cases, exit 0; covers the F2 banner rewording)Addedtest_watch_restart_reap_bound_covers_crew_state_waittotests/fm-watcher-lock.test.sh:545— the crew-state half of the widened bound had no coverageVerified the new test FAILS against the pre-widening tree:git archive 66f8df0into a temp checkout, copy the new test file in,bash tests/fm-watcher-lock.test.sh→not ok - restart refused a predecessor parked in the crew-state span with FM_POLL=1Manual end-to-end:scenario-duplicate-arm.sh— owner arm starts a real watcher, a second arm attaches, adone:status closes the cycle; compared the duplicate arm's stdout, exit code, and lifecycle-ledger reason at base 3772964 vs target 09caadaManual end-to-end:scenario-stop-hook-banner.sh— drovebin/fm-claude-stop-autoarm.shas a child of a fake harness holding the session lock with a typed-failure arm fixture, rendering the exact exit-2 alarm before and afterManual end-to-end:scenario-restart-reap.sh—bin/fm-watch-arm.sh --restartagainst a TERM-deaf predecessor; FM_POLL=30/6s span at de67e55 vs HEAD, and FM_POLL=1 FM_CREW_STATE_NM_TIMEOUT=10/8s span at 66f8df0 vs HEADManual end-to-end:scenario-pause-churn.sh— countedfm-crew-state.shinvocations over 20s of steady supervision of one static parked pane at FM_POLL=1, base vs HEADManual end-to-end:scenario-secondmate-pane.sh— 12s watching a paused secondmate pane past its recheck cadence, then a routed status append, base vs HEADCode check that the constraint held:git diff 3772964..HEAD -- bin/fm-watch-arm.shshowscycle_log_appendmechanics unchanged (new callers only), and the removedpause_state_classcase / duplicatekey=recompute are provably unreachable/redundant✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.