fix(bin): prevent silent second-mate intake stalls - #2640
Open
tiago-peixoto wants to merge 8 commits into
Open
Conversation
…ng re-arm to handling A second mate's event intake can die permanently and silently, and did for thirteen hours. Two independent defects, fixed so neither assumes the other is handling it. Nothing outside a second mate checked that its supervision was alive. The loop is circular - the mate is woken by its watcher, its watcher is armed by its own session, and its session runs only when woken - so once the watcher stops with a wake already queued, nothing inside that home can restart it. The only outside check proved one thing: that an agent process exists. An idle second mate and a deaf one answer that identically. bin/fm-secondmate-wake-check.sh asks the other half: is that home still CONSUMING its own durable wake queue? The signal is the age of the oldest unacknowledged row. Each row carries its own append epoch, written once and never rewritten, and leaves the queue only through the generation-bound acknowledgement that follows handling - so it is positive evidence produced by the mechanism that failed, and an empty queue is a healthy idle mate. The script header records why the two alternatives were rejected: a stale liveness beacon is the normal steady state of an idle home, and the .watcher-down marker is republished on every queue append so its mtime tracks the last append rather than the outage. It runs as an adjunct to paths that already run - the watcher's ordinary poll cycle and the session-start liveness sweep - not as a monitor of its own, and it only reads. Arming a watcher across homes stays forbidden; recovery is firstmate nudging that second mate through its own endpoint so it arms its own watcher. Separately, bin/fm-procevent.sh retires a spent registration at capture for an adapter whose result is terminal for that exact registration, while re-arming the next source lives inside that adapter's application step. One failed application therefore ended intake permanently rather than delaying it. `reconcile` now retries `autohandle` for every captured result with no handled acknowledgement yet, on the same idempotent seam and in the same adapter-declared order. Acknowledgement is unchanged: a failed application still leaves its result unhandled, announced, and eligible for the next retry. Because two callers can now reach one generation, application is serialized per source at a boundary separate from the source lock the adapter's own re-arm takes; the runner waits for it, reconcile declines and retries rather than blocking the watcher. Windows that remain, deliberately: the check finds a wake loop that received work and did not consume it, not one that is dead with nothing arriving; a remote second mate's home is not locally readable, so those routes stay uncovered; detection is bounded by the stall threshold plus one scan interval; and reconcile itself runs from a home's own watcher, so a deaf home's intake stays down until its wake loop is restored - which is exactly why the two halves are independent. Both fixes were proven red first: a live agent whose queue sat unconsumed for thirteen hours was reported "already live", and a terminal capture whose application failed left no registration and never re-armed across repeated reconciles.
The supervision loop now bounds every call it shells out to except the wake-queue lock wait. That wait cannot be bounded additively: fm_lock_acquire_wait is a sourced shell function running in the watcher process rather than a subprocess, so fm_run_timed cannot wrap it. Its two in-loop callers, procevent_surface_queued and resurface_after_downtime via fm_recovery_marker_arm_check, deliver an actionable wake by calling wake, which exits the watcher process. Moving those callers into a bounded subshell would swallow that wake and recreate the silent loss this change prevents. Dead holders are already reclaimed by fm_lock_try_acquire after FM_LOCK_STALE_AFTER. The remaining gap is only a live holder that never releases, a different defect filed as separate work. Re-derivation found two entries previously marked already bounded were wrong: fm_backend_wait_transition and run_check_process. It also found the omitted crew_absorb_class to fm-crew-state.sh call. All three are fixed in this pass.
The supervision loop is now bounded at every call it shells out to except the wake-queue lock wait. That wait cannot be bounded additively: fm_lock_acquire_wait is a sourced shell function running in the watcher process rather than a subprocess, so fm_run_timed cannot wrap it. Its two in-loop callers, procevent_surface_queued and resurface_after_downtime via fm_recovery_marker_arm_check, deliver an actionable wake by calling wake, which exits the watcher process. Wrapping them in a bounded subshell would swallow that wake and recreate the silent loss this change prevents. The dead-holder case is already covered because fm_lock_try_acquire reclaims an abandoned hold after FM_LOCK_STALE_AFTER. The true residual is only a live holder that never releases, which is a different defect filed as its own work. Two entries previously marked already bounded were re-derived and found wrong: fm_backend_wait_transition and run_check_process. One call was omitted entirely: crew_absorb_class to fm-crew-state.sh. All three were fixed in this delivery. This follow-up also normalizes kill-after status, preserves shared Herdr helper contracts, and separates apply locks from source locks.
tiago-peixoto
force-pushed
the
fm/firstmate-secondmate-intake-dies-silently
branch
from
August 19, 2026 18:45
34a7200 to
0092a5e
Compare
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
A second mate's event intake can die permanently and silently. It did, for thirteen hours: on 2026-08-19 at 00:16 the Artemis review coordinator's registered long-poll source emitted event 102 correctly, then everything stopped - the wake stayed queued and unhandled in that coordinator's own queue, .watcher-down was set, no watcher process remained, and nothing reached the lane again: not the captain's review requests, not new heads, not check failures. The captain found it by looking at GitHub himself, which is exactly what the intake exists to prevent. Fix the two defects behind that, together, so neither assumes the other is handling it.
Defect 1 - nothing outside checks that a second mate's supervision is alive. The loop is circular: the mate is woken by its watcher, its watcher is armed by its own session, and its session runs only when woken, so once the watcher stops WITH A WAKE ALREADY QUEUED, nothing inside that home can restart it, because waking is precisely what it lost. And nothing outside looks: the only outside check was secondmate_liveness_sweep in bin/fm-bootstrap.sh, which runs at session start only and proves exactly one thing - that an agent process exists. The fleet asked "is the agent alive?" and never asked "is its wake loop working?". An idle second mate and a deaf one are indistinguishable from outside; that is the whole defect. The task required establishing what a supervision-alive check should actually ASSERT before writing one, weighing at least these candidates - a queued-but-undrained wake older than some bound, a stale watcher beacon, a .watcher-down marker nobody cleared - preferring whichever cannot itself go stale silently, and SAYING WHY the others were rejected.
Defect 2 - handling and re-arming are coupled. bin/fm-procevent.sh states it: "a handling adapter is expected to re-arm its own next source." The source is one-shot by design and exits when it emits, which is correct, so if the event is never handled the source is never re-armed and intake ends PERMANENTLY rather than falling behind. A delay is acceptable; a silent permanent stop is not. Whatever changed here had to preserve the existing guarantee that a captured result stays eligible for re-announcement until explicitly acknowledged - acknowledgement must not be weakened to solve re-arming.
Explicit refusals the captain set, which the diff must be read against: do NOT build a daemon, a poller-watcher, or a monitoring service - we have this problem precisely because a monitor with no supervision was trusted, and adding a second unsupervised monitor repeats it one level up, so prefer a check that runs on paths that already run. Do NOT reach across homes to arm anything: bin/fm-watch-arm.sh is single-home by design because reaching across homes would kill sibling homes' watchers, and that boundary stays. Do NOT claim guarantees the runner does not make - its contract explicitly disclaims at-least-once and lossless delivery - and if a fix narrows a window rather than closing it, say which window remains. Every fix needed a test proven RED against current behaviour first.
Explicitly OUT of scope and deliberately not addressed: the third defect in this path, a source refreshing its snapshot without emitting and losing a transition, recorded as artemis-pr4224-intake-miss-2026-08-14.md in the coordinator's own home. It lives in that mate's own tooling and is routed separately; it is not fixed here and must not be assumed fixed.
Decisions and tradeoffs made while doing the work, which a reviewer reading only the diff would not know:
Delivery constraints: this is a permanent-fork divergence topic branched from upstream/main. The ordinary registration targets official upstream. Never force-push or rewrite this published topic branch, and do not routinely merge official upstream or fork main into it.
What Changed
Risk Assessment
✅ Low: The prior blocking issues are resolved, and the durable queue, acknowledgement, retry, timeout, logging, and lock-namespace invariants are consistently preserved across the changed paths.
Testing
No prior baseline log was supplied; targeted automation, base RED checks, concurrency/load stress, and two end-to-end CLI/state scenarios all succeeded after two test-only macOS/load fixture fixes.
Evidence: Second-mate wake-loop end-to-end transcript
Evidence: Process-event reconciliation end-to-end transcript
Evidence: Regression RED proof against base
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-procevent.sh:575- The intent requires that “reconcile declines a held generation and retries on a later cycle” so it cannot stall the watcher. However,nowaitcovers only the apply-lock acquisition; the adapter then runs synchronously. The real remote-reply adapter waits on another lifecycle lock and may perform an unbounded SSH-backed document fetch, whilefm-watch.shinvokes reconcile synchronously. A held lifecycle lock or wedged remote command can therefore stop the watcher cycle indefinitely and recreate deaf intake. Bound or detach actual adapter execution at this shared reconcile boundary while preserving retries and acknowledgement semantics.bin/fm-procevent.sh:202- The new apply-lock path creates the claim root but does not enforce the existing non-symlink invariant used byfm_procevent_source_lock_acquire. With a pending capture and symlinkedFM_PROCEVENT_CLAIM_ROOT, reconcile can create lock state through the redirected root and execute the adapter even though its later register/handled operations reject that root, potentially repeating application. Add a shared apply-lock acquisition helper that validates the source ID and requires a real, non-symlink root before either wait or try-lock.🔧 Fix: Bound supervision calls and hardened apply locks
4 errors still open:
bin/fm-watch.sh:743- The accepted property says “nothing the supervision loop SHELLS OUT TO can stop it from completing its cycle,” with only the wake-queue lock wait left residual.fm_backend_wait_transitionremains outsidefm_run_timed; its Herdr path performs unboundedherdr session listandagent getcalls, so a server wedged after capability probing can still stop the watcher permanently. Recommend bounding this shared event-wait boundary; otherwise explicitly add it to the residual contract.bin/fm-watch.sh:726- The capability probe now runs in a fresh shell that sources the canonical backend library, so the registeredfm-supervision-events.test.shoverrides are never called;CAP_CALLSremains zero and its memoization contract cannot pass. Move those fixtures to a subprocess-visible seam while retaining the one-probe assertion.bin/fm-pending-reply-lib.sh:670- The pending-reply probes now run in fresh shells, bypassing the backend fixtures infm-pending-reply.test.shthat drive fallback-idle, harness-scoping, and observation-cache assertions. Convert them to PATH/executable or otherwise subprocess-visible fakes.🔧 Fix: Bound remaining supervision calls
3 errors still open:
bin/fm-watch.sh:1018- Kill-after timeouts can exit 137 when KILL is required, but the new retry branch recognizes only 124. A TERM-ignoring check can therefore have partial output treated as complete and advance.last-check. Normalize 137 to the timeout outcome before consuming output or updating bookkeeping; update the timeout fixture to emulate that status.bin/backends/herdr.sh:2941- The event-only deadline was added to sharedfm_backend_herdr_agent_status_raw, also used insidefm-crew-stateand submit confirmation. In those watcher-owned subprocessestriage_logis undefined, so a 10-second timeout becomes empty output with exit 0 and the outer boundary records no required timeout log. Use an event-specific bounded probe or propagate the timeout status to the logging parent.bin/fm-procevent-lib.sh:93- The new apply-lock namespace collides with valid source-lock names: the apply lock forremote-reply-aand source lock forremote-reply-a.applyboth resolve toremote-reply-a.apply.lock, because dots are allowed in source IDs. A long application can consequently block an unrelated source's retirement or re-arm. Put source and apply locks in disjoint filename namespaces or validated subdirectories.🔧 Fix: Normalize bounded supervision failure handling
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-secondmate-wake-check.test.sh tests/fm-secondmate-liveness.test.sh tests/fm-procevent.test.sh tests/fm-remote-reply.test.shbin/fm-test-run.sh tests/fm-watch-bounded-calls.test.sh tests/fm-pending-reply.test.sh tests/fm-supervision-events.test.shafter test-fixture fixes30-iteration independent-process pending-reply concurrency stress and five consecutiveFM_BOUNDED_CASE=recovery_sendrepetitionsCurrent regression tests overlaid on base03bb1d8b78a8632ae2d9cea4c10868eb100e885e, confirming both defects are RED before the fixManualfm-secondmate-wake-check.sh probe/scan --startupscenario with idle, stalled, remote-route, deduplication, and foreign-home integrity controlsManual terminal-capture/reconciliation scenario using the realfm-procevent.sh, showing failed application remains pending, later reconciliation re-arms and acknowledges without a handler, and subsequent reconciliation is idempotent✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Suppress intentional child-shell and subshell lint findings
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.