From bc12956cd184874d8473a8bb0b94b5e7b15e7cae Mon Sep 17 00:00:00 2001 From: Sherlock Yin Date: Sun, 20 Sep 2026 18:41:30 +0800 Subject: [PATCH] fix(desktop): bound the quit sequence so a stuck stage cannot strand an update Installing a Desktop update could leave the app running forever with its Runtime Host already retired. Every Host-backed IPC channel failed, the Projects list emptied to "No project", model refresh reported a network error that had nothing to do with the network, and the update was never applied. Only a manual relaunch recovered, after which it failed again. installUpdate retires the Host before handing off to the updater, and that retirement only unwinds through its rollback. The quit that was supposed to follow was awaited without any upper bound: prepareToQuit() and cleanup() each wait on Host retirement, MCP child processes, peer mesh teardown and native resources, any of which can stop settling. When one did, phase stayed in preparing/cleaning and every later quit request was swallowed by the "if (phase !== 'running') return" guard after the event had already been prevented. Squirrel's ShipIt waits for this process to exit before replacing the bundle, so the install blocked indefinitely with nothing to read. Bound both stages and give the sequence a way out. Preparation is a courtesy to the Host, so exceeding its bound reports and continues; refusing to quit is the worse outcome. Cleanup has nothing left to wait for, so exceeding its bound exits the process outright. A quit that is cancelled or fails now reports through onQuitAbandoned, which releases the install handoff: the coordinator knew the quit was abandoned but had no way to say so, leaving the Host retired with nothing left to restart it. Timeouts report through their own channel rather than the error sinks. A stage that stopped answering is not a Host that refused, and conflating them invents a failure the product then has to explain. Generated-by: Maka (Claude) --- .../__tests__/app-quit-coordinator.test.ts | 212 ++++++++++++++++++ .../main/__tests__/app-update-service.test.ts | 34 +++ apps/desktop/src/main/app-quit-coordinator.ts | 157 ++++++++++--- apps/desktop/src/main/app-update-service.ts | 10 + apps/desktop/src/main/runtime-host-boot.ts | 10 + 5 files changed, 387 insertions(+), 36 deletions(-) diff --git a/apps/desktop/src/main/__tests__/app-quit-coordinator.test.ts b/apps/desktop/src/main/__tests__/app-quit-coordinator.test.ts index 190516dd4e..db1a26f7f8 100644 --- a/apps/desktop/src/main/__tests__/app-quit-coordinator.test.ts +++ b/apps/desktop/src/main/__tests__/app-quit-coordinator.test.ts @@ -29,6 +29,7 @@ describe('app quit coordinator', () => { prepareToQuit: async () => 'ready' as const, cleanup: async () => {}, focusOrCreateWindow: () => {}, + forceExit: () => {}, onPreparationError: () => {}, onCleanupError: () => {}, onWindowCreationError: () => {}, @@ -72,6 +73,7 @@ describe('app quit coordinator', () => { focusOrCreateWindow: () => { focusOrCreateCount += 1; }, + forceExit: () => {}, onPreparationError: () => {}, onCleanupError: () => {}, onWindowCreationError: () => {}, @@ -119,6 +121,7 @@ describe('app quit coordinator', () => { focusOrCreateCount += 1; windowCreationSignal = signal; }, + forceExit: () => {}, onPreparationError: () => {}, onCleanupError: () => {}, onWindowCreationError: () => {}, @@ -145,6 +148,7 @@ describe('app quit coordinator', () => { focusOrCreateWindow: () => { focusOrCreateCount += 1; }, + forceExit: () => {}, onPreparationError: () => {}, onCleanupError: () => {}, onWindowCreationError: () => {}, @@ -170,6 +174,7 @@ describe('app quit coordinator', () => { focusOrCreateWindow: async () => { throw failure; }, + forceExit: () => {}, onPreparationError: () => {}, onCleanupError: () => {}, onWindowCreationError: (error) => reportedErrors.push(error), @@ -202,6 +207,7 @@ describe('app quit coordinator', () => { focusOrCreateWindow: () => { focusOrCreateCount += 1; }, + forceExit: () => {}, onPreparationError: (error) => reportedErrors.push(error), onCleanupError: () => {}, onWindowCreationError: () => {}, @@ -239,6 +245,7 @@ describe('app quit coordinator', () => { focusOrCreateWindow: () => { focusOrCreateCount += 1; }, + forceExit: () => {}, onPreparationError: () => {}, onCleanupError: (error: unknown) => { reportedErrors.push(error); @@ -265,8 +272,213 @@ describe('app quit coordinator', () => { assert.equal(resumeQuitCount, 1); assert.equal(secondQuitPrevented, false); }); + + it('forces exit when cleanup never settles', async () => { + const timeouts: Array<{ stage: string; timeoutMs: number }> = []; + let forceExitCount = 0; + let resumeQuitCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: async () => 'ready', + cleanup: () => new Promise(() => {}), + focusOrCreateWindow: () => {}, + forceExit: () => { + forceExitCount += 1; + }, + onPreparationError: () => {}, + onCleanupError: () => {}, + onWindowCreationError: () => {}, + onQuitTimeout: (stage, timeoutMs) => { + timeouts.push({ stage, timeoutMs }); + }, + resumeQuit: () => { + resumeQuitCount += 1; + }, + cleanupTimeoutMs: 10, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await delay(60); + + assert.deepEqual(timeouts, [{ stage: 'cleaning', timeoutMs: 10 }]); + assert.equal(forceExitCount, 1); + assert.equal(resumeQuitCount, 0); + }); + + it('still quits when quit preparation never settles', async () => { + const timeouts: string[] = []; + let cleanupCount = 0; + let forceExitCount = 0; + let resumeQuitCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: () => new Promise<'ready' | 'cancelled'>(() => {}), + cleanup: async () => { + cleanupCount += 1; + }, + focusOrCreateWindow: () => {}, + forceExit: () => { + forceExitCount += 1; + }, + onPreparationError: () => {}, + onCleanupError: () => {}, + onWindowCreationError: () => {}, + onQuitTimeout: (stage) => { + timeouts.push(stage); + }, + resumeQuit: () => { + resumeQuitCount += 1; + }, + prepareTimeoutMs: 10, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await delay(60); + await flushQuitCoordinator(); + + assert.deepEqual(timeouts, ['preparing']); + assert.equal(cleanupCount, 1); + assert.equal(forceExitCount, 0); + assert.equal(resumeQuitCount, 1); + }); + + it('unwinds staged work when quit preparation is cancelled', async () => { + let abandonedCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: async () => 'cancelled', + cleanup: async () => {}, + focusOrCreateWindow: () => {}, + forceExit: () => {}, + onPreparationError: () => {}, + onCleanupError: () => {}, + onWindowCreationError: () => {}, + onQuitAbandoned: () => { + abandonedCount += 1; + }, + resumeQuit: () => {}, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await flushQuitCoordinator(); + + assert.equal(abandonedCount, 1); + }); + + it('unwinds staged work when quit preparation fails', async () => { + let abandonedCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: async () => { + throw new Error('retirement failed'); + }, + cleanup: async () => {}, + focusOrCreateWindow: () => {}, + forceExit: () => {}, + onPreparationError: () => {}, + onCleanupError: () => {}, + onWindowCreationError: () => {}, + onQuitAbandoned: () => { + abandonedCount += 1; + }, + resumeQuit: () => {}, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await flushQuitCoordinator(); + + assert.equal(abandonedCount, 1); + }); + + it('leaves staged work alone once the quit is committed', async () => { + // Unwinding here would resume a Host the process is about to drop, and + // an install handoff would be rolled back out from under the updater. + let abandonedCount = 0; + let resumeQuitCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: async () => 'ready', + cleanup: async () => {}, + focusOrCreateWindow: () => {}, + forceExit: () => {}, + onPreparationError: () => {}, + onCleanupError: () => {}, + onWindowCreationError: () => {}, + onQuitAbandoned: () => { + abandonedCount += 1; + }, + resumeQuit: () => { + resumeQuitCount += 1; + }, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await flushQuitCoordinator(); + + assert.equal(resumeQuitCount, 1); + assert.equal(abandonedCount, 0); + }); + + it('leaves staged work alone when a stuck quit is forced through', async () => { + let abandonedCount = 0; + let forceExitCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: async () => 'ready', + cleanup: () => new Promise(() => {}), + focusOrCreateWindow: () => {}, + forceExit: () => { + forceExitCount += 1; + }, + onPreparationError: () => {}, + onCleanupError: () => {}, + onWindowCreationError: () => {}, + onQuitAbandoned: () => { + abandonedCount += 1; + }, + resumeQuit: () => {}, + cleanupTimeoutMs: 10, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await delay(60); + + assert.equal(forceExitCount, 1); + assert.equal(abandonedCount, 0); + }); + + it('does not report an abandoned stage as a failure', async () => { + // A stage that stopped answering is not a Host that refused: routing it to + // the error sinks would invent a failure the product then has to explain. + const preparationErrors: unknown[] = []; + const cleanupErrors: unknown[] = []; + let forceExitCount = 0; + const coordinator = createAppQuitCoordinator({ + prepareToQuit: () => new Promise<'ready' | 'cancelled'>(() => {}), + cleanup: () => new Promise(() => {}), + focusOrCreateWindow: () => {}, + forceExit: () => { + forceExitCount += 1; + }, + onPreparationError: (error) => { + preparationErrors.push(error); + }, + onCleanupError: (error) => { + cleanupErrors.push(error); + }, + onWindowCreationError: () => {}, + resumeQuit: () => {}, + prepareTimeoutMs: 5, + cleanupTimeoutMs: 5, + }); + + coordinator.handleBeforeQuit({ preventDefault: () => {} }); + await delay(60); + + assert.deepEqual(preparationErrors, []); + assert.deepEqual(cleanupErrors, []); + assert.equal(forceExitCount, 1); + }); }); +async function delay(ms: number): Promise { + await new Promise((resolve) => setTimeout(resolve, ms)); +} + async function flushQuitCoordinator(): Promise { await new Promise((resolve) => setImmediate(resolve)); await new Promise((resolve) => setImmediate(resolve)); diff --git a/apps/desktop/src/main/__tests__/app-update-service.test.ts b/apps/desktop/src/main/__tests__/app-update-service.test.ts index 805f0b3c54..bc0ed9af94 100644 --- a/apps/desktop/src/main/__tests__/app-update-service.test.ts +++ b/apps/desktop/src/main/__tests__/app-update-service.test.ts @@ -587,6 +587,40 @@ describe('AppUpdateService', () => { assert.deepEqual(order, ['host-prepared', 'install']); }); + test('releases the Host handoff when the dispatched quit never happens', async () => { + let rollbacks = 0; + const updater = new FakeUpdater(); + const { service } = createHarness({ + updater, + prepareInstall: async () => ({ + kind: 'prepared', + rollback: () => { + rollbacks += 1; + }, + }), + }); + updater.emit('update-downloaded', { + ...updateInfo('1.1.0'), + downloadedFile: '/tmp/maka-update.zip', + }); + await settleUpdateVerification(); + + assert.deepEqual(await service.installUpdate({ allowInterruptActiveTasks: false }), { + ok: true, + }); + assert.equal(rollbacks, 0); + + // The installer was dispatched but the process stayed up. Nothing else + // observes a quit that did not happen, so the Host would stay retired + // with nothing left to restart it. + service.abandonPendingInstall(); + assert.equal(rollbacks, 1); + + // Releasing twice must not resume a Host a later install already owns. + service.abandonPendingInstall(); + assert.equal(rollbacks, 1); + }); + test('reports synchronous and asynchronous installer failures through status', async () => { let synchronousRollbacks = 0; const synchronous = createHarness({ diff --git a/apps/desktop/src/main/app-quit-coordinator.ts b/apps/desktop/src/main/app-quit-coordinator.ts index 565f450262..ff3624dcbe 100644 --- a/apps/desktop/src/main/app-quit-coordinator.ts +++ b/apps/desktop/src/main/app-quit-coordinator.ts @@ -26,6 +26,24 @@ export interface AppQuitCoordinator { handleBeforeQuit(event: AppQuitEvent): void; } +/** The two awaited stages of an orderly quit, in the order they run. */ +export type AppQuitStage = 'preparing' | 'cleaning'; + +/** + * Quitting waits on Host retirement, MCP child processes, peer mesh teardown + * and native resources. Any of those can stop settling, and an unbounded wait + * is indistinguishable from a hang: the process keeps running with its windows + * already committed to closing, and every later quit request is swallowed + * because the sequence still owns the phase. + * + * An updater install makes that failure permanent rather than merely annoying. + * Squirrel's ShipIt waits for this process to exit before it replaces the + * bundle, so a quit that never finishes silently blocks the update instead of + * reporting anything. + */ +const DEFAULT_PREPARE_TIMEOUT_MS = 15_000; +const DEFAULT_CLEANUP_TIMEOUT_MS = 10_000; + export interface AppQuitCoordinatorDeps { prepareToQuit(): Promise<'ready' | 'cancelled'>; cleanup(): Promise; @@ -34,13 +52,63 @@ export interface AppQuitCoordinatorDeps { onCleanupError(error: unknown): void; onWindowCreationError(error: unknown): void; resumeQuit(): void; + /** + * Terminate now, abandoning whatever never settled. Reached only after a + * stage exceeds its bound, so it is the difference between a stuck quit and + * a process that always goes away. + */ + forceExit(): void; + onQuitTimeout?(stage: AppQuitStage, timeoutMs: number): void; + /** + * The app is staying up after all. Anything staged for the quit that was + * expected to follow has to be unwound here, because nothing else observes + * a quit that simply did not happen. + */ + onQuitAbandoned?(): void; + prepareTimeoutMs?: number; + cleanupTimeoutMs?: number; } type AppQuitPhase = 'running' | 'preparing' | 'cleaning' | 'ready-to-exit'; +type StageOutcome = + | { kind: 'settled'; value: T } + | { kind: 'failed'; error: unknown } + | { kind: 'timeout' }; + +/** + * Run one quit stage under an upper bound. + * + * A late result is dropped rather than raced back in: once the bound elapses + * the sequence has already moved on, and its rejection is still observed here + * so abandoning the stage cannot surface as an unhandled rejection. + */ +function runStage(run: () => Promise, timeoutMs: number): Promise> { + return new Promise>((resolve) => { + let done = false; + const settle = (outcome: StageOutcome) => { + if (done) return; + done = true; + clearTimeout(timer); + resolve(outcome); + }; + const timer = setTimeout(() => settle({ kind: 'timeout' }), timeoutMs); + // A pending bound must never be the reason the process stays alive. + timer.unref?.(); + Promise.resolve() + .then(run) + .then( + (value) => settle({ kind: 'settled', value }), + (error) => settle({ kind: 'failed', error }), + ); + }); +} + export function createAppQuitCoordinator(deps: AppQuitCoordinatorDeps): AppQuitCoordinator { let phase: AppQuitPhase = 'running'; let windowCreationAbort = new AbortController(); + const prepareTimeoutMs = deps.prepareTimeoutMs ?? DEFAULT_PREPARE_TIMEOUT_MS; + const cleanupTimeoutMs = deps.cleanupTimeoutMs ?? DEFAULT_CLEANUP_TIMEOUT_MS; const focusOrCreateWindow = (): Promise => { if (phase !== 'running') return Promise.resolve(); @@ -54,6 +122,58 @@ export function createAppQuitCoordinator(deps: AppQuitCoordinatorDeps): AppQuitC } }; + const abandonQuit = (): void => { + phase = 'running'; + windowCreationAbort = new AbortController(); + deps.onQuitAbandoned?.(); + void focusOrCreateWindow(); + }; + + const finishCleanup = (): void => { + // `before-quit` was cancelled inside Electron's native quit transaction. + // Resuming from the cleanup Promise's microtask re-enters that transaction: + // Electron closes the windows but emits `window-all-closed` instead of + // `will-quit`, leaving the macOS process alive. Start a fresh transaction + // only after the current event-loop turn has unwound. + setImmediate(() => { + phase = 'ready-to-exit'; + deps.resumeQuit(); + }); + }; + + const runQuitSequence = async (): Promise => { + const preparation = await runStage(() => deps.prepareToQuit(), prepareTimeoutMs); + + if (preparation.kind === 'failed') { + deps.onPreparationError(preparation.error); + abandonQuit(); + return; + } + if (preparation.kind === 'settled' && preparation.value === 'cancelled') { + abandonQuit(); + return; + } + if (preparation.kind === 'timeout') { + // The user asked to quit and preparation is only a courtesy to the Host. + // Report it and keep going: refusing to quit is the worse outcome. + deps.onQuitTimeout?.('preparing', prepareTimeoutMs); + } + + phase = 'cleaning'; + const cleanup = await runStage(() => deps.cleanup(), cleanupTimeoutMs); + + if (cleanup.kind === 'failed') deps.onCleanupError(cleanup.error); + if (cleanup.kind === 'timeout') { + deps.onQuitTimeout?.('cleaning', cleanupTimeoutMs); + // Nothing left to wait for and no orderly exit available. Leaving the + // process up would strand an in-flight updater install indefinitely. + phase = 'ready-to-exit'; + deps.forceExit(); + return; + } + finishCleanup(); + }; + return { focusOrCreateWindow, handleBeforeQuit(event): void { @@ -62,42 +182,7 @@ export function createAppQuitCoordinator(deps: AppQuitCoordinatorDeps): AppQuitC if (phase !== 'running') return; phase = 'preparing'; windowCreationAbort.abort(); - const finishCleanup = () => { - // `before-quit` was cancelled inside Electron's native quit transaction. - // Resuming from the cleanup Promise's microtask re-enters that transaction: - // Electron closes the windows but emits `window-all-closed` instead of - // `will-quit`, leaving the macOS process alive. Start a fresh transaction - // only after the current event-loop turn has unwound. - setImmediate(() => { - phase = 'ready-to-exit'; - deps.resumeQuit(); - }); - }; - void Promise.resolve() - .then(() => deps.prepareToQuit()) - .then( - (preparation) => { - if (preparation === 'cancelled') { - phase = 'running'; - windowCreationAbort = new AbortController(); - focusOrCreateWindow(); - return; - } - phase = 'cleaning'; - return Promise.resolve() - .then(() => deps.cleanup()) - .then(finishCleanup, (error) => { - deps.onCleanupError(error); - finishCleanup(); - }); - }, - (error) => { - phase = 'running'; - windowCreationAbort = new AbortController(); - deps.onPreparationError(error); - focusOrCreateWindow(); - }, - ); + void runQuitSequence(); }, }; } diff --git a/apps/desktop/src/main/app-update-service.ts b/apps/desktop/src/main/app-update-service.ts index 1433308b1d..261734c912 100644 --- a/apps/desktop/src/main/app-update-service.ts +++ b/apps/desktop/src/main/app-update-service.ts @@ -43,6 +43,15 @@ export interface AppUpdateService { getStatus(): AppUpdateStatus; retryUpdateDownload(): Promise; installUpdate(input: AppUpdateInstallRequest): Promise; + /** + * Release an install handoff whose quit never happened. + * + * `installUpdate` retires the Host before handing off to the updater, and + * that retirement only unwinds through its rollback. A quit that is + * cancelled or abandoned would otherwise leave the Host retired with + * nothing left to restart it, so the quit path reports back here. + */ + abandonPendingInstall(): void; /** Check because the window regained focus, subject to a shared throttle. */ checkForUpdatesOnFocus(): Promise; /** @@ -524,6 +533,7 @@ export function createAppUpdateService(deps: AppUpdateServiceDeps): AppUpdateSer getStatus: currentStatus, retryUpdateDownload, installUpdate, + abandonPendingInstall: rollbackInstallHandoff, checkForUpdatesOnFocus, checkForUpdatesNow, }; diff --git a/apps/desktop/src/main/runtime-host-boot.ts b/apps/desktop/src/main/runtime-host-boot.ts index 958ffbc26a..7f0109334c 100644 --- a/apps/desktop/src/main/runtime-host-boot.ts +++ b/apps/desktop/src/main/runtime-host-boot.ts @@ -1447,7 +1447,17 @@ const quitCoordinator = createAppQuitCoordinator({ console.error("[runtime-host] shutdown failed:", error), onWindowCreationError: (error) => console.error("[window] creation failed:", error), + onQuitTimeout: (stage, timeoutMs) => { + // Squirrel's ShipIt waits for this process before it replaces the bundle, + // so a quit that never finishes blocks the update with nothing to read. + console.error( + `[runtime-host] quit ${stage} did not settle within ${timeoutMs}ms; ` + + (stage === "cleaning" ? "exiting without it" : "continuing shutdown"), + ); + }, + onQuitAbandoned: () => updateService.abandonPendingInstall(), resumeQuit: () => app.quit(), + forceExit: () => app.exit(0), }); app.on("before-quit", quitCoordinator.handleBeforeQuit); updateDesktopStartupProgress('connect');