From 833dedd4a16a8404e965ea47d46ff056f7a5017f Mon Sep 17 00:00:00 2001 From: Jannis Braun <151788261+TheZwiss@users.noreply.github.com> Date: Sun, 3 May 2026 13:51:04 +0200 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20boot-timer=20race=20=E2=80=94?= =?UTF-8?q?=20handle=20rendererReady=20ping=20arriving=20before=20arm?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/systems/desktop.md | 4 +-- packages/desktop/src/recovery.test.ts | 40 ++++++++++++++++++++++++++- packages/desktop/src/recovery.ts | 33 ++++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/docs/systems/desktop.md b/docs/systems/desktop.md index 8d1b0888..21de7179 100644 --- a/docs/systems/desktop.md +++ b/docs/systems/desktop.md @@ -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/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 diff --git a/packages/desktop/src/recovery.test.ts b/packages/desktop/src/recovery.test.ts index ce96874b..8014301d 100644 --- a/packages/desktop/src/recovery.test.ts +++ b/packages/desktop/src/recovery.test.ts @@ -9,6 +9,8 @@ import { armBootTimer, clearBootTimer, isBootArmed, + handleRendererReady, + resetBootTimerStateForTest, } from './recovery'; // 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', () => { 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://', () => { @@ -303,4 +307,38 @@ describe('armBootTimer / clearBootTimer', () => { clearBootTimer(); 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); + }); }); diff --git a/packages/desktop/src/recovery.ts b/packages/desktop/src/recovery.ts index 8dddffcc..b53e68fc 100644 --- a/packages/desktop/src/recovery.ts +++ b/packages/desktop/src/recovery.ts @@ -215,6 +215,16 @@ let bootTimer: ReturnType | null = null; let bootArmed = false; 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. * Wired by main.ts to call enterRecoveryMode({ code: 'renderer-stalled', ... }). @@ -234,6 +244,11 @@ export function armBootTimer(win: BrowserWindow): void { if (!app.isPackaged) return; const url = win.webContents.getURL(); 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(); bootArmed = true; bootTimer = setTimeout(() => { @@ -251,6 +266,19 @@ export function clearBootTimer(): void { 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. */ export function isBootArmed(): boolean { return bootArmed; @@ -259,8 +287,12 @@ export function isBootArmed(): boolean { /** * Called from the renderer-ready IPC handler. * 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 { + pingReceivedThisNav = true; if (bootArmed) clearBootTimer(); } @@ -435,6 +467,7 @@ export function attachRecoveryHandlers(win: BrowserWindow): void { win.webContents.on('did-navigate', () => { clearBootTimer(); pendingArm = true; + pingReceivedThisNav = false; // reset for new navigation }); win.webContents.on('did-finish-load', () => {