fix: bound transcript growth and resume cost — rotation, streaming tail load, output cap - #96
Conversation
…il load, output cap Transcripts grew without bound and were read whole into memory on resume (file.text() + full parse), with the 20 s resume deadline only checked BETWEEN sessions — one huge transcript could block startup or OOM. - Rotate the live JSONL past 32 MiB into numbered segments, keeping two (~96 MiB retention per session instead of unbounded). - Stream-parse on load; with a byte budget, read only the newest window (skipping older segments and slicing into the oldest surviving file). Resume passes 24 MiB — just above the scrollback cap it feeds. - Apply the resume deadline WITHIN the parse (every 512 lines), not just between sessions. - Cap persisted tool output at 64 KiB per line — the dominant growth term (three lines per tool call, one carrying the full output). Broadcast and in-memory scrollback keep the full text. - Seed scrollback byte accounting from transcript line lengths on resume instead of re-serializing every restored message. 100 MB transcript: resume drops from 196 ms / +183 MB RSS to 110 ms / +36 MB; the new path is O(min(size, 24 MiB)) instead of O(size). Fixes #85 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)
📝 WalkthroughWalkthroughTranscript storage now rotates into numbered segments, loads replay data with byte and deadline bounds, and caps persisted tool output. Resume wiring now passes transcript byte hints into scrollback replay, and tests cover rotation, bounded loading, truncation, and resume restoration. ChangesTranscript rotation and bounded resume
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SessionManager
participant TranscriptStore
participant Session
participant ScrollbackBuffer
SessionManager->>TranscriptStore: loadTranscript(sessionId, { maxBytes, deadlineAt })
TranscriptStore->>TranscriptStore: select newest segments within maxBytes
loop stream-parse lines
TranscriptStore->>TranscriptStore: readLines(path, offset)
TranscriptStore->>TranscriptStore: check deadlineAt, merge entries by messageId
end
TranscriptStore-->>SessionManager: entries with bytes hints
SessionManager->>Session: restoreScrollback(messages, nextSeq, sizeHints)
loop each message
Session->>ScrollbackBuffer: push(msg, sizeHints[i])
ScrollbackBuffer->>ScrollbackBuffer: update bytes using sizeHint, evict if needed
end
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 #96 +/- ##
==========================================
+ Coverage 73.06% 74.20% +1.14%
==========================================
Files 65 65
Lines 11032 11153 +121
==========================================
+ Hits 8060 8276 +216
+ Misses 2972 2877 -95
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/tests/scrollback.test.ts (1)
139-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the upsert path with
sizeHint.Current test only covers pushing new entries with a hint. The reconciliation branch (Line 81-88 in
scrollback.ts, re-pushing an existingmessageId) is exercised during resume'sreconcileResumedMessageflow but isn't covered here.✅ Suggested additional test case
test("push honors a caller-provided size hint instead of re-serializing", () => { // Real serialized size of each message is ~150+ bytes; hints say 10. // With a 300-byte cap, hinted accounting keeps all five messages — // re-serialization would have evicted some. const buf = new ScrollbackBuffer({ maxEntries: 1000, maxBytes: 300 }); for (let i = 0; i < 5; i++) { buf.push(makeMsg(`hinted message ${i} ${"p".repeat(200)}`), 10); } expect(buf.length).toBe(5); expect(buf.bytes).toBe(50); }); + + test("push re-accounts an existing entry using the new size hint", () => { + const buf = new ScrollbackBuffer({ maxEntries: 1000, maxBytes: 1000 }); + const msg = makeMsg("original"); + buf.push(msg, 10); + buf.push({ ...msg, content: "updated" }, 25); + expect(buf.length).toBe(1); + expect(buf.bytes).toBe(25); + }); });🤖 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/scrollback.test.ts` around lines 139 - 151, The current ScrollbackBuffer sizeHint test only covers new pushes, but the upsert/reconciliation path is untested. Add a test around ScrollbackBuffer.push that re-pushes an existing messageId with a sizeHint so it exercises the reconciliation branch in scrollback.ts and verifies bytes/length are updated using the hint rather than re-serializing; this will also cover the behavior used by reconcileResumedMessage during resume.src/daemon/transcript.ts (1)
429-441: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer the already-imported async
rmover dynamicunlinkSync.
delete()dynamically importsnode:fsand uses blockingunlinkSyncfor 4 files, while the rest of the class (andnode:fs/promises) is already imported and used consistently elsewhere (rename,rm,appendFile,writeFile). Synchronous unlinks block Bun's single event loop for every other in-flight session whilesession.destroy()awaits this call.♻️ Proposed fix
async delete(sessionId: string): Promise<void> { - const { unlinkSync } = await import("node:fs"); this.#liveBytes.delete(sessionId); - - try { unlinkSync(this.transcriptPath(sessionId)); } catch { /* ignore */ } - try { unlinkSync(this.metaPath(sessionId)); } catch { /* ignore */ } + await rm(this.transcriptPath(sessionId), { force: true }); + await rm(this.metaPath(sessionId), { force: true }); for (let i = 1; i <= this.#maxRotatedSegments; i++) { - try { unlinkSync(this.#segmentPath(sessionId, i)); } catch { /* ignore */ } + await rm(this.#segmentPath(sessionId, i), { force: true }); } }🤖 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/daemon/transcript.ts` around lines 429 - 441, The `delete()` method in `Transcript` is using a dynamic `node:fs` import and blocking `unlinkSync` calls for the live file, metadata, and rotated segments, which is inconsistent with the rest of the class. Update `delete(sessionId)` to use the already imported async `rm` from `node:fs/promises` for each path, and keep the cleanup logic in `transcriptPath`, `metaPath`, and `#segmentPath` unchanged so the method remains non-blocking while `session.destroy()` awaits it.src/tests/transcript.test.ts (1)
259-282: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a non-ASCII case to the truncation test.
This test only uses ASCII content, so it wouldn't catch the char-vs-byte cap discrepancy flagged in
transcript.ts'scapPersistedToolOutput(see review comment there). Once that's fixed, a companion case with multi-byte characters (e.g. CJK or emoji) repeated past the cap would pin down the actual persisted byte size rather than just the character count.🤖 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/transcript.test.ts` around lines 259 - 282, The truncation test only covers ASCII, so it won’t catch the char-vs-byte mismatch in capPersistedToolOutput. Update the existing "caps persisted tool output while leaving the in-memory message alone" test in transcript.test.ts to add a companion non-ASCII case (for example using repeated CJK or emoji text in a tool message) and verify the persisted transcript is capped by actual byte size, not just character count. Keep the same append/flush/loadTranscript flow and assertions around the persisted tool state so the new case pins down the behavior of capPersistedToolOutput.
🤖 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/transcript.ts`:
- Around line 481-497: The persisted tool-output cap in capPersistedToolOutput
is using string length and slice, which truncates by UTF-16 code units instead
of UTF-8 bytes. Update the logic to measure and trim the
SessionMessage.tool.state.output by actual byte length so the
TOOL_OUTPUT_PERSIST_CAP budget is respected for non-ASCII content, and keep the
truncation notice accurate. Also adjust the related tests around
capPersistedToolOutput/TOOL_OUTPUT_PERSIST_CAP to cover multibyte output (not
just ASCII) and verify the persisted payload stays within the intended byte cap.
- Around line 196-239: In `#rotate`() of TranscriptWriter, the rename error
handling is too broad because both catch blocks swallow every failure instead of
only tolerating missing files. Update the segment-shift loop and the live-file
move to only ignore ENOENT from rename, and let other errors propagate or be
logged so real failures in transcriptPath() and `#segmentPath`() are not hidden.
---
Nitpick comments:
In `@src/daemon/transcript.ts`:
- Around line 429-441: The `delete()` method in `Transcript` is using a dynamic
`node:fs` import and blocking `unlinkSync` calls for the live file, metadata,
and rotated segments, which is inconsistent with the rest of the class. Update
`delete(sessionId)` to use the already imported async `rm` from
`node:fs/promises` for each path, and keep the cleanup logic in
`transcriptPath`, `metaPath`, and `#segmentPath` unchanged so the method remains
non-blocking while `session.destroy()` awaits it.
In `@src/tests/scrollback.test.ts`:
- Around line 139-151: The current ScrollbackBuffer sizeHint test only covers
new pushes, but the upsert/reconciliation path is untested. Add a test around
ScrollbackBuffer.push that re-pushes an existing messageId with a sizeHint so it
exercises the reconciliation branch in scrollback.ts and verifies bytes/length
are updated using the hint rather than re-serializing; this will also cover the
behavior used by reconcileResumedMessage during resume.
In `@src/tests/transcript.test.ts`:
- Around line 259-282: The truncation test only covers ASCII, so it won’t catch
the char-vs-byte mismatch in capPersistedToolOutput. Update the existing "caps
persisted tool output while leaving the in-memory message alone" test in
transcript.test.ts to add a companion non-ASCII case (for example using repeated
CJK or emoji text in a tool message) and verify the persisted transcript is
capped by actual byte size, not just character count. Keep the same
append/flush/loadTranscript flow and assertions around the persisted tool state
so the new case pins down the behavior of capPersistedToolOutput.
🪄 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: 9dddc0f8-5b7c-4297-89f9-ec3b2e8fcbc3
📒 Files selected for processing (7)
src/daemon/scrollback.tssrc/daemon/session-manager.tssrc/daemon/session.tssrc/daemon/transcript.tssrc/tests/scrollback.test.tssrc/tests/session-manager-tenant.test.tssrc/tests/transcript.test.ts
…nc delete CodeRabbit review on #96: cap persisted tool output by real UTF-8 bytes (non-ASCII CLI output ran 2-3x past the cap), only swallow ENOENT in segment rotation (log real rename failures instead of risking silent segment loss), use async rm in delete() instead of blocking unlinkSync, and cover the upsert-with-hint scrollback path plus a non-ASCII truncation case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/daemon/transcript.ts (3)
417-424: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMerge
toolbefore the generic shallow overwrite.The loop assigns
existingMsg.tool = newMsg.tool, so the followingObject.assign(existingMsg.tool, newMsg.tool)is a no-op. Partial tool updates can therefore drop existing nested state instead of preserving it.Proposed fix direction
// Shallow merge — newer fields overwrite older, preserving what's not in the update for (const [k, v] of Object.entries(newMsg)) { + if (k === "tool") continue; if (v !== undefined) existingMsg[k] = v; } // Deep merge tool state specifically if (newMsg.tool && existingMsg.tool) { - Object.assign(existingMsg.tool as Record<string, unknown>, newMsg.tool as Record<string, unknown>); + const existingTool = existingMsg.tool as Record<string, unknown>; + const newTool = newMsg.tool as Record<string, unknown>; + const existingState = existingTool.state; + const newState = newTool.state; + Object.assign(existingTool, newTool); + if ( + existingState && + newState && + typeof existingState === "object" && + typeof newState === "object" + ) { + existingTool.state = { + ...(existingState as Record<string, unknown>), + ...(newState as Record<string, unknown>), + }; + } + } else if (newMsg.tool) { + existingMsg.tool = newMsg.tool; }🤖 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/daemon/transcript.ts` around lines 417 - 424, The merge logic in transcript update handling is overwriting tool state before the deep merge runs, causing nested tool fields to be lost. In the message merge path in transcript.ts, update the existingMsg/newMsg merge so the tool-specific deep merge happens before the generic shallow overwrite, or otherwise skip assigning tool in the generic loop and then apply the deep Object.assign on existingMsg.tool. Make sure the logic in the merge block around existingMsg, newMsg, and Object.assign(existingMsg.tool, newMsg.tool) preserves partial nested tool updates.
392-401: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate parsed transcript entries before dereferencing.
JSON.parse()can still yieldnull, arrays, or primitives, soentry.message/msg.messageIdcan throw on a malformed-but-valid JSON line. Add a Zod schema or object guard here and skip invalid entries like corrupted lines.🤖 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/daemon/transcript.ts` around lines 392 - 401, Validate parsed transcript entries before accessing nested fields: JSON.parse in transcript processing can return null, arrays, or primitives, so entry.message and msg.messageId may throw even after a successful parse. In the transcript.ts parsing flow around the entry/messageId logic, add an object/schema guard (preferably Zod with the TranscriptEntry shape) or equivalent typeof/null checks immediately after JSON.parse, and continue to the next line for any invalid entry just like corrupted lines.Source: Coding guidelines
113-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate transcript rotation options in the constructor
segmentMaxBytesandmaxRotatedSegmentsare used directly in#rotate()andloadTranscript(), so zero/negative/non-integer values can make rotated transcript segments unreadable after restart. Parseoptswith a Zod schema here and reject invalid config early.🤖 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/daemon/transcript.ts` around lines 113 - 116, Validate the transcript rotation options in the TranscriptStore constructor by parsing opts with a Zod schema instead of assigning raw values to `#segmentMaxBytes` and `#maxRotatedSegments`. The issue is that zero, negative, or non-integer values can break `#rotate`() and loadTranscript() after restart, so reject invalid config early with a clear validation error. Update the constructor in TranscriptStore to use the schema for segmentMaxBytes and maxRotatedSegments before storing them on the instance.Source: Coding guidelines
♻️ Duplicate comments (1)
src/daemon/transcript.ts (1)
227-247: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStop rotation after non-ENOENT rename failures.
This still logs and continues after a failed segment shift. If
2 → 3fails, the later1 → 2rename can overwrite segment2, losing history; propagate the error instead of continuing. This is the same underlying rotation-corruption concern previously raised, but the current fix only added logging.Proposed fix
} catch (err) { if ((err as NodeJS.ErrnoException).code !== "ENOENT") { - console.error( - `[codeoid] transcript ${sessionId}: segment shift ${i}→${i + 1} failed: ${err instanceof Error ? err.message : String(err)}`, - ); + throw err; } } @@ } catch (err) { if ((err as NodeJS.ErrnoException).code !== "ENOENT") { - console.error( - `[codeoid] transcript ${sessionId}: rotate live→1 failed: ${err instanceof Error ? err.message : String(err)}`, - ); + throw err; } }🤖 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/daemon/transcript.ts` around lines 227 - 247, The `#rotate` method in transcript.ts still continues after a non-ENOENT rename failure during segment shifting, which can corrupt the rotation chain. Update the error handling in `#rotate` so that when rename fails for any reason other than ENOENT, the error is propagated immediately instead of only logging and continuing; apply this to both the segment-shift loop and the final live→1 rename path so later moves cannot overwrite earlier segments.
🤖 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/transcript.ts`:
- Around line 443-449: The delete() flow in TranscriptStore can race pending
fire-and-forget writes from append() and saveMeta(), causing transcript or
metadata files to be recreated after removal. Update TranscriptStore.delete() to
drain or quiesce the session first by awaiting flush() (or an equivalent
per-session write barrier) before calling rm() on transcriptPath(), metaPath(),
and the rotated segment paths.
---
Outside diff comments:
In `@src/daemon/transcript.ts`:
- Around line 417-424: The merge logic in transcript update handling is
overwriting tool state before the deep merge runs, causing nested tool fields to
be lost. In the message merge path in transcript.ts, update the
existingMsg/newMsg merge so the tool-specific deep merge happens before the
generic shallow overwrite, or otherwise skip assigning tool in the generic loop
and then apply the deep Object.assign on existingMsg.tool. Make sure the logic
in the merge block around existingMsg, newMsg, and
Object.assign(existingMsg.tool, newMsg.tool) preserves partial nested tool
updates.
- Around line 392-401: Validate parsed transcript entries before accessing
nested fields: JSON.parse in transcript processing can return null, arrays, or
primitives, so entry.message and msg.messageId may throw even after a successful
parse. In the transcript.ts parsing flow around the entry/messageId logic, add
an object/schema guard (preferably Zod with the TranscriptEntry shape) or
equivalent typeof/null checks immediately after JSON.parse, and continue to the
next line for any invalid entry just like corrupted lines.
- Around line 113-116: Validate the transcript rotation options in the
TranscriptStore constructor by parsing opts with a Zod schema instead of
assigning raw values to `#segmentMaxBytes` and `#maxRotatedSegments`. The issue is
that zero, negative, or non-integer values can break `#rotate`() and
loadTranscript() after restart, so reject invalid config early with a clear
validation error. Update the constructor in TranscriptStore to use the schema
for segmentMaxBytes and maxRotatedSegments before storing them on the instance.
---
Duplicate comments:
In `@src/daemon/transcript.ts`:
- Around line 227-247: The `#rotate` method in transcript.ts still continues after
a non-ENOENT rename failure during segment shifting, which can corrupt the
rotation chain. Update the error handling in `#rotate` so that when rename fails
for any reason other than ENOENT, the error is propagated immediately instead of
only logging and continuing; apply this to both the segment-shift loop and the
final live→1 rename path so later moves cannot overwrite earlier segments.
🪄 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: df1ca4b6-93a2-48b9-9cec-0862de0fdcf9
📒 Files selected for processing (3)
src/daemon/transcript.tssrc/tests/scrollback.test.tssrc/tests/transcript.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/transcript.test.ts
… clamp rotation opts CodeRabbit round 2 on #96: delete() now awaits the session's in-flight append/meta chains so a fire-and-forget write can't recreate files after removal; loadTranscript skips JSON lines that parse to null/arrays/ primitives instead of throwing; rotation tuning knobs are sanity-clamped to positive integers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem (#85)
Transcript files grew without bound (no rotation, compaction, or truncation) and were read whole into memory on resume via
file.text()+ full-file parse. The 20 s resume deadline was only checked between sessions, so a single months-old autonomous session with a 300 MB–1 GB transcript could block daemon startup — or OOM it.Fixes
Rotation (
transcript.ts)The live
<id>.jsonlrotates into numbered segments (.1newer,.2older) once it passes 32 MiB; two rotated segments are kept, older ones deleted. Retention is now ~96 MiB per session instead of unbounded — a deliberate policy change: the transcript is a resume/replay log, not an archive. Rotation runs inside the existing per-session append chain (no race with writes), and the size counter is seeded lazily from the on-disk size, so it picks up pre-existing files correctly.delete()removes segments too.Bounded, streaming load (
transcript.ts,session-manager.ts)loadTranscriptnever materializes a whole file as a string anymore — files are stream-parsed line by line. WithmaxBytesset, only the newest window of the log is read at all: older segments are skipped and the oldest surviving file is entered mid-way (byte offset, first partial line dropped). Resume passes 24 MiB — just above the 20 MiB scrollback cap it feeds, so everything it would have parsed beyond that got evicted on arrival anyway. Trimming logs a warning (no silent cap). This also fixes resume for pre-existing giant transcripts from before rotation existed — the tail-slice bounds the read regardless of file size.share.packkeeps its unbounded (but now streaming) read.Deadline within the parse (
transcript.ts,session-manager.ts)The resume deadline is now checked every 512 lines during a parse, not just between sessions. If it fires mid-file, the entries merged so far are replayed (logged) rather than wedging startup.
Persisted tool-output cap (
transcript.ts)Up to three JSONL lines are written per tool call, one carrying the full tool output — the dominant transcript-growth term. Persisted output is now capped at 64 KiB per line with an explicit truncation marker. The live broadcast and in-memory scrollback keep the full text; only the on-disk replay copy is trimmed. The cap clones — the caller's message object is untouched (test-pinned).
No re-serialization on restore (
scrollback.ts,session.ts)restoreScrollbackused toJSON.stringifyevery restored message purely for byte accounting.loadTranscriptnow returns each entry's line byte-length (bytes, load-only, never persisted) andScrollbackBuffer.pushaccepts it as a size hint.Micro-bench
Synthetic 100 MB transcript (10k tool-call lines with 10 KB outputs):
The structural point: resume was O(transcript size) in both time and memory — at 1 GB that's ~2 s and ~1.8 GB RSS on the daemon event loop before listen. It is now O(min(size, 24 MiB)): constant ~110 ms / +36 MB no matter how large the log on disk, and rotation caps the on-disk size going forward anyway. (The pack path still materializes all entries — inherent to its export contract — but no longer holds the whole file as one string alongside them.)
Tests
maxBytesreturns strictly the newest suffix withbyteshints populated, expireddeadlineAtaborts mid-file without wedging, 80 KiB tool output truncated on disk while the caller's object is untouched,deleteremoves segments.pushhonors the size hint (hint-vs-reserialize distinguishable via the byte cap).resumeSessionsend-to-end test — meta + transcript on disk → resume → attach replays the restored scrollback (this path had no direct coverage before).bun x tsc --noEmit✓ ·bun run lint✓ ·bun run test759 pass / 0 fail ✓Fixes #85
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests