mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 00:03:46 +00:00
fix(crash-reporting): close three auto-recovery mis-attribution paths
Adversarial round 6 found round 5 had covered two of four manual main-window reload paths, and that the outcome check's lookback was wider than its own arming delay. - `app:reload` (the renderer's own retry button) never disarmed. The round-5 wiring test could not see it: it anchors on `onBeforeReload` and this site is spelled `onBeforeRendererReload`, so even its "exactly two occurrences" assertion still passed. - A replacement main window consumed a live arm. The arm is module- global rather than keyed to a webContents, so a fresh window opened after the old one died (dock re-activation, tray reopen, second instance) bootstrapped and claimed the dead window's arm, dismissing every renderer crash in the lookback as auto-recovered. - The lookback reused RENDERER_RECOVERY_OUTCOME_WINDOW_MS for two different jobs. That constant bounds how long an arm stays valid; it must not also bound how far back the arm may credit crashes. The reload is armed 250ms after its crash, so only that crash's own kill burst is ever in scope. At 30s an unrelated crash 25s older — with a different reason the reload never healed — was dismissed and stamped rendererAutoRecovered. Split out a burst-sized lookback. Rather than add a third and fourth call-site disarm, the disarm now lives inside markExpectedRendererReload. Every caller is a manual main-window reload, so this covers all of them at once and a future site cannot forget it. The wiring test pins the invariant at the definition instead of at three call-site spellings. Known gap, deliberately not fixed here: browser-guest-ui's forceReload shortcut reloads the host renderer directly, bypassing this path. That site is also missing the pre-existing markExpectedRendererReload marking, so both halves belong in one browser-scoped change. Mutation-verified: deleting any of the three fixes turns exactly one test red. Gates: 3152 passed | 11 skipped, tsc 0, oxlint/oxfmt clean.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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()')
|
||||
})
|
||||
})
|
||||
|
||||
+8
-6
@@ -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 })
|
||||
},
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user