Repository navigation
ci: stop new main-process code from freezing Orca with synchronous launches - #27756
Conversation
The Electron main process freezes every window while a synchronous child process runs, and on Windows each launch is slow because antivirus scans it. Nothing stopped new synchronous launches from landing in main-process code, and the spawn observer behind ORCA_MAIN_THREAD_DIAGNOSTICS only saw async spawns, so the worst blocks were invisible to it. Ratchet: check:main-sync-spawn-ratchet parses production src/main and src/shared files (test-only paths excluded) and fails when a file not in config/main-sync-spawn-baseline.txt reaches execFileSync, spawnSync or execSync from child_process, runProcessSync from @orca/process-host, or one of the synchronous secure-file writers (they run icacls on Windows). Named, aliased, namespace, default, require() and import() forms count. A baseline entry whose file no longer launches synchronously fails as stale, so the list can only shrink. It runs in lint and in the PR static-analysis job next to the process-host import check. Observer: runProcessSync now reports to the spawn observer, timing the whole call (that is how long the thread was stuck) and reporting from a finally so failing, throwing and timed-out children are counted too. Runtime behavior is unchanged except that the diagnostics observer now sees synchronous launches; with no observer registered the sync path pays one extra branch.
The sync-spawn ratchet only tracked which files launched synchronously, so a file already on the list (wsl.ts, win32-utils.ts, git-exec-file.ts) could add more launches unnoticed, and new callers of the repo's own synchronous wrappers passed CI. The baseline now stores a per-file count of uses, in the same format as the focus-read ratchet: a rise or a new file fails, a lower or zero count fails until `pnpm check:main-sync-spawn-ratchet --prune` lowers it, and --prune never raises an entry. Every value reference to a gated function counts (call, hand-off, alias and its uses, re-export); imports alone count nothing. Gated wrappers: gitExecFileSync (and the git/runner re-export), restrictWindowsPathSync, hardenSecurePath when called with sync: true, grantDirAcl, getWslHome, listWslDistros, wslUncDirectoryExists, isWslAvailable (and the wsl.ts re-export), isPwshAvailable. Their 18 current caller files join the baseline. Failure messages now give advice per family: the async process-host API for child-process launches, the async twin for wrappers, and for the secure writers a note that no async secure writer exists yet, so the sync one stays and must never become a plain write (that drops the Windows ACL hardening). Also refresh two stale blockMs descriptions and drop a no-op line from the spawn-observer "stays silent" test.
…tests
hardenSecurePath was counted only when `{ sync: true }` was written inline,
so moving the options into a variable or a spread dropped the count and the
check then asked for a prune. It now counts every use except a call with no
options or a spread-free object literal whose `sync` is absent or false.
Also: resolve backslash paths in the importing file as well as the
specifier; still visit heritage and instantiation expressions
(`extends mixin(fn)`, `fn<T>`); state the reference-only counting limit;
and point the secure-writer advice at raising this file's baseline count.
Tests cover the hardenSecurePath shapes, the alias chains that need the
fixpoint loop, and a prune that must not raise a grown row.
|
Waiting synchronously for another program holds Electron’s main event loop and can leave Orca’s windows unresponsive. Existing code already contains these waits, the original file allowlist let listed files add more, and diagnostics omitted The review fixed per-file growth enforcement, coverage of known synchronous wrappers, conditional secure-hardening detection, useful secure-writer failure advice, and the workflow’s pinned background-step test. The final independent review found and fixed one further gap: a spread of call arguments could hide Known limits remain deferred: the scanner counts references rather than tracing arbitrary wrappers, two cache-miss helpers are outside its explicit list, and unusual module-loading/member syntax can escape this scanner. Unconventional test harnesses remain in the baseline, diagnostics combine existing synchronous/asynchronous command buckets, and baseline-command deduplication remains cleanup work. These limits are disclosed in the PR body and do not change execution behavior. Final verification: final head Remote Electron QA now passed on M4Air at Diagnostics coverage remains unit-only: success, nonzero exit, missing programs and timeouts were tested through the process API; a live app diagnostics path was not exercised. The native folder picker, Windows, Linux and SSH flows were not exercised in this QA run. No latency improvement is claimed or measured for this prevention-and-reporting change. Review status: source review, readiness, final CI and the required remote startup/folder QA passed at Startup on M4Air:
Plain-folder workspace selected:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The ratchet and synchronous timing change are mergeable after normal checks. No actionable failure was established in the reviewed paths. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 9 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly identifies the main change: CI now prevents new synchronous launches in the main process. It is concise and directly related to the pull request. Description check The description is mostly complete. It includes the ELI5 summary, changes, rationale, linked issue, visual-proof explanation, testing details, platform notes, and checklist. It also documents known li… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 9 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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: Advanced
- Run ID:
13e0584f-1e89-46ea-993e-d134c44262df
📒 Files selected for processing (12)
.github/workflows/pr.ymlconfig/main-sync-spawn-baseline.txtconfig/scripts/check-main-sync-spawn-ratchet.mjsconfig/scripts/check-main-sync-spawn-ratchet.test.mjsconfig/scripts/pr-preflight-gates.test.mjsconfig/scripts/static-module-specifiers.mjspackage.jsonsrc/main/diagnostics/main-thread-churn-probe.tssrc/packages/process-host/src/run-process.tssrc/packages/process-host/src/spawn-observer.test.tstests/e2e/main-thread-git-cost.spec.tstests/tools/benchmarks/main-thread-jank-bench.mjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| */ | ||
| export function runProcessSync(spec: ProcessSpec): ProcessResult { | ||
| const resolved = resolveSpawn(spec, process.platform) | ||
| // Diagnostics only: spawnSync holds the calling thread until the child exits, | ||
| // so the whole call is the block, reported even when it throws. | ||
| const spawnStartedAt = hasSpawnObserver() ? performance.now() : null | ||
| try { | ||
| return runResolvedProcessSync(spec, resolved) | ||
| } finally { | ||
| if (spawnStartedAt !== null) { | ||
| notifySpawnObserver(resolved.file, resolved.args, performance.now() - spawnStartedAt) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function runResolvedProcessSync(spec: ProcessSpec, resolved: ResolvedSpawn): ProcessResult { | ||
| const result = nodeSpawnSync(resolved.file, [...resolved.args], { | ||
| ...resolved.options, | ||
| input: spec.input, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '310,390p' src/packages/process-host/src/run-process.ts
rg -n 'notifySubprocessSpawn|spawnObserver|SpawnObserver|recordSubprocessSpawn' src/packages/process-host/src src/main/diagnostics/main-thread-churn-probe.tsRepository: stablyai/orca
Length of output: 5105
🏁 Script executed:
printf '%s\n' '--- spawn-observer.ts ---'
nl -ba src/packages/process-host/src/spawn-observer.ts
printf '%s\n' '--- run-process imports and async path ---'
nl -ba src/packages/process-host/src/run-process.ts | sed -n '1,85p'
printf '%s\n' '--- observer registrations and consumers ---'
rg -n -F --glob '*.{ts,tsx,mjs,js}' -- 'setSpawnObserver' src packages tests 2>/dev/null || true
rg -n -F --glob '*.{ts,tsx,mjs,js}' -- 'recordSubprocessSpawn' src packages tests 2>/dev/null || true
printf '%s\n' '--- observer tests ---'
nl -ba src/packages/process-host/src/spawn-observer.test.ts
printf '%s\n' '--- package exports ---'
rg -n -F --glob 'package.json' -- 'process-host' src packages 2>/dev/null || trueRepository: stablyai/orca
Length of output: 13331
🏁 Script executed:
printf '%s\n' '--- diagnostics recorder ---'
nl -ba src/main/diagnostics/main-thread-churn-probe.ts | sed -n '1,180p'
printf '%s\n' '--- startup registration ---'
nl -ba src/main/startup/main-process-preflight.ts | sed -n '220,270p'
printf '%s\n' '--- recorder tests ---'
nl -ba src/main/diagnostics/main-thread-churn-probe.test.ts | sed -n '1,150p'
printf '%s\n' '--- observer-related process-host tests and contracts ---'
rg -n -F --glob '*.{ts,tsx,mjs,js}' -- 'runProcessSync' src/packages/process-host src/main | head -80
rg -n -i --glob '*.{ts,tsx,mjs,js}' -- 'observer.*throw|throw.*observer|best.?effort|diagnostic' src/packages/process-host/src src/main/diagnostics src/main/startup | head -120Repository: stablyai/orca
Length of output: 38183
🏁 Script executed:
printf '%s\n' '--- process-host package metadata ---'
cat src/packages/process-host/package.json
printf '%s\n' '--- process-host entrypoints ---'
rg --files src/packages/process-host | sort | sed -n '1,120p'
rg -n -F --glob '*.{ts,tsx}' -- 'from "./spawn-observer"' src/packages/process-host
rg -n -F --glob '*.{ts,tsx}' -- 'setSpawnObserver' src/packages/process-host
printf '%s\n' '--- complete runProcess catch boundary ---'
nl -ba src/packages/process-host/src/run-process.ts | sed -n '61,105p'Repository: stablyai/orca
Length of output: 9029
Contain observer exceptions in notifySpawnObserver.
setSpawnObserver accepts an arbitrary callback, and notifySpawnObserver invokes it without containment. When that callback throws, runProcessSync can throw after a successful command, or its finally completion can replace the command's original startup error. The same callback can also make runProcess reject after the child has started, despite its documented contract.
Suggested fix
export function notifySpawnObserver(
command: string,
args: readonly string[],
blockMs: number
): void {
- observer?.(command, args, blockMs)
+ try {
+ observer?.(command, args, blockMs)
+ } catch {
+ // Diagnostics must not change process execution.
+ }
}
…ary (#28054) #27756's script imported baseline helpers from check-focus-read-ratchet.mjs after #27788 had moved them into ratchet-baseline.mjs, so it crashed on start and failed every PR that merges main. Use the library's helpers, and record the two daemon-process launches from #27769 that the broken check never saw. Co-authored-by: Orca Worker <orca-worker@localhost>
…ary (#28054) #27756's script imported baseline helpers from check-focus-read-ratchet.mjs after #27788 had moved them into ratchet-baseline.mjs, so it crashed on start and failed every PR that merges main. Use the library's helpers, and record the two daemon-process launches from #27769 that the broken check never saw. Co-authored-by: Orca Worker <orca-worker@localhost>
…27875) * fix(browser): annotation card and outline follow the selected element The grab overlay keeps re-measuring the picked element after the click (scroll in any box, resize, element size changes), and the isolated viewport bridge reports that live box so the pending card follows it, including page zoom. The card's side pick uses its measured height. * fix(browser): bind annotation card to selected element * fix(browser): keep document annotation card on selection * fix(ci): point the main sync-spawn ratchet at the shared ratchet library (#28054) #27756's script imported baseline helpers from check-focus-read-ratchet.mjs after #27788 had moved them into ratchet-baseline.mjs, so it crashed on start and failed every PR that merges main. Use the library's helpers, and record the two daemon-process launches from #27769 that the broken check never saw. Co-authored-by: Orca Worker <orca-worker@localhost> * fix(browser): follow selected element outer-box size * refactor(chat): give the start-refusal resumable table its own module (#28085) #27946 and #26684 together put agent-session-failure-words.ts at 304 lines, over max-lines, which fails lint and the pre-commit hook on every merge of main. The refusal-code table is standalone with one reader, so it moves out. Co-authored-by: Orca Worker <orca-worker@localhost> --------- Co-authored-by: OrcaWin <alpha-eng@stably.ai> Co-authored-by: Orca Worker <orca-worker@localhost>
ELI5
When Orca's main process starts another program and waits for it to finish, every Orca window freezes until it does. On Windows, starting a program is slow (antivirus scans each one), so these waits are the main cause of the "Orca froze for a moment" reports. This PR does two things. CI now fails if new main-process code adds another one of these waits. And Orca's existing slow-launch diagnostics can now see the waits that go through
runProcessSync.What Changed
The problem. The Electron main process handles every window's input, every terminal and every timer. A synchronous child-process launch (
execFileSync,spawnSync,execSync,runProcessSync) blocks all of that until the child exits. So does the synchronous secure-file writer, which runsicacls.exeseveral times per write on Windows. #27731 shows one such path freezing Orca for about 90 ms per agent notification on Windows. Production code insrc/mainandsrc/sharedstill has dozens of these calls: 118, in 80 files, counting callers of Orca's own synchronous helpers. Nothing stopped new ones from appearing, and the built-in spawn diagnostics couldn't see them:ORCA_MAIN_THREAD_DIAGNOSTICS=1(the[main-thread]churn probe) record async spawns only.runProcessSyncnever reported to the observer they use.User-facing change: none. Runtime behavior is unchanged. The only difference shows up when the spawn observer is installed (diagnostics on): synchronous launches are then reported, with their full blocking time.
The mechanism.
check:main-sync-spawn-ratchet(config/scripts/check-main-sync-spawn-ratchet.mjs) runs inpnpm lintand in the PR static-analysis job next tocheck:process-host-imports. It is modeled on that check.src/main/**andsrc/shared/**. It parses them with the same TypeScript-based scanner that check uses, and finds calls to the synchronous launch functions imported fromchild_processor@orca/process-host, and to the synchronous secure-file writers.require,import x = require(...)and dynamicimport(). Look-alike names from other modules, type-only imports, and strings or comments are ignored.gitExecFileSync,getWslHome,listWslDistros,wslUncDirectoryExists,isWslAvailable,isPwshAvailable,grantDirAcl,restrictWindowsPathSync, andhardenSecurePathunless the call is visibly async. Only their callers are counted;src/main/wsl.tsitself is untouched.config/main-sync-spawn-baseline.txtstores a call count per file, the same format as the focus-read ratchet. The check fails when any file's count rises, including a file that wasn't listed before. When a count falls, the baseline must be lowered with--prune. So the debt can only shrink.runProcess/spawnProcess, and each helper points to its async counterpart. For the secure writers, which have no async version yet, the message says to keep the writer and raise the count, never to swap in a plain write.runProcessSyncnow reports to the existing spawn observer, likerunProcessalready does. Files that callchild_processsynchronous functions directly still don't report, for examplewsl.ts,wsl-availability.tsandwin32-utils.ts(11 files, listed in the review comment). Each becomes visible once it moves to the process-host API. It reports the whole call, because that's how long the thread was blocked, and it reports from afinallyso failed and timed-out children are counted too. When no observer is installed, it costs one boolean check.Why
process-host-direct-import-baseline.txtand the max-lines ratchet.getDefaultWslDistroandresolveCurrentWindowsIdentity, aren't gated yet; they're listed as follow-ups in the review comment.Linked Issue
Refs #20497 (synchronous spawns on the main-process IPC thread). Linear: STA-7408.
Visual Proof
N/A. This adds a CI check and a diagnostics hook, with no runtime or UI change, so there is no before/after to measure. The measured effect of removing one of these waits is in #27731: a Windows notification went from 89 ms to 0.5 ms of main-process block.
Testing
check-main-sync-spawn-ratchet.test.mjscovers count rises and falls,--prune, helper gating, and:spawn-observer.test.tscovers a sync run that exits non-zero, a missing program (throws), a timeout, and no observer installed.node config/scripts/check-main-sync-spawn-ratchet.mjspasses on the tree (80 files, 118 uses), under Node andbun --bun, in about 4 s.check-process-host-importsstill passes.tscfor node and the process-host test project is clean, and oxlint is clean on the changed files.Agent skill upstream boundary
Notes
Checklist
N/Avisual proof, with reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (CI will cover)