fix(windows): make the install-dir ACL repair rescue the launch it runs in - #18361
fix(windows): make the install-dir ACL repair rescue the launch it runs in#18361nwparker wants to merge 4 commits into
Conversation
…not after The install-dir LPAC ACL poison (electron/electron#51761) still costs every affected machine at least one crash: the probe that detects it is setImmediate-deferred and answers 0.9-3.0s in, while createMainWindow runs synchronously in the same frame and its renderer dies at init 48-1373ms later. - Persist the poison verdict the moment the probe reports it, and await the repair (bounded at 20s) before any window is created on a launch that already carries the marker. - Do not engage the GPU safe-graphics fallback while the install-dir ACL verdict is poisoned or still outstanding. Safe graphics does not rescue a poisoned tree, and --in-process-gpu removes the GPU child, erasing the sibling-death evidence that identifies the shape (4 field reports landed in 'misc' this way). - Clear the safe-graphics marker once the repair lands, so a repaired machine stops launching software-rendered for the rest of that build. - Give the repair marker a bounded retry budget: it was written on failure and matched regardless of outcome, so one transient failure pinned a machine to 'marker-hit' for the life of that version.
Adversarial review round 1. Five blocking findings, all addressed. 1. gpu-lifecycle guard had only a source grep (green with the polarity inverted). The stated justification -- that gpu-lifecycle's import graph cannot be driven in-process -- was wrong: mocking `electron` plus `@electron-toolkit/utils` imports it fine. Replaced with gpu-lifecycle-install-dir-acl-guard.test.ts, which drives the real handleGpuChildCrash against a stub tracker. All four cases go red when the guard is flipped to `if (!isInstallDirAclSuspect())`. 2. A clean probe verdict retired the on-disk marker but not the in-memory `poison` verdict, so a machine the probe just proved healthy kept suppressing the GPU safe-graphics fallback and kept the dialog accusing the install folder -- permanently, since a `status:'failed'` probe deliberately keeps the marker. A positive clean reading now latches `installDirReadClean`, drops the verdict, and outranks a repair result that lands after it (a 'failed' from a repair with nothing left to fix must not re-accuse). 'repaired' is kept: it is not a contradiction and it is what tells the user to reload. 3. `noteWindowsInstallDirAclProbePending()` ran on every `openMainWindow` while the probe is once-per-process, so every tray/second-instance reopen armed a 15s window in which `recordGpuCrash` was never called at all -- on healthy machines. `probeWindowsInstallDirAcl` now reports whether THIS call dispatched, and only a dispatch arms the grace window. 4. The pre-window ordering guarantee was defeatable and untested. `focusExistingMainWindow` opens a window whenever there is none and the app is ready -- true for the whole 20s gate, which is exactly when a user double-clicks the shortcut again. Added a `canOpenWindow` seam (same 'pending' semantics as the existing `!app.isReady()` case) wired to `isBlockingInstallDirAclRepairInFlight()`, plus windows-install-dir-acl-startup-wiring.test.ts pinning the await ahead of both window-creation paths and both new call sites. 5. windows-install-dir-acl-repair.win32.test.ts was absent from the pr.yml win32 allowlist, so it ran nowhere. Added. Also from the non-blocking list: - The repair no longer clears a `userConfirmed: true` safe-graphics marker; "keep safe graphics" is a user choice, not Orca's automatic latch. - `repairWindowsInstallDirPackageAcl` now reports its dispatch too, so a second entry into the gate resolves immediately instead of eating the full 20s budget waiting on an `onDone` that is never coming. - The gate is wrapped in try/catch/finally, matching the contract the probe documents as mandatory for anything upstream of window creation. Rebutted, not applied: - "Gate should be conditioned on app.isPackaged." A dev launch only carries the poison marker if a dev launch actually probed that tree and found the signature, in which case the dev renderer is dying the same way and the repair is exactly what is needed. The adjacent `isPackaged` check guards a packaged-only early-window optimisation, not a correctness boundary. - "Fold the poison marker into the repair marker's `outcome`." They answer different questions with different lifetimes. The repair marker is a retry budget (`attempts >= 3` disables the repair for that version) and is never cleared; the poison marker is cleared by a successful repair and by a clean probe. A `'pending'` outcome written before the attempt would bump `attempts`, so three launches killed mid-repair would permanently disable a repair that never once ran icacls to completion.
…t is pending
Adversarial review round 2. Both blocking findings addressed.
1. handleGpuChildCrash early-returned on isInstallDirAclSuspect() BEFORE
recordGpuCrash, so the crash left no trace in the 30s rolling window. The
suspect window is armed on every win32 non-serve launch, and the field
bundles put it at 0.8-1.7s after main_window_created on hosts whose DACL is
clean (matchesPoisonSignature=false) -- squarely inside the 2.1-6.2s
bad-driver bursts this repo already pinned in
gpu-crash-fallback-field-sessions.test.ts. A healthy machine with a failing
driver could lose an entire coalesced burst and never engage safe graphics.
The crash is now always recorded; only the engagement consults the verdict,
and it waits for the verdict rather than acting on the suspicion
(waitForInstallDirAclVerdict, resolved by the probe's onDone or by the
existing 15s grace, whichever lands first).
Deviation from the review's suggested shape, deliberately: awaiting the
verdict before persisting anything reintroduces the exact race
gpu-fallback-engagement.ts documents -- Chromium aborts the whole browser
process on the 6th GPU crash, ~1.3s after the 3rd, which is less than the
probe takes to answer. So the unconfirmed marker is written up front and
withdrawn if the verdict comes back poisoned. A machine killed mid-wait
still comes back software-rendered, and its marker is unconfirmed, which is
the state the repair's own clear already retires.
gpu-lifecycle-install-dir-acl-guard.test.ts now drives the real
GpuCrashFallbackTracker and the real engagement path (the restart prompt
firing is the signal) instead of a stub tracker, and covers the case the
previous suite could not express: a burst that lands entirely inside the
pending window still engages once the probe reports clean. Four reverts go
red -- restoring the pre-record guard (2 tests), dropping the wait, dropping
the post-wait re-check, and dropping the pre-wait marker write (2 tests).
2. The round-1 evidence block quoted commits, a test name and pass counts that
no longer exist, and its real-icacls Windows run predated the commit that
rewrote the gate. Re-run at this commit; counts and the live-Windows result
are restated in the handoff rather than carried forward.
Also from the non-blocking list:
- 'marker-hit' conflated "already repaired" with "retry budget spent", because
hasMarkerFor matches outcome === 'repaired' too. The result now carries
alreadyRepaired, and the recovery maps that to stage 'repaired' -- so a launch
killed between a successful repair and its marker clear no longer tells the
user the folder needs an administrator, no longer latches
isInstallDirAclSuspect() for the session, and does retire the poison marker.
Not applied, with reasoning:
- "clearGpuFallbackMarker narrowed to userConfirmed === false leaves the target
population software-rendered after a repair." The summary was overstated and
is corrected, but the narrowing stands: a userConfirmed marker now requires a
clean DACL verdict, because the restart prompt that writes it is exactly what
the gate above withholds while the install is a suspect. The population this
family targets can no longer reach confirmMarker while poisoned.
- "writeInstallDirAclPoisonMarker re-stamps on a budget-exhausted machine
forever." True, but on that machine the tree really is still poisoned and the
gate resolves immediately ('skipped', no icacls spawn, no 20s wait), so the
marker is telling the truth. Retiring it would be wrong; only a clean probe
reading should.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (16)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe changes add persistent Windows install-directory ACL poison markers and bounded repair retries. Startup now repairs known poisoned directories before window creation and blocks reopen actions during repair. ACL probe dispatch reports whether a probe started and tracks a verdict grace period. GPU crash fallback waits for the ACL verdict and suppresses fallback when the install DACL remains suspect. Tests cover lifecycle behavior, startup wiring, retry handling, real Windows ACL repair, and CI execution. Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk is established for the current Windows ACL recovery changes. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is technically detailed and covers the problem, implementation, rationale, trade-offs, testing evidence, and follow-ups. However, it omits required template items, including a Linked Issue, Visual Proof or an exact N/A statement, AI Disclosure, Agent skill upstream boundary, Notes, and completed checklist confirmations. Resolution Add a valid Linked Issue entry using Full details: Docstring CoverageExplanation Docstring coverage is 45.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 15 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run was the fresh adversarial pass the PR body asks for (round 3's findings were lost in the handoff). I read the complete diff end-to-end and traced the full verdict/repair state machine, the GPU-fallback guard, and the second-instance reopen path.
- Persisted poison verdict gates the window —
windows-install-dir-acl-poison-marker.tswrites a synchronous installDir+appVersion-keyed marker the moment the probe reports poison;repairKnownPoisonedInstallDirBeforeWindowreads it and awaits the 20s-bounded repair before any window, wired atmain-process-runtime-launch.ts:299. - GPU fallback guard no longer misattributes —
gpu-lifecycle.tsalways records the crash and only the engagement consultsisInstallDirAclSuspect(), waiting for the verdict rather than discarding the crash. - Un-latch safe graphics on repair — a successful repair clears an unconfirmed
gpu-fallback-marker, leavinguserConfirmedmarkers alone. - Retry budget replaces outcome-agnostic pin —
MAX_REPAIR_ATTEMPTS = 3with backwards-compatibleattemptshandling, so one transient Defender lock no longer bricks a machine for the life of a version.
Notes from tracing the concurrency, all of which check out: the pre-window gate runs after app.isReady() (via initializeMainProcessReady), so the canOpenWindow seam in focusExistingMainWindow is load-bearing — a second-instance reopen during the blocking repair is dropped ('pending'), not queued, and !app.isReady() alone would not cover it. The marker-hit/alreadyRepaired distinction correctly maps a completed repair to repaired (clears the poison marker) while an exhausted budget holds the verdict. A clean probe outranks a later repair verdict via installDirReadClean, and an unreadable DACL clears the in-memory suspect but keeps the on-disk marker, matching the stated invariant.
The regression tests are real: the guard suite drives the actual handleGpuChildCrash/tracker (with an inverted-polarity rejection), and the real-icacls .win32.test.ts is now in the pr.yml win32 allowlist. The only caveats are the author's own open follow-ups (re-verify the live icacls run at HEAD via the win32 CI job, add durationMs to the repair breadcrumb, and a progress indicator for the gate) — none of these block merge.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
1 participant
What
Three crash clusters —
G1-windows-acl,G8-misc-crashedandG2-killed— are one bug, not three. Nine field diagnostic bundles (eight distinct reporters; the two 1.4.194G1bundles share a session) all carry the same install-directory DACL poison signature, all on Windows only (win3210.0.19045 / 10.0.26100 / 10.0.26200), on Orca 1.4.194, 1.4.195 and 1.4.196, and all end inrenderer / crashed / exit code -2147483645(0x80000003).The root cause is an orphan
S-1-15-2-*LPAC package ACE on the install tree with noS-1-15-2-1/-2grant to satisfy it — electron/electron#51761. Every sandboxed child dies reading Orca's own shipped modules. The read-only probe that detects it (#15107) and the repair that fixes it (#17740) both already exist. Neither rescues the launch it runs in:src/main/startup/windows-install-dir-acl-probe.ts:229— the probe issetImmediate-deferred and spawnsicacls, so its verdict lands seconds in.src/main/startup/main-window-controller.ts:80vs:96— the probe is dispatched, thencreateMainWindowsynchronously spawns the renderer that the verdict was supposed to save.Everything downstream then misreads the result.
handleGpuChildCrashsees the ACL-induced GPU child deaths as a bad driver, latches--in-process-gpu, and the resulting software-rendered launches file intomiscinstead ofwindows-acl.Four changes:
src/main/startup/windows-install-dir-acl-poison-marker.tswrites a tiny synchronouswindows-install-dir-acl-poison.jsonin userData keyed on installDir+appVersion (same shape as the existinggpu-fallback-marker.ts). It is written the moment the probe reports poison, before the repair runs, so a launch killed mid-repair still leaves state. NewrepairKnownPoisonedInstallDirBeforeWindow(windows-install-dir-acl-recovery.ts:198) reads it synchronously and, on a hit, awaits the real repair bounded at 20s — wired atsrc/main/startup/main-process-runtime-launch.ts:299, ahead of both desktop-window paths. A healthy machine pays one absent-filereadFileSync.gpu-lifecycle.ts:167still records every GPU crash unconditionally; only the engagement consults the install-DACL verdict (gpu-lifecycle.ts:173), and it waits for the verdict rather than acting on the suspicion.gpu-fallback-marker, so a repaired machine stops launching software-rendered for the rest of that build.windows-install-dir-package-acl-repair.tspreviously wrote its marker on failure and matched it regardless of outcome, so one transient failure (Defender-locked file, timeout, contended volume) pinned the machine tomarker-hit— repair permanently skipped — for the life of that version. NowMAX_REPAIR_ATTEMPTS = 3, backwards-compatible (scheme-1 markers readattemptsas 0, so already-pinned machines retry).Fix evidence
The field data
All nine poisoned bundles, and how many DACL verdicts each machine recorded across its retained launches:
145 of 149 verdicts are
poison=true. What those same nine bundles recorded downstream:186 blank-window give-ups, 112 manual Retry clicks by users, every one of the nine machines latched safe graphics — and one repair breadcrumb in the entire corpus. (The repair only exists in 1.4.196+; the one bundle on that version is the one that has it. That is the honest framing — the repair is not "never firing", it is firing too late and only on the newest build.)
The ordering, on the crashing launch,
mssincemainProcessStartedAt:On 1.4.196 the repair completes at +36.8s, thirty seconds after the first GPU child died and eight seconds after the recovery circuit breaker had already given up. That is the defect this PR fixes.
Regression tests: RED without each production hunk, GREEN with
Each production hunk reverted individually, then restored. Command, from
/tmp/fix-windows-acl-family:The GPU guard is the widest-blast-radius hunk, so it is also pinned against an inverted polarity — round 1 rejected an earlier source-grep test that stayed green when the guard was flipped. Flipping
if (!isInstallDirAclSuspect())toif (isInstallDirAclSuspect())ingpu-lifecycle.ts:135:Full suites, typecheck, lint
The 121 passing files include the
child-process-import-boundaryratchet (no new directnode:child_processimport) and the desktop-startup-ordering assertions.Live real-
icaclsrun on Windows — and what is NOT provensrc/main/startup/windows-install-dir-acl-repair.win32.test.tsis a real-binary integration test (describe.skipoff win32, following thewindows-command-line.win32.test.tsprecedent) and is now in thepr.ymlwin32 allowlist (.github/workflows/pr.yml:793), so the Windows CI job runs it against this HEAD. It was run manually on a real Windows host (win32, node v24.18.0, realicacls.exe) at commit3b528bd3fd:That run confirmed the documented trap against the real binary:
icacls <file> /grant "*S-1-15-2-2:(OI)(CI)(RX)"exits 0, printsFailed processing 0 files, and writes nothing to the file — which is why the recursive pass must use the flagless(RX)form. It also confirmed the real probe reportsmatchesPoisonSignature=truebefore andfalseafter, within the same blocking call, on a tree whose inheritance was disabled.Repair latency measured on the same host, on a realistically sized tree:
Not proven, stated plainly:
3b528bd3fd. The.win32.test.tsfile itself is byte-identical at HEAD, butwindows-install-dir-acl-recovery.ts(+126/-...) andwindows-install-dir-package-acl-repair.ts(+29/-...) changed afterwards in the two review-round commits. The Windows host was not reachable to re-run at HEAD (ssh: connect to host awin port 22: Operation timed out). The win32 CI job on this PR is what re-proves it against HEAD — treat this PR as unmerged until that job is green.windows-install-dir-acl-startup-wiring.test.tsplus the behaviouralfocus-existing-windowtests, not by a packaged launch.ELI5
On some Windows machines, the folder Orca is installed into ends up with a broken permission entry. It is not Orca's doing — Windows or an installer leaves it behind. The effect is that Orca's own helper processes are not allowed to read Orca's own files. So Orca starts, opens a window, and the window is blank. Click Retry and it goes blank again. Users in these reports clicked Retry 112 times.
Orca already knew how to fix this — it runs a Windows permissions command on itself. The problem was timing: it only started checking after it had already opened the window, so by the time it knew what was wrong and fixed it, the window had already died three times and given up. On one report the fix finished 37 seconds into a launch that had already surrendered at 29 seconds. To make it worse, Orca also mistook these deaths for a broken graphics driver, permanently switched itself to slow software rendering, and then filed the crash under the wrong category so nobody could see the pattern.
This change makes Orca remember "this install folder was broken last time". On the next start it fixes the permissions before opening any window, so the window that opens is one that works. It also stops Orca blaming the graphics driver for something that is not the graphics driver, and undoes the slow-rendering switch once the real problem is fixed.
Trade-offs
Real costs, all Windows-only.
windows-install-dir-acl-poison.jsonmatching this installDir+appVersion, window creation waits on theicaclsrepair (BLOCKING_REPAIR_BUDGET_MS,windows-install-dir-acl-recovery.ts). On timeout the repair keeps running in the background and the window opens anyway. Measured 708ms on a warm 3241-entry tree; a real 600MB+ install with Defender cold will be slower and is not measured. What it replaces on those same machines is today's behaviour: a blank window, three renderer deaths, the circuit breaker, and a manual Retry. Healthy machines pay one absent-filereadFileSync.icaclsis actively rewriting the per-file DACLs a fresh renderer would read — sofocusExistingMainWindowreturns'pending'(focus-existing-window.ts:148, same semantics as the existing!app.isReady()case) and that click produces nothing. The gated launch opens the window. On a machine where the gate times out at 20s, a user who double-clicked at 5s sees nothing extra from that click.gpu-lifecycle.ts:167) — a real driver burst is never erased from the rolling window. But engagement waits for the DACL verdict, bounded at 15s from probe dispatch. A user with a genuinely bad driver and a genuinely poisoned install will not be offered safe graphics this session; that is intended, because safe graphics does not rescue a poisoned tree and--in-process-gpudestroys the sibling-death evidence. A user with a bad driver on a healthy install sees engagement delayed by at most the probe's answer time, not skipped.icaclsup to 3 times (across 3 launches) instead of once. A hopeless standard-user Program Files install pays two extraicaclsattempts before pinning permanently. That is the price of not bricking a machine on one transient Defender lock — which is what the code did before this PR.userConfirmed: truemarker ("keep safe graphics", an explicit user choice) is left alone.windows-install-dir-acl-poison.json, written only on a positive poison verdict, win32 only. Unknown to older builds, harmless if left behind, self-retiring on a clean probe.Not affected: macOS, Linux, serve mode (explicitly exempt), SSH execution hosts (this is entirely local main-process startup — no RPC, no stream frame, no wire change), folder workspaces (no worktree assumptions anywhere).
Adversarial review
3 round(s), and the honest answer is that it did not converge clean.
Round 1 — 5 blocking, all fixed (commit
70c525847ee):gpu-lifecycle-install-dir-acl-guard.test.ts, which drives the realhandleGpuChildCrash; the inversion evidence above is the proof.installDirReadClean, which outranks any later repair result.noteWindowsInstallDirAclProbePending()ran on everyopenMainWindowwhile the probe is once-per-process, so every tray/second-instance reopen armed a 15s window on healthy machines. Only a dispatched probe arms it now (probeWindowsInstallDirAclreturns whether it dispatched).focusExistingMainWindowand had no test file. Added thecanOpenWindowseam andwindows-install-dir-acl-startup-wiring.test.ts.icaclstest was absent from thepr.ymlwin32 allowlist, so it ran nowhere. Added.Round 2 — 2 blocking, both fixed (commit
00ab2f57f55):recordGpuCrash, so a suspected crash was discarded rather than deferred — and the suspect window is armed on every win32 launch, including hosts whose DACL is clean. The reviewer measured 0.8–1.7s suppression windows on clean hosts, squarely inside the 2.1–6.2s bad-driver bursts this repo already pins ingpu-crash-fallback-field-sessions.test.ts. Fixed: the crash is always recorded; only the engagement consults the verdict, and it waits for it.00ab2f57f55.Rebutted, not applied (and worth a reviewer's disagreement):
app.isPackaged." Not applied. A dev launch only carries the poison marker if a dev launch actually probed that tree and found the signature — in which case the dev renderer is dying the same way and the repair is exactly what is wanted.outcome." Not applied. They have different lifetimes: the repair marker is a retry budget that is never cleared; the poison marker is cleared by a successful repair and by a clean probe. A'pending'outcome written pre-attempt would burn the retry budget, so three launches killed mid-repair would permanently disable a repair that never once completed.Round 3 — not clean, and not addressed. Round 3 re-reviewed HEAD (
00ab2f57f55) and did not sign off. Its findings were truncated out of the handoff this PR was written from, so I cannot restate them, and no commit on this branch responds to them. I am not going to invent a summary of a review I do not have. A reviewer should treat this section as incomplete and re-run an adversarial pass against HEAD before merging.Follow-ups
icaclsverification at HEAD. The manual Windows run is from3b528bd3fd; both modules it exercises changed afterwards. Thepr.ymlwin32 job covers this automatically now, but the result must be checked green before merge, and the ordering assertion (repairKnownPoisonedInstallDirBeforeWindowreturns before any window exists) is still only pinned by source assertions, not by a packaged launch.windows_install_dir_acl_repairbreadcrumb. It currently carriesstatusandfailedFileCountonly, so the 20s budget cannot be validated against the real population. AddingdurationMswould let the next release tell us whether 20s is generous or tight on real installs.describeInstallDirAclPoison()hands over theicaclscommands, but only through the renderer-recovery dialog — which needs a renderer that survives. A standard-user Program Files install that exhausts its 3 attempts has no path to that copy.gpu_fallback_withheld_install_dir_aclis a new breadcrumb with no dashboard. Worth adding to whatever monitors the GPU fallback so a regression in the suppression window is visible in aggregate rather than one bundle at a time.