fix(desktop): boot-timer race — handle rendererReady ping arriving before arm
CRITICAL BUG. In real SPAs, useEffect fires during document load (microtask after bundle execute + React render), which is BEFORE did-finish-load fires (after window.onload). Without this fix, the ping arrived when bootArmed=false (no-op), then did-finish-load armed a timer nothing would clear → 20s later every successful packaged build falsely entered recovery. Caught by smoke scenario 13 (positive control: page that DOES ping should NOT recover). The smoke proved the page's script ran AND the ping was sent, yet recovery still fired. Fix: module-level pingReceivedThisNav flag, reset on did-navigate, set in handleRendererReady, checked in armBootTimer (early-return if true). Late-ping case (ping after arm) preserved via existing 'if (bootArmed) clearBootTimer()'. Also exports resetBootTimerStateForTest() to ensure full module-state isolation between tests (pingReceivedThisNav is module-level and must not bleed across test cases in the same run). 3 new tests pin the early-ping, late-ping, and per-nav persistence semantics. 48/48 tests pass. Build clean. Spec + docs updated.
This commit is contained in:
@@ -317,9 +317,9 @@ The boot ping is **not a heartbeat** — it is a one-shot per-navigation signal.
|
|||||||
- `packages/web/src/App.tsx` — `useEffect` with `[]` deps, fires on first commit (semantic: "renderer survived render," not "data loaded")
|
- `packages/web/src/App.tsx` — `useEffect` with `[]` deps, fires on first commit (semantic: "renderer survived render," not "data loaded")
|
||||||
- `packages/web/src/main.tsx` — `ErrorBoundary.componentDidCatch`, fires when in-app error UI mounts (so the boot timer doesn't override the ErrorBoundary fallback 20s later)
|
- `packages/web/src/main.tsx` — `ErrorBoundary.componentDidCatch`, fires when in-app error UI mounts (so the boot timer doesn't override the ErrorBoundary fallback 20s later)
|
||||||
|
|
||||||
Main-side gating: `bootArmed` flag ensures only the first `renderer-ready` IPC matters; subsequent calls are no-ops.
|
Main-side gating uses a per-navigation `pingReceivedThisNav` flag (reset on `did-navigate`, set in `handleRendererReady`). If the ping arrives BEFORE the timer is armed — the typical SPA case, because `useEffect` runs in a microtask after bundle execution + React render, which is before `window.onload` that `did-finish-load` waits on — `armBootTimer` checks the flag and short-circuits. If the ping arrives AFTER the timer is armed (less-common ordering), it clears the existing timer via `clearBootTimer`. Either path means a healthy renderer never trips false recovery. The `bootArmed` flag retains its role: it ensures a `clearBootTimer` call inside the timeout callback is a no-op if the timer was already disarmed by the ping.
|
||||||
|
|
||||||
Navigation-aware arming: `did-navigate` (top-level non-same-document) clears any pending timer and queues a fresh arm for `did-finish-load`. `did-navigate-in-page` (SPA routing) is ignored, so React Router channel switches don't trip the timer.
|
Navigation-aware arming: `did-navigate` (top-level non-same-document) clears any pending timer, resets `pingReceivedThisNav`, and queues a fresh arm for `did-finish-load`. `did-navigate-in-page` (SPA routing) is ignored, so React Router channel switches don't trip the timer.
|
||||||
|
|
||||||
### Recovery UI
|
### Recovery UI
|
||||||
|
|
||||||
|
|||||||
@@ -9,6 +9,8 @@ import {
|
|||||||
armBootTimer,
|
armBootTimer,
|
||||||
clearBootTimer,
|
clearBootTimer,
|
||||||
isBootArmed,
|
isBootArmed,
|
||||||
|
handleRendererReady,
|
||||||
|
resetBootTimerStateForTest,
|
||||||
} from './recovery';
|
} from './recovery';
|
||||||
|
|
||||||
// Mock electron with isPackaged=true so the real arm path executes in all tests below.
|
// Mock electron with isPackaged=true so the real arm path executes in all tests below.
|
||||||
@@ -272,7 +274,9 @@ function fakeWin(url: string): FakeWindow {
|
|||||||
|
|
||||||
describe('armBootTimer / clearBootTimer', () => {
|
describe('armBootTimer / clearBootTimer', () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
clearBootTimer();
|
// Reset all module-level boot-timer state including pingReceivedThisNav so
|
||||||
|
// tests that call handleRendererReady() don't pollute subsequent tests.
|
||||||
|
resetBootTimerStateForTest();
|
||||||
});
|
});
|
||||||
|
|
||||||
it('arms when URL is http://', () => {
|
it('arms when URL is http://', () => {
|
||||||
@@ -303,4 +307,38 @@ describe('armBootTimer / clearBootTimer', () => {
|
|||||||
clearBootTimer();
|
clearBootTimer();
|
||||||
expect(isBootArmed()).toBe(false);
|
expect(isBootArmed()).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('does NOT arm if rendererReady arrived before armBootTimer (early-ping case)', () => {
|
||||||
|
// Simulate SPA timing: useEffect fires (microtask) before did-finish-load.
|
||||||
|
// The ping arrives when bootArmed=false, then armBootTimer is called by
|
||||||
|
// did-finish-load. Without the pingReceivedThisNav flag this would arm a
|
||||||
|
// 20s timer that nothing clears → false renderer-stalled recovery.
|
||||||
|
handleRendererReady();
|
||||||
|
armBootTimer(fakeWin('http://localhost:3005/') as never);
|
||||||
|
expect(isBootArmed()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('clears existing timer if rendererReady arrives after armBootTimer (late-ping case)', () => {
|
||||||
|
// Simulate the less-common ordering: did-finish-load fires first (arms timer),
|
||||||
|
// then the ping arrives. Preserved existing behavior — ping clears the timer.
|
||||||
|
armBootTimer(fakeWin('http://localhost:3005/') as never);
|
||||||
|
expect(isBootArmed()).toBe(true);
|
||||||
|
handleRendererReady();
|
||||||
|
expect(isBootArmed()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('flag is per-navigation: after clearBootTimer reset, a subsequent arm should still be blocked by the set flag', () => {
|
||||||
|
// Verify that pingReceivedThisNav persists across clearBootTimer calls until
|
||||||
|
// a real did-navigate resets it. The module-level flag is only reset by
|
||||||
|
// did-navigate (wired in attachRecoveryHandlers). Here we confirm the flag
|
||||||
|
// behaviour in isolation: ping → arm (blocked) → clear → arm again (still blocked).
|
||||||
|
// The per-nav reset is integration-tested by smoke scenario 4 + 13 together.
|
||||||
|
handleRendererReady();
|
||||||
|
armBootTimer(fakeWin('http://localhost:3005/') as never);
|
||||||
|
expect(isBootArmed()).toBe(false);
|
||||||
|
// clearBootTimer resets bootArmed but NOT pingReceivedThisNav
|
||||||
|
clearBootTimer();
|
||||||
|
armBootTimer(fakeWin('http://localhost:3005/') as never);
|
||||||
|
expect(isBootArmed()).toBe(false);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -215,6 +215,16 @@ let bootTimer: ReturnType<typeof setTimeout> | null = null;
|
|||||||
let bootArmed = false;
|
let bootArmed = false;
|
||||||
let onBootStallCallback: (() => void) | null = null;
|
let onBootStallCallback: (() => void) | null = null;
|
||||||
|
|
||||||
|
// Tracks whether the renderer-ready ping has arrived for the current top-level
|
||||||
|
// navigation. Reset on did-navigate (in attachRecoveryHandlers).
|
||||||
|
//
|
||||||
|
// Why this matters: in real SPAs, useEffect fires during document load
|
||||||
|
// (microtask after bundle execution), which is BEFORE did-finish-load (which
|
||||||
|
// fires after window.onload). Without this flag, the ping arrives when
|
||||||
|
// bootArmed=false (no-op), then did-finish-load arms a fresh timer that
|
||||||
|
// nothing clears → 20s later, false renderer-stalled recovery.
|
||||||
|
let pingReceivedThisNav = false;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Register the callback fired when the boot timer expires.
|
* Register the callback fired when the boot timer expires.
|
||||||
* Wired by main.ts to call enterRecoveryMode({ code: 'renderer-stalled', ... }).
|
* Wired by main.ts to call enterRecoveryMode({ code: 'renderer-stalled', ... }).
|
||||||
@@ -234,6 +244,11 @@ export function armBootTimer(win: BrowserWindow): void {
|
|||||||
if (!app.isPackaged) return;
|
if (!app.isPackaged) return;
|
||||||
const url = win.webContents.getURL();
|
const url = win.webContents.getURL();
|
||||||
if (!url.startsWith('http://') && !url.startsWith('https://')) return;
|
if (!url.startsWith('http://') && !url.startsWith('https://')) return;
|
||||||
|
// Critical: if the renderer-ready ping arrived before did-finish-load
|
||||||
|
// (typical SPA timing — useEffect runs in microtask before window.onload),
|
||||||
|
// do not arm. Otherwise the timer fires 20s later despite the renderer
|
||||||
|
// being healthy.
|
||||||
|
if (pingReceivedThisNav) return;
|
||||||
clearBootTimer();
|
clearBootTimer();
|
||||||
bootArmed = true;
|
bootArmed = true;
|
||||||
bootTimer = setTimeout(() => {
|
bootTimer = setTimeout(() => {
|
||||||
@@ -251,6 +266,19 @@ export function clearBootTimer(): void {
|
|||||||
bootArmed = false;
|
bootArmed = false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Reset all module-level boot-timer state.
|
||||||
|
* Exported for test isolation only — do not call from production code.
|
||||||
|
* Production code resets pingReceivedThisNav exclusively via the
|
||||||
|
* did-navigate handler in attachRecoveryHandlers.
|
||||||
|
*/
|
||||||
|
export function resetBootTimerStateForTest(): void {
|
||||||
|
if (bootTimer) clearTimeout(bootTimer);
|
||||||
|
bootTimer = null;
|
||||||
|
bootArmed = false;
|
||||||
|
pingReceivedThisNav = false;
|
||||||
|
}
|
||||||
|
|
||||||
/** Returns true while the boot timer is armed and waiting for renderer-ready. */
|
/** Returns true while the boot timer is armed and waiting for renderer-ready. */
|
||||||
export function isBootArmed(): boolean {
|
export function isBootArmed(): boolean {
|
||||||
return bootArmed;
|
return bootArmed;
|
||||||
@@ -259,8 +287,12 @@ export function isBootArmed(): boolean {
|
|||||||
/**
|
/**
|
||||||
* Called from the renderer-ready IPC handler.
|
* Called from the renderer-ready IPC handler.
|
||||||
* Disarms the boot timer — the renderer booted successfully.
|
* Disarms the boot timer — the renderer booted successfully.
|
||||||
|
* Sets pingReceivedThisNav so that if did-finish-load fires after the ping
|
||||||
|
* (typical SPA timing), armBootTimer will short-circuit rather than arming a
|
||||||
|
* timer that nothing will clear.
|
||||||
*/
|
*/
|
||||||
export function handleRendererReady(): void {
|
export function handleRendererReady(): void {
|
||||||
|
pingReceivedThisNav = true;
|
||||||
if (bootArmed) clearBootTimer();
|
if (bootArmed) clearBootTimer();
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -435,6 +467,7 @@ export function attachRecoveryHandlers(win: BrowserWindow): void {
|
|||||||
win.webContents.on('did-navigate', () => {
|
win.webContents.on('did-navigate', () => {
|
||||||
clearBootTimer();
|
clearBootTimer();
|
||||||
pendingArm = true;
|
pendingArm = true;
|
||||||
|
pingReceivedThisNav = false; // reset for new navigation
|
||||||
});
|
});
|
||||||
|
|
||||||
win.webContents.on('did-finish-load', () => {
|
win.webContents.on('did-finish-load', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user