diff --git a/src/main/updater-install-commitment.test.ts b/src/main/updater-install-commitment.test.ts index 2014be50660..33345d50aef 100644 --- a/src/main/updater-install-commitment.test.ts +++ b/src/main/updater-install-commitment.test.ts @@ -19,6 +19,7 @@ vi.mock('electron', () => ({ } })) +const { BrowserWindow } = await import('electron') const { UPDATER_INSTALL_COMMITTED_CHANNEL, clearUpdaterInstallCommitted, @@ -87,6 +88,18 @@ describe('updater install commitment', () => { expect(isUpdaterInstallCommitted()).toBe(true) }) + it('survives getAllWindows itself throwing', () => { + // The outer boundary matters for the same reason as the inner one: this runs + // inside the install path. + const spy = vi.spyOn(BrowserWindow, 'getAllWindows').mockImplementation(() => { + throw new Error('electron is tearing down') + }) + + expect(() => markUpdaterInstallCommitted()).not.toThrow() + expect(isUpdaterInstallCommitted()).toBe(true) + spy.mockRestore() + }) + it('does not broadcast a clear that changes nothing', () => { const win = addWindow() diff --git a/src/main/updater-install-commitment.ts b/src/main/updater-install-commitment.ts index 8f798738946..f636fca63f4 100644 --- a/src/main/updater-install-commitment.ts +++ b/src/main/updater-install-commitment.ts @@ -1,6 +1,8 @@ import { BrowserWindow } from 'electron' +import { UPDATER_INSTALL_COMMITTED_CHANNEL } from '../shared/updater-install-events' + +export { UPDATER_INSTALL_COMMITTED_CHANNEL } -export const UPDATER_INSTALL_COMMITTED_CHANNEL = 'updater:installCommitted' // Why: the installer replaces app.asar while renderers are still alive, so every // renderer needs to know — not just the one that pressed Restart. Renderer-local @@ -53,18 +55,6 @@ export function clearUpdaterInstallCommitted(): void { broadcast(false) } -/** Seeds a window created mid-install, which would otherwise never see the broadcast. */ -export function syncUpdaterInstallCommitmentTo(win: BrowserWindow): void { - if (!installCommitted || win.isDestroyed()) { - return - } - try { - win.webContents.send(UPDATER_INSTALL_COMMITTED_CHANNEL, true) - } catch { - // Best effort: the renderer also seeds itself over IPC when it registers. - } -} - export function resetUpdaterInstallCommitmentForTest(): void { installCommitted = false } diff --git a/src/main/window/attach-main-window-services.ts b/src/main/window/attach-main-window-services.ts index ad1ce98490c..035b00d3430 100644 --- a/src/main/window/attach-main-window-services.ts +++ b/src/main/window/attach-main-window-services.ts @@ -1,6 +1,7 @@ /* 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 { app, ipcMain } from 'electron' import type { BrowserWindow, IpcMainInvokeEvent } from 'electron' @@ -545,9 +546,9 @@ export function registerUpdaterHandlers(_store: Store): void { ipcMain.removeHandler('updater:showLinuxPackage') ipcMain.removeHandler('updater:listBuilds') - ipcMain.removeHandler('updater:isInstallCommitted') + ipcMain.removeHandler(UPDATER_IS_INSTALL_COMMITTED_CHANNEL) // Why: a window created mid-install misses the broadcast, so it seeds itself here. - ipcMain.handle('updater:isInstallCommitted', () => isUpdaterInstallCommitted()) + ipcMain.handle(UPDATER_IS_INSTALL_COMMITTED_CHANNEL, () => isUpdaterInstallCommitted()) ipcMain.handle('updater:getStatus', () => getUpdateStatus()) ipcMain.handle('updater:getVersion', () => app.getVersion()) ipcMain.handle('updater:check', (_event, options?: UpdateCheckOptions) => { diff --git a/src/main/window/dashboard-popout-window.test.ts b/src/main/window/dashboard-popout-window.test.ts index 7ffa56d7b53..138713eeb89 100644 --- a/src/main/window/dashboard-popout-window.test.ts +++ b/src/main/window/dashboard-popout-window.test.ts @@ -133,6 +133,10 @@ vi.mock('../ipc/ui', () => ({ sendToTrustedUIRenderer: sendToTrustedUIRendererMo vi.mock('./privileged-window-navigation', () => ({ installPrivilegedWindowNavigationPolicy: installNavigationPolicyMock })) +const installCommittedMock = vi.fn(() => false) +vi.mock('../updater-install-commitment', () => ({ + isUpdaterInstallCommitted: () => installCommittedMock() +})) import { createOrFocusDashboardPopout, @@ -173,6 +177,7 @@ const RENDERER_URL = 'http://localhost:5173' describe('createOrFocusDashboardPopout', () => { beforeEach(() => { instances.length = 0 + installCommittedMock.mockReturnValue(false) isMock.dev = false vi.stubEnv('ELECTRON_RENDERER_URL', '') getAllDisplaysMock.mockReturnValue([{ workArea: { x: 0, y: 0, width: 1920, height: 1080 } }]) @@ -246,6 +251,33 @@ describe('createOrFocusDashboardPopout', () => { expect(win.loadURL).toHaveBeenCalledWith(`${RENDERER_URL}/popout.html?view=kanban`) }) + + it('refuses to open a new popout once an update install is committed', () => { + // Reviewer P1: a popout created during the Linux revalidation window never saw + // the broadcast, and its seed IPC can go unanswered because the package install + // blocks main inside spawnSync — so its first lazy render would read a swapped + // archive and request the doomed recovery reload. + installCommittedMock.mockReturnValue(true) + + const created = createOrFocusDashboardPopout(makeStore() as never) + + expect(created).toBeNull() + expect(instances).toHaveLength(0) + }) + + it('still focuses an already-open popout during an install', () => { + // It already exists, is already registered, and already got the broadcast; + // refusing to focus it would be a gratuitous regression. + const store = makeStore() + const existing = createOrFocusDashboardPopout(store as never) + installCommittedMock.mockReturnValue(true) + + const focused = createOrFocusDashboardPopout(store as never) + + expect(focused).toBe(existing) + expect(instances).toHaveLength(1) + }) + it('focuses the existing window instead of creating a second one', () => { const store = makeStore() const first = createOrFocusDashboardPopout(store as never) diff --git a/src/main/window/dashboard-popout-window.ts b/src/main/window/dashboard-popout-window.ts index d23742f375a..f0e6f85d00e 100644 --- a/src/main/window/dashboard-popout-window.ts +++ b/src/main/window/dashboard-popout-window.ts @@ -1,4 +1,5 @@ import { app, BrowserWindow, nativeTheme, type WebContents } from 'electron' +import { isUpdaterInstallCommitted } from '../updater-install-commitment' import { join } from 'node:path' import { is } from '@electron-toolkit/utils' import type { Store } from '../persistence' @@ -140,7 +141,7 @@ export function createOrFocusDashboardPopout( store: Store | null, view?: string, options: { getKeybindings?: () => KeybindingOverrides | undefined } = {} -): BrowserWindow { +): BrowserWindow | null { if (dashboardPopoutWindow && !dashboardPopoutWindow.isDestroyed()) { if (dashboardPopoutWindow.isMinimized()) { dashboardPopoutWindow.restore() @@ -152,6 +153,15 @@ export function createOrFocusDashboardPopout( return dashboardPopoutWindow } + // Why: the installer is replacing app.asar, so this window's lazy chunks would be + // read from a swapped archive. It cannot be told to stand down in time either — + // its seed is an IPC round trip, and the Linux package install blocks main inside + // spawnSync, so the answer can arrive after the chunk has already failed. The app + // is seconds from restarting; refusing to open a new window is the safe answer. + if (isUpdaterInstallCommitted()) { + return null + } + const initialView = view ?? DEFAULT_VIEW const savedBounds = resolveRestoredBounds(store) diff --git a/src/preload/index.ts b/src/preload/index.ts index 7d66d3bb1b6..4d60e286028 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1,5 +1,9 @@ /* eslint-disable max-lines -- Why: preload is the audited renderer/Electron IPC contract; co-locating the surface eases security and type-drift review. */ import { contextBridge, ipcRenderer, webFrame, webUtils } from 'electron' +import { + UPDATER_INSTALL_COMMITTED_CHANNEL, + UPDATER_IS_INSTALL_COMMITTED_CHANNEL +} from '../shared/updater-install-events' import { electronAPI } from '@electron-toolkit/preload' import { preloadE2EConfig } from './e2e-config' import { glApi } from './gitlab' @@ -3037,11 +3041,11 @@ const api = { awaitBeforeUnloadCheckpoint ), - isInstallCommitted: () => ipcRenderer.invoke('updater:isInstallCommitted'), + isInstallCommitted: () => ipcRenderer.invoke(UPDATER_IS_INSTALL_COMMITTED_CHANNEL), onInstallCommitted: (callback) => { const listener = (_event: Electron.IpcRendererEvent, committed: boolean) => callback(committed) - ipcRenderer.on('updater:installCommitted', listener) - return () => ipcRenderer.removeListener('updater:installCommitted', listener) + ipcRenderer.on(UPDATER_INSTALL_COMMITTED_CHANNEL, listener) + return () => ipcRenderer.removeListener(UPDATER_INSTALL_COMMITTED_CHANNEL, listener) }, onStatus: (callback) => { const listener = (_event: Electron.IpcRendererEvent, status: UpdateStatus) => callback(status) diff --git a/src/renderer/src/lib/updater-install-commitment.test.ts b/src/renderer/src/lib/updater-install-commitment.test.ts new file mode 100644 index 00000000000..47303d6d74a --- /dev/null +++ b/src/renderer/src/lib/updater-install-commitment.test.ts @@ -0,0 +1,123 @@ +// @vitest-environment happy-dom +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { + ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT, + ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT +} from '../../../shared/updater-renderer-events' +import { + isUpdaterInstallCommitted, + registerUpdaterInstallCommitment, + resetUpdaterInstallCommitmentForTest +} from './updater-install-commitment' + +const listeners: ((committed: boolean) => void)[] = [] + +function installBridge(seed: boolean | 'never' = false): void { + ;(window as unknown as { api: unknown }).api = { + updater: { + isInstallCommitted: () => + seed === 'never' ? new Promise(() => undefined) : Promise.resolve(seed), + onInstallCommitted: (cb: (committed: boolean) => void) => { + listeners.push(cb) + return () => { + listeners.splice(listeners.indexOf(cb), 1) + } + } + } + } +} + +const broadcast = (committed: boolean): void => { + for (const listener of listeners.slice()) { + listener(committed) + } +} + +describe('renderer updater install commitment', () => { + let unregister: (() => void) | null = null + + beforeEach(() => { + listeners.length = 0 + resetUpdaterInstallCommitmentForTest() + }) + + afterEach(() => { + unregister?.() + unregister = null + resetUpdaterInstallCommitmentForTest() + delete (window as unknown as { api?: unknown }).api + vi.restoreAllMocks() + }) + + it('believes main over anything local', () => { + installBridge() + unregister = registerUpdaterInstallCommitment() + + expect(isUpdaterInstallCommitted()).toBe(false) + broadcast(true) + expect(isUpdaterInstallCommitted()).toBe(true) + }) + + it('arms the initiating window before main answers', () => { + // This window dispatched the start event itself; waiting for the round trip + // would leave a gap exactly when the archive is being replaced. + installBridge('never') + unregister = registerUpdaterInstallCommitment() + + window.dispatchEvent(new Event(ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT)) + + expect(isUpdaterInstallCommitted()).toBe(true) + }) + + it('ignores a local abort while main still says an install is committed', () => { + installBridge() + unregister = registerUpdaterInstallCommitment() + broadcast(true) + + window.dispatchEvent(new Event(ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT)) + + // Only main can stand the archive back up. + expect(isUpdaterInstallCommitted()).toBe(true) + }) + + it('stands down on main clearing, including a local arm', () => { + installBridge() + unregister = registerUpdaterInstallCommitment() + window.dispatchEvent(new Event(ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT)) + + broadcast(false) + + expect(isUpdaterInstallCommitted()).toBe(false) + }) + + it('seeds a window that opened after the broadcast went out', async () => { + installBridge(true) + unregister = registerUpdaterInstallCommitment() + + await Promise.resolve() + await Promise.resolve() + + expect(isUpdaterInstallCommitted()).toBe(true) + }) + + it('survives a renderer with no updater bridge at all', () => { + ;(window as unknown as { api: unknown }).api = {} + + expect(() => { + unregister = registerUpdaterInstallCommitment() + }).not.toThrow() + expect(isUpdaterInstallCommitted()).toBe(false) + }) + + it('detaches cleanly so a stale listener cannot re-arm a new document', () => { + installBridge() + const stop = registerUpdaterInstallCommitment() + broadcast(true) + + stop() + + expect(isUpdaterInstallCommitted()).toBe(false) + expect(listeners).toHaveLength(0) + }) +}) diff --git a/src/renderer/src/web/web-updater-install-commitment.test.ts b/src/renderer/src/web/web-updater-install-commitment.test.ts new file mode 100644 index 00000000000..c566d3ba698 --- /dev/null +++ b/src/renderer/src/web/web-updater-install-commitment.test.ts @@ -0,0 +1,25 @@ +// @vitest-environment happy-dom +import { afterEach, describe, expect, it } from 'vitest' + +import { installWebPreloadApi } from './web-preload-api' + +describe('web build updater install commitment', () => { + afterEach(() => { + delete (window as unknown as { api?: unknown }).api + }) + + it('never claims an install is committed', async () => { + // The web build serves chunks over HTTP from a running server; no installer + // ever swaps an archive underneath it, so chunk recovery must stay enabled. + installWebPreloadApi() + const updater = (window as unknown as { api: { updater: PreloadUpdater } }).api.updater + + await expect(updater.isInstallCommitted()).resolves.toBe(false) + expect(updater.onInstallCommitted(() => undefined)).toBeTypeOf('function') + }) +}) + +type PreloadUpdater = { + isInstallCommitted: () => Promise + onInstallCommitted: (cb: (committed: boolean) => void) => () => void +} diff --git a/src/shared/updater-install-events.ts b/src/shared/updater-install-events.ts new file mode 100644 index 00000000000..a3fee1ff02e --- /dev/null +++ b/src/shared/updater-install-events.ts @@ -0,0 +1,3 @@ +/** 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'