From 2b652299268421416d902d1af105fce091a6b0d5 Mon Sep 17 00:00:00 2001 From: YifuGu Date: Sat, 3 Oct 2026 12:31:38 +0900 Subject: [PATCH 1/6] Ring the steering inbox on state transitions from one per-message record 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, .inbox/.ring-state, owned by the new fork seam bin/fm-task-inbox-ladder.sh: TAB TAB TAB TAB 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. --- bin/fm-task-inbox-ladder.sh | 129 ++++++++++++ bin/fm-task-inbox-lib.sh | 136 ++++++------ bin/fm-test-run-fork.sh | 2 +- bin/fm-watch.sh | 63 ++++-- docs/scripts.md | 1 + tests/fm-task-inbox-ladder.test.sh | 326 +++++++++++++++++++++++++++++ tests/fm-task-inbox.test.sh | 13 +- 7 files changed, 574 insertions(+), 96 deletions(-) create mode 100644 bin/fm-task-inbox-ladder.sh create mode 100644 tests/fm-task-inbox-ladder.test.sh diff --git a/bin/fm-task-inbox-ladder.sh b/bin/fm-task-inbox-ladder.sh new file mode 100644 index 00000000000..f94afa2f184 --- /dev/null +++ b/bin/fm-task-inbox-ladder.sh @@ -0,0 +1,129 @@ +#!/usr/bin/env bash +# fm-task-inbox-ladder.sh - the one durable state record per steering-inbox +# message (fork issue #48: re-ring on state transition, not on a timer alone). +# +# Fork-owned seam sourced by bin/fm-task-inbox-lib.sh, which keeps owning the +# inbox contract and the ladder decision (fm_task_inbox_due_action). This file +# owns only the record: its format, its atomic write, and the transitions that +# change it. Nothing else in the inbox keeps ladder state. +# +# One record, .inbox/.ring-state, one line, tab separated: +# \t\t\t\t +# msg basename of the oldest unhandled message the record describes +# state ringing | escalated +# count delivery attempts spent on msg +# ring_at epoch of the last delivery attempt +# seen_at epoch the worker was last seen busy while msg was ringing, else 0 +# +# The record is valid only for the message it names. A message with no record, +# or whose name differs from the record's, is in the implicit `delivered` +# state, so a new oldest message is a fresh ladder by construction rather than +# by a reset someone must remember. `handled` is implicit too: an +# acknowledged message leaves the inbox root, and the record is removed once no +# unhandled message remains. +# +# Transitions, each driven by exactly one event and each ONE atomic rewrite: +# delivered -> ringing the watcher attempted delivery (record_ring) +# ringing -> ringing a re-ring (record_ring: count + 1) or a busy +# sighting after a ring (note_inflight: seen_at) +# ringing/delivered -> escalated the stale wake was queued (record_escalated) +# any -> handled the worker moved the message (record dropped) +# A busy sighting never acks, never touches the message, and only moves the +# re-arm point (see fm_task_inbox_due_action), so suppression cannot consume a +# row or change the idempotent re-handling semantics. +# +# Writes are temp-then-rename inside the inbox directory, so a reader never +# sees a partial record. A write failure returns 1 while the inbox exists and +# is a quiet no-op for a concurrently removed inbox; the watcher reports the +# failure through its one stale-wake path. +# +# Sourced by bin/fm-task-inbox-lib.sh; no side effects on source. + +fm_task_inbox_ladder_path() { # + printf '%s/%s.inbox/.ring-state' "$1" "$2" +} + +# Print " " for : the record's own +# values when it names , else the implicit delivered state. +fm_task_inbox_ladder_read() { # + local path rec_msg="" state="" count="" ring_at="" seen_at="" + path=$(fm_task_inbox_ladder_path "$1" "$2") + IFS=$(printf '\t') read -r rec_msg state count ring_at seen_at 2>/dev/null < "$path" || true + case "$state" in + ringing|escalated) ;; + *) rec_msg= ;; + esac + case "$count" in ''|*[!0-9]*) rec_msg= ;; esac + case "$ring_at" in ''|*[!0-9]*) rec_msg= ;; esac + case "$seen_at" in ''|*[!0-9]*) rec_msg= ;; esac + if [ -n "$rec_msg" ] && [ "$rec_msg" = "$3" ]; then + printf '%s %s %s %s' "$state" "$count" "$ring_at" "$seen_at" + else + printf 'delivered 0 0 0' + fi +} + +fm_task_inbox_ladder_write() { # + local dir path tmp + dir="$1/$2.inbox" + path=$(fm_task_inbox_ladder_path "$1" "$2") + [ -d "$dir" ] || return 0 + if [ ! -d "$path" ] \ + && tmp=$(mktemp "$dir/.ring-state.XXXXXX" 2>/dev/null) \ + && { printf '%s\t%s\t%s\t%s\t%s\n' "$3" "$4" "$5" "$6" "$7" > "$tmp"; } 2>/dev/null \ + && mv "$tmp" "$path" 2>/dev/null; then + return 0 + fi + [ -z "${tmp:-}" ] || rm -f "$tmp" 2>/dev/null || true + [ -d "$dir" ] || return 0 + return 1 +} + +fm_task_inbox_ladder_drop() { # + rm -f "$(fm_task_inbox_ladder_path "$1" "$2")" 2>/dev/null || true +} + +# A delivery attempt was made: a failed ring or a composer-protected skip still +# consumes budget, so neither an unreadable pane nor a permanently blocked +# composer can retry silently forever. +fm_task_inbox_ladder_record_ring() { # + local base=${3##*/} state count ring_at seen_at + IFS=" " read -r state count ring_at seen_at < is durably queued. Wake-before-record ordering +# favors at-least-once recovery: a crash or write failure can cause a rare +# duplicate wake, never a lost one. A dead-pane escalation never rang, so it +# records count 0. +fm_task_inbox_ladder_record_escalated() { # + local base=${3##*/} state count ring_at seen_at + IFS=" " read -r state count ring_at seen_at < + local state count ring_at seen_at + IFS=" " read -r state count ring_at seen_at < is ringing: re-arm the next ring from this +# sighting. Anything but a ringing record for is left untouched. +fm_task_inbox_ladder_note_inflight() { # + local state count ring_at seen_at + IFS=" " read -r state count ring_at seen_at <.inbox/handled/ the worker's `mv` here IS the acknowledgement # .inbox/.seq.lock serializes sequence allocation across writers # (the session and the away daemon) -# .inbox/.ring-state watcher re-ring ladder: "\t\t" -# .inbox/.escalated oldest-message name already surfaced as stale, -# so later polls suppress another escalation +# .inbox/.ring-state the one per-message ladder record (state, attempts, +# last ring, last busy sighting); format and +# transitions are owned by bin/fm-task-inbox-ladder.sh # .inbox/.retry-ring name of a fire-and-forget record still owed its # one retry ring (fm_task_inbox_mark_retry) # @@ -47,20 +47,28 @@ # lifetime even if every doorbell is duplicated. Concurrent writers serialize # on .seq.lock; the worst racing outcome is ordering, never loss. # -# Re-ring ladder (fm_task_inbox_due_action): an unhandled message older than -# FM_TASK_INBOX_GRACE_SECS is due one delivery attempt per grace period; an -# attempt may ring or be skipped to protect another draft in a proven pending -# composer; an unsubmitted copy of this doorbell is retried. After -# FM_TASK_INBOX_RING_MAX attempts without an acknowledgement it escalates. The -# caller owns the busy and recovery-grade endpoint checks: a busy pane waits, -# while a positively dead or missing endpoint skips delivery and the ladder and -# escalates directly. This library owns only the schedule and escalation marker. -# If attempt bookkeeping cannot be persisted while the record remains unhandled, -# the caller surfaces that failure instead of retrying silently; a concurrently -# removed inbox is a quiet no-op. Escalation deliberately queues the wake before -# writing the deduplication marker: normal polls surface a message once, while a -# crash or marker failure may produce a rare duplicate rather than silently lose -# a wake. +# Re-ring ladder (fm_task_inbox_due_action): a pure function of the oldest +# unhandled message and its one ladder record (bin/fm-task-inbox-ladder.sh), so +# it rings on state transitions. A message with no record is `delivered`: once +# older than FM_TASK_INBOX_GRACE_SECS it is due its first delivery attempt. A +# `ringing` message is due the next attempt one grace after its last +# presentation, where a busy sighting after the ring counts as that +# presentation: a handling turn in flight (the ring waits in the composer or +# the worker is mid-turn) is not rung again at the first idle poll. A turn that +# died leaves no further sightings, so lost handling re-rings at the re-armed +# deadline. After FM_TASK_INBOX_RING_MAX attempts without an acknowledgement it +# escalates, after the same in-flight wait; an `escalated` message stays quiet +# for recovery. A different oldest message never inherits another message's +# record. An attempt may ring or be skipped to protect another draft in a proven +# pending composer; an unsubmitted copy of this doorbell is retried. The caller +# owns the busy and recovery-grade endpoint checks: a busy pane waits, while a +# positively dead or missing endpoint skips delivery and the ladder and +# escalates directly. If the record cannot be persisted while the message +# remains unhandled, the caller surfaces that failure instead of retrying +# silently; a concurrently removed inbox is a quiet no-op. Escalation +# deliberately queues the wake before writing its record: normal polls surface a +# message once, while a crash or write failure may produce a rare duplicate +# rather than silently lose a wake. # # Retry ring (fm_task_inbox_mark_retry): only while config/wait-no-turns is # present. A fire-and-forget record never enters the ladder, but when @@ -94,6 +102,8 @@ _FM_TASK_INBOX_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" . "$_FM_TASK_INBOX_LIB_DIR/fm-wake-lib.sh" # shellcheck source=/dev/null . "$_FM_TASK_INBOX_LIB_DIR/fm-backend.sh" +# shellcheck source=bin/fm-task-inbox-ladder.sh +. "$_FM_TASK_INBOX_LIB_DIR/fm-task-inbox-ladder.sh" FM_TASK_INBOX_SCHEMA='fm-task-inbox.v1' FM_TASK_INBOX_GRACE_DEFAULT=90 @@ -405,17 +415,16 @@ fm_task_inbox_clear_retry() { # # The re-ring ladder decision for one task. Prints exactly one of: # quiet nothing due (healthy, within grace or spacing, -# or already escalated for the current oldest) +# handling seen in flight, or already escalated) # ring one doorbell re-ring is due # escalate attempt budget spent; surface as stale # retry a fire-and-forget record's one retry ring is due -# An empty inbox also resets the ladder bookkeeping so the next message starts -# a fresh ladder. +# An empty inbox also drops the ladder record so the next message starts fresh. fm_task_inbox_due_action() { # - local dir oldest base now grace max ladder rec_base count last + local dir oldest base now grace max state count ring_at seen_at last dir=$(fm_task_inbox_dir "$1" "$2") if ! oldest=$(fm_task_inbox_oldest_unhandled "$1" "$2"); then - rm -f "$dir/.ring-state" "$dir/.escalated" 2>/dev/null || true + fm_task_inbox_ladder_drop "$1" "$2" # The one retry ring exists only while config/wait-no-turns is present. # Absent, a mark is left untouched and the inbox stays quiet, as before. if [ -e "${FM_CONFIG_OVERRIDE:-${FM_HOME:-}/config}/wait-no-turns" ]; then @@ -436,33 +445,27 @@ fm_task_inbox_due_action() { # printf 'quiet' return 0 fi - count=0 - last=0 - ladder=$(cat "$dir/.ring-state" 2>/dev/null || true) - IFS=$(printf '\t') read -r rec_base count last </dev/null || true - fi - case "$count" in ''|*[!0-9]*) count=0 ;; esac - case "$last" in ''|*[!0-9]*) last=0 ;; esac - if [ "$(cat "$dir/.escalated" 2>/dev/null || true)" = "$base" ]; then + if [ "$state" = escalated ]; then printf 'quiet' return 0 fi + now=$(date +%s) max=$(fm_task_inbox_ring_max) if [ "$count" -ge "$max" ]; then + # A spent budget escalates at once, unless handling was seen after the last + # ring and has not been seen for a full grace. + if [ "$seen_at" -gt "$ring_at" ] && [ "$((now - seen_at))" -lt "$grace" ]; then + printf 'quiet' + return 0 + fi printf 'escalate %s %s' "$oldest" "$count" return 0 fi - now=$(date +%s) + last=$ring_at + [ "$seen_at" -le "$last" ] || last=$seen_at if [ "$((now - last))" -lt "$grace" ]; then printf 'quiet' return 0 @@ -470,41 +473,32 @@ EOF printf 'ring %s' "$oldest" } -# Advance the ladder after a delivery attempt. A failed ring or a composer- -# protected skip still consumes budget so neither an unreadable pane nor a -# permanently blocked composer can retry silently forever. A positively dead or -# missing endpoint never enters the ladder: the watcher escalates it directly. -# A concurrently removed inbox is a successful no-op; otherwise failure means -# the caller must surface the unwritable ladder while the record remains +# Whether a busy sighting would change the schedule: the oldest unhandled +# message is ringing (see fm_task_inbox_ladder_probe_due). +fm_task_inbox_inflight_probe_due() { # + local oldest + oldest=$(fm_task_inbox_oldest_unhandled "$1" "$2") || return 1 + fm_task_inbox_ladder_probe_due "$1" "$2" "${oldest##*/}" +} + +# Record that the worker was busy while the oldest unhandled message is +# ringing, so the next ring waits a full grace from this sighting. +fm_task_inbox_note_inflight() { # + local oldest + oldest=$(fm_task_inbox_oldest_unhandled "$1" "$2") || return 0 + fm_task_inbox_ladder_note_inflight "$1" "$2" "${oldest##*/}" +} + +# Advance the ladder after a delivery attempt. A positively dead or missing +# endpoint never enters the ladder: the watcher escalates it directly. Failure +# means the caller must surface the unwritable record while the message remains # unhandled. fm_task_inbox_record_ring() { # - local dir base ladder rec_base count last - dir=$(fm_task_inbox_dir "$1" "$2") - base=${3##*/} - count=0 - ladder=$(cat "$dir/.ring-state" 2>/dev/null || true) - IFS=$(printf '\t') read -r rec_base count last < "$dir/.ring-state"; } 2>/dev/null; then - [ -d "$dir" ] || return 0 - return 1 - fi + fm_task_inbox_ladder_record_ring "$@" } # Mark the current oldest as escalated after its stale wake is durably queued, -# suppressing another wake on later polls. Wake-before-marker ordering favors -# at-least-once recovery: a crash or marker failure can cause a rare duplicate; -# stuck-crewmate-recovery owns the message from here. +# suppressing another wake on later polls. fm_task_inbox_record_escalated() { # - local dir - dir=$(fm_task_inbox_dir "$1" "$2") - [ -d "$dir" ] || return 0 - if ! { printf '%s\n' "${3##*/}" > "$dir/.escalated"; } 2>/dev/null; then - [ -d "$dir" ] || return 0 - return 1 - fi + fm_task_inbox_ladder_record_escalated "$@" } diff --git a/bin/fm-test-run-fork.sh b/bin/fm-test-run-fork.sh index a3812d85392..75b94e8ceea 100644 --- a/bin/fm-test-run-fork.sh +++ b/bin/fm-test-run-fork.sh @@ -20,7 +20,7 @@ fm_fork_family_for_basename() { # fm-merge-local.test.sh|fm-pr-merge-admin.test.sh|fm-task-register.test.sh) printf '%s\n' pr-forge ;; - fm-wake-autoack.test.sh) + fm-wake-autoack.test.sh|fm-task-inbox-ladder.test.sh) printf '%s\n' watcher-wake-lock ;; fm-claude-keepwarm-selfwake.test.sh) diff --git a/bin/fm-watch.sh b/bin/fm-watch.sh index f6cd43fcbce..f5f59ddb5ee 100755 --- a/bin/fm-watch.sh +++ b/bin/fm-watch.sh @@ -522,18 +522,44 @@ inbox_steer_escalate_unavailable() { # wake "$reason" } +# The one report for an unwritable ladder record while its message stays +# unhandled: the doorbell cannot advance toward escalation, so it surfaces as a +# single stale wake (which ends this watcher cycle) rather than a silent retry +# on every poll. Acknowledgement or teardown makes the race quiet. +inbox_steer_state_unwritable() { # + local w=$1 task=$2 rec=$3 reason + if [ ! -f "$rec" ]; then + fm_task_inbox_due_action "$STATE" "$task" >/dev/null || true + return 0 + fi + [ -d "${rec%/*}" ] || return 0 + reason="stale: $w (steering-inbox ladder bookkeeping unwritable: ${rec%/*}/.ring-state cannot be written while $rec stays unhandled; the doorbell cannot advance toward escalation - inspect the inbox directory)" + fm_wake_append stale "$w" "$reason" || exit 1 + wake "$reason" +} + +# Record a busy sighting for the oldest unhandled message, surfacing an +# unwritable record through the same single report. +inbox_steer_note_inflight() { # + local rec + fm_task_inbox_note_inflight "$STATE" "$2" && return 0 + rec=$(fm_task_inbox_oldest_unhandled "$STATE" "$2") || return 0 + inbox_steer_state_unwritable "$1" "$2" "$rec" +} + # Steering-inbox loss detection, one cheap check per recorded window per poll. # Quiet when healthy: an absent, empty, or handled inbox costs one directory # glob and produces nothing. When the ladder (fm_task_inbox_due_action, the # policy owner) reports a due action, a busy pane just waits - the record is # durable and the worker will reach a turn boundary - an idle pane gets one -# delivery attempt, and a spent attempt budget surfaces as an ordinary stale -# wake for stuck-crewmate-recovery, and a pane whose agent is positively dead -# or missing skips the ladder altogether: it is never typed into and surfaces -# as that same stale wake exactly once. If the attempt's ladder write fails while -# its record remains unhandled, that unwritable state surfaces through the same -# stale path instead of silently re-ringing forever; acknowledgement or teardown -# still makes the race quiet. The attempt is data-plane typing or a +# delivery attempt (a busy pane after a ring is recorded as handling in flight, +# which re-arms the next ring from that sighting), and a spent attempt budget +# surfaces as an ordinary stale wake for stuck-crewmate-recovery, and a pane +# whose agent is positively dead or missing skips the ladder altogether: it is +# never typed into and surfaces as that same stale wake exactly once. +# If the attempt's ladder write fails while its record remains unhandled, that +# unwritable state surfaces through the same stale path instead of silently +# re-ringing forever; acknowledgement or teardown still makes the race quiet. The attempt is data-plane typing or a # composer-protected skip, never a wake, so normal retries keep the watcher # blocking. A fire-and-forget record's one retry ring follows the same busy # wait, also waits while the worker has an open decision or blocker of its own @@ -545,7 +571,17 @@ inbox_steer_check() { # local w=$1 task=$2 action verb rec count tail40 reason ring_rc backend agent_state action=$(fm_task_inbox_due_action "$STATE" "$task") || return 0 verb=${action%% *} - [ "$verb" != quiet ] || return 0 + if [ "$verb" = quiet ]; then + # A ringing message with a busy pane is a handling turn in flight: record + # the sighting so the next ring waits a full grace from it. + if fm_task_inbox_inflight_probe_due "$STATE" "$task"; then + tail40=$(fm_backend_capture "$(window_backend "$w")" "$w" 40 "$(window_label "$w")" 2>/dev/null) || tail40= + if window_is_busy "$w" "$tail40"; then + inbox_steer_note_inflight "$w" "$task" + fi + fi + return 0 + fi if [ "$verb" = retry ] && [ -n "$(status_own_open_decisions "$STATE/$task.status")" ]; then return 0 fi @@ -571,6 +607,7 @@ inbox_steer_check() { # esac tail40=$(fm_backend_capture "$backend" "$w" 40 "$(window_label "$w")" 2>/dev/null) || tail40= if window_is_busy "$w" "$tail40"; then + [ "$verb" != ring ] && [ "$verb" != escalate ] || inbox_steer_note_inflight "$w" "$task" return 0 fi case "$verb" in @@ -582,15 +619,7 @@ inbox_steer_check() { # return 0 fi if ! fm_task_inbox_record_ring "$STATE" "$task" "$rec"; then - if [ ! -f "$rec" ]; then - fm_task_inbox_due_action "$STATE" "$task" >/dev/null || true - return 0 - fi - if [ -d "${rec%/*}" ]; then - reason="stale: $w (steering-inbox ladder bookkeeping unwritable: ${rec%/*}/.ring-state cannot be written while $rec stays unhandled; the doorbell cannot advance toward escalation - inspect the inbox directory)" - fm_wake_append stale "$w" "$reason" || exit 1 - wake "$reason" - fi + inbox_steer_state_unwritable "$w" "$task" "$rec" fi triage_log "steer-inbox delivery attempt: $task ${rec##*/} result=$ring_rc" ;; diff --git a/docs/scripts.md b/docs/scripts.md index fbd1ac9fec5..8a96064e06a 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -82,6 +82,7 @@ The shared no-mistakes gate lifecycle boundary is summarized in [architecture.md | `fm-review-diff.sh` | Review a crewmate branch or resolved PR head against the authoritative base | | `fm-marker-lib.sh` | Compatibility entry point for the from-firstmate carrier owned by `fm-operational-input.sh` | | `fm-task-inbox-lib.sh` | Single owner of durable steering-inbox records, acknowledgement, doorbells, and the delivery-attempt ladder | +| `fm-task-inbox-ladder.sh` | Fork-owned one-record-per-message ladder state for the steering inbox: states, atomic write, transitions, busy sightings | | `fm-pending-reply-lib.sh` | Parent-owned secondmate pending-reply expectations, recovery, and keyed escalation lifecycle | | `fm-secondmate-report.sh` | Optional helper that resolves the parent channel itself and appends a correlated status or document-pointer report | | `fm-extension.mjs` | Bind, inspect, verify, and strictly invoke trusted external process-event adapter packages | diff --git a/tests/fm-task-inbox-ladder.test.sh b/tests/fm-task-inbox-ladder.test.sh new file mode 100644 index 00000000000..18754064a24 --- /dev/null +++ b/tests/fm-task-inbox-ladder.test.sh @@ -0,0 +1,326 @@ +#!/usr/bin/env bash +# tests/fm-task-inbox-ladder.test.sh - the steering-inbox re-ring ladder rings on +# state transitions, from one durable record per message (fork issue #48). +# +# bin/fm-task-inbox-ladder.sh owns the record, bin/fm-task-inbox-lib.sh owns the +# decision (fm_task_inbox_due_action), bin/fm-watch.sh records busy sightings. +# Every case drives those production functions against a fixture inbox. Ordering +# is fixed by the fixture - records carry explicit epochs far from the grace +# boundary and the pane is a stub that answers busy or idle on demand - so no +# case sleeps or races a deadline. +# +# The issue's three named sequences: +# 1. ring -> in flight (busy, no ack, no state change) -> no second ring +# 2. ring -> state change (a different oldest message) -> immediate ring +# 3. ring -> ack lost (handling died) -> re-ring at the re-armed deadline +# And the edge cases that broke the marker-file attempt (PR 70 review rounds): +# - a fresh new oldest message is a transition, never quiet +# - a new message after an escalation (even a dead-pane one that never rang) +# inherits nothing from the escalated message +# - an unwritable record surfaces one stale wake and never rings or queues on +# every poll +set -u + +# shellcheck source=tests/wake-helpers.sh +. "$(dirname "${BASH_SOURCE[0]}")/wake-helpers.sh" + +TMP_ROOT=$(fm_test_tmproot fm-task-inbox-ladder) +TMP_ROOT=$(cd "$TMP_ROOT" && pwd) +GRACE=90 +MAX=3 + +inbox_lib() { # [args...] + local state=$1 + shift + FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fn=$2 + shift 2 + "$fn" "$@" + ' _ "$ROOT/bin/fm-task-inbox-lib.sh" "$@" +} + +new_state() { # -> echoes the state dir + mkdir -p "$TMP_ROOT/$1/state" + printf '%s' "$TMP_ROOT/$1/state" +} + +write_msg() { # [aged] -> echoes the record path + local rec + rec=$(inbox_lib "$1" fm_task_inbox_write "$1" t1 "$2") || fail "write failed" + [ "${3:-}" != aged ] || touch -t 202001010000 "$rec" + printf '%s' "$rec" +} + +# Put the ladder record for in an explicit state; epochs are seconds ago. +set_record() { # + local now seen=0 + now=$(date +%s) + [ "$6" = never ] || seen=$((now - $6)) + printf '%s\t%s\t%s\t%s\t%s\n' "${2##*/}" "$3" "$4" "$((now - $5))" "$seen" \ + > "$1/t1.inbox/.ring-state" +} + +record_state() { # -> the record's " " + cut -f1-3 "$1/t1.inbox/.ring-state" | tr '\t' ' ' +} + +record_seen_at() { # + cut -f5 "$1/t1.inbox/.ring-state" +} + +due() { # + FM_TASK_INBOX_GRACE_SECS=$GRACE FM_TASK_INBOX_RING_MAX=$MAX \ + inbox_lib "$1" fm_task_inbox_due_action "$1" t1 +} + +# The watcher's own inbox_steer_check against a stubbed pane and doorbell. +# Prints "ring" when the doorbell rang and "wake" when the cycle surfaced a wake +# (wake exits the cycle exactly as the real one does). +watcher_check() { # [agent-state] + local state=$1 busy=$2 agent=${3:-running} + # shellcheck disable=SC2016 # the stub script is expanded by the inner shell + env -u FM_TASK_ID FM_STATE_OVERRIDE="$state" FM_TASK_INBOX_GRACE_SECS=$GRACE \ + FM_TASK_INBOX_RING_MAX=$MAX FAKE_BUSY="$busy" FAKE_AGENT="$agent" bash -c ' + . "$1" + window_backend() { printf tmux; } + window_label() { printf fm-t1; } + fm_backend_capture() { printf "pane\n"; } + fm_backend_agent_state() { printf "%s" "$FAKE_AGENT"; } + window_is_busy() { [ "$FAKE_BUSY" = 1 ]; } + fm_task_inbox_ring() { printf ring; return 0; } + wake() { printf wake; exit 0; } + triage_log() { :; } + inbox_steer_check sess:fm-t1 t1 + ' _ "$ROOT/bin/fm-watch.sh" 2>/dev/null +} + +wake_rows() { # + local n + n=$(grep -c 'steering-inbox ladder bookkeeping unwritable\|unread firstmate instruction' \ + "$1/.wake-queue" 2>/dev/null || true) + printf '%s' "${n:-0}" +} + +# Sequence 1: a handling turn in flight is not rung again. +test_in_flight_handling_is_not_rung_again() { + local state rec before + state=$(new_state seq1) + rec=$(write_msg "$state" "please continue" aged) + [ "$(watcher_check "$state" 0)" = ring ] || fail "an aged delivered message must ring" + [ "$(record_state "$state")" = "${rec##*/} ringing 1" ] \ + || fail "the first ring must record ringing/1, got: $(record_state "$state")" + # The ring was spent a long time ago, but the worker is busy handling. + set_record "$state" "$rec" ringing 1 200 never + [ "$(due "$state")" = "ring $rec" ] || fail "setup: a ring 200s ago with no sighting must be due" + [ "$(watcher_check "$state" 1)" = "" ] || fail "a busy pane must not be rung" + [ "$(record_seen_at "$state")" -gt 0 ] || fail "the watcher must record the busy sighting in the one record" + [ "$(watcher_check "$state" 0)" = "" ] \ + || fail "the first idle poll after a handling turn must not re-ring" + [ "$(due "$state")" = quiet ] || fail "handling seen just now must hold the next ring, got: $(due "$state")" + before=$(record_state "$state") + [ "$(watcher_check "$state" 0)" = "" ] || fail "a second idle poll still inside grace must not ring" + [ "$(record_state "$state")" = "$before" ] || fail "a quiet poll must not change the record" + pass "ladder: ring -> in flight (no ack, no state change) -> no second ring" +} + +# Sequence 2: a state change rings at once. +test_state_change_rings_at_once() { + local state rec rec2 + state=$(new_state seq2) + rec=$(write_msg "$state" "first steer" aged) + set_record "$state" "$rec" ringing 1 10 5 + [ "$(due "$state")" = quiet ] || fail "setup: a fresh sighting must hold the ladder" + mv "$rec" "$state/t1.inbox/handled/" + rec2=$(write_msg "$state" "second steer" aged) + [ "$(due "$state")" = "ring $rec2" ] \ + || fail "a new oldest message must ring at once despite the previous sighting, got: $(due "$state")" + [ "$(watcher_check "$state" 0)" = ring ] || fail "the watcher must ring the new oldest message" + [ "$(record_state "$state")" = "${rec2##*/} ringing 1" ] \ + || fail "the ring must record the new message from a fresh count, got: $(record_state "$state")" + pass "ladder: ring -> state change -> immediate ring from a fresh ladder" +} + +# Sequence 3: lost handling re-rings at the re-armed deadline, then escalates. +test_lost_handling_rerings_at_the_rearmed_deadline() { + local state rec + state=$(new_state seq3) + rec=$(write_msg "$state" "please continue" aged) + set_record "$state" "$rec" ringing 1 500 40 + [ "$(due "$state")" = quiet ] || fail "handling seen 40s ago must hold the ring, got: $(due "$state")" + set_record "$state" "$rec" ringing 1 500 200 + [ "$(due "$state")" = "ring $rec" ] \ + || fail "handling that died (last seen 200s ago) must re-ring at the deadline, got: $(due "$state")" + [ "$(watcher_check "$state" 0)" = ring ] || fail "the watcher must re-ring lost handling" + [ "$(record_state "$state")" = "${rec##*/} ringing 2" ] || fail "the re-ring must spend an attempt" + [ "$(record_seen_at "$state")" = 0 ] || fail "a new ring must restart the sighting" + set_record "$state" "$rec" ringing "$MAX" 500 40 + [ "$(due "$state")" = quiet ] || fail "an escalation must wait while handling was just seen" + set_record "$state" "$rec" ringing "$MAX" 500 200 + [ "$(due "$state")" = "escalate $rec $MAX" ] \ + || fail "an exhausted ladder with stale sightings must escalate, got: $(due "$state")" + set_record "$state" "$rec" ringing "$MAX" 5 never + [ "$(due "$state")" = "escalate $rec $MAX" ] \ + || fail "an exhausted ladder with no sighting escalates at once, got: $(due "$state")" + pass "ladder: ring -> ack lost -> re-ring at the re-armed deadline, then escalation" +} + +# wake-review-2: a fresh new oldest record is never reported quiet. +test_fresh_new_oldest_is_a_transition() { + local state rec rec2 + state=$(new_state fresh-oldest) + rec=$(write_msg "$state" "first" aged) + set_record "$state" "$rec" ringing 2 20 10 + mv "$rec" "$state/t1.inbox/handled/" + rec2=$(write_msg "$state" "second" aged) + [ "$(due "$state")" = "ring $rec2" ] || fail "fresh new oldest must ring, got: $(due "$state")" + pass "ladder: a fresh new oldest message rings instead of reading quiet" +} + +# wake-review-6: a dead-pane escalation never rang, so no ring record existed +# for the old marker scheme to reset; the new message must start clean. +test_new_message_after_an_escalation_inherits_nothing() { + local state rec rec2 rec3 + state=$(new_state after-escalation) + rec=$(write_msg "$state" "first" aged) + [ "$(watcher_check "$state" 0 dead)" = wake ] || fail "a dead pane must surface its stale wake" + [ "$(record_state "$state")" = "${rec##*/} escalated 0" ] \ + || fail "a dead-pane escalation records escalated/0, got: $(record_state "$state")" + [ "$(due "$state")" = quiet ] || fail "an escalated message stays quiet for recovery" + mv "$rec" "$state/t1.inbox/handled/" + rec2=$(write_msg "$state" "second" aged) + [ "$(due "$state")" = "ring $rec2" ] \ + || fail "an aged new message after an escalation rings at once, got: $(due "$state")" + rec3=$(write_msg "$state" "third") + mv "$rec2" "$state/t1.inbox/handled/" + [ "$(due "$state")" = quiet ] || fail "a young new message waits its own grace, not more" + touch -t 202001010000 "$rec3" + [ "$(due "$state")" = "ring $rec3" ] \ + || fail "the young message rings at its own deadline, got: $(due "$state")" + pass "ladder: a new message after an escalation is delayed by nothing and inherits nothing" +} + +test_stale_record_for_another_message_is_ignored() { + local state rec + state=$(new_state stale-record) + rec=$(write_msg "$state" "only" aged) + printf '%s\tescalated\t3\t1\t1\n' "000.msg" > "$state/t1.inbox/.ring-state" + [ "$(due "$state")" = "ring $rec" ] || fail "a record naming another message must read as delivered" + printf 'garbage\n' > "$state/t1.inbox/.ring-state" + [ "$(due "$state")" = "ring $rec" ] || fail "an unparseable record must read as delivered" + printf '%s\tringing\t1\t100\n' "${rec##*/}" > "$state/t1.inbox/.ring-state" + [ "$(due "$state")" = "ring $rec" ] || fail "a truncated record must read as delivered" + pass "ladder: a record that does not describe the oldest message is ignored by construction" +} + +test_later_messages_do_not_reset_the_oldest() { + local state rec + state=$(new_state later-msg) + rec=$(write_msg "$state" "first" aged) + set_record "$state" "$rec" ringing 1 10 never + write_msg "$state" "second" aged >/dev/null + [ "$(due "$state")" = quiet ] || fail "a later message must not restart the oldest's ladder" + [ "$(record_state "$state")" = "${rec##*/} ringing 1" ] || fail "the oldest's record must survive" + pass "ladder: a later message leaves the oldest message's ladder alone" +} + +test_busy_before_the_first_ring_writes_nothing() { + local state + state=$(new_state busy-early) + write_msg "$state" "first" aged >/dev/null + [ "$(watcher_check "$state" 1)" = "" ] || fail "a busy pane must not be rung" + [ ! -e "$state/t1.inbox/.ring-state" ] || fail "a busy sighting before any ring must leave no record" + inbox_lib "$state" fm_task_inbox_inflight_probe_due "$state" t1 \ + && fail "a delivered message must not ask for a pane capture" + pass "ladder: a busy worker before the first ring changes nothing" +} + +test_suppression_never_acks_or_consumes() { + local state rec + state=$(new_state no-ack) + rec=$(write_msg "$state" "keep me" aged) + set_record "$state" "$rec" ringing 1 200 never + watcher_check "$state" 1 >/dev/null + watcher_check "$state" 0 >/dev/null + [ -f "$rec" ] || fail "suppression consumed the row" + [ -z "$(ls "$state/t1.inbox/handled/")" ] || fail "suppression acknowledged a row" + [ "$(inbox_lib "$state" fm_task_inbox_oldest_unhandled "$state" t1)" = "$rec" ] \ + || fail "the unhandled row must stay the oldest" + pass "ladder: suppression never acks and never consumes a row" +} + +test_record_writes_are_atomic_and_clean() { + local state rec leftovers f + state=$(new_state atomic) + rec=$(write_msg "$state" "x" aged) + inbox_lib "$state" fm_task_inbox_record_ring "$state" t1 "$rec" || fail "record_ring failed" + inbox_lib "$state" fm_task_inbox_record_ring "$state" t1 "$rec" || fail "second record_ring failed" + inbox_lib "$state" fm_task_inbox_record_escalated "$state" t1 "$rec" || fail "record_escalated failed" + [ "$(record_state "$state")" = "${rec##*/} escalated 2" ] \ + || fail "ring, ring, escalate must record escalated/2, got: $(record_state "$state")" + leftovers=0 + for f in "$state/t1.inbox"/.ring-state.*; do [ ! -e "$f" ] || leftovers=$((leftovers + 1)); done + [ "$leftovers" = 0 ] || fail "atomic writes left $leftovers temp files" + mv "$rec" "$state/t1.inbox/handled/" + [ "$(due "$state")" = quiet ] || fail "a handled inbox must be quiet" + [ ! -e "$state/t1.inbox/.ring-state" ] || fail "an acknowledged message must drop its record" + pass "ladder: one record, written atomically, dropped on acknowledgement" +} + +test_unwritable_record_surfaces_once_without_ringing_each_poll() { + local state rec out + state=$(new_state unwritable-sighting) + rec=$(write_msg "$state" "stuck" aged) + set_record "$state" "$rec" ringing 1 200 never + # Swap the record for a directory so every write fails while the message + # stays unhandled. (A directory still reads as an invalid record.) + rm -f "$state/t1.inbox/.ring-state" + mkdir "$state/t1.inbox/.ring-state" + out=$(watcher_check "$state" 0) + [ "$out" = ringwake ] || fail "an unwritable record rings once then surfaces one wake, got: $out" + [ "$(wake_rows "$state")" = 1 ] || fail "expected exactly one stale wake row, got $(wake_rows "$state")" + [ -f "$rec" ] || fail "the unhandled record disappeared" + pass "ladder: an unwritable record surfaces through the one stale-wake path" +} + +# A busy sighting that cannot be written is reported through the same single +# wake path, never rings, and queues one row per cycle rather than one per poll. +test_unwritable_sighting_surfaces_once_and_never_rings() { + local state rec out + state=$(new_state unwritable-sighting-write) + [ "$(id -u)" != 0 ] || { pass "ladder: unwritable sighting (skipped as root)"; return; } + rec=$(write_msg "$state" "stuck" aged) + set_record "$state" "$rec" ringing 1 200 never + chmod 555 "$state/t1.inbox" + out=$(watcher_check "$state" 1) + chmod 755 "$state/t1.inbox" + [ "$out" = wake ] || fail "an unwritable sighting must surface one wake and never ring, got: $out" + [ "$(wake_rows "$state")" = 1 ] || fail "expected one stale wake row, got $(wake_rows "$state")" + [ -f "$rec" ] && [ -z "$(ls "$state/t1.inbox/handled/")" ] || fail "the unhandled row must be untouched" + pass "ladder: an unwritable busy sighting surfaces once through the same path and never rings" +} + +test_unwritable_record_acknowledged_is_quiet() { + local state rec + state=$(new_state unwritable-acked) + rec=$(write_msg "$state" "stuck" aged) + mkdir "$state/t1.inbox/.ring-state" + mv "$rec" "$state/t1.inbox/handled/" + [ "$(watcher_check "$state" 0)" = "" ] || fail "an acknowledged message must stay quiet" + [ "$(wake_rows "$state")" = 0 ] || fail "an acknowledged message queued a bookkeeping wake" + pass "ladder: acknowledgement makes an unwritable record quiet" +} + +test_in_flight_handling_is_not_rung_again +test_state_change_rings_at_once +test_lost_handling_rerings_at_the_rearmed_deadline +test_fresh_new_oldest_is_a_transition +test_new_message_after_an_escalation_inherits_nothing +test_stale_record_for_another_message_is_ignored +test_later_messages_do_not_reset_the_oldest +test_busy_before_the_first_ring_writes_nothing +test_suppression_never_acks_or_consumes +test_record_writes_are_atomic_and_clean +test_unwritable_record_surfaces_once_without_ringing_each_poll +test_unwritable_sighting_surfaces_once_and_never_rings +test_unwritable_record_acknowledged_is_quiet diff --git a/tests/fm-task-inbox.test.sh b/tests/fm-task-inbox.test.sh index 3c9c8dce4d4..4c9cc840ce9 100644 --- a/tests/fm-task-inbox.test.sh +++ b/tests/fm-task-inbox.test.sh @@ -625,10 +625,10 @@ test_ring_ladder_policy() { [ "$action" = quiet ] || fail "a ring within the spacing window should be quiet, got: $action" # Backdate the ladder: the next ring becomes due, and at the budget the # action turns into a single escalation. - printf '001.msg\t1\t100\n' > "$state/t1.inbox/.ring-state" + printf '001.msg\tringing\t1\t100\t0\n' > "$state/t1.inbox/.ring-state" action=$(FM_TASK_INBOX_GRACE_SECS=60 FM_TASK_INBOX_RING_MAX=3 inbox_lib "$state" fm_task_inbox_due_action "$state" t1) [ "$action" = "ring $rec" ] || fail "an aged ladder should ring again, got: $action" - printf '001.msg\t3\t100\n' > "$state/t1.inbox/.ring-state" + printf '001.msg\tringing\t3\t100\t0\n' > "$state/t1.inbox/.ring-state" action=$(FM_TASK_INBOX_GRACE_SECS=60 FM_TASK_INBOX_RING_MAX=3 inbox_lib "$state" fm_task_inbox_due_action "$state" t1) [ "$action" = "escalate $rec 3" ] || fail "a spent ring budget should escalate, got: $action" # Escalation fires at most once per message. @@ -639,7 +639,7 @@ test_ring_ladder_policy() { mv "$rec" "$state/t1.inbox/handled/" action=$(FM_TASK_INBOX_GRACE_SECS=60 inbox_lib "$state" fm_task_inbox_due_action "$state" t1) [ "$action" = quiet ] || fail "a handled inbox should be quiet, got: $action" - [ ! -e "$state/t1.inbox/.escalated" ] || fail "the ack should clear the escalation marker" + [ ! -e "$state/t1.inbox/.ring-state" ] || fail "the ack should clear the ladder record" rec=$(inbox_lib "$state" fm_task_inbox_write "$state" t1 "next thing") age_path "$rec" action=$(FM_TASK_INBOX_GRACE_SECS=60 FM_TASK_INBOX_RING_MAX=3 inbox_lib "$state" fm_task_inbox_due_action "$state" t1) @@ -923,9 +923,8 @@ test_watcher_dead_pane_escalates_once_without_ringing() { || fail "the stale wake should say the agent has exited:"$'\n'"$(cat "$state/.wake-queue")" grep -qF "$rec" "$state/.wake-queue" || fail "the stale wake should name the record path" [ -f "$rec" ] || fail "the durable record must survive for recovery" - [ "$(cat "$state/t1.inbox/.escalated")" = "${rec##*/}" ] \ - || fail "the escalation marker should suppress further surfacing of this record" - [ ! -e "$state/t1.inbox/.ring-state" ] || fail "a dead pane must not enter the re-ring ladder" + [ "$(cut -f1-3 "$state/t1.inbox/.ring-state")" = "$(printf '%s\tescalated\t0' "${rec##*/}")" ] \ + || fail "the escalated record should suppress further surfacing without a delivery attempt" # The ladder is capped: nothing further is due for this record, so no later # poll rings the dead pane or queues a second wake. [ "$(inbox_lib "$state" fm_task_inbox_due_action "$state" t1)" = quiet ] \ @@ -950,7 +949,7 @@ test_watcher_dead_pane_ignores_stale_busy_state() { [ "$(grep -cF 'unread firstmate instruction' "$state/.wake-queue" 2>/dev/null || true)" = 1 ] \ || fail "a busy-marked dead pane should surface exactly once:"$'\n'"$(cat "$state/.wake-queue" 2>/dev/null)" [ -f "$rec" ] || fail "the durable record must survive stale busy-state recovery" - [ "$(cat "$state/t1.inbox/.escalated")" = "${rec##*/}" ] \ + [ "$(cut -f1-2 "$state/t1.inbox/.ring-state")" = "$(printf '%s\tescalated' "${rec##*/}")" ] \ || fail "stale busy-state recovery should suppress repeated surfacing" pass "watcher: dead-pane recovery overrides stale busy state" } From 15be7fe399e1a74b666086e148651564c6287ffd Mon Sep 17 00:00:00 2001 From: YifuGu Date: Sat, 3 Oct 2026 13:21:15 +0900 Subject: [PATCH 2/6] no-mistakes(review): Clarify first-doorbell timing and add crash recovery coverage --- bin/fm-task-inbox-lib.sh | 3 ++ tests/fm-task-inbox-ladder.test.sh | 76 ++++++++++++++++++++++++------ tests/fm-task-inbox.test.sh | 18 +++++++ 3 files changed, 83 insertions(+), 14 deletions(-) diff --git a/bin/fm-task-inbox-lib.sh b/bin/fm-task-inbox-lib.sh index 4a1f12264cd..cb8696c18a9 100644 --- a/bin/fm-task-inbox-lib.sh +++ b/bin/fm-task-inbox-lib.sh @@ -441,6 +441,9 @@ fm_task_inbox_due_action() { # fi base=${oldest##*/} grace=$(fm_task_inbox_grace_secs) + # fm-send's first prompt is the delivery doorbell; this age gate governs + # only watcher re-rings after that first attempt, so a fresh row is not + # delayed when fm-send is the writer. if [ "$(fm_path_age "$oldest")" -lt "$grace" ]; then printf 'quiet' return 0 diff --git a/tests/fm-task-inbox-ladder.test.sh b/tests/fm-task-inbox-ladder.test.sh index 18754064a24..2d2cb25c8c8 100644 --- a/tests/fm-task-inbox-ladder.test.sh +++ b/tests/fm-task-inbox-ladder.test.sh @@ -6,15 +6,16 @@ # decision (fm_task_inbox_due_action), bin/fm-watch.sh records busy sightings. # Every case drives those production functions against a fixture inbox. Ordering # is fixed by the fixture - records carry explicit epochs far from the grace -# boundary and the pane is a stub that answers busy or idle on demand - so no -# case sleeps or races a deadline. +# boundary and the pane is a stub that answers busy or idle on demand. The +# crash case alone uses a short real deadline to preserve the process boundary. # # The issue's three named sequences: # 1. ring -> in flight (busy, no ack, no state change) -> no second ring # 2. ring -> state change (a different oldest message) -> immediate ring # 3. ring -> ack lost (handling died) -> re-ring at the re-armed deadline # And the edge cases that broke the marker-file attempt (PR 70 review rounds): -# - a fresh new oldest message is a transition, never quiet +# - a fresh new oldest message waits for fm-send's first doorbell, then +# re-rings on the ladder's own age gate # - a new message after an escalation (even a dead-pane one that never rang) # inherits nothing from the escalated message # - an unwritable record surfaces one stale wake and never rings or queues on @@ -77,17 +78,22 @@ due() { # # The watcher's own inbox_steer_check against a stubbed pane and doorbell. # Prints "ring" when the doorbell rang and "wake" when the cycle surfaced a wake # (wake exits the cycle exactly as the real one does). -watcher_check() { # [agent-state] - local state=$1 busy=$2 agent=${3:-running} +watcher_check() { # [agent-state] [handler-pid] [grace] + local state=$1 busy=$2 agent=${3:-running} handler=${4:-} grace=${5:-$GRACE} # shellcheck disable=SC2016 # the stub script is expanded by the inner shell - env -u FM_TASK_ID FM_STATE_OVERRIDE="$state" FM_TASK_INBOX_GRACE_SECS=$GRACE \ - FM_TASK_INBOX_RING_MAX=$MAX FAKE_BUSY="$busy" FAKE_AGENT="$agent" bash -c ' + env -u FM_TASK_ID FM_STATE_OVERRIDE="$state" FM_TASK_INBOX_GRACE_SECS=$grace \ + FM_TASK_INBOX_RING_MAX=$MAX FAKE_BUSY="$busy" FAKE_AGENT="$agent" \ + FAKE_HANDLER_PID="$handler" bash -c ' . "$1" window_backend() { printf tmux; } window_label() { printf fm-t1; } fm_backend_capture() { printf "pane\n"; } fm_backend_agent_state() { printf "%s" "$FAKE_AGENT"; } - window_is_busy() { [ "$FAKE_BUSY" = 1 ]; } + window_is_busy() { + [ "$FAKE_BUSY" = 1 ] || { + [ -n "$FAKE_HANDLER_PID" ] && kill -0 "$FAKE_HANDLER_PID" 2>/dev/null + } + } fm_task_inbox_ring() { printf ring; return 0; } wake() { printf wake; exit 0; } triage_log() { :; } @@ -165,16 +171,57 @@ test_lost_handling_rerings_at_the_rearmed_deadline() { pass "ladder: ring -> ack lost -> re-ring at the re-armed deadline, then escalation" } -# wake-review-2: a fresh new oldest record is never reported quiet. -test_fresh_new_oldest_is_a_transition() { +# The ladder's age gate applies to re-rings; fm-send owns the first doorbell +# for a freshly written oldest record. +test_fresh_new_oldest_waits_for_first_doorbell() { local state rec rec2 state=$(new_state fresh-oldest) rec=$(write_msg "$state" "first" aged) set_record "$state" "$rec" ringing 2 20 10 mv "$rec" "$state/t1.inbox/handled/" - rec2=$(write_msg "$state" "second" aged) - [ "$(due "$state")" = "ring $rec2" ] || fail "fresh new oldest must ring, got: $(due "$state")" - pass "ladder: a fresh new oldest message rings instead of reading quiet" + rec2=$(write_msg "$state" "second") + [ "$(due "$state")" = quiet ] \ + || fail "the ladder must wait for fm-send's first doorbell, got: $(due "$state")" + touch -t 202001010000 "$rec2" + [ "$(due "$state")" = "ring $rec2" ] \ + || fail "a fresh message must become a re-ring only after its own grace, got: $(due "$state")" + pass "ladder: a fresh oldest record waits for fm-send's first doorbell" +} + +# A real handling process is observed busy after the first ring, then dies +# without moving the record. The watcher must preserve the row and re-ring only +# after the sighting's grace deadline. +test_crashed_handling_process_rerings_after_rearmed_deadline() { + local state rec handler seen ring_at out i=0 now + state=$(new_state crashed-handler) + rec=$(write_msg "$state" "please continue" aged) + [ "$(watcher_check "$state" 0)" = ring ] || fail "the first ring must land" + ring_at=$(cut -f4 "$state/t1.inbox/.ring-state") + ( while :; do sleep 1; done ) & + handler=$! + [ "$(watcher_check "$state" 0 running "$handler" 1)" = "" ] \ + || fail "a live handling process must not be rung again" + seen=$(record_seen_at "$state") + [ "$seen" -ge "$ring_at" ] || fail "the busy sighting was not recorded after the ring" + kill -KILL "$handler" 2>/dev/null || fail "the handling process could not be killed" + wait "$handler" 2>/dev/null || true + [ "$(watcher_check "$state" 0 running "$handler" 2)" = "" ] \ + || fail "a just-crashed handling process must wait for the re-armed deadline" + [ -f "$rec" ] || fail "a crashed handling process must not consume the message" + out= + while [ "$i" -lt 40 ]; do + out=$(watcher_check "$state" 0 running "$handler" 2) + [ "$out" = ring ] && break + sleep 0.1 + i=$((i + 1)) + done + [ "$out" = ring ] || fail "the watcher did not re-ring after the re-armed deadline" + now=$(date +%s) + [ "$((now - seen))" -ge 2 ] || fail "the re-ring preceded the busy sighting's deadline" + [ "$(record_state "$state")" = "${rec##*/} ringing 2" ] \ + || fail "the crash recovery ring must spend the next attempt" + [ -f "$rec" ] || fail "the crash recovery ring must leave the row unhandled" + pass "watcher: a killed handling process re-rings at the re-armed deadline" } # wake-review-6: a dead-pane escalation never rang, so no ring record existed @@ -314,7 +361,8 @@ test_unwritable_record_acknowledged_is_quiet() { test_in_flight_handling_is_not_rung_again test_state_change_rings_at_once test_lost_handling_rerings_at_the_rearmed_deadline -test_fresh_new_oldest_is_a_transition +test_fresh_new_oldest_waits_for_first_doorbell +test_crashed_handling_process_rerings_after_rearmed_deadline test_new_message_after_an_escalation_inherits_nothing test_stale_record_for_another_message_is_ignored test_later_messages_do_not_reset_the_oldest diff --git a/tests/fm-task-inbox.test.sh b/tests/fm-task-inbox.test.sh index 4c9cc840ce9..321294d770e 100644 --- a/tests/fm-task-inbox.test.sh +++ b/tests/fm-task-inbox.test.sh @@ -661,6 +661,23 @@ idle_capture() { # printf '%s\n' "$1/idle.capture" } +test_fm_send_rings_a_fresh_oldest_immediately() { + local dir state log rec + dir=$(setup_watch_case send-fresh) + state="$dir/state"; log="$dir/send.log"; : > "$log" + rec=$(inbox_lib "$state" fm_task_inbox_write "$state" t1 "retired steer") + mv "$rec" "$state/t1.inbox/handled/" + FM_HOME="$dir" FM_ROOT_OVERRIDE="$dir" PATH="$dir/fakebin:$PATH" \ + FM_SEND_SETTLE=0 FM_SEND_LOG="$log" \ + "$ROOT/bin/fm-send.sh" fm-t1 "fresh steer" 2>/dev/null \ + || fail "fm-send failed to deliver a fresh oldest steer" + rec="$state/t1.inbox/002.msg" + [ -f "$rec" ] || fail "fm-send did not create the fresh oldest record" + grep -qF 'Firstmate instruction waiting' "$log" \ + || fail "fm-send did not ring the fresh oldest record immediately" + pass "fm-send: a fresh oldest record gets its first doorbell without ladder grace" +} + test_watcher_rerings_idle_pane_quietly() { local dir state out log pid rec dir=$(setup_watch_case rering) @@ -969,6 +986,7 @@ test_fire_and_forget_records_never_enter_the_ladder test_fire_and_forget_retry_is_owed_once test_fire_and_forget_retry_is_quiet_without_the_flag test_ring_ladder_policy +test_fm_send_rings_a_fresh_oldest_immediately test_watcher_rerings_idle_pane_quietly test_watcher_waits_on_busy_pane test_watcher_quiet_on_healthy_inbox From 51a61c1458856518afe55f85b7be819a4d6a088d Mon Sep 17 00:00:00 2001 From: YifuGu Date: Sat, 3 Oct 2026 13:30:32 +0900 Subject: [PATCH 3/6] no-mistakes(review): Throttle repeated busy sightings within the re-ring grace window --- bin/fm-task-inbox-ladder.sh | 22 +++++++++++++------ bin/fm-task-inbox-lib.sh | 10 +++++---- tests/fm-task-inbox-ladder.test.sh | 34 ++++++++++++++++++++++++++---- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/bin/fm-task-inbox-ladder.sh b/bin/fm-task-inbox-ladder.sh index f94afa2f184..eee9cd6b938 100644 --- a/bin/fm-task-inbox-ladder.sh +++ b/bin/fm-task-inbox-ladder.sh @@ -106,24 +106,34 @@ EOF fm_task_inbox_ladder_write "$1" "$2" "$base" escalated "$count" "$ring_at" "$seen_at" } -# True while the oldest message is ringing: the only time a busy sighting -# changes the schedule, so the watcher captures the pane for it only then and -# a healthy or idle inbox stays one glob. -fm_task_inbox_ladder_probe_due() { # +# True while the oldest message is ringing and its busy sighting may be +# refreshed. A recent sighting gates the expensive pane capture and rewrite; +# after half a grace the watcher may refresh it again. +fm_task_inbox_ladder_sighting_due() { # + local ring_at=$1 seen_at=$2 grace=$3 half now + [ "$seen_at" = 0 ] && return 0 + half=$(( (grace + 1) / 2 )) + now=$(date +%s) + [ "$((now - seen_at))" -ge "$half" ] +} + +fm_task_inbox_ladder_probe_due() { # local state count ring_at seen_at IFS=" " read -r state count ring_at seen_at < is ringing: re-arm the next ring from this # sighting. Anything but a ringing record for is left untouched. -fm_task_inbox_ladder_note_inflight() { # +fm_task_inbox_ladder_note_inflight() { # local state count ring_at seen_at IFS=" " read -r state count ring_at seen_at < - local oldest + local oldest grace oldest=$(fm_task_inbox_oldest_unhandled "$1" "$2") || return 1 - fm_task_inbox_ladder_probe_due "$1" "$2" "${oldest##*/}" + grace=$(fm_task_inbox_grace_secs) + fm_task_inbox_ladder_probe_due "$1" "$2" "${oldest##*/}" "$grace" } # Record that the worker was busy while the oldest unhandled message is # ringing, so the next ring waits a full grace from this sighting. fm_task_inbox_note_inflight() { # - local oldest + local oldest grace oldest=$(fm_task_inbox_oldest_unhandled "$1" "$2") || return 0 - fm_task_inbox_ladder_note_inflight "$1" "$2" "${oldest##*/}" + grace=$(fm_task_inbox_grace_secs) + fm_task_inbox_ladder_note_inflight "$1" "$2" "${oldest##*/}" "$grace" } # Advance the ladder after a delivery attempt. A positively dead or missing diff --git a/tests/fm-task-inbox-ladder.test.sh b/tests/fm-task-inbox-ladder.test.sh index 2d2cb25c8c8..886f4777c84 100644 --- a/tests/fm-task-inbox-ladder.test.sh +++ b/tests/fm-task-inbox-ladder.test.sh @@ -78,16 +78,22 @@ due() { # # The watcher's own inbox_steer_check against a stubbed pane and doorbell. # Prints "ring" when the doorbell rang and "wake" when the cycle surfaced a wake # (wake exits the cycle exactly as the real one does). -watcher_check() { # [agent-state] [handler-pid] [grace] - local state=$1 busy=$2 agent=${3:-running} handler=${4:-} grace=${5:-$GRACE} +watcher_check() { # [agent-state] [handler-pid] [grace] [capture-count] + local state=$1 busy=$2 agent=${3:-running} handler=${4:-} grace=${5:-$GRACE} count_file=${6:-} # shellcheck disable=SC2016 # the stub script is expanded by the inner shell env -u FM_TASK_ID FM_STATE_OVERRIDE="$state" FM_TASK_INBOX_GRACE_SECS=$grace \ FM_TASK_INBOX_RING_MAX=$MAX FAKE_BUSY="$busy" FAKE_AGENT="$agent" \ - FAKE_HANDLER_PID="$handler" bash -c ' + FAKE_HANDLER_PID="$handler" FAKE_CAPTURE_COUNT_FILE="$count_file" bash -c ' . "$1" window_backend() { printf tmux; } window_label() { printf fm-t1; } - fm_backend_capture() { printf "pane\n"; } + fm_backend_capture() { + if [ -n "$FAKE_CAPTURE_COUNT_FILE" ]; then + capture_count=$(cat "$FAKE_CAPTURE_COUNT_FILE" 2>/dev/null || printf 0) + printf "%s\n" "$((capture_count + 1))" > "$FAKE_CAPTURE_COUNT_FILE" + fi + printf "pane\n" + } fm_backend_agent_state() { printf "%s" "$FAKE_AGENT"; } window_is_busy() { [ "$FAKE_BUSY" = 1 ] || { @@ -282,6 +288,25 @@ test_busy_before_the_first_ring_writes_nothing() { pass "ladder: a busy worker before the first ring changes nothing" } +test_repeated_busy_polls_are_gated_after_a_sighting() { + local state rec count_file before after + state=$(new_state busy-gated) + rec=$(write_msg "$state" "keep working" aged) + set_record "$state" "$rec" ringing 1 0 never + count_file="$state/capture-count" + printf '0\n' > "$count_file" + [ "$(watcher_check "$state" 1 running "" "$GRACE" "$count_file")" = "" ] \ + || fail "the first busy sighting must stay quiet" + [ "$(cat "$count_file")" = 1 ] || fail "the first busy sighting must capture the pane" + before=$(cat "$state/t1.inbox/.ring-state") + [ "$(watcher_check "$state" 1 running "" "$GRACE" "$count_file")" = "" ] \ + || fail "a repeated busy poll must stay quiet" + after=$(cat "$state/t1.inbox/.ring-state") + [ "$(cat "$count_file")" = 1 ] || fail "a recent sighting recaptured the pane" + [ "$after" = "$before" ] || fail "a recent sighting rewrote the ladder record" + pass "watcher: repeated busy polls skip capture and ladder rewrites inside half grace" +} + test_suppression_never_acks_or_consumes() { local state rec state=$(new_state no-ack) @@ -367,6 +392,7 @@ test_new_message_after_an_escalation_inherits_nothing test_stale_record_for_another_message_is_ignored test_later_messages_do_not_reset_the_oldest test_busy_before_the_first_ring_writes_nothing +test_repeated_busy_polls_are_gated_after_a_sighting test_suppression_never_acks_or_consumes test_record_writes_are_atomic_and_clean test_unwritable_record_surfaces_once_without_ringing_each_poll From 7cb7bcf2664a487addde02af25cc9ba07ea23bf4 Mon Sep 17 00:00:00 2001 From: YifuGu Date: Sat, 3 Oct 2026 14:11:51 +0900 Subject: [PATCH 4/6] no-mistakes(document): Updated steering-inbox ladder documentation --- AGENTS.md | 2 +- bin/fm-brief.sh | 8 +++++--- bin/fm-send.sh | 5 +++-- bin/fm-watch.sh | 10 ++++++---- docs/architecture.md | 2 +- docs/configuration.md | 4 ++-- docs/scripts.md | 2 +- docs/verification/runtime-backends.md | 2 +- 8 files changed, 20 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d1067393222..785112f0394 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -207,7 +207,7 @@ When the configured tasks-axi backlog gate applies, the spawn itself moves the w After spawning, confirm the worker is processing the brief and handle any trust dialog through `harness-adapters`. A persistent secondmate is recorded in the secondmate registry and runtime state, never as a backlog work item. -Steer a worker with ordinary text through fail-closed `fm-send`: the message becomes a durable record in the task's steering inbox (multi-line text is legal, local and remote alike) and the worker's terminal receives only a constant doorbell line, with the watcher re-ringing an unacknowledged local message and escalating a stuck one (`bin/fm-task-inbox-lib.sh`; `bin/fm-send.sh` owns the typed-plane carve-outs). +Steer a worker with ordinary text through fail-closed `fm-send`: the message becomes a durable record in the task's steering inbox (multi-line text is legal, local and remote alike) and the worker's terminal receives only a constant doorbell line, with the watcher re-ringing an unacknowledged local message and escalating a stuck one (`bin/fm-task-inbox-lib.sh` owns the inbox and ladder policy, while `bin/fm-task-inbox-ladder.sh` owns durable ladder state; `bin/fm-send.sh` owns the typed-plane carve-outs). A remote secondmate steer rides the same durable-inbox model through the remote transport; after an unconfirmed delivery, only the exact `FM_PENDING_REPLY_EXISTING_CORR=` resend command printed by `fm-send` is safe because it preserves the request body for remote enqueue deduplication (`bin/fm-send.sh` header). When a steer answers an open keyed decision or blocker, pass `fm-send`'s `--resolve-key` so the answer itself closes that decision record at answer time, identically for local and remote workers (contract: `bin/fm-send.sh` header). `fm-send` is the data plane for text the worker should read; never use its key or text paths for interrupt, exit, or other lifecycle control, because routing-marked lifecycle text becomes chat the worker reasons about instead of executing. diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 2ff9c727762..523453a62c8 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -98,7 +98,8 @@ # scaffold never emits a substitution a file-write tool would copy through. # Every scaffold also carries the steering-inbox receive-and-ack section: # process state/.inbox/*.msg in order and acknowledge each by moving it to -# handled/ (record, doorbell, and ladder owned by bin/fm-task-inbox-lib.sh). +# handled/ (record, doorbell, and ladder policy owned by bin/fm-task-inbox-lib.sh; +# durable ladder state owned by bin/fm-task-inbox-ladder.sh). # Ship tasks include a project-memory section bounding crewmate edits to a # project's AGENTS.md/CLAUDE.md: only corrections of factually wrong # information, including wrong information the task itself introduced - never @@ -347,8 +348,9 @@ STATUS_APPEND="echo \"{state} [at=]: {one short line}\" >> $STATUS_FILE & INBOX_DIR=$(shell_quote "$STATE/$ID.inbox") # The receive-and-ack half of the steering-inbox contract, included in every -# scaffold kind. The record format, doorbell line, and re-ring ladder are -# owned by bin/fm-task-inbox-lib.sh. The doorbell names the inbox as +# scaffold kind. The inbox record format, doorbell line, and re-ring policy are +# owned by bin/fm-task-inbox-lib.sh; durable ladder state is owned by +# bin/fm-task-inbox-ladder.sh. The doorbell names the inbox as # "$FM_TASK_INBOX", which bin/fm-spawn.sh exports into every launch; the full # path here remains the fallback for a worker launched without that export. # The doorbell itself is self-describing, so this section is reinforcement diff --git a/bin/fm-send.sh b/bin/fm-send.sh index 19562680313..c78e5ebcec3 100755 --- a/bin/fm-send.sh +++ b/bin/fm-send.sh @@ -54,8 +54,9 @@ # fire-and-forget record is excluded from that ladder; when config/wait-no-turns # is present and its ring here was skipped or failed, the watcher rings it # exactly once more. -# bin/fm-task-inbox-lib.sh owns the record format, the doorbell line, and the -# re-ring ladder. The composer pre-check before the ring is ADVISORY only: when +# bin/fm-task-inbox-lib.sh owns the inbox record format, doorbell line, and +# re-ring policy; bin/fm-task-inbox-ladder.sh owns the durable ladder state. +# The composer pre-check before the ring is ADVISORY only: when # the composer visibly holds pending text the ring is skipped with a notice and # the watcher re-rings an ordinary record later; no composer verdict is # delivery proof on this plane, and a failed ring never fails the send. diff --git a/bin/fm-watch.sh b/bin/fm-watch.sh index f5f59ddb5ee..c3d93ccb22e 100755 --- a/bin/fm-watch.sh +++ b/bin/fm-watch.sh @@ -82,7 +82,8 @@ # stale: (steering-inbox ladder bookkeeping unwritable: ...) # an unhandled record's ladder cannot advance; quiet # successful attempts never wake firstmate -# (bin/fm-task-inbox-lib.sh owns the ladder policy) +# (bin/fm-task-inbox-lib.sh owns the ladder policy; +# bin/fm-task-inbox-ladder.sh owns its durable state) # check: