perf(chat): keep the pane hydrate and warm bounded while a slot runs - #10026
perf(chat): keep the pane hydrate and warm bounded while a slot runs#10026CrysisDeu wants to merge 1 commit into
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound bounded-fetch design, but its correctness now rests on client code re-deriving the server's row-counting semantics with nothing pinning the two sides together. Watch
[DESIGN-REVIEWED] 6d57406 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS The load-earlier bar read cleanly blind, but a failed widen stacks two contradictory error notices and its "Retry" secretly widens instead of retrying. Watch
Evidence gaps
[UX-REVIEWED] 6d57406 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Verify the at-cap full-history press on a real >500-row thread — the pod evidence covers only the 200-row widen path, vitest covers the rest. Not justified as shipped
What this change shipsInventory (9 items) — 8 justifiedIntent: stop a long-lived running thread (Members DM, 10 MB/~3000 rows) from being re-read whole on every open and every completed turn — a FIX (#10005, measured, with tests that fail on base).
Deleted pins ("unbounded while streaming is deliberate", the [FIRST-PRINCIPLES-REVIEWED] 6d57406 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/components/ChatPane.tsx:489 -- passive False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings that block. FINDING — website/src/components/ChatPane.tsx:489 — [OPUS-REVIEWED] 6d57406 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
6195ec4 to
2ee1875
Compare
|
Round 2 (
|
2ee1875 to
9175dd6
Compare
|
Round 3 (
|
9175dd6 to
e61a064
Compare
|
Round 4 (
|
|
@bolichen97 — addressed in
|
bad3ad7 to
3bae0f1
Compare
|
Head is now |
3bae0f1 to
ebef572
Compare
|
Round 9 ( |
ebef572 to
61b13a7
Compare
|
Round 10 ( |
61b13a7 to
065e987
Compare
|
Round 11 ( |
bolichen97
left a comment
There was a problem hiding this comment.
Maintainer re-review (agent-assisted), pinned to 065e987. Requesting changes: the Members DM has no full-session fallback, and Load earlier only expands to 500 then disappears. The body now explicitly acknowledges that older rows in a longer thread are inaccessible until P5-e lands. This removes access users had before; a future pager is not a present recovery path.
Owner-delegated decision: bounded initial hydration and incremental reads are welcome, but retain a working route to all older history now (cursor paging or an explicit safe full-history fallback) before landing. Add a >500-row regression proving the earliest rows remain reachable. The warm-cache stitching work can remain; the blocker is the DM access cap, not the bounded initial page. Source/test review and current-head CI evidence only; local suites not rerun in this wave.
…10005) A Crew Members DM thread got slower the longer it was used: one real thread is 10 MB / ~3000 persisted rows, and opening it -- plus every turn the member finished -- pulled and rendered the whole transcript. Two paths went unbounded for a running slot, and neither reason still holds. ChatPane's hydrate lifted PANE_HYDRATE_LIMIT to undefined and latched it as soon as the slot was running (#3240), on the theory that a limit would slice the in-flight response's raw chunk rows. 1afc7dc corrected that: the handler collapses chunk runs before it computes total and slices, so the hazard never existed -- but the lift stayed. A member thread is almost always running, so the pane always took the unbounded read. The bound now holds regardless; the turn's own rows reach the pane over the WS routing, not this fetch. The first hydrate also drops live frames the page already carries (tailNotInPage): a duplicated mid cannot anchor, so it would have sent the first warm unbounded. warmSlotCache fires on every background chat_done and went unbounded both while streaming and whenever the pane already held rows (#6839). #7916 retired exactly those two exemptions for switchSlot -- verify after the response instead of pre-purchasing a wider read -- and left the warm as the one branch it did not revisit. The warm now sizes its window on the durable rows the pane holds (slotSwitchFetchLimit, floor PANE_HYDRATE_LIMIT, the handler's 500 as cap) and accepts the page when it can be STITCHED onto the cache: it reaches the start of history, its own oldest row is in the cache (the head-keeping cut), or it contains the cache's oldest identified row and no mid-less durable row sits above that (pageStitchesOntoView + unidentifiedDurableHead; the stitch predicate is extracted from refreshSlot's post-fetch check so the two share one). That is the fulfilled reducer's actual need -- it keeps any head above the page and rescues any tail below it -- and deliberately not slotCoverageShortfall's multiset, which would read a row appended during the await, or a cache past 500 rows, as a hole and send every warm of a busy pane to the unbounded read. A ~3000-row pane therefore reaches a steady state of one 500-row read per chat_done. Counts stay in one unit. The unbounded retry's own total is raw (a `done` per finished turn), so the retry carries the bounded page's collapsed count and the reducer's shrink/rewrite comparisons and retention all read that one; a raw count is never a baseline: only a count in SETTLED units is (retainServerTotal via settledTotalOf). A bounded read's count is the collapsed corpus less the pending permission cards it keeps (they are in the page), a floor the settled count can only grow from -- raw, the next settled warm read that fall as a truncation and dropped its rescued tail. An unbounded read's `total` counts a `done` per finished turn and is never used; a COMPLETE unbounded read derives its settled count from its prepared rows instead, so an empty-view refresh still leaves the baseline a later remote regeneration is recognised against. One policy on switchSlot, refreshSlot, the hydrate and the warm. The warm re-validates after EACH await: a /clear (slotClearSeq, bumped by clearSlotCache / clearMessages), an emptied cache, or the slot's absence from the loaded authoritative slot list (a delete evicts every per-slot trace, so a cold cache alone cannot show it) discards the page, which describes a transcript that no longer exists; a reducer guarding only on activeSlot would write it back. A bounded page must not be the end of the road. A host that can open the full session keeps its "earlier messages" row; the Crew Members DM (no full session to open) gets a load-earlier bar that WIDENS the bounded window a page at a time (hydrateSlotMessages now accepts a wider bounded page over the held one), up to the handler's cap -- the cursor-based pager of P5-e replaces it. Real-pod capture: temp-screenshots/members-dm-load-earlier/ (50 display rows on open, 100 after one press; scripts/capture-members-dm-load-earlier.mjs). Also corrects three stale "Virtuoso" comments (ChatPage x2, ChatMessageList header): the virtualizer is hooks/virtualizer/useVirtualChat, and ChatPage does not use ChatMessageList. Tests (CI-run): ChatPane stays bounded while running and issues no unbounded refetch when a turn starts mid-hydrate; warm stays bounded while streaming, sizes on durable rows, keeps a 3000-row pane on one 500-row read, does not retry over server growth or a live row appended in flight, retries only on a page that anchors nowhere or only spans a mid-less head, skips the retry at the start of history, carries the bounded count through a retry, and never retains a running count.
|
Round 11 (
Static gates: |
|
Round 11 CI note: the one red on Update: the rerun failed on the same single test (1 failed / 22287 passed), and main's latest completed run of this workflow (run 34673414316 on |
Part 1 of 2 for #10005 (chat-core P5-e). Stops the bleeding: the pane's history reads stay bounded. Rebased onto main after PR-2 (#10146, the virtualized transcript) merged, so the Members DM's earlier-history bar now rides
VirtualTranscript'searlierslot.Summary
Two hydrate paths pulled a running slot's whole transcript, and neither reason still holds:
ChatPanehydrate (components/ChatPane.tsx:470-476on main) liftedPANE_HYDRATE_LIMITtoundefinedand latched it the moment the slot was running. The justification from perf(chat): bound the per-pane history hydrate in the session grid #3240 ("a limit slices raw chunk rows mid-stream") was falsified by 1afc7dc — the handler collapses chunk runs before it computestotaland slices — but the lift stayed. The bound now holds whether or not the slot runs; a turn's rows reach the pane over the WS routing, not this fetch.warmSlotCache(store/chatSlice.ts:2846on main) fires on every backgroundchat_doneand went unbounded while streaming or whenever the pane already held rows (fix(chat): stop a bounded switch refetch shrinking a painted transcript #6839). perf(chat): reachable archived history and stable phone scrolling #7916 retired exactly those exemptions forswitchSlotand left the warm as the one branch it did not revisit. The warm now sizes its window on the durable rows the pane holds (slotSwitchFetchLimit, floorPANE_HYDRATE_LIMIT, cap 500) and accepts the page when it can be stitched onto the cache — the fulfilled reducer's actual need (it keeps any head above the page and rescues any tail below it), notslotCoverageShortfall's multiset, which would read a row appended during the await or a cache past 500 rows as a hole and send every warm of a busy pane unbounded.Also fixes three stale "Virtuoso" comments (
ChatPage.tsx:5108,:6105,ChatMessageList.tsxheader): the virtualizer is our ownhooks/virtualizer/useVirtualChat, and ChatPage does not useChatMessageList.Why now
The Crew Members page's member DM gets slower the longer it is used. One real thread is 10 MB / ~3000 persisted rows; a member thread is almost always running (monitor loops, patrols), so opening it took the unbounded read and rendered ~3000 rows, and every completed turn re-pulled the full 10 MB through the warm. Fixing only the hydrate would have lasted until the member's first turn finished.
What changed
website/src/components/ChatPane.tsxhydrateLimitstarts atPANE_HYDRATE_LIMIT; therunninglatch and itsuseRefpair are gone, so no second fetch is issued when a turn starts mid-hydrate.hydrateSlotMessagesaccepts a wider bounded page over the held one) up to the handler's 500 cap — and at the cap the bar stays: the next press is the explicit full-history read (no limit), which the reducer accepts as the final, widest page. So a thread longer than 500 rows reaches row 0 in ten presses, and the only unbounded read is the one the reader asked for (the maintainer's floor for landing the bound). Cursor paging withnext_beforecan replace the last step later without changing what the reader sees.website/src/store/chatSlice.tswarmSlotCache: bounded window sized on durable rows →pageStitchesOntoViewdecides'complete' | 'overlaps' | 'spans' | null;'spans'alone is accepted only when no mid-less durable row sits above the cache's oldest identified row (unidentifiedDurableHead, legacy history). Anything else takes the unbounded retry, which carries the bounded page's collapsedtotal(comparableTotal) — the retry's own count is raw. Net effect for a ~3000-row pane: one 500-row read perchat_done, head kept by the reducer.pageStitchesOntoView/identifiedServerRowsare extracted fromrefreshSlot's inline post-fetch check so the two share one predicate;refreshSlotbehaviour is unchanged (truthy test over the same three conditions, samerequireTsanchors).warmSlotCache.fulfilled:totalis the bounded count (own or carried) for the shrink/rewrite comparisons and retention alike; a raw count is never a baseline.retainServerTotalretains only a count in settled units (settledTotalOf): an unbounded read's rawtotal(adonerow per finished turn even when settled) is never used — a complete unbounded read derives its settled count from its prepared durable rows instead, so an empty-view refresh still leaves a baseline; a bounded running count is normalised bysettledBoundedTotal: the bounded corpus keeps pendingpermissioncards in its count and drops answered ones, so a mid-turn total exceeds the settled one by exactly the cards in the page — subtract them and the result is a floor the settled count can only grow from. Retained raw, the next settled warm read the cards' resolution as a truncation and dropped its rescued tail; refused outright, a slot that streams most of its life never got a baseline and a remote rewind of it was never recognised.switchSlot,refreshSlot, the hydrate and the warm all go through the one policy.warmSlotCachere-validates after each await against a per-key generation (slotClearSeq: moved byclearSlotCache,clearMessagesandevictSlotState, and deliberately not evicted with the slot so a delete-and-recreate under the same key reads as a move), plus an emptied-cache check and the slot's presence in the loaded authoritative list; any of them makes the thunk discard the page instead of writing a transcript that no longer exists back.hydrateSlotMessagesfirst path drops live frames the page already carries (tailNotInPage): a duplicatedmidcannot anchor and would have sent the first warm unbounded. Docblock updated (the unbounded-upgrade branch is kept for callers/tests but no host takes it).fetchSlotDetailandswitchSlotthat still described the warm as unbounded-while-streaming.Screenshots (real pod,
kirocrew pod up wt-p5e-pr1 --seed minimal, a 200-rowmember-defaultthread)Crew Members DM opens on the newest bounded page (50 display rows of 200) with the load-earlier bar; one press widens the window to 100 rows. Captured by
website/scripts/capture-members-dm-load-earlier.mjsagainst the running pod (asserts the row counts).What was tested
Only the static gates ran locally:
npx tsc -b(exit 0) andnpx eslinton the seven changed files (exit 0). No local test run — CI runs the suites.Tests added/changed (CI-run):
ChatPane.hydrateBound.test.tsx: running slot hydrates withPANE_HYDRATE_LIMIT; a turn starting mid-hydrate issues no unbounded refetch (absence asserted after settle, notwaitFor); the running page reports bounded and retains the running bounded count in settled units; the load-earlier bar appears only when the host passes noonOpenFull, widens by one page per press; >500-row regression: a 600-row thread widens 50 → 500 over nine presses, the bar stays at the cap, the tenth press issues the full read (limitundefined) androw-0is held with the bounded marker cleared — and no unbounded read happened before the reader asked; a wider bounded page supersedes the held one and keeps the live tail.chatSlice.warmSlotCacheBound.test.ts: streaming warm is bounded; window ≥ durable rows held, sized on durable rows only; a 3000-row pane stays intact on one 500-row read; no retry over server growth or a live row appended in flight; retry only on a page that anchors nowhere or only spans a mid-less head; a tailerrorbubble does not pin the slot unbounded; no retry at start of history; the retry carries the bounded count; a running bounded count is retained in settled units (pending cards subtracted), on the page path and through the retry.useWebSocketReconnect.splitPanes.test.ts: streaming member of a split warms bounded.store/chatSlice.boundedRefetchShrink.test.ts: the background-warm pin now asserts the property (first read sized ≥ what the pane holds; a window disjoint by construction takes the unbounded retry; nothing shrinks) rather than the old[undefined]mechanism.settledTotalOfreturnsundefined; it would be short by the in-flight row and hide a later one-row rewind), so the bounded page's normalised count carries through the retry; a cold slot deleted and recreated under the same key is detected through the recreated record'screated(the eviction cannot move the generation of a slot with no residue), alongside the generation for slots with residue. Tests for both.slotClearSeqbecomes a per-key generation that survives eviction and is bumped byevictSlotState, so a slot deleted and recreated under the same key during a cold warm no longer receives the deleted transcript's page; test with realdashboardReducer(evict, recreate, resolve).liveTailNotInPagelike the first hydrate, so a repeatedsendIdcannot drop a distinct confirmed live row; reducer test added.dashboard.slotslist (test with a real dashboard reducer); a complete unbounded read derives its settled count from prepared durable rows (settledTotalOf) so an empty-view refresh leaves a baseline (tests re-pinned: 120 durable rows against a raw 900; hydrate → 2 against a raw 7)./clearor delete landing during a warm's fetch or retry → page discarded (slotClearSeq+ emptied-cache check), two tests. AlsochatSlice.warmSlotCacheBound.test.tsre-pinned where an unbounded hydrate used to move the count (raw units are refused now).refreshSlotno longer seeds the baseline (rawdone-inclusive units vs. the bounded warm's collapsed units);retainServerTotalkeeps only bounded normalised counts or a carriedcomparableTotal.serverTotalBaseline.test.tspins the unbounded refusal; the refresh positive-control test now paints the view so its refresh is the bounded read.settledBoundedTotal(pending permission rows in the page subtracted) on every path, so neither the inflated-baseline drop (round 3) nor the absent-baseline rewind miss (round 5) applies.serverTotalBaseline.test.tsre-pinned (running bounded switch → 900; two pending cards → 900 from a raw 902; unbounded retry still refused); warm/hydrate tests pin the same normalisation.liveTailNotInPagematches a confirmed row throughidAnchorsOneRow(the file's one identity invariant: themidnames exactly one row on each side and the two rows do not contradict onts) rather than a baremidset lookup, sincemeta.midis minted only when absent and a caller can post one twice. Two reducer tests pin the declines (ts contradiction; mid carried twice by the page).retainServerTotalnow refuses a running count on every path;chatSlice.serverTotalBaseline.test.tsre-pinned to the unified policy (its earlier premise — that the switch's coverage check reads the baseline — has been stale since perf(chat): reachable archived history and stable phone scrolling #7916).midonly (liveTailNotInPage);sendIdis used solely for rows the server has not yet named, so a repeatedsendIdacross two valid sends cannot drop the newer row. Three reducer tests pin it.Four rounds of blind adversarial review (Logic / Concurrency lenses) on the source diff; every Required/Critical finding was fixed and re-reviewed. Three Low findings are left deliberately and listed below.
UX CONCERNS
Load earlier messagesbar at the top (screenshots above). Each press widens the window by a page; a very long thread reaches the handler's 500-row cap after nine presses, and the tenth press loads the full history — so every older row a user could reach before this PR is still reachable, at the cost of one explicit unbounded read. Before this PR the pane loaded all ~3000 rows on every open and froze.switchSlot.pending) keeps it: the bounded warm anchors and the reducer keeps the head. Render cost for such a pane is the virtualizer's (feat(chat-core): P5-e — one virtualized transcript for every chat surface #10146); the 10 MB re-fetch per turn is gone.Known Low residuals (documented, not fixed here): a client-minted
error/noticerow that is the pane's first durable row keeps that pane on the unbounded warm (unlikely layout; safe direction); under'spans'a page shorter than the cache is discarded by the reducer'skeptPriorrather than merged (no data loss; reconciled by a later warm).Reviewer questions (bolichen97)
settledTotalOf's client-side count equal the bounded handler's collapsedtotalfor persistederror/mcp_oauth/toolrows? Yes. The boundedtotalislen(_collapse_wire_rows(all_msgs))(chat_handlers.pybounded branch), and an unbounded read'smessagescome through_prepare_messages, whose loop iterates_collapse_wire_rows(messages)as well (chat_utils.py): both sides start from the same collapsed row set — every persisted row,error/mcp_oauth/toolincluded,donedropped, a chunk run folded to one row.isDurableRowexcludes onlyqueued/streaming/thinking/permission, so the two counts can differ by exactly (a) the pending permission cards, whichsettledBoundedTotalsubtracts on the bounded side andisDurableRowexcludes on the unbounded side, and (b) the one collapsed in-flight row while a turn runs, which the bounded count includes and the unbounded derivation does not — an under-count of at most 1 on the unbounded side, which is the safe direction (a baseline below the settled count can never read as a shrink).tsstrings match the slot-detail handler's format for the same row? Both are the server's own stamp for that row (monotonic_transcript_tsat append; the WS frame carries the persisted row'sts, and the handler returns the persisted row), so they agree for assistant/tool rows. The one row that can differ is the dashboard's own optimistic user bubble, whosetsis the client ISO stamp and which has no server echo — its identity is themidthe send receipt supplies (confirmOptimisticSend).idAnchorsOneRowin the stitch check anchors onmidand treatstsas a non-contradiction test, so an optimistic user row is never the anchor row that decides the page (the oldest identified row / the page's oldest row are server rows). Where atsmismatch could bite is perf-only: an unnecessary unbounded retry, never a drop — and the earlier rounds' tests (does not retry over a live row appended while the window was in flight) cover the appended-row case.Follow-ups
PR-2 (P5-e proper)— merged as feat(chat-core): P5-e — one virtualized transcript for every chat surface #10146.next_before) for the pane's earlier-history bar beyond the handler cap, replacing the explicit full-history read as the last step.ChatPageonto the shared unit.total(pending permission rows excluded) so the client-sidesettledBoundedTotalnormalisation can go.Closes nothing; part of #10005.