From 99ad116232ea4ee712a06a32460ac6963174b71e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:18:40 -0700 Subject: [PATCH] fix(updater): isolate shutdown checkpoint retries Reset full-session staging retry state when a shutdown attempt is abandoned, and route Terminal-less closes through the same scoped synthetic checkpoint as mounted workspaces. Keep arbitrary thrown-value diagnostics non-throwing and localize the quit failure toast. --- .../shutdown-checkpoint-persist.test.ts | 45 +++++++++--- .../app-shell/shutdown-checkpoint-persist.ts | 26 +++++-- .../app-shell/use-app-session-persistence.ts | 72 ++++++++++--------- src/renderer/src/app-startup-routing.test.ts | 10 ++- src/renderer/src/components/Terminal.tsx | 8 ++- .../window-close-request-coordinator.test.ts | 22 +++++- .../window-close-request-coordinator.ts | 7 +- src/renderer/src/i18n/locales/en.json | 3 +- .../src/lib/shutdown-checkpoint-guard.test.ts | 19 +++++ .../src/lib/shutdown-checkpoint-guard.ts | 9 ++- src/shared/renderer-shutdown-events.ts | 8 +++ 11 files changed, 167 insertions(+), 62 deletions(-) diff --git a/src/renderer/src/app-shell/shutdown-checkpoint-persist.test.ts b/src/renderer/src/app-shell/shutdown-checkpoint-persist.test.ts index cfd72cffbba..20600136dfe 100644 --- a/src/renderer/src/app-shell/shutdown-checkpoint-persist.test.ts +++ b/src/renderer/src/app-shell/shutdown-checkpoint-persist.test.ts @@ -29,7 +29,7 @@ describe('createShutdownCheckpointPersist', () => { } }) - expect(createShutdownCheckpointPersist(deps)).not.toThrow() + expect(createShutdownCheckpointPersist(deps).run).not.toThrow() // The full snapshot must still be staged — a weaker resume record for done // panes is strictly better than blocking the update. expect(deps.stageBeforeUnloadSync).toHaveBeenCalledWith({ @@ -39,11 +39,24 @@ describe('createShutdownCheckpointPersist', () => { vi.restoreAllMocks() }) + it('keeps sleeping-capture diagnostics non-throwing for unstringifiable values', () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + const deps = makeDeps({ + captureSleepingAgentSessions: () => { + throw Object.create(null) + } + }) + + expect(createShutdownCheckpointPersist(deps).run).not.toThrow() + expect(deps.stageBeforeUnloadSync).toHaveBeenCalledTimes(1) + vi.restoreAllMocks() + }) + it('keeps the first full-staging failure a visible, retryable error (STA-5505)', () => { const stageBeforeUnloadSync = vi.fn(() => { throw new Error('sync IPC staging failed') }) - const persist = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) + const { run: persist } = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) // A transient failure must not silently cost the full snapshot on attempt one. expect(persist).toThrow('sync IPC staging failed') @@ -57,7 +70,7 @@ describe('createShutdownCheckpointPersist', () => { throw new Error('sync IPC staging failed') } }) - const persist = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) + const { run: persist } = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) expect(persist).toThrow('sync IPC staging failed') expect(persist).not.toThrow() @@ -77,7 +90,7 @@ describe('createShutdownCheckpointPersist', () => { throw new Error('transient staging failure') } }) - const persist = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) + const { run: persist } = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) expect(persist).toThrow('transient staging failure') expect(persist).not.toThrow() @@ -94,7 +107,7 @@ describe('createShutdownCheckpointPersist', () => { throw new Error('sync IPC staging failed') }) }) - const persist = createShutdownCheckpointPersist(deps) + const { run: persist } = createShutdownCheckpointPersist(deps) expect(persist).toThrow('sync IPC staging failed') expect(persist).toThrow('sync IPC staging failed') @@ -107,7 +120,7 @@ describe('createShutdownCheckpointPersist', () => { throw new Error('sync IPC staging failed') }) }) - const persist = createShutdownCheckpointPersist(deps) + const { run: persist } = createShutdownCheckpointPersist(deps) expect(persist).toThrow('sync IPC staging failed') expect(persist).toThrow('sync IPC staging failed') @@ -124,7 +137,7 @@ describe('createShutdownCheckpointPersist', () => { }) }) - expect(createShutdownCheckpointPersist(deps)).toThrow('durable staging failed') + expect(createShutdownCheckpointPersist(deps).run).toThrow('durable staging failed') vi.restoreAllMocks() }) @@ -136,7 +149,7 @@ describe('createShutdownCheckpointPersist', () => { } }) - expect(createShutdownCheckpointPersist(deps)).not.toThrow() + expect(createShutdownCheckpointPersist(deps).run).not.toThrow() expect(deps.stageBeforeUnloadSync).toHaveBeenCalledTimes(1) expect(deps.stageBeforeUnloadSync).toHaveBeenCalledWith({ sessions: [], @@ -153,14 +166,14 @@ describe('createShutdownCheckpointPersist', () => { } }) - expect(createShutdownCheckpointPersist(deps)).toThrow('snapshot build failed') + expect(createShutdownCheckpointPersist(deps).run).toThrow('snapshot build failed') expect(deps.stageBeforeUnloadSync).not.toHaveBeenCalled() }) it('skips capture and stages empty sessions before hydration completes', () => { const deps = makeDeps({ shouldCaptureSession: () => false }) - createShutdownCheckpointPersist(deps)() + createShutdownCheckpointPersist(deps).run() expect(deps.captureTerminalBuffers).not.toHaveBeenCalled() expect(deps.captureSleepingAgentSessions).not.toHaveBeenCalled() @@ -169,4 +182,16 @@ describe('createShutdownCheckpointPersist', () => { ui: { activeView: 'workspace' } }) }) + + it('forgets a failed attempt when an aborted shutdown resets the lifecycle', () => { + const stageBeforeUnloadSync = vi.fn(() => { + throw new Error('sync IPC staging failed') + }) + const persist = createShutdownCheckpointPersist(makeDeps({ stageBeforeUnloadSync })) + + expect(persist.run).toThrow('sync IPC staging failed') + persist.reset() + expect(persist.run).toThrow('sync IPC staging failed') + expect(stageBeforeUnloadSync).toHaveBeenCalledTimes(2) + }) }) diff --git a/src/renderer/src/app-shell/shutdown-checkpoint-persist.ts b/src/renderer/src/app-shell/shutdown-checkpoint-persist.ts index 75cd2ec0660..17d71729a8c 100644 --- a/src/renderer/src/app-shell/shutdown-checkpoint-persist.ts +++ b/src/renderer/src/app-shell/shutdown-checkpoint-persist.ts @@ -1,4 +1,5 @@ import type { PersistedUIState } from '../../../shared/persisted-ui-state-types' +import { formatShutdownCheckpointFailureReason } from '../../../shared/renderer-shutdown-events' import type { WorkspaceSessionHostSnapshot } from '../lib/workspace-session-host-persistence' import { recordRendererCrashBreadcrumb } from '../lib/crash-breadcrumb-recorder' @@ -21,16 +22,23 @@ export type ShutdownCheckpointPersistDeps = { stageBeforeUnloadSync: (args: ShutdownCheckpointStageArgs) => void } -/** Returns the synchronous body of the shutdown checkpoint: capture renderer-owned - * state, then stage everything durable through one main-process call. Throwing - * fails the checkpoint and blocks the restart, so only unstageable data may throw. +export type ShutdownCheckpointPersist = { + run: () => void + reset: () => void +} + +/** Returns the shutdown checkpoint attempt lifecycle. Running it captures renderer-owned + * state, then stages everything durable through one main-process call; reset discards + * retry state when that shutdown attempt is abandoned. Only unstageable data may throw. * * A factory rather than a bare function so full-session staging failures can stay * visible-and-retryable on the first attempt and only degrade on a repeat: a * transient failure gets its retry, a deterministic one can't strand the user. */ -export function createShutdownCheckpointPersist(deps: ShutdownCheckpointPersistDeps): () => void { +export function createShutdownCheckpointPersist( + deps: ShutdownCheckpointPersistDeps +): ShutdownCheckpointPersist { let fullStagingFailedOnPriorAttempt = false - return (): void => { + const run = (): void => { const shouldCaptureSession = deps.shouldCaptureSession() if (shouldCaptureSession) { deps.captureTerminalBuffers() @@ -43,7 +51,7 @@ export function createShutdownCheckpointPersist(deps: ShutdownCheckpointPersistD // alternative (no update, or a SIGKILL'd quit losing the whole snapshot). console.error('[app] Sleeping-agent quit capture failed; continuing checkpoint', error) recordRendererCrashBreadcrumb('renderer_shutdown_sleeping_capture_failed', { - message: error instanceof Error ? error.message : String(error) + message: formatShutdownCheckpointFailureReason(error) }) } } @@ -92,4 +100,10 @@ export function createShutdownCheckpointPersist(deps: ShutdownCheckpointPersistD } fullStagingFailedOnPriorAttempt = false } + return { + run, + reset: () => { + fullStagingFailedOnPriorAttempt = false + } + } } diff --git a/src/renderer/src/app-shell/use-app-session-persistence.ts b/src/renderer/src/app-shell/use-app-session-persistence.ts index a48a11cb644..8a0ab3bf49c 100644 --- a/src/renderer/src/app-shell/use-app-session-persistence.ts +++ b/src/renderer/src/app-shell/use-app-session-persistence.ts @@ -121,42 +121,44 @@ export function useAppSessionPersistence(): void { // two firings, PTY exit events can arrive and unmount TerminalPanes, // emptying shutdownBufferCaptures. The guard prevents the second call // from overwriting the good session data with an empty snapshot. - const shutdownCheckpoint = createShutdownCheckpointGuard( - createShutdownCheckpointPersist({ - shouldCaptureSession: () => shouldPersistWorkspaceSession(useAppStore.getState()), - captureTerminalBuffers: () => { - for (const capture of shutdownBufferCaptures.values()) { - try { - capture({ includeLocalBuffers: false }) - } catch { - // Don't let one pane's failure block the rest. - } + const shutdownCheckpointPersist = createShutdownCheckpointPersist({ + shouldCaptureSession: () => shouldPersistWorkspaceSession(useAppStore.getState()), + captureTerminalBuffers: () => { + for (const capture of shutdownBufferCaptures.values()) { + try { + capture({ includeLocalBuffers: false }) + } catch { + // Don't let one pane's failure block the rest. } - }, - // Why: agent provider session ids live only in agentStatusByPaneKey, - // which is in-memory. Capture them into the persisted sleeping-session - // map so a daemon/session death while the app is closed can still - // cold-restore via the agent's resume command (#5232). - captureSleepingAgentSessions: () => - useAppStore.getState().captureAllSleepingAgentSessions('quit'), - // Why: re-read state after capture() calls populated scrollback buffers - // into the store via Zustand setters. The shouldCaptureSession read is - // only for the gating flags and would miss those updates. - buildSessionSnapshots: () => { - const freshState = useAppStore.getState() - return buildWorkspaceSessionHostSnapshots( - buildWorkspaceSessionPayload(freshState), - freshState - ) - }, - buildUiPatch: () => buildActiveViewUnloadPatch(useAppStore.getState()), - hasDirtyOpenFiles: () => useAppStore.getState().openFiles.some((file) => file.isDirty), - // Why: an app-level quit degrades too — the pre-fix alternative was a quit - // the user could only complete with SIGKILL, which loses strictly more (#15352). - isDegradableShutdownInProgress: () => - isIntentionalAppRestartInProgress() || isWindowCloseCheckpointInProgress(), - stageBeforeUnloadSync: (args) => window.api.app.stageBeforeUnloadSync(args) - }) + } + }, + // Why: agent provider session ids live only in agentStatusByPaneKey, + // which is in-memory. Capture them into the persisted sleeping-session + // map so a daemon/session death while the app is closed can still + // cold-restore via the agent's resume command (#5232). + captureSleepingAgentSessions: () => + useAppStore.getState().captureAllSleepingAgentSessions('quit'), + // Why: re-read state after capture() calls populated scrollback buffers + // into the store via Zustand setters. The shouldCaptureSession read is + // only for the gating flags and would miss those updates. + buildSessionSnapshots: () => { + const freshState = useAppStore.getState() + return buildWorkspaceSessionHostSnapshots( + buildWorkspaceSessionPayload(freshState), + freshState + ) + }, + buildUiPatch: () => buildActiveViewUnloadPatch(useAppStore.getState()), + hasDirtyOpenFiles: () => useAppStore.getState().openFiles.some((file) => file.isDirty), + // Why: an app-level quit degrades too — the pre-fix alternative was a quit + // the user could only complete with SIGKILL, which loses strictly more (#15352). + isDegradableShutdownInProgress: () => + isIntentionalAppRestartInProgress() || isWindowCloseCheckpointInProgress(), + stageBeforeUnloadSync: (args) => window.api.app.stageBeforeUnloadSync(args) + }) + const shutdownCheckpoint = createShutdownCheckpointGuard( + shutdownCheckpointPersist.run, + shutdownCheckpointPersist.reset ) const persistBeforeUnload = createShutdownCheckpointBeforeUnloadHandler(shutdownCheckpoint) window.addEventListener('beforeunload', persistBeforeUnload) diff --git a/src/renderer/src/app-startup-routing.test.ts b/src/renderer/src/app-startup-routing.test.ts index 63f2011b51f..273ae36ee38 100644 --- a/src/renderer/src/app-startup-routing.test.ts +++ b/src/renderer/src/app-startup-routing.test.ts @@ -488,7 +488,7 @@ describe('renderer startup runtime routing', () => { it('checkpoints activeView and all session snapshots through one beforeunload handler (#9002)', () => { const source = readSource(SESSION_PERSISTENCE_PATH) const checkpointStart = source.indexOf( - 'const shutdownCheckpoint = createShutdownCheckpointGuard(' + 'const shutdownCheckpointPersist = createShutdownCheckpointPersist({' ) const checkpointEnd = source.indexOf( 'const persistBeforeUnload = createShutdownCheckpointBeforeUnloadHandler(shutdownCheckpoint)', @@ -498,9 +498,11 @@ describe('renderer startup runtime routing', () => { expect(checkpointEnd).toBeGreaterThan(checkpointStart) const checkpointBlock = source.slice(checkpointStart, checkpointEnd) - expect(checkpointBlock).toContain('createShutdownCheckpointPersist({') expect(checkpointBlock).toContain( - 'buildWorkspaceSessionHostSnapshots(\n buildWorkspaceSessionPayload(freshState),\n freshState\n )' + 'const shutdownCheckpointPersist = createShutdownCheckpointPersist({' + ) + expect(checkpointBlock).toContain( + 'buildWorkspaceSessionHostSnapshots(\n buildWorkspaceSessionPayload(freshState),\n freshState\n )' ) expect(checkpointBlock).toContain('buildUiPatch: () => buildActiveViewUnloadPatch(') // Why pin the exact gate: the degrade tiers must arm only for intentional @@ -514,6 +516,8 @@ describe('renderer startup runtime routing', () => { expect(checkpointBlock).toContain( 'stageBeforeUnloadSync: (args) => window.api.app.stageBeforeUnloadSync(args)' ) + expect(checkpointBlock).toContain('shutdownCheckpointPersist.run') + expect(checkpointBlock).toContain('shutdownCheckpointPersist.reset') expect(source).toContain( 'window.addEventListener(ORCA_APP_RESTART_ABORTED_EVENT, shutdownCheckpoint.reset)' ) diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index fa9f5f1b9c2..3b951c6b6f1 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -539,7 +539,13 @@ function Terminal(): React.JSX.Element | null { // no reason — its deferred dialog flow already gives the user a surface. const reason = consumeShutdownCheckpointFailureReason() if (reason) { - toast.error(`Quit canceled: the session snapshot could not be saved (${reason}).`) + toast.error( + translate( + 'auto.components.Terminal.quitSnapshotSaveFailed', + 'Quit canceled: the session snapshot could not be saved ({{value0}}).', + { value0: reason } + ) + ) } return } diff --git a/src/renderer/src/components/window-close-request-coordinator.test.ts b/src/renderer/src/components/window-close-request-coordinator.test.ts index 5ff742e5f98..b89a04ff052 100644 --- a/src/renderer/src/components/window-close-request-coordinator.test.ts +++ b/src/renderer/src/components/window-close-request-coordinator.test.ts @@ -20,9 +20,11 @@ describe('window-close-request-coordinator', () => { confirmWindowClose.mockClear() // Why: dispatch falls back to the preload bridge when no rich handler is // registered; stub just the surface it touches. - ;( - globalThis as unknown as { window: { api: { ui: { confirmWindowClose: () => void } } } } - ).window = { api: { ui: { confirmWindowClose } } } + const windowTarget = new EventTarget() as EventTarget & { + api: { ui: { confirmWindowClose: () => void } } + } + windowTarget.api = { ui: { confirmWindowClose } } + ;(globalThis as unknown as { window: typeof windowTarget }).window = windowTarget }) afterEach(() => { @@ -55,6 +57,20 @@ describe('window-close-request-coordinator', () => { expect(confirmWindowClose).toHaveBeenCalledTimes(1) }) + it('runs the Terminal-less close checkpoint in the degradable close scope', async () => { + const beforeUnload = vi.fn((event: Event) => { + expect(isWindowCloseCheckpointInProgress()).toBe(true) + event.preventDefault() + }) + window.addEventListener('beforeunload', beforeUnload, { once: true }) + + await dispatchWindowCloseRequest({ isQuitting: true }) + + expect(beforeUnload).toHaveBeenCalledTimes(1) + expect(confirmWindowClose).not.toHaveBeenCalled() + expect(isWindowCloseCheckpointInProgress()).toBe(false) + }) + it('delegates to the rich handler and does NOT confirm directly when one is registered', async () => { const handler = vi.fn() setWindowCloseRequestHandler(handler) diff --git a/src/renderer/src/components/window-close-request-coordinator.ts b/src/renderer/src/components/window-close-request-coordinator.ts index 5b6542a7330..c7ce3c1e62a 100644 --- a/src/renderer/src/components/window-close-request-coordinator.ts +++ b/src/renderer/src/components/window-close-request-coordinator.ts @@ -86,5 +86,10 @@ export async function dispatchWindowCloseRequest(data: { isQuitting: boolean }): activeHandler(data) return } - window.api.ui.confirmWindowClose() + const accepted = runWithWindowCloseCheckpointScope(() => + window.dispatchEvent(new Event('beforeunload', { cancelable: true })) + ) + if (accepted) { + window.api.ui.confirmWindowClose() + } } diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 970aca9702a..05c2543aa54 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -2099,7 +2099,8 @@ "61ed600d29": "\"{{value0}}\" has unsaved changes. Do you want to save before closing?", "cdc9ac4b2d": "editor", "e57db40c11": "Could not build launch command for {{value0}}.", - "5b2c1a9e44": "No agent CLI detected — install one or pick a default agent in Settings." + "5b2c1a9e44": "No agent CLI detected — install one or pick a default agent in Settings.", + "quitSnapshotSaveFailed": "Quit canceled: the session snapshot could not be saved ({{value0}})." }, "TerminalSearch": { "db234b7519": "Close", diff --git a/src/renderer/src/lib/shutdown-checkpoint-guard.test.ts b/src/renderer/src/lib/shutdown-checkpoint-guard.test.ts index ebc7d682998..47c48834376 100644 --- a/src/renderer/src/lib/shutdown-checkpoint-guard.test.ts +++ b/src/renderer/src/lib/shutdown-checkpoint-guard.test.ts @@ -84,6 +84,25 @@ describe('createShutdownCheckpointGuard', () => { expect(consumeShutdownCheckpointFailureReason()).toBeNull() }) + it('keeps failure reporting non-throwing for unstringifiable thrown values', () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + const guard = createShutdownCheckpointGuard(() => { + throw Object.create(null) + }) + + expect(guard.persistOnce()).toBe(false) + expect(consumeShutdownCheckpointFailureReason()).toBe('Unknown shutdown checkpoint failure') + }) + + it('resets state owned by the persist attempt lifecycle', () => { + const resetPersistAttempt = vi.fn() + const guard = createShutdownCheckpointGuard(vi.fn(), resetPersistAttempt) + + guard.reset() + + expect(resetPersistAttempt).toHaveBeenCalledTimes(1) + }) + it('reports checkpoint failure separately from the unload verdict', () => { const eventTarget = new EventTarget() const failed = vi.fn() diff --git a/src/renderer/src/lib/shutdown-checkpoint-guard.ts b/src/renderer/src/lib/shutdown-checkpoint-guard.ts index a5505894c60..d88fa9a5852 100644 --- a/src/renderer/src/lib/shutdown-checkpoint-guard.ts +++ b/src/renderer/src/lib/shutdown-checkpoint-guard.ts @@ -1,5 +1,6 @@ import { clearShutdownCheckpointFailureReason, + formatShutdownCheckpointFailureReason, ORCA_RENDERER_SHUTDOWN_CHECKPOINT_FAILED_EVENT, ORCA_RENDERER_UNLOAD_PREVENTED_EVENT, publishShutdownCheckpointFailureReason @@ -15,12 +16,15 @@ export type ShutdownCheckpointGuard = { // build behind an error that names the symptom while the cause is swallowed (STA-5505). function reportShutdownCheckpointFailure(error: unknown): void { console.error('[app] Shutdown checkpoint persist failed:', error) - const message = error instanceof Error ? error.message : String(error) + const message = formatShutdownCheckpointFailureReason(error) publishShutdownCheckpointFailureReason(message) recordRendererCrashBreadcrumb('renderer_shutdown_checkpoint_failed', { message }) } -export function createShutdownCheckpointGuard(persist: () => void): ShutdownCheckpointGuard { +export function createShutdownCheckpointGuard( + persist: () => void, + resetPersistAttempt?: () => void +): ShutdownCheckpointGuard { let persisted = false return { persistOnce(): boolean { @@ -41,6 +45,7 @@ export function createShutdownCheckpointGuard(persist: () => void): ShutdownChec }, reset(): void { persisted = false + resetPersistAttempt?.() } } } diff --git a/src/shared/renderer-shutdown-events.ts b/src/shared/renderer-shutdown-events.ts index b55831b818b..ef52094d17b 100644 --- a/src/shared/renderer-shutdown-events.ts +++ b/src/shared/renderer-shutdown-events.ts @@ -9,6 +9,14 @@ export const ORCA_RENDERER_SHUTDOWN_CHECKPOINT_FAILED_EVENT = export const ORCA_SHUTDOWN_CHECKPOINT_FAILURE_REASON_ATTRIBUTE = 'data-orca-shutdown-checkpoint-failure' +export function formatShutdownCheckpointFailureReason(error: unknown): string { + try { + return String(error instanceof Error ? error.message : error) + } catch { + return 'Unknown shutdown checkpoint failure' + } +} + export function publishShutdownCheckpointFailureReason(reason: string): void { try { globalThis.document?.documentElement?.setAttribute(