Record late streaming content after compression rebuild (Fixes #3338) - #3372
Conversation
#3264) HistoryService.add()/clear() queue operations while compression holds the history lock, and endCompression() used to flush that queue in plain FIFO order. A streaming add() queued before the compression rebuild's clear() was therefore destroyed by that clear, and because RecordingIntegration suppresses contentAdded until compressionLockReleased, the wiped content was never recorded either: mid-compression streaming output silently vanished from history and from replay. Flush is now two-phase, partitioned at the FIRST queued clear: the rebuild slice runs first (its contentAdded stays inside the recording suppression window, so rebuilt entries are not double-recorded), then compressionLockReleased/compressionEnded, routed through runSynchronousHistoryMutation so they share the mutation FIFO with deferred closures and the drain order rebuild, events, streaming holds even while a replaceAll is in flight, then the streaming slice, whose contentAdded now fires after the lock release so recording captures it and replay can recover it. The queue attempts every unit under its own failure capture and combines failures with combineMutationFailures, so a throwing listener can no longer discard queued operations or suppress the release events, and a thrown undefined propagates truthfully. The queue lives in a new CompressionOperationQueue class preserving the #2852 never-drop contract and its one-shot high-water diagnostics (re-armed by both flush() and clear()).
|
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 ignored due to path filters (1)
📒 Files selected for processing (11)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds synchronous ChangesCompression rebuild handling
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🔵 Low · up to The change preserves late streaming content in history recordings and replay, but a failing compression lifecycle listener could prevent completion notification and leave downstream state inconsistent. The PR is mergeable with explicit owner awareness and follow-up to isolate listener failures. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The production changes and tests remain focused on issue Full details: Description checkExplanation The description includes all required sections. It explains the change, implementation, test plan, testing results, platform limitations, and linked issues. The incomplete testing matrix entries are clearly marked as unverified, while the macOS results and known test limitations are documented.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
maintainer:e2e:ok
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughBefore this change, compression rebuilds could erase or misorder late streaming content because history rebuild and streaming mutations were not scoped separately, and cache-anchor state could survive a truncating fallback even though the preserved prefix had been destroyed. After this PR, the core history service tags queued operations as rebuild or streaming and flushes them in a defined rebuild-then-streaming order so queued streaming content is reapplied after the rebuild and recorded after the lock is released. The compression and agent enforcement paths now wrap fallback and cache-anchor rebuilds in the new rebuild scope, and the cache-anchor helper explicitly resets or advances the anchor based on whether the prefix survived. Release NotesBug Fixes
Refactor
Tests
Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent Enforcer
participant History as HistoryService
participant Queue as CompressionOperationQueue
participant Recorder as RecordingIntegration
Agent->>History: startCompression()
Agent->>History: rebuildWith(clear/add/anchor updates)
History->>Queue: tag operations as rebuild
Agent->>History: late streaming add()
History->>Queue: tag operation as streaming
Agent->>History: endCompression(summary)
History->>Queue: flush rebuild operations first
Queue-->>History: rebuilt contentAdded events
History->>Recorder: compressionLockReleased
History->>Recorder: compressionEnded(summary)
Queue-->>History: flush streaming operations
History->>Recorder: late streaming contentAdded
Magnitude🎯 3 (L) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — PR #3372
|
|
OCR finding |
|
OCR finding |
|
CodeRabbit’s docstring-coverage warning is classified Reject. It is a generic finishing-touch metric, not an identified behavior defect. The touched functions include test helpers and small private methods whose names and types state their contracts; adding docstrings to reach an arbitrary percentage would conflict with this repository’s sparse-comment style. CodeRabbit generated no actionable code comments, and its status check passed. |
1 participant
TLDR
This stacked change makes compression rebuild work explicit. Rebuilt history still emits while recording suppression is active, while ordinary streaming content queued before or after the rebuild emits after
compressionLockReleasedand remains recoverable from the session recording.This PR is based on
issue3264, the head of PR #3336. It should remain stacked until #3336 lands. Reviewers should focus onHistoryService.rebuildWith(), phase classification inCompressionOperationQueue.flush(), and the recording/replay regression test.Dive Deeper
PR #3336 partitions queued compression work by position at the first
clear(). That preserves streaming content queued before the clear, but a streamingadd()queued after the rebuild block is still placed in the rebuild phase. ItscontentAddedevent fires whileRecordingIntegrationis suppressing rebuilt entries, so live history contains the entry while replay loses it.This change adds one synchronous rebuild scope to
HistoryService. Adds and clears made inside that scope are taggedrebuild; ordinary queued mutations outside it are taggedstreaming. The queue runs rebuild operations first, emits the existing release/end lifecycle events, then runs streaming operations. FIFO within each phase, failure aggregation, high-water diagnostics, and the never-drop contract are unchanged.The three production rebuild paths now use the scope:
The callback type is
() => undefined, which rejects async callbacks at type-check time. A compile-time regression test protects that synchronous contract.Behavior tests cover clear-only rebuilds, multiple rebuilt entries, streaming entries on both sides of the scope, multiple clear/retry sequences, no-lock synchronous use, all three agent callers under an active compression lock, exact lifecycle event order, final history order, and a real recording/replay round trip. The persisted sequence is
content(original) -> compressed -> content(late), and replay yields[summary, late entry]without duplicating retained entries.TDD RED logs, review triage, mutation checks, test-audit comparison, and verification details are recorded in
project-plans/issue3338/plan.md. Compliance and local Open Code Review findings are resolved or explicitly rejected with rationale there.Reviewer Test Plan
Run the focused behavior suites:
The core set passes 1,068 tests across 68 files. The focused agent compression set passes 580 tests. Mutation checks confirm that removing any migrated
rebuildWithwrapper breaks its caller test, and forcing all queued work into the streaming phase breaks 12 boundary rows.Local verification on macOS:
npm run typecheck,npm run format, andnpm run buildpass.npm run testcompleted every workspace. Four unrelated agents files reached their isolated-process timeouts after 372 of 376 files passed; all four passed together in isolation, 47 tests in 7.17 seconds.bun scripts/start.ts --profile-load zai "write me a haiku and nothing else"passes againstzai:glm-5.3.Testing Matrix
The macOS npm result is marked with a warning because the full test orchestrator had the four load-sensitive timeouts described above and the fixed-heap lint script exhausted its heap. All affected files and the equivalent full lint traversal pass separately.
Linked issues / bugs
Fixes #3338
Depends on #3336. Related: #3264, #3132, #3263, #2852.
Summary by CodeRabbit