Keep long-lived multi-agent workspaces fast with experimental terminal pane eviction (opt-in)#7448
Open
brennanb2025 wants to merge 10 commits into
Open
Keep long-lived multi-agent workspaces fast with experimental terminal pane eviction (opt-in)#7448brennanb2025 wants to merge 10 commits into
brennanb2025 wants to merge 10 commits into
Conversation
…spaces fast Hidden terminal panes beyond a 12-tab warm set are unmounted after 5 minutes (xterm/DOM/listeners/heap released) and restored from the always-current main-process mirror on re-show; the PTY and its status/title feed stay alive via a parked transport. Experimental, default OFF. Flag-off behavior is pinned byte-identical to today. STA-1282 Co-authored-by: Orca <help@stably.ai>
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds an experimental terminal pane eviction flow that parks hidden PTYs instead of destroying them, with shared settings, mount-policy logic, a parked-pane registry, coordinator logic, transport parking support, lifecycle integration, store wiring, UI settings, and test coverage. It also updates the 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/renderer/src/components/terminal-pane/pane-mount-policy.test.ts (1)
173-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for parked-candidate ranking interaction.
The
disabledandremounts a parked pane...suites each test parked panes in isolation. None test a parked candidate alongside several non-parked eligible candidates under a tightwarmBudget, which is the scenario that would catch the ranking bug flagged inpane-mount-policy.ts(parked panes consuming warm-budget rank slots).tests/e2e/terminal-hidden-pane-evict-restore.spec.ts (1)
116-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "evict tabs + confirm unmount" pattern.
The same block — loop over remaining tabs to push one out of the warm budget, then poll
mountedTabIdsfor the evicted tab's absence — is duplicated across four tests. Extracting a small helper would reduce duplication and centralize the eviction-wait timeout/margin logic.♻️ Suggested helper
+async function evictTab(page: Page, allTabIds: string[], targetTab: string): Promise<void> { + for (const tabId of allTabIds.filter((id) => id !== targetTab)) { + await activateTab(page, tabId) + } + await expect + .poll(async () => (await mountedTabIds(page)).includes(targetTab), { timeout: 15_000 }) + .toBe(false) +}Then each call site becomes
await evictTab(page, tabIds, firstTab).Also applies to: 156-162, 231-237, 256-262
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 95f1ba0e-6b20-4659-93cb-8ad220e82103
📒 Files selected for processing (40)
src/main/ipc/pty.test.tssrc/main/ipc/pty.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/Terminal.tsxsrc/renderer/src/components/settings/ExperimentalPane.tsxsrc/renderer/src/components/settings/TerminalPaneEvictionExperimentalSetting.tsxsrc/renderer/src/components/settings/experimental-search.tssrc/renderer/src/components/settings/terminal-pane-eviction-experimental-search-entry.tssrc/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsxsrc/renderer/src/components/terminal-pane/evicted-pane-registry.test.tssrc/renderer/src/components/terminal-pane/evicted-pane-registry.tssrc/renderer/src/components/terminal-pane/pane-mount-policy.test.tssrc/renderer/src/components/terminal-pane/pane-mount-policy.tssrc/renderer/src/components/terminal-pane/pane-unmount-action.test.tssrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/pty-transport.test.tssrc/renderer/src/components/terminal-pane/pty-transport.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-breadcrumbs.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-inputs.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/renderer/src/components/terminal/split-group-mount.test.tssrc/renderer/src/store/index.tssrc/renderer/src/store/slices/diffComments.test.tssrc/renderer/src/store/slices/store-test-helpers.tssrc/renderer/src/store/slices/terminal-pane-eviction.tssrc/renderer/src/store/types.tssrc/shared/constants.tssrc/shared/pty-main-buffer-snapshot.tssrc/shared/terminal-pane-eviction-settings.test.tssrc/shared/terminal-pane-eviction-settings.tssrc/shared/types.tstests/e2e/terminal-hidden-pane-evict-restore.spec.tstests/e2e/terminal-hidden-tui-visual-restore.spec.ts
…-pane-unmount # Conflicts: # src/renderer/src/components/terminal-pane/pty-connection.ts
…cInTerminal Co-authored-by: Orca <help@stably.ai>
…ted, harden park claim/title (review round 1) - Warm set never engaged through the real UI: a visible tab was absent from terminalPaneMountByTabId, so it unmounted in the same commit it flipped hidden and never reported the hide — the managed/warm/park pipeline was unreachable. Include visible tabs in the mount map (and prune stale visible ids on close). - Activity-page terminal portals: an aged-out worktree opened via Activity never re-entered the mounted-worktree set, so its overlay layer (and the portaled pane) never rendered. Add portal worktrees to the flag-ON mounted set. - park() claim race: the parked-entry release happened at bind but adoption is deferred to a rAF; a dispose before attach orphaned the live PTY (gate #8). Defer the release to bindActivePanePty (the adoption chokepoint). - Parked split-tab titles: the parked onTitleChange drove the tab title unconditionally and skipped synthetic-title suppression; mirror the live handler's active-pane + Codex-synthetic guards. Regression tests added for the warm-set transition and the claim-before-adopt race. Full terminal unit suites + typecheck green. Co-authored-by: Orca <help@stably.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/pty-connection.test.ts (1)
7969-7993: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegistry reset is skipped if an assertion throws.
__resetEvictedPaneRegistryForTest()at Line 7993 only runs if bothexpectcalls on Lines 7991-7992 pass. If either assertion fails, the parked entry and self-disable counters fromregisterEvictedPaneremain in the module-scoped registry and can pollute later tests in this suite.♻️ Wrap in try/finally
- binding.dispose() - expect(isPaneParked(paneKey)).toBe(true) - expect(releaseForClaim).not.toHaveBeenCalled() - __resetEvictedPaneRegistryForTest() + try { + binding.dispose() + expect(isPaneParked(paneKey)).toBe(true) + expect(releaseForClaim).not.toHaveBeenCalled() + } finally { + __resetEvictedPaneRegistryForTest() + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8fec028e-2a03-4938-ab67-ed5d4839ca35
📒 Files selected for processing (5)
src/renderer/src/components/Terminal.tsxsrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/src/components/Terminal.tsx
- src/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.test.ts
- src/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.ts
- src/renderer/src/components/terminal-pane/pty-connection.ts
…lose remote parked stream (review round 2)
Round-2 review found the round-1 deferred-claim fix was incomplete and the
remote park path leaks:
- Deferred parked-claim release was wired only into bindActivePanePty, but
daemon/SSH remounts adopt the PTY inline in handleReattachResult (never
calling bindActivePanePty). The claim then never released: the stale parked
feed kept double-driving the store and a later re-park's registry eviction
would destroy() the LIVE daemon PTY. Extract finalizeEvictionClaim() and call
it from BOTH adoption points.
- Remote releaseForClaim used park({}), which for a remote transport keeps the
per-instance multiplexed host stream open — leaking a live subscription +
parser on every remote evict/remount. Route remote release through detach()
(closes only this instance's stream, safe for the freshly-subscribed pane);
local must stay park({}) (its ptyId-keyed handler is shared and already
overwritten by the fresh attach; detach() would sever the live pane).
- Runtime disable ran an O(managed) prune on every store tick while OFF; drop
it (warm map is ignored while disabled), keep only the parked close-reconcile.
- Document the accepted Tier-2 gap: OSC-133 command lifecycle is not retained
while parked (agent completion/title/status/exit ARE).
Tests added: successful-adoption claim release; remote detach() closes the
parked stream. Full terminal unit suites + typecheck green.
Co-authored-by: Orca <help@stably.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b79fd898-c7ce-42e4-9c9c-bc1af23c8748
📒 Files selected for processing (4)
src/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/components/terminal-pane/terminal-pane-eviction-coordinator.ts
- src/renderer/src/components/terminal-pane/pty-connection.ts
…b reporter (P0-1/P0-2)
The visible->hidden report lived inside the mount-gated overlay slot, which
unmounts the instant a pane goes hidden-and-not-yet-warm. So the hide was never
reported, the warm map stayed {} forever, nothing parked, every switch-back
replayed, and a hidden tab that was closed leaked its live PTY (unmounted =>
no destroy, not parked => no close-reconcile).
Move the report to a persistent TerminalPaneVisibilityReporter that the overlay
layer renders for every terminal tab regardless of the gate, so the transition
is always reported before/without unmounting. It is edge-honest (never-visited
tabs do not report hidden, preserving the mount-storm fix) and flag-OFF inert.
With warming restored, a closing hidden tab always reaches destroy() (warm) or
close-reconcile (parked); resolvePaneUnmountAction already destroys a tab closed
mid-eviction, so no orphan park.
Tests: new overlay integration test drives the real render+gate+coordinator seam
(the unit tests bypassed it by calling noteTerminalPaneVisibility directly);
adds the close-mid-eviction destroy case. Fixes an optional-call typecheck error
in the remote-transport claim-release test.
Co-authored-by: Orca <help@stably.ai>
…tion path (P1-A) Codex claim-takeover review, validated against the current code: P1-A (real): an evicted remount that still owns tab.ptyId adopts via the direct transport.attach()/spawn path (bindActivePanePty), which never reached the snapshot/replay logic that only handleReattachResult ran. A silent evicted pane came back history-blank and the claim outcome never resolved. Fix: after adoption in bindActivePanePty, drive the same markHiddenOutputRestoreNeeded()/ report-nil path. Idempotent (requestHiddenOutputRestoreIfNeeded dedupes in-flight snapshots) and exactly-once (the two adoption paths are mutually exclusive per connect), so it never double-writes the replay. P1-B and P1-C were already resolved in the review round-2 commit and are NOT re-patched: the claim is deferred to adoption (the parked feed's exit observer stays live through the rAF gap and clears tab.ptyId + drops the entry on a gap exit), and releaseForClaim already closes a remote parked stream via detach() (local stays park()). Added the missing PTY-exit-during-claim-gap test to lock that behavior; the direct-attach replay test ships with the P1-A change. Co-authored-by: Orca <help@stably.ai>
… mirror, not the daemon reattach snapshot (STA-1282) Pre-existing replay-tail/live-head overlap exposed at high frequency by eviction. An evicted daemon-backed pane remounts via a deferred daemon reattach whose snapshot is a bare string with no sequence; its tail overlaps the first live chunk and the renderer has no baseline to trim it, so the evict->remount boundary line duplicates (load-dependent: only when the main-side buffer races ahead of pty:data delivery). The daemon TerminalSnapshot carries no byte offset, and the daemon/runtime byte counters diverge across app relaunch, so a threaded snapshotSeq could not be proven seq-aligned. Instead, route claimed eviction remounts through the local runtime mirror (getMainBufferSnapshot), whose snapshot seq shares the pty:data meta.seq domain — drainPendingLiveChunksAfterSnapshot already trims the overlap there. Consumption gated to claimed remounts (evictionRemountReplayPending); flag-off / non-eviction reattach paint is byte-identical. coldRestore and remote relay-replay keep their paint. Tests: renderer test asserting a claimed remount with connectResult.snapshot paints the mirror and never the sequence-less daemon snapshot; runtime contract test pinning snapshot.seq == getPtyOutputSequence == pty:data seq. Validated: eviction e2e contiguity passes across repeated full-file runs (previously duplicated the boundary line 2/4).
- Exclude parked panes from the warm-budget ranking pool: they are always classified evict, so counting them silently stole warm slots from live hidden panes (+ regression test) - Invoke requestIdleCallback as a method — an extracted unbound reference throws Illegal invocation in Chromium - Correct the settings hydration docstring (feature defaults OFF) Co-authored-by: Orca <help@stably.ai>
…A-1372) Co-authored-by: Orca <help@stably.ai>
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.
Implements Linear STA-1282 (terminal panes not unmounted when switching away — root cause of the 10s workspace in STA-1280).
What this changes
Opt-in experimental feature (
Settings → Experimental → Terminal pane eviction, default OFF): terminal panes that have been hidden for 5+ minutes (beyond the 12 most-recently-viewed, which always stay live) are unmounted from the renderer — their DOM, xterm instance, listeners, and heap are released — and rebuilt from Orca's always-current main-process terminal mirror when you next view them. The shell/agent process is never touched. With the flag off (everyone, by default), behavior is byte-identical to today — verified by dedicated inertness tests.Fixes the field report behind STA-1280/STA-1282: a multi-day multi-agent workspace accumulated 350k DOM nodes, 122k listeners, and ~1GB renderer heap, making workspace open/tab switch take ~10 seconds.
ELI5
Orca currently keeps every terminal you've ever opened fully alive in the window, even ones you haven't looked at in days — they just get hidden. After days of multi-agent work that's hundreds of invisible live terminals, and the app slows to a crawl. With this feature turned on, Orca puts terminals to sleep if you haven't looked at them for 5 minutes, and redraws them from its saved copy the moment you click back — like minimizing apps you're not using instead of keeping every window open forever. Your 12 most recently used terminals never sleep, so switching between the things you're actually working on stays instant. The running programs inside the terminals are never stopped — only the picture of them is put away and redrawn. It's off by default; flip it on in Settings → Experimental if your workspaces get slow.
Risk assessment
Who is exposed: nobody, until they opt in. The flag-off path is pinned byte-identical to today by dedicated tests (mount-everything branch, zero new subscriptions/timers). The single highest-severity residual risk in this PR is a gating bug that leaks behavior to flag-off users — which is why flag-off inertness has its own oracle rather than being assumed.
The core risk (rated 9/10 pre-mitigation): restore fidelity. The buffer-replay path previously ran roughly once per app launch (restart restore); for opt-in users it now runs on every re-show of an evicted pane — a frequency change, not a new mechanism. What failure looks like, concretely, in worst-to-likely order:
What failure structurally cannot look like: a killed agent/shell (eviction can only take PTY-alive paths; the kill branch is unreachable and outcome-tested), a dead/frozen pane (replay failure degrades to live-blank), or any effect on users who haven't opted in.
Why we believe the replay risk is manageable: the replay source is the main-process mirror that is fed every byte continuously and never disconnects — fresher than the restart path that already ships; the streaming race is closed by reusing the existing sequence-numbered snapshot/drain machinery (the #7173 fix class), with a dedicated e2e that streams monotonically-numbered lines through the entire evict→re-show cycle and asserts contiguity; alt-screen state (frame, cursor, paste/mouse modes) restores via the same rehydrate sequences used by restart restore; geometry reconverges through the existing spawn-time reconcile (different-size re-show → natural SIGWINCH repaint; 0×0 remains impossible).
Containment if fidelity problems escape anyway: per-session 3-strike self-disable (structural replay failures only — an empty terminal is not a failure), a plain kill switch, breadcrumb logs on every evict/re-show/failure so field reports are attributable, and an architectural fallback: the mount-gating/policy/registry machinery is shared with a lower-risk "park the live instance" variant, so if opt-in field evidence demands it, Tier 2 can be downgraded without redoing the feature — trading back the memory win, keeping the speed win.
Known limitation while the flag is ON (documented, accepted): tabs never visited this session don't mount at all, so tab titles and title-based (hookless) agent status for those tabs appear on first visit instead of live. Claude/Codex status is main-side and unaffected. Follow-up candidate: a lightweight parked parser for never-visited live PTYs.
Accepted validation gaps (explicit, not hidden): live Windows/ConPTY pass pending (a machine is available; unit coverage exists for resize-on-remount and the replay path is platform-neutral renderer code); the remote-runtime park path (stream close on claim, host-snapshot channel) is unit-tested but was not driven against a live relay; the exit-during-claim-gap race is locked by a unit test rather than manual reproduction (it is a single-frame window); measured perf-budget latency oracles partially manual (bounded-count oracles automated; live DOM/mount measurements recorded in validation).
Secondary risks: parked-transport leak on close (gate #8: closing an evicted tab kills its PTY promptly — tested); teardown racing a re-show (gate #9: generation-keyed cancel — tested); status/title staleness while evicted (gate #3: parked parser retention — tested); coordinator cost when idle (event-driven, no polling; the only timer is armed when a pane is actually due to age out).
Process summary (Brennan's PR process)
lastVisibleAtin coordinator module state (nothing needs it reactively); registry keyed per-pane leaf.Made with Orca 🐋