mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 08:03:33 +00:00
fix(updater): refuse to open a new dashboard popout mid-install
Closes the P1 from re-review. A popout opened during the Linux package revalidation window never saw the broadcast, and its authoritative seed is an IPC round trip - which can go unanswered because the deb/rpm install blocks main inside spawnSync. Its first lazy render would then read a swapped archive and request the doomed recovery reload (measured at 777ms by the reviewer's oracle). Refusing to create the window is the safe answer: the app is seconds from restarting, and an already-open popout is still focused normally. Also: drop the unused syncUpdaterInstallCommitmentTo (it had isDestroyed() outside its own try), centralise the IPC channel names in src/shared so main and preload cannot drift, lock the outer getAllWindows() catch, and cover the renderer commitment module and the web build's always-false.
This commit is contained in:
@@ -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()
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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) => {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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<boolean>(() => 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)
|
||||
})
|
||||
})
|
||||
@@ -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<boolean>
|
||||
onInstallCommitted: (cb: (committed: boolean) => void) => () => void
|
||||
}
|
||||
@@ -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'
|
||||
Reference in New Issue
Block a user