Repository navigation
fix(runtime): surface desktop RPC startup failures - #11037
brennanb2025 merged 12 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:
📝 WalkthroughWalkthroughAdds runtime RPC startup failure classification with privacy-safe telemetry, localized error messaging, and Electron dialog handling. Desktop startup now captures the RPC start result, records failures, and displays a recovery dialog tied to the main window. Tests cover error classification, telemetry payloads, dialog content, window lifecycle timing, dialog rejection handling, and startup ordering. Localized strings are added for English, Spanish, Japanese, Korean, and Chinese. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
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: 19808e8a-fabe-47b9-a5f6-00bbad460ac0
📒 Files selected for processing (10)
src/main/index.tssrc/main/runtime/runtime-rpc-startup-failure.test.tssrc/main/runtime/runtime-rpc-startup-failure.tssrc/main/startup/desktop-startup-ordering.test.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/shared/telemetry-events.ts
23900ab to
f158569
Compare
…dialog tests The `no-floating-promises` label span covers the whole `app.whenReady().then()` callback, so adding lines inside it made a long-standing finding overlap changed code. `void` is the linter's own suppression; no `.catch()` on purpose. The startup-failure tests were vacuous: mutation runs showed the wait-for-show deferral, the destroyed-window guard, the `closed` companion event, listener cleanup, the cause walk, the cycle guard, and the truncation bound could all be deleted with every test still green. The "not called yet" assertion ran before any microtask, so it passed either way.
Anchoring the slice on the full destructure and matching the whole dialog call expression made an innocuous rename break the test with a cryptic 'expected -1'. Match the shape that is actually the contract instead.
The desktopEnd anchor comment lost a word in 98b00d3, so indexOf returned -1 and slice(start, -1) covered index.ts to EOF. Moving the dialog call to a path that never runs at startup still passed. Anchor on code instead, and assert both bounds so a future reword fails loudly.
Round 3 bounded the desktop pair but left attachStart/attachEnd unguarded in the same test: deleting the PTY startup barrier from attachMainWindowServices() and breaking the rateLimits.attach(window) end anchor still left the case green.
Rounds 3 and 4 fixed the desktop and attach pairs; two instances of the same class survived in the same file, both proven vacuous by mutation: - it #3 never bounded readyEnd. Renaming the `pairing:` payload key makes it -1, widening readyPayload from 372B to ~52KB. Moving the reconciliation status out of the serve-ready payload (its whole point) but leaving it later in index.ts then kept all 6 cases green. - it #2 bounded desktopWindowStart against reconciliationStart rather than serveEnd. An earlier `Promise.resolve(openMainWindow())` steals the anchor, collapsing desktopStartup to '' while every existing guard still passes, so its only assertion — a negative — succeeds against an empty string. Both mutants now fail. `src/main/ipc/pty-startup-barrier-ordering.test.ts:11` has the same latent shape; left alone as out of scope for this PR.
2d6977a to
2f45607
Compare
getErrorCode returned the first code it found, so an outer wrapper carrying an unrecognised code masked a nested EACCES/ENOSPC and classified it unknown. Only a mapped code ends the walk now; every other input classifies as before. Unreachable today (writeSecureFile rethrows raw fs errors with .code intact), but the classifier's job is surviving whatever error shape reaches it.
The dialog's only advice was "Restart Orca to try again", which is true for address_in_use and wrong for the rest: permissions, a full or read-only disk, and a missing data folder all survive a relaunch, so the user restarted, hit the same failure and had no next step. Route the error class we already compute into the copy so each cause names the thing the user has to change. Guidance and telemetry now derive from the same classifier, so they cannot drift apart.
Greptile SummaryThis PR surfaces previously-silent desktop RPC startup failures by converting the bare
Confidence Score: 5/5Safe to merge — the change adds error reporting on a failure path that previously swallowed errors silently, with no impact on the happy path. Every failure mode in the new code (telemetry throws, dialog rejects, window destroyed before or after the show event) is caught and logged rather than propagated. Startup remains non-blocking: the dialog is fired with void and the rest of the startup sequence continues unaffected whether or not the RPC transport started. The settled-result pattern in Promise.all guarantees the outer startup never rejects. Tests are thorough, mutation-checked per the PR description, and the startup-ordering assertions verify the new calls are wired on the correct code path. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/main/runtime/runtime-rpc-startup-failure.ts | New module: classifies errno codes, records privacy-safe telemetry, and shows a deferred native dialog. Error cases (telemetry throws, dialog rejects, destroyed window) are all caught and logged. Listener cleanup is correct; truncation logic is sound. |
| src/main/index.ts | Swaps the bare .catch for a settled-result pattern: telemetry is recorded synchronously in the rejection handler and the dialog is fired with void so startup is not blocked. |
| src/main/runtime/runtime-rpc-startup-failure.test.ts | 15 cases cover classification, cause chain walking, cyclic-cause safety, telemetry isolation, privacy checking, truncation, deferred-show, destroyed-window guards, and dialog-error swallowing. |
| src/main/startup/desktop-startup-ordering.test.ts | Anchors hardened against comment rewording; bounds checks added for all slice anchors; new assertions verify the new calls are wired on the startup path. |
| src/shared/telemetry-events.ts | Adds runtimeRpcStartErrorClassSchema (closed enum) and runtime_rpc_start_failed event schema following the same pattern as daemonStartFailedSchema. |
| src/renderer/src/i18n/locales/en.json | Adds runtimeRpc.startupFailure i18n tree with all keys consumed by runtime-rpc-startup-failure.ts. Keys and fallback strings match exactly. |
Reviews (3): Last reviewed commit: "fix(runtime): guard destroyed window bef..." | Re-trigger Greptile
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
* fix(runtime): surface desktop RPC startup failures * fix(runtime): isolate RPC failure telemetry * fix(runtime): satisfy the changed-code quality gate and kill vacuous dialog tests The `no-floating-promises` label span covers the whole `app.whenReady().then()` callback, so adding lines inside it made a long-standing finding overlap changed code. `void` is the linter's own suppression; no `.catch()` on purpose. The startup-failure tests were vacuous: mutation runs showed the wait-for-show deferral, the destroyed-window guard, the `closed` companion event, listener cleanup, the cause walk, the cycle guard, and the truncation bound could all be deleted with every test still green. The "not called yet" assertion ran before any microtask, so it passed either way. * test(runtime): de-brittle the desktop RPC-failure source assertions Anchoring the slice on the full destructure and matching the whole dialog call expression made an innocuous rename break the test with a cryptic 'expected -1'. Match the shape that is actually the contract instead. * test(runtime): repair the silently-unbounded desktop startup slice The desktopEnd anchor comment lost a word in 624232e, so indexOf returned -1 and slice(start, -1) covered index.ts to EOF. Moving the dialog call to a path that never runs at startup still passed. Anchor on code instead, and assert both bounds so a future reword fails loudly. * test(runtime): bound the attach anchors in the startup ordering slice Round 3 bounded the desktop pair but left attachStart/attachEnd unguarded in the same test: deleting the PTY startup barrier from attachMainWindowServices() and breaking the rateLimits.attach(window) end anchor still left the case green. * test(startup): bound the last two unguarded slice anchors in this file Rounds 3 and 4 fixed the desktop and attach pairs; two instances of the same class survived in the same file, both proven vacuous by mutation: - it stablyai#3 never bounded readyEnd. Renaming the `pairing:` payload key makes it -1, widening readyPayload from 372B to ~52KB. Moving the reconciliation status out of the serve-ready payload (its whole point) but leaving it later in index.ts then kept all 6 cases green. - it stablyai#2 bounded desktopWindowStart against reconciliationStart rather than serveEnd. An earlier `Promise.resolve(openMainWindow())` steals the anchor, collapsing desktopStartup to '' while every existing guard still passes, so its only assertion — a negative — succeeds against an empty string. Both mutants now fail. `src/main/ipc/pty-startup-barrier-ordering.test.ts:11` has the same latent shape; left alone as out of scope for this PR. * fix(runtime): keep walking the cause chain past an unmapped code getErrorCode returned the first code it found, so an outer wrapper carrying an unrecognised code masked a nested EACCES/ENOSPC and classified it unknown. Only a mapped code ends the walk now; every other input classifies as before. Unreachable today (writeSecureFile rethrows raw fs errors with .code intact), but the classifier's job is surviving whatever error shape reaches it. * fix(runtime): tell the user what to fix, not just to restart The dialog's only advice was "Restart Orca to try again", which is true for address_in_use and wrong for the rest: permissions, a full or read-only disk, and a missing data folder all survive a relaunch, so the user restarted, hit the same failure and had no next step. Route the error class we already compute into the copy so each cause names the thing the user has to change. Guidance and telemetry now derive from the same classifier, so they cannot drift apart. * fix(runtime): guide users through long RPC paths * fix(runtime): avoid false window listener warning * fix(runtime): guard destroyed window before web contents
Summary
and cause-specific guidance on what to fix
Live QA
Reproduced the real failure end to end on macOS against an isolated dev profile, with no product code modified. The injection makes
orca-runtime.jsonimmutable (chflags uchg), sowriteSecureFile()'srenameSyncfails withEPERM— the exact "single failed write toorca-runtime.json" the issue describes:The failure is total, and silent without this PR.
orca-runtime.jsonwas never published, and the CLI is dead against a live, healthy-looking app — the same symptom as #7848:{ "app": { "running": false }, "runtime": { "state": "stale_bootstrap", "reachable": false } }1. Dialog over the main window — appears only after the window is visible, so it is never orphaned behind a hidden window.
2. Close-up — i18n resolves (real copy, not raw keys), and both
{{cause}}and{{guidance}}interpolate rather than printing placeholders. The guidance line is chosen from the sameerror_classthe telemetry reports, so advice and telemetry cannot drift.3. After "Continue without CLI" — the app stays fully usable, matching the copy's promise.
Guidance copy
A bare "Restart Orca to try again" is only correct for
address_in_use— permissions, a full or read-only disk, and a missing data folder all survive a relaunch. Each class now names the thing the user has to change:error_classpermission_deniedstorage_unavailableinvalid_pathaddress_in_useunknownMutation-checked: forcing every class to the generic line fails 6 tests, dropping the
{{guidance}}substitution fails 7, and swapping one class's copy for another's fails 2 — so the per-class mapping is pinned, not just "some guidance is present".Also confirmed:
recordRuntimeRpcStartFailureran (EPERM→permission_denied), with zero dialog-show and zero telemetry-record errors logged.Note: macOS ignores
titleon message boxes, so the "Orca CLI unavailable" headline does not render there —message+detailcarry the impact and cause, so nothing essential is lost.Not covered: Windows/Linux were not exercised (both additionally have the 10s
initialRevealFallbackTimer, so reveal is at least as likely there).Final Electron validation
Re-ran the real immutable-
orca-runtime.jsonfailure in this exact worktree through Electron/CDP and the native macOS accessibility surface. The captures below show the full product context rather than a recreated fixture.Native dialog over the usable app — the CLI impact, permission-specific remediation, raw local cause, and “Continue without CLI” action all render without clipping or overlap.
After “Continue without CLI” — the native sheet closes and the app remains interactive; Settings navigation was exercised immediately afterward.
The Electron run also exposed and then verified the fixes for a false
MaxListenersExceededWarningon the failure path and destroyed-window access ordering. At final head932556687b, the same real failure showed the dialog with no listener warning or unhandled rejection.Verification
vitest run src/main/runtime/runtime-rpc-startup-failure.test.ts src/main/startup/desktop-startup-ordering.test.ts— 36 tests passedconsole.error, each fail the startup-ordering testpnpm run typecheck:nodepnpm run lintnode config/scripts/check-changed-code-quality.mjs -- origin/mainCloses #11025