fix: gate secondmate stall alerts on delivery progress - #3107
fix: gate secondmate stall alerts on delivery progress#3107zachlandes wants to merge 11 commits into
Conversation
* The stall detector's only test was the arrival age of the oldest unacknowledged row in a mate's foreign wake queue. Under the auto-arm model that mate's watcher appends at a turn boundary and the rows clear only once that turn's handling acknowledges them, so the age measured the mate's current turn rather than an abandoned queue, and alerted on every turn that outlived the threshold. One healthy mate produced 17 such alerts over two days, at ages from 63s to 506s. * No queue-derived measurement could separate the two cases, because every one of them is bounded by turn length and turn length is unbounded. Raising the threshold would only have traded the false positive for a later true positive, so the signal had to come from outside the queue. * Gated the existing alert on the mate's own liveness beacon, which its watcher touches on every poll and which this repo already trusts as the one record of a home's supervision being alive. Reading it only to suppress cannot blind a genuinely unserved queue, since a home nothing polls has a stale beacon under every supervision model. At each of the four confirmed false positives the mate's beacon was well inside the supervision grace. * Suppressed before any marker or receipt write, so a row held while the loop was alive still notifies if that loop later lapses. Claude-Session: https://claude.ai/code/session_01ULP6pSaTbTPgmw9ZreF9kF
e50baa3 to
246a22a
Compare
Confidence Score: 4/5The PR does not appear safe to merge until progress suppression respects row-level actor ownership. A branch can acknowledge only a newer eligible row while the committed global sequence watermark also covers an older main-owned row; the detector then suppresses that older row indefinitely even though it was never handled. Files Needing Attention: bin/fm-push-transition-lib.sh, bin/fm-watch.sh Reviews (2): Last reviewed commit: "no-mistakes(document): Clarify secondmat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ -z "$extra" ] && [ "$progress_seq" -ge "$seq" ]; then | ||
| case "$progress_state" in | ||
| committed) | ||
| continue | ||
| ;; |
There was a problem hiding this comment.
When a Pi branch handles an eligible row while an older main-owned row remains queued, the committed queue-wide watermark covers that excluded row and this unconditional continue suppresses its stall alert indefinitely, even though the branch actor cannot consume it.
* Refreshed the branch past upstream main so the pull request is mergeable again after upstream moved underneath the reviewed change. Merged rather than rebased so no already-pushed, already-reviewed commit is rewritten and no force-push is needed. * Resolved the one conflict, in docs/configuration.md, where two unrelated single-line edits happened to land on adjacent lines. The three-way comparison showed each line was changed by exactly one side, so the resolution is the union: upstream keeps its new FM_PAUSE_RESURFACE_SECS clause, and this branch keeps its FM_SECONDMATE_WAKE_STALL_SECS clause. No behavior was changed.
zachlandes
changed the title
|
We are following the maintainer's direction on #2912 to help this existing proposal rather than open a competing stall detector. Our operational evidence distinguishes three separate facts: an agent can be alive, its outer supervision can run, and its durable input can still remain unconsumed. A delivery marker therefore must be described as delivery evidence, not proof that the handling turn consumed the row. We can supply a bounded regression scenario for a busy supervisor versus genuinely unconsumed input, preserving foreign-queue bytes and making the documented detector limit explicit. We have not re-reviewed this branch or reproduced its current head; this offer does not prescribe a second progress mechanism or change the scope the maintainer accepted. |
2 participants
Intent
The primary watcher decided a secondmate's wake loop had stalled using the age of a single row: if the oldest row in that mate's durable wake queue had sat longer than
FM_SECONDMATE_WAKE_STALL_SECS, it raised a stall alert. Age alone does not mean stalled. A mate that is steadily delivering actionable wakes still has an oldest row that keeps getting older, so healthy homes produced stall alerts that were simply wrong, and the alert lost its meaning.This gates the alert on evidence of delivery instead of elapsed time. A mate now publishes a durable progress marker when its
wake()transition actually delivers an actionable wake, and the primary suppresses the stall alert only when that marker proves a delivery happened at or after the aged row was enqueued. Covering progress suppresses the alert at any row age; an empty or younger queue stays silent as before.The marker is committed causally rather than optimistically, which is where most of the diff goes. The transition takes bounded queue and progress locks, publishes a short-lived in-flight record before its bounded output, and commits the monotonic timestamp and authoritative queue-sequence watermark only after direct output succeeds, or the arm/checkpoint wrapper confirms it forwarded unchanged, or the away-mode daemon confirms durable handling. Serialization or forwarding failure leaves the progress unusable. The effect is that a clock change cannot reorder causality, and an absent, unreadable, malformed, future-dated, unconfirmed, abandoned, or lower-watermark marker never suppresses an alert - every one of those still alerts.
The deliberate cost: a home whose watcher polls but whose handling turn never begins is no longer alerted by the parent. That case is now invisible to this detector. It is accepted because the previous behaviour only caught it by also crying wolf on every healthy mate, which is what made the alert ignorable in practice.
Worth knowing before touching this: the primary only ever reads the foreign marker and queue. It never locks, consumes, or rewrites another home's wake queue, and the tests pin byte-for-byte preservation of foreign rows.
Verified by exercising the real watcher, checkpoint, and away-daemon interfaces end to end rather than by stubbing them - covering progress suppressed the false alert, while malformed, future-dated, unforwarded, abandoned, and pre-durable-crash progress each still produced one. The transcript is linked under Testing below.
Written with AI assistance and reviewed by the submitter before opening.
What Changed
Risk Assessment
✅ Low: The target preserves both reviewed and upstream ancestry, and the sole merge conflict is correctly resolved as the union of two independent documentation edits with no substantiated source regression.
Testing
Verified the merge descends from both the reviewed head and current upstream main, confirmed the sole conflict resolution preserves both documentation edits, and exercised the real watcher/checkpoint/daemon interfaces end-to-end: covering causal progress suppressed the false alert, while malformed, future-dated, unforwarded, abandoned, and pre-durable-crash progress still produced stall alerts. Focused tests succeeded and transient worktree files were removed.
Evidence: End-to-end stall detector behavior transcript
Source: End-to-end stall detector behavior transcript
Evidence: Merge ancestry and union conflict-resolution evidence
Source: Merge ancestry and union conflict-resolution evidence
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
git status --short,git log --oneline --decorate --graph -8, and targeted commit diffsgit merge-base --is-ancestor 246a22a60636251e551a9d9df08261d081fd0944 HEADandgit merge-base --is-ancestor 4ad8cbaeafc109a17c1af3911867b7fe9e04e801 HEADgit show --remerge-diff --format= --no-ext-diff HEAD -- docs/configuration.mdGenerated a transient runner fortest_secondmate_stall_follows_actionable_handoff_progress,test_secondmate_unforwarded_checkpoint_does_not_commit_progress, andtest_secondmate_away_daemon_crash_does_not_commit_progress, then removed itRepeated the focused executable scenarios while capturing delivered wakes, suppressed false positives, and retained true-positive alerts into reviewer-visible evidenceVerifiedgit status --shortafter cleanup✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Confirm lint passes with pinned tooling
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.