fix: chunk large scrollback replays to avoid WS backpressure lockout (#84) - #100
Conversation
…84) attach() replayed the entire scrollback as one scrollback.replay frame. A session near its 20MB buffer cap produced a frame larger than the 16MB WS outbound backpressure limit, so Bun force-closed every client that attached (a permanent reconnect loop that made the session un-attachable), and the one-shot JSON.stringify of the whole buffer stalled the event loop. Partition the replay by byte budget (ScrollbackBuffer.readChunked) and stream ordered chunks oldest->newest (seq/final), pacing on socket drain so no single frame approaches the backpressure limit. Sessions small enough to fit one frame still send the legacy single-frame shape unchanged. Because the pacing is async, live broadcasts to the attaching client are buffered until the replay completes so newer messages can't interleave ahead of older replayed ones. Also speeds first paint for the web UI and TUI: chunks render incrementally instead of blocking on one large frame + parse. The seq/final fields are additive and optional; clients that predate them replace on each frame and end on the newest chunk (graceful degradation). Co-Authored-By: Claude Opus 4.8 (1M context) <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)
📝 WalkthroughWalkthroughThis PR splits large scrollback replays into bounded chunks, adds replay sequencing and completion fields, waits on WebSocket backpressure between chunks, and updates the web client to append multi-frame replay data in order. ChangesChunked Scrollback Replay
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant ScrollbackBuffer
participant WebSocket
participant WebStore
Client->>Session: attach()
Session->>ScrollbackBuffer: readChunked(REPLAY_CHUNK_BYTES)
ScrollbackBuffer-->>Session: chunks[]
loop each chunk
Session->>WebSocket: send scrollback.replay(seq, final)
WebSocket-->>Client: replay frame
Session->>WebSocket: flush()
WebSocket-->>Session: resolve on drain
WebStore->>WebStore: replaceScrollback or appendScrollback
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 75.66% 75.85% +0.19%
==========================================
Files 66 66
Lines 11258 11308 +50
==========================================
+ Hits 8518 8578 +60
+ Misses 2740 2730 -10
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.
🧹 Nitpick comments (1)
src/daemon/server.ts (1)
376-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
ws.datashape into a shared type.The same inline
ws.datatype is spelled out four times (open,message,drain,close) with slightly different field subsets. That is drift-prone — a future field added to one cast can silently diverge from the others. Define oneSocketDatatype and cast against it everywhere.♻️ Suggested extraction
type SocketData = { clientId: string; authenticated: boolean; auth: AuthContext | null; authTimer?: ReturnType<typeof setTimeout>; drainWaiters?: Array<() => void>; };Then use
const data = ws.data as SocketData;in each handler.Also applies to: 385-385
🤖 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/server.ts` at line 376, The ws.data object shape is duplicated across the server socket handlers, which risks the casts drifting apart over time. Define a shared SocketData type in server.ts for the common fields used by the open, message, drain, and close handlers, then replace each inline ws.data cast with ws.data as SocketData so the type stays consistent in one place.
🤖 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.
Nitpick comments:
In `@src/daemon/server.ts`:
- Line 376: The ws.data object shape is duplicated across the server socket
handlers, which risks the casts drifting apart over time. Define a shared
SocketData type in server.ts for the common fields used by the open, message,
drain, and close handlers, then replace each inline ws.data cast with ws.data as
SocketData so the type stays consistent in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 414cd2b0-8cd7-4a11-ae0e-b4d6838442c2
📒 Files selected for processing (10)
src/daemon/scrollback.tssrc/daemon/server.tssrc/daemon/session.tssrc/protocol/types.tssrc/tests/scrollback.test.tssrc/tests/session-integration.test.tsweb/src/protocol/types.tsweb/src/state/connection.tsweb/src/state/messages.test.tsweb/src/state/messages.ts
Address CodeRabbit review on #100: the ws.data shape was spelled out inline in four socket handlers (open/message/drain/close) with slightly different field subsets — drift-prone. Define one SocketData type, cast against it everywhere, and enforce the shape at the source via `satisfies SocketData` on the upgrade init. Type-only change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Applied the nitpick in ffd204b — extracted a shared |
…log (#117) The 0.2.0 entry only covered the protocol/packages train (#100-#116) and missed ten PRs that also ship in this release: the untrusted-content sanitization and cross-tenant memory fixes (#91, #93 — now under a proper Security heading), the performance run (#94-#99), and the model catalog work (#78, #79). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #84.
Problem
Session.attach()replayed the entire scrollback as a singlescrollback.replayWS frame. A session near its 20 MB buffer cap produced a frame larger than the server's 16 MB outbound backpressure limit (closeOnBackpressureLimit: true), so Bun force-closed every client that attached — a permanent reconnect loop that made the session un-attachable. Separately, the one-shotJSON.stringifyof the whole buffer stalled the daemon event loop on every attach.Fix
ScrollbackBuffer.readChunked(maxBytes)— partitions buffered messages into ordered chunks (oldest→newest) using the byte sizes already accounted per entry (no re-serialization). A lone message larger than the budget gets its own chunk.Session.attach()— sessions that fit one frame still send the legacy single-frame shape unchanged (noseq/final, fully synchronous). Larger sessions stream chunks paced on socket drain, so no single frame approaches the backpressure limit and the event loop isn't blocked.AttachedClient.flush?()resolves on socket drain (BungetBufferedAmount+ adrainhandler; also released oncloseso a mid-replay detach can't hang).ScrollbackReplayMsggains optionalseq/final(additive; ignore-unknown-fields discipline). Web client resets scrollback onseq === 0/absent and appends onseq > 0. Clients that predate the fields replace on each frame and end on the newest chunk (graceful degradation).Bonus: the web UI and TUI get faster first paint on large sessions — chunks render incrementally instead of blocking on one large frame + parse.
Testing
ScrollbackBuffer.readChunked: empty →[], single-chunk-fits, ordered multi-chunk partition covering every message once within budget, lone-oversized-message gets its own chunk.Session+restoreScrollbackwith 3×~3 MB messages): orderedseq/finalframes covering every message; flush pacing (only the first chunk emitted while parked on drain); detach mid-replay stops streaming; live broadcast during replay is buffered and delivered after the final replay frame.appendScrollback: extends a reset buffer in order, epoch bump + empty-chunk no-op, redelivered-messageId upsert keeps position (uses the O(1)indexBySessionintroduced in fix: O(1) positional lookup in the web streaming reducers #99).Note
The WS-level
flush/drainglue inserver.ts(~15 lines over Bun's documented API) is not unit-tested — there's no real-socket daemon harness in the repo. The pacing logic it feeds is fully covered at theSessionlevel via an injectableflush.Follow-ups (not in this PR)
codeoid-ui) can adopt the same append-on-seq>0logic to show full history on large sessions; today it degrades gracefully to the newest chunk.ScrollbackBuffer.readSince()exists but is still unwired — an incremental catch-up protocol message would further cut reconnect bandwidth on flaky links (relevant to the mobile client).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests