perf(serve): bound memory for headless orca serve - #13684
IamCoder18 wants to merge 3 commits into
Conversation
orca serve kept the desktop's per-window memory footprint even though it has no renderer. Per-PTY HeadlessEmulator scrollback alone cost ~1 MB per PTY; long-lived sessions leaked Maps (gitUsernameCache, plugin init, non-local host partitions, client session tabs) without bound. Measured in-process: 50 PTYs at scrollback=5000 = 51 MB RSS vs scrollback=1000 = 6 MB RSS (~88% reduction per PTY). - QW-1: skip enableRendererHeapHeadroom in serve (no renderer V8 isolate) - QW-2: skip enableMainProcessGpuFeatures in serve (renderer-only flags) - QW-3: lazy-init Plugin services (and kill-list/marketplace/installer/ bundled-bootstrap) when pluginSystemEnabled is off in serve mode - QW-4: skip ensureMainI18n + setMainUiLanguage at boot in serve (translateMain falls back to English when not initialized) - QW-7: skip StarNagService constructor + listeners in serve (no renderer to nag, IPC handlers unreachable) - ME-1: bound ClientSessionTabSelectionStore.statesByClient with a BoundedMap LRU keyed by lastAccessedAt (cap 256) - ME-2: clearGitReadCachesForPaths prunes submodulePathsCache, resolvedUpstreamNameCache, effectiveUpstreamStatusCache by path prefix; Store.removeProject calls it for the removed repo's paths - ME-3: swap gitUsernameCache from Map to BoundedMap (cap 5000) - ME-4: StoreOptions.dropNonLocalHostWorkspaceSessions drops non-local workspaceSessionsByHostId partitions after load (serve needs only local) - ME-5: HeadlessEmulatorOptions.defaultScrollback + runtime dep; serve sets 1000 instead of the 5000 desktop default - ME-7: new BoundedMap<K,V> primitive (insertion-order or LRU eviction, Map-iterator protocol)
📝 WalkthroughWalkthroughThe change bounds Git username and client-state caches and clears repository-related Git read caches during removal. It adds configurable headless emulator scrollback with explicit-option precedence and a serve-mode limit. Serve mode now skips renderer heap and GPU setup, filters non-local workspace sessions, avoids unnecessary plugin, Star Nag, and i18n initialization, and includes renderer heap guard coverage. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 7
🧹 Nitpick comments (2)
src/main/memory/bounded-map.ts (1)
101-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid a full temporary entry array during eviction.
Line 104 allocates an array of every cached entry for each insertion after the cap. Scan the
Mapiterator directly. This keeps ranked eviction at O(n) time but reduces temporary memory from O(n) to O(1).Proposed refactor
- // Why: Map iteration is insertion-ordered, so for the default rank (always 0) the first - // key is the oldest insertion. For LRU, the rank comparison only makes sense against a - // second entry — compare each candidate against a fixed reference and keep the minimum. - const all = [...this.entries_] - if (all.length === 0) { + // Map iteration preserves insertion order, so rank ties keep the oldest entry. + const entries = this.entries_.entries() + const first = entries.next() + if (first.done) { return } - let oldestKey: K = all[0][0] - for (let i = 1; i < all.length; i++) { - const candidate = all[i] - if (this.evictionRank(candidate, [oldestKey, this.entries_.get(oldestKey)!]) < 0) { - oldestKey = candidate[0] + let oldest = first.value + for (const candidate of entries) { + if (this.evictionRank(candidate, oldest) < 0) { + oldest = candidate } } - this.entries_.delete(oldestKey) + this.entries_.delete(oldest[0])src/main/index.ts (1)
2628-2630: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse direct
storeaccess in the plugin block.storeis initialized before this block and is not reset tonull, sostore?.getSettings()andstore!are redundant. Plugin RPC methods intentionally reject requests when serve mode skips initialization; they do not throw a null-dereferenceTypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed1b98be-4d76-438b-86dd-4037921539bf
📒 Files selected for processing (13)
src/main/daemon/headless-emulator.test.tssrc/main/daemon/headless-emulator.tssrc/main/git/status-clear-caches-for-paths.test.tssrc/main/git/status.tssrc/main/index.tssrc/main/memory/bounded-map.test.tssrc/main/memory/bounded-map.tssrc/main/persistence.test.tssrc/main/persistence.tssrc/main/runtime/client-session-tab-selection.tssrc/main/runtime/orca-runtime.tssrc/main/startup/renderer-heap-headroom.test.tssrc/main/startup/renderer-heap-headroom.ts
- Replace the redundant main/memory/BoundedMap with the shared src/shared/bounded-map.ts. The local class collided with the existing one in dashboard-payload-validation.ts, which uses maxBytes + sizeOf for byte-based eviction. The shared class supports maxEntries, LRU via get() reorder, byte caps, and onEvict. - Revert dropNonLocalHostWorkspaceSessions. The previous implementation mutated state and scheduled a save, which would overwrite a shared desktop+serve profile with the filtered partition map on the next write. Removed StoreOptions.dropNonLocalHostWorkspaceSessions, constructor branch, index wiring, and the test. - Apply clearGitReadCachesForPaths + gitUsernameCache.delete in removeProjectForHost when the repo id is fully gone (matching removeProject). Captures paths from the matching repo before the filter runs; leaves the cache alone when another host still holds the repo id. - HeadlessEmulatorOptions.defaultScrollback: replace the shallow 'construction succeeds' tests with three that assert actual constructor precedence (defaultScrollback used, explicit scrollback wins, module default applies when neither is set). Tests read the effective scrollback from the underlying xterm options to avoid adding a public getter that would push the file past the 300-line oxlint max-lines budget. - git/status.ts: add primeGitReadCachesForTests() so status-clear- caches-for-paths.test.ts can populate the module-level caches and assert the matching-eviction contract instead of only the empty- cache no-op path. Verified locally: pnpm tc:node clean, oxlint max-lines at exactly 300 non-blank/non-comment lines, 7,948 tests across persistence, git, runtime, daemon, startup, plugins, i18n, and star-nag.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/git/status.ts (1)
127-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a path boundary when clearing Git read caches.
key.startsWith(path)also matches sibling paths. Removing/worktrees/repo-acan remove cache entries for/worktrees/repo-ab. Extract the path before\0, then match the exact path or a descendant path with a separator. Add a sibling-prefix regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aec79c1-fe3d-404f-9340-eea170106a02
📒 Files selected for processing (6)
src/main/daemon/headless-emulator.test.tssrc/main/git/status-clear-caches-for-paths.test.tssrc/main/git/status.tssrc/main/index.tssrc/main/persistence.tssrc/main/runtime/client-session-tab-selection.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/daemon/headless-emulator.test.ts
- src/main/index.ts
- src/main/persistence.ts
CodeRabbit review caught a sibling-path false positive: clearing
/worktrees/repo-a would also clear cache entries for /worktrees/repo-ab
because the matcher used startsWith on the raw cache key. The cache
key format is [path, wslDistro].join('\0'), so the path is the
substring before the first NUL.
The matcher now extracts the path portion and accepts either an exact
match or a descendant with a '/' or '\' separator boundary. Siblings
with a shared prefix are preserved; descendants are still cleared.
Regression test added in status-clear-caches-for-paths.test.ts that
primes /worktrees/repo-a, /worktrees/repo-ab, and /worktrees/repo-a/sub
then asserts clearing /worktrees/repo-a leaves exactly one entry
(repo-ab) behind.
|
Ready to review! |
|
Heads-up on overlap: #13061 gives every daemon terminal session an explicit flat 1000-row scrollback window at creation (set via |
This branch has not been deployed
5 participants
Summary
Headless
orca serveinherits the desktop's per-window memory footprint. This caps unbounded long-lived Maps and skips work the serve path does not need.Per-PTY memory (bench, N=50,
--expose-gc)88% reduction per PTY. For 50 concurrent agents: ~45 MB saved. For 100: ~90 MB.
Cold-boot RSS at 20s (no PTYs, no clients — framework overhead only)
Most optimizations are workload-dependent and don't show until clients/repos/PTYs exist. The per-PTY win above dominates under agent-session load.
Changes
--max-old-space-sizein serve (no renderer V8)src/main/startup/renderer-heap-headroom.ts:83enableMainProcessGpuFeaturesin serve (renderer-only flags)src/main/index.ts:828pluginSystemEnabledis off in servesrc/main/index.ts:2618-2747ensureMainI18n+setMainUiLanguagein serve (translateMain falls back to English)src/main/index.ts:2822new StarNagService+ listeners in serve (no renderer to nag)src/main/index.ts:2749clientSessionTabSelections.statesByClientviasrc/shared/bounded-map.tsLRU (cap 256,get()reorder)src/main/runtime/client-session-tab-selection.ts:131clearGitReadCachesForPaths(paths)prunessubmodulePathsCache/resolvedUpstreamNameCache/effectiveUpstreamStatusCacheby path boundary (exact OR descendant with//\separator — siblings are preserved);Store.removeProject+Store.removeProjectForHostcapture repo paths before filter and call itsrc/main/git/status.ts:122,src/main/persistence.ts:4729,4749Store.gitUsernameCacheswapped from plainMaptoBoundedMap({ maxEntries: 5000 })src/main/persistence.ts:2813HeadlessEmulatorOptions.defaultScrollback; runtime readsdeps.headlessEmulatorScrollback; serve sets 1000 instead of 5000src/main/daemon/headless-emulator.ts:51,src/main/runtime/orca-runtime.ts:3168,11356,src/main/index.ts:2460Reuse:
src/shared/bounded-map.ts(already supportsmaxEntries,maxBytes,sizeOf,onEvict, LRU viaget()reorder) is the onlyBoundedMapin the repo.Skipped: QW-5 (churn probe already env-gated), ME-4 (shared-profile data-loss risk — would have overwritten non-local partitions on the next
scheduleSave()), ME-6 (StatsCollector already bounded and serve needs stats for analytics), mobileSessionTabsByWorktree + forgetWorktree orphan sweeps (already wired).Screenshots
No visual change. Renderer is not loaded by
orca serve.Testing
pnpm lint—audit:code-quality:native+audit:code-quality:type-awareclean.pnpm typecheck—pnpm tc:nodeclean.pnpm test— 7,463 passed acrosssrc/main/git,persistence,runtime,daemon. Full suite: 49,182 passed, 7 pre-existing failures (verified onupstream/main@c0c893d171viagit stash --include-untracked):src/relay/git-handler,src/relay/pty-shell-launch × 3,src/main/providers/local-pty-shell-ready × 2,src/main/ssh/ssh-remote-commands. None touched by this PR.pnpm build—pnpm build:cliclean.src/main/git/status-clear-caches-for-paths.test.ts— 6 tests including sibling-prefix regression (/worktrees/repo-avs/worktrees/repo-abvs/worktrees/repo-a/sub).src/main/daemon/headless-emulator.test.ts— 3 tests asserting constructor scrollback precedence (default used, explicit wins, module default).src/main/persistence.test.ts—clearGitReadCachesForPathsintegration viaremoveProject.src/main/startup/renderer-heap-headroom.test.ts— serve-mode skip.src/main/runtime/client-session-tab-selection.test.ts— bounded-map LRU semantics.Cross-platform compatibility (macOS, Linux, Windows)
isServeModeflag; no new platform branches.enableMainProcessGpuFeatures's macOSdisable-skia-graphiteswitch and Linux Wayland/X11 branches remain reachable for desktop — only the call site is gated.clearGitReadCachesForPathspath boundary check handles both/and\separators.SSH / remote / local
Storeregardless of execution host.Git provider / git binary compatibility
clearGitReadCachesForPathsis path-prefix-based and provider-neutral.Security
isServeMode,pluginSystemEnabled,defaultScrollback).defaultScrollbackis passed to xterm.js's documentedscrollbackoption.Notes
--no-offscreen-browser) lands QW-6 (the only item from the original analysis already in flight). This PR covers QW-1..QW-8 and ME-1..ME-7 and is independent of feat(serve): add --no-offscreen-browser flag #13434.store.onSettingsChangedpath. Documented inline at the QW-3 gate.pnpm vitest run --config config/vitest.config.ts src/main/daemon/headless-emulator-memory.bench.test.ts --reporter=verbose. Vitest workers run with--expose-gc.