Skip to content

fix: deduplicate steering-inbox re-rings during handling - #71

Merged
ironerumi merged 6 commits into
mainfrom
fm/fork-wake-noise-48-redesign
Oct 3, 2026
Merged

ironerumi merged 6 commits into
mainfrom
fm/fork-wake-noise-48-redesign

Conversation

@ironerumi

@ironerumi ironerumi commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Design note

One durable record per message is the source of truth for the steering-inbox re-ring ladder; it replaces the .ring-state + .escalated (+ .inflight) marker combination.

State record. <task>.inbox/.ring-state, one line, owned by the fork seam bin/fm-task-inbox-ladder.sh:
<msg> TAB <ringing|escalated> TAB <count> TAB <ring_at> TAB <seen_at>.
It is valid only for the message it names. A message with no record, or a record naming another message, is implicitly delivered, so a new oldest message is a fresh ladder by construction. handled is implicit: the message leaves the inbox root and the record is dropped once nothing unhandled remains.

Transitions (one event each, each one atomic temp-then-rename rewrite).

  • delivered -> ringing: the watcher attempts delivery (record_ring).
  • ringing -> ringing: a re-ring (count + 1), or a busy sighting after a ring (note_inflight moves seen_at).
  • delivered/ringing -> escalated: the stale wake is queued (a dead pane records escalated/0 without ringing).
  • any -> handled: the worker moves the message into handled/.

How it replaces the timer ladder. fm_task_inbox_due_action in bin/fm-task-inbox-lib.sh now reads that record instead of the timer markers: the next ring is due one grace after the later of the last ring and the last busy sighting, and a spent budget escalates after the same in-flight wait. A handling turn that dies leaves no further sightings, so it re-rings at the re-armed deadline. Suppression only moves the re-arm point; it never acks and never touches a message, so docs/watcher-continuity.md idempotency is untouched. The first prompt for a new message is the doorbell fm-send rings at delivery; the ladder governs only re-rings.

Write failures. An unwritable record surfaces once through the existing stale-wake path (inbox_steer_state_unwritable), which ends the watcher cycle, so it never rings or queues on every poll.

Intent

Build #48 (re-ring on state transition, not timer; dedupe wakes while handling is in flight) as a redesign. The first attempt was pulled from #70 by decision ("48 c is fine"): it layered marker files (.ring-state, .escalated, .inflight) around the timer-driven steering-inbox re-ring ladder, and four review rounds each found a new combination the markers got wrong - a fresh new oldest message not detected as a transition, a new message after an escalation delayed by a grace period, and an unwritable in-flight marker re-queuing the same stale wake on every poll. The decided direction: one durable state per message (delivered, ringing, handled, escalated) that rings only on state transitions. #45/#46 already landed in #70.

