Repository navigation
diag(crash): make renderer OOM profiles actionable - #10626
AmethystLiang wants to merge 10 commits into
Conversation
|
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 changes configure precise renderer memory behavior through shared guest preferences and a startup Chromium switch. Crash diagnostics now use four high-water thresholds, retain up to eight entries, and clear renderer-specific retained breadcrumbs between crashes. Renderer profiling adds PTY side-effect gauges, terminal output queue peak and backlog metrics, pane terminal lifecycle counters, and aggregated pane buffer and connection metrics. Associated tests cover configuration, retention, lifecycle cleanup, queue behavior, and profiling output. 🚥 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: 1
🧹 Nitpick comments (3)
src/main/browser/offscreen-browser-backend.web-preferences.test.ts (1)
27-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRetain the offscreen-backend wiring assertion.
toMatchObjectonly validates the imported constant; it does not prove thatoffscreen-browser-backend.tsstill appliesORCA_BROWSER_GUEST_WEB_PREFERENCES. Keep the source/runtime integration assertion and add this value check rather than replacing it, so a regression could not silently re-enable precise memory reporting for offscreen guests.src/renderer/src/components/terminal-pane/pty-transport.ts (1)
912-916: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGauge disposal isn't guaranteed on every teardown path, and the registry has no defensive cap.
disposePendingSideEffectGauge()is only invoked fromdetach()/destroy()inpty-transport.ts; the normal PTY-exit handler doesn't call it, and if that's a path by which a transport is discarded without a laterdestroy(), its gauge (and thependingSideEffectsclosure) leaks forever inpty-side-effect-pending-census.ts's uncappedpendingGaugesSet — undermining the very leak diagnostics this feature adds.
src/renderer/src/components/terminal-pane/pty-transport.ts#L912-L916: confirm the pane/lifecycle layer always callsdestroy()after a PTY exit even outside the detach hand-off flow, or calldisposePendingSideEffectGauge()from the exit handler itself as a safety net.src/renderer/src/components/terminal-pane/pty-side-effect-pending-census.ts#L4-L19: consider boundingpendingGauges(mirroringregisterRendererMemoryProfileContributor'sMAX_PROFILE_CONTRIBUTORScap) so a missed disposal degrades gracefully instead of growing unbounded.src/renderer/src/lib/pane-manager/pane-terminal-output-queue-production-profile.test.ts (1)
41-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't prove the instantaneous snapshot fields work in production — only the peaks.
The assertions run after a full drain (
vi.advanceTimersByTime(1000)), soqueuedChars === 0etc. would pass identically whether or notreadQueueDebugSnapshot()'s instantaneous fields (terminals,queuedChars,maxQueuedCharsPerTerminal) actually work outside e2e — a drained queue reports 0 either way. Given this file's stated purpose is proving production visibility of the whole contributor, consider also asserting the non-zero snapshot values immediately afterwriteTerminalOutput(before advancing timers) to confirmreadQueueDebugSnapshot()isn't itself gated behind e2e-only debug state.✅ Suggested additional assertion
writeTerminalOutput(createTerminal(), 'q'.repeat(32 * 1024), { foreground: false, latencySensitive: false }) + expect(collectRendererMemoryProfileCounts()).toEqual( + expect.objectContaining({ + 'terminalOutputQueue.terminals': 1, + 'terminalOutputQueue.queuedChars': 32 * 1024, + 'terminalOutputQueue.maxQueuedCharsPerTerminal': 32 * 1024 + }) + ) vi.advanceTimersByTime(1000)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bf35fb0-7b19-4bc1-9dd1-2b958da8673c
📒 Files selected for processing (28)
src/main/browser/browser-manager.test.tssrc/main/browser/browser-manager.tssrc/main/browser/offscreen-browser-backend.web-preferences.test.tssrc/main/crash-reporting/crash-breadcrumb-store.test.tssrc/main/crash-reporting/crash-breadcrumb-store.tssrc/main/crash-reporting/process-gone-recorder.test.tssrc/main/crash-reporting/process-gone-recorder.tssrc/main/index.tssrc/main/startup/renderer-heap-headroom.test.tssrc/main/startup/renderer-heap-headroom.tssrc/main/window/createMainWindow.test.tssrc/renderer/src/components/browser-pane/BrowserPane.webview-preferences.test.tssrc/renderer/src/components/terminal-pane/pty-side-effect-pending-census.test.tssrc/renderer/src/components/terminal-pane/pty-side-effect-pending-census.tssrc/renderer/src/components/terminal-pane/pty-transport.tssrc/renderer/src/lib/crash-diagnostics.test.tssrc/renderer/src/lib/crash-diagnostics.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.test.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.tssrc/renderer/src/lib/pane-manager/pane-manager-registry.test.tssrc/renderer/src/lib/pane-manager/pane-manager-registry.tssrc/renderer/src/lib/pane-manager/pane-manager.tssrc/renderer/src/lib/pane-manager/pane-terminal-instance-census.test.tssrc/renderer/src/lib/pane-manager/pane-terminal-instance-census.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-queue-production-profile.test.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.tssrc/shared/browser-guest-web-preferences.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 836367eb-c4b4-4f10-9f1d-0eebb74f66fe
📒 Files selected for processing (11)
src/main/crash-reporting/crash-breadcrumb-store.test.tssrc/main/crash-reporting/crash-breadcrumb-store.tssrc/main/crash-reporting/process-gone-recorder.test.tssrc/main/crash-reporting/process-gone-recorder.tssrc/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.test.tssrc/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/lib/crash-diagnostics.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.tssrc/shared/crash-reporting.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/crash-reporting/process-gone-recorder.ts
- src/renderer/src/lib/crash-diagnostics.ts
|
@coderabbitai[bot] addressed |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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/crash-reporting/process-gone-recorder.ts (1)
114-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope retry carryover to the renderer surface.
keyomitsrendererSurface, whilebreadcrumbscontains profiles from every surface. A failed main report can therefore carry a popout profile; if the popout clears before the retry, Lines 125-128 merge that stale profile back. Use a renderer-surface-scoped dedupe/carry key and stash only the dead surface’s profiles.Also applies to: 172-174
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 678f66f6-eb46-4669-a8a9-8b46f98776fa
📒 Files selected for processing (7)
src/main/crash-reporting/crash-breadcrumb-store.test.tssrc/main/crash-reporting/crash-breadcrumb-store.tssrc/main/crash-reporting/process-gone-dedupe.tssrc/main/crash-reporting/process-gone-highwater-carryover.test.tssrc/main/crash-reporting/process-gone-highwater-carryover.tssrc/main/crash-reporting/process-gone-recorder.test.tssrc/main/crash-reporting/process-gone-recorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/crash-reporting/process-gone-recorder.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/crash-reporting/process-gone-recorder.test.ts (1)
427-458: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve the stash remains available to the matching surface.
This passes if failed persistence never stashes carryover. After the popout attempt, add a main-surface retry and assert its persisted breadcrumbs include the
85%highwater; this verifies both stashing and non-consumption by the sibling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 499dd5c9-a8e6-4b59-96a2-ede565ed0e03
📒 Files selected for processing (6)
src/main/crash-reporting/crash-breadcrumb-store.tssrc/main/crash-reporting/process-gone-highwater-carryover.test.tssrc/main/crash-reporting/process-gone-highwater-carryover.tssrc/main/crash-reporting/process-gone-recorder.test.tssrc/main/crash-reporting/process-gone-recorder.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-queue-production-profile.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/crash-reporting/process-gone-highwater-carryover.ts
- src/renderer/src/lib/pane-manager/pane-terminal-output-queue-production-profile.test.ts
- src/main/crash-reporting/process-gone-recorder.ts
- src/main/crash-reporting/crash-breadcrumb-store.ts
|
@coderabbitai[bot] addressed |
|
Tip For best results, initiate chat on the files or code changes.
|
|
@coderabbitai[bot] addressed |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e073c80-8e05-4a9e-868e-87c8b600c55b
📒 Files selected for processing (5)
src/main/crash-reporting/process-gone-highwater-carryover.test.tssrc/main/crash-reporting/process-gone-highwater-carryover.tssrc/main/crash-reporting/process-gone-recorder.test.tssrc/renderer/src/lib/crash-diagnostics.test.tssrc/renderer/src/lib/crash-diagnostics.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/crash-reporting/process-gone-highwater-carryover.ts
- src/main/crash-reporting/process-gone-recorder.test.ts
C1_DIAGNOSIS.md: the field performance.memory samples are Chromium-bucketized (~6% geometric) AND cached 20 minutes, so every published climb rate was a 20-min average; primary root-cause candidate is the mounted-worktree retention policy (activated worktrees never unmount; SSH/remote/null-ptyId worktrees bypass the cold-park cap and TTL entirely). Instrumentation so the next field crash names the accumulator: - enable-precise-memory-info switch (unbuckets + unfreezes the sampler) - highwater ladder [0.6,0.8] -> [0.4,0.6,0.75,0.85] - paneTerminals created/disposed/live census contributor - terminalOutputQueue backlog contributor (production-visible) Co-authored-by: Orca <help@stably.ai>
…r ladder The 4-level ladder ([0.4, 0.6, 0.75, 0.85]) needs 4 thresholds x 2 renderer surfaces of retained keys, but MAX_RETAINED_BREADCRUMBS stayed at 4 (sized for the old 2-level ladder). Retained highwater breadcrumbs never fall back into the 30-entry ring, so eviction silently dropped the low-threshold baseline the ladder was widened to produce. Also: - pin the census create/dispose call sites and the terminalOutputQueue contributor with tests; only the census counters were covered - correct the diagnosis doc's H2 label on the pane-terminal census (it tests H1) and the census docstring's detached-terminal claim - note that the precise-memory switch is process-global and reaches browser-pane webview guests (behavior unchanged) - move the diagnosis doc under docs/ next to its sibling profile writeup and drop the machine-specific corpus path
pendingSideEffects is the one uncapped hot-path queue (64/tick drain vs a background-throttled ~1 Hz timer; pause cancels the drain without clearing) — the C1 H2 suspect the existing contributors cannot see. Each output processor registers a pending-count gauge; transports dispose it on detach()/destroy(). Co-authored-by: Orca <help@stably.ai>
…uced them Two ways the C1 instrumentation could report the wrong thing: Retained high-water profiles are keyed by surface+threshold with no renderer generation, and nothing dropped them when a renderer died. Because a reload resets the renderer-side one-shot guard while the main-process store survives, a fresh renderer that died before re-crossing its ladder inherited the dead one's ~3 GB profiles — the "OOM kill at a tiny heap" misreading the diagnosis attributes to the existing corpus. Clear them at process-gone for renderers. terminalOutputQueue reported instantaneous gauges only, sampled at most once per threshold on a 60 s cadence, so a backlog that spiked and drained in between read as zero — indistinguishable from "never grew". Add O(1) high-water marks on the enqueue path and an ungated dropped-backlog counter; the peaks are the sampling-immune half of the H3 signal, and the drop count is the strongest evidence the lossy cap fired at all. Both regressions are covered by revert-proven tests, including a non-e2e suite since the sibling scheduler tests mock exposeStore=true and cannot see the production path.
- remote PTY transport disposes its pending side-effect gauge on detach and destroy, matching the IPC transport; parked byte watchers dispose theirs too - process-gone clears retained high-water profiles on any renderer generation end, including suppressed reloads and non-crash exits, not just recorded ones - a failed persist re-seeds the cleared profiles so the retry's durable report still carries the OOM ladder - the clear is scoped to the dead surface so a live dashboard popout keeps its own profiles (it never re-emits them)
The re-seed put cleared profiles back into the live store, so a retry that never arrived left the next renderer's crash report carrying a dead generation's ladder. Stash them against the dedupe key with the dedupe window as TTL instead, and merge inside the 30-breadcrumb report budget.
The retry carryover key omits the surface, so sibling surfaces can share it. Without scoping, a popout dying inside the window would inherit the main renderer's stashed heap ladder. Carry only profiles belonging to the dying surface (and unattributed ones that have no owner).
Dedupe key omits surface, so sibling surfaces (main/dashboard-popout) dying the same way would share stash. Scope the carryover key by surface so each surface owns its breadcrumbs independently; a sibling's failed persist no longer evicts the other's profiles.
Early memory warnings at 40–60% heap now skip expensive DOM walks and subsystem profiling, emitting only lightweight heap metrics. Detailed diagnostics with DOM counts and store snapshots kick in only at 75%+ to reduce overhead on healthy sessions while preserving diagnostic value when approaching OOM.
b04a8f3 to
219d85a
Compare
|
too large. used a smaller PR #12795 |
1 participant
Summary
This PR makes the next renderer out-of-memory report identify which terminal subsystem is growing.
It does not claim to fix the underlying leak. It replaces stale/coarse memory sampling with precise trusted-renderer measurements, records four staged high-water profiles, and adds production diagnostics for terminal membership, xterm buffer weight, renderer output backlog, and pending PTY side effects.
Why the previous data was misleading
Electron 43.1.0 embeds Chromium 150. In that build, Chromium's
MemoryInfo::GetHeapSizereports V8used_heap_size()plusexternal_memory(). Thereforeperformance.memory.usedJSHeapSizeis not a pure JavaScript-object heap measurement. xterm'sUint32Arrayrow backing stores contribute through V8 external memory and remain a plausible source of the observed growth.Without
--enable-precise-memory-info, Chromium also quantizesperformance.memoryinto geometric buckets and caches it for about 20 minutes. That means the field traces could establish sustained growth, but could not distinguish a boot burst, smooth leak, short GC sawtooth, or exact climb rate within each cache window.Aggregate review of 57 crash bundles found:
The leading hypothesis remains retained, streaming xterm buffers in mounted worktrees, but it is provisional. Other candidates are the uncapped PTY side-effect queue, renderer output/backpressure, and large passive editor/diff text working sets.
What changed
performance.memoryreadings before trusted Orca renderer processes start.PreciseMemoryInfoin every browser guest path:<webview>contents, including attach-time hardening;created - disposed = livearithmetic with truthful terminal metrics:registered,removed, anddisposeErrors;trackedmembership;bufferRows,scrollbackRows, andestimatedBufferCells.ptySideEffects.processorsandptySideEffects.pendinggauges, disposed with their owning transports.How to read the next profile
paneTerminals.trackedandestimatedBufferCellswith memory supports mounted-terminal retention.ptySideEffects.pendingwith flat buffer weight supports the side-effect queue hypothesis.Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm test— 3,411 files passed; 36,229 tests passed; 62 skippedpnpm build— not run; typecheck and the complete unit/integration suite passedAI Review Report
The review checked the measurement model, reload lifecycle, browser security boundary, terminal census semantics, breadcrumb retention, public-data disclosure, formatting, and comments.
It flagged and this PR addresses:
usedJSHeapSizewas incorrectly described as excluding typed-array backing stores.design-docs/rather than included in the public diff.Cross-platform review covered macOS, Linux, and Windows. This PR changes no shortcuts, labels, shell commands, or path handling. Electron-specific behavior is shared across platforms and tested at each guest-creation seam. The diagnostics read existing renderer state and apply equally to local, SSH, remote-runtime, git-worktree, and folder workspaces.
Security Audit
The main security risk was Chromium's process-global
enable-precise-memory-infoswitch, which can expose allocation and GC timing to untrusted pages. The shared guest policy explicitly disables BlinkPreciseMemoryInfofor webviews, offscreen windows, and popup contents; popup descendants inherit the same hardened creation policy. Trusted Orca UI surfaces keep precise diagnostics.The new crash fields contain bounded numeric counts only—no terminal output, paths, URLs, profile names, tokens, or buffer contents. Contributor counts remain subject to the existing crash-profile budgets. No dependency, authentication, command-execution, or new IPC surface is introduced.
Notes
This is diagnosis instrumentation, not the final leak fix. The next precise field cycle should determine whether to implement a memory-weighted mounted-terminal budget, cap/collapse pending side effects, backport remote delivery-credit behavior, or profile large retained strings.