fix: add custody-aware stall recovery - #159
Merged
Merged
Conversation
dnth
changed the title
dnth
changed the title
dnth
changed the title
When a steering inbox record stays unhandled past the re-ring ladder or its endpoint proves dead, the watcher now runs fm-stall-recovery.sh before publishing the stale wake. The helper re-proves the record is still the oldest unhandled instruction, classifies the endpoint as live-non-turning or missing, requires a clean non-run crew-state, a clean worktree with no unlanded commits, and the durable fm-<id> Treehouse lease, then re-proves the chain immediately before invoking fm-control relaunch. A handled record reports recovered; a busy worker or a just-published relaunch defers; every unprovable or unsafe shape escalates to the ordinary stale wake with the reason appended. One automatic relaunch per stalled record is bounded by a per-record attempt counter that resets only when the inbox empties. OMP gains semantic busy wiring (omp-ext source on turn_start/turn_end/session_shutdown) so live OMP workers classify instead of falling through to unknown, and an OMP relaunch retires the prior generation's request.* doorbell receipts after every refusal gate so a stale .acked tombstone cannot suppress the replacement incarnation's doorbell.
- fm-busy-lib: restore the hermes-hook adapter arm dropped by the omp-ext addition; its loss broke the crew-state status fallback (serial 3 failure). - fm-stall-recovery: SC1007 empty assignment and SC2097/SC2098 env-prefix expansion on the fm-control relaunch call. - fm-stall-recovery.test: skip at top level when bun is absent; the in-function exit only ended the command-substitution subshell and left FAKEBIN_DIR holding the skip text (serial 4 failure).
- fm-stall-recovery acquires fm-control's lifecycle lock before the final custody/record re-check and holds it across the relaunch via --lock-preheld, so a handled record or concurrent lifecycle action can never relaunch a now-productive worker or double-relaunch - fm-control --stall-record re-proves the named inbox record inside the lock immediately before the agent is touched, cancelling with exit 3 when it resolved in flight - clean-worktree and unlanded-commit gates removed from prove_custody: recovery exists to preserve exactly that work in the same worktree, branch, and commits - OMP extension serializes busy-state writes through one awaited chain so turn_end's idle can never land after a following turn_start's busy - fm-spawn writes control_relaunch_tx into the published record so post-publish failures classify the successor correctly - one-relaunch-per-record bound is a fixed invariant; the FM_STALL_RECOVERY_MAX override is removed
The cancelled:record-resolved journal write moved RELAUNCH_PHASE past the rollback trap's checkpoint|noted restore branch, so a record resolved inside the lifecycle lock left the brief modified despite no relaunch. Restore the brief byte-exact before journaling the cancellation, assert it in the in-lock test, and correct docs/architecture.md's stale claim that recovery requires a clean worktree with no unlanded commits.
…per record Astra re-verification of 0607ba7 (data/fm-159-astra-review/report.md): F1 (P1): a worker that published a valid busy event during safe_checkpoint was still interrupted - the final in-lock gate re-proved only the record's identity, then do_exit sent Escape + /exit to a now-productive worker. The supervised --stall-record path now fails closed on any busy verdict that is not a proven idle: busy returns exit 4, which do_relaunch maps to a clean cancellation (byte-exact instruction restore, cancelled:worker-busy journal phase) that fm-stall-recovery reports as a deferred episode. Manual relaunch and the plain exit verb keep interrupt-first semantics. F2 (P2): a well-formed .recovery-attempts marker naming a previously handled record was rejected as malformed, denying the next queued record its first attempt whenever the watcher never observed an empty inbox. Marker structure is now validated separately from record identity; a marker naming a different record starts the current oldest record's count at zero and is replaced atomically under the lifecycle lock. Corrupt markers still escalate, and the one-attempt bound per record is unchanged. Regressions: busy-during-checkpoint defer (no interrupt/exit/relaunch, byte-exact brief restore), next-record fresh attempt with no intervening empty-inbox observation, and corrupt-marker fail-closed escalation.
Astra re-verification of 4042180 (data/fm-159-astra-reverify/report.md): R1 (P2): the marker line-count check accepted a canonical line followed by an unterminated suffix (001.msg<TAB>1<NL>CORRUPT) because wc -l counts only newlines and read parses only the first line. The marker is now validated as exactly one canonical newline-terminated record by byte count, so any trailing byte or missing terminator escalates; the separate identity reset and atomic replacement are unchanged. R2 (P2): an unproven busy verdict at the final do_exit gate died generically, routing into the stopping rollback where the appended progress note survived even though no lifecycle action occurred. The supervised --stall-record path now returns exit 5 for unproven custody before any transport, do_relaunch routes it through stall_relaunch_cancel (byte-exact instruction restore, cancelled:custody-unproven journal phase), and fm-stall-recovery maps it to escalate - never deferral or instruction-handled. D1 (P3): docs/architecture.md now describes the per-record attempt bound, new-record replacement, and empty-inbox cleanup separately. D2 (P3): docs/configuration.md now states deferred/recovered outcomes suppress the stale wake while escalation or helper failure retains it. Regressions: unterminated marker suffix escalates; unproven custody during checkpoint escalates with no transport and byte-exact instructions.
…ng endpoints relaunch Captain ruling (inbox 031/036): recovery must NEVER interrupt a live worker. - LIVE-BUT-NON-TURNING (endpoint present, any busy state): detect and escalate only. No interrupt keys, no exit, no relaunch, no invalidation. - POSITIVELY ABSENT endpoint: relaunch allowed with no busy-state proof - there is no live worker to interrupt. - Unknown/unproven crew-state fails closed on every path. - Deleted the busy-record machinery this task added: OMP extension busy events, omp-ext source registration, busy verdict gate in the helper, and fm-control's supervised busy-deferred/custody-unproven exits. - Kept: missing-endpoint relaunch under the lifecycle lock with the in-lock record re-check, per-record attempt bound, relaunch_tx, and doorbell receipt retirement; shared busy-state infrastructure untouched. 19/19 stall-recovery tests pass; fm-lint clean.
dnth
force-pushed
the
fm/fm-stall-recovery-ship
branch
from
September 24, 2026 12:24
cf20fb8 to
fd6134a
Compare
dnth
changed the title
…te is now captured for all relaunches and the custody guard runs after the stopping journal immediately before lifecycle control. This resolves both portable serial failures (the reproduced unbound-variable error). Focused dead-endpoint relaunch tests pass. The Herdr failure was an external stale-watcher/worktree-state failure, not caused by this change; no Herdr-specific code was modified
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
Stall recovery per captain ruling: a live worker is NEVER interrupted - live-but-non-turning endpoints get DETECT+ESCALATE only; positively-absent endpoints may relaunch with NO busy-state proof. All busy-record machinery added by this task is deleted; shared busy-state infrastructure (fm-busy-lib contracts, fm-busy-event) stays for its other consumers.
What Changed
Risk Assessment
✅ Low: The reviewed changes satisfy the stated stall-recovery constraints without a source-verifiable defect or unauthorized busy-record machinery addition.
Testing
Ran the focused product-level stall-recovery integration suite against the real shell entrypoints. Missing-endpoint recovery, live-worker non-interruption, fail-closed custody, race cancellation, lease/worktree preservation, receipt handling, and bounded retries all passed; no source changes or transient worktree artifacts were left behind.
bash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.sh; live test logEvidence: stall recovery live test output
Focused end-to-end stall recovery suite passed all 19 behavioral scenarios, including missing-endpoint relaunch, live-worker escalation, custody failures, race cancellation, lease preservation, and retry bounds.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (3) ✅
bin/fm-control.sh:911- The claimed invariant “a live worker is NEVER interrupted” is still reachable through a missing→live race.fm-stall-recovery.shprovesmissingand then invokesfm-control.shunder the lock, butdo_relaunchonly rechecks the inbox before journalingstopping; at lines 910-922 it treats any state other thanmissingas permission to calldo_exit. If the endpoint is recreated or becomes live after the helper’s final custody proof but before line 911, this path sends the interrupt/exit sequence to the live worker. The final guard must fail closed onaliveand other non-absent states immediately before lifecycle control; this contradicts the authoritative live-worker criterion and needs confirmation of the intended remedy.bin/fm-stall-recovery.sh:140- The new recovery path still fails open when the inbox cannot be read.fm_task_inbox_oldest_unhandledcollapses an unreadable/read failure into status 1, and the added calls infm-stall-recovery.shuse|| trueat lines 140 and 239, treating that failure as an empty/handled inbox and returningverdict=recovered; the new in-lock--stall-recordcheck infm-control.shdoes the same. A transient permission/I/O failure can therefore suppress the stale wake and skip recovery while the durable instruction remains unhandled. Preserve the empty-vs-unreadable distinction at the shared inbox boundary and propagate unreadable status as escalation.🔧 Fix applied.
1 error still open:
bin/fm-task-inbox-lib.sh:408- Unreadable inbox errors are still suppressed by the watcher.fm_task_inbox_due_actionnow returns status 2 for an existing-but-unreadable inbox (the changed path atbin/fm-task-inbox-lib.sh:408), butinbox_steer_checkdoesaction=$(...) || return 0atbin/fm-watch.sh:405; the unavailable/escalation branches also use|| trueat lines 352 and 455. A transient permission/I/O failure therefore produces no recovery attempt and no stale wake, suppressing an actually unhandled instruction instead of failing closed.🔧 Fix applied.
2 errors still open:
bin/fm-control.sh:918- The supervised recovery path classifies bothdeadandmissingendpoints as the safe absent path infm-stall-recovery.sh:185, butfm-control.shrejects every state other thanmissingatbin/fm-control.sh:918. Therefore a positively classified dead endpoint can never be relaunched: the helper records the attempt, invokes control, and control escalates instead of performing the permitted recovery. Accept the same positively absent states in the final supervised gate (or narrow the helper classification consistently).bin/fm-control.sh:917- The final endpoint check is not actually adjacent to the lifecycle action. Afterstate=$(agent_state)recordsmissingatbin/fm-control.sh:917,journal_write stoppingruns at line 921 and further checkpoint/launch work follows; if the endpoint is recreated and becomes live in that gap, the capturedmissingvalue still drives the relaunch path and skipsdo_exit. This leaves the live-worker-never-interrupted invariant reachable through a missing→live race. Reuse one final custody guard immediately before the launch/stop operation and fail closed on any changed state.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.sh; live test logbash tests/fm-stall-recovery.test.shCaptured output to~/.no-mistakes/evidence/01M39KRS3A8CN1Y7Q783HQFY7F/fm-stall-recovery-live.log✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.