Repository navigation
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:
📝 WalkthroughWalkthroughAdds Windows renderer sandbox crash classification and threshold tracking for launch-time 🚥 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc2d6451-b191-4e41-a911-583f06f80f08
📒 Files selected for processing (12)
src/main/app-relaunch.tssrc/main/crash-reporting/renderer-sandbox-crash-decision.test.tssrc/main/crash-reporting/renderer-sandbox-crash-decision.tssrc/main/index.tssrc/main/startup/renderer-sandbox-fallback-marker.test.tssrc/main/startup/renderer-sandbox-fallback-marker.tssrc/main/window/createMainWindow.test.tssrc/main/window/createMainWindow.tssrc/main/window/dashboard-popout-window.test.tssrc/main/window/dashboard-popout-window.tssrc/main/window/renderer-sandbox-fallback-state.test.tssrc/main/window/renderer-sandbox-fallback-state.ts
… via build-scoped sandbox fallback (#9891) On Windows 11 build 26200 the sandboxed renderer breakpoints (STATUS_BREAKPOINT / 0x80000003, exit -2147483645) ~2s after launch on nearly every start. The existing GPU-child-crash fallback only disables hardware acceleration and cannot fix a renderer *sandbox* fault, so the renderer-recovery circuit breaker reloads the same crashing renderer 3x and dead-ends at "Orca keeps failing to load". Add a build-scoped renderer sandbox fallback mirroring the GPU fallback marker system: after 3 launch-window renderer STATUS_BREAKPOINT crashes on win32, persist a marker and relaunch with the top-level renderer created unsandboxed (webPreferences.sandbox:false) - the surgical, per-window equivalent of the reporter-confirmed --no-sandbox, keeping GPU/utility/network and webview-guest sandboxes intact. The marker is scoped to appVersion+electronVersion and auto-clears on any update, so a fixed Electron build regains the sandbox automatically. Detection keys strictly on exit code -2147483645 so renderer OOM (#9872, -36861) and other crashes never trigger the downgrade; the dashboard pop-out honors the same flag so it does not breakpoint either. Worst case (sandbox:false still crashes) degrades to today's dialog - never an infinite relaunch loop. The fallback depends on per-window sandbox config: it is documented and guarded (a durable renderer_sandbox_fallback_neutralized breadcrumb) that adding a global app.enableSandbox() would ignore the per-window opt-out and neutralize the fix.
b4c34c5 to
cb489a0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/window/renderer-sandbox-fallback-state.ts (1)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense this security comment.
The warning is useful, but this nine-line block is more detailed than necessary. Keep the rationale and critical
app.enableSandbox()warning in one concise comment.Proposed simplification
-// IMPORTANT (security-relevant): the fallback works by creating those windows -// with webPreferences.sandbox:false, which Electron only honors PER-WINDOW when -// the app does NOT call app.enableSandbox(). Orca deliberately relies on -// per-window sandbox config so the default stays sandboxed and only the affected -// build's main/pop-out renderers opt out - webview guests and every other window -// remain sandboxed. Adding a global app.enableSandbox() would ignore the -// per-window opt-out entirely and silently neutralize this fallback (`#9891` -// machines would crash-loop again); don't, without a replacement. +// Why: keep the per-window `#9891` sandbox fallback effective; do not add +// app.enableSandbox(), which overrides the sandbox:false opt-out.As per coding guidelines, comments in
**/*.{ts,tsx,js,jsx}should be concise, ideally one line, explaining why rather than how.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1550b9c9-721a-4b63-a230-52f68a4ddbf8
📥 Commits
Reviewing files that changed from the base of the PR and between aecb5d2 and b4c34c5d43e80b69b6625c049320b47897935b11.
📒 Files selected for processing (3)
src/main/index.tssrc/main/window/createMainWindow.tssrc/main/window/renderer-sandbox-fallback-state.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/window/createMainWindow.ts
- src/main/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/startup/renderer-sandbox-fallback-marker.ts (1)
52-75: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject semantically invalid marker values before activating the fallback.
Number.isFinitestill accepts negative/fractional timestamps and crash counts, plus unrelated exit codes. A matching corrupted marker can therefore disable the renderer sandbox without representing a qualifying crash. Validate safe-integer/range constraints and reuse the crash tracker’s qualifying exit-code/threshold contract here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20aff40f-950c-448b-b339-90b1d7860783
📥 Commits
Reviewing files that changed from the base of the PR and between cb489a0 and debd10ca9ca31816b7cffe691856de1a35673bfc.
📒 Files selected for processing (3)
src/main/index.tssrc/main/startup/renderer-sandbox-fallback-marker.test.tssrc/main/startup/renderer-sandbox-fallback-marker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/index.ts
| export function markRendererSandboxFallbackNoticeShown(userDataPath: string): void { | ||
| const marker = readRendererSandboxFallbackMarker(userDataPath) | ||
| if (!marker || marker.noticeShown) { | ||
| return | ||
| } | ||
| writeFileSync(markerPath(userDataPath), JSON.stringify({ ...marker, noticeShown: true })) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make notice-state persistence best effort.
writeFileSync can fail because of permissions, disk exhaustion, or the marker disappearing between the read and write. Since the caller persists this state before showing the dialog, the exception can prevent the notice from appearing. Catch and log the write failure so dialog presentation continues.
…9891), never auto-drop Reworks the #9891 fallback so Orca never lowers a security layer on the user's behalf. Detection is now silent: a launch-time renderer STATUS_BREAKPOINT burst is remembered but not acted on. When the renderer-recovery breaker gives up, if the crash loop is the win32 sandbox signature, Orca shows a consent dialog ("Orca keeps crashing on startup" → "Disable the sandbox and restart" / "Quit") in place of the generic dead-end prompt. The build-scoped marker (and therefore webPreferences.sandbox:false) is written ONLY if the user chooses to disable it; declining persists nothing, so the next launch is sandboxed again. The choice is remembered per build (marker clears on update), so a build that still can't launch re-asks. Removes the earlier after-the-fact "sandbox disabled" notice and its marker noticeShown field, since consent up front replaces it. Breadcrumbs: renderer_sandbox_crash_burst_detected, _user_accepted / _declined, _engaged / _applied.
debd10c to
b4e0c4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/index.ts (1)
1552-1565: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMarker-write failure leaves the user silently stuck.
If
writeRendererSandboxFallbackMarkerthrows, the function onlyconsole.warns and returns — the user already clicked "Disable the sandbox and restart" but nothing happens: no relaunch, no dialog, no further recovery prompt, andrendererSandboxBurstremains stale for any future exhaustion event. This is a real risk on this exact platform: the file itself works around a known Windows userData-ACL write failure elsewhere (lines 1026-1037), so a marker write failing for the same reason here is plausible, not hypothetical.🩹 Suggested direction
} catch (error) { console.warn('[renderer-sandbox-fallback] failed to persist marker:', error) + recordDurableCrashBreadcrumb('renderer_sandbox_fallback_marker_write_failed') + void presentRendererRecoveryPrompt(burst.crashesInWindow) return }
🧹 Nitpick comments (1)
src/main/index.ts (1)
1543-1546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a renderer-sandbox-specific Windows env helper for this marker.
getWindowsGpuFallbackEnvironment()returnsWindowsGpuFallbackEnvironment, which structurally satisfieswriteRendererSandboxFallbackMarker, but the helper name and upstream GPU-fallback import path make this look coupled to GPU fallback handling. Add a small renderer-sandbox-specific helper (or reuse a shared Windows env source) so the intent and types match the persisted marker contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2c09059e-b93e-4ae3-953e-68c53e0f8f1a
📥 Commits
Reviewing files that changed from the base of the PR and between debd10ca9ca31816b7cffe691856de1a35673bfc and b4e0c4e.
📒 Files selected for processing (1)
src/main/index.ts
| recordCrashBreadcrumb('renderer_sandbox_fallback_engaged', { | ||
| reason: burst.reason, | ||
| exitCode: burst.exitCode, | ||
| crashesInWindow: burst.crashesInWindow | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fallback-engaged breadcrumb likely lost — use the durable recorder before exit.
recordCrashBreadcrumb only appends to an in-memory list; it doesn't flush. app.exit(0) runs a few lines later without triggering before-quit/will-quit, so this breadcrumb — the single event confirming the workaround actually engaged — can be dropped before any batching/flush occurs. Every sibling event in this same flow (burst detected, user accepted/declined) uses recordDurableCrashBreadcrumb, which explicitly calls flushActiveSink().
🩹 Proposed fix
- recordCrashBreadcrumb('renderer_sandbox_fallback_engaged', {
+ recordDurableCrashBreadcrumb('renderer_sandbox_fallback_engaged', {
reason: burst.reason,
exitCode: burst.exitCode,
crashesInWindow: burst.crashesInWindow
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| recordCrashBreadcrumb('renderer_sandbox_fallback_engaged', { | |
| reason: burst.reason, | |
| exitCode: burst.exitCode, | |
| crashesInWindow: burst.crashesInWindow | |
| }) | |
| recordDurableCrashBreadcrumb('renderer_sandbox_fallback_engaged', { | |
| reason: burst.reason, | |
| exitCode: burst.exitCode, | |
| crashesInWindow: burst.crashesInWindow | |
| }) |
| isQuitting = true | ||
| relaunchApp('renderer-sandbox-fallback', { | ||
| processReason: burst.reason, | ||
| exitCode: burst.exitCode, | ||
| crashesInWindow: burst.crashesInWindow | ||
| }) | ||
| app.exit(0) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm relaunchApp's signature/behavior and whether it already preserves agent auth internally.
rg -n -B3 -A20 'function relaunchApp' src/main/app-relaunch.ts
rg -n 'preserveAgentAuthBeforeRestart' src/mainRepository: stablyai/orca
Length of output: 1947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant relaunch call sites and the preservation helper without executing repository code.
sed -n '1150,1215p' src/main/index.ts
printf '\n--- preservation helper ---\n'
sed -n '1,140p' src/main/agent-auth-restart-preservation.ts
printf '\n--- usages of engageRendererSandboxFallback ---\n'
rg -n 'engageRendererSandboxFallback|renderer-sandbox-fallback' src/main/index.ts src/main/app-relaunch.tsRepository: stablyai/orca
Length of output: 7338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the crash handler trigger for engageRendererSandboxFallback and whether the relaunch path is synchronous around it.
sed -n '1495,1585p' src/main/index.ts
sed -n '90,125p' src/main/app-relaunch.ts
sed -n '1,45p' src/main/app-relaunch.tsRepository: stablyai/orca
Length of output: 4065
Preserve agent auth before the renderer sandbox fallback relaunch
The sandbox fallback path is user-initiated before relaunch, so it should not bypass the shutdown/auth preservation step used by the other relaunch quit flows; make this path async/await the preservation helper before relaunchApp(...) + app.exit(0).
🩹 Suggested direction
- isQuitting = true
- relaunchApp('renderer-sandbox-fallback', {
+ isQuitting = true
+ desktopRelayService?.fenceAndCloseNow()
+ await preserveAgentAuthBeforeRestart({ codexRuntimeHome, claudeRuntimeAuth, store })
+ relaunchApp('renderer-sandbox-fallback', {
processReason: burst.reason,
exitCode: burst.exitCode,
crashesInWindow: burst.crashesInWindow
})
app.exit(0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isQuitting = true | |
| relaunchApp('renderer-sandbox-fallback', { | |
| processReason: burst.reason, | |
| exitCode: burst.exitCode, | |
| crashesInWindow: burst.crashesInWindow | |
| }) | |
| app.exit(0) | |
| isQuitting = true | |
| desktopRelayService?.fenceAndCloseNow() | |
| await preserveAgentAuthBeforeRestart({ codexRuntimeHome, claudeRuntimeAuth, store }) | |
| relaunchApp('renderer-sandbox-fallback', { | |
| processReason: burst.reason, | |
| exitCode: burst.exitCode, | |
| crashesInWindow: burst.crashesInWindow | |
| }) | |
| app.exit(0) |
|
Moving this PR to draft because it has been untouched for more than 80 days (latest recorded PR activity or commit timestamp: 2026-07-26T01:10:02Z; checked on 2026-10-27). This keeps the review queue focused on active work while preserving the proposal. Please mark it ready for review when work resumes. |
Fixes #9891
Description
On Windows 11 build 26200 (Intel Iris Xe), Orca's renderer process breakpoints
~2s after launch with
STATUS_BREAKPOINT—0x80000003, reported by Electron asrender-process-gone { reason: 'crashed', exitCode: -2147483645 }— on nearly every start.The reporter established that
--in-process-gpuremoves the GPU-child crash but the rendererstill crashes, and only
--no-sandboxfully fixes it: a renderer‑sandbox incompatibilitywith this Windows build.
Two existing mechanisms are the wrong tool for this:
touch the renderer sandbox, so the renderer keeps breakpointing (the reporter's breadcrumb trail
gpu_fallback_applied → main_window_created → renderer crashedconfirms this).breakpoints again, 3× → then dead-ends at the "Orca keeps failing to load" dialog.
This PR adds a build‑scoped renderer sandbox fallback that mirrors the existing GPU‑fallback
marker system — but Orca never disables the sandbox on the user's behalf. After 3
launch‑window renderer
STATUS_BREAKPOINTcrashes on win32, the crash is remembered but nothingis done; when the renderer‑recovery breaker gives up, Orca shows a consent dialog offering
Disable the sandbox and restart or Quit. Only on consent does it persist a marker and
relaunch with the top‑level renderer unsandboxed (
webPreferences.sandbox: false) — thesurgical, per‑window equivalent of the confirmed
--no-sandbox, while GPU / utility /network child processes and webview guests stay sandboxed. The marker is scoped to
appVersion + electronVersionand auto‑clears on any update, so a fixed Electron buildautomatically regains the sandbox (and a still‑broken build re‑asks).
Key safety properties:
-36861) — detection keys strictly on exit code-2147483645, so OOM and other crashes can never trigger the downgrade.hasEngaged/ active‑this‑launch guard means it engages at most once;if
sandbox:falsestill crashes it degrades to today's behavior (the dialog), never a relaunchloop. Independent of the GPU marker (both can coexist; the app converges to a stable state).
replaces the generic "keeps failing to load" prompt for this signature (single dialog, no
stacking). The marker is written only if the user chooses to disable the sandbox; declining
persists nothing, so the next launch is sandboxed again.
Transparency & hardening
the crash loop is this signature and the recovery breaker gives up, a native dialog ("Orca keeps
crashing on startup") offers Disable the sandbox and restart or Quit, explaining that it
lowers one hardening layer for the app window only, that workspaces/data stay isolated, and that
the sandbox is retried automatically after the next update.
sandbox:falseis persisted onlyon consent; declining persists nothing. Re‑asked per build (the marker clears on update).
sandbox:truevia
will-attach-webview.contextIsolationstays on and the page has no Node access, so thereduction is limited to the top‑level first‑party renderer process only.
app.enableSandbox(); this is documented at the source and a durablerenderer_sandbox_fallback_neutralizedbreadcrumb fires if the sandbox is ever forced globally,so a regressed machine is diagnosable rather than silently crash‑looping.
ELI5
Imagine Orca's window is a worker who has to put on a heavy safety suit (the "sandbox") before
starting. On these specific Windows 11 machines, that one suit is defective — the moment the worker
zips it up, they trip and fall (crash). Orca keeps handing them the same broken suit and telling
them to try again; they fall three times and Orca gives up with "keeps failing to load."
This change teaches Orca: "if the worker trips putting on this exact suit over and over at the
start, ask them whether they'd like to work without that one suit — and take it off only if they
say yes." Everyone else in the building keeps theirs on. It remembers their choice only for the
current app version (so a future fixed suit is tried again automatically), and it only offers this
for the very specific "tripped on the suit" fall, not for other kinds of falls (like running out of
memory).
Proof of fix
Reproduction (verified by code‑path trace on the affected build, Win11 10.0.26200):
Because auto‑recovery reloads mark only
recoveryReloadInFlight(orphan‑sweep), notexpectedRendererReload,getExpectedTeardownScopereturns'none'for every crash in the loop —so each launch‑window
STATUS_BREAKPOINTis a countable, "unexpected" event.After this change:
New tests proving the behavior (all green):
renderer-sandbox-crash-decision.test.ts— tracker window/threshold/engage‑once semantics; thepredicate matrix asserts only
{win32, renderer, -2147483645}is a candidate and that OOM-36861, other exit codes,childsource, and non‑win32 are all rejected.renderer-sandbox-fallback-marker.test.ts— round‑trip, build‑change/non‑win32/corrupt/missing‑fieldclearing (mirrors
gpu-fallback-marker.test.ts).createMainWindow.test.ts—webPreferences.sandbox === falseiffdisableRendererSandbox: true,trueby default.dashboard-popout-window.test.ts+renderer-sandbox-fallback-state.test.ts— pop‑out honors theshared launch flag; default stays sandboxed.
Screenshots
The new consent decision, and — for contrast — the generic dead‑end it replaces
for this crash signature.
Before — a launch‑time renderer
STATUS_BREAKPOINTloop dead‑ended here, andReload just re‑crashes the same sandboxed renderer:
After — when the loop is the win32 sandbox signature, Orca instead asks the
user to choose (nothing is disabled unless they pick the first button):
Proof of no other regressions
tsc --noEmit -p config/tsconfig.node.json→ clean (exit 0).oxlinton all changed files → clean (exit 0).Full baseline diff across
src/main/{window,crash-reporting,startup,ipc}+app-relaunch(2,486 tests): with my changes stashed, the failing‑file set is byte‑for‑byte identical to
the working tree —
All 9 pre‑existing failures are unrelated to this change: 6 are Windows‑environment tests
(
spawn /bin/sh ENOENT, symlinks, native fs watchers, clipboard) and 2 are brittleindex.tssource‑string‑ordering tests already out of sync with the file onmain.Review process
Reproduced (opus) on the affected build → design doc adversarially reviewed by a fable panel
(sandbox‑semantics / loop‑safety / detection / security / code‑fit) until no major issue →
implemented → adversarial fable review of the diff (0 critical, 0 major; 2 actionable minors
applied: window widened 30s→60s to match the recovery breaker + slow cold starts; pop‑out coverage)
→ performance review (clean, 0 findings). A third minor — gating on
reason==='crashed'or"never reached did‑finish‑load" — was rejected: the latter would exclude the actual #9891
crash, which fires ~2s after the window loads.
Design notes kept as a local planning doc per the repo's
docs/**convention (not checked in).