fix: resurface durable work after watcher re-arm - #46
Merged
Merged
Conversation
Port kunchenguid#2065, adapted to this fork's OMP-primary, multi-backend wake core. After an accepted watcher down stretch, a re-armed watcher now guarantees both a durable wake-queue drain and resurfacing of still-open decisions. state/.watcher-down holds one generation-bound recovery episode; wakes stay queued until the handling turn runs the exact --ack-through command the drain prints as WAKE_ACK_REQUIRED, so an interrupted turn replays instead of losing its work. Fork adaptations beyond the upstream diff: - The Pi and OMP primary adapters share bin/fm-primary-watch-core.ts here, so the handling-delivery handshake lands once in that core and both adapters inherit it. Upstream patched its Pi extension directly. - fm_wake_append's bounded-attempt mode keeps its whole-call bound: recovery publication takes the same attempt budget and reports exhaustion as 3 rather than blocking, so bin/fm-send.sh's delivered-no-turn path cannot hang. - bin/fm-watch.sh's EXIT trap performs a marker transition, and a signal can fire that trap while an interrupted frame still holds the marker lock. Waiting there for a lock the same process holds can never be satisfied: the watcher spun forever, ignored every later signal, and pinned the durable queue's lock with it. fm_lock_held_by_self detects that reentry, proceeds under the exclusion already held, and leaves the lock for the next process to reclaim from its dead owner. Exclusion between processes is unchanged. - The upstream --reemit session-start hunks do not apply; this fork has no --reemit path. Tests: durable drain plus open-decision resurfacing after re-arm, interrupted handling replay, generation-bound acknowledgement, marker-lock reentry from an exiting frame, and the OMP adapter's handling-delivery handshake. Every intentional mid-test watcher stop in tests/fm-watch-triage.test.sh now acknowledges its recovery episode, and that suite's status-signature fixture matches this fork's inode:size:nanosecond signal scan.
Port kunchenguid#2212, the correctness follow-up to the recovery contract in the previous commit. A watcher cycle that opened and closed while the model handled its presented wakes minted a fresh recovery generation, which invalidated the exact acknowledgement the drain had just printed. That acknowledgement then consumed nothing, the marker stayed pending, and every later arm spent its whole cycle re-announcing the same recovery instead of supervising - a livelock the home could not leave on its own. A downtime publication now reuses the generation of an outstanding episode, so a close inside the handling window cannot orphan the printed acknowledgement. The acknowledgement itself separates its two facts: queue-row consumption is bound to the monotonic --ack-through sequence and always happens, while only retiring the episode is bound to --recovery-generation. A generation that moved on is a non-fatal result naming its own remedy instead of a refusal that consumes nothing. Adapted: upstream's ack path also threads its inactive-outcome receipt stage from kunchenguid#2167, which this fork has not ported; the sequence/generation split lands without it. The reentrancy regression from the previous commit now asserts the stronger invariant this change provides - a reentrant publication preserves the outstanding generation rather than minting a new one.
Port kunchenguid#2733, the newest hardening on top of the recovery contract and its livelock fix. A lost handling handshake re-announced the same recovery generation on every cycle and spent the successor's first ~55s blind, so a real crew event could be ignored and then dropped. The recovery marker now records the announcement: an unacknowledged downtime generation is announced at most once, a non-successor start after an announced episode mints a fresh generation so buried decisions still resurface, and a handling successor enters its poll loop immediately instead of re-announcing and instead of waiting out a fixed handling window. The adapters confirm the handling handoff before scheduling the follow-up, retry once against the current generation and successor, and classify a failed confirmation into exactly one typed wake rather than swallowing it. Fork adaptations: - The Pi half lands in bin/fm-primary-watch-core.ts, so OMP inherits the same confirmation, retry, and typed-failure behavior. Upstream did not touch OMP. - tests/fm-omp-primary.test.sh gains the OMP-bound assertion that a refused handshake produces one typed steer, alongside the successful-handshake case. - tests/fm-watch-recovery-loop.test.sh's fixture also installs the shared core and the Pi-compatible runtime allowlist, without which this fork's adapter cannot load. - tests/wake-helpers.sh takes only the two self-contained helpers the new coverage needs; upstream's prime_status_seen depends on wake-lib signature owners this fork does not have. - The runner's watcher-wake-lock family gains the new suite; upstream's fm-wake-drain-unread-status and fm-inactive-reconcile entries do not exist here.
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
Port the upstream supervision-recovery tranche from kunchenguid/firstmate into this fork (dnth/firstmate) as ONE coherent, OMP-aware PR: PRs kunchenguid#2065 -> kunchenguid#2212 -> kunchenguid#2733, which form a dependency chain and are landed in that order as three commits.
Why this work exists: when the watcher goes down and later re-arms, this fork can silently lose queued wakes and the resurfacing of still-open decisions - a critical supervision-continuity gap that surfaces as the "WATCHER DOWN" state at session start. The tranche guarantees durable recovery after re-arm, makes the recovery acknowledgement livelock-safe, and bounds recovery announcements so the successor supervision cycle is never lost.
What each PR must deliver, adapted to this fork's OMP-primary, multi-backend wake core:
Porting approach the user chose: port ADAPTED, not cherry-picked - this fork adapts upstream, so patch IDs differ. Preserve the fork's OMP primary-extension watcher-continuity model, the durable wake-queue and open-decisions machinery, and every experimental backend (herdr, zellij, orca, cmux) plus the Pi, OpenCode, and OMP adapters.
Constraints the user set: this is the wake/supervision core the whole fleet depends on, so blast radius is CRITICAL. No existing safety invariant may be weakened - fail-closed behavior on malformed input, single-flight arming, never broadly killing watchers, and the OMP extension-owned continuity contract must all survive unchanged, and the OMP primary adapter's watcher-continuity semantics must be preserved exactly. Delivery is no-mistakes with yolo off, held for captain merge. Because this edits firstmate's own tracked material, the firstmate-coding-guidelines apply: one full sentence per line in tracked Markdown, plain dash never an em dash, shellcheck-clean bin scripts via bin/fm-lint.sh, colocated tests extending existing tests/ scripts rather than a new runner, and no agent name as a commit co-author.
Deliberate decisions and tradeoffs made while doing this work, which a reviewer reading only the diff would not know:
Upstream patched its Pi extension directly. This fork had already extracted the watcher lifecycle into the shared bin/fm-primary-watch-core.ts used by BOTH the Pi and OMP adapters, so the handling-delivery handshake, the confirm-with-retry logic, and the typed-failure classification were placed in that core exactly once. That is how OMP inherits the contract instead of receiving a duplicated copy, and it is why .omp/extensions/fm-primary-omp.ts itself is unchanged.
Intentional fix beyond the upstream diff: bin/fm-watch.sh's EXIT trap performs a recovery-marker transition, and a signal can fire that trap while an interrupted frame is still inside a marker critical section. Waiting there for a lock the same process already holds can never be satisfied, so the watcher spun forever, ignored every later signal, and pinned state/.wake-queue.lock with it - wedging the whole home rather than one cycle. This was reproduced deterministically, traced to fm_recovery_marker_arm_check's release being interrupted by watcher_cleanup, and fixed with fm_lock_held_by_self plus _fm_recovery_lock_acquire/_fm_recovery_lock_release. On reentry the transition proceeds under the exclusion it already holds and deliberately leaves the lock exactly as the interrupted frame left it, because the exiting process abandons it and fm_lock_try_acquire reclaims a dead owner's lock as usual. Reentry is scoped to self-ownership only, so exclusion between processes is deliberately unchanged; tests/fm-wake-queue.test.sh pins both halves, including that a live foreign holder is still not treated as reentry.
Deliberate adaptation: fm_wake_append's fork-only bounded-attempt mode keeps its whole-call bound. Recovery publication takes the same attempt budget and reports exhaustion as 3 rather than blocking, so bin/fm-send.sh's delivered-no-turn path cannot hang. Upstream had no bounded mode to preserve.
Deliberate exclusions from the upstream diffs, because the corresponding fork surfaces do not exist: upstream's --reemit hunks in bin/fm-session-start.sh and tests/fm-session-start.test.sh (this fork has no --reemit path); upstream fix(bin): prevent watcher recovery acknowledgement livelock kunchenguid/firstmate#2212's inactive-outcome receipt stage in the acknowledgement path (it comes from upstream feat(bin): reconcile inactive terminal crew outcomes kunchenguid/firstmate#2167, which this fork has not ported); upstream's prime_status_seen helper in tests/wake-helpers.sh (it depends on wake-lib signature owners this fork does not have, so only recovery_marker_generation and ack_drain_err were taken); and upstream's fm-wake-drain-unread-status and fm-inactive-reconcile entries in bin/fm-test-run.sh's watcher-wake-lock family (neither test exists here).
Deliberate test-fixture adaptations to this fork's real contracts, not test weakening. In tests/fm-watch-arm.test.sh the remote parent-reply fixture carries its correlation in the note rather than a second bracket token, because bin/fm-procevent-remote-reply.sh's line_valid accepts exactly one bracket token before the colon and separately requires a corr= token; this keeps the line both ingestible and foldable as a keyed decision. In the same file status_signature was aligned to bin/fm-watch.sh's stat_sig (inode:size:nanosecond mtime); upstream's coarser size:mtime signature never suppressed this fork's signal scan, so the fixture watcher raced an unrelated signal wake into the case under test. Also in that file, the re-arm liveness check became a bounded wait instead of a fixed sub-second sample, because this fork's watcher runs PR-check migration, pending-reply, and process-event work before its first poll, so a healthy re-arm was being read as a stayed-live failure; wait_for_exit still stops the child on timeout, so the never-surfacing path still leaves nothing behind.
Every intentional mid-test watcher stop in tests/fm-watch-triage.test.sh now acknowledges its recovery episode through ack_stopped_cycle. That is required by the new contract - without it the next cycle correctly announces check: rearm-resurface instead of the path under test - and is not incidental churn.
Added fork-specific OMP coverage the user explicitly asked for, in tests/fm-omp-primary.test.sh: OMP confirms the recovery handling handshake only after delivering its wake steer, and a refused handshake is surfaced as exactly one typed wake rather than swallowed. tests/fm-watch-recovery-loop.test.sh's fixture also installs the shared core and the Pi-compatible runtime allowlist, without which this fork's adapter cannot load at all.
bin/fm-guard.sh's header comment was corrected because the drain no longer empties the queue; that is the only change to that file and it is deliberate.
bin/fm-classify-lib.sh appeared in the task's expected touched surface but needed no change: this fork's incremental open-decisions cursor already satisfies the resurfacing contract, so it was deliberately left alone rather than modified to match the upstream file list.
Deliberately NOT changed: tests/fm-watcher-lock.test.sh's HUP-case timing bound. Its 8s wait has little headroom because the watcher can sit in sleep FM_POLL=5 before its trap runs, but that bound predates this branch and the path was measured terminating correctly 10 times out of 10, so widening an unrelated test's timing inside a critical-safety port was rejected.
What Changed
Risk Assessment
🚨 High: The away-mode recovery episode can still be retired after only partial or unverified decision resurfacing, violating the approved fail-closed continuity invariant.
Testing
No baseline commands were supplied; targeted Pi, OpenCode, OMP, durable queue, watcher-lock, decision-routing, re-arm, generation-stability, and bounded-announcement checks all passed, with direct evidence showing one recovery announcement, a live successor, and a durably queued crew event.
Evidence: Bounded recovery and live successor transcript
T2_WATCH_OUTPUT=signal: .../state/crew.status T2_QUEUE_ROW=... signal crew.status ... T1_MESSAGES=1 T1_MARKER=announced:downtime:seed.1.aaaPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-primary-watch-core.ts:390- Intent requires “OMP confirms the recovery handling handshake only after delivering its wake steer,” but this confirms first and callssendWakeafterward. A failed/blocked steer can therefore leave the episode marked as handling without delivering it. Reverse the ordering and strengthen the OMP behavioral test, which currently claims but does not assert confirmation-after-steer.bin/fm-primary-watch-core.ts:616- Intent requires that “the successor supervision cycle is never lost,” but an actionable successor close is discarded while the preceding restoration/follow-up remains active. If successor B finds a real event while A's follow-up blocks, B clears the child reference and this return loses its notification and next re-arm. Queue the close or process it after current delivery; apply the equivalent fix to OpenCode'srestorationInFlightguard.bin/fm-wake-lib.sh:525- The required confirm-with-retry and typed-failure path can block forever here:fm_recovery_marker_begin_handlingwaits unboundedly for.watcher-down.lock, while both adapters invoke this command synchronously. A live stopped foreign holder freezes delivery and supervision without a typed failure. Add a bounded acquire at this shared confirmation boundary while preserving foreign-owner exclusion.bin/fm-supervise-daemon.sh:1354- The durable no-loss requirement remains reachable in away mode:handle_wakecan fail to persist an actionable escalation, but its status is ignored and the code still acknowledges all rows at line 1367. An unwritable/full state directory can therefore delete the only durable wake without buffering or injection. Abort before acknowledgement unless routing persistence succeeds.bin/fm-watch-arm.sh:391- The new internal--handling-deliveredmode and conditionalrecovery-generationstarted-line suffix are absent from the script's authoritative usage header, which still documents only--restartand the old exact output. Update the header to own both protocol additions.bin/fm-wake-drain.sh:29- This introduces the critical acknowledgement CLI, but the authoritative script header has no usage syntax for it. Add an explicit usage block covering presentation and--ack-through ... --recovery-generation ...modes.docs/verification/supervision.md:326- The new dated maintainer-verification record includes command and output but no tested source/tool version, contrary to the repository's verification-record requirements. Record the tested revision or equivalent version.🔧 Fix: Bound recovery confirmation and document protocols
2 errors still open:
bin/fm-primary-watch-core.ts:378- The required retry “against the current generation and successor” repeats the same handoff. BothspawnSyncconfirmations execute back-to-back, preventing child output/close callbacks from updatingowner.childorarmRecoverybefore the second snapshot. A replacement between readiness and confirmation therefore causes two attempts against the stale generation/PID, followed by a fallback. Make the retry refresh the live handoff before its second confirmation; the equivalent OpenCode implementation has the same defect.bin/fm-supervise-daemon.sh:1350- Away-mode recovery drops decision-only resurfacing. The daemon parses only five-field queue rows from the drain output, discarding itsOPEN DECISIONSsection; with an empty queue and a buried still-open decision, it routes only genericcheck: rearm-resurfaceand then acknowledges the episode. The injected digest claims it was pre-read but contains no decision, while last-status housekeeping cannot recover a buried decision. Route the owned OPEN DECISIONS output before acknowledgement.🔧 Fix: Route away-mode decisions before recovery acknowledgement
1 error still open:
bin/fm-supervise-daemon.sh:1365- The new decision route ignoresescalate_addfailure and proceeds to generation-bound acknowledgement. If the escalation buffer becomes unwritable or storage is exhausted, the open decision is never persisted or injected, yet the recovery episode is retired at line 1384. This contradicts the required guarantee that still-open decisions cannot be lost after re-arm. Track routing failure and return before acknowledgement, while leaving the separately approvedhandle_wakebehavior unchanged.🔧 Fix: Retain recovery when away decision routing fails
1 error still open:
bin/fm-supervise-daemon.sh:1370- The required fail-closed behavior is still violated when the drain'sOPEN DECISIONSsection is truncated: this branch logs an unterminated section but continues to the generation-bound acknowledgement. Becausefm-wake-drain.shdeliberately swallows decision-section output failures, the daemon can route only a prefix, delete its capture files, and then successfully retire the recovery episode. Treat EOF whilein_open_decisions=trueas a routing failure and return before acknowledgement.🔧 Fix: Fail closed until away decisions are fully delivered
1 error still open:
bin/fm-supervise-daemon.sh:1396- The standing invariant requires acknowledgement only after the COMPLETE open-decision content is routed, but completeness is not positively proven. The parser acceptsoutsideas valid when a capture is truncated before its header, and it treatsOPEN DECISIONS: N more omitted (byte cap)as ordinary routed content. Either path reaches--ack-throughwhile decisions were never injected. Make the drain emit an explicit complete frame even for an empty set, reject omitted/incomplete frames, and acknowledge only a positively completed machine-readable presentation.✅ **Test** - passed
✅ No issues found.
FM_TEST_EVIDENCE=1 bash tests/fm-watch-recovery-loop.test.shbash tests/fm-omp-primary.test.shbash tests/fm-wake-queue.test.shbash tests/fm-watch-arm.test.shbash tests/fm-wake-daemon-lifecycle-e2e.test.shbash tests/fm-watcher-lock.test.shbash tests/fm-pi-watch-extension.test.sh(initially exposed the fixture race, then passed after the test-only synchronization fix)bin/fm-primary-watch-core.ts:390- The intent requires OMP handling confirmation after wake-steer delivery, but the shared core confirms first and sends afterward; executable changes are outside this documentation phase.🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Scope fixture environments to satisfy ShellCheck
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.