mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 00:02:41 +00:00
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.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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<string, (...args: any[]) => 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<void> => {
|
||||
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()
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -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<void> => {
|
||||
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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -27,13 +27,16 @@ const MILESTONE_RANK: Record<RecoveryReloadMilestone, number> = {
|
||||
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<RecoveryReload, 'details' | 'recentReco
|
||||
|
||||
/** Bounds stalled recovery reloads while still observing success after escalation. */
|
||||
export function createRendererRecoveryReloadWatchdog(args: {
|
||||
/** Live recovery count from the circuit breaker, read at exhaustion; the issue-time snapshot is up to a stall old. */
|
||||
getRecentRecoveryCount?: () => 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<typeof setTimeout> | 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)
|
||||
|
||||
Reference in New Issue
Block a user