From ed4d6979b13c05bc0d5ff066b334a2efcec43d74 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 3 Aug 2026 23:11:36 -0700 Subject: [PATCH] fix(app): await durable checkpoints before restart actions (#12433) * fix(app): await durable checkpoints before restart actions * fix(app): clear restart latch after refused reload * fix(persistence): invalidate hash after stale rename --- .../ipc/renderer-shutdown-checkpoint.test.ts | 2 +- src/main/ipc/renderer-shutdown-checkpoint.ts | 4 +- .../persistence-async-write-syscalls.test.ts | 272 ++++++++++-------- src/main/persistence.ts | 30 +- .../app-restart-checkpoint-routing.test.ts | 93 ++++++ src/preload/index.ts | 45 ++- src/preload/renderer-restart-wiring.test.ts | 8 +- src/preload/renderer-restart-wiring.ts | 21 ++ .../github-project/GhAuthErrorHelp.test.ts | 25 +- .../github-project/GhAuthErrorHelp.tsx | 14 +- 10 files changed, 346 insertions(+), 168 deletions(-) create mode 100644 src/preload/app-restart-checkpoint-routing.test.ts diff --git a/src/main/ipc/renderer-shutdown-checkpoint.test.ts b/src/main/ipc/renderer-shutdown-checkpoint.test.ts index 76fc0509e..abbb77ab0 100644 --- a/src/main/ipc/renderer-shutdown-checkpoint.test.ts +++ b/src/main/ipc/renderer-shutdown-checkpoint.test.ts @@ -74,7 +74,7 @@ describe('registerRendererShutdownCheckpointHandler', () => { ) expect(store.updateUI).toHaveBeenCalledWith({ activeView: 'settings' }) expect(store.flushPendingOrThrowAsync).toHaveBeenCalledTimes(1) - // Why: a live app keeps mutating state, so draining to a stable generation would livelock. + // Why: Store fences the staged generation without draining unrelated live mutations. expect(store.flushPendingOrThrowAsync).toHaveBeenCalledWith( expect.objectContaining({ drainToStableGeneration: false }) ) diff --git a/src/main/ipc/renderer-shutdown-checkpoint.ts b/src/main/ipc/renderer-shutdown-checkpoint.ts index e875f950d..121ff86b2 100644 --- a/src/main/ipc/renderer-shutdown-checkpoint.ts +++ b/src/main/ipc/renderer-shutdown-checkpoint.ts @@ -23,8 +23,8 @@ function flushStagedStateWithDeadline(store: Store): Promise ({ ok: true })) diff --git a/src/main/persistence-async-write-syscalls.test.ts b/src/main/persistence-async-write-syscalls.test.ts index 2b3a6ea68..2d6d58159 100644 --- a/src/main/persistence-async-write-syscalls.test.ts +++ b/src/main/persistence-async-write-syscalls.test.ts @@ -143,7 +143,7 @@ type TestStore = { waitForPendingWrite(): Promise flushOrThrow(): void flushPendingAsync(): Promise - flushPendingOrThrowAsync(): Promise + flushPendingOrThrowAsync(options?: { drainToStableGeneration?: boolean }): Promise upsertSshPtyConsumerRecovery(record: { targetId: string clientInstanceId: string @@ -185,6 +185,36 @@ function dataFile(dir: string): string { return join(dir, 'orca-data.json') } +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void + const promise = new Promise((next) => { + resolve = next + }) + return { promise, resolve } +} + +function recordFsCalls(dir: string): void { + fsCalls.dirPrefix = dir + fsCalls.recording = true +} + +function delayNextDataFileRename(dir: string): ReturnType & { + started: Promise +} { + const release = deferred() + const started = deferred() + let held = false + fsCalls.waitAsync = (fn, target) => { + if (held || fn !== 'rename' || !target.startsWith(dataFile(dir))) { + return null + } + held = true + started.resolve() + return release.promise + } + return { ...release, started: started.promise } +} + function seedStaleBackup(dir: string): void { const path = `${dataFile(dir)}.bak.0` writeFileSync(path, '{"stale":true}', 'utf-8') @@ -232,8 +262,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { sidebarWidth: number ): Promise { store.updateUI({ sidebarWidth }) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { vi.advanceTimersByTime(PAST_ROTATION_INTERVAL_MS) await store.waitForPendingWrite() @@ -322,8 +351,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const staleBackup = readFileSync(`${dataFile(dir)}.bak.0`, 'utf-8') store.updateUI({ sidebarWidth: 361 }) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) let flushed = false fsCalls.beforeAsync = (fn, target) => { if (flushed || fn !== 'stat' || !target.endsWith('.bak.0')) { @@ -351,34 +379,19 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { it('a sync checkpoint vetoes an async write already parked on rename', async () => { const dir = makeDir() const store = await createStore(dir) - let releaseRename!: () => void - const renameRelease = new Promise((resolve) => { - releaseRename = resolve - }) - let signalRename!: () => void - const renameStarted = new Promise((resolve) => { - signalRename = resolve - }) - fsCalls.waitAsync = (fn, target) => { - if (fn !== 'rename' || target === dataFile(dir) || !target.startsWith(dataFile(dir))) { - return null - } - signalRename() - return renameRelease - } + const rename = delayNextDataFileRename(dir) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) store.updateUI({ sidebarWidth: 501 }) vi.advanceTimersByTime(SAVE_DEBOUNCE_MS) const pending = store.waitForPendingWrite() - await renameStarted + await rename.started store.updateUI({ sidebarWidth: 502 }) store.flushOrThrow() expect(JSON.parse(readFileSync(dataFile(dir), 'utf-8')).ui.sidebarWidth).toBe(502) - releaseRename() + rename.resolve() await pending fsCalls.recording = false fsCalls.waitAsync = null @@ -391,8 +404,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const dir = makeDir() const store = await createStore(dir) const errors = vi.spyOn(console, 'error').mockImplementation(() => {}) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) fsCalls.failAsync = (fn, target) => fn === 'rename' && target.startsWith(dataFile(dir)) && !target.includes('.bak.') ? Object.assign(new Error('mount disappeared'), { code: 'ENOENT' }) @@ -413,31 +425,14 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { it('the throwing async barrier drains mutations made during its write', async () => { const dir = makeDir() const store = await createStore(dir) - let releaseRename!: () => void - const renameRelease = new Promise((resolve) => { - releaseRename = resolve - }) - let signalRename!: () => void - const renameStarted = new Promise((resolve) => { - signalRename = resolve - }) - let held = false - fsCalls.waitAsync = (fn, target) => { - if (held || fn !== 'rename' || !target.startsWith(dataFile(dir))) { - return null - } - held = true - signalRename() - return renameRelease - } - fsCalls.dirPrefix = dir - fsCalls.recording = true + const rename = delayNextDataFileRename(dir) + recordFsCalls(dir) store.updateUI({ sidebarWidth: 601 }) const barrier = store.flushPendingOrThrowAsync() - await renameStarted + await rename.started store.updateUI({ sidebarWidth: 602 }) - releaseRename() + rename.resolve() await barrier fsCalls.recording = false @@ -447,31 +442,14 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { it('bounds a best-effort flush to one state generation', async () => { const dir = makeDir() const store = await createStore(dir) - let releaseRename!: () => void - const renameRelease = new Promise((resolve) => { - releaseRename = resolve - }) - let signalRename!: () => void - const renameStarted = new Promise((resolve) => { - signalRename = resolve - }) - let held = false - fsCalls.waitAsync = (fn, target) => { - if (held || fn !== 'rename' || !target.startsWith(dataFile(dir))) { - return null - } - held = true - signalRename() - return renameRelease - } - fsCalls.dirPrefix = dir - fsCalls.recording = true + const rename = delayNextDataFileRename(dir) + recordFsCalls(dir) store.updateUI({ sidebarWidth: 621 }) const flush = store.flushPendingAsync() - await renameStarted + await rename.started store.updateUI({ sidebarWidth: 622 }) - releaseRename() + rename.resolve() await flush fsCalls.recording = false @@ -481,33 +459,96 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { expect(JSON.parse(readFileSync(dataFile(dir), 'utf-8')).ui.sidebarWidth).toBe(622) }) + it('keeps a bounded barrier open when its staged writer is superseded before rename', async () => { + const dir = makeDir() + const store = await createStore(dir) + const openRelease = deferred() + const openStarted = deferred() + const renameRelease = deferred() + const renameStarted = deferred() + let heldOpen = false + let heldRename = false + fsCalls.waitAsync = (fn, target) => { + if ( + !heldOpen && + fn === 'open' && + target.startsWith(dataFile(dir)) && + target.endsWith('.tmp') + ) { + heldOpen = true + openStarted.resolve() + return openRelease.promise + } + if (!heldRename && fn === 'rename' && target.startsWith(dataFile(dir))) { + heldRename = true + renameStarted.resolve() + return renameRelease.promise + } + return null + } + recordFsCalls(dir) + + store.updateUI({ sidebarWidth: 631 }) + const barrier = store.flushPendingOrThrowAsync({ drainToStableGeneration: false }) + await openStarted.promise + store.updateUI({ sidebarWidth: 632 }) + openRelease.resolve() + const firstOutcome = await Promise.race([ + barrier.then(() => 'settled' as const), + renameStarted.promise.then(() => 'retrying' as const) + ]) + + expect(firstOutcome).toBe('retrying') + renameRelease.resolve() + await barrier + fsCalls.recording = false + + expect(JSON.parse(readFileSync(dataFile(dir), 'utf-8')).ui.sidebarWidth).toBe(632) + }) + + it('rewrites a matching hash after a superseded rename installed stale state', async () => { + const dir = makeDir() + const store = await createStore(dir) + store.updateUI({ sidebarWidth: 641 }) + await store.flushPendingOrThrowAsync() + const rename = delayNextDataFileRename(dir) + recordFsCalls(dir) + + store.updateUI({ sidebarWidth: 642 }) + vi.advanceTimersByTime(SAVE_DEBOUNCE_MS) + const staleWrite = store.waitForPendingWrite() + await rename.started + store.updateUI({ sidebarWidth: 641 }) + rename.resolve() + await staleWrite + + expect(JSON.parse(readFileSync(dataFile(dir), 'utf-8')).ui.sidebarWidth).toBe(642) + await store.flushPendingOrThrowAsync({ drainToStableGeneration: false }) + fsCalls.recording = false + + expect(JSON.parse(readFileSync(dataFile(dir), 'utf-8')).ui.sidebarWidth).toBe(641) + }) + it('the throwing async barrier drains mutations made during sidecar I/O', async () => { const dir = makeDir() const store = await createStore(dir) - let releaseRename!: () => void - const renameRelease = new Promise((resolve) => { - releaseRename = resolve - }) - let signalRename!: () => void - const renameStarted = new Promise((resolve) => { - signalRename = resolve - }) + const renameRelease = deferred() + const renameStarted = deferred() fsCalls.waitAsync = (fn, target) => { if (fn !== 'rename' || !target.includes('orca-github-cache.json.')) { return null } - signalRename() - return renameRelease + renameStarted.resolve() + return renameRelease.promise } - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) store.updateUI({ sidebarWidth: 611 }) store.setGitHubCache({ pr: {}, issue: {} }) const barrier = store.flushPendingOrThrowAsync() - await renameStarted + await renameStarted.promise store.updateUI({ sidebarWidth: 612 }) - releaseRename() + renameRelease.resolve() await barrier fsCalls.recording = false @@ -522,42 +563,35 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { utimesSync(`${dataFile(dir)}.bak.0`, almostDueSeconds, almostDueSeconds) const staleBackup = readFileSync(`${dataFile(dir)}.bak.0`, 'utf-8') const statCall = `stat:${dataFile(dir)}.bak.0` - let releaseRotation!: () => void - const rotationRelease = new Promise((resolve) => { - releaseRotation = resolve - }) - let signalRotation!: () => void - const rotationStarted = new Promise((resolve) => { - signalRotation = resolve - }) + const rotationRelease = deferred() + const rotationStarted = deferred() let held = false fsCalls.waitAsync = (fn, target) => { if (held || fn !== 'stat' || target !== `${dataFile(dir)}.bak.0`) { return null } held = true - signalRotation() - return rotationRelease + rotationStarted.resolve() + return rotationRelease.promise } - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) store.updateUI({ sidebarWidth: 371 }) vi.advanceTimersByTime(SAVE_DEBOUNCE_MS) const firstWrite = store.waitForPendingWrite() let allWrites = firstWrite try { - await rotationStarted + await rotationStarted.promise store.updateUI({ sidebarWidth: 372 }) store.flushOrThrow() store.updateUI({ sidebarWidth: 373 }) vi.advanceTimersByTime(SAVE_DEBOUNCE_MS) allWrites = store.waitForPendingWrite() expect(fsCalls.asyncCalls.filter((call) => call === statCall)).toHaveLength(1) - releaseRotation() + rotationRelease.resolve() await Promise.all([firstWrite, allWrites]) } finally { - releaseRotation() + rotationRelease.resolve() await allWrites fsCalls.recording = false fsCalls.waitAsync = null @@ -579,8 +613,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const expectedTarget = `${dataFile(dir)}${targetSuffix}` store.updateUI({ sidebarWidth: 363 }) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) let flushed = false fsCalls.beforeAsync = (fn, target) => { if (flushed || fn !== expectedFn || target !== expectedTarget) { @@ -688,8 +721,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const dir = makeDir() const store = await createStore(dir) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { await store.upsertSshPtyConsumerRecovery(consumerRecovery('client-1')) } finally { @@ -710,8 +742,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const store = await createStore(dir) const writeError = Object.assign(new Error('profile mount rejected write'), { code: 'EIO' }) const errors = vi.spyOn(console, 'error').mockImplementation(() => {}) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) fsCalls.failAsync = (fn, target) => fn === 'open' && target.startsWith(`${dataFile(dir)}.`) ? writeError : null @@ -730,8 +761,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const store = await createStore(dir) await store.upsertSshPtyConsumerRecovery(consumerRecovery('client-1')) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { await store.removeSshPtyConsumerRecovery('ssh-1') } finally { @@ -750,8 +780,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { const store = await createStore(dir) store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { await store.markSshRemotePtyLeasesAsync('ssh-1', 'detached') } finally { @@ -774,8 +803,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { // Why: a PTY that exits mid-reattach is terminated before the batch write lands; it must stay dead. store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-4', state: 'terminated' }) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { await store.markSshRemotePtyLeasesAttachedAsync('ssh-1', ['pty-1', 'pty-2', 'pty-4']) } finally { @@ -799,39 +827,32 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { it('keeps async writers serialized across a synchronous shutdown flush', async () => { const dir = makeDir() const store = await createStore(dir) - let signalFirstOpen!: () => void - const firstOpen = new Promise((resolve) => { - signalFirstOpen = resolve - }) - let releaseFirstOpen!: () => void - const firstOpenRelease = new Promise((resolve) => { - releaseFirstOpen = resolve - }) + const firstOpen = deferred() + const firstOpenRelease = deferred() let held = false fsCalls.waitAsync = (fn, target) => { if (held || fn !== 'open' || !target.endsWith('.tmp')) { return null } held = true - signalFirstOpen() - return firstOpenRelease + firstOpen.resolve() + return firstOpenRelease.promise } - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { const firstWrite = store.upsertSshPtyConsumerRecovery(consumerRecovery('client-1')) - await firstOpen + await firstOpen.promise store.flushOrThrow() const secondWrite = store.upsertSshPtyConsumerRecovery(consumerRecovery('client-2')) await Promise.resolve() await Promise.resolve() expect(fsCalls.asyncCalls.filter((call) => call.startsWith('open:'))).toHaveLength(1) - releaseFirstOpen() + firstOpenRelease.resolve() await Promise.all([firstWrite, secondWrite]) } finally { - releaseFirstOpen() + firstOpenRelease.resolve() fsCalls.recording = false fsCalls.waitAsync = null } @@ -885,8 +906,7 @@ describe('async persistence write path avoids synchronous fs syscalls', () => { seedStaleBackup(dir) store.updateUI({ sidebarWidth: 331 }) - fsCalls.dirPrefix = dir - fsCalls.recording = true + recordFsCalls(dir) try { store.flushOrThrow() } finally { diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 077d9b871..f74f5d926 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2786,6 +2786,7 @@ export class Store { private quitFlushPromise: Promise | null = null // Content hash at last write, to skip no-op writes; derived from the payload with encrypted blobs normalized back to plaintext (see buildStateToSave), since encrypt() uses a random IV per call. private lastWrittenStateHash: string | null = null + private lastDurableWriteGeneration = -1 private firstPendingSaveAt: number | null = null private githubCacheDirty = false private githubCacheGeneration = 0 @@ -3941,6 +3942,7 @@ export class Store { const { payload, stateHash } = this.buildStateToSave() // Why: don't rewrite a byte-identical multi-MB file when state nets out to already-persisted. if (stateHash === this.lastWrittenStateHash) { + this.lastDurableWriteGeneration = Math.max(this.lastDurableWriteGeneration, gen) return } const dataFile = this.dataFile @@ -3976,9 +3978,14 @@ export class Store { this.inFlightAsyncTmpFile = null } } - // Why re-check gen: a sync flush during the rename await may have written fresher state; don't record a stale hash over it. + // Why re-check gen: a mutation or sync flush during rename makes the installed hash ambiguous; invalidate the no-op guard. if (renamed && this.writeGeneration === gen) { this.lastWrittenStateHash = stateHash + } else if (renamed) { + this.lastWrittenStateHash = null + } + if (renamed) { + this.lastDurableWriteGeneration = Math.max(this.lastDurableWriteGeneration, gen) } } finally { if (!renamed) { @@ -4020,6 +4027,10 @@ export class Store { writeFileDurableSync(tmpFile, dataFile, payload) renamed = true this.lastWrittenStateHash = stateHash + this.lastDurableWriteGeneration = Math.max( + this.lastDurableWriteGeneration, + this.writeGeneration + ) } finally { if (!renamed) { try { @@ -7297,7 +7308,7 @@ export class Store { if (this.writesFrozen || this.quitFlushStarted) { return Promise.reject(new Error('Cannot flush while persistence is finalized')) } - return this.flushCurrentStateAsync(false, options.signal, options.drainToStableGeneration) + return this.flushCurrentStateAsync(false, options.signal, options.drainToStableGeneration, true) } // Async twin of flushOrThrow: durable state only. Active-view and GitHub sidecars are @@ -7323,8 +7334,10 @@ export class Store { private async flushCurrentStateAsync( final: boolean, signal?: AbortSignal, - drainToStableGeneration = true + drainToStableGeneration = true, + requireInitialGenerationDurable = false ): Promise { + const requiredDurableGeneration = requireInitialGenerationDurable ? this.writeGeneration : null for (;;) { if (signal?.aborted) { throw new Error('Persistence flush aborted') @@ -7351,7 +7364,16 @@ export class Store { if (signal?.aborted) { throw new Error('Persistence flush aborted') } - if (!drainToStableGeneration || generation === this.writeGeneration) { + if (!drainToStableGeneration) { + if ( + requiredDurableGeneration === null || + this.lastDurableWriteGeneration >= requiredDurableGeneration + ) { + break + } + continue + } + if (generation === this.writeGeneration) { break } } diff --git a/src/preload/app-restart-checkpoint-routing.test.ts b/src/preload/app-restart-checkpoint-routing.test.ts new file mode 100644 index 000000000..9f64569e3 --- /dev/null +++ b/src/preload/app-restart-checkpoint-routing.test.ts @@ -0,0 +1,93 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { PreloadApi } from './api-types' +import { + ORCA_APP_RESTART_ABORTED_EVENT, + ORCA_APP_RESTART_STARTED_EVENT +} from '../shared/updater-renderer-events' + +const { exposeInMainWorld, invoke, on, removeListener, send, sendSync } = vi.hoisted(() => ({ + exposeInMainWorld: vi.fn(), + invoke: vi.fn(), + on: vi.fn(), + removeListener: vi.fn(), + send: vi.fn(), + sendSync: vi.fn() +})) + +vi.mock('electron', () => ({ + contextBridge: { exposeInMainWorld }, + ipcRenderer: { invoke, on, removeListener, send, sendSync }, + webFrame: { + getZoomFactor: vi.fn(() => 1), + setZoomFactor: vi.fn(), + setVisualZoomLevelLimits: vi.fn() + }, + webUtils: { getPathForFile: vi.fn(() => '') } +})) + +vi.mock('@electron-toolkit/preload', () => ({ electronAPI: {} })) + +describe('native preload destructive app actions', () => { + const originalContextIsolated = Object.getOwnPropertyDescriptor(process, 'contextIsolated') + let eventTarget: EventTarget + + beforeEach(() => { + vi.resetModules() + exposeInMainWorld.mockReset() + invoke.mockReset() + on.mockReset() + removeListener.mockReset() + send.mockReset() + sendSync.mockReset() + Object.defineProperty(process, 'contextIsolated', { configurable: true, value: true }) + eventTarget = new EventTarget() + vi.stubGlobal('window', eventTarget) + vi.stubGlobal('document', { addEventListener: vi.fn() }) + }) + + afterEach(() => { + vi.unstubAllGlobals() + if (originalContextIsolated) { + Object.defineProperty(process, 'contextIsolated', originalContextIsolated) + } else { + Reflect.deleteProperty(process, 'contextIsolated') + } + }) + + const loadApi = async (): Promise => { + await import('./index') + return exposeInMainWorld.mock.calls.find(([name]) => name === 'api')?.[1] as PreloadApi + } + + for (const action of ['reload', 'relaunch'] as const) { + it(`prepares and awaits durability before ${action}`, async () => { + const api = await loadApi() + const calls: string[] = [] + eventTarget.addEventListener(ORCA_APP_RESTART_STARTED_EVENT, () => calls.push('prepared')) + invoke.mockImplementation(async (channel: string) => { + calls.push(channel) + return channel === 'app:await-before-unload-checkpoint' ? { ok: true } : undefined + }) + + await api.app[action]() + + expect(calls).toEqual(['prepared', 'app:await-before-unload-checkpoint', `app:${action}`]) + }) + + it(`refuses ${action} when the durable checkpoint fails`, async () => { + const api = await loadApi() + const aborted = vi.fn() + eventTarget.addEventListener(ORCA_APP_RESTART_ABORTED_EVENT, aborted) + invoke.mockImplementation(async (channel: string) => + channel === 'app:await-before-unload-checkpoint' ? { ok: false } : undefined + ) + + await expect(api.app[action]()).rejects.toThrow( + 'Failed to persist renderer state before unload.' + ) + + expect(invoke).not.toHaveBeenCalledWith(`app:${action}`) + expect(aborted).toHaveBeenCalledTimes(1) + }) + } +}) diff --git a/src/preload/index.ts b/src/preload/index.ts index de829aafe..13a450bce 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -241,11 +241,7 @@ import type { } from '../shared/ai-vault-types' import type { AiVaultPrepareSessionResumeArgs } from '../shared/ai-vault-resume-preparation' import type { AgentType } from '../shared/native-chat-types' -import { - ORCA_APP_RESTART_ABORTED_EVENT, - ORCA_APP_RESTART_STARTED_EVENT, - ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT -} from '../shared/updater-renderer-events' +import { ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT } from '../shared/updater-renderer-events' import { ORCA_INTERNAL_FILE_DRAG_TYPE, createNativeFileDropPayload, @@ -281,11 +277,9 @@ import type { } from '../shared/crash-reporting' import type { RendererHeapStatistics } from '../shared/renderer-heap-statistics' import { readRendererHeapStatistics } from './renderer-heap-statistics-reader' +import { createUpdaterQuitAbortRelay } from '../shared/renderer-restart-preparation' import { - createUpdaterQuitAbortRelay, - prepareRendererForAppRestart -} from '../shared/renderer-restart-preparation' -import { + prepareAndInvokeAppRestart, prepareAndInvokeUpdaterInstall, registerRendererRestartIpcRelays } from './renderer-restart-wiring' @@ -486,21 +480,24 @@ const api = { getIdentity: (): Promise => ipcRenderer.invoke('app:getIdentity'), getFeatureWallAssetBaseUrl: (): Promise => ipcRenderer.invoke('app:getFeatureWallAssetBaseUrl'), - relaunch: (): Promise => ipcRenderer.invoke('app:relaunch'), - restart: async (): Promise => { - await prepareRendererForAppRestart(window, { - startedEventName: ORCA_APP_RESTART_STARTED_EVENT, - abortedEventName: ORCA_APP_RESTART_ABORTED_EVENT, - awaitCheckpoint: awaitBeforeUnloadCheckpoint - }) - try { - return await ipcRenderer.invoke('app:restart') - } catch (error) { - window.dispatchEvent(new Event(ORCA_APP_RESTART_ABORTED_EVENT)) - throw error - } - }, - reload: (): Promise => ipcRenderer.invoke('app:reload'), + relaunch: (): Promise => + prepareAndInvokeAppRestart( + window, + () => ipcRenderer.invoke('app:relaunch'), + awaitBeforeUnloadCheckpoint + ), + restart: (): Promise => + prepareAndInvokeAppRestart( + window, + () => ipcRenderer.invoke('app:restart'), + awaitBeforeUnloadCheckpoint + ), + reload: (): Promise => + prepareAndInvokeAppRestart( + window, + () => ipcRenderer.invoke('app:reload'), + awaitBeforeUnloadCheckpoint + ), stageBeforeUnloadSync: (args: Parameters[0]) => { const result = ipcRenderer.sendSync('app:stage-before-unload-sync', args) as { ok?: unknown diff --git a/src/preload/renderer-restart-wiring.test.ts b/src/preload/renderer-restart-wiring.test.ts index 0198c72a4..06be6537f 100644 --- a/src/preload/renderer-restart-wiring.test.ts +++ b/src/preload/renderer-restart-wiring.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it, vi } from 'vitest' import { ORCA_RENDERER_UNLOAD_PREVENTED_EVENT } from '../shared/renderer-shutdown-events' -import { ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT } from '../shared/updater-renderer-events' +import { + ORCA_APP_RESTART_ABORTED_EVENT, + ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT +} from '../shared/updater-renderer-events' import { prepareAndInvokeUpdaterInstall, registerRendererRestartIpcRelays @@ -10,6 +13,7 @@ describe('renderer restart wiring', () => { it('relays updater status and prevented unload events', () => { const eventTarget = new EventTarget() const unloadPrevented = vi.fn() + const restartAborted = vi.fn() const handleStatus = vi.fn() const listeners = new Map void>() const ipcRenderer = { @@ -19,6 +23,7 @@ describe('renderer restart wiring', () => { }) } as unknown as Parameters[0] eventTarget.addEventListener(ORCA_RENDERER_UNLOAD_PREVENTED_EVENT, unloadPrevented) + eventTarget.addEventListener(ORCA_APP_RESTART_ABORTED_EVENT, restartAborted) registerRendererRestartIpcRelays(ipcRenderer, eventTarget, { handleStatus }) listeners.get('updater:status')?.({}, { state: 'error', message: 'install failed' }) @@ -27,6 +32,7 @@ describe('renderer restart wiring', () => { expect(ipcRenderer.on).toHaveBeenCalledTimes(2) expect(handleStatus).toHaveBeenCalledWith({ state: 'error', message: 'install failed' }) expect(unloadPrevented).toHaveBeenCalledTimes(1) + expect(restartAborted).toHaveBeenCalledTimes(1) }) it('marks preparation before invoking main and aborts on IPC failure', async () => { diff --git a/src/preload/renderer-restart-wiring.ts b/src/preload/renderer-restart-wiring.ts index 254d38640..9b140e711 100644 --- a/src/preload/renderer-restart-wiring.ts +++ b/src/preload/renderer-restart-wiring.ts @@ -6,6 +6,8 @@ import { } from '../shared/renderer-restart-preparation' import type { UpdateStatus } from '../shared/types' import { + ORCA_APP_RESTART_ABORTED_EVENT, + ORCA_APP_RESTART_STARTED_EVENT, ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT, ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT } from '../shared/updater-renderer-events' @@ -20,6 +22,7 @@ export function registerRendererRestartIpcRelays( }) ipcRenderer.on('window:unload-prevented', () => { eventTarget.dispatchEvent(new Event(ORCA_RENDERER_UNLOAD_PREVENTED_EVENT)) + eventTarget.dispatchEvent(new Event(ORCA_APP_RESTART_ABORTED_EVENT)) }) } @@ -42,3 +45,21 @@ export async function prepareAndInvokeUpdaterInstall( throw error } } + +export async function prepareAndInvokeAppRestart( + eventTarget: EventTarget, + invoke: () => Promise, + awaitCheckpoint: () => Promise +): Promise { + await prepareRendererForAppRestart(eventTarget, { + startedEventName: ORCA_APP_RESTART_STARTED_EVENT, + abortedEventName: ORCA_APP_RESTART_ABORTED_EVENT, + awaitCheckpoint + }) + try { + await invoke() + } catch (error) { + eventTarget.dispatchEvent(new Event(ORCA_APP_RESTART_ABORTED_EVENT)) + throw error + } +} diff --git a/src/renderer/src/components/github-project/GhAuthErrorHelp.test.ts b/src/renderer/src/components/github-project/GhAuthErrorHelp.test.ts index da666d545..248783de9 100644 --- a/src/renderer/src/components/github-project/GhAuthErrorHelp.test.ts +++ b/src/renderer/src/components/github-project/GhAuthErrorHelp.test.ts @@ -1,11 +1,16 @@ -import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import type { GhAuthDiagnostic } from '../../../../shared/github-auth-types' vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) -import { buildRemediation } from './GhAuthErrorHelp' +import { buildRemediation, reloadOrcaRenderer } from './GhAuthErrorHelp' + +afterEach(() => { + vi.restoreAllMocks() + vi.unstubAllGlobals() +}) function diagnostic(overrides: Partial = {}): GhAuthDiagnostic { return { @@ -57,4 +62,20 @@ describe('GitHub Project auth remediation host routing', () => { } ]) }) + + it('does not force navigation when the checkpointed reload is refused', async () => { + const reload = vi.fn(() => Promise.reject(new Error('checkpoint failed'))) + const locationReload = vi.fn() + const errors = vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.stubGlobal('window', { + api: { app: { reload } }, + location: { reload: locationReload } + }) + + reloadOrcaRenderer() + await vi.waitFor(() => expect(errors).toHaveBeenCalledTimes(1)) + + expect(reload).toHaveBeenCalledTimes(1) + expect(locationReload).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/components/github-project/GhAuthErrorHelp.tsx b/src/renderer/src/components/github-project/GhAuthErrorHelp.tsx index 30d1ea61b..7bbeff291 100644 --- a/src/renderer/src/components/github-project/GhAuthErrorHelp.tsx +++ b/src/renderer/src/components/github-project/GhAuthErrorHelp.tsx @@ -40,14 +40,12 @@ function refreshCommandForHost(host: string | null | undefined): string { // macOS/Linux vs PowerShell on Windows. const IS_WINDOWS = typeof navigator !== 'undefined' && /Win(dows|32|64)/i.test(navigator.userAgent) -function reloadOrcaRenderer(): void { - const reload = window.api.app.reload - if (typeof reload !== 'function') { - window.location.reload() - return - } - void reload().catch(() => { - window.location.reload() +export function reloadOrcaRenderer(): void { + void window.api.app.reload().catch((error) => { + console.error( + '[github-projects] Renderer reload refused:', + error instanceof Error ? error.name : typeof error + ) }) }