Issue acceptance criteria (from #48): duplicate same-kind rings during in-flight handling drop to about zero without delaying the first prompt for a genuinely new transition; lost-handling safety preserved (a handling turn that dies without ack re-rings at the re-armed deadline, crash path tested); suppression never acks and never consumes a row (docs/watcher-continuity.md idempotency untouched); transition detection uses existing durable markers, no new heuristics; fork-owned seam, no hot upstream file rewrites.

What Changed

  • Added a durable, atomic per-message ladder state with transition-aware ringing and escalation.
  • Re-armed deadlines after busy handling sightings while preserving unacknowledged rows and crash recovery.
  • Updated fm-send for immediate first delivery, added ladder coverage, and documented the fork-owned behavior.

Risk Assessment

⚠️ Medium: The state-machine redesign satisfies the reviewed functional paths, but its new busy-poll path can impose repeated backend and filesystem work during long handling turns.

Testing

The Test agent exceeded its invocation budget before live validation completed; no evidence was gathered for this head.

  • Outcome: ⚠️ 1 warning across 1 run (30m1s)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 3 warnings
  • 🚨 bin/fm-task-inbox-lib.sh:442 - The age check returns quiet before reading the durable record, so a genuinely new oldest row can wait a full grace period instead of ringing on transition. This contradicts the required behavior: “re-ring on state transition, not timer” and “without delaying the first prompt for a genuinely new transition.” The same invariant is codified incorrectly at tests/fm-task-inbox-ladder.test.sh:196, while tests/fm-task-inbox-ladder.test.sh:135,175,191 backdate new rows and therefore miss the failure. Detect the record/message transition before the age gate and ring immediately; this intent contradiction requires author confirmation.
  • ⚠️ tests/fm-task-inbox-ladder.test.sh:149 - The required criterion says “a handling turn that dies without ack ... crash path tested,” but this regression only fabricates old timestamps with set_record; it never exercises a handling process dying and the watcher recovering from that crash boundary. The lost-handling assertion at lines 151-165 therefore does not verify crash durability or restart behavior.

🔧 Fix applied.
1 warning still open:

  • ⚠️ bin/fm-watch.sh:579 - Every quiet poll for a ringing message captures the pane and rewrites .ring-state whenever it remains busy; fm_task_inbox_ladder_probe_due only checks state, ignoring seen_at (bin/fm-task-inbox-ladder.sh:112-128, bin/fm-task-inbox-lib.sh:481-492). A long handling turn therefore performs repeated backend captures and atomic writes instead of waiting until the re-armed deadline. Gate probing after a sighting, or otherwise avoid repeating the same busy-state transition until the deadline.

🔧 Fix applied.
3 warnings still open:

  • ⚠️ bin/fm-watch.sh:579 - Every quiet poll for a ringing message captures the pane and rewrites .ring-state whenever it remains busy; fm_task_inbox_ladder_probe_due only checks state, ignoring seen_at (bin/fm-task-inbox-ladder.sh:112-128, bin/fm-task-inbox-lib.sh:481-492). A long handling turn therefore performs repeated backend captures and atomic writes instead of waiting until the re-armed deadline. Gate probing after a sighting, or otherwise avoid repeating the same busy-state transition until the deadline.
  • ⚠️ bin/fm-task-inbox-ladder.sh:51 - The new parser rejects the base version's 3-field .ring-state records and ignores the base version's .escalated marker. After upgrading a persistent home, a previously ringing or escalated message is treated as delivered; once aged, the watcher can ring or escalate it again, duplicating a wake and losing its attempt budget. The sibling behavior is consumed at bin/fm-task-inbox-lib.sh:454. Decide whether upgrade compatibility is required; if so, migrate or consume the old markers before applying the new state semantics.
  • ⚠️ bin/fm-watch.sh:545 - The busy-sighting path conflates “no ringing record applies” with successful persistence: fm_task_inbox_note_inflight returns success when .ring-state is unreadable, malformed, or a directory because fm_task_inbox_ladder_note_inflight returns 0 for non-ringing state (bin/fm-task-inbox-ladder.sh:136). For an aged message with such an unwritable marker and a busy pane, bin/fm-watch.sh:610 silently skips the sighting and stale-wake report, repeating captures while never advancing bookkeeping. The quiet-probe sibling is bin/fm-watch.sh:577-581. Distinguish an absent/non-applicable record from a failed record read/write and route the latter through inbox_steer_state_unwritable; the throttle fix round left this sibling failure path behind.
⚠️ **Test** - 1 warning
  • ⚠️ The Test agent did not finish within its invocation budget. Reported: agent run tests timed out after 30m0s: agent last produced output 2s ago (255 observed); agent reported: pi exited: exit status 143. This is a budget or provider-slowness cut, not a code failure. Re-running the same request costs another full budget, so no further attempt is made automatically. If this repository's targeted tests or evidence gathering routinely approach the default 30m0s, raise test_agent_timeout in global config. Respond with fix to spend another budget: a repair turn runs only for selected findings other than this budget cut, then validation re-runs. Or abort and retry after raising the budget.
✅ **Document** - passed

✅ No issues found.

🔧 **Lint** - 1 issue found → auto-fixed ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix applied.
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

The re-ring ladder kept its state in .ring-state plus a separate .escalated
marker, and the earlier #48 attempt added a third (.inflight). Each marker had
to be reconciled with the others and with the current oldest message, and four
review rounds found a new combination they got wrong.

Design: one durable record per message, <task>.inbox/.ring-state, owned by the
new fork seam bin/fm-task-inbox-ladder.sh:
  <msg> TAB <ringing|escalated> TAB <count> TAB <ring_at> TAB <seen_at>
The record is valid only for the message it names; a message with no record, or
a record naming another message, is the implicit delivered state, so a new
oldest message is a fresh ladder by construction. Handled is implicit: the
message leaves the inbox root and the record is dropped once nothing unhandled
remains. Every write is temp-then-rename in the inbox directory.

Transitions, one event each: delivered -> ringing (watcher delivery attempt),
ringing -> ringing (re-ring, or a busy sighting after a ring updates seen_at),
any -> escalated (stale wake queued; a dead pane records escalated/0 without
ringing). fm_task_inbox_due_action now reads that record instead of the timer
ladder: the next ring is due one grace after the later of the last ring and the
last busy sighting, and a spent budget escalates after the same in-flight wait.
A handling turn that dies leaves no further sightings, so it re-rings at the
re-armed deadline. Suppression only moves the re-arm point; it never acks and
never touches a message.

An unwritable record is reported through the existing single stale-wake path
(inbox_steer_state_unwritable), which ends the watcher cycle, so a write failure
surfaces once per cycle and never rings or queues on every poll.

tests/fm-task-inbox-ladder.test.sh covers the issue's three sequences and the
review edge cases; tests/fm-task-inbox.test.sh follows the one-record format.
@ironerumi

Copy link
Copy Markdown
Owner Author

On-record decisions and validation notes for this PR.

review-1 (upgrade compatibility): no compatibility layer.
The standing hard-cutover rule applies. After upgrading a home with a message already in flight, an old 3-field .ring-state record or an old .escalated marker is simply ignored, so the message reads as delivered. The worst case is one extra ring or a reset attempt budget for that one message, which is benign. No migration was added.

Test step: agent budget cut, not a failure.
The pipeline's Test agent ran the full 30m0s invocation budget with no configured test command and was cut off (exit 143). That is a budget or provider-slowness cut, not a code failure, and the decision was to approve the step rather than spend another budget. The suites run locally on the implementation commit before the review fix rounds, all passing, were:

  • tests/fm-task-inbox.test.sh
  • tests/fm-task-inbox-ladder.test.sh (5 of 5 stable runs)
  • tests/fm-secondmate-reconcile.test.sh
  • tests/fm-send-remote-delivery.test.sh
  • the whole watcher-wake-lock family (24 scripts)

The review fix rounds then changed the code and tests (age-gate comment and first-doorbell test, crash-path test, busy-sighting throttle), so those local runs predate the final head. CI runs the full suite on the final head.

Review findings left open at approval.
The busy-poll throttle finding returned once after its fix round, and a new finding (an unreadable or unwritable .ring-state on the busy-sighting path skipping the stale-wake report, bin/fm-watch.sh:545) were left open when the review step was approved, as listed in the Review section above.

…t staging fixture. Relevant test and shellcheck -x pass
@ironerumi
ironerumi merged commit b87f28f into main Oct 3, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant