fix(bin): bound recovery-episode reopen so a restarted watcher stays up - #5514
marcus-vsc-moreira wants to merge 19 commits into
Conversation
cc10860 to
58be1e8
Compare
|
Premise update from live production evidence After this PR's fix landed (bounded recovery-episode reopen), firstmate reproduced the reported defect again live, with this bound already active, using standalone This does not contradict this PR's fix - Keeping this PR's fix in scope: the reopen-bound hardening is correct and independently needed (the unacknowledged-resurface case it fixes was confirmed live in the reported home). |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ "$READ_ONLY" -eq 0 ] && [ ! -e "$STATE/.lock" ] && [ ! -L "$STATE/.lock" ]; then | ||
| "$SCRIPT_DIR/fm-lock.sh" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
fm-remote-secondmate-control.sh retire runs from another harness session while the target home has no session lock, this guard calls fm-lock.sh for the target home without checking ownership. The new lock records the caller's PID, so the target session can be refused as a foreign owner and cannot restore its supervision.
Knowledge Base Used:
| if [ -n "$acked_generation" ] && [ -n "$FM_RECOVERY_MARKER_TOKEN" ] \ | ||
| && [ "${FM_RECOVERY_MARKER_TOKEN##*:}" = "$acked_generation" ]; then | ||
| rm -f -- "${marker}.reopen-count" "${marker}.reopen-settled" 2>/dev/null || true |
There was a problem hiding this comment.
--ack-through command is retried for a settled generation while newer rows remain queued, it can consume zero rows and still reach this snapshot. The matching generation clears the settlement sentinel, so the next arm re-announces the episode and exits instead of keeping the watcher up until the next drain.
Knowledge Base Used: Watch and wake workflows
| # (_fm_recovery_marker_ack) or arm-check minting a fresh episode from a missing | ||
| # or invalid marker both clear the counter, so this bound never shortens the | ||
| # once-per-genuine-generation resurface a live, attentive session relies on. | ||
| FM_RECOVERY_REOPEN_LIMIT=${FM_RECOVERY_REOPEN_LIMIT:-3} |
There was a problem hiding this comment.
FM_RECOVERY_REOPEN_LIMIT is set to a nonnumeric value, it passes through unchanged and each numeric comparison fails. A stuck episode then keeps reopening on every restart. Validating the setting would prevent a configuration typo from defeating this safeguard.
| case "$field" in | ||
| comm=) printf '%s\n' opencode ;; | ||
| args=) printf '%s\n' opencode ;; | ||
| ppid=) printf '%s\n' 1 ;; | ||
| esac |
There was a problem hiding this comment.
ps stub calls every queried PID opencode, including the short-lived fm-lock.sh process. The test can therefore pass with a lock owned by a PID that is dead when the guard returns: it checks only that the PID is numeric. Using a live harness in the fixture and checking owner liveness would make this test catch ineffective repairs.
Knowledge Base Used: Restore Claude lock ownership after helper recycling
41b61ce to
5d056a0
Compare
marcus-vsc-moreira
changed the title
An announced-but-unacked recovery episode (state/.watcher-down) reopens into a brand new generation on every plain restart with no session or re-arm loop to run the printed acknowledgement, so each restart resurfaces "check: rearm-resurface" and exits again - the watcher never holds the lock or lives past its first beacon. Bound consecutive reopens of one stuck episode with FM_RECOVERY_REOPEN_LIMIT (default 3): past the limit, settle the episode to acked directly instead of minting another generation, so the next restart proceeds into its normal poll loop and stays armed. A real acknowledgement or a fresh downtime episode both clear the counter, so this never shortens the once-per-genuine-generation resurface a live session still relies on.
* docs: make watcher-continuity easier to read Restructure the prose into sections, lists, and tables without changing documented behavior. Every original heading, anchor, identifier, link target, and number is kept. * no-mistakes(review): Fix actor and supervision-host scope in watcher-continuity doc * no-mistakes(review): Make readiness TERM and retry conditional on unready successor
… home is gone (kunchenguid#5552) * fix(bin): refuse watchers from disposable checkouts and exit when the home is gone Fixes kunchenguid#321 Fixes kunchenguid#4760 A watcher armed from a disposable no-mistakes validation checkout under .no-mistakes/worktrees/ outlived the validation step and kept writing the real home's state, and a running watcher never noticed when its home, state directory, or code root disappeared. The arm now refuses from such a checkout with the typed failure line, the watcher checks once per poll that its home, state directory (or its own lock holder record), and bin directory still exist and exits with a logged reason scoped to itself, and the shared test helpers reap every watcher a suite armed for a temporary home through the home-scoped stop. * no-mistakes(lint): fix SC1007 by assigning empty string in watch-arm test * no-mistakes(ci): Found and fixed a genuine, reproducible hang introduced by this branch's test-watcher reaper, which is what killed both CI checks (serial-2 cancelled at the 30-min cap; Lint 2 exit 143 = the suite's own TERM-trap code). Root cause: test_drain_asserts_watcher_liveness (tests/fm-wake-queue.test.sh) fabricates a .watch.lock whose pid is the test runner's own $$ with the runner's real identity, to make the drain believe a live watcher exists. The new make_case tracking registers that state dir for reaping, so at fm_test_cleanup the new fm_test_reap_watchers drives fm-watch-arm.sh --stop; its identity check matches (the fixture recorded the runner's identity) and it kill -TERMs the test runner. tests/lib.sh:231 is `trap 'fm_test_cleanup; exit 143' TERM`, so the TERM re-enters cleanup -> reap -> kills $$ again -> infinite loop until the runner cap. I reproduced this locally: the suite ran all tests then looped forever in cleanup spawning fm-watch-arm.sh --stop against a lock naming its own PID. Fix (tests/lib.sh, +5 lines): in fm_test_reap_watchers, skip any tracked lock whose pid equals our own $$ before driving --stop. This is the single shared reap boundary; seven $$-self-lock fixtures across four test files are all covered by the one guard, and real armed watchers (pid != $$) are still reaped. Invariant: the test reaper must only signal real armed watcher processes, never the test runner itself. Verified locally: tests/fm-wake-queue.test.sh -> EXIT 0 (63 ok, no hang); tests/fm-watch-arm.test.sh -> EXIT 0 (21 ok, including test_reaper_stops_a_tracked_watcher, confirming the guard does not over-skip). Lint 2's exit 143 was the same shard/cap signature; a fresh CI run on this new commit will re-evaluate it --------- Co-authored-by: firstmate-oss <firstmate@kunchenguid.local>
An announced-but-unacked recovery episode (state/.watcher-down) reopens into a brand new generation on every plain restart with no session or re-arm loop to run the printed acknowledgement, so each restart resurfaces "check: rearm-resurface" and exits again - the watcher never holds the lock or lives past its first beacon. Bound consecutive reopens of one stuck episode with FM_RECOVERY_REOPEN_LIMIT (default 3): past the limit, settle the episode to acked directly instead of minting another generation, so the next restart proceeds into its normal poll loop and stays armed. A real acknowledgement or a fresh downtime episode both clear the counter, so this never shortens the once-per-genuine-generation resurface a live session still relies on.
…ed recovery episodes
sessionOwnsLock() in the OpenCode arm plugin reads state/.lock at face value and treats it as ownerless whenever the file is simply absent, same as a genuine foreign live owner. Per the successor-gap decision (Option B), leave that check and beginArm untouched; instead have fm-guard.sh - which only runs non-read-only after ownership was already verified this call chain - repair a missing lock via bin/fm-lock.sh's existing acquire path. An existing lock, foreign or not, is never touched, so the tested foreign-live-owner refusal keeps failing fast.
…ran pass. I ran the two new watcher tests against the old code, and both failed there. - **ci-3 (lock repair reachable from other callers):** No code change was needed. `bin/fm-guard.sh` is the same as on base, so the guard never calls `fm-lock.sh`. `bin/fm-remote-secondmate-control.sh` does not call `fm-lock.sh` either. Only `bin/fm-lock.sh`'s own acquire, run from the LOCK step in `bin/fm-session-start.sh`, can write a missing `state/.lock`. The test `tests/fm-guard-session-lock-repair.test.sh` checks that the guard never writes it (main, read-only and branch callers). - **ci-4 (retried empty ack):** In `bin/fm-wake-drain.sh`, the snapshot gets the ack generation only when the ack removed at least one row (`ACK_REMOVED > 0`). So a retried stale ack that removes nothing keeps `.watcher-down.reopen-settled`. The ack path that empties the queue (`fm_recovery_marker_ack`) is unchanged, as the earlier R3-2 fix requires. New test: `test_stale_zero_row_ack_keeps_bound_settle`. It checks that the sentinel stays and that the next watcher stays up without a resurface. - **ci-5 (non-numeric limit):** In `bin/fm-wake-lib.sh`, a non-numeric `FM_RECOVERY_REOPEN_LIMIT` now falls back to the default of 3. New test: `test_invalid_reopen_limit_falls_back_to_default`. It sets the limit to `three` and checks that the episode settles and the watcher stays up. - **ci-6 (repair test accepted a dead pid):** The `ps` stub now calls a pid a harness only if that pid is in a list of live harness pids. It sends every other query to the real `ps`. `fm-lock.sh` runs as a child of a live harness process. The test checks that `state/.lock` holds that harness pid and that the pid is alive. The foreign-owner test runs the same way. Checks: - shellcheck is clean on the four changed files. - `tests/fm-guard-session-lock-repair.test.sh`: 3 of 3 pass. - The 6 recovery tests in `tests/fm-watch-arm.test.sh`: all pass. I ran them alone, because on this host the full file stops early on a test that also fails on base (T1). - All four `tests/fm-wake-drain*.test.sh` files pass
c1d6e3c to
6780209
Compare
3 participants
Intent
Supervision of this firstmate home does not hold. The watcher is armed, beacons once, and then exits
within seconds, so the home sits unsupervised and the turn-end guard fires on nearly every turn.
This is a firstmate defect, not operator error. It was reproduced with no re-arm loop running, no
session lock present, and 18 tasks in flight. Arming reports success -
fm-watch-arm.sh --restartprints
watcher: started pid=... (beacon fresh)- andstate/.last-watcher-beatupdates, but thewatcher process is gone shortly after and
state/.watch.lockis absent.What is needed: supervision that stays up, so the home does not depend on someone re-arming it by
hand.
What Changed
bin/fm-wake-lib.shnow limits reopens of a stuck recovery episode that nobody acknowledged. Before, each plain watcher restart with queued rows reopened the same episode into a new generation, which made the watcher resurface and exit. Now a counter instate/.watcher-down.reopen-counttracks these reopens. AfterFM_RECOVERY_REOPEN_LIMITreopens (default 1; a non-numeric value falls back to 1), the episode is set to acked. Its generation is stored instate/.watcher-down.reopen-settled. After that, arm-check does not re-announce that episode because of queued rows, and a watcher lock close (--restartor stale-lock clear) does not publish new downtime for it. The queued rows stay durable for the next session's drain.bin/fm-wake-drain.sh, the ack snapshot clears the settle record only when the ack removed at least one row. A retried stale ack that removes nothing keeps the settle.FM_RECOVERY_REOPEN_LIMITand the two new state files indocs/configuration.md,docs/watcher-continuity.md("Bounded reopen") and the operational-home-layout skill. It adds regression tests totests/fm-watch-arm.test.sh. It also addstests/fm-guard-session-lock-repair.test.sh, which checks three things:fm-guard.shnever writes a missingstate/.lock, thefm-lock.showner path repairs it, and a foreign live lock is never rewritten.🤖 Generated with Claude Code
Risk Assessment
Testing
I drove the real
bin/fm-watch-arm.sh --restart,--stopandbin/fm-wake-drain.shin disposable marked lab homes and removed each lab in the same run. On base code the reported failure reproduced: every restart exited and no watch lock was left. On the change, the watcher settles on the second restart and holdsstate/.watch.lockfor 60s and more. It also survives a later restart. A config typo in the limit falls back to 1. A genuine ack plus new work still resurfaces. I also ran the 8 new watch-arm tests, the guard lock-repair test and the 4 wake-drain test files, and all passed. Those test-only checks are not live results, so I mark the two scenarios that only they cover as untested. There is no UI surface, so the evidence is CLI transcripts. I did not start a harness CLI primary (claude) in the lab, because the watcher arm scripts are the surface this change touches.Evidence: Live drive on change: restart #2 and #3 stay up with watch.lock
Source: Live drive on change: restart #2 and #3 stay up with watch.lock
Evidence: Live drive on base: every restart exits, no watch.lock (bug reproduced)
Source: Live drive on base: every restart exits, no watch.lock (bug reproduced)
Evidence: Live adversarial: typo limit, 60s soak, genuine ack still resurfaces
Source: Live adversarial: typo limit, 60s soak, genuine ack still resurfaces
Evidence: Lab drive scripts
Source: Lab drive scripts
Evidence: New watch-arm tests output
Source: New watch-arm tests output
Evidence: Guard lock-repair tests output
Source: Guard lock-repair tests output
Evidence: Base vs change restart summary
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-wake-lib.sh:991- Intent conformance: the intent says 'supervision that stays up, so the home does not depend on someone re-arming it by hand'. The reopen bound defaults to FM_RECOVERY_REOPEN_LIMIT=3. The watcher still resurfaces and exits on the first 3 restarts of a stuck announced episode that has queued rows. Only the 4th restart stays up. Sequence: marker announced:downtime:G, queue has rows, no session, no re-arm loop.fm-watch-arm.sh --restartruns, resurfaces and exits (count=1). The operator re-arms by hand (count=2, then count=3). Only then does the settle keep the watcher up. So in the reported setup the operator must still re-arm by hand 3 more times. This repeats for every new episode, because each new durable row mints a fresh generation. Does the default meet the intent, or should it be lower (for example 1 or 0)? This is a product decision.bin/fm-wake-lib.sh:1011- state/.watcher-down.reopen-count is not per-episode, but docs/watcher-continuity.md says 'consecutive reopens of one episode'. Only these clear the counter: a genuine ack, a settle, or an arm-check mint from a missing or invalid marker. A durable append that mints a fresh generation from an announced marker (_fm_recovery_marker_publish, source=append, bin/fm-wake-lib.sh:752) does not clear it. Sequence (limit 3): episode G1 is reopened twice (count=2). A new wake row is appended, so the marker becomes pending:downtime:G5. The arm announces G5 and resurfaces. The next restart reopens it (count=3). The restart after that settles it (count=4). So the new episode gets 1 reopen, not 3. Its first announce still happens, so no row is lost, but the bound is shorter than documented. Fix: remove the counter in _fm_recovery_marker_publish when an append writes a fresh generation, as arm-check already does for a missing or invalid marker.🔧 Fix applied.
1 info still open:
bin/fm-wake-lib.sh:1011- state/.watcher-down.reopen-count is not per-episode, but docs/watcher-continuity.md says 'consecutive reopens of one episode'. Only these clear the counter: a genuine ack, a settle, or an arm-check mint from a missing or invalid marker. A durable append that mints a fresh generation from an announced marker (_fm_recovery_marker_publish, source=append, bin/fm-wake-lib.sh:752) does not clear it. Sequence (limit 3): episode G1 is reopened twice (count=2). A new wake row is appended, so the marker becomes pending:downtime:G5. The arm announces G5 and resurfaces. The next restart reopens it (count=3). The restart after that settles it (count=4). So the new episode gets 1 reopen, not 3. Its first announce still happens, so no row is lost, but the bound is shorter than documented. Fix: remove the counter in _fm_recovery_marker_publish when an append writes a fresh generation, as arm-check already does for a missing or invalid marker.🔧 Fix applied.
3 infos still open:
bin/fm-wake-lib.sh:1011- state/.watcher-down.reopen-count is not per-episode, but docs/watcher-continuity.md says 'consecutive reopens of one episode'. Only these clear the counter: a genuine ack, a settle, or an arm-check mint from a missing or invalid marker. A durable append that mints a fresh generation from an announced marker (_fm_recovery_marker_publish, source=append, bin/fm-wake-lib.sh:752) does not clear it. Sequence (limit 3): episode G1 is reopened twice (count=2). A new wake row is appended, so the marker becomes pending:downtime:G5. The arm announces G5 and resurfaces. The next restart reopens it (count=3). The restart after that settles it (count=4). So the new episode gets 1 reopen, not 3. Its first announce still happens, so no row is lost, but the bound is shorter than documented. Fix: remove the counter in _fm_recovery_marker_publish when an append writes a fresh generation, as arm-check already does for a missing or invalid marker.docs/watcher-continuity.md:230- Round 2's R-2 fix (0475f89) updated the code comment at bin/fm-wake-lib.sh:992-996 to say that a durable append minting a fresh episode from an announced one also clears the reopen counter. The owner doc was not updated. docs/watcher-continuity.md:230 still says that only a real acknowledgement or a watcher start minting from a missing or invalid marker clears the counter. The doc now describes the bound wrongly. Fix: add the append-from-announced case to that sentence.bin/fm-wake-lib.sh:775- Round 2's R-2 fix removes.watcher-down.reopen-countinside _fm_recovery_marker_publish (source=append) before the queue row is written. In fm_wake_append_locked (bin/fm-wake-lib.sh:2106-2119), a later seq or queue write failure calls _fm_wake_append_recovery_restore_locked. That restores the marker to the old announced:downtime:G token, but the deleted counter is not restored. Sequence (limit 1): G was reopened once (count=1). An append from announced deletes the counter, then the queue write fails (for example a full disk). The marker goes back to announced:G with no counter. The next restart reopens G again (count=1) and exits, instead of settling and staying up. The cost is one extra resurface-and-exit, and only on a failed write. Fix: delete the counter only after the append succeeds (the success branch at bin/fm-wake-lib.sh:2121), or restore it in the restore path.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
live-drive.sh <worktree>/binin a disposable lab (bin/fm-lab-home.sh create): announced-unacked marker, 18 queued rows, nostate/.lock, three realbin/fm-watch-arm.sh --restartcalls, then--stopandbin/fm-wake-drain.shSamelive-drive.shagainst base 46d58d6 bin (viagit archiveinto a temp dir) to reproduce the defectlive-adversarial.sh:FM_RECOVERY_REOPEN_LIMIT=bogusfallback, 60s soak of the settled watcher, genuine drain--ack-throughplus a new row, then a restart that must resurfaceThe 8 newtests/fm-watch-arm.test.shcases (bounded reopen, queued settle, genuine-ack resurface, late ack, stale zero-row ack, invalid limit, append resets budget, failed append keeps count)bash tests/fm-guard-session-lock-repair.test.shbash tests/fm-wake-drain*.test.sh(4 files)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.