Repository navigation
fix(gateway): preserve consumed inputs when send becomes steer - #602
Conversation
jacoblee-io
left a comment
There was a problem hiding this comment.
Approving. The change is correct and it closes a hole that has been open since fd004c319.
What it actually fixes. fd004c319's commit message argued the turn-end sweep was safe because "with rows queued only on acceptance, what remains at the end belongs to the turn that just ended" — but that same commit moved the push into the two early-returning steer branches (server.ts:943 busy-degrade, :1054 the 409 fallback). So its own premise never held: the busy-degrade handler pushed a row and then, one await updateMessage later, wiped it in its finally — along with every other pending row on that session, including a row the owning turn or a concurrent chat.steer was still waiting to have sequenced. A concurrent send that degraded to steer therefore almost never got sequenced, and an unsequenced role='user' row is dropped by loadFullHistory. This PR restores the invariant that commit intended.
Verified locally, not just read: reverting the guard to the unconditional clear() makes both new cases fail (sequenceMessageMock never called with q2; claim("S", …) returns undefined instead of q409), and they pass with it. 42/42 in server-chat-abort.test.ts + pending-user-rows.test.ts.
The 409 retention deserves a sentence in the code, because what makes it correct lives on the other side of the boundary. Holding the Runtime turn lock and still getting a 409 means the box is running a turn that has no consumer in this Runtime — the lock is released in the same finally as the sweep, i.e. after consumeAgentSse returns, so there is no overlap — and onUserMessageStarted (server.ts:1140) is the only claim call site in the tree. Read on its own, the retained row looks unclaimable. It is claimable only because pi keeps its own steering queue and replays it on the next prompt, which is exactly what the abort path's clearQueue() exists to prevent ("so it does NOT replay on the next prompt"). Please state that dependency where the retention happens; a future reader who checks only the Runtime side will conclude the push is dead and remove it.
Residual asymmetry worth knowing about (not a blocker, no change requested): the two queues can be desynchronised in one direction. If the box-side steer is dropped while the Runtime-side row is retained — a Stop's clearQueue(), a box recycle, a session eviction — nothing will ever echo that text. claim refuses a head whose guard fails without shifting it, so the next turn's own user row goes unsequenced and is filtered out of cold restore; it self-heals when that turn's finally sweeps. Bounded to one message, and strictly better than what main does today, but a generation stamp or a TTL on the queue would close it.
Pre-existing, adjacent: the queue assumes push order equals box consumption order, and the busy-degrade path can invert it. If the owner's prompt() ack takes longer than the lock's 5s DEFAULT_WAIT_MS, the concurrent send steers and pushes first while the owner pushes q1 after, giving [q2, q1] against a box that consumes q1 first. The text guard prevents a mis-claim, so the cost is both rows unsequenced rather than wrongly ordered. Out of scope here.
Test notes:
- The first case spends ~5.4s of its 5.7s inside the lock's default wait. Fine, but it is now one of the slower cases in the file.
await new Promise(r => setTimeout(r, 10))as the barrier beforeexpect(size("S")).toBe(1)is a false-negative shape: if thefinallyhad not yet run, the pre-fix code would show the same size and the case would pass. It does fail today, so the timing holds — but a deterministic signal (awaiting the mockedupdateMessagesettle) would keep it that way.- The
loadFullHistoryhalf derivessequencedfrom the mock and feeds a hand-built row, so it re-checks the knownseqSequencedfilter rather than adding evidence for this fix. No objection to keeping it.
6c34e23 to
a4e1641
Compare
jacoblee-io
left a comment
There was a problem hiding this comment.
Re-approving a4e1641d after the force-push dismissed the previous approval. All three follow-ups landed, and the Runtime diff against the reviewed head is comment-only.
- The 409 push now states the cross-boundary dependency at the site itself, including that the AgentBox abort path clears pi's queue — which is the one direction in which the two queues desynchronise.
- Both fixed sleeps are gone. The replacement reasoning holds: nothing awaits between the
bindMessageTraceIdinvocation and thefinally, so a poll that observes the call is necessarily observing a completed cleanup. Re-verified that the sleep removal did not weaken the cases — reverting the guard still fails both. - 42/42 in the two affected files; CI green on all four checks.
The residual asymmetry noted in the previous review (box-side queue cleared while the Runtime-side row is retained, costing the next turn's own user row its sequence until that turn's sweep) is unchanged and remains out of scope.
fix(gateway): preserve consumed inputs when send becomes steer
Summary
When a concurrent
chat.sendjoins an active turn as a steer, its handler can finish before the owning stream receives the input's consumption acknowledgement. Unconditionally clearing the session's pending rows at that point leaves an input the model consumed unsequenced, so it is omitted during cold history reconstruction.Restrict cleanup to the handler that owns the turn and has not steered into another active turn. The Runtime-busy path leaves pending inputs for the active stream. The AgentBox 409 fallback retains records for unconsumed steers that pi may replay on a subsequent prompt. The owner still clears unconsumed inputs when it finishes. No RPC or schema changes are required.
Test Plan
npx tsc --noEmitandnpm run buildpassed.Review follow-up: clarified the 409 replay dependency and removed fixed sleeps. Both regression cases still fail with the unconditional sweep; 42 related tests and TypeScript passed. Runtime emitted code is unchanged. The full-suite, build, and deployment results above preceded this follow-up.
General Checklist