Repository navigation
fix(native-chat): carry the retained submission window on a replacing history page - #20586
brennanb2025 wants to merge 3 commits into
Conversation
… history page A structured send whose settlement arrived while no pane was subscribed could never be reconciled, leaving the message silently unsendable and a false "Working for N" spinner running indefinitely. Since #19863 the Claude lane returns from dispatch on the transport write and writes no dispatch row: the submission rests `pending` and a single at-most-once streaming frame carries the `accepted` record. A pane that is not subscribed when that frame publishes re-attaches, advances its cursor to the journal head, and never sees the row. Five sites advance a cursor past rows they did not deliver -- the reducer's tail-page replace, the subscribers' reset branch, `replay()`, a cold `open()`, and the read transport's resume -- and all five ship a hydration or tail page. Repair was impossible because a page carried only the submissions whose user item survived the page's item window, and the renderer overwrites a submission only on key collision. Measured on a real journal: submission at seq 90, `accepted` at seq 91, turn completed, and the item at index 34 of 331 -- far outside the 200-item tail window. Every later page reported no submissions at all, so the stale `pending` survived. The outbox head parked at `dispatching` and the head-of-line pump never dequeued; two user messages sat behind it for five hours. A replacing page now carries the renderer's whole retained submission window. `after` keeps the item window: it is the live streaming batch, which already carries every submission it touches, and widening it would rebuild the array on every frame. Those extra records are priced into the page byte budget, which charges a submission against its own item and so cannot see one riding with an off-window item. Separately, a tail refresh no longer nulls the renderer's ownership fence. `agentSession.history` never stamps one, so every refresh cleared it, and a null fence silently holds both the outbox pump and the unconfirmed probe with nothing shown in the UI. The same-cursor branch already preserved it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughHistory pages retain the newest 256 submissions and reserve byte budget for them. Forward pages include item-referenced submissions. Replacing pages include retained submissions and referenced submissions. Dispatch reasons are bounded when journal rows are written and when pages serialize persisted rows. The reducer updates equal-cursor tail pages when submission data changes and preserves an existing fence when a page omits one. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed technical context, scope, rationale, testing, and limitations. However, it omits several required template sections, including ELI5, What Changed, Why, Linked Issue, Visual Proof or N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the checklist.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 97a16973-8ba7-484a-bb0a-ad5566393bb9
📒 Files selected for processing (5)
src/main/native-chat/agent-session-wire/agent-session-history-page.test.tssrc/main/native-chat/agent-session-wire/agent-session-history-page.tssrc/shared/structured-agent-session-reducer.test.tssrc/shared/structured-agent-session-reducer.tssrc/shared/structured-agent-session-submission-retention.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
…issed Follow-up to the page widening in this branch: the widened page never reached the pane it was written for. The `tail-page` early return discards a page whose cursor is not ahead of the renderer's. A wedged pane is head-parked by construction -- its cursor tracks the journal head through streaming batches, and a tail refresh carries that same head -- so every refresh was a no-op and the widened submissions were dropped on arrival. Window focus is the gesture users actually perform, and it routes straight into that return. The branch now reconciles submissions there. Merging touches no items, so its reason for existing -- not discarding paged-in older items mid-scroll -- is unaffected. It merges against `state.items`, not the page's, because the page window can be narrower and evicting a retained submission would re-open the same hole from the other side. It is gated on cursor equality only: dispatch states are not monotone, a proven retry returns a submission to `pending`, and merging a strictly-behind page could clobber a fresh `accepted` and un-settle a message the outbox had already released. Equality is structural, not referential. A widened page carries every retained key, so the merge overwrites each one with a fresh wire object and a reference check would never match -- minting a new state object on every focus and re-rendering the transcript. A dispatch `reason` is now bounded. It was the only unbounded field written by Orca's own code, and a replacing page carries the whole retained window, so 256 records at the journal's 16 KiB inline head reach the 4 MiB outbound channel cap exactly -- the read fails to serialize rather than merely truncating. It is bounded twice, at different limits, for different reasons: at the write site against journal growth, and on the page at a limit derived from the page budget and the window size, which is what protects journals already holding unbounded reasons on disk. Six tests, each observed failing for its own reason, including one pinning that the page bounds the field rather than dropping records to fit.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- A replacing page now carries the retained submission window —
pageSubmissionsinagent-session-history-page.tssends the newestMAX_RETAINED_SUBMISSIONS(256) snapshot submissions plus the ones whose item is on the page fortail/before, whileafterkeeps the item-scoped set. This restores the carrier a pane that attached after the soleacceptedstreaming frame published. - Shared retention definition —
MAX_RETAINED_SUBMISSIONSandretainedSubmissionWindowmove intostructured-agent-session-submission-retention.ts, so the host page width and the renderer'smergeSubmissionsretention can no longer drift apart. - Byte budget covers the widened payload —
itemBudgetBytesreserves the retained submission bytes beforeboundHistoryItemsByItemsbounds the item window, keeping the page insideHISTORY_PAGE_CONTENT_BUDGET_BYTES. - Tail refresh keeps the ownership fence — the
tail-pagereplace branch now usesaction.page.fence ?? state.fence, matching the same-cursor branch, so a fence-less refresh no longer holds the outbox pump and unconfirmed probe. - Two regression tests — one asserts the page carries a settled submission whose item aged out (with an explicit precondition that the item is absent from
page.items); the other asserts the fence survives a tail refresh with no fence.
Verification this run: pnpm test on agent-session-history-page.test.ts and structured-agent-session-reducer.test.ts passes (40 tests); snapshot submissions are submittedAt-ascending (journal-reducer.ts:339) so the page window aligns with mergeSubmissions retention; submission records are metadata-only, so the widened window stays small and the byte accounting is conservative.
ℹ️ retainedSubmissionWindow selects by submittedAt alone
The page slices the newest 256 of the host's submittedAt-sorted list while mergeSubmissions slices the newest 256 of the renderer's accumulated union. Same-millisecond ties could in principle order differently across the two and place one tied submission on opposite sides of the 256 boundary, so a page could omit a fresh copy the reducer still holds. Reaching it needs more than 256 submissions sharing a millisecond, which user sends cannot produce, so this is an observation rather than a request — noting it because it is the exact invariant the fix leans on.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Run failed. View the logs →
|
Three pieces of this branch could be removed with every test still green. `submissionsEqual` was a hand-maintained field list. A tenth field on `AgentJournalSubmission` would make a page whose only correction is that field compare equal, so the reducer would keep `state` and the settlement would never land -- this branch's own bug, re-created by an edit to an unrelated file. The list is now exhaustive at compile time: omitting a field fails the build here and names the field it missed. A runtime test flips each field independently, which the type guard cannot do. `itemBudgetBytes` was unpinned -- removing the reservation failed no test in the whole wire directory. It protects the page cap, not the channel cap, which is why the existing transport gate cannot see its absence: a page overruns `AGENT_SESSION_HISTORY_MAX_PAGE_BYTES` by ~204 KB and the 4 MiB channel never throws. Measured at both call sites and asserted against the page cap, with an item-count assertion so the byte check cannot pass on a fixture that never filled the budget. The page bound must keep the first 23 bytes of a reason, because `dispatchRejectionWasTransportWriteFailure` prefix-matches it and a clipped transport failure that stops classifying renders raw internal text to the user. Pinned at both bounds this branch introduces. The concrete hazard is a tail-preserving bound; the codebase's other bounding idiom keeps a 40-unit head, so it does not break classification -- the comment says so rather than overstating the threat. Also corrects the page-limit comment: the divisor reads as an eighth of the budget but bounds raw bytes while the page is priced in JSON, so escaping takes a full window to a measured 79%. It still fits because the reserve prices the serialized record.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Reconciles a missed settlement on the
tail-pageearly return — the same-cursor branch now computesmergeSubmissions(state.submissions, page.submissions, state.items)and returns new state whensubmissionListsEqualreports a change, so a head-parked pane finally picks up anacceptedit was never subscribed for. Kept offstate.itemsbecause a narrower page window must not evict a retained submission, and gated on cursor equality because dispatch states are not monotone. - Structural submission equality extracted to a shared module —
submissionsEqual/submissionListsEquallive inagent-session-submission-equality.ts, derived from aCOMPARED_FIELDSlist whoseeveryComparedFieldguard turns a newly-addedAgentJournalSubmissionfield into a compile error rather than a silent missed refresh. This is what keeps the widened page from minting new transcript state on every focus. - A dispatch
reasonis bounded at both ends — the write site clips at 16 KiB viaboundedDispatchReason, andboundedPageSubmissionsclips again atDISPATCH_REASON_PAGE_LIMITS(budget/8/256) so journals already holding unbounded reasons on disk cannot push a replacing page past the outbound cap. Clipping is head-preserving, and the retained record is never dropped;submissionBytesByItemIdprices the clipped form so the budget reservation stays bounded. - Guard and regression tests — head-preservation of the clipped transport prefix at both bounds (independent levers), retention boundary,
afterstaying item-scoped, the page budget reservation on tail and hydration pages, and the equality field guard.
The two new production commits are well-tested and internally consistent. I verified the byte accounting covers every carried record (retained → reserved, on-page → charged per item), that the head-preserving clip keeps dispatchRejectionWasTransportWriteFailure classifying a clipped transport failure, and that equal-cursor freshness makes the reconcile safe. All focused suites pass locally (equality 5, head-preservation 3, retention 2, budget 2, history-page 26, reducer 19).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by #20581, which landed the same fix with a better mechanism. Both PRs diagnose the same defect: a bounded tail read moved the client cursor to the journal head while carrying only a page of items, the send's user item fell off that page, its submission was not carried across the merge, the retained This PR widened the page so the record could still arrive, then reconciled it in the With #20581 merged, the substance here no longer applies:
The one independent piece is split out as #20654: Findings worth keeping from the review here, recorded so they are not lost with the branch:
|
The bug
A structured native-chat pane goes silently unsendable: typing a message queues it forever, and a false "Working for N" spinner runs indefinitely. Four live panes were in this state, all Claude, none of the 21 live Codex panes. One had two user messages stranded behind it for five hours, against a turn that had completed.
Mechanism
Since #19863 the Claude lane returns from dispatch on the transport write and writes no dispatch row. The submission rests
pending, and a single at-most-once streaming frame carries theacceptedrecord. That change was right -- it removed a false "unconfirmed" banner firing on 25% of real sends -- but it left one frame as the sole carrier, and Codex was deliberately left on the old inline path. Hence the provider asymmetry.A pane not subscribed when that frame publishes never sees the row, and that needs no fault anywhere:
publish()over an empty subscriber map is zero deliveries with no error and no queue,close()deletes the subscriber record outright, and there is no per-client delivery ledger. On re-attach the client supplies a cursor and the host reads strictly after it.Five sites advance a cursor past rows they did not deliver -- the reducer's tail-page replace, the subscribers' reset branch,
replay(), a coldopen(), and the read transport's resume. All five ship a hydration or tail page.Repair was then impossible, because a page carried only the submissions whose user item survived the page's item window, and
mergeSubmissionsoverwrites only on key collision. Measured on a real journal: submission at seq 90,acceptedat seq 91, turncompleted, and the submission's item at index 34 of 331 live items -- far outside the 200-item tail window. Every later page reported no submissions at all, so the stalependingsurvived. The outbox head parked atdispatching, which the head-of-line pump never dequeues, andhasUnansweredStructuredAgentSessionDispatchkept the spinner alive off the same row.The page filter is not new; it predates #19863 and was harmless while a reliable second carrier existed.
The fix
A replacing page carries the renderer's whole retained submission window, not just the submissions its items reference.
afterkeeps the item window: it is the live streaming batch, which already carries every submission it touches, and widening it would rebuild the array on every frame and churn every consumer for the duration of a turn -- including mobile, which does AsyncStorage work whenever settled sends change.The
tail-pageearly return now reconciles submissions. Widening the page alone fixed nothing for a live pane: that branch discards a page whose cursor is not ahead of the renderer's, and a wedged pane is head-parked by construction, so every refresh was a no-op. Window focus is the gesture users actually perform and it routes straight into that return. Merging touches no items, so the branch's reason for existing -- not discarding paged-in older items mid-scroll -- is unaffected. It merges againststate.itemsrather than the page's, because the page window can be narrower and evicting a retained submission re-opens the same hole from the other side. It is gated on cursor equality only: dispatch states are not monotone, a proven retry returns a submission topending, and merging a strictly-behind page could clobber a freshacceptedand un-settle a message the outbox had already released.Equality there is structural, not referential. A widened page carries every retained key, so the merge overwrites each with a fresh wire object and a reference check would never match -- minting a new state object on every focus.
A dispatch
reasonis now bounded, at two different limits for two different reasons. It was the only unbounded field written by Orca's own code, and a replacing page carries the whole retained window, so 256 records at the journal's 16 KiB inline head reach the 4 MiB outbound cap exactly: the read throws rather than merely truncating. The write site bounds against journal growth; the page bounds at a limit derived from the page budget and the window size, which is what protects journals already holding unbounded reasons on disk. The window itself stays count-bounded, so the host and the renderer's own trim remain one definition -- bounding the field never drops a record, and a dropped record is exactly a settlement that never reaches a re-attaching pane.MAX_RETAINED_SUBMISSIONSmoves to its own module so both sides share one definition.clientMessageIdis deliberately untouched: it is the join key the renderer matches outbox entries against, and truncating it would silently rebuild this bug.Testing
Every test was observed failing before its own fix, with single-lever arms confirming each fails for its own reason:
expected [] to match object [ { clientMessageId: 'msg-1', dispatchState: 'accepted' } ])dispatchState: "pending"where"accepted"was expected)expected null to be 7)aftergate holding a forward page to its item windowRemoteRuntimeClientError: Remote runtime JSON payload exceeds 4194304 bytes)expected Array(197) to have a length of 256)The two
reasonbounds are pinned independently: the write-site lever observes the reduced snapshot and never a page, and the page-side lever starts from a row inserted directly into the database, bypassing the write site. Without that asymmetry the page-side bound -- the half protecting journals already in the field -- would have passed on the write-site bound alone and gone unpinned.pnpm tccleansrc/main/native-chat/-- 141 files, 1130 passedsrc/shared/structured-agent-session-- 15 files, 146 passedsrc/main/native-chat/agent-session-journal/-- 19 files, 210 passedcheck:code-quality:changed-- 0 new findings, all five gatesmax-lineslinted against the merge result, not just this head: cleanGuards
Three pieces of this change could be removed with every test still green. Each is now pinned.
submissionsEqualwas a hand-maintained field list. A tenth field onAgentJournalSubmissionwould make a page whose only correction is that field compare equal, so the reducer keepsstateand the settlement never lands -- this branch's own bug, re-created by an edit to an unrelated file. The list is exhaustive at compile time; omitting a field fails the build and names the field. A runtime test flips each field independently, which the type guard cannot do. Verified firing with an optional tenth field across all three tsconfig projects.itemBudgetByteswas unpinned -- removing the reservation failed no test in the entire wire directory. It protects the page cap, not the channel cap, which is exactly why the existing transport gate cannot see its absence: measured at 1,993,060 bytes with the reservation and 2,301,089 without, against a 2,097,152 page cap, with both arms under the 4 MiB channel. The overflow is silent. Pinned at both call sites againstAGENT_SESSION_HISTORY_MAX_PAGE_BYTES, with an item-count assertion ahead of the byte check so it cannot pass on a fixture that never filled the budget.The page bound must keep the first 23 bytes of a reason, because
dispatchRejectionWasTransportWriteFailureprefix-matches it and a clipped transport failure that stops classifying renders raw internal text to the user. Pinned at both bounds this branch introduces. The concrete hazard is a tail-preserving bound. Note the codebase's other bounding idiom,boundJournalKeyComponent, does not break classification -- it keeps a 40-unit head and the prefix is 23 bytes -- but it does drop the truncation marker, so a clipped reason would read as the provider's complete explanation. The comment records the narrower true constraint rather than the broader plausible one.Not addressed
providerItemIdandpayloadFingerprintare unbounded on the wire the same wayreasonwas. They are not join keys, so bounding them is possible, but first-party clients never produce large ones and admission-time refusal is the honest remedy -- a separate change.Some wedged entries are missed frames whose item is still inside the window; those already heal on the next transport run. The
emitpath atstructured-agent-session-subscribers.tsstill swallows a throw and silently drops a subscriber -- after this change such a loss is recoverable on the next page, but it remains unreported. A submission strandedpendingwithin a live fence now readsworkingon the pane as well as the session list, which already showed it; giving that case a settlement path is a follow-up.clientMessageIdis deliberately excluded, not overlooked. It is unbounded on the wire the same wayproviderItemIdandpayloadFingerprintare, but it must not be bounded: it is the join keyreconcileStructuredAgentSessionOutboxmatches outbox entries against (structured-agent-session-outbox.ts:99-101), so truncating it on the page rebuilds exactly the wedge this PR fixes, and truncating it at the write site hands the client back an id it cannot match. Refusal at admission is the only honest remedy. Do not "finish the set" by bounding it alongside the other two.Budget-test margin, for whoever trips it.
leaves room for the retained window when items alone would fill the pagepasses with 103,947 bytes of headroom against the page cap. A future change that grows the page envelope past that will fail it, and the failure will read as a budget regression when the real cause is envelope growth.