diff --git a/src/main/startup/gpu-lifecycle.ts b/src/main/startup/gpu-lifecycle.ts index da852bcb491..78c051657d6 100644 --- a/src/main/startup/gpu-lifecycle.ts +++ b/src/main/startup/gpu-lifecycle.ts @@ -16,6 +16,7 @@ import { promptForGpuFallbackRestart } from '../crash-reporting/gpu-fallback-res import { engageGpuFallbackAfterCrashBurst } from '../crash-reporting/gpu-fallback-engagement' import { recordCrashBreadcrumb } from '../crash-reporting/crash-breadcrumb-store' import { recordDurableCrashBreadcrumb } from '../crash-reporting/durable-crash-breadcrumb' +import { isInstallDirAclSuspect } from './windows-install-dir-acl-recovery' import { mainProcessState as state, gpuFallbackEnvironment } from './main-process-state' import { createGpuAccelerationAboutPanelOptions } from '../menu/gpu-acceleration-about-panel' @@ -124,6 +125,12 @@ export async function handleGpuChildCrash( if (state.gpuFallbackActiveThisLaunch || state.isQuitting || state.isServeMode) { return } + // Why: a poisoned install DACL kills the GPU child exactly like a bad driver, but + // safe graphics does not rescue it and --in-process-gpu removes the GPU child, so + // every later crash loses the sibling deaths that identify the real cause. + if (isInstallDirAclSuspect()) { + return + } const result = state.gpuCrashFallbackTracker.recordGpuCrash(crashedAt) if (!result.shouldEngageFallback) { return diff --git a/src/main/startup/main-process-runtime-launch.ts b/src/main/startup/main-process-runtime-launch.ts index 9df28ecea8c..0ff42fd58bf 100644 --- a/src/main/startup/main-process-runtime-launch.ts +++ b/src/main/startup/main-process-runtime-launch.ts @@ -24,6 +24,7 @@ import { import { prepareCodexRuntimeHomeForLaunch } from './codex-launch-preparation' import { prepareCodexSessionResumeForLaunch } from './codex-session-resume-launch' import { startWindowsDesktopBeforeShellPathReady } from './windows-desktop-shell-path-startup' +import { repairKnownPoisonedInstallDirBeforeWindow } from './windows-install-dir-acl-recovery' import { registerServeSignalHandlers } from './serve-signal-handlers' import { settleServeDesktopActivation } from './serve-desktop-activation' import { @@ -292,6 +293,17 @@ export async function initializeMainProcessRuntimeLaunch( // Why published: the renderer's git-environment barrier must fence on the same // generation the terminal startup services wait for, not a later re-read. state.shellPathReady = shellPathReady + // Why before any window: the poisoned install DACL kills the renderer at init, and + // the probe that detects it cannot finish before createMainWindow. Bounded, and a + // no-op (one absent-file read) unless a previous launch already recorded the verdict. + const aclGate = await repairKnownPoisonedInstallDirBeforeWindow({ + isServeMode: state.isServeMode || serveOptions !== null, + userDataPath: app.getPath('userData'), + appVersion: app.getVersion() + }) + if (aclGate !== 'not-marked' && aclGate !== 'skipped') { + logStartupMilestone('install-dir-acl-repair-blocking-done', { mode: aclGate }) + } let desktopWindow: BrowserWindow | null = null if (process.platform === 'win32' && app.isPackaged && !serveOptions) { const desktopStartup = startWindowsDesktopBeforeShellPathReady({ diff --git a/src/main/startup/main-window-controller.ts b/src/main/startup/main-window-controller.ts index 0d935d5f84a..a569c15e2f4 100644 --- a/src/main/startup/main-window-controller.ts +++ b/src/main/startup/main-window-controller.ts @@ -10,7 +10,10 @@ import { resolveConsent } from '../telemetry/consent' import { trackAppOpenedOnce } from '../telemetry/client' import { ensureWindowsUserDataAclGrant } from './windows-user-data-acl' import { probeWindowsInstallDirAcl } from './windows-install-dir-acl-probe' -import { startWindowsInstallDirAclRepairIfPoisoned } from './windows-install-dir-acl-recovery' +import { + noteWindowsInstallDirAclProbePending, + startWindowsInstallDirAclRepairIfPoisoned +} from './windows-install-dir-acl-recovery' import { logStartupMilestone } from './startup-diagnostics' import { notifyMainWindowBecameVisible } from '../window/main-window-visibility' import { setTrayAttention } from '../tray/system-tray' @@ -74,6 +77,9 @@ export function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {} }) // Why here: read-only, and the install DACL is the one thing a 0x80000003 // child death cannot tell us about itself. See electron/electron#51761. + if (!state.isServeMode) { + noteWindowsInstallDirAclProbePending() + } probeWindowsInstallDirAcl({ isServeMode: state.isServeMode, onDone: (data) => diff --git a/src/main/startup/windows-install-dir-acl-poison-marker.ts b/src/main/startup/windows-install-dir-acl-poison-marker.ts new file mode 100644 index 00000000000..46035e04e74 --- /dev/null +++ b/src/main/startup/windows-install-dir-acl-poison-marker.ts @@ -0,0 +1,77 @@ +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' + +/** + * "This install directory was found poisoned and has not been proven healthy since." + * + * Why a separate marker from `windows-install-dir-acl-repair.json`: that one is + * written after an attempt finishes, so a launch the poison kills mid-repair + * leaves no state at all and the next launch repeats the whole late-repair dance. + * This one is written the moment the probe's verdict lands, and it is the only + * thing that lets a later launch know it is poisoned *before* it creates a window + * — the probe itself cannot answer that early. Same tiny synchronous-JSON shape + * as `gpu-fallback-marker.ts`, for the same reason. + */ + +export const WINDOWS_INSTALL_DIR_ACL_POISON_MARKER_FILE = 'windows-install-dir-acl-poison.json' +export const WINDOWS_INSTALL_DIR_ACL_POISON_SCHEME_VERSION = 1 + +type PoisonMarker = { + schemeVersion: number + installDir: string + appVersion: string + detectedAt: number +} + +function markerPath(userDataPath: string): string { + return join(userDataPath, WINDOWS_INSTALL_DIR_ACL_POISON_MARKER_FILE) +} + +/** Keyed on both: a reinstall elsewhere or an update ships files with a fresh DACL. */ +export function hasInstallDirAclPoisonMarker( + userDataPath: string, + installDir: string, + appVersion: string +): boolean { + try { + const parsed = JSON.parse(readFileSync(markerPath(userDataPath), 'utf-8')) as + | Partial + | undefined + return ( + parsed?.schemeVersion === WINDOWS_INSTALL_DIR_ACL_POISON_SCHEME_VERSION && + parsed.installDir === installDir && + parsed.appVersion === appVersion + ) + } catch { + return false // missing or corrupt -> treat the install as healthy + } +} + +export function writeInstallDirAclPoisonMarker( + userDataPath: string, + installDir: string, + appVersion: string +): void { + const marker: PoisonMarker = { + schemeVersion: WINDOWS_INSTALL_DIR_ACL_POISON_SCHEME_VERSION, + installDir, + appVersion, + detectedAt: Date.now() + } + try { + if (!existsSync(userDataPath)) { + mkdirSync(userDataPath, { recursive: true }) + } + writeFileSync(markerPath(userDataPath), JSON.stringify(marker)) + } catch { + // Best effort: without it the next launch just falls back to today's late repair. + } +} + +export function clearInstallDirAclPoisonMarker(userDataPath: string): void { + try { + rmSync(markerPath(userDataPath), { force: true }) + } catch { + // Best effort; a stale marker only costs one redundant icacls pass. + } +} diff --git a/src/main/startup/windows-install-dir-acl-recovery.test.ts b/src/main/startup/windows-install-dir-acl-recovery.test.ts index 2b5d00ff40a..cd99236fc61 100644 --- a/src/main/startup/windows-install-dir-acl-recovery.test.ts +++ b/src/main/startup/windows-install-dir-acl-recovery.test.ts @@ -1,16 +1,23 @@ -import { mkdtempSync } from 'node:fs' +import { mkdtempSync, readFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { beforeEach, describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import type { ProcessResult, ProcessSpec } from '../../shared/child-process/run-process' +import type { CrashReportBreadcrumbData } from '../../shared/crash-reporting' +import { readActiveGpuFallbackMarker, writeGpuFallbackMarker } from './gpu-fallback-marker' import { probeWindowsInstallDirAcl, resetWindowsInstallDirAclProbeForTest } from './windows-install-dir-acl-probe' +import { hasInstallDirAclPoisonMarker } from './windows-install-dir-acl-poison-marker' import { describeInstallDirAclPoison, + isInstallDirAclSuspect, + noteWindowsInstallDirAclProbePending, + repairKnownPoisonedInstallDirBeforeWindow, resetWindowsInstallDirAclRecoveryForTest, - startWindowsInstallDirAclRepairIfPoisoned + startWindowsInstallDirAclRepairIfPoisoned, + type WindowsInstallDirAclRecoveryOptions } from './windows-install-dir-acl-recovery' import { resetWindowsInstallDirAclRepairForTest } from './windows-install-dir-package-acl-repair' import { @@ -26,6 +33,8 @@ import { const INSTALL_DIR = 'C:\\Users\\neil\\AppData\\Local\\Programs\\orca' const APP_VERSION = '1.4.184' +type Runner = (spec: ProcessSpec) => Promise + /** * Drives the production path: the real probe hands its verdict to the real gate, * which decides whether icacls ever runs. Only the two process seams are faked. @@ -185,3 +194,268 @@ describe('describeInstallDirAclPoison', () => { expect(describeInstallDirAclPoison()?.detail).toContain('repairing the permissions now') }) }) + +const POISON_VERDICT: CrashReportBreadcrumbData = { + status: 'ok', + matchesPoisonSignature: true, + wellKnownNameCheckReliable: true +} +const GPU_ENV = { appVersion: APP_VERSION, electronVersion: '43.4.1', platform: 'win32' } as const + +function recoveryOptions(userDataPath: string, run: Runner): WindowsInstallDirAclRecoveryOptions { + return { + platform: 'win32', + installDir: INSTALL_DIR, + appVersion: APP_VERSION, + userDataPath, + runProcessFn: run as never, + recordBreadcrumb: () => undefined + } +} + +/** icacls' real success summary, as the repair's parser expects it. */ +const okRun: Runner = async () => ({ + code: 0, + signal: null, + stdout: 'Successfully processed 3200 files; Failed processing 0 files', + stderr: '', + timedOut: false +}) + +describe('install-dir ACL repair vs the GPU safe-graphics marker', () => { + beforeEach(() => { + resetWindowsInstallDirAclProbeForTest() + resetWindowsInstallDirAclRepairForTest() + resetWindowsInstallDirAclRecoveryForTest() + }) + + it('clears the sticky safe-graphics marker once the real cause is repaired', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-gpu-')) + // The machine is in the reproduced state: the poisoned install DACL killed the + // GPU child three times, so Orca latched safe graphics for this build. + writeGpuFallbackMarker( + userDataPath, + { engagedAt: Date.now(), crashesInWindow: 3, userConfirmed: false }, + GPU_ENV + ) + expect(readActiveGpuFallbackMarker(userDataPath, GPU_ENV)).not.toBeNull() + + await new Promise((resolve) => { + startWindowsInstallDirAclRepairIfPoisoned(POISON_VERDICT, { + ...recoveryOptions(userDataPath, okRun), + // Settles after the repair's own setImmediate hop and its two icacls passes. + recordBreadcrumb: () => { + setTimeout(resolve, 0) + return undefined + } + }) + }) + + expect(describeInstallDirAclPoison()?.detail).toContain('repaired the permissions') + // The GPU child deaths were never a driver fault, so safe graphics — and the + // --in-process-gpu launch that hides the next crash's evidence — must not outlive the repair. + expect(readActiveGpuFallbackMarker(userDataPath, GPU_ENV)).toBeNull() + }) +}) + +describe('isInstallDirAclSuspect', () => { + beforeEach(() => { + resetWindowsInstallDirAclProbeForTest() + resetWindowsInstallDirAclRepairForTest() + resetWindowsInstallDirAclRecoveryForTest() + }) + + it('is false when nothing has suggested the install DACL is involved', () => { + expect(isInstallDirAclSuspect()).toBe(false) + }) + + // The GPU child dies ~74ms in and the probe answers 0.9-3.0s later, so "no verdict + // yet" is the entire window in which the misdiagnosis happens. + it('holds while the probe verdict is outstanding, and releases on a clean verdict', () => { + noteWindowsInstallDirAclProbePending() + expect(isInstallDirAclSuspect()).toBe(true) + + startWindowsInstallDirAclRepairIfPoisoned( + { status: 'ok', matchesPoisonSignature: false }, + recoveryOptions(mkdtempSync(join(tmpdir(), 'orca-acl-suspect-')), okRun) + ) + expect(isInstallDirAclSuspect()).toBe(false) + }) + + it('releases once the wait exceeds the grace window, so a silent probe cannot pin it', () => { + noteWindowsInstallDirAclProbePending() + expect(isInstallDirAclSuspect(Date.now() + 14_000)).toBe(true) + expect(isInstallDirAclSuspect(Date.now() + 16_000)).toBe(false) + }) + + it('holds through a repair that failed, and releases once one succeeds', async () => { + const failing: Runner = async () => ({ + code: 5, + signal: null, + stdout: '', + stderr: 'Access is denied.', + timedOut: false + }) + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-suspect-')) + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(userDataPath, failing) + ) + expect(isInstallDirAclSuspect()).toBe(true) + await vi.waitFor(() => expect(describeInstallDirAclPoison()?.detail).toContain('could not')) + // Still suspect: the tree is proven poisoned, and safe graphics does not rescue it. + expect(isInstallDirAclSuspect()).toBe(true) + + resetWindowsInstallDirAclRecoveryForTest() + resetWindowsInstallDirAclRepairForTest() + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(mkdtempSync(join(tmpdir(), 'orca-acl-suspect-')), okRun) + ) + await vi.waitFor(() => expect(isInstallDirAclSuspect()).toBe(false)) + }) +}) + +describe('repairKnownPoisonedInstallDirBeforeWindow', () => { + beforeEach(() => { + resetWindowsInstallDirAclProbeForTest() + resetWindowsInstallDirAclRepairForTest() + resetWindowsInstallDirAclRecoveryForTest() + }) + + it('costs a healthy machine one absent-file read and no icacls', async () => { + const specs: ProcessSpec[] = [] + const run: Runner = async (spec) => { + specs.push(spec) + return okRun(spec) + } + const mode = await repairKnownPoisonedInstallDirBeforeWindow( + recoveryOptions(mkdtempSync(join(tmpdir(), 'orca-acl-gate-')), run) + ) + expect(mode).toBe('not-marked') + expect(specs).toHaveLength(0) + }) + + // The crash this fixes: launch 1 detects the poison but createMainWindow already + // ran, so the renderer is dead before icacls is spawned. Launch 2 must not repeat it. + it('repairs a launch that a previous one recorded as poisoned, before returning', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-gate-')) + // Launch 1: the probe reports poison and the app dies mid-repair. + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(userDataPath, (() => new Promise(() => undefined)) as Runner) + ) + expect(hasInstallDirAclPoisonMarker(userDataPath, INSTALL_DIR, APP_VERSION)).toBe(true) + + // Launch 2. + resetWindowsInstallDirAclRecoveryForTest() + resetWindowsInstallDirAclRepairForTest() + writeGpuFallbackMarker( + userDataPath, + { engagedAt: Date.now(), crashesInWindow: 3, userConfirmed: false }, + GPU_ENV + ) + const specs: ProcessSpec[] = [] + const run: Runner = async (spec) => { + specs.push(spec) + return okRun(spec) + } + const mode = await repairKnownPoisonedInstallDirBeforeWindow(recoveryOptions(userDataPath, run)) + expect(mode).toBe('repaired') + // Both passes have already run by the time the window may be created. + expect(specs.map((spec) => spec.args?.[2])).toEqual([ + '*S-1-15-2-2:(OI)(CI)(RX)', + '*S-1-15-2-2:(RX)' + ]) + expect(hasInstallDirAclPoisonMarker(userDataPath, INSTALL_DIR, APP_VERSION)).toBe(false) + expect(readActiveGpuFallbackMarker(userDataPath, GPU_ENV)).toBeNull() + }) + + it('gives up on its budget rather than holding the window open forever', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-gate-')) + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(userDataPath, (() => new Promise(() => undefined)) as Runner) + ) + resetWindowsInstallDirAclRecoveryForTest() + resetWindowsInstallDirAclRepairForTest() + + const mode = await repairKnownPoisonedInstallDirBeforeWindow({ + ...recoveryOptions(userDataPath, (() => new Promise(() => undefined)) as Runner), + timeoutMs: 20 + }) + expect(mode).toBe('timeout') + }) + + it('is a no-op off win32 and in serve mode', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-gate-')) + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(userDataPath, (() => new Promise(() => undefined)) as Runner) + ) + resetWindowsInstallDirAclRecoveryForTest() + resetWindowsInstallDirAclRepairForTest() + + expect( + await repairKnownPoisonedInstallDirBeforeWindow({ + ...recoveryOptions(userDataPath, okRun), + platform: 'darwin' + }) + ).toBe('skipped') + expect( + await repairKnownPoisonedInstallDirBeforeWindow({ + ...recoveryOptions(userDataPath, okRun), + isServeMode: true + }) + ).toBe('skipped') + }) + + it('retires the marker when a later probe reports the install clean', () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-gate-')) + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(userDataPath, (() => new Promise(() => undefined)) as Runner) + ) + expect(hasInstallDirAclPoisonMarker(userDataPath, INSTALL_DIR, APP_VERSION)).toBe(true) + + resetWindowsInstallDirAclRecoveryForTest() + startWindowsInstallDirAclRepairIfPoisoned( + { status: 'ok', matchesPoisonSignature: false }, + recoveryOptions(userDataPath, okRun) + ) + expect(hasInstallDirAclPoisonMarker(userDataPath, INSTALL_DIR, APP_VERSION)).toBe(false) + }) + + // An unreadable DACL is not evidence of health; forgetting the verdict there would + // hand the next launch straight back to the crash it already recorded. + it('keeps the marker when the probe could not read the DACL', () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-acl-gate-')) + startWindowsInstallDirAclRepairIfPoisoned( + POISON_VERDICT, + recoveryOptions(userDataPath, (() => new Promise(() => undefined)) as Runner) + ) + resetWindowsInstallDirAclRecoveryForTest() + startWindowsInstallDirAclRepairIfPoisoned( + { status: 'failed', reason: 'all-targets-unreadable' }, + recoveryOptions(userDataPath, okRun) + ) + expect(hasInstallDirAclPoisonMarker(userDataPath, INSTALL_DIR, APP_VERSION)).toBe(true) + }) +}) + +/** + * Why a source assertion: gpu-lifecycle's transitive import graph reaches the real + * `electron` binding, so the guard cannot be driven in-process. This pins the one + * thing that matters — the ACL verdict is consulted before the crash is counted + * towards the burst that latches safe graphics. + */ +describe('handleGpuChildCrash call site', () => { + it('consults the install-dir ACL verdict before counting the crash', () => { + const source = readFileSync(join(__dirname, 'gpu-lifecycle.ts'), 'utf8') + const start = source.indexOf('export async function handleGpuChildCrash') + const countIndex = source.indexOf('recordGpuCrash(', start) + expect(start).toBeGreaterThanOrEqual(0) + expect(countIndex).toBeGreaterThan(start) + expect(source.slice(start, countIndex)).toContain('isInstallDirAclSuspect()') + }) +}) diff --git a/src/main/startup/windows-install-dir-acl-recovery.ts b/src/main/startup/windows-install-dir-acl-recovery.ts index 0aa6870192e..15516ff5843 100644 --- a/src/main/startup/windows-install-dir-acl-recovery.ts +++ b/src/main/startup/windows-install-dir-acl-recovery.ts @@ -1,6 +1,12 @@ import { dirname } from 'node:path' import type { CrashReportBreadcrumbData } from '../../shared/crash-reporting' import { logStartupMilestone } from './startup-diagnostics' +import { clearGpuFallbackMarker } from './gpu-fallback-marker' +import { + clearInstallDirAclPoisonMarker, + hasInstallDirAclPoisonMarker, + writeInstallDirAclPoisonMarker +} from './windows-install-dir-acl-poison-marker' import { buildInstallDirAclRepairCommands, isInstallDirAclPoisonVerdict, @@ -25,10 +31,62 @@ export type WindowsInstallDirAclRecoveryOptions = Omit void +): void { + poison = { installDir, stage: 'pending' } + writeInstallDirAclPoisonMarker(options.userDataPath, installDir, options.appVersion) + repairWindowsInstallDirPackageAcl({ + ...options, + installDir, + onDone: (result) => { + poison = { installDir, stage: result.mode } + logStartupMilestone('install-dir-acl-repair-done', { mode: result.mode }) + if (result.mode === 'repaired') { + clearInstallDirAclPoisonMarker(options.userDataPath) + // The GPU child deaths were never a driver fault, so safe graphics — and the + // --in-process-gpu launch that hides the next crash's evidence — must not outlive the repair. + clearGpuFallbackMarker(options.userDataPath) + } + if (result.mode === 'failed') { + console.warn('[win32-acl] install dir package ACL repair failed:', result.reason) + } + onDone?.(result) + } + }) } /** The probe's `onDone`: no-op unless the machine is in the reproduced state. */ @@ -36,21 +94,52 @@ export function startWindowsInstallDirAclRepairIfPoisoned( data: CrashReportBreadcrumbData, options: WindowsInstallDirAclRecoveryOptions ): void { + probePendingSince = null if (!isInstallDirAclPoisonVerdict(data)) { + // Only a positive clean reading retires the marker; an unreadable DACL proves nothing. + if (data.matchesPoisonSignature === false) { + clearInstallDirAclPoisonMarker(options.userDataPath) + } return } + // The blocking pre-window gate may already own this launch's repair; restarting it + // would reset the verdict to 'pending' against a repair that can no longer report. + if (poison) { + return + } + startRepair(options.installDir ?? dirname(process.execPath), options) +} + +/** + * Pre-window gate for a machine a previous launch already found poisoned. + * + * Why blocking, and why only here: the probe is `setImmediate`-deferred and takes + * 0.9-3.0s on the affected hosts, while the renderer it has to save is spawned + * synchronously by `createMainWindow` and dies at init 48-1373ms in. The + * persisted verdict is what buys that knowledge for free — a healthy machine + * reads one absent file and pays nothing. + */ +export async function repairKnownPoisonedInstallDirBeforeWindow( + options: WindowsInstallDirAclRecoveryOptions & { timeoutMs?: number } +): Promise<'not-marked' | 'skipped' | WindowsInstallDirAclRepairResult['mode'] | 'timeout'> { + if ((options.platform ?? process.platform) !== 'win32' || options.isServeMode === true) { + return 'skipped' + } const installDir = options.installDir ?? dirname(process.execPath) - poison = { installDir, stage: 'pending' } - repairWindowsInstallDirPackageAcl({ - ...options, - installDir, - onDone: (result) => { - poison = { installDir, stage: result.mode } - logStartupMilestone('install-dir-acl-repair-done', { mode: result.mode }) - if (result.mode === 'failed') { - console.warn('[win32-acl] install dir package ACL repair failed:', result.reason) - } - } + if (!hasInstallDirAclPoisonMarker(options.userDataPath, installDir, options.appVersion)) { + return 'not-marked' + } + logStartupMilestone('install-dir-acl-repair-blocking-start') + return await new Promise((resolve) => { + const timer = setTimeout( + () => resolve('timeout'), + options.timeoutMs ?? BLOCKING_REPAIR_BUDGET_MS + ) + timer.unref?.() + startRepair(installDir, options, (result) => { + clearTimeout(timer) + resolve(result.mode) + }) }) } diff --git a/src/main/startup/windows-install-dir-package-acl-repair.test.ts b/src/main/startup/windows-install-dir-package-acl-repair.test.ts index 65d11e9fcde..f53ead860c1 100644 --- a/src/main/startup/windows-install-dir-package-acl-repair.test.ts +++ b/src/main/startup/windows-install-dir-package-acl-repair.test.ts @@ -233,6 +233,38 @@ describe('repairWindowsInstallDirPackageAcl', () => { expect(marker.outcome).toBe('failed') }) + // The bricking mechanism: a marker was written on failure and matched regardless of + // outcome, so one Defender-locked file or one timeout pinned the machine to + // 'marker-hit' — repair permanently skipped — for the life of that version. + it('retries a failed repair on later launches, then stops once the budget is spent', async () => { + const userDataPath = userDataDir() + const failing = fakeRunner(() => ({ code: 5, stderr: 'Access is denied.' })) + for (let attempt = 0; attempt < 3; attempt++) { + resetWindowsInstallDirAclRepairForTest() + expect((await repair({ userDataPath, run: failing.run })).result.mode).toBe('failed') + } + expect(failing.specs).toHaveLength(6) + + resetWindowsInstallDirAclRepairForTest() + const spent = fakeRunner() + const { result } = await repair({ userDataPath, run: spent.run }) + expect(result).toEqual({ mode: 'marker-hit' }) + expect(spent.specs).toHaveLength(0) + }) + + it('stops retrying immediately once a repair has succeeded', async () => { + const userDataPath = userDataDir() + resetWindowsInstallDirAclRepairForTest() + await repair({ userDataPath, run: fakeRunner(() => ({ code: 5 })).run }) + resetWindowsInstallDirAclRepairForTest() + expect((await repair({ userDataPath })).result).toEqual({ mode: 'repaired' }) + + resetWindowsInstallDirAclRepairForTest() + const after = fakeRunner() + expect((await repair({ userDataPath, run: after.run })).result).toEqual({ mode: 'marker-hit' }) + expect(after.specs).toHaveLength(0) + }) + it('is a no-op off win32 and in serve mode', async () => { const off = fakeRunner() repairWindowsInstallDirPackageAcl({ diff --git a/src/main/startup/windows-install-dir-package-acl-repair.ts b/src/main/startup/windows-install-dir-package-acl-repair.ts index 606edae417c..cd29ff299e5 100644 --- a/src/main/startup/windows-install-dir-package-acl-repair.ts +++ b/src/main/startup/windows-install-dir-package-acl-repair.ts @@ -81,8 +81,17 @@ type RepairMarker = { appVersion: string attemptedAt: number outcome: string + /** Absent on schemeVersion-1 markers written before the retry budget existed. */ + attempts?: number } +// Why bounded rather than one-and-done: the failure modes are not all permanent. +// A Defender-locked file, a timeout or a contended volume fails one launch and +// succeeds the next, and pinning on the first failure leaves the machine blank +// forever for that version. Three is enough to stop a standard-user Program Files +// install — which can never win — from re-spawning icacls on every launch. +const MAX_REPAIR_ATTEMPTS = 3 + /** * The probe's verdict is the only trigger: an orphan package ACE with no * well-known package grant to satisfy it. A localized icacls prints those grants @@ -106,31 +115,43 @@ function markerPath(userDataPath: string): string { return join(userDataPath, WINDOWS_INSTALL_DIR_ACL_REPAIR_MARKER_FILE) } -function hasMarkerFor(args: WindowsInstallDirAclRepairArgs): boolean { +/** The marker for this exact install and version, or null. */ +function readMarkerFor(args: WindowsInstallDirAclRepairArgs): Partial | null { try { const parsed = JSON.parse(readFileSync(markerPath(args.userDataPath), 'utf-8')) as | Partial | undefined - return ( - parsed?.schemeVersion === WINDOWS_INSTALL_DIR_ACL_REPAIR_SCHEME_VERSION && - parsed.installDir === args.installDir && - parsed.appVersion === args.appVersion - ) + if ( + parsed?.schemeVersion !== WINDOWS_INSTALL_DIR_ACL_REPAIR_SCHEME_VERSION || + parsed.installDir !== args.installDir || + parsed.appVersion !== args.appVersion + ) { + return null + } + return parsed } catch { - return false // missing or corrupt -> attempt again + return null // missing or corrupt -> attempt again } } -// Why write it on failure too: a standard-user Program Files install can never -// win, and re-spawning icacls on every launch forever buys nothing. Reinstall or -// update changes the key and retries. +function hasMarkerFor(args: WindowsInstallDirAclRepairArgs): boolean { + const marker = readMarkerFor(args) + if (!marker) { + return false + } + return marker.outcome === 'repaired' || (marker.attempts ?? 0) >= MAX_REPAIR_ATTEMPTS +} + +// Why write it on failure too: re-spawning icacls on every launch forever buys +// nothing, so failures spend the retry budget. Reinstall or update changes the key. function writeMarker(args: WindowsInstallDirAclRepairArgs, outcome: string): void { const marker: RepairMarker = { schemeVersion: WINDOWS_INSTALL_DIR_ACL_REPAIR_SCHEME_VERSION, installDir: args.installDir ?? '', appVersion: args.appVersion, attemptedAt: Date.now(), - outcome + outcome, + attempts: (readMarkerFor(args)?.attempts ?? 0) + 1 } if (!existsSync(args.userDataPath)) { mkdirSync(args.userDataPath, { recursive: true })