From a4a730dd34569df0f08370260daa278db2ded346 Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 10 Sep 2026 22:12:08 -0700 Subject: [PATCH] fix(window): stop recovery reloads starting behind an unanswered recovery prompt `fail()` states the invariant "A pending prompt or crash recovery owns the next reload" and returns early when a prompt is standing. `issue()` did not check the same latch, so every fresh render-process-gone started another recovery reload behind the unanswered modal: invisible to the user, unable to recover a renderer the box is already about, and silently spending the circuit breaker's budget. Three related corrections: - `issue()` honours the prompt latch, like `fail()` already did. - `escalate()` lets a later 'crash-loop' verdict supersede a standing 'reload-stalled' one instead of dropping it. A native message box cannot be dismissed programmatically, so the corrected verdict is recorded via `supersedesStandingPrompt` and no second box is stacked. - The exhaustion payload reports the count the breaker actually holds at exhaustion rather than the snapshot frozen when the reload was issued, which is what `main-window-controller`'s breadcrumb and prompt copy consume. --- src/main/startup/main-window-controller.ts | 12 +- ...ndow-recovery-reload-behind-prompt.test.ts | 156 ++++++++++++++++++ ...ainWindow-recovery-reload-watchdog.test.ts | 41 ++++- src/main/window/main-window-contracts.ts | 5 + .../window/main-window-focus-lifecycle.ts | 3 + .../renderer-recovery-reload-watchdog.ts | 36 +++- 6 files changed, 241 insertions(+), 12 deletions(-) create mode 100644 src/main/window/createMainWindow-recovery-reload-behind-prompt.test.ts diff --git a/src/main/startup/main-window-controller.ts b/src/main/startup/main-window-controller.ts index 8ea245fc256..7b52b438682 100644 --- a/src/main/startup/main-window-controller.ts +++ b/src/main/startup/main-window-controller.ts @@ -113,7 +113,13 @@ export function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {} reason: details.reason, expectedTeardown: getExpectedTeardownScope(webContentsId, false) }), - onRendererRecoveryExhausted: ({ details, recentRecoveryCount, cause, retry }) => { + onRendererRecoveryExhausted: ({ + details, + recentRecoveryCount, + cause, + retry, + supersedesStandingPrompt + }) => { // Why two names: a stalled reload never opened the breaker, and a bundle that says it did misreads the failure. recordDurableCrashBreadcrumb( cause === 'reload-stalled' @@ -125,6 +131,10 @@ export function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {} recentRecoveryCount } ) + // The standing box is unanswered and cannot be replaced; a second one would stack on top of it. + if (supersedesStandingPrompt) { + return + } void showRendererRecoveryPrompt(recentRecoveryCount, cause, retry) }, deferLoad: true, diff --git a/src/main/window/createMainWindow-recovery-reload-behind-prompt.test.ts b/src/main/window/createMainWindow-recovery-reload-behind-prompt.test.ts new file mode 100644 index 00000000000..1448952ab97 --- /dev/null +++ b/src/main/window/createMainWindow-recovery-reload-behind-prompt.test.ts @@ -0,0 +1,156 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', async () => + (await import('./createMainWindow-test-harness')).electronModuleMock() +) +vi.mock('@electron-toolkit/utils', async () => + (await import('./createMainWindow-test-harness')).electronToolkitUtilsMock() +) +vi.mock('./macos-tahoe-release', async () => + (await import('./createMainWindow-test-harness')).macosTahoeReleaseMock() +) +vi.mock('../app-icon', async () => (await import('./createMainWindow-test-harness')).appIconMock()) +vi.mock('../browser/browser-manager', async () => + (await import('./createMainWindow-test-harness')).browserManagerMock() +) +vi.mock('../browser/browser-client-page-renderer-runtime', async () => { + const harness = await import('./createMainWindow-test-harness') + return { + attachBrowserClientPageRenderer: harness.attachClientPageRendererMock, + retireBrowserClientPageRenderer: harness.retireClientPageRendererMock + } +}) + +import { createMainWindow } from './createMainWindow' +import { shouldRecoverRendererAfterProcessGone } from '../crash-reporting/process-gone-classification' +import { resetExpectedTeardownStateForTest } from '../crash-reporting/expected-teardown-state' +import { + browserWindowMock, + resetMainWindowMocks, + withPlatform +} from './createMainWindow-test-harness' + +/** + * Windows launch-failed / exit 18 (win32 10.0.26200, v1.4.198 + v1.4.199). + * + * Field bundle qNKP0hy6kIeMczMWzuXsEA, main pid 28400: + * t+0.000 render-process-gone launch-failed 18 + * t+0.268 renderer_recovery_reload (auto attempt 1) + * t+1.086 reload_failed attempt=1 ERR_FAILED -> auto attempt 2 + * t+1.092 reload_failed attempt=2 ERR_FAILED + * t+1.093 renderer_recovery_reload_exhausted recentRecoveryCount=1 <- modal prompt raised + * t+1.354 renderer_recovery_reload <- STARTED BEHIND THE UNANSWERED PROMPT + * t+2.226 renderer_recovery_reload <- and again + * t+2.830 render-process-gone launch-failed 18 (breaker now refuses; escalation swallowed) + * t+19.83 renderer_recovery_manual_retry <- user finally answers prompt #1 + * + * A launch-failed renderer never spawned, so reloading the same webContents can + * never produce one. `fail()` in renderer-recovery-reload-watchdog.ts guards that + * with "A pending prompt or crash recovery owns the next reload" -- but `issue()` + * has no such guard, so every fresh render-process-gone starts another reload + * behind the modal dialog, silently spends the circuit breaker's budget, and the + * resulting `crash-loop` escalation is then dropped by `escalate()`'s one-prompt + * latch. Every bundle in the cluster therefore reports recentRecoveryCount: 1. + */ +describe('renderer recovery reload storm behind an unanswered prompt', () => { + beforeEach(() => { + resetMainWindowMocks() + resetExpectedTeardownStateForTest() + vi.useRealTimers() + }) + + it('does not start recovery reloads while a recovery prompt is unanswered', async () => { + vi.useFakeTimers() + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const windowHandlers: Record void> = {} + const webContents = { + id: 143, + getURL: vi.fn(() => 'file:///opt/orca/renderer/index.html'), + isDestroyed: vi.fn(() => false), + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn() + } + // The field shape: the load rejects with ERR_FAILED because the renderer never spawned. + const loadFailure = Object.assign(new Error('ERR_FAILED (-2) loading index.html'), { + code: 'ERR_FAILED' + }) + const browserWindowInstance = { + webContents, + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(() => Promise.reject(loadFailure)), + loadURL: vi.fn(() => Promise.reject(loadFailure)) + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + const onRendererRecoveryExhausted = vi.fn() + withPlatform('win32', () => { + createMainWindow(null, { + onRendererRecoveryExhausted, + shouldRecoverRenderer: (details) => + shouldRecoverRendererAfterProcessGone({ + reason: details.reason, + expectedTeardown: 'none' + }) + }) + }) + + const details = { + reason: 'launch-failed', + exitCode: 18 + } as Electron.RenderProcessGoneDetails + const driveLaunchFailure = async (): Promise => { + windowHandlers['render-process-gone']?.({} as never, details) + await vi.advanceTimersByTimeAsync(250) + await vi.advanceTimersByTimeAsync(0) + } + + await driveLaunchFailure() + + // Initial load + the watchdog's two attempts, both rejecting with ERR_FAILED. + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(3) + expect(onRendererRecoveryExhausted).toHaveBeenCalledTimes(1) + expect(onRendererRecoveryExhausted).toHaveBeenCalledWith( + expect.objectContaining({ cause: 'reload-stalled', recentRecoveryCount: 1 }) + ) + + // The prompt is a native message box; nothing has answered it yet. + const loadsWhilePromptUnanswered = browserWindowInstance.loadFile.mock.calls.length + await driveLaunchFailure() + await driveLaunchFailure() + + // A pending prompt owns the next reload -- no reload may start behind it. + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(loadsWhilePromptUnanswered) + + // Those hidden reloads spend the breaker's budget, so this crash opens it -- + // and the crash-loop verdict is then swallowed by the one-prompt latch. + await driveLaunchFailure() + expect(onRendererRecoveryExhausted).toHaveBeenCalledWith( + expect.objectContaining({ cause: 'crash-loop' }) + ) + // The box on screen cannot be replaced, so the corrected verdict is recorded without stacking a second one. + expect(onRendererRecoveryExhausted).toHaveBeenLastCalledWith( + expect.objectContaining({ supersedesStandingPrompt: true, recentRecoveryCount: 3 }) + ) + } finally { + consoleError.mockRestore() + } + }) +}) diff --git a/src/main/window/createMainWindow-recovery-reload-watchdog.test.ts b/src/main/window/createMainWindow-recovery-reload-watchdog.test.ts index e551579a7ab..fdf9e80f61b 100644 --- a/src/main/window/createMainWindow-recovery-reload-watchdog.test.ts +++ b/src/main/window/createMainWindow-recovery-reload-watchdog.test.ts @@ -246,27 +246,60 @@ describe('renderer recovery reload watchdog', () => { expect(onRendererRecoveryExhausted).toHaveBeenCalledTimes(1) expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(3) - // The renderer dies again while the box is up; the breaker never counted stalls, so it lets the reload go. + // The renderer dies again while the box is up. The prompt owns the next reload, so this one never starts: + // behind an unanswered box it can neither be seen nor recover the renderer the box is already about. crashRenderer() - expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(4) vi.advanceTimersByTime(RENDERER_RECOVERY_LOAD_TIMEOUT_MS * 2) // Nothing dismisses a native message box: a retry the user never asked for, or a second box, stacks on it. - expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(4) + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(3) expect(onRendererRecoveryExhausted).toHaveBeenCalledTimes(1) // The stall is still on the record, so the bundle does not read as a recovery that quietly worked. expect(onRecoveryReloadOutcome).toHaveBeenLastCalledWith( - expect.objectContaining({ status: 'timeout', attempt: 1 }) + expect.objectContaining({ status: 'timeout', attempt: 2 }) ) // Answering the box with Reload hands the next verdict back to the user. onRendererRecoveryExhausted.mock.calls[0]?.[0].retry() + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(4) vi.advanceTimersByTime(RENDERER_RECOVERY_LOAD_TIMEOUT_MS * 2) expect(onRendererRecoveryExhausted).toHaveBeenCalledTimes(2) consoleError.mockRestore() }) + it('reports the recovery count the breaker holds at exhaustion, not the one frozen at issue time', async () => { + const onRendererRecoveryExhausted = vi.fn() + const { consoleError, crashRenderer, settleLoad } = createHarness() + + createMainWindow(null, { onRendererRecoveryExhausted }) + const failLoad = async (index: number): Promise => { + settleLoad[index]?.reject(new Error('ERR_FAILED (-2)')) + await vi.advanceTimersByTimeAsync(0) + } + + crashRenderer() + await failLoad(1) + await failLoad(2) + expect(onRendererRecoveryExhausted).toHaveBeenLastCalledWith( + expect.objectContaining({ recentRecoveryCount: 1 }) + ) + + // Two more renderer deaths spend breaker budget under the box without starting a reload. + crashRenderer() + crashRenderer() + + // The prompt's Reload fails too: its verdict must count those deaths, not repeat the first snapshot. + onRendererRecoveryExhausted.mock.calls[0]?.[0].retry() + await failLoad(3) + await failLoad(4) + expect(onRendererRecoveryExhausted).toHaveBeenLastCalledWith( + expect.objectContaining({ recentRecoveryCount: 3 }) + ) + + consoleError.mockRestore() + }) + it('still reloads from a crash-loop prompt raised after an earlier recovery had landed', async () => { const onRendererRecoveryExhausted = vi.fn() const { browserWindowInstance, consoleError, crashRenderer, settleLoad } = createHarness() diff --git a/src/main/window/main-window-contracts.ts b/src/main/window/main-window-contracts.ts index 5135be0fbe6..1af03d33044 100644 --- a/src/main/window/main-window-contracts.ts +++ b/src/main/window/main-window-contracts.ts @@ -31,6 +31,11 @@ export type CreateMainWindowOptions = { webContentsId: number recentRecoveryCount: number cause?: RecoveryExhaustionCause + /** + * This verdict corrects one already reported for a prompt that is still on screen. Record it, but do not + * raise a second box: a native message box cannot be dismissed programmatically, so another one stacks. + */ + supersedesStandingPrompt?: boolean /** Watched manual retry for the recovery prompt; an unwatched one cannot re-raise the prompt when it stalls too. */ retry?: () => void }) => void diff --git a/src/main/window/main-window-focus-lifecycle.ts b/src/main/window/main-window-focus-lifecycle.ts index d494992e692..903eb2cfbc9 100644 --- a/src/main/window/main-window-focus-lifecycle.ts +++ b/src/main/window/main-window-focus-lifecycle.ts @@ -168,6 +168,9 @@ export function installMainWindowFocusLifecycle(args: { // Why: the reload can stall with a live window and no document — no did-fail-load fires, and the breaker counts // renderer deaths, so a load that never lands is invisible to every other observer on this path. const recoveryReloadWatchdog = createRendererRecoveryReloadWatchdog({ + // Why live: a stall escalates up to 45s after its reload was issued, by which time more renderer + // deaths have registered — the issue-time snapshot understates what the breaker is holding. + getRecentRecoveryCount: () => rendererRecoveryCircuitBreaker.recentRecoveryCount(Date.now()), isRecoveryPending: () => rendererRecoveryTimer !== null, isWindowClosing, mainWindow, diff --git a/src/main/window/renderer-recovery-reload-watchdog.ts b/src/main/window/renderer-recovery-reload-watchdog.ts index 1295ca13ac4..45d58e36f67 100644 --- a/src/main/window/renderer-recovery-reload-watchdog.ts +++ b/src/main/window/renderer-recovery-reload-watchdog.ts @@ -27,13 +27,16 @@ const MILESTONE_RANK: Record = { export type RecoveryExhaustionCause = 'crash-loop' | 'reload-stalled' export type RendererRecoveryReloadWatchdog = { - /** Issues a recovery reload and arms the stall watchdog. */ + /** Issues a recovery reload and arms the stall watchdog. No-op while a prompt owns the next reload. */ issue: ( details: Electron.RenderProcessGoneDetails, recentRecoveryCount: number, trigger?: RecoveryReloadTrigger ) => void - /** Raises the recovery prompt at most once: a native message box cannot be dismissed, so a second one stacks. */ + /** + * Raises the recovery prompt at most once: a native message box cannot be dismissed, so a second one stacks. + * A later 'crash-loop' verdict still supersedes a standing 'reload-stalled' one so the record names the real cause. + */ escalate: (subject: RecoveryPromptSubject, cause: RecoveryExhaustionCause) => void /** * A main-frame document finished loading. Only an attempt whose load was superseded takes this as its outcome; @@ -65,6 +68,8 @@ export type RecoveryPromptSubject = Pick number /** True when a renderer death has already queued its own recovery, which then owns the next load. */ isRecoveryPending: () => boolean isWindowClosing: () => boolean @@ -74,6 +79,7 @@ export function createRendererRecoveryReloadWatchdog(args: { rendererWebContentsId: number }): RendererRecoveryReloadWatchdog { const { + getRecentRecoveryCount, isRecoveryPending, isWindowClosing, mainWindow, @@ -88,6 +94,7 @@ export function createRendererRecoveryReloadWatchdog(args: { let latest: RecoveryReload | null = null // Keep one prompt until answered; native message boxes cannot be dismissed programmatically. let prompt: RecoveryPromptSubject | null = null + let promptCause: RecoveryExhaustionCause | null = null let documentLanded = false let timer: ReturnType | null = null @@ -187,6 +194,7 @@ export function createRendererRecoveryReloadWatchdog(args: { const retryFrom = (subject: RecoveryPromptSubject): void => { prompt = null + promptCause = null // A late recovery makes the prompt's Reload unnecessary. if (documentLanded) { return @@ -200,17 +208,23 @@ export function createRendererRecoveryReloadWatchdog(args: { const escalate = (subject: RecoveryPromptSubject, cause: RecoveryExhaustionCause): void => { // A new crash invalidates any document that landed while the prompt was open. documentLanded = false - if (prompt) { + // A stall prompt raised first used to swallow the crash-loop verdict that followed, so bundles named the + // wrong cause. The later verdict supersedes it; the box itself cannot be replaced, only the record. + const supersedes = prompt !== null + if (supersedes && !(cause === 'crash-loop' && promptCause !== 'crash-loop')) { return } prompt = subject + promptCause = cause opts?.onRendererRecoveryExhausted?.({ details: subject.details, webContentsId: rendererWebContentsId, - recentRecoveryCount: subject.recentRecoveryCount, + // The snapshot is taken when the reload is issued; more renderer deaths register during a stall. + recentRecoveryCount: Math.max(subject.recentRecoveryCount, getRecentRecoveryCount?.() ?? 0), cause, + ...(supersedes ? { supersedesStandingPrompt: true } : {}), // Watch manual retries too, so another stall can offer recovery again. - retry: () => retryFrom(subject) + retry: () => retryFrom(prompt ?? subject) }) } @@ -280,8 +294,15 @@ export function createRendererRecoveryReloadWatchdog(args: { rendererWebContents.on('did-fail-load', onDidFailLoad) return { - issue: (details, recentRecoveryCount, trigger = 'automatic') => - start({ attempt: 1, details, recentRecoveryCount }, trigger), + issue: (details, recentRecoveryCount, trigger = 'automatic') => { + // Same invariant fail() enforces: a pending prompt owns the next reload. A reload started behind an + // unanswered native box is invisible, cannot recover a renderer the prompt is already about, and + // silently spends the circuit breaker's budget. + if (prompt) { + return + } + start({ attempt: 1, details, recentRecoveryCount }, trigger) + }, escalate, notifyDocumentLoaded: () => { // Timed-out replacements can still recover beneath the prompt. @@ -301,6 +322,7 @@ export function createRendererRecoveryReloadWatchdog(args: { inFlight = null latest = null prompt = null + promptCause = null clearTimer() rendererWebContents.off?.('did-navigate', onDidNavigate) rendererWebContents.off?.('dom-ready', onDomReady)