diff --git a/src/main/crash-reporting/gpu-fallback-engagement.test.ts b/src/main/crash-reporting/gpu-fallback-engagement.test.ts new file mode 100644 index 00000000000..29db1b2d70d --- /dev/null +++ b/src/main/crash-reporting/gpu-fallback-engagement.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, it, vi } from 'vitest' +import { + engageGpuFallback, + shouldKeepProvisionalGpuFallbackMarker, + type GpuFallbackEngagementDeps, + type GpuFallbackEngagementOutcome +} from './gpu-fallback-engagement' + +/** Fake disk so tests assert the marker's real state, not just call counts. */ +function createDeps( + overrides: Partial & { clearSucceeds?: boolean } = {} +) { + const { clearSucceeds = true, ...depOverrides } = overrides + const order: string[] = [] + const state = { markerOnDisk: false } + const deps: GpuFallbackEngagementDeps = { + persistMarker: vi.fn(() => { + order.push('persist') + state.markerOnDisk = true + }), + clearMarker: vi.fn(() => { + order.push('clear') + if (!clearSucceeds) { + return false + } + state.markerOnDisk = false + return true + }), + prompt: vi.fn(async () => { + order.push('prompt') + return 'restart' as const + }), + isQuitting: () => false, + onMarkerPersistFailed: vi.fn(), + onMarkerClearFailed: vi.fn(), + onPromptFailed: vi.fn(), + onRestartDeferred: vi.fn(), + ...depOverrides + } + return { deps, order, state } +} + +describe('engageGpuFallback', () => { + it('persists the marker before showing the prompt', async () => { + const { deps, order } = createDeps() + + await expect(engageGpuFallback(deps)).resolves.toBe('restart') + + // The whole point: Chromium can hard-kill the process while the modal is up. + expect(order).toEqual(['persist', 'prompt']) + }) + + it('persists the marker synchronously, before the first await yields', async () => { + const { deps, state } = createDeps() + + // Deliberately not awaited: any await added ahead of the write would defer it + // past Chromium's GPU CHECK, which is the exact bug this module exists to fix. + const pending = engageGpuFallback(deps) + expect(deps.persistMarker).toHaveBeenCalledOnce() + expect(state.markerOnDisk).toBe(true) + + await pending + }) + + it('has the marker on disk while the prompt is still unanswered', async () => { + const { deps, state } = createDeps({ prompt: vi.fn(() => new Promise(() => {})) }) + + const outcome = await Promise.race([ + engageGpuFallback(deps), + new Promise((resolve) => setTimeout(() => resolve('still-pending'), 10)) + ]) + + expect(outcome).toBe('still-pending') + // A kill landing here must find a marker the next launch can read. + expect(state.markerOnDisk).toBe(true) + }) + + it('clears the marker after the user chooses to keep running', async () => { + const { deps, order, state } = createDeps({ + prompt: vi.fn(async () => { + order.push('prompt') + return 'continue' as const + }) + }) + + await expect(engageGpuFallback(deps)).resolves.toBe('deferred') + + expect(order).toEqual(['persist', 'prompt', 'clear']) + expect(state.markerOnDisk).toBe(false) + expect(deps.onRestartDeferred).toHaveBeenCalledOnce() + expect(deps.onMarkerClearFailed).not.toHaveBeenCalled() + }) + + it('honors "keep running" even when a quit races the answer', async () => { + // Why: a tray/OS quit resolves the parented dialog; discarding the opt-out + // there would strand the user in safe-graphics mode they declined. + const { deps, state } = createDeps({ + prompt: vi.fn(async () => 'continue' as const), + isQuitting: () => true + }) + + await expect(engageGpuFallback(deps)).resolves.toBe('deferred') + + expect(state.markerOnDisk).toBe(false) + }) + + it('keeps the caller armed when the decline could not be written through', async () => { + const { deps, state } = createDeps({ + prompt: vi.fn(async () => 'continue' as const), + clearSucceeds: false + }) + + // Why a distinct outcome: a plain 'deferred' lets the caller stand down and + // the declined marker then latches the user in on the next launch. + await expect(engageGpuFallback(deps)).resolves.toBe('deferred-uncleared') + + expect(state.markerOnDisk).toBe(true) + expect(deps.onMarkerClearFailed).toHaveBeenCalledOnce() + }) + + it('rejects an async persistMarker instead of prompting behind it', async () => { + // Why: `() => void` structurally accepts an async function, whose write + // would land after the kill window and whose rejection would escape. + const { deps } = createDeps({ persistMarker: vi.fn(async () => {}) }) + + await expect(engageGpuFallback(deps)).resolves.toBe('marker-failed') + + expect(deps.prompt).not.toHaveBeenCalled() + expect(deps.onMarkerPersistFailed).toHaveBeenCalledWith(expect.any(TypeError)) + }) + + it('keeps the marker when the prompt itself fails', async () => { + const error = new Error('no display') + const { deps, state } = createDeps({ + prompt: vi.fn(async () => { + throw error + }) + }) + + await expect(engageGpuFallback(deps)).resolves.toBe('latched') + + expect(deps.onPromptFailed).toHaveBeenCalledWith(error) + expect(state.markerOnDisk).toBe(true) + expect(deps.clearMarker).not.toHaveBeenCalled() + }) + + it('keeps a consented restart when a quit is already in flight', async () => { + const { deps, state } = createDeps({ isQuitting: () => true }) + + // Why not 'latched': the caller must not treat consented state as provisional + // and delete the marker on the way out. + await expect(engageGpuFallback(deps)).resolves.toBe('confirmed-quitting') + + expect(state.markerOnDisk).toBe(true) + expect(deps.clearMarker).not.toHaveBeenCalled() + }) + + it('keeps the shutdown retry armed only for unsettled outcomes', () => { + // Why exhaustive: the caller drops its retry on a false, so a wrong entry here + // either strands the user in declined safe graphics or deletes consented state. + const expected: Record = { + restart: false, + 'confirmed-quitting': false, + deferred: false, + 'deferred-uncleared': true, + latched: true, + 'marker-failed': false + } + for (const [outcome, keep] of Object.entries(expected)) { + expect([outcome, shouldKeepProvisionalGpuFallbackMarker(outcome as never)]).toEqual([ + outcome, + keep + ]) + } + }) + + it('does not prompt when the marker cannot be persisted', async () => { + const error = new Error('EACCES') + const { deps } = createDeps({ + persistMarker: vi.fn(() => { + throw error + }) + }) + + await expect(engageGpuFallback(deps)).resolves.toBe('marker-failed') + + // Why: a relaunch would come back on the same broken GPU, so offering one lies. + expect(deps.prompt).not.toHaveBeenCalled() + expect(deps.onMarkerPersistFailed).toHaveBeenCalledWith(error) + }) +}) diff --git a/src/main/crash-reporting/gpu-fallback-engagement.ts b/src/main/crash-reporting/gpu-fallback-engagement.ts new file mode 100644 index 00000000000..a1f25c33923 --- /dev/null +++ b/src/main/crash-reporting/gpu-fallback-engagement.ts @@ -0,0 +1,99 @@ +import type { GpuFallbackRestartDecision } from './gpu-fallback-restart-prompt' + +/** + * Orders the steps that engage Windows safe-graphics mode. + * + * Why the marker is persisted BEFORE the prompt: Chromium terminates the whole + * browser process with a fatal CHECK ("GPU process isn't usable. Goodbye." -> + * STATUS_BREAKPOINT / 0x80000003) on the 6th GPU-process failure. Orca engages + * on the 3rd, so the modal restart prompt is racing a hard kill that lands ~2.5s + * later when the GPU crashes, and ~7ms later when it cannot launch at all. + * Persisting after the click meant an unattended machine never latched the + * fallback and crash-looped on every subsequent launch. + * + * The marker is therefore written without consent, so callers must drop it again + * on an orderly shutdown — see `shouldKeepProvisionalGpuFallbackMarker`. + */ +export type GpuFallbackEngagementDeps = { + /** Must be synchronous — an async write would miss the kill window entirely. */ + persistMarker: () => void + /** Returns false when the marker could not be removed. */ + clearMarker: () => boolean + prompt: () => Promise + isQuitting: () => boolean + onMarkerPersistFailed: (error: unknown) => void + onMarkerClearFailed: () => void + onPromptFailed: (error: unknown) => void + onRestartDeferred: () => void +} + +export type GpuFallbackEngagementOutcome = + /** Marker is on disk and the user asked to restart now. */ + | 'restart' + /** User confirmed, but a quit is already running it down; keep the marker, skip the relaunch. */ + | 'confirmed-quitting' + /** User chose to keep running; marker cleared, next launch keeps hardware GPU. */ + | 'deferred' + /** User declined but the marker is still on disk; the caller must keep retrying. */ + | 'deferred-uncleared' + /** Marker is on disk but unconfirmed; only an abnormal death should keep it. */ + | 'latched' + /** Marker could not be persisted, so neither answer could be honored. */ + | 'marker-failed' + +/** + * Whether the caller must keep retrying the marker delete on shutdown. + * + * Pure so the mapping is testable: `latched` never got an answer, and + * `deferred-uncleared` got a decline the delete could not honor. + */ +export function shouldKeepProvisionalGpuFallbackMarker( + outcome: GpuFallbackEngagementOutcome +): boolean { + return outcome === 'latched' || outcome === 'deferred-uncleared' +} + +export async function engageGpuFallback( + deps: GpuFallbackEngagementDeps +): Promise { + try { + // `() => void` structurally accepts an async function, whose write would land + // after the kill window and whose rejection would escape this catch. + const persisted = deps.persistMarker() as unknown + if (isThenable(persisted)) { + void (persisted as PromiseLike).then?.( + () => undefined, + () => undefined + ) + throw new TypeError('persistMarker must be synchronous') + } + } catch (error) { + // Without a durable marker a relaunch returns to the same broken GPU. + deps.onMarkerPersistFailed(error) + return 'marker-failed' + } + + let decision: GpuFallbackRestartDecision + try { + decision = await deps.prompt() + } catch (error) { + deps.onPromptFailed(error) + return 'latched' + } + + // The decision is honored before `isQuitting`: a tray/OS quit resolves the + // parented dialog, and dropping the answer there would ignore explicit consent. + if (decision !== 'restart') { + const cleared = deps.clearMarker() + if (!cleared) { + deps.onMarkerClearFailed() + } + deps.onRestartDeferred() + return cleared ? 'deferred' : 'deferred-uncleared' + } + return deps.isQuitting() ? 'confirmed-quitting' : 'restart' +} + +function isThenable(value: unknown): boolean { + return typeof (value as PromiseLike | undefined)?.then === 'function' +} diff --git a/src/main/index.ts b/src/main/index.ts index a80bea0d965..629e812a1f8 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -129,11 +129,17 @@ import { import { enableRendererHeapHeadroom } from './startup/renderer-heap-headroom' import { ensureVirtualDisplayForHeadlessServe } from './startup/ensure-virtual-display' import { + clearGpuFallbackMarker, readActiveGpuFallbackMarker, + sweepStaleGpuFallbackMarkerTempFiles, writeGpuFallbackMarker, type GpuFallbackEnvironment, type WindowsGpuFallbackEnvironment } from './startup/gpu-fallback-marker' +import { + engageGpuFallback, + shouldKeepProvisionalGpuFallbackMarker +} from './crash-reporting/gpu-fallback-engagement' import { applyGpuFallbackCommandLineSwitches } from './startup/gpu-fallback-switches' import { DEFAULT_GPU_CRASH_FALLBACK_THRESHOLD, @@ -141,10 +147,7 @@ import { GpuCrashFallbackTracker, isGpuFallbackCrashCandidate } from './crash-reporting/gpu-crash-fallback-decision' -import { - promptForGpuFallbackRestart, - type GpuFallbackRestartDecision -} from './crash-reporting/gpu-fallback-restart-prompt' +import { promptForGpuFallbackRestart } from './crash-reporting/gpu-fallback-restart-prompt' import { shouldSuppressDevEducation, suppressDevEducationForStore @@ -366,6 +369,9 @@ const gpuCrashFallbackTracker = new GpuCrashFallbackTracker({ threshold: DEFAULT_GPU_CRASH_FALLBACK_THRESHOLD }) let gpuFallbackActiveThisLaunch = false +// Why: the safe-graphics marker is written before the user answers, so it must be +// dropped again if the app reaches an orderly shutdown instead of a GPU kill. +let provisionalGpuFallbackUserDataPath: string | null = null let gpuFeatureStatus: Electron.GPUFeatureStatus | null = null let localPtyStartupReady: Promise = Promise.resolve() let localPtyProviderStartupReady: Promise = Promise.resolve() @@ -1250,6 +1256,9 @@ function openMainWindow(): BrowserWindow { } }) recordCrashBreadcrumb('main_window_created') + // Why: a Windows logoff/shutdown can end the session without reaching will-quit, + // and that reboot is exactly the driver-update case that must not latch safe graphics. + window.on('session-end', dropUnconfirmedGpuFallbackMarker) logStartupMilestone('window-created') // Why: Windows Tray construction can block synchronously on Shell_NotifyIcon, so both platforms defer creation to after first paint. let trayCreated = false @@ -1553,7 +1562,12 @@ function maybeApplyGpuFallbackForThisLaunch(): void { if (isServeMode || process.platform !== 'win32') { return } - const marker = readActiveGpuFallbackMarker(app.getPath('userData'), getGpuFallbackEnvironment()) + // Why canonical: handleGpuChildCrash writes through the same helper, and a late + // app.setName/setPath would silently split the read and write paths. + const userDataPath = getCanonicalUserDataPath() + // A kill between the marker's write and rename orphans a temp file next to it. + void sweepStaleGpuFallbackMarkerTempFiles(userDataPath).catch(() => undefined) + const marker = readActiveGpuFallbackMarker(userDataPath, getGpuFallbackEnvironment()) if (!marker) { return } @@ -1584,12 +1598,8 @@ async function handleGpuChildCrash(reason: string, exitCode: number | null): Pro crashesInWindow: result.crashesInWindow }) const engagedAt = Date.now() - const window = mainWindow && !mainWindow.isDestroyed() ? mainWindow : undefined - let restartDecision: GpuFallbackRestartDecision - try { - restartDecision = await promptForGpuFallbackRestart(window) - } catch (error) { - console.warn('[gpu-fallback] failed to show restart prompt:', error) + const environment = getWindowsGpuFallbackEnvironment() + if (!environment) { return } const fallbackData = { @@ -1597,28 +1607,55 @@ async function handleGpuChildCrash(reason: string, exitCode: number | null): Pro exitCode, crashesInWindow: result.crashesInWindow } - if (isQuitting) { - return + // Why canonical: the reader above resolves the same way, and a late app.setName + // would otherwise move the write out from under it. + const userDataPath = getCanonicalUserDataPath() + const window = mainWindow && !mainWindow.isDestroyed() ? mainWindow : undefined + // Why: Chromium fatally CHECKs the browser process on the 6th GPU failure, so + // the marker must land before the prompt or an unattended crash never latches. + const outcome = await engageGpuFallback({ + persistMarker: () => { + writeGpuFallbackMarker( + userDataPath, + { engagedAt, crashesInWindow: result.crashesInWindow }, + environment + ) + provisionalGpuFallbackUserDataPath = userDataPath + }, + clearMarker: () => clearGpuFallbackMarker(userDataPath), + prompt: () => promptForGpuFallbackRestart(window), + isQuitting: () => isQuitting, + onMarkerPersistFailed: (error) => { + console.warn('[gpu-fallback] failed to persist marker:', error) + recordDurableCrashBreadcrumb('gpu_fallback_marker_failed', fallbackData) + }, + onMarkerClearFailed: () => + recordDurableCrashBreadcrumb('gpu_fallback_marker_clear_failed', fallbackData), + onPromptFailed: (error) => { + console.warn('[gpu-fallback] failed to show restart prompt:', error) + // Why durable: this is the state the process is most likely to be killed in. + recordDurableCrashBreadcrumb('gpu_fallback_prompt_failed', fallbackData) + }, + onRestartDeferred: () => + recordDurableCrashBreadcrumb('gpu_fallback_restart_deferred', fallbackData) + }) + if (!shouldKeepProvisionalGpuFallbackMarker(outcome)) { + provisionalGpuFallbackUserDataPath = null } - if (restartDecision !== 'restart') { - recordDurableCrashBreadcrumb('gpu_fallback_restart_deferred', fallbackData) - return + if (outcome === 'confirmed-quitting') { + // Why re-persist: a session-end drop can land while the prompt is still open. + try { + writeGpuFallbackMarker( + userDataPath, + { engagedAt, crashesInWindow: result.crashesInWindow }, + environment + ) + } catch (error) { + console.warn('[gpu-fallback] failed to re-persist marker after consent:', error) + recordDurableCrashBreadcrumb('gpu_fallback_marker_failed', fallbackData) + } } - const environment = getWindowsGpuFallbackEnvironment() - if (!environment) { - return - } - try { - writeGpuFallbackMarker( - app.getPath('userData'), - { - engagedAt, - crashesInWindow: result.crashesInWindow - }, - environment - ) - } catch (error) { - console.warn('[gpu-fallback] failed to persist marker:', error) + if (outcome !== 'restart') { return } isQuitting = true @@ -1628,6 +1665,21 @@ async function handleGpuChildCrash(reason: string, exitCode: number | null): Pro app.exit(0) } +// Why: an orderly shutdown proves Chromium's GPU CHECK never landed, so a marker +// the user never confirmed must not survive into the next launch. +function dropUnconfirmedGpuFallbackMarker(): void { + if (provisionalGpuFallbackUserDataPath === null) { + return + } + if (!clearGpuFallbackMarker(provisionalGpuFallbackUserDataPath)) { + // `quit` is the last JS event, so this is terminal: the next launch starts in + // safe graphics and clears itself on the next Orca or Electron version. + recordDurableCrashBreadcrumb('gpu_fallback_marker_clear_failed', { phase: 'shutdown' }) + return + } + provisionalGpuFallbackUserDataPath = null +} + function recordProcessGoneCrash( source: 'renderer' | 'child', processType: string, @@ -2934,6 +2986,11 @@ app.on('before-quit', () => { rateLimits?.stop() }) +// Why 'quit', not will-quit: will-quit's first pass only means a quit was requested +// and then defers teardown for seconds, during which Chromium's GPU CHECK can still +// fire. Only 'quit' proves the process is leaving without that kill. +app.on('quit', dropUnconfirmedGpuFallbackMarker) + // Why: will-quit fires twice — first pass runs sync cleanup + preventDefault to await checkpoint writes; second pass exits. let daemonDisconnectDone = false app.on('will-quit', (e) => { diff --git a/src/main/startup/desktop-startup-ordering.test.ts b/src/main/startup/desktop-startup-ordering.test.ts index ec51c690ff6..c4873449f25 100644 --- a/src/main/startup/desktop-startup-ordering.test.ts +++ b/src/main/startup/desktop-startup-ordering.test.ts @@ -95,6 +95,104 @@ describe('startup ordering', () => { ) }) + it('persists the GPU fallback marker in the same turn as the GPU crash event', () => { + const source = readFileSync(join(process.cwd(), 'src/main/index.ts'), 'utf8') + const start = source.indexOf('async function handleGpuChildCrash(') + const end = source.indexOf('function recordProcessGoneCrash(', start) + const handler = source.slice(start, end) + const engageIndex = handler.indexOf('await engageGpuFallback({') + const persistIndex = handler.indexOf('persistMarker: () => {') + + expect(start).toBeGreaterThanOrEqual(0) + expect(end).toBeGreaterThan(start) + expect(engageIndex).toBeGreaterThanOrEqual(0) + expect(persistIndex).toBeGreaterThan(engageIndex) + + // Why the dispatch site too: the turn begins at child-process-gone, so an + // await introduced there would push the write past the kill just as surely. + const listenerStart = source.indexOf("app.on('child-process-gone'") + const listenerEnd = source.indexOf('void handleGpuChildCrash(', listenerStart) + expect(listenerStart).toBeGreaterThanOrEqual(0) + expect(listenerEnd).toBeGreaterThan(listenerStart) + const listener = source.slice(listenerStart, listenerEnd) + expect(listener).not.toMatch(/\bawait[\s(]/) + expect(listener).not.toContain('async (') + + // Why: Chromium fatally CHECKs the browser process on the 6th GPU failure — + // ~7ms after the first when it cannot launch. Any await before the marker + // write pushes it past that kill and restores the cross-launch crash loop. + expect(handler.slice(0, engageIndex)).not.toMatch(/\bawait[\s(]/) + + // Why: pin the write inside persistMarker — asserting only that the source + // mentions writeGpuFallbackMarker lets it drift into a later callback. + // Why bound on the next dep key, not the next `},` — the write's own object + // literal closes with `},` and would truncate the body being asserted. + const persistEnd = handler.indexOf('clearMarker:', persistIndex) + expect(persistEnd).toBeGreaterThan(persistIndex) + const persistBody = handler.slice(persistIndex, persistEnd) + expect(persistBody).toContain('writeGpuFallbackMarker(') + expect(persistBody).toContain('provisionalGpuFallbackUserDataPath = userDataPath') + // Why: an await inside persistMarker defers the write past the kill just as + // surely as one before the call. (An `async` callback fails the anchor above.) + expect(persistBody).not.toMatch(/\bawait[\s(]/) + + // Why exactly two writers: the pre-prompt persist, plus the re-persist that + // recovers a consented restart whose marker a session-end drop already took. + expect(handler.split('writeGpuFallbackMarker(')).toHaveLength(3) + const confirmedQuittingIndex = handler.indexOf("outcome === 'confirmed-quitting'") + expect(confirmedQuittingIndex).toBeGreaterThanOrEqual(0) + expect(handler.lastIndexOf('writeGpuFallbackMarker(')).toBeGreaterThan(confirmedQuittingIndex) + + // Why pinned: the outcome -> retry mapping is unit-tested, so the handler must + // defer to it rather than re-deriving the outcome list inline. + expect(handler).toContain('if (!shouldKeepProvisionalGpuFallbackMarker(outcome)) {') + }) + + it('drops an unconfirmed GPU fallback marker only once the quit is committed', () => { + const source = readFileSync(join(process.cwd(), 'src/main/index.ts'), 'utf8') + // Why not will-quit: its first pass only means a quit was requested, then defers + // teardown for seconds during which Chromium's GPU CHECK can still fire. + expect(source).toContain("app.on('quit', dropUnconfirmedGpuFallbackMarker)") + // Why session-end too: Windows logoff/shutdown is the driver-update reboot case. + expect(source).toContain("window.on('session-end', dropUnconfirmedGpuFallbackMarker)") + + // Why bound on the NEXT listener: bounding on an early statement inside the + // handler let a drop reintroduced further down pass unnoticed. + const willQuitStart = source.indexOf("app.on('will-quit'") + const willQuitEnd = source.indexOf("app.on('window-all-closed'", willQuitStart) + expect(willQuitStart).toBeGreaterThanOrEqual(0) + expect(willQuitEnd).toBeGreaterThan(willQuitStart) + expect(source.slice(willQuitStart, willQuitEnd)).not.toContain( + 'dropUnconfirmedGpuFallbackMarker' + ) + + // Why source-wide: the marker may only be dropped by the shutdown helper and + // by the explicit "Keep Running" answer. + expect(source.split('clearGpuFallbackMarker(')).toHaveLength(3) + }) + + it('reads and writes the GPU fallback marker through the same userData path', () => { + const source = readFileSync(join(process.cwd(), 'src/main/index.ts'), 'utf8') + const readerStart = source.indexOf('function maybeApplyGpuFallbackForThisLaunch()') + const readerEnd = source.indexOf('async function handleGpuChildCrash(', readerStart) + const writerEnd = source.indexOf('function recordProcessGoneCrash(', readerEnd) + + expect(readerStart).toBeGreaterThanOrEqual(0) + expect(readerEnd).toBeGreaterThan(readerStart) + expect(writerEnd).toBeGreaterThan(readerEnd) + + // Why: a split between reader and writer paths turns the whole fallback into + // a silent no-op — the marker lands somewhere the next launch never looks. + expect(source.slice(readerStart, readerEnd)).toContain('getCanonicalUserDataPath()') + // Why here: a kill between the marker's write and rename orphans a temp file, + // and this is the only launch-time site that reclaims them. + expect(source.slice(readerStart, readerEnd)).toContain( + 'sweepStaleGpuFallbackMarkerTempFiles(userDataPath)' + ) + expect(source.slice(readerEnd, writerEnd)).toContain('getCanonicalUserDataPath()') + expect(source.slice(readerStart, writerEnd)).not.toContain("app.getPath('userData')") + }) + it('reconciles retained Codex homes after authoritative daemon inventory', () => { const source = readFileSync(join(process.cwd(), 'src/main/index.ts'), 'utf8') const daemonInitIndex = source.indexOf('await initDaemonPtyProvider(signal') diff --git a/src/main/startup/gpu-fallback-marker.test.ts b/src/main/startup/gpu-fallback-marker.test.ts index 6aeacf8ec3e..b4d130285a2 100644 --- a/src/main/startup/gpu-fallback-marker.test.ts +++ b/src/main/startup/gpu-fallback-marker.test.ts @@ -1,12 +1,52 @@ -import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs' import os from 'node:os' import { join } from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as NodeFs from 'node:fs' +import type * as DurableFileWrite from '../durable-file-write' +// Why vi.mock, not vi.spyOn: the named ESM import is inlined, so a spy on the +// module namespace never intercepts and the failure cases silently pass. +const durableWrite = vi.hoisted(() => ({ failWith: null as Error | null, calls: 0 })) +// Why a counter, not a spy: same inlining problem, and rmSync's own maxRetries +// applies only to recursive removals, so the hand-rolled retry needs proving. +const rmControl = vi.hoisted(() => ({ failuresLeft: 0, calls: 0 })) +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + rmSync: (path: Parameters[0], options?: object) => { + rmControl.calls += 1 + if (rmControl.failuresLeft > 0) { + rmControl.failuresLeft -= 1 + throw Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }) + } + return actual.rmSync(path, options) + } + } +}) +vi.mock('../durable-file-write', async (importOriginal) => { + const actual = await importOriginal() + const fs = await import('node:fs') + return { + ...actual, + writeFileDurableSync: (tmpPath: string, finalPath: string, payload: string) => { + durableWrite.calls += 1 + if (durableWrite.failWith) { + // Why write first: the real failure is renameSync, which leaves the temp + // behind — throwing before it would leave the orphan cleanup uncovered. + fs.writeFileSync(tmpPath, payload, 'utf-8') + throw durableWrite.failWith + } + actual.writeFileDurableSync(tmpPath, finalPath, payload) + } + } +}) import { GPU_FALLBACK_MARKER_FILE, clearGpuFallbackMarker, readActiveGpuFallbackMarker, readGpuFallbackMarker, + sweepStaleGpuFallbackMarkerTempFiles, writeGpuFallbackMarker } from './gpu-fallback-marker' @@ -20,9 +60,15 @@ describe('gpu-fallback-marker', () => { beforeEach(() => { userDataPath = mkdtempSync(join(os.tmpdir(), 'orca-gpu-fallback-test-')) + durableWrite.failWith = null + durableWrite.calls = 0 + rmControl.failuresLeft = 0 + rmControl.calls = 0 }) afterEach(() => { + durableWrite.failWith = null + rmControl.failuresLeft = 0 rmSync(userDataPath, { recursive: true, force: true }) }) @@ -38,6 +84,42 @@ describe('gpu-fallback-marker', () => { }) }) + it('overwrites an existing marker and leaves no temp file behind', () => { + writeGpuFallbackMarker(userDataPath, { engagedAt: 1, crashesInWindow: 3 }, environment) + writeGpuFallbackMarker(userDataPath, { engagedAt: 2, crashesInWindow: 5 }, environment) + + expect(readGpuFallbackMarker(userDataPath)?.engagedAt).toBe(2) + // Why: the durable write renames through a temp file; an orphan would accumulate + // on every GPU crash burst. + expect(readdirSync(userDataPath).filter((name) => name.endsWith('.tmp'))).toEqual([]) + }) + + it('still writes the marker when the durable rename is refused', () => { + // Why: Windows AV/indexer holds make renameSync EPERM where a direct write + // lands. This path must never end up worse than a plain writeFileSync. + durableWrite.failWith = Object.assign(new Error('EPERM: rename'), { code: 'EPERM' }) + + writeGpuFallbackMarker(userDataPath, { engagedAt: 7, crashesInWindow: 3 }, environment) + + // Why assert the call: an ineffective mock would leave this green while + // covering nothing but the happy path. + expect(durableWrite.calls).toBe(1) + expect(readGpuFallbackMarker(userDataPath)?.engagedAt).toBe(7) + expect(readdirSync(userDataPath).filter((name) => name.endsWith('.tmp'))).toEqual([]) + }) + + it('reports the durable-write cause when the direct fallback also fails', () => { + const durableError = Object.assign(new Error('EPERM: rename'), { code: 'EPERM' }) + durableWrite.failWith = durableError + // A directory at the marker path makes the direct writeFileSync fail too. + mkdirSync(join(userDataPath, GPU_FALLBACK_MARKER_FILE)) + + // Why the durable error, not the fallback's: it names the real cause. + expect(() => + writeGpuFallbackMarker(userDataPath, { engagedAt: 9, crashesInWindow: 3 }, environment) + ).toThrow(durableError) + }) + it('returns null when no marker exists', () => { expect(readGpuFallbackMarker(userDataPath)).toBeNull() expect(readActiveGpuFallbackMarker(userDataPath, environment)).toBeNull() @@ -109,6 +191,85 @@ describe('gpu-fallback-marker', () => { expect(existsSync(join(userDataPath, GPU_FALLBACK_MARKER_FILE))).toBe(false) }) + it('sweeps temp files orphaned by a kill between write and rename', async () => { + writeGpuFallbackMarker(userDataPath, { engagedAt: 1, crashesInWindow: 3 }, environment) + // A foreign pid: the sweeper deliberately skips this process's own in-flight temps. + const orphan = join(userDataPath, `${GPU_FALLBACK_MARKER_FILE}.999999.123.abc.tmp`) + writeFileSync(orphan, '{}') + + await sweepStaleGpuFallbackMarkerTempFiles(userDataPath) + + expect(existsSync(orphan)).toBe(false) + // The marker itself shares the prefix and must survive the sweep. + expect(readGpuFallbackMarker(userDataPath)?.engagedAt).toBe(1) + }) + + it('retries a marker delete that is briefly refused', () => { + writeGpuFallbackMarker(userDataPath, { engagedAt: 1, crashesInWindow: 3 }, environment) + rmControl.calls = 0 + rmControl.failuresLeft = 2 + + expect(clearGpuFallbackMarker(userDataPath)).toBe(true) + + expect(rmControl.calls).toBe(3) + expect(existsSync(join(userDataPath, GPU_FALLBACK_MARKER_FILE))).toBe(false) + }) + + it('reports a marker delete that never succeeds, after backing off', () => { + writeGpuFallbackMarker(userDataPath, { engagedAt: 1, crashesInWindow: 3 }, environment) + rmControl.failuresLeft = Number.MAX_SAFE_INTEGER + + const startedAt = Date.now() + expect(clearGpuFallbackMarker(userDataPath)).toBe(false) + const elapsed = Date.now() - startedAt + + // Why the timing: a hot spin of 4 immediate unlinks satisfies the call count + // identically but cannot outlast the AV hold the backoff exists for. + expect(elapsed).toBeGreaterThanOrEqual(100) + rmControl.failuresLeft = 0 + expect(existsSync(join(userDataPath, GPU_FALLBACK_MARKER_FILE))).toBe(true) + }) + + it('never blocks startup revalidation on an undeletable marker', () => { + writeGpuFallbackMarker(userDataPath, { engagedAt: 1, crashesInWindow: 3 }, environment) + rmControl.failuresLeft = Number.MAX_SAFE_INTEGER + + const startedAt = Date.now() + // Version mismatch: the revalidation path that runs pre-window on every launch. + expect( + readActiveGpuFallbackMarker(userDataPath, { ...environment, appVersion: '9.9.9' }) + ).toBeNull() + const elapsed = Date.now() - startedAt + + // Why: this runs before the window exists, so an AV hold must not add a + // multi-hundred-millisecond stall for a delete the next launch redoes anyway. + expect(elapsed).toBeLessThan(100) + expect(rmControl.calls).toBe(1) + }) + + it('treats an already-gone marker as cleared even when the unlink errors', () => { + // Why: a concurrent delete makes rmSync throw on a file that is already gone; + // reporting that as a failure would keep the caller retrying forever. + rmControl.failuresLeft = Number.MAX_SAFE_INTEGER + + expect(clearGpuFallbackMarker(userDataPath)).toBe(true) + expect(rmControl.calls).toBe(1) + }) + + it('writes the marker before spending any time on the orphan temp', () => { + durableWrite.failWith = Object.assign(new Error('EPERM: rename'), { code: 'EPERM' }) + rmControl.failuresLeft = Number.MAX_SAFE_INTEGER + + const startedAt = Date.now() + writeGpuFallbackMarker(userDataPath, { engagedAt: 5, crashesInWindow: 3 }, environment) + const elapsed = Date.now() - startedAt + + expect(readGpuFallbackMarker(userDataPath)?.engagedAt).toBe(5) + // Why: backing off on the temp before the write would push the marker past + // the Chromium kill this whole path exists to survive. + expect(elapsed).toBeLessThan(100) + }) + it('can explicitly clear the marker', () => { writeGpuFallbackMarker(userDataPath, { engagedAt: 1, crashesInWindow: 4 }, environment) clearGpuFallbackMarker(userDataPath) diff --git a/src/main/startup/gpu-fallback-marker.ts b/src/main/startup/gpu-fallback-marker.ts index e94e654b3ec..c901f5113c4 100644 --- a/src/main/startup/gpu-fallback-marker.ts +++ b/src/main/startup/gpu-fallback-marker.ts @@ -1,5 +1,10 @@ import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { join } from 'node:path' +import { + durableWriteTempPath, + removeStaleDurableWriteTempFiles, + writeFileDurableSync +} from '../durable-file-write' /** * Persisted "disable hardware acceleration for this build" marker. @@ -80,14 +85,90 @@ export function writeGpuFallbackMarker( electronVersion: environment.electronVersion, platform: 'win32' } - writeFileSync(markerPath(userDataPath), JSON.stringify(marker)) + // Why: this write races Chromium's GPU kill (and often a driver TDR), so a bare + // writeFileSync can leave a truncated file that reads back as "no fallback". + const target = markerPath(userDataPath) + const payload = JSON.stringify(marker) + const tmp = durableWriteTempPath(target) + try { + writeFileDurableSync(tmp, target, payload) + } catch (durableError) { + // writeFileDurableSync cannot throw once its rename returned (the directory + // fsync swallows its own errors), so reaching here means nothing was written. + try { + // Why: renameSync is EPERM on Windows while an AV/indexer holds the target + // open, where a direct write still lands. Never end up worse than before. + writeFileSync(target, payload) + } catch { + // Why rethrow the durable error: it names the real cause (EPERM on rename). + throw durableError + } finally { + // Why after the write and without retries: the same handle that refused the + // rename usually refuses this unlink, and blocking here would push the + // marker past the kill it exists to survive. Orphans are swept next launch. + try { + rmSync(tmp, { force: true }) + } catch { + // Recoverable: sweepStaleGpuFallbackMarkerTempFiles reclaims it. + } + } + } } -export function clearGpuFallbackMarker(userDataPath: string): void { +/** Why: the kill this marker guards against can land between write and rename. */ +export async function sweepStaleGpuFallbackMarkerTempFiles(userDataPath: string): Promise { + await removeStaleDurableWriteTempFiles(markerPath(userDataPath)) +} + +/** + * Removes the marker, blocking for a short backoff if Windows refuses the unlink. + * Returns false when it is still on disk, so callers can report it. + * + * Only for callers with no second chance: the terminal `quit` event, and the + * prompt's "Keep Running" answer. Revalidation on the startup path must not + * block a window behind an AV hold — it uses `clearMarkerBestEffort`. + */ +export function clearGpuFallbackMarker(userDataPath: string): boolean { + return removeFileWithRetries(markerPath(userDataPath)) +} + +/** Why no retry: startup can just revalidate again on the next launch. */ +function clearMarkerBestEffort(userDataPath: string): void { try { rmSync(markerPath(userDataPath), { force: true }) } catch { - // best effort; a stale marker is revalidated on the next launch + // A stale marker is revalidated on the next launch. + } +} + +// Why hand-rolled: rmSync's maxRetries/retryDelay only apply to recursive removals, +// so a single-file unlink gets no retry at all. +const FILE_REMOVE_RETRY_DELAYS_MS = [20, 40, 80] + +function removeFileWithRetries(filePath: string): boolean { + for (let attempt = 0; ; attempt += 1) { + try { + rmSync(filePath, { force: true }) + return true + } catch { + if (!existsSync(filePath)) { + return true + } + if (attempt >= FILE_REMOVE_RETRY_DELAYS_MS.length) { + return false + } + sleepSync(FILE_REMOVE_RETRY_DELAYS_MS[attempt]) + } + } +} + +// Why Atomics.wait: `quit` and the deferred prompt path are synchronous, so a +// timer-based backoff would never run before the process leaves. +function sleepSync(milliseconds: number): void { + try { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, milliseconds) + } catch { + // SharedArrayBuffer unavailable: fall through and retry immediately. } } @@ -98,7 +179,7 @@ export function readActiveGpuFallbackMarker( const marker = readGpuFallbackMarker(userDataPath) if (!marker) { if (existsSync(markerPath(userDataPath))) { - clearGpuFallbackMarker(userDataPath) + clearMarkerBestEffort(userDataPath) } return null } @@ -110,7 +191,7 @@ export function readActiveGpuFallbackMarker( ) { // Why: the marker is sticky only for the build that observed the driver // crash burst; updates get one fresh hardware attempt automatically. - clearGpuFallbackMarker(userDataPath) + clearMarkerBestEffort(userDataPath) return null } return marker