From 8127d656351396544bc7fce9a65e802196cd4385 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sat, 8 Aug 2026 11:53:12 -0400 Subject: [PATCH] fix(updater): capture install commitment synchronously at document load Third-pass review found the popout guard was too narrow: View -> Reload, force reload, the app:reload IPC, renderer-crash recovery, and dock/tray activation can all create a fresh renderer document during the Linux revalidation window. Each one misses the broadcast, and its async seed can never be answered because the package install blocks main in spawnSync. Guarding those five call sites would be whack-a-mole. Instead preload reads the state synchronously before any document script runs, so every document - however it was created - knows before its first lazy import. The renderer reads it lazily, so it is available before React mounts rather than in an effect. Also fixes a defect mutation testing exposed in my own logic: a stale false broadcast from an earlier aborted install masked the local arming, so the initiating window lost its early coverage on the next restart. Adds a cross-layer test that kills both wiring survivors review 3 listed: dropping the install-path mark, and dropping the clear on recovery. --- src/main/updater.test.ts | 26 ++++++++++++++ .../window/attach-main-window-services.ts | 11 +++++- src/preload/api-types.ts | 2 ++ src/preload/index.ts | 17 +++++++++- .../lazy-with-retry.update-install.test.ts | 29 ++++++++++++++++ .../lib/updater-install-commitment.test.ts | 14 ++++++++ .../src/lib/updater-install-commitment.ts | 34 ++++++++++++++++--- src/renderer/src/web/web-preload-api.ts | 1 + src/shared/updater-install-events.ts | 10 ++++++ 9 files changed, 138 insertions(+), 6 deletions(-) diff --git a/src/main/updater.test.ts b/src/main/updater.test.ts index dce8b39a7b6..877d91881ad 100644 --- a/src/main/updater.test.ts +++ b/src/main/updater.test.ts @@ -1742,6 +1742,32 @@ describe('updater', () => { expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledWith(false, true) }) + it('commits install state before the native installer, and clears it on recovery', async () => { + // Cross-layer wiring: renderers read this to stop loading lazy chunks out of an + // archive the installer is replacing. If this call is ever dropped, every + // renderer silently loses that protection while the tests stay green. + vi.useFakeTimers() + const commitment = await import('./updater-install-commitment') + commitment.resetUpdaterInstallCommitmentForTest() + + const mainWindow = { webContents: { send: vi.fn() } } + const { setupAutoUpdater, quitAndInstall } = await import('./updater') + setupAutoUpdater(mainWindow as never) + + let committedWhenInstallerRan: boolean | null = null + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + committedWhenInstallerRan = commitment.isUpdaterInstallCommitted() + autoUpdaterMock.emit('error', new Error('installer refused')) + }) + + quitAndInstall() + await vi.advanceTimersByTimeAsync(100) + + expect(committedWhenInstallerRan).toBe(true) + // A failed install leaves the archive intact, so renderers must recover normally. + expect(commitment.isUpdaterInstallCommitted()).toBe(false) + }) + it('runs pre-quit cleanup before local PTY cleanup during update install', async () => { vi.useFakeTimers() diff --git a/src/main/window/attach-main-window-services.ts b/src/main/window/attach-main-window-services.ts index 035b00d3430..40644182b6b 100644 --- a/src/main/window/attach-main-window-services.ts +++ b/src/main/window/attach-main-window-services.ts @@ -1,7 +1,10 @@ /* eslint-disable max-lines -- Why: this file is the central main-window IPC wiring point; splitting it during the mobile release compatibility rebase would increase release risk. */ import { randomUUID } from 'node:crypto' import { isUpdaterInstallCommitted } from '../updater-install-commitment' -import { UPDATER_IS_INSTALL_COMMITTED_CHANNEL } from '../../shared/updater-install-events' +import { + UPDATER_IS_INSTALL_COMMITTED_CHANNEL, + UPDATER_IS_INSTALL_COMMITTED_SYNC_CHANNEL +} from '../../shared/updater-install-events' import { app, ipcMain } from 'electron' import type { BrowserWindow, IpcMainInvokeEvent } from 'electron' @@ -549,6 +552,12 @@ export function registerUpdaterHandlers(_store: Store): void { ipcMain.removeHandler(UPDATER_IS_INSTALL_COMMITTED_CHANNEL) // Why: a window created mid-install misses the broadcast, so it seeds itself here. ipcMain.handle(UPDATER_IS_INSTALL_COMMITTED_CHANNEL, () => isUpdaterInstallCommitted()) + ipcMain.removeAllListeners(UPDATER_IS_INSTALL_COMMITTED_SYNC_CHANNEL) + // Why: preload reads this before any document script, so a window created or + // reloaded mid-install knows before its first lazy import. + ipcMain.on(UPDATER_IS_INSTALL_COMMITTED_SYNC_CHANNEL, (event) => { + event.returnValue = isUpdaterInstallCommitted() + }) ipcMain.handle('updater:getStatus', () => getUpdateStatus()) ipcMain.handle('updater:getVersion', () => app.getVersion()) ipcMain.handle('updater:check', (_event, options?: UpdateCheckOptions) => { diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index cf5c4acfdaa..a6b2e19e998 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -2740,6 +2740,8 @@ export type PreloadApi = { onChanged: (callback: (event: RemoteWorkspaceChangedEvent) => void) => () => void } updater: { + /** Captured synchronously in preload, before any document script runs. */ + installCommittedAtLoad: boolean isInstallCommitted: () => Promise onInstallCommitted: (callback: (committed: boolean) => void) => () => void getVersion: () => Promise diff --git a/src/preload/index.ts b/src/preload/index.ts index 4d60e286028..11cc3b947b3 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -2,8 +2,19 @@ import { contextBridge, ipcRenderer, webFrame, webUtils } from 'electron' import { UPDATER_INSTALL_COMMITTED_CHANNEL, - UPDATER_IS_INSTALL_COMMITTED_CHANNEL + UPDATER_IS_INSTALL_COMMITTED_CHANNEL, + UPDATER_IS_INSTALL_COMMITTED_SYNC_CHANNEL } from '../shared/updater-install-events' + +function readInstallCommittedAtLoad(): boolean { + try { + return ipcRenderer.sendSync(UPDATER_IS_INSTALL_COMMITTED_SYNC_CHANNEL) === true + } catch { + // An unanswered probe must never stop a window loading; the broadcast and the + // async seed still cover every case except a document born mid-install. + return false + } +} import { electronAPI } from '@electron-toolkit/preload' import { preloadE2EConfig } from './e2e-config' import { glApi } from './gitlab' @@ -3041,6 +3052,10 @@ const api = { awaitBeforeUnloadCheckpoint ), + // Why: captured before any document script runs, so a window created or + // reloaded during an install knows before its first lazy import. The async + // seed below cannot be relied on — the Linux package install blocks main. + installCommittedAtLoad: readInstallCommittedAtLoad(), isInstallCommitted: () => ipcRenderer.invoke(UPDATER_IS_INSTALL_COMMITTED_CHANNEL), onInstallCommitted: (callback) => { const listener = (_event: Electron.IpcRendererEvent, committed: boolean) => callback(committed) diff --git a/src/renderer/src/lib/lazy-with-retry.update-install.test.ts b/src/renderer/src/lib/lazy-with-retry.update-install.test.ts index 8869263aae5..10e0dbae74d 100644 --- a/src/renderer/src/lib/lazy-with-retry.update-install.test.ts +++ b/src/renderer/src/lib/lazy-with-retry.update-install.test.ts @@ -27,6 +27,7 @@ import { loadLazyWithRetry, resetLazyChunkReloadRequestsForTest } from './lazy-with-retry' +import { resetUpdaterInstallCommitmentForTest } from './updater-install-commitment' const RELOAD_GUARD_KEY = 'orca:lazy-chunk-reload-attempted' @@ -79,6 +80,7 @@ describe('loadLazyWithRetry during a committed update install', () => { afterEach(() => { unregister?.() unregister = null + resetUpdaterInstallCommitmentForTest() vi.restoreAllMocks() window.sessionStorage.clear() resetLazyChunkReloadRequestsForTest() @@ -275,6 +277,33 @@ describe('loadLazyWithRetry during a committed update install', () => { expect(getItem).not.toHaveBeenCalled() }) + it('skips recovery in a document born mid-install, before any registration', async () => { + // Every reload/reopen path lands here: View -> Reload, force reload, the + // app:reload IPC, renderer-crash recovery, dock activation, a new popout. The + // fresh document never saw a broadcast, and its async seed can never be + // answered while the Linux package install blocks main inside spawnSync. The + // synchronous preload capture is the only thing that can be true this early — + // note this test deliberately does NOT call registerUpdaterInstallCommitment. + unregister?.() + unregister = null + resetUpdaterInstallCommitmentForTest() + ;(window as unknown as { api: Record }).api = { + crashReports: { recordBreadcrumb: () => undefined }, + updater: { + installCommittedAtLoad: true, + isInstallCommitted: () => new Promise(() => undefined), + onInstallCommitted: () => () => undefined + } + } as never + + await loadLazyWithRetry(() => Promise.reject(CORRUPT_CHUNK_ERROR()), { + retries: 0, + reloadKey: 'app.root' + }).catch(() => undefined) + + expect(window.location.reload).not.toHaveBeenCalled() + }) + it('still requests recovery when no install is committed', async () => { installBreadcrumbSink() diff --git a/src/renderer/src/lib/updater-install-commitment.test.ts b/src/renderer/src/lib/updater-install-commitment.test.ts index 47303d6d74a..8cc8cc2b2f0 100644 --- a/src/renderer/src/lib/updater-install-commitment.test.ts +++ b/src/renderer/src/lib/updater-install-commitment.test.ts @@ -110,6 +110,20 @@ describe('renderer updater install commitment', () => { expect(isUpdaterInstallCommitted()).toBe(false) }) + it('arms again after an earlier install aborted', () => { + // The user cancels one update, then restarts for the next. A stale false from + // the first must not swallow the second window's early arming. + installBridge() + unregister = registerUpdaterInstallCommitment() + broadcast(true) + broadcast(false) + expect(isUpdaterInstallCommitted()).toBe(false) + + window.dispatchEvent(new Event(ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT)) + + expect(isUpdaterInstallCommitted()).toBe(true) + }) + it('detaches cleanly so a stale listener cannot re-arm a new document', () => { installBridge() const stop = registerUpdaterInstallCommitment() diff --git a/src/renderer/src/lib/updater-install-commitment.ts b/src/renderer/src/lib/updater-install-commitment.ts index 59a36cf3945..328c56b9f2a 100644 --- a/src/renderer/src/lib/updater-install-commitment.ts +++ b/src/renderer/src/lib/updater-install-commitment.ts @@ -4,11 +4,34 @@ import { ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT } from '../../../shared/upd // to be true in the dashboard popout too — it has its own JS context and its own // lazy chunks read from the same archive. Main broadcasts the authoritative value; // the local event only arms the initiating window a few milliseconds earlier. -let broadcastCommitted = false +// null until main has said something authoritative for this document. +let broadcastCommitted: boolean | null = null let locallyStarted = false +let loadTimeCommitted: boolean | null = null + +// Why: a document created or reloaded mid-install — View → Reload, crash recovery, +// dock activation, a popout — misses the broadcast, and its async seed can lose the +// race outright because the Linux package install blocks main inside spawnSync. +// Preload captures this synchronously before any document script, so the very first +// lazy import already knows. Read lazily so it is available before React mounts. +function committedAtDocumentLoad(): boolean { + if (loadTimeCommitted === null) { + loadTimeCommitted = window.api?.updater?.installCommittedAtLoad === true + } + return loadTimeCommitted +} export function isUpdaterInstallCommitted(): boolean { - return broadcastCommitted || locallyStarted + // This window just asked to install, which is fresher than any earlier verdict — + // a stale `false` from a previous aborted install must not mask it. + if (locallyStarted) { + return true + } + // Otherwise main is authoritative: only it can stand the archive back up. + if (broadcastCommitted !== null) { + return broadcastCommitted + } + return committedAtDocumentLoad() } /** @@ -28,6 +51,7 @@ export function registerUpdaterInstallCommitment(): () => void { broadcastCommitted = committed if (!committed) { locallyStarted = false + loadTimeCommitted = false } } const unsubscribe = window.api?.updater?.onInstallCommitted?.(applyBroadcast) @@ -47,12 +71,14 @@ export function registerUpdaterInstallCommitment(): () => void { return () => { window.removeEventListener(ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT, markLocal) unsubscribe?.() - broadcastCommitted = false + broadcastCommitted = null locallyStarted = false + loadTimeCommitted = null } } export function resetUpdaterInstallCommitmentForTest(): void { - broadcastCommitted = false + broadcastCommitted = null locallyStarted = false + loadTimeCommitted = null } diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index 011bab9d2ee..09fe2c56c8b 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -3106,6 +3106,7 @@ function createUpdaterApi(): NonNullable['updater']> { getStatus: () => Promise.resolve({ state: 'idle' } as never), // Why: the web build serves chunks over HTTP from a running server, so no // installer ever swaps the archive underneath it. + installCommittedAtLoad: false, isInstallCommitted: () => Promise.resolve(false), onInstallCommitted: () => () => undefined, check: () => Promise.resolve(), diff --git a/src/shared/updater-install-events.ts b/src/shared/updater-install-events.ts index a3fee1ff02e..266efbb6277 100644 --- a/src/shared/updater-install-events.ts +++ b/src/shared/updater-install-events.ts @@ -1,3 +1,13 @@ /** Single source of truth: main broadcasts on this channel, preload subscribes to it. */ export const UPDATER_INSTALL_COMMITTED_CHANNEL = 'updater:installCommitted' export const UPDATER_IS_INSTALL_COMMITTED_CHANNEL = 'updater:isInstallCommitted' +/** + * Read synchronously in preload, before any document script runs. + * + * Why sync: a renderer document created or reloaded mid-install (View → Reload, + * crash recovery, dock activation, …) misses the broadcast, and an async seed can + * lose the race — on Linux main is blocked inside the package manager's spawnSync + * and cannot answer at all. A document that starts during an install must know it + * before its first lazy import, or it reads a swapped archive. + */ +export const UPDATER_IS_INSTALL_COMMITTED_SYNC_CHANNEL = 'updater:isInstallCommittedSync'