fix: streamed messages pushed into scrollback twice — commit finalize via upsert - #74
Merged
Merged
Conversation
… via upsert, not a second push The #50 fix covered only #artificiallyStreamText. The main streaming path still called #persistAndBuffer at BOTH stream start (text_delta creates the message) and finalize (text_done), and the same double-push lived in #flushActiveAssistant (interrupt/turn-boundary flush) and #finalizeActiveThinking (every thinking block). Consequences: - scrollback.replay carried two entries per streamed messageId; clients rendered the message twice and the web virtualizer's messageId-keyed caches collided (the residual cause of the 'intermittent message overlap' that #73 partially fixed) - the memory chunker received the stream-start push with empty content, emitting a prompt-only user_turn episode and then a promptless assistant_turn — every plain turn fragmented into two half-episodes - byte accounting drifted negative (push #1 accounted the empty size, eviction subtracted the grown size twice), permanently disabling the 20MB scrollback cap Fixes: - ScrollbackBuffer now records the accounted size per entry and upserts by messageId: re-pushing a buffered id re-accounts the existing entry in place (keeping its replay position) instead of appending a duplicate. Eviction subtracts exactly what was added — negative drift is structurally impossible. updateMessage is O(1) via the id index (was a front-to-back scan). - Session stream-start sites push to scrollback only; the new #commitStreamed emits the durable transcript row and the chunker event exactly once, at finalize, with final content. #artificiallyStreamText drops its bespoke reset-and-updateMessage dance for the same helper. - #seq now seeds past the persisted transcript tail on resume instead of restarting at 0, making seq usable as a monotonic replay cursor. Tests: session-stream-commit.test.ts pins one-scrollback-entry-per- messageId across all four finalize paths (text_done, thinking_done, batch-reply artificial streaming, turn-boundary flush), buffer upsert + byte-cap accounting under by-reference growth, chunker episode pairing (including a test documenting the pre-fix fragmentation), and seq continuation after resume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughScrollbackBuffer now upserts entries by messageId with byte accounting, streaming session paths defer durable commits until finalization, resumed sessions restore the next sequence number, and new tests cover the updated behavior. ChangesStreamed Commit Deduplication and Resume Sequencing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant Session
participant ScrollbackBuffer
participant TranscriptStore
participant EpisodeChunker
Provider->>Session: text_delta / thinking_delta
Session->>ScrollbackBuffer: push (scrollback-only)
Provider->>Session: text_done / turn end
Session->>Session: `#commitStreamed`(msg)
Session->>ScrollbackBuffer: updateMessage(final content)
Session->>TranscriptStore: append once
Session->>EpisodeChunker: notify once
sequenceDiagram
participant SessionManager
participant TranscriptStore
participant Session
SessionManager->>TranscriptStore: read persisted transcript entries
SessionManager->>SessionManager: compute max seq
SessionManager->>Session: restoreScrollback(messages, maxSeq + 1)
Session->>Session: advance sequence cursor
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
+ Coverage 80.72% 81.63% +0.90%
==========================================
Files 56 56
Lines 7895 7899 +4
==========================================
+ Hits 6373 6448 +75
+ Misses 1522 1451 -71
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
src/tests/session-stream-commit.test.ts (2)
421-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftC7 bypasses the actual
SessionManager.resumeSessionscode path.The test hand-computes
maxSeqfromloadTranscriptand callssession.restoreScrollbackdirectly instead of invokingSessionManager.resumeSessions. Per the PR stack, the real fix lives inSessionManager.resumeSessions(computing max persisted seq and passingmaxSeq+1). If that method's logic ever diverges from this manual reimplementation (e.g. different reduce semantics, additional filtering, off-by-one), this test would still pass while the production resume path regresses.Consider invoking
SessionManager.resumeSessions(or its session-scanning helper) directly, if feasible with the existing daemon test scaffolding, so the test protects the actual code path rather than a parallel reimplementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/session-stream-commit.test.ts` around lines 421 - 424, The test is reimplementing the resume logic instead of exercising SessionManager.resumeSessions, so it can miss regressions in the real production path. Update the test to call SessionManager.resumeSessions directly, or the same session-scanning helper it uses, and assert the resumed scrollback/seq state through that path rather than manually loading transcript entries and calling session.restoreScrollback. Keep the existing daemon scaffolding and use the SessionManager and resumeSessions symbols to locate the change.
354-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff"Pre-fix fragmentation" test only exercises
EpisodeChunkerin isolation.This test documents the historical bug by manually feeding
EpisodeChunker.onMessagewith a hand-crafted empty-then-full assistant sequence, but never verifies that the liveSessionactually calls the chunker exactly once with final content (the C1-C4 tests only check scrollback duplicates, not chunker call count/content). A regression whereSessionre-introduces a straychunker.onMessagecall at stream-start would not be caught by any test in this suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/session-stream-commit.test.ts` around lines 354 - 371, The fragmentation regression test only covers EpisodeChunker in isolation and misses the real Session behavior. Extend the session-stream commit coverage to assert that Session invokes EpisodeChunker.onMessage only once for the streamed assistant turn, with the final non-empty content, so a stray stream-start empty call would fail. Use the existing Session and EpisodeChunker test setup around the streamed-message flow to verify call count and payload, not just scrollback contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/daemon/scrollback.ts`:
- Around line 69-77: The scrollback byte tracking in scrollback.ts is using
JSON.stringify(...).length, which counts characters instead of UTF-8 bytes and
can violate the 1MB cap for non-ASCII messages. Add a shared helper in
Scrollback to compute serialized size with real byte accounting
(Buffer.byteLength(..., "utf8") or TextEncoder) and use it everywhere size is
set or updated, including the existing entry update path and the Entry creation
path, so `#bytes` and eviction in `#evict` stay accurate.
---
Nitpick comments:
In `@src/tests/session-stream-commit.test.ts`:
- Around line 421-424: The test is reimplementing the resume logic instead of
exercising SessionManager.resumeSessions, so it can miss regressions in the real
production path. Update the test to call SessionManager.resumeSessions directly,
or the same session-scanning helper it uses, and assert the resumed
scrollback/seq state through that path rather than manually loading transcript
entries and calling session.restoreScrollback. Keep the existing daemon
scaffolding and use the SessionManager and resumeSessions symbols to locate the
change.
- Around line 354-371: The fragmentation regression test only covers
EpisodeChunker in isolation and misses the real Session behavior. Extend the
session-stream commit coverage to assert that Session invokes
EpisodeChunker.onMessage only once for the streamed assistant turn, with the
final non-empty content, so a stray stream-start empty call would fail. Use the
existing Session and EpisodeChunker test setup around the streamed-message flow
to verify call count and payload, not just scrollback contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7955792b-792f-4257-9fc0-1355e06bbb83
📒 Files selected for processing (4)
src/daemon/scrollback.tssrc/daemon/session-manager.tssrc/daemon/session.tssrc/tests/session-stream-commit.test.ts
…ession test CodeRabbit review follow-ups on #74: - String.length counts UTF-16 code units; use Buffer.byteLength so the 20MB cap holds for non-ASCII payloads - end-to-end test that a real Session + MemoryEngine ingests exactly one combined user+assistant episode per streamed turn (a stray stream-start chunker feed would fail it) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Addressed the review in b01879e:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
This was referenced Jul 3, 2026
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
1 participant
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.
Problem
The #50 fix covered only
#artificiallyStreamText. The main streaming path still pushed every streamed message into scrollback twice — once whentext_deltacreates the message, again attext_done— and the same double-push lived in#flushActiveAssistant(interrupt/turn-boundary flush) and#finalizeActiveThinking(every thinking block).Consequences (all verified in code):
scrollback.replaycarried two entries per streamed messageId; the web virtualizer's${sid}:${messageId}-keyed size/element caches collided → the residual cause of the intermittent overlap fix: O(N) session switch and intermittent message overlap in web UI #73 partially fixed.user_turn+ a promptlessassistant_turn.#bytesdrifted negative and the 20MB cap never fired.Fix
ScrollbackBufferrecords the accounted size per entry and upserts by messageId: re-pushing a buffered id re-accounts the existing entry in place (keeping replay position) instead of appending a duplicate. Eviction subtracts exactly what was added — negative drift is structurally impossible.updateMessageis now O(1) via the id index (was a front-to-back scan on every tool state transition).#commitStreamedemits the durable transcript row + chunker event exactly once, at finalize, with final content.#artificiallyStreamTextdrops its bespoke reset-and-updateMessage dance for the same helper.#seqseeds past the persisted transcript tail on resume (was reset to 0), making seq usable as a monotonic replay cursor for future attach pagination.Tests
New
session-stream-commit.test.ts(12 tests) pins:Full suite: 640 pass / 3 pre-existing failures (optional
openai/@google/generative-aipackages not installed locally). Lint clean.🤖 Generated with Claude Code
Summary by CodeRabbit