Skip to content

[P2] Stop a catch-up sweep from swallowing a live command (including \stop) it read from history first #38

Open

Description

@MatthewS65537

Severity: P2 · Confidence: high · Area: Catch-up / routing

Where

Problem

The thread sweep reads replies up to now (upperTs: timestampFromMs(state.now())), so a message can be read from history just before its live socket event is processed. History never replays commands: other commands are held (historical_command) and the thread floor is retired past them; a history \stop only calls cancelRecovery (sets canceledThroughTs) and never aborts the run. When the live event then arrives, classifyRecovery returns canceled owner_canceled (\stop) or held retired_history (others), and the router returns without executing or replying.

Failure scenario

The owner sends \stop while a run streams; the sweep's conversations.replies returns before the socket event is handled (Slack event latency > replies RTT, or after a socket blip: catch-up runs on recovery, then Slack redelivers). The stop is silently dropped and the agent keeps running. Same for \model x, \verbose off, etc.; the raced command also sets needsFreshIntent, which can hold the next missed prompt.

Evidence

Local probe cc_mnt/review-20261011/routing/probe.test.ts ("drops a live \stop / \model / \verbose"): sweep with the message in the replies page, then handleIncomingMessage(live) for the same message → \stop → canceled (owner_canceled), \model → held (retired_history), \verbose off → held (retired_history); nothing executed. Mechanism certain; real-world frequency medium.

Suggested fix

Give live delivery a grace window: the sweep only classifies/retires messages older than ~30–60s (upperTs = now - LIVE_GRACE_MS); younger rows wait for the next pass. Optionally a history \stop newer than the active run's prompt should actually abort it.

Acceptance

A sweep followed immediately by live delivery of the same \stop/\model executes the command exactly once.


Found in the 2026-10-11 codebase review of commit 22ce7ee (typecheck clean, 1,142/1,142 tests passing). Probe paths refer to the maintainer's local, gitignored review ledger.

Activity

  1. MatthewS65537 commented on Oct 11, 2026

    @MatthewS65537
    OwnerAuthor

    Proposed fix

    Approach: Give live delivery a short grace period. Neither history sweep classifies, retires or dispatches a message younger than LIVE_GRACE_MS (30s). Its socket event therefore makes the first claim, and history only acts on messages that genuinely never arrived. The race exists only because the scan's upper bound is "now". Lowering that bound removes the race in one place, with no new state, receipts or policy rules.

    Changes:

    src/slack/catchup.ts

    • Add the constant.
    • Set the bound-thread scan's upperTs to now - LIVE_GRACE_MS.
    • Skip the fetch entirely when nothing is outside the grace yet. Otherwise oldest > latest, e.g. a thread bound by a prompt a few seconds ago, whose cursor is prevTs(root).
    export const MAX_SNAPSHOT_CANDIDATES = 200;
    +/** History never classifies messages this young: their live event gets first claim (#38). */
    +export const LIVE_GRACE_MS = 30_000;
     …
         if (!scan) {
           const oldest = [thread.historyCursorTs, recovery.replayFloorTs, replayFloor(state.now())]
             .filter((ts): ts is string => !!ts).sort(compareTs).at(-1)!;
    -      scan = { upperTs: timestampFromMs(state.now()), afterTs: oldest, generation: recovery.bindingGeneration,
    +      const upperTs = timestampFromMs(state.now() - LIVE_GRACE_MS);
    +      if (compareTs(oldest, upperTs) > 0) continue; // nothing outside the live grace yet
    +      scan = { upperTs, afterTs: oldest, generation: recovery.bindingGeneration,
             intentVersion: recovery.intentVersion, candidates: [], complete: false };
         }
    • Apply the same horizon in sweepTopLevelMessages. A missed top-level command there is harmless, but a top-level prompt that history beats its live event to is currently run as a "history" replay and counted as a ghost replay. The watermark never passes the horizon, so younger messages are picked up next pass.
    -    const newer = page.messages.filter((m) => timestampMicros(m.ts) !== undefined && compareTs(m.ts, mark) > 0);
    +    const horizon = timestampFromMs(state.now() - LIVE_GRACE_MS);
    +    const newer = page.messages.filter((m) => timestampMicros(m.ts) !== undefined && compareTs(m.ts, mark) > 0
    +      && compareTs(m.ts, horizon) <= 0);

    The end-of-scan retireHistory(key, nextTs(scan.upperTs)) now moves the floor only to now - 30s. The live path's retired_history / owner_canceled checks therefore can no longer fire for a message whose event is still in flight.

    Tests:

    • test/router.test.ts (real router + sweepMissedMessages): a bound thread receives \verbose off posted 2s ago in a replies page.
      • After the sweep: fetchReplies got latest === timestampFromMs(NOW - LIVE_GRACE_MS), and no lastDecision or floor was recorded for it.
      • handleIncomingMessage(sameMsg) (live) returns "accepted" and the thread's verbose is "off".
      • A second case with \stop asserts the stop handler ran (abort called) instead of "canceled". This case is not prototyped; the \verbose case is.
    • test/catchup.test.ts: the same message is still classified (held historical_command) once the clock has moved past the grace. This guards that the grace delays recovery and does not disable it.
    • test/catchup.test.ts top-level: a root newer than the horizon is neither dispatched nor passed by the watermark.
    • Existing tests to update (prototype run: the four touched files plus the new tests pass):
      • catchup.test.ts "page-three stop": assert latest: timestampFromMs(NOW - LIVE_GRACE_MS).
      • "eight recent and two dormant slots": recent owner timestamps NOW - (LIVE_GRACE_MS + 1000).
      • "coalesces overlapping timer/reconnect calls": advance a mutable clock by LIVE_GRACE_MS before release, since a frozen clock now correctly skips the rerun.
      • start-reliability.test.ts "recovers a history-only prompt at ten seconds": it now recovers at the 40s tick.

    Risks / interactions:

    Alternatives considered:

    • Write a durable "history held" tombstone receipt and let a live delivery of the same ts override it. This needs new disposition semantics in claimMessage / classifyRecovery and still leaves \stop canceled-but-not-aborted.
    • Make a history \stop actually abort the active run. Worth considering separately, but it does not fix \model / \verbose loss.

    Effort: S. Confidence: high


    Proposal only, not implemented. Sketches are against 22ce7ee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions