fix(daemon): retain a flat per-session scrollback window - #13061
Conversation
The daemon retains ~5000 rows of xterm grid per live session with no aggregate bound, so retention scales with an unbounded session count. A host owning 100+ terminals held ~1 GB of grid, was killed under system memory exhaustion, and took every session it owned with it. Split a fixed row budget across live sessions instead: full depth until the budget binds, then an even share down to a floor that still restores the command which produced the visible screen. Re-applied on create and reap, so depth returns to survivors as terminals exit. The emulator itself stays load-bearing (on-disk history is a projection of it), so this bounds the aggregate rather than lowering the per-session default.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe daemon now defines configurable scrollback depths and selects least-recently-viewed parked sessions for trimming. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/daemon/daemon-scrollback-budget.ts (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the rationale comments.
Keep the retention rationale in one concise comment. Move incident details and heap estimates to documentation.
As per coding guidelines, “Comments must be concise, limited to non-obvious information, and preferably one line.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f44f0b1-eb22-456e-bcfc-4c4cbc44a3ac
📒 Files selected for processing (7)
src/main/daemon/daemon-scrollback-budget.test.tssrc/main/daemon/daemon-scrollback-budget.tssrc/main/daemon/headless-emulator-modes.tssrc/main/daemon/headless-emulator.tssrc/main/daemon/session.tssrc/main/daemon/terminal-host-scrollback-budget.test.tssrc/main/daemon/terminal-host.ts
Electron QA — daemon scrollback budget (PR #13061)Build verified: CDP attach to Method: electron-vite dev with What was exercised
Daemon RSS (this PR only — not vs main)
Daemon stayed well under the multi-GB incident class with 100+ sessions. Did not run the same heavy scenario on ScreenshotsFew terminals — deep output present (end of 2500-line fill):
Many terminals open (41):
Budget-path terminal after 4000-line fill (still shows recent history / prompt):
101 sessions open (tab overflow / scroll controls visible):
Remount / reattach Terminal 1 (history still present):
Terminal 1 still live with ~100 tabs open:
After open/close stress (app still healthy):
Caveats / what I could not verify
Bottom line
|
…rked LRU overflow Replace the even-split budget: dividing a fixed row budget across all sessions shallowed every terminal as the count grew, so a user with 60 terminals silently lost half their reachable scrollback — depth degradation with no signal. Retention now trims by attention, not arithmetic. Attached sessions always keep full depth and never consume the cap. Parked sessions keep full depth up to a cap of 24, least-recently-viewed evicted first down to 1000 rows — enough to reattach with the recent command context on screen. A reattached session returns to full depth for everything it emits afterward, and a freed slot returns the newest trimmed session to deep retention. Worst case is bounded at cap x full depth plus trimmed remainder, the same memory class as the old budget, without ever shallowing a terminal the user is looking at.
brennanb2025
changed the title
…nnot pin full depth The attached-client exemption made an attachment that outlives its transport a permanent full-depth pin: protocol detach was logging-only, and neither control- nor stream-socket loss removed the Session.attachedClients entry. Enough dead attachments would silently rebuild the unbounded retention the LRU cap prevents. Track the attach token per session, release exactly the dropped client's attachments in one batched retention pass, implement protocol detach for real, and cancel an attach whose client vanished mid-flight. Move session-exit bookkeeping to the host reap hook — it previously lived in the per-attachment exit callback, which only fired for unattached sessions BECAUSE of the leak, so fixing the leak made an unattached session's exit invisible to idle shutdown. Also drop the create-time double recency increment. Co-developed with a review pass; transport-drop release is regression-tested against the unfixed daemon (fails without, passes with).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/daemon/daemon-server-attachment-lifecycle.test.ts (1)
118-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cancellation error.
Lines 118-120 convert every
createOrAttachfailure into success. The test does not verify the requiredTerminalAttachCanceledErrorresult. Assert thatrequestrejects withTerminalAttachCanceledErrorafterfinishRequest().src/main/daemon/terminal-host.ts (1)
108-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an authoritative daemon-buffer regression test.
The current QA evidence did not prove that parked sessions are trimmed because snapshot APIs may read another buffer. Add a test that parks a session, exceeds
DAEMON_SCROLLBACK_TRIMMED_PARKED_ROWS, triggers a lifecycle retention pass, and reads the authoritative daemon snapshot. Also verify reattachment behavior for subsequent output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d1e611e-bb4a-4f3d-990b-807535fb686f
📒 Files selected for processing (4)
src/main/daemon/daemon-server-attachment-lifecycle.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-transport-attachment-release.test.tssrc/main/daemon/terminal-host.ts
Electron QA — attention-based scrollback retention (post design change)Build under test: 1) Headline — actively used terminal keeps full history with 30+ sessions ✅
After fill (bottom of history):
After 34 sessions + reattach + scroll to top (earliest lines intact):
Hard requirement satisfied: a terminal I’m using still scrolls all the way back with far more than 24 terminals open. 2) Recently-viewed reattach keeps full history ✅
3) Long-parked past LRU cap → ~1000 rows
|
| Item | Status |
|---|---|
| Parked-beyond-cap trim to ~1000 rows in real UI | Unit/host tests only; open tabs did not enter detached park |
| Dead client transport releases attachments so a dead client cannot pin full depth | Code path present (detachClientSessions on transport drop); not exercised in this Electron session |
| Windows host (incident platform) | macOS only |
| Multi-client (desktop + mobile) attach/park interaction | Not tested |
Verdict
Headline hard requirement passes on this build: with 34 live sessions, the terminal in use retains and can scroll back to QA_LINE_0001. Recently-viewed reattach also keeps full filled history. Daemon RSS stayed modest. Parked-cap trim and transport-drop release are supported by tests/code but were not reproduced as live UI screenshots here — call that residual risk, not a headline regression.
terminal-host.ts crossed the 300-line cap; the entry-building and depth application belong with the selection policy anyway, leaving the host with only the recency bookkeeping it uniquely owns.
…budget The previous commit accidentally swept QA screenshots and a report from the repo root into the tree (root-directory-guard failure), and headless-emulator had crept to 301 counted lines. Screenshots live on the PR via the attachments CDN, never in the tree. The retained-scrollback trim moves to headless-emulator-modes with the OSC-link shift as a pure helper.
Replace the LRU parked-session retention with what every peer terminal ships: a flat, small per-session window in the durable host. Sessions retain 1000 rows — the generous end of what terminal products restore on a rebuild — and deep scrolling on an open terminal remains the renderer's live buffer. This deletes the dynamic-retention machinery entirely: no recency tracking, no attached-exemption, no runtime trimming, no OSC-link shift on trim. The window is set once at session creation. Worst case at the incident's 100+ sessions is ~100k rows of grid, versus ~500k before. The bounded env override can tune the window within [100, 5000]; anything outside falls back to the default, since an unbounded daemon is the failure this window exists to prevent. The v29->v30 history-handoff fixture now pins the old-daemon depth explicitly: it plays an old binary whose sessions retained ~5000 rows, and the new flat window would otherwise shrink its history below the chunked-seed threshold and silently skip the transfer path the test exists to cover.
brennanb2025
changed the title
QA: flat 1000-row daemon scrollback window (HEAD
|
1 participant
Summary
The terminal daemon retained ~5000 rows of xterm grid per live session with no bound, so memory scaled with an unbounded session count. On a workstation running many agent worktrees the daemon reached ~1.9 GB, was killed under system memory exhaustion, and took every session it owned with it.
The fix: a flat, small per-session scrollback window. Daemon sessions retain 1000 rows, set once at creation. A terminal the user has open scrolls its full live renderer buffer regardless; the daemon window is what a rebuild (window reload, pane remount, app restart, remote/mobile attach) restores.
Why 1000, and why 5000 was never a real decision
Agent conversations, explicitly considered
Inline-rendering agent CLIs write their conversation into normal-buffer scrollback, so on a rebuild an agent transcript longer than the window shows only its newest 1000 rows. This was judged acceptable to ship because (a) the durable conversation record is the agent's own transcript file, which the native-chat subsystem reads in full, plus agent resume; (b) the old 5000 already truncated long agent sessions on rebuild — any RAM window does; and (c) raising the window for agent sessions specifically would rebuild the incident on exactly the agent-heavy hosts this PR protects. The product answer for full-depth agent scrollback is disk-backed session history (bounded per-session file, rebuilds replay from disk), tracked as a separate designed project; this window then becomes its RAM cache.
Honesty about the incident's composition
Per the #13684 bench, clean grid at 5000 rows accounts for ~100–300 MB at the incident's session count — the dominant scaling term this PR bounds, but not the full 1.9 GB, which included width/attribute-heavy content and other per-session terms. Daemon memory instrumentation is a follow-up.
Two earlier revisions of this PR live in its history and were deliberately replaced: an even-split row budget (shallowed every terminal as the count grew) and an LRU parked-session retention (preserved deep rebuilds for recent sessions, at the cost of recency tracking and runtime trimming — moving parts with failure modes of their own, one of which reviews caught live). The final design deletes all of it: one constant, one bounded override.
Retained from those revisions because they fix real pre-existing bugs regardless of retention policy: transport-drop attachment release (protocol
detachwas a logging-only TODO; a dropped client leftattachedClientsentries forever), the attach-vs-disconnect race guard, and session-exit bookkeeping moved to the host reap hook (idle shutdown previously only worked for unattached sessions because of the attachment leak).Related: #13684 bounds serve-side memory and is complementary — it feeds a smaller default to serve-mode runtime emulators, a surface this PR deliberately leaves untouched (history replay must handle old deep checkpoints). It will need a rebase once this lands, since daemon sessions then always receive an explicit window.
Screenshots
No visual change for open terminals (live renderer buffer). Behavior change on rebuild-from-daemon: restores 1000 rows instead of 5000. QA evidence in PR comments (latest comment tests this final design; the two earlier QA comments tested superseded designs and are historical): live terminal scrolls to line 1 of 3000 with no reload; a real remount restores the ~1000-row window with newest content intact.
Testing
pnpm lintpnpm typecheckpnpm test— fullsrc/main/daemonsuite: 1355 passed, 3 skipped, 0 failures. Full-repo run not executed locally.pnpm build— not run.Window default/override bounds (inclusive [100, 5000], malformed falls back); end-to-end host test proving retained rows cap at the window with newest content surviving; the v29→v30 history-handoff fixture pins old-daemon depth explicitly so the chunked-seed path stays covered; transport-drop release regression-tested against the unfixed daemon.
AI Review Report
Three until-clean review rounds across this PR's three designs, all with same-model high-reasoning subagents; the final flat-window round verified: the window reaches exactly the one production Session creation site and only daemon sessions (history-reader scratch and runtime emulators keep prior depth so old deep checkpoints replay fully); deletion completeness for all removed retention symbols; the kept transport/detach/idle-shutdown fixes intact and tested; chunked history-seed transfer reachable where it matters; override clamping. One finding (stale retention-era comments) fixed in cfa9e98. Cross-platform: no platform branches; identical on macOS, Linux, Windows; the incident host was Windows.
Security Audit
No input handling, command execution, path, auth, secrets, or IPC surface changes. One env var (
ORCA_DAEMON_SESSION_SCROLLBACK_ROWS) parsed as a strict bounded integer; out-of-range falls back to the default. No new dependencies.Notes
Restore-depth expectation changes on rebuilds only (5000 → 1000). Follow-ups tracked separately: disk-backed session history (full-depth agent scrollback + history surviving daemon death), daemon memory instrumentation, daemon crash-loop guard, build-tool memory backpressure.