diff --git a/src/main/crash-reporting/renderer-recovery-crash-outcome.ts b/src/main/crash-reporting/renderer-recovery-crash-outcome.ts index 4fb52b3c151..ed90e6f7e4e 100644 --- a/src/main/crash-reporting/renderer-recovery-crash-outcome.ts +++ b/src/main/crash-reporting/renderer-recovery-crash-outcome.ts @@ -7,6 +7,12 @@ export const RENDERER_BOOTSTRAP_RENDERED_BREADCRUMB = 'renderer_bootstrap_render // has to survive a loaded machine, and must expire before an unrelated boot. const RENDERER_RECOVERY_OUTCOME_WINDOW_MS = 30_000 +// Why not the window above: that bounds how long an arm stays valid, this bounds +// how far back the arm may credit crashes — different jobs. The reload is armed +// 250ms after its crash, so only that crash's own kill burst is ever in scope; +// reusing the 30s window dismissed older, unhealed crashes as auto-recovered. +const RENDERER_RECOVERY_CRASH_LOOKBACK_MS = 5_000 + let recoveryReloadIssuedAtMs: number | null = null export function noteRendererRecoveryReloadIssued(nowMs = Date.now()): void { @@ -49,7 +55,7 @@ export function resolveRecoveredRendererCrashReports( return } void store - .markRendererCrashesAutoRecovered(issuedAtMs - RENDERER_RECOVERY_OUTCOME_WINDOW_MS) + .markRendererCrashesAutoRecovered(issuedAtMs - RENDERER_RECOVERY_CRASH_LOOKBACK_MS) .then((resolved) => { if (resolved.length === 0) { return diff --git a/src/main/crash-reporting/renderer-recovery-outcome-wiring.test.ts b/src/main/crash-reporting/renderer-recovery-outcome-wiring.test.ts index 909255e6247..7db57bc70c6 100644 --- a/src/main/crash-reporting/renderer-recovery-outcome-wiring.test.ts +++ b/src/main/crash-reporting/renderer-recovery-outcome-wiring.test.ts @@ -37,6 +37,18 @@ describe('renderer recovery outcome wiring in index.ts', () => { expect(block).toContain('clearRendererRecoveryReloadIssued()') }) + // Why this carries the invariant: every manual main-window reload marks an + // expected reload, so disarming in the marker covers all of them at once — + // including a future site that would otherwise repeat the round-5 miss. + it('disarms inside markExpectedRendererReload itself', () => { + const block = sliceBlock( + 'function markExpectedRendererReload(webContentsId: number, durationMs = 10_000): void {', + '\nfunction clearExpectedRendererReload' + ) + + expect(block).toContain('clearRendererRecoveryReloadIssued()') + }) + it('disarms on every user-initiated reload of the main window', () => { const reloadAnchor = 'onBeforeReload: ({ ignoreCache, webContentsId }) => {' // Why: createMainWindow's force-reload shortcut and the app menu wire this @@ -59,8 +71,31 @@ describe('renderer recovery outcome wiring in index.ts', () => { expect(guardStart).toBeGreaterThanOrEqual(0) const guard = block.slice(guardStart, block.lastIndexOf('}')) + // The disarm rides on this call — see the markExpectedRendererReload test. expect(guard).toContain('markExpectedRendererReload(webContentsId)') - expect(guard).toContain('clearRendererRecoveryReloadIssued()') } }) + + // Why: `app:reload` is a third manual-reload path with a different callback + // name, so the two `onBeforeReload` sites above do not cover it. + it('disarms on the renderer-initiated app:reload of the main window', () => { + const block = sliceBlock( + 'onBeforeRendererReload: ({ ignoreCache, webContentsId }) => {', + "recordCrashBreadcrumb('renderer_reload_requested', { ignoreCache })" + ) + const guardStart = block.indexOf('if (window.webContents.id === webContentsId) {') + expect(guardStart).toBeGreaterThanOrEqual(0) + const guard = block.slice(guardStart, block.lastIndexOf('}')) + + // The disarm rides on this call — see the markExpectedRendererReload test. + expect(guard).toContain('markExpectedRendererReload(webContentsId)') + }) + + // Why: a replacement main window bootstraps a renderer the dead window's + // recovery reload never produced, so it must not consume that window's arm. + it('disarms before the first load of a replacement main window', () => { + const block = sliceBlock("logStartupMilestone('load-start')", 'loadMainWindow(window)') + + expect(block).toContain('clearRendererRecoveryReloadIssued()') + }) }) diff --git a/src/main/index.ts b/src/main/index.ts index faa4396e4c2..bddecd4b364 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -665,6 +665,11 @@ function createWebContentsTimedFlag(defaultDurationMs = 10_000): { function markExpectedRendererReload(webContentsId: number, durationMs = 10_000): void { expectedRendererReload.mark(webContentsId, durationMs) + // Why here and not at each call site: every caller is a manual main-window + // reload, so the next bootstrap is the user's retry and a stalled recovery + // reload must not claim it. Three call sites each repeated this and a fourth + // (app:reload) was missed; at the definition a new site cannot forget it. + clearRendererRecoveryReloadIssued() } function clearExpectedRendererReload(webContentsId?: number): void { @@ -1247,9 +1252,6 @@ function openMainWindow(): BrowserWindow { onBeforeReload: ({ ignoreCache, webContentsId }) => { if (mainWindow?.webContents.id === webContentsId) { markExpectedRendererReload(webContentsId) - // Why: the user reloading by hand makes the next bootstrap their retry, so a - // recovery reload that stalled short of bootstrap must not claim the credit. - clearRendererRecoveryReloadIssued() } recordCrashBreadcrumb('manual_reload_requested', { ignoreCache }) }, @@ -1495,6 +1497,9 @@ function openMainWindow(): BrowserWindow { } }) logStartupMilestone('load-start') + // Why: this window replaces a torn-down one, so any arm still pending belongs to + // a recovery reload that died with it — this bootstrap is a fresh window, not it. + clearRendererRecoveryReloadIssued() loadMainWindow(window) return window } @@ -2657,9 +2662,6 @@ void app.whenReady().then(async () => { onBeforeReload: ({ ignoreCache, webContentsId }) => { if (mainWindow?.webContents.id === webContentsId) { markExpectedRendererReload(webContentsId) - // Why: the user reloading by hand makes the next bootstrap their retry, so a - // recovery reload that stalled short of bootstrap must not claim the credit. - clearRendererRecoveryReloadIssued() } recordCrashBreadcrumb('manual_reload_requested', { ignoreCache }) }, diff --git a/src/main/ipc/crash-reporting-recovered-renderer-prompt.test.ts b/src/main/ipc/crash-reporting-recovered-renderer-prompt.test.ts index a80db0398aa..f4b002f35fd 100644 --- a/src/main/ipc/crash-reporting-recovered-renderer-prompt.test.ts +++ b/src/main/ipc/crash-reporting-recovered-renderer-prompt.test.ts @@ -240,6 +240,20 @@ describe('auto-recovered renderer crash reporting', () => { await expect(getLatestPending()).resolves.toMatchObject({ id: unrelated.id, status: 'pending' }) }) + // Why 25s: inside the arm's own 30s validity, but long before the crash that + // armed it. Crediting it would dismiss an unhealed crash as auto-recovered. + it('leaves a crash older than the burst pending when a later reload is armed', async () => { + const store = await createStore() + registerCrashReportingHandlers(store) + const stale = await store.record({ ...killedRendererCrash(), reason: 'oom', exitCode: null }) + + const armedMs = Date.parse(stale.createdAt) + 25_000 + noteRendererRecoveryReloadIssued(armedMs) + resolveRecoveredRendererCrashReports(store, armedMs) + + await expect(getLatestPending()).resolves.toMatchObject({ id: stale.id, status: 'pending' }) + }) + it('still prompts when the renderer booted without an auto-recovery reload', async () => { const store = await createStore() registerCrashReportingHandlers(store)