Repository navigation
Conversation
Synchronous execFileSync calls on Electron's main-process event loop stall every pending ipcMain reply, freezing the window. Worst on Windows, where a cold wsl.exe probe can block for seconds and CreateProcess plus the Defender filter driver tax every spawn. Routes the IPC-reachable spawns through the sanctioned runProcess wrapper and keeps narrow, commented runProcessSync lanes for the callers that are synchronous by construction (crash-time writes, instance locks, terminal teardown, runner-script builders). - wsl.ts, wsl-availability.ts: wsl.exe --list/--status/$HOME probes async - win32-utils.ts, secure-path-windows-acl.ts: whoami SID lookup cached and single-flighted, icacls async; SID resolution extracted to windows-current-user-sid.ts - git-exec-file.ts and the repo-detection/repo/default-base-ref callers async - codex-accounts/service.ts and its managed-home cascade async - posix-pty-foreground-group.ts: SIGWINCH signalling async (fired twice per revealed pane) - daemon-process-start-time.ts: getconf CLK_TCK cached per process lifetime - browser-cookie-key.ts: keychain and DPAPI lookups async - durable-file-write.ts, secure-file.ts: per-path write serializer plus an async write lane; pinned fsync ordering and 0o700/0o600 modes unchanged Shrinks both ratchets: child_process direct importers 155 -> 146, windowsHide-less spawners 65 -> 58.
Contributor
📝 WalkthroughWalkthroughThe pull request replaces many direct synchronous child-process calls with shared process runners and asynchronous APIs. It adds asynchronous Git, WSL, PTY, Windows ACL, browser-key, account, and daemon paths. It adds per-path write serialization and asynchronous secure-file persistence. It also updates runtime environment tracking, service readiness, process caches, tests, and child-process boundary fixtures. Priority: ⬆️ High Severity of issue fixed: High Merge Risk: 🟠 High · up to The change can lose newer runtime-environment state, expose temporary credentials under broader Windows ACLs, and report successful remote operations as failures. These issues should be fixed before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 39.39% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 50 files. (43 skipped: 43 over the file limit.)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 1a7d9494-448b-4c8f-8719-95c5d61349cd
📒 Files selected for processing (95)
src/cli/runtime/client.tssrc/cli/runtime/environments.tssrc/cli/runtime/remote-runtime-compat-gate.tssrc/main/ai-vault/cached-session-list-wsl-probe.test.tssrc/main/browser/browser-cookie-chromium-prepare.tssrc/main/browser/browser-cookie-import.test.tssrc/main/browser/browser-cookie-key.tssrc/main/codex-accounts/codex-account-identity-api-key-guard.test.tssrc/main/codex-accounts/codex-account-identity.tssrc/main/codex-accounts/codex-account-registration.tssrc/main/codex-accounts/codex-account-selection.tssrc/main/codex-accounts/codex-config-mirror.tssrc/main/codex-accounts/codex-login-session.tssrc/main/codex-accounts/codex-managed-home-lifecycle.tssrc/main/codex-accounts/codex-managed-home-path.tssrc/main/codex-accounts/service-account-add-login.test.tssrc/main/codex-accounts/service-account-selection-and-removal.test.tssrc/main/codex-accounts/service-add-account-from-home.test.tssrc/main/codex-accounts/service-login-process-teardown.test.tssrc/main/codex-accounts/service-managed-home-removal-retries.test.tssrc/main/codex-accounts/service-quota-refresh-decoupling.test.tssrc/main/codex-accounts/service-reauthenticate-activation.test.tssrc/main/codex-accounts/service-reset-credit-durability.test.tssrc/main/codex-accounts/service-reset-credit-target-routing.test.tssrc/main/codex-accounts/service-system-default-identity.test.tssrc/main/codex-accounts/service-wsl-accounts.test.tssrc/main/codex-accounts/service.test.tssrc/main/codex-accounts/service.tssrc/main/daemon/daemon-pid-identity.tssrc/main/daemon/daemon-process-identity-query.tssrc/main/daemon/daemon-process-start-time-clock-tick.test.tssrc/main/daemon/daemon-process-start-time.tssrc/main/daemon/daemon-tcc-attribution-main-thread.test.tssrc/main/durable-file-write.tssrc/main/git/command-runner/git-exec-file.tssrc/main/git/git-capability-probe-async-path.test.tssrc/main/git/repo-default-base-ref.tssrc/main/git/repo-detection.test.tssrc/main/git/repo-detection.tssrc/main/git/repo.test.tssrc/main/git/repo.tssrc/main/hooks-runner.test.tssrc/main/ipc/folder-repo-git-upgrade.tssrc/main/ipc/repos-local-add-and-project-setup.test.tssrc/main/ipc/repos-nested-import.test.tssrc/main/ipc/repos-nested-scan.test.tssrc/main/ipc/repos-remote-test-harness.tssrc/main/ipc/repos/local-repo-registration.tssrc/main/ipc/repos/nested-repo-import-handler.tssrc/main/ipc/runtime-environment-status-owner.tssrc/main/ipc/runtime-environment-support-routing.tssrc/main/ipc/runtime-environment-transport-routing.tssrc/main/ipc/runtime-environments-call-routing.test.tssrc/main/ipc/runtime-environments-status-diagnostics.test.tssrc/main/ipc/runtime-environments-subscription-lifecycle.test.tssrc/main/persistence/loading-store/user-data-path.tssrc/main/project-groups/nested-repo-discovery.tssrc/main/providers/local-pty-session-operations.tssrc/main/pty/posix-pty-foreground-group.test.tssrc/main/pty/posix-pty-foreground-group.tssrc/main/pty/posix-pty-process-groups.test.tssrc/main/pty/posix-pty-process-groups.tssrc/main/pwsh.test.tssrc/main/pwsh.tssrc/main/runtime/mobile-notification-dismissal-read-failure.test.tssrc/main/runtime/mobile-notification-dismissal-store.test.tssrc/main/runtime/mobile-notification-dismissal-store.tssrc/main/runtime/runtime-mobile-notification-controller.tssrc/main/runtime/runtime-nested-repo-import.tssrc/main/runtime/runtime-repository-registration-controller.tssrc/main/win32-utils-async-acl.test.tssrc/main/win32-utils.test.tssrc/main/win32-utils.tssrc/main/wsl-availability-missing-kernel.test.tssrc/main/wsl-availability.tssrc/main/wsl-distro-list-single-flight.test.tssrc/main/wsl-running-distros.test.tssrc/main/wsl.test.tssrc/main/wsl.tssrc/relay/pty-handler.tssrc/shared/bounded-secure-json-file.tssrc/shared/child-process/__fixtures__/child-process-import-allowlist.txtsrc/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txtsrc/shared/child-process/child-process-import-boundary.test.tssrc/shared/child-process/windows-console-visibility.test.tssrc/shared/path-write-serializer.tssrc/shared/runtime-environment-store.test.tssrc/shared/runtime-environment-store.tssrc/shared/secure-file-async-write-serialization.test.tssrc/shared/secure-file-async-write.tssrc/shared/secure-file.test.tssrc/shared/secure-file.tssrc/shared/secure-path-windows-acl-sid-lookup.test.tssrc/shared/secure-path-windows-acl.tssrc/shared/windows-current-user-sid.ts
💤 Files with no reviewable changes (2)
- src/shared/child-process/fixtures/child-process-import-allowlist.txt
- src/shared/child-process/fixtures/windows-console-visibility-allowlist.txt
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Comment on lines
+58
to
+61
| void markEnvironmentUsed(this.userDataPath, this.environmentSelector, { | ||
| runtimeId: response._meta.runtimeId | ||
| }).catch((error: unknown) => { | ||
| console.warn('[runtime-environments] could not record environment use:', error) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the environment-use failure policy consistent.
A failed preflight stamp is caught here, but RuntimeClient.call awaits markEnvironmentUsed(...) again after the same remote response succeeds. The same store error then rejects the completed RPC call. Use one stamp owner, or also make the post-response remote stamp best-effort if a failed lastUsedAt update must not fail the call.
Based on PR objectives: a failed lastUsedAt stamp must not fail the call.
| this.entries = next | ||
| if (!this.unreadable) { | ||
| writeSecureJsonFile(this.path, next) | ||
| await writeSecureJsonFileAsync(this.path, next) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/shared/secure-file-async-write.ts --items all
rg -n -C 8 '\bwriteSecure(File|JsonFile)Async\s*\(' src/shared
rg -n -C 4 'writeSecureJsonFileAsync\s*\(' srcRepository: stablyai/orca
Length of output: 13729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,145p' src/shared/secure-file-async-write.ts
printf '\n--- related result contracts and implementations ---\n'
rg -n -C 10 'Promise<boolean>|return false|return true|HardeningOutcome|writeSecureFile\(' src/shared/secure-file.ts src/shared src/main/runtime
printf '\n--- dismissal store callers and warning handling ---\n'
sed -n '1,125p' src/main/runtime/mobile-notification-dismissal-store.ts
rg -n -C 8 'mobile-notification-dismissal|dismissal|persistence warning|persist|unreadable' src/mainRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/shared/secure-file-async-write.ts ---'
sed -n '1,88p' src/shared/secure-file-async-write.ts
printf '%s\n' '--- src/shared/secure-file.ts result-related lines ---'
rg -n -C 12 'Promise<boolean>|return false|return true|HardeningOutcome|writeSecureFile' src/shared/secure-file.ts
printf '%s\n' '--- dismissal store ---'
sed -n '1,120p' src/main/runtime/mobile-notification-dismissal-store.ts
printf '%s\n' '--- exact dismissal-store tests ---'
fd -i 'mobile-notification-dismissal-store' . --type f --exec sh -c 'echo --- "$1"; sed -n "1,180p" "$1"' sh {} \;Repository: stablyai/orca
Length of output: 21997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- record callers ---'
rg -n -C 12 '\.record\(|dismissalStore|MobileNotificationDismissalStore' src/main/runtime/runtime-mobile-notification-controller.ts src/main/runtime src/main --glob '*.ts' --glob '!**/*.spec.ts' --glob '!**/*.test.ts' | head -n 260
printf '%s\n' '--- warning/error terms in the controller ---'
rg -n -C 10 'warn|warning|persist|persistence|dismissal|record' src/main/runtime/runtime-mobile-notification-controller.tsRepository: stablyai/orca
Length of output: 28403
Handle a failed secure-file hardening result
writeSecureJsonFileAsync returns false when it writes the file but cannot restrict its permissions. record ignores this result, so the controller's rejection handler does not emit its persistence warning. Treat false as a failure or handle the permission failure explicitly.
| // Serialized: the read-modify-write now spans awaits, so two concurrent stamps deriving `next` | ||
| // from the same snapshot would lose one. The key is deliberately distinct from the store path — | ||
| // the file write nested inside this takes that lane, and re-entering it would deadlock. | ||
| await serializePathWrite(`${getEnvironmentStorePath(userDataPath)}#mark-used`, async () => { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize all environment-store mutations in one transaction lane.
This lane only orders markEnvironmentUsed calls. A synchronous store mutation can commit while the asynchronous secure write yields. The pending write then publishes its stale snapshot and discards the newer mutation.
Use one shared read-modify-write lane for every store mutation. If synchronous callers must remain, add version validation and retry before publication.
| // recursive mkdir is a no-op on an existing directory, so no exists-then-create race. | ||
| await mkdir(dir, { recursive: true, mode: 0o700 }) | ||
| // Windows dir hardening stays async + path-cached (it stormed the main thread, #4901); POSIX keeps the metadata cache to catch chmod/ctime drift. | ||
| hardenSecurePathOnce(dir, true) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- async writer ---'
cat -n src/shared/secure-file-async-write.ts | sed -n '1,130p'
printf '%s\n' '--- hardening definitions and callers ---'
rg -n -C 8 'hardenSecureDirectoryOnce|function hardenSecurePathOnce|writeFile\\(|tmpFile|applySecurePathRestriction|HardeningOutcome' src/shared/secure-file.ts src/shared/secure-path-windows-acl.ts src/shared/secure-file-async-write.ts
printf '%s\n' '--- Windows ACL helper ---'
cat -n src/shared/secure-path-windows-acl.ts | sed -n '1,230p'Repository: stablyai/orca
Length of output: 6799
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- secure-file hardening ---'
rg -n -F -C 12 'hardenSecureDirectoryOnce' src/shared/secure-file.ts
rg -n -F -C 12 'function hardenSecurePathOnce' src/shared/secure-file.ts
printf '%s\n' '--- Windows ACL implementation ---'
rg -n -F -C 12 'function bestEffortRestrictWindowsPath' src/shared/secure-path-windows-acl.ts
rg -n -F -C 12 'restrictAsync' src/shared/secure-path-windows-acl.ts
printf '%s\n' '--- hardening cache and async result symbols ---'
rg -n -F -C 8 'hardenedPathsThisProcess' src/shared/secure-file.ts
rg -n -F -C 8 'Promise<boolean>' src/shared/secure-path-windows-acl.tsRepository: stablyai/orca
Length of output: 11590
Sensitive Data Exposure
Reachability: Internal
Exploitability: Moderate
CWE: CWE-377 — Insecure Temporary File
Reachability path
● Entry
src/cli/runtime/remote-runtime-compat-gate.ts:29
send: The preflight callback is synchronous and a failed lastUsedAt stamp must not fail the call.
│
▼
● Hop
src/shared/runtime-environment-store.ts:180
markEnvironmentUsed: Serialized: the read-modify-write now spans awaits, so two concurrent stamps deriving `next`
│
▼
● Hop
src/shared/bounded-secure-json-file.ts:15
writeSecureJsonFileWithinLimitAsync
│
▼
● Sink
src/shared/secure-file-async-write.ts
Await Windows directory hardening before creating the temporary file. hardenSecurePathOnce(dir, true) starts asynchronous ACL hardening and returns immediately. writeFile can create the temporary credential file before the directory ACL is restricted. Await a single-flight hardening result before writeFile, and continue only after restriction succeeds.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
One real data-loss race: runtime-environment-store.ts is now written through two unsynchronized lanes. Detail inline.
Reviewed changes
- WSL probes moved async (
wsl.ts,wsl-availability.ts):--list/--status/$HOMEnow run throughrunProcess, argv viabuildWslExecArgs, with the sync twins retained for sync-by-construction callers. Non-zero exits are now checked fromresult.codeinstead of relying onexecFileSyncthrowing. - Windows ACL/identity (
win32-utils.ts, newwindows-current-user-sid.ts,secure-path-windows-acl.ts):whoamiSID cached and single-flighted across both lanes,icaclsasync,resolveWindowsCommandmemoized; SID logic split out to respect the 300-line cap. - Git exec (
git-exec-file.ts,repo*.ts): async by default;gitExecFileSynckept only for the synchronous runner-script builders and routed throughrunProcessSync. - Codex accounts: broad async cascade; the constructor's config-mirror move is exposed as
readonly ready: Promise<void>. - Secure-file writes: new
path-write-serializer.ts(per-path async lane) andsecure-file-async-write.ts;durable-file-write.tswrites wrapped in the serializer. - PTY SIGWINCH (
posix-pty-foreground-group.ts): async twin for the high-cadence renderer path; sync twin retained for the daemon. - Ratchets shrink as designed:
DIRECT_IMPORTER_PIN155→146,UNHIDDEN_SPAWNER_PIN65→58.
See the inline comment for the one issue and the nitpicks below.
ℹ️ Nitpicks
CodexAccountService.readyis awaited by tests only; no production caller references it (main-process-account-services.ts:62drops it). The startup mirror is benign because every launch and rate-limit path re-syncs at point of use, but the JSDoc promises a contract nothing exercises — either await it at the one construction site or drop the field.src/shared/path-write-serializer.ts:5starts a comment with// ponytail:, which reads like a leaked harness codename. The rest of the repo uses// Why:— worth normalizing.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Comment on lines
+185
to
+188
| // Serialized: the read-modify-write now spans awaits, so two concurrent stamps deriving `next` | ||
| // from the same snapshot would lose one. The key is deliberately distinct from the store path — | ||
| // the file write nested inside this takes that lane, and re-entering it would deadlock. | ||
| await serializePathWrite(`${getEnvironmentStorePath(userDataPath)}#mark-used`, async () => { |
Contributor
There was a problem hiding this comment.
markEnvironmentUsed now reads the store and then awaits a multi-step async write, but the three synchronous mutators (addEnvironmentFromPairingCode, removeEnvironment, updateEnvironmentFromPairingCode) still write the same orca-environments.json without taking this lane. A mutation that runs during the await window is overwritten by next, which was derived from the pre-mutation read — a just-paired environment can vanish, or a removed one reappear.
Technical details
# Cross-lane lost update on orca-environments.json
## Affected sites
- `src/shared/runtime-environment-store.ts:188` — `markEnvironmentUsed` takes the `#mark-used` lane, which only serializes other `markEnvironmentUsed` calls.
- `src/shared/runtime-environment-store.ts:213` — the write (`writeEnvironmentStoreAsync` -> `writeSecureFileAsync`) spans several awaits (mkdir, write, async icacls on Windows, rename).
- `src/shared/runtime-environment-store.ts:89,96,135` — `writeEnvironmentStore` (sync) writes the same path via `writeSecureFile`, which never calls `serializePathWrite`; `secure-file.ts`'s docstring already warns a file must use one lane or the other, not both.
- `src/main/ipc/runtime-environment-connectivity-handlers.ts:76,107` — reachable sync mutators that can run as queued main-thread tasks inside the window.
## Why it is reachable
Store is `[A]`. A detached `markEnvironmentUsed(A)` reads `[A]`, computes `[A']`, then awaits. An `addFromPairingCode(B)` handler queued behind it reads `[A]`, writes `[A, B]`, and renames. `markEnvironmentUsed` resumes and renames its stale `[A']` over it; `B` is gone. The trailing rename makes the stale snapshot authoritative, so the loss is one-directional but real. `remove`/`update` interleavings have the mirror consequence (a removed environment or an old pairing payload is restored).
## Required outcome
All writers of `orca-environments.json` must share a single serialization domain, or the read-modify-write must be made safe against a concurrent synchronous writer.
## Suggested approach
Move `addEnvironmentFromPairingCode`/`removeEnvironment`/`updateEnvironmentFromPairingCode` onto the async secure-file lane and `await` them from their IPC handlers, so every writer takes the same per-path serializer. Keeping the sync lane for a path that also has an async writer is the invariant `secure-file.ts` documents as forbidden.
This was referenced Oct 9, 2026
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 10, 2026
…f the IPC thread Repo registration, nested import, folder upgrade and the setup/issue runner script called git synchronously from the main process (600-700 ms per rev-parse on affected hosts). Detection and the runner-script git-path lookup now run over gitExecFileAsync, and the provider contract's isGitRepo is async. The repo probe keeps the 15 s ceiling of the synchronous call it replaced and still falls back to the .git marker. The wsl.exe, whoami and icacls probes go through runProcess so the spawn observer sees them. The synchronous isWslAvailable twin (no production caller) is gone, the synchronous whoami fallback in grantDirAcl's identity lookup is replaced by a warm-the-cache async lookup, and grantDirAcl passes /q so a recursive grant cannot overflow the captured output. Remaining synchronous sites, each with a comment: grantDirAcl's EPERM recovery (stablyai#1095), isPwshAvailable for the daemon's synchronous launch plan, and wsl.ts getWslHome / listWslDistros / wslUncDirectoryExists, whose ~20 synchronous callers need their own increment. Part of mercurai/claude-code-config#1096. Borrowed from stablyai/orca stablyai#20498. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 10, 2026
…debounced dismissals A Windows secure write ran five synchronous icacls on the main thread (verify, reset, grant, verify, then a verify on the published path), and the profile of a running build put 8.6 s of a 75 s sample in those spawns. The write lane now stages the file, restricts it with exactly two async icacls runs (reset, then inheritance plus grants), and renames; the protected DACL travels with the rename, so no read-back is needed. The current-user SID is resolved once through an async single-flight whoami. The sync writer drops its verify passes too (two spawns). The mobile notification dismissal store persists latest-wins snapshots after a 5 s debounce and not at all when no push service or phone listener exists. Main-process markEnvironmentUsed bookkeeping persists detached on the async lane, deduplicated per environment; the CLI keeps the blocking path. Pairing mutators stay on the sync writer. Tests: zero sync spawns and two icacls per async write, DACL on a real file equals the verified sequence's, coalescing, unpersisted records. Part of mercurai/claude-code-config#1090. Borrowed from stablyai/orca stablyai#20498 and stablyai#26027. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit fe0e79b)
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 10, 2026
…debounced dismissals A Windows secure write ran five synchronous icacls on the main thread (verify, reset, grant, verify, then a verify on the published path), and the profile of a running build put 8.6 s of a 75 s sample in those spawns. The write lane now stages the file, restricts it with exactly two async icacls runs (reset, then inheritance plus grants), and renames; the protected DACL travels with the rename, so no read-back is needed. The current-user SID is resolved once through an async single-flight whoami. The sync writer drops its verify passes too (two spawns). The mobile notification dismissal store persists latest-wins snapshots after a 5 s debounce and not at all when no push service or phone listener exists. Main-process markEnvironmentUsed bookkeeping persists detached on the async lane, deduplicated per environment; the CLI keeps the blocking path. Pairing mutators stay on the sync writer. Tests: zero sync spawns and two icacls per async write, DACL on a real file equals the verified sequence's, coalescing, unpersisted records. Part of mercurai/claude-code-config#1090. Borrowed from stablyai/orca stablyai#20498 and stablyai#26027. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit fe0e79b)
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 10, 2026
…f the IPC thread Repo registration, nested import, folder upgrade and the setup/issue runner script called git synchronously from the main process (600-700 ms per rev-parse on affected hosts). Detection and the runner-script git-path lookup now run over gitExecFileAsync, and the provider contract's isGitRepo is async. The repo probe keeps the 15 s ceiling of the synchronous call it replaced and still falls back to the .git marker. The wsl.exe, whoami and icacls probes go through runProcess so the spawn observer sees them. The synchronous isWslAvailable twin (no production caller) is gone, the synchronous whoami fallback in grantDirAcl's identity lookup is replaced by a warm-the-cache async lookup, and grantDirAcl passes /q so a recursive grant cannot overflow the captured output. Remaining synchronous sites, each with a comment: grantDirAcl's EPERM recovery (stablyai#1095), isPwshAvailable for the daemon's synchronous launch plan, and wsl.ts getWslHome / listWslDistros / wslUncDirectoryExists, whose ~20 synchronous callers need their own increment. Part of mercurai/claude-code-config#1096. Borrowed from stablyai/orca stablyai#20498. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 9635bf0)
Contributor
|
Closing this broad refactor as not planned. It spans security, credential homes, durable writes, signalling and dozens of production modules; the claimed Windows freeze reduction was not measured on Windows. Current main already has async base-ref/registration paths, identity single-flight caching and host-scoped getconf caching, so several headline changes are superseded. We require a measured remaining hot path and a narrow low-risk fix, rather than this transplant. Broader Windows symptom reports remain open. |
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 11, 2026
…debounced dismissals A Windows secure write ran five synchronous icacls on the main thread (verify, reset, grant, verify, then a verify on the published path), and the profile of a running build put 8.6 s of a 75 s sample in those spawns. The write lane now stages the file, restricts it with exactly two async icacls runs (reset, then inheritance plus grants), and renames; the protected DACL travels with the rename, so no read-back is needed. The current-user SID is resolved once through an async single-flight whoami. The sync writer drops its verify passes too (two spawns). The mobile notification dismissal store persists latest-wins snapshots after a 5 s debounce and not at all when no push service or phone listener exists. Main-process markEnvironmentUsed bookkeeping persists detached on the async lane, deduplicated per environment; the CLI keeps the blocking path. Pairing mutators stay on the sync writer. Tests: zero sync spawns and two icacls per async write, DACL on a real file equals the verified sequence's, coalescing, unpersisted records. Part of mercurai/claude-code-config#1090. Borrowed from stablyai/orca stablyai#20498 and stablyai#26027. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit fe0e79b)
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 11, 2026
…f the IPC thread Repo registration, nested import, folder upgrade and the setup/issue runner script called git synchronously from the main process (600-700 ms per rev-parse on affected hosts). Detection and the runner-script git-path lookup now run over gitExecFileAsync, and the provider contract's isGitRepo is async. The repo probe keeps the 15 s ceiling of the synchronous call it replaced and still falls back to the .git marker. The wsl.exe, whoami and icacls probes go through runProcess so the spawn observer sees them. The synchronous isWslAvailable twin (no production caller) is gone, the synchronous whoami fallback in grantDirAcl's identity lookup is replaced by a warm-the-cache async lookup, and grantDirAcl passes /q so a recursive grant cannot overflow the captured output. Remaining synchronous sites, each with a comment: grantDirAcl's EPERM recovery (stablyai#1095), isPwshAvailable for the daemon's synchronous launch plan, and wsl.ts getWslHome / listWslDistros / wslUncDirectoryExists, whose ~20 synchronous callers need their own increment. Part of mercurai/claude-code-config#1096. Borrowed from stablyai/orca stablyai#20498. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 9635bf0)
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 11, 2026
…debounced dismissals A Windows secure write ran five synchronous icacls on the main thread (verify, reset, grant, verify, then a verify on the published path), and the profile of a running build put 8.6 s of a 75 s sample in those spawns. The write lane now stages the file, restricts it with exactly two async icacls runs (reset, then inheritance plus grants), and renames; the protected DACL travels with the rename, so no read-back is needed. The current-user SID is resolved once through an async single-flight whoami. The sync writer drops its verify passes too (two spawns). The mobile notification dismissal store persists latest-wins snapshots after a 5 s debounce and not at all when no push service or phone listener exists. Main-process markEnvironmentUsed bookkeeping persists detached on the async lane, deduplicated per environment; the CLI keeps the blocking path. Pairing mutators stay on the sync writer. Tests: zero sync spawns and two icacls per async write, DACL on a real file equals the verified sequence's, coalescing, unpersisted records. Part of mercurai/claude-code-config#1090. Borrowed from stablyai/orca stablyai#20498 and stablyai#26027. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit fe0e79b)
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 11, 2026
…f the IPC thread Repo registration, nested import, folder upgrade and the setup/issue runner script called git synchronously from the main process (600-700 ms per rev-parse on affected hosts). Detection and the runner-script git-path lookup now run over gitExecFileAsync, and the provider contract's isGitRepo is async. The repo probe keeps the 15 s ceiling of the synchronous call it replaced and still falls back to the .git marker. The wsl.exe, whoami and icacls probes go through runProcess so the spawn observer sees them. The synchronous isWslAvailable twin (no production caller) is gone, the synchronous whoami fallback in grantDirAcl's identity lookup is replaced by a warm-the-cache async lookup, and grantDirAcl passes /q so a recursive grant cannot overflow the captured output. Remaining synchronous sites, each with a comment: grantDirAcl's EPERM recovery (stablyai#1095), isPwshAvailable for the daemon's synchronous launch plan, and wsl.ts getWslHome / listWslDistros / wslUncDirectoryExists, whose ~20 synchronous callers need their own increment. Part of mercurai/claude-code-config#1096. Borrowed from stablyai/orca stablyai#20498. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 9635bf0)
ph47s74x
added a commit
to mercurai/orca
that referenced
this pull request
Oct 11, 2026
…f the IPC thread Repo registration, nested import, folder upgrade and the setup/issue runner script called git synchronously from the main process (600-700 ms per rev-parse on affected hosts). Detection and the runner-script git-path lookup now run over gitExecFileAsync, and the provider contract's isGitRepo is async. The repo probe keeps the 15 s ceiling of the synchronous call it replaced and still falls back to the .git marker. The wsl.exe, whoami and icacls probes go through runProcess so the spawn observer sees them. The synchronous isWslAvailable twin (no production caller) is gone, the synchronous whoami fallback in grantDirAcl's identity lookup is replaced by a warm-the-cache async lookup, and grantDirAcl passes /q so a recursive grant cannot overflow the captured output. Remaining synchronous sites, each with a comment: grantDirAcl's EPERM recovery (stablyai#1095), isPwshAvailable for the daemon's synchronous launch plan, and wsl.ts getWslHome / listWslDistros / wslUncDirectoryExists, whose ~20 synchronous callers need their own increment. Part of mercurai/claude-code-config#1096. Borrowed from stablyai/orca stablyai#20498. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
The app answers all UI requests on one thread. That thread was occasionally stopping to run
wsl.exe --status,whoami.exe,icacls.exeandgitand waiting for them to finish. On Windows that wait can be seconds, and while it waits the window is frozen. This makes those calls async, and caches the ones whose answer never changes.What Changed
Routes the IPC-reachable synchronous spawns through the existing sanctioned wrapper
src/shared/child-process/run-process.ts(runProcess→Promise<ProcessResult>), and caches what cannot change within a session. Narrow, commentedrunProcessSynclanes are kept for the callers that are synchronous by construction.wsl.ts,wsl-availability.tswsl.exe --list/--status/$HOMEprobes async; argv now built viabuildWslExecArgs; existing single-flight + backoff reused; dropped both directnode:child_processimportswin32-utils.ts,secure-path-windows-acl.tswhoamiSID lookup cached for the process lifetime and single-flighted;icaclsasync; PATH resolution memoized (10s negative TTL so a mid-session install is still found). SID resolution extracted to newsrc/shared/windows-current-user-sid.ts—secure-path-windows-acl.tswas near the 300-line cap and AGENTS.md forbids amax-linesdisable, so it was split, not suppressedgit/command-runner/git-exec-file.tsgitExecFileAsync; propagated throughrepo-detection.ts,repo.ts,repo-default-base-ref.tsand their callers. Capability probes go through the existingGitCapabilityCache; argv, the-c-before-subcommand form, and the Git 2.25 fallbacks are unchangedcodex-accounts/service.tstaskkill/ login spawn /validateWslPathasync, plus the managed-home assert/mirror cascade.validateWslPathdeliberately not cached — it is a TOCTOU ownership checkpty/posix-pty-foreground-group.tsdaemon-process-start-time.tsgetconf CLK_TCKresolved once per process lifetime (kernel build constant) instead of once per liveness checkbrowser/browser-cookie-key.tssecurity/secret-tool) and DPAPI lookups async; PowerShell invocation byte-identicaldurable-file-write.ts,secure-file.tssrc/shared/path-write-serializer.ts) + an async write lane (src/shared/secure-file-async-write.ts)Both ratchets shrink, which is the direction they already point:
DIRECT_IMPORTER_PIN155 → 146,UNHIDDEN_SPAWNER_PIN65 → 58 (the 7 de-listed files are exactly the ones fixed here).Deliberately kept synchronous
Each with a one-line comment naming the caller, and each verified not on an
ipcMainreply path:pwsh.tsavailability probe — both IPC readers already useisPwshAvailableAsync; the only production caller of the sync probe is the daemon's synchronouscreatePtyShellLaunchPlanposix-pty-process-groups.tsforce-kill — every caller is a synchronous throw-ordered state machine that uses the throw to arm a node-pty fallback; two tty-scopedpsreads, once per teardownwriteFileDurableSync/ synchronouswriteSecureFile— crash-time and teardown writes; nothing can be awaited at crash timeorcad-instance-lock.ts—acquireOrcadInstanceLockis synchronous by naturegitExecFileSync— retained for the sync worktree-runner-script builders, its documented sole callerWhy
The renderer's
ipcRenderer.invokeis answered on the main process's single event loop, so one synchronous spawn stalls every pending IPC reply and the window stops responding. Reachability analysis (depth 6, from all 170ipcMain-registering files) found 27 synchronous spawn sites in 16 IPC-reachable modules;wsl.exe --statuson a cold WSL VM is seconds long and was reachable from 124 entrypoints.Full method, the ranked table, and the honest limitations are in #20497.
Approach rationale:
runProcessalready exists as the chokepoint (it pinswindowsHide, refusesshell: true, and encodes.cmd/.batargs so neitherCommandLineToArgvWnorcmd.exemangles them), andsecure-path-windows-acl.tsalready demonstrated the async-default + narrow-sync-fallback shape. This follows those instead of inventing a mechanism, which is also why both ratchets move in the direction they were built to move.The ~1,139 IPC-reachable synchronous
fscalls are not touched — individually microseconds, and converting them speculatively would be a far riskier diff for no measured gain. Only the fsync/durability path was included.Linked Issue
Fixes #20497
Context on the symptom side: #11363 (Windows UI freeze with ~5 terminals, WSL2 UNC worktrees), and plausibly #7175 / #6874 / #11207. See the limitations section of #20497 — I do not claim this closes those.
Visual Proof
N/A — no UI, layout, or interaction surface changes. This moves blocking work off the main thread; the only observable difference is the absence of a stall, which needs a Windows profile rather than a screenshot (see Testing).
Testing
Platforms actually tested: Linux only. I do not have a Windows or macOS host. The Windows-specific paths (
wsl.exe,whoami.exe,icacls.exe, DPAPI) are covered by mocked-process tests, not by execution on Windows. This needs a Windows reviewer before merge — ideallypnpm bench:main-thread-jankandpnpm test:e2e:windows-fresh-startup-golden, which already exist in this repo.Ran the full CI set from CONTRIBUTING.md:
pnpm typecheck— clean (all three projects;tc:nodealone missed anodenextexplicit-extension error in a new test, caught by the full run)pnpm lint— clean, includingcheck:max-lines-ratchet,check:ts-nocheck-ratchet,check:runtime-electron-ratchet,check:reliability-gatesand everyverify:*steppnpm test— 79,786 passed / 12 failed. All 12 failures reproduce unchanged on a pristinefe4237cdworktree and are environmental: no git tags (shallow clone) breaks the cross-version-wire harness, Windows native addons absent on Linux (electron-builder-config), the repo pinspnpm@12.0.0which is unpublished for linux-x64 (patched-dependencies-frozen-install), plus two pre-existingconfigure-processGPU-flag expectations. Verified by running each failing file in both trees.pnpm build— not run (needs a full Electron/native toolchain fetch this environment could not complete). Flagging rather than implying it passed.Tests added/updated — each defends the anti-freeze property rather than the refactor:
src/main/wsl-distro-list-single-flight.test.ts— repeat IPC-path probes spawn exactly twice andrunProcessSyncis never calledsrc/main/win32-utils-async-acl.test.ts— rewritten onto therun-processmock; adds "spawns whoami once for concurrent and repeated ACL grants" and a non-zero-exit case proving the oldexecFileSyncthrow contract survived the move to a code-returning APIsrc/shared/secure-path-windows-acl-sid-lookup.test.ts(new) — Linux-runnable; onewhoamiacross two concurrent plus one later hardening call, andicacls /resetstill reachedsrc/main/git/git-capability-probe-async-path.test.ts(new) — an unsupported capability is probed once viaGitCapabilityCache, never synchronouslysrc/main/daemon/daemon-process-start-time-clock-tick.test.ts(new) —CLK_TCKfetched once across repeated inspectionssrc/shared/secure-file-async-write-serialization.test.ts(new) — concurrent writes to one path do not interleave, fsync orderwrite → fsync:file → rename → fsync:directorypreserved, and the in-flight lane map does not leakdaemon-tcc-attribution-main-thread.test.ts,secure-file.test.ts,hooks-runner.test.ts— re-pointed from partialnode:child_processmocks onto the sanctioned layer. No assertion loosened: thelive/unverifiable/exitedfail-open vocabulary, theintact/unknownACL verdicts, theunexpected D ruledeny-rule rejection, the LRU-eviction proofs, and the Windows/WSL launch-shell and MSYS path expectations are all unchangedThree assertions were mutation-checked to prove they are not vacuous: flipping
pendingSidLookup ??=to=fails the SID single-flight test; switchinggetPsProcessIdentityAsynctorunProcessSyncfails the daemon "without a synchronous spawn" guard at that exact line; both were reverted and re-verified.AI Disclosure
Written with Claude (Opus) driving the loom agent harness: analysis, implementation across five parallel work slices, and verification. Every claim above was produced by running the named command in this repo; the unrun ones are called out as unrun.
Review
Please look hardest at:
src/shared/secure-file.tstwin lanes. MakingwriteSecureFileasync naively typechecks clean and is a data-loss regression — all ~20 callers ignore the return value, so they silently become fire-and-forget writers with lost throws. Full propagation reaches 100+ files including their synchronous test suites. I chose the async/sync twin pattern already used bywriteFileDurable/writeFileDurableSyncandrestrictAsync/restrictWindowsPathSyncinstead. If you want the full propagation, say so and I will do it as a follow-up.runtime-environment-store.markEnvironmentUsed. Its read-modify-write now spans awaits, so it takes a lane keyed${storePath}#mark-used, deliberately distinct from the file lane it nests into — re-entering the same key would deadlock.assertExitZeroinwin32-utils.ts.execFileSyncthrew on non-zero,runProcessreturns it as data. I preserved the throw for the threegrantDirAclcallers and it now carries stderr — please confirm that is the error propagation you want.Agent skill upstream boundary
N/A — no changes under
skills/,skill-guides/, orskill-stubs/.