fix: publish the dispatch pointer for promoted queue turns - #166
Merged
Conversation
Queue promotion materializes a real user turn but never published `latestUserMsgId`, while ordinary execution advances `lastHandledUserMsgId` to every turn that runs. Once a queue drained, the two pointers were left permanently unequal, which `getPendingUserTurnActivationId` reads as a still-pending activation: the watcher waited out HISTORY_SYNC_WAIT_TIMEOUT_MS for a history entry that was already present and terminal, then negatively acknowledged an already-executed turn with a bogus `message_delivery_failed` notice. Observed on a local session where all three user turns completed normally; turns 2 and 3 arrived through the message queue, so the pointer still named turn 1 and the false delivery failure fired five minutes after the last turn finished. Any session whose last executed turn came from the queue reproduces it. Make promotion a dispatch producer and publish the pointer for the turn it materializes. `lastMissingHistoryUserMsgId` is deliberately not cleared: the producers that clear it supersede the acknowledged entry to `canceled` first, and promotion does not, so clearing it here would let that stale `pending` copy dispatch. Model: claude-fable-5
The previous commit fixed queue promotion by adding a second, hand-written meta write next to its history append. That is the same shape that caused the bug: the history entry and the activation pointer are one fact split across two documents, paired only by convention, so every producer is one forgotten line away from a turn that runs without advancing `latestUserMsgId`. Move both halves behind `SessionDocument.appendUserTurn`, which appends the entry and publishes the pointer in one call, and route queue promotion through it. The pointer lives in workspace meta because that is the activation index startup scans, so it cannot be derived from history without opening every session doc — which is why the binding has to be at the write site rather than a projection. The guard rejects a non-user entry so the method cannot publish a dispatch pointer for assistant output. `appendUserTurn` is covered directly against a real SessionDocument, and the queue-promotion regression tests now run against a real document too, so they exercise the binding instead of a fake that could drift from it. All four fail when the pointer write is removed. Model: claude-fable-5
The pointer pair is an activation INDEX, not the truth: startup scans it so it never opens thousands of session docs. It can therefore disagree with history, and exactly one disagreement is legitimate — the entry has not synced yet. The watcher assumed that was the ONLY explanation, so any stale pointer sent it into a five-minute wait and then a permanent negative acknowledgement plus a user-visible `message_delivery_failed`, all without once checking the record it was making claims about. Check it. When the pointed-at entry is already terminal in history, history has answered and waiting cannot change that, so retire the activation through the existing missing-history marker instead. That reuses the suppression mechanism rather than adding a field, and needs no change to `markMissingUserTurnRecovery`: it re-reads meta and returns early once the marker lands, which is what keeps the notice out of this path. Settling also matters on its own — leaving the pointer stale would keep the session watched forever. Any future producer that forgets the pointer now degrades to one no-op instead of a permanent false alarm. Also removes redundancy this line of work exposed: the `hasPendingUserTurnSignal` pass-through wrapper, a log line that computed and printed the same id twice, and the rationale I had duplicated across four places (kept once, in the binding). Model: claude-fable-5
Review catch (P1). The settle path claimed `lastMissingHistoryUserMsgId` as a cheap suppression slot, but that field holds exactly ONE turn and it is the only thing keeping an acknowledged-undelivered turn out of dispatch. Writing an unrelated id into it unprotects the recorded turn: its late-arriving `pending` entry becomes dispatchable again on the next pass and repeats whatever side effects it had, violating "recovery is a fresh send, never a revival". Reachable as: marker X (payload arrived late, still `pending`), then a promoted turn whose pointer write failed leaves `latest=A / lastHandled=B` with A terminal. Settling A evicted X. Collapse the stale pointer instead — the thing that is actually wrong — and leave the marker alone. `latestUserMsgId` folds onto `lastHandledUserMsgId` and a `processingUserMsgId` naming the same terminal turn is cleared, both behind a fresh re-read so a send published while we were reading history still wins. Note the same single-slot eviction exists on main in `markMissingUserTurnRecovery`, which overwrites the marker unconditionally. That is pre-existing and left alone here. Model: claude-fable-5
Settling re-reads meta before writing, because `latestUserMsgId` is producer-owned. When that re-read shows a producer has moved the pointer, the write was correctly skipped — but the caller still got `null` and fell straight into `markMissingUserTurnRecovery`, which re-reads meta itself, finds the brand-new turn, and marks it `message_delivery_failed`. A message published seconds earlier would be accused of never arriving, with no grace window at all: the same bug class this branch exists to remove, reachable because settling returns immediately where the old path waited five minutes. Report whether the pointer was actually settled and only short-circuit when it was. A moved pointer falls through to the ordinary history-sync wait, so the fresh turn gets its full window. Model: claude-fable-5
The stale-pointer guard re-read history that `checkHistoryAndQueue` had just read one line earlier. Both reads are in-memory mirror reads, but each one shallow-copies every entry, so a long session paid that twice on the path taken whenever metadata claims work the turn sources cannot find. Return the snapshot the check judged and let the guard reuse it. The snapshot predates queue promotion, but every path that appends also returns a turn, so it is accurate whenever `turn` is null — which is the only case a caller can act on it. Documented on the method. Model: claude-fable-5
Two review catches (both P1), same root: settlement was still expressed by mutating the pointers it was judging. Partial settlement reported success. With `processing` naming a terminal turn and `latest` naming a NEWER one, the patch retired only the first but was non-empty, so settling claimed the session was done. The caller short-circuited into missing-history recovery, which re-read meta, found the newer turn, and marked it `message_delivery_failed` with no grace window at all. Report settled only when no activation survives the patch; a survivor falls through to the ordinary history wait. Rewriting `latestUserMsgId` had no CAS. The field is producer-owned and the backing map is LWW, so a send published between the read and the write was overwritten. Worse than a false alarm: a fresh turn whose entry had not synced lost its activation, so the session went unwatched and the message was never run and never reported. Record the retirement in `settledActivationUserMsgId` instead and leave both producer pointers untouched. Unlike the negative-ack marker this slot is freely replaceable, because the turn it names is terminal and no path can revive it. `processingUserMsgId` is still cleared directly: it is execution-owned, dispatch is serialized per session, and we only reach here with no active turn, so clearing a verifiably terminal slot repairs a crashed-mid-turn leftover without racing anyone. Model: claude-fable-5
Review catch (P1). Both suppression slots retire an activation while leaving `latestUserMsgId !== lastHandledUserMsgId` on purpose — there is no CAS against the LWW map, so rewriting the producer pointer would drop a concurrent send. Only the watcher understood that. Three other consumers re-derived pending work from the raw comparison and so disagreed with it: - auto review's `isSessionBusy` waits forever on a session that finished - idle GC's `hasPendingUserWork` never reclaims that session - MCP's `hasPendingDispatchPointer` reports a queued turn that does not exist This predates `settledActivationUserMsgId`: the negative-ack marker has left the same divergence since recovery stopped clearing the pointers. Settling made it routine rather than exceptional, which is what surfaced it. Move `getPendingUserTurnActivationId` / `hasPendingUserTurnActivation` to `@lody/shared` beside `SessionMeta` and route all four call sites through it; `session-dispatch-logic` re-exports them as the watcher's decision surface. Unit tests on today's consumers cannot catch tomorrow's, so the regression guard is shape-level: `tests/dispatch-activation-predicate.test.ts` fails if any CLI source compares the two pointers outside the predicate. It already earned its keep — it caught a stale comment in the watcher describing the old rule, now corrected. Model: claude-fable-5
Cleanup pass over this branch; no behavior change.
The four settlement tests each rebuilt the same watcher, session doc, and
meta literal — ~150 near-identical lines. `createSettleHarness` follows the
`createAccessHarness` precedent already in the file and takes the two things
that actually differ (meta pointers, history), so each test is now its setup
plus its assertions. The mid-settle race test expresses the concurrent send
as a harness argument instead of reaching into the watcher's deps.
`getPendingUserTurnActivationId` had two import paths after moving to
`@lody/shared` — the re-export contradicted the guard test's own message.
Dropped it; the watcher imports from the package like every other consumer.
MCP's `hasPendingDispatchPointer` had become a pure alias, so it is gone too.
The guard test moves to `packages/shared/tests/`, beside the predicate it
protects, and adopts the shape of `mirror-construction-sites.test.ts`:
recursive `readdirSync` instead of a hand-rolled walk, and the same two
search roots, so `packages/components` is now covered as well. A
`.includes()` prefilter skips the regex for the ~280 files that never
mention the pointer.
`peekStashedRpcTurn` carried its own inline copy of the terminal-status set
that `isActivationAwaitingHistory` now expresses — and the two had already
drifted (one read `entry.status` raw, the other normalizes). One call site
instead of two.
Also: `Object.keys(patch).length === 0` survived only because
`Object.keys({x: undefined})` is `['x']`; the two conditions above it are the
real answer. The settle call no longer hides a doc write inside an `&&`. The
AGENTS rule "append only through `appendUserTurn`" had five standing
exceptions and the renderer cannot reach `SessionDocument` at all, so it now
states the actual invariant and names the exceptions.
Model: claude-fable-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
Maintainer-authored change requested directly in a working session; there is no
prior tracking issue. The external-contribution gate does not apply here
(
shouldEnforcePullRequestis false for aMEMBERauthor), so no issue wasopened solely to satisfy automation.
Problem / pressure
A local session surfaced
message_delivery_failed("消息送达失败 - 请在同步恢复后重发")five minutes after its last turn finished, but every user turn had already
executed successfully. The notice was a false alarm.
Queue promotion materializes a real user turn into history but never published
latestUserMsgId, while ordinary execution advanceslastHandledUserMsgIdtoevery turn that runs. Once a queue drained, the two pointers were left
permanently unequal:
latestUserMsgIdstill named the last direct send,while
lastHandledUserMsgIdnamed the last promoted turn.getPendingUserTurnActivationIdreads that plain inequality as a still-pendingactivation, so the watcher entered
waitForPendingUserTurnHistorySyncfor ahistory entry that was already present and terminal — the log line "metadata is
visible but history is missing it" is misleading, because the entry exists and is
handled. AfterHISTORY_SYNC_WAIT_TIMEOUT_MSit ranmarkMissingUserTurnRecovery, wrotelastMissingHistoryUserMsgId, and recordedthe bogus
message_delivery_failednotice against a turn that had already run.Any session whose last executed turn came from the message queue reproduces this.
It became common after #161, whose renderer routing queues on
unfinished_assistant_turn.Diagnosis was confirmed by decoding the workspace Flock meta snapshot: the entry
clock on
latestUserMsgIdwas still the original producer write from the firstturn, with no later overwrite. This is a missing write, not a CRDT revert or a
sync race, and the network flapping visible in the same logs was a red herring.
Summary
promoteNextQueuedMessagenow publisheslatestUserMsgIdfor the turn itmaterializes, making queue promotion a dispatch producer. That restores lockstep
between the two pointers: every turn that RUNS passes through
latestUserMsgIdbefore
lastHandledUserMsgIdlands on it, so a drained queue settles instead oflooking like a permanently pending activation.
lastMissingHistoryUserMsgIdis deliberately not cleared. The otherproducers that clear it supersede the acknowledged entry to
canceledfirst;promotion does not, so clearing it here would let that stale
pendingcopydispatch — the resurrection
apps/cli/src/session/AGENTS.mdforbids. Leaving themarker costs nothing, since it only suppresses its own turn id.
apps/cli/src/session/AGENTS.mdrecords the widened single-writer-roleinvariant: promotion is now a producer, and it is the one producer that must not
clear the marker.
Before / after
latestUserMsgId, so a drained queue leaves it naming an older turn thanlastHandledUserMsgIdforeverHISTORY_SYNC_WAIT_TIMEOUT_MSon an entry that is already present and terminal, then postsmessage_delivery_failedfor a turn that succeededTest plan
Two regression tests added in
apps/cli/tests/session-dispatch-watcher.test.ts:leaves no pending activation once a promoted queue turn is handled— assertsthe promoted turn becomes the pending activation, and that after execution
writes
lastHandledUserMsgIdno activation remains. Verified to FAIL withoutthe fix (
expected undefined to be 'queued-1').keeps a missing-history marker when promoting a queued turn— asserts thepointer is published, the marker survives, and
findNextDispatchableUserTurnstill selects the promoted turn rather than the stranded one.
Both assert end-state behavior through the pure predicates that drive the bug,
not mock call counts.
Commands run locally:
pnpm vitest run tests/ src/session/inapps/cli— 916 passed, 1 skipped(
kimi-acp-package-smoke, skipped by its own guard).pnpm --filter lody typecheck— clean.pnpm format— run; no diff left in the changed files.Not run: the full root
pnpm check(workspace-wide lint/build across everypackage) and any manual end-to-end desktop run against a live queue.
Context handoff
Instructions for reviewing agents
promoteNextQueuedMessageinapps/cli/src/session/session-dispatch-watcher.ts— specifically that the newupsertDocMetawrite is ordered after the history append, and that it does notclear
lastMissingHistoryUserMsgId.latestUserMsgId; a conditional "only publish when the pointer is settled"variant was considered and rejected, and that trade-off deserves independent
judgment.
another peer can be overwritten by this write; the fallback is that the peer's
history entry still dispatches through the
pendingstatus path, which isargued rather than covered by a test.
Authoring context
message_delivery_failed, then apply the pointer-publishing fix the user chosefrom two proposed options.
dispatch path; the alternative option (making the recovery predicate skip
terminal entries) was explicitly not taken, and no renderer or policy code is
touched.
latestUserMsgId's single-writer role toinclude queue promotion, and deliberately preserving the missing-history marker
so a negatively acknowledged turn cannot be revived.
field write; no migration, deletion, or rollback path is involved, and the
durable turn content is written to history before the pointer.
remote-producer race described above, because reproducing it needs two live
peers; the root
pnpm checkwas not run.confirmed against the real session's persisted Flock entry clocks; residual
uncertainty is limited to the narrow concurrent-producer window.
🤖 Generated with Claude Code