From a157b81b9ae47090a63152caab60de0fa275075d Mon Sep 17 00:00:00 2001 From: ethernet Date: Thu, 10 Sep 2026 02:29:33 -0400 Subject: [PATCH] fix(desktop): recover failed App Installer handoffs A failed descriptor open stopped the backend but left no recovery path. Keep ownership of the ready relaunch waiter, cancel it on failure, and restore only after backend teardown begins. Preserve cleanup errors. Primary and pooled stops retain failed child handles for retry. Replacement stays blocked until the prior process exits, including while a persistent ownership claim is pending. Successful handoff keeps the update gate closed while the application quits. Verified: 178 focused tests passed with eight host skips. Both desktop typechecks and lint passed. Tests use real disposable children and real strategy/registration owners with external apply edges intercepted. No installed app, native signed update, or full desktop build was run. --- apps/desktop/electron/backend-child.ts | 38 +++++ .../electron/backend-connection-state.test.ts | 94 ++++++++++++ .../electron/backend-connection-state.ts | 61 +++++++- apps/desktop/electron/backend-exit.test.ts | 69 +++++++++ apps/desktop/electron/main.ts | 139 +++++++----------- apps/desktop/electron/pool-stop.test.ts | 39 ++++- apps/desktop/electron/pool-stop.ts | 47 +++--- .../updater/app-installer-recovery.test.ts | 114 ++++++++++++++ .../updater/app-installer-strategy.test.ts | 9 +- .../desktop/electron/updater/app-installer.ts | 77 ++++++---- .../updater/relaunch-registration.test.ts | 83 +++++++++++ .../updater/relaunch-waiter-lifecycle.test.ts | 115 +++++++++++++++ .../electron/updater/relaunch-waiter.test.ts | 33 +++-- .../electron/updater/relaunch-waiter.ts | 107 ++++++++++---- apps/desktop/electron/updater/relaunch.ts | 110 ++++++++------ apps/desktop/electron/updater/updater.test.ts | 25 +--- 16 files changed, 917 insertions(+), 243 deletions(-) create mode 100644 apps/desktop/electron/backend-exit.test.ts create mode 100644 apps/desktop/electron/updater/app-installer-recovery.test.ts create mode 100644 apps/desktop/electron/updater/relaunch-registration.test.ts create mode 100644 apps/desktop/electron/updater/relaunch-waiter-lifecycle.test.ts diff --git a/apps/desktop/electron/backend-child.ts b/apps/desktop/electron/backend-child.ts index df720be591..0c5619241d 100644 --- a/apps/desktop/electron/backend-child.ts +++ b/apps/desktop/electron/backend-child.ts @@ -20,6 +20,8 @@ * the function body. */ +import type { ChildProcess } from 'node:child_process' + export interface StopBackendChildDeps { /** Defaults to the real platform check; injectable for tests. */ isWindows?: boolean @@ -101,3 +103,39 @@ export function stopBackendTreesForUpdate( deps.stopAllPoolBackends() } + +export async function waitForBackendExit( + child: ChildProcess | null | undefined, + escalate: (child: ChildProcess) => void, + timeoutMs = 5000 +): Promise { + if (!child || child.exitCode !== null || child.signalCode !== null) { return } + + const exited = (): boolean => child.exitCode !== null || child.signalCode !== null + + const wait = (delay: number): Promise => new Promise(resolve => { + if (exited()) { + resolve() + + return + } + + const finish = (): void => { + clearTimeout(timer) + child.removeListener('exit', finish) + resolve() + } + + const timer = setTimeout(finish, delay) + child.once('exit', finish) + }) + + await wait(timeoutMs) + + if (exited()) { return } + + escalate(child) + await wait(1000) + + if (!exited()) { throw new Error(`Backend PID ${child.pid} did not exit after escalation`) } +} diff --git a/apps/desktop/electron/backend-connection-state.test.ts b/apps/desktop/electron/backend-connection-state.test.ts index ce9dd2fded..b6e4ef7d03 100644 --- a/apps/desktop/electron/backend-connection-state.test.ts +++ b/apps/desktop/electron/backend-connection-state.test.ts @@ -1,7 +1,10 @@ import assert from 'node:assert/strict' +import { type ChildProcess, spawn } from 'node:child_process' +import { once } from 'node:events' import { test } from 'vitest' +import { waitForBackendExit } from './backend-child' import { createBackendConnectionState } from './backend-connection-state' type FakeProcess = { id: string } @@ -105,3 +108,94 @@ test('an invalidated attempt cannot attach a late-spawned process', () => { assert.equal(state.attachProcess(staleAttempt, { id: 'late' }), null) assert.equal(state.getProcess(), null) }) + +test('a failed primary stop retains its child and blocks a replacement until retry exits', async () => { + const state = createBackendConnectionState() + + const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], { + stdio: ['ignore', 'pipe', 'ignore'], windowsHide: true + }) + + try { + await once(child.stdout!, 'data') + const attempt = state.startAttempt() + state.setPromise(attempt, Promise.resolve('ready')) + const owner = state.attachProcess(attempt, child) + assert.ok(owner) + const failure = new Error('primary is still running') + const calls: ChildProcess[] = [] + + const fail = async (current: ChildProcess): Promise => { calls.push(current); throw failure } + + const stopping = state.stopProcess(fail) + assert.equal(state.getProcess(), null) + assert.equal(state.getPromise(), null) + assert.throws(() => state.startAttempt(), /has not stopped/) + assert.equal(state.stopProcess(fail), stopping) + await assert.rejects(stopping, error => error === failure) + state.invalidate() + assert.throws(() => state.startAttempt(), /has not stopped/) + assert.equal(child.exitCode, null) + assert.equal(child.signalCode, null) + + await state.stopProcess(async current => { + calls.push(current) + current.kill() + await waitForBackendExit(current, process => { process.kill('SIGKILL') }) + }) + assert.deepEqual(calls, [child, child]) + assert.ok(child.exitCode !== null || child.signalCode !== null) + const replacement = state.startAttempt() + const connection = Promise.resolve('new') + assert.equal(state.setPromise(replacement, connection), true) + assert.equal(state.clearForCurrentProcess(owner), false) + assert.equal(state.getPromise(), connection) + } finally { + if (child.exitCode === null && child.signalCode === null) { + const closed = once(child, 'close') + child.kill() + await closed + } + } +}, 15_000) + +test('shutdown sees a spawned child while its persistent claim is still pending', async () => { + const state = createBackendConnectionState() + + const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], { + stdio: ['ignore', 'pipe', 'ignore'], windowsHide: true + }) + + const claim = deferred() + + try { + await once(child.stdout!, 'data') + const attempt = state.startAttempt() + + const claiming = state.claimProcess(attempt, child, async current => { + assert.equal(current, child) + assert.equal(state.getProcess(), child) + await claim.promise + }) + + assert.equal(state.getProcess(), child) + await state.stopProcess(async current => { + assert.equal(current, child) + current.kill() + await waitForBackendExit(current, process => { process.kill('SIGKILL') }) + }) + assert.ok(child.exitCode !== null || child.signalCode !== null) + claim.resolve() + assert.equal(await claiming, null, 'a completed claim cannot revive the stopped generation') + assert.equal(state.getProcess(), null) + assert.doesNotThrow(() => state.startAttempt()) + } finally { + claim.resolve() + + if (child.exitCode === null && child.signalCode === null) { + const closed = once(child, 'close') + child.kill() + await closed + } + } +}, 15_000) diff --git a/apps/desktop/electron/backend-connection-state.ts b/apps/desktop/electron/backend-connection-state.ts index d20165db85..2cf0da2bf2 100644 --- a/apps/desktop/electron/backend-connection-state.ts +++ b/apps/desktop/electron/backend-connection-state.ts @@ -8,13 +8,33 @@ export type BackendProcessOwner = { process: TProcess } +interface PendingBackendStop { + process: TProcess + completion: Promise + failed: boolean +} + export function createBackendConnectionState() { let generation = 0 let process: TProcess | null = null let promise: Promise | null = null + let stopping: PendingBackendStop | null = null + + function invalidate(): TProcess | null { + const currentProcess = process + generation += 1 + process = null + promise = null + + return currentProcess + } return { startAttempt(): BackendConnectionAttempt { + if (stopping) { + throw new Error('The previous backend has not stopped. Retry its shutdown before starting a replacement.') + } + return { generation, promise: null } }, @@ -46,6 +66,22 @@ export function createBackendConnectionState() { return { generation, process: nextProcess } }, + async claimProcess( + attempt: BackendConnectionAttempt, + nextProcess: TProcess, + claim: (current: TProcess) => Promise + ): Promise | null> { + const owner = this.attachProcess(attempt, nextProcess) + + if (!owner) { + return null + } + + await claim(nextProcess) + + return owner.generation === generation && process === nextProcess ? owner : null + }, + clearForCurrentProcess(owner: BackendProcessOwner): boolean { if (owner.generation !== generation || owner.process !== process) { return false @@ -75,14 +111,27 @@ export function createBackendConnectionState() { return promise }, - invalidate(): TProcess | null { - const currentProcess = process + invalidate, - generation += 1 - process = null - promise = null + stopProcess(stop: (current: TProcess) => Promise): Promise { + if (stopping && !stopping.failed) { + return stopping.completion + } - return currentProcess + const current = stopping?.process ?? invalidate() + + if (current === null) { + return Promise.resolve() + } + + const completion = Promise.resolve().then(() => stop(current)).then( + () => { stopping = null }, + error => { stopping!.failed = true; throw error } + ) + + stopping = { process: current, completion, failed: false } + + return completion } } } diff --git a/apps/desktop/electron/backend-exit.test.ts b/apps/desktop/electron/backend-exit.test.ts new file mode 100644 index 0000000000..db913be41d --- /dev/null +++ b/apps/desktop/electron/backend-exit.test.ts @@ -0,0 +1,69 @@ +import assert from 'node:assert/strict' +import { type ChildProcess, spawn } from 'node:child_process' +import { once } from 'node:events' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { test } from 'vitest' + +import { waitForBackendExit } from './backend-child' + +async function makeChild(): Promise { + const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], { + stdio: ['ignore', 'pipe', 'ignore'], windowsHide: true + }) + + await once(child.stdout!, 'data') + + return child +} + +test('backend exit waits through escalation and removes its listener', async () => { + const child = await makeChild() + let escalated = 0 + const before = child.listenerCount('exit') + + try { + await waitForBackendExit(child, process => { escalated++; process.kill() }, 0) + assert.equal(escalated, 1) + assert.ok(child.exitCode !== null || child.signalCode !== null) + assert.equal(child.listenerCount('exit'), before) + await waitForBackendExit(child, () => { throw new Error('an exited process must not be killed again') }) + } finally { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'close') + child.kill() + await exited + } + } +}, 15_000) + +test('backend exit refuses a live child rather than reporting a completed shutdown', async () => { + const child = await makeChild() + const before = child.listenerCount('exit') + + try { + await assert.rejects(waitForBackendExit(child, () => {}, 0), /did not exit/) + assert.equal(child.exitCode, null) + assert.equal(child.signalCode, null) + assert.equal(child.listenerCount('exit'), before) + } finally { + const exited = once(child, 'close') + child.kill() + await exited + } +}, 15_000) + +test('a failed spawn has no process to escalate', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'backend-no-process-')) + const child = spawn(path.join(root, 'absent-executable'), [], { stdio: 'ignore' }) + + try { + await once(child, 'error') + assert.equal(child.pid, undefined) + await waitForBackendExit(child, () => { throw new Error('a failed spawn must not be signalled') }, 0) + } finally { + fs.rmSync(root, { recursive: true, force: true }) + } +}, 15_000) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 7b2f8349c0..a9b36d6850 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -1,4 +1,4 @@ -import { execFileSync, spawn } from 'node:child_process' +import { type ChildProcess, execFileSync, spawn } from 'node:child_process' import crypto from 'node:crypto' import fs from 'node:fs' import http from 'node:http' @@ -35,7 +35,7 @@ import { destroyKeepaliveAgents, downloadAgentFor, jsonAgentFor, withRetry } fro import { appIconCandidates, resolveAppIcon } from './app-icon' import { stageAppInstallerFile } from './app-installer-file' import { runAppInstallerChecker } from './appinstaller-checker' -import { stopBackendChild as stopBackendChildImpl, stopBackendTreesForUpdate } from './backend-child' +import { stopBackendChild as stopBackendChildImpl, stopBackendTreesForUpdate, waitForBackendExit as waitForBackendExitImpl } from './backend-child' import { type BackendOutputTail, claimDecision, @@ -3157,15 +3157,8 @@ function resolvePackagedUpdateStrategy(): UpdaterStrategy | null { appVersion: app.getVersion(), log: rememberLog, emitProgress: emitUpdateProgress, - beforeInstall: async () => { - isQuittingForHandoff = true - await Promise.all([teardownPrimaryBackendAndWait(), stopAllPoolBackends()]) - }, - onInstallFailure: async () => { - isQuittingForHandoff = false - updateInFlight = false - await startHermes() - } + beforeInstall: teardownBundledBackend, + onInstallFailure: restoreBundledBackend }) return packagedUpdateStrategy @@ -3191,6 +3184,7 @@ function resolvePackagedUpdateStrategy(): UpdaterStrategy | null { open: file => shell.openPath(file) }, teardownBundledBackend, + restoreBundledBackend, emitUpdateProgress, appVersion: app.getVersion(), quit: () => app.quit(), @@ -3312,23 +3306,32 @@ function isLightVariant(): boolean { return INSTALL_STAMP?.payload === 'light' } -/** - * Graceful teardown before the OS App Installer swaps the package: stop the - * primary backend + all pool backends (tree-kill their children) so no - * process keeps files in the install dir locked while Windows replaces it. - * Same primitive the update hand-off uses (stopBackendTreesForUpdate). - */ +/** Invalidate connections and wait for every owned backend before the swap. */ async function teardownBundledBackend(): Promise { - const hermesProcess = backendConnectionState.getProcess() + isQuittingForHandoff = true + const results = await Promise.allSettled([teardownPrimaryBackendAndWait(), stopAllPoolBackends()]) + const errors = results.filter(result => result.status === 'rejected').map(result => result.reason) - if (hermesProcess || backendPool.size > 0) { - stopBackendTreesForUpdate(hermesProcess, { - forceKillProcessTree, - stopAllPoolBackends - }) + if (errors.length) { + // An incomplete shutdown blocks replacement until a retry proves exit. + backendStartFailure = new AggregateError(errors, 'Backend shutdown failed') + throw backendStartFailure } } +async function restoreBundledBackend(): Promise { + try { + // A failed stop retains its process handle. Retry before enabling a new start. + await teardownBundledBackend() + } finally { + isQuittingForHandoff = false + updateInFlight = false + } + + backendStartFailure = null + await startHermes() +} + // Set to true when the desktop is about to quit so a detached swap/install/ // uninstall script can take over. On macOS, app.quit() closes windows but // window-all-closed deliberately keeps the process alive (standard Electron @@ -10891,10 +10894,11 @@ function resetHermesConnection({ soft = false } = {}) { // dashboard process to actually exit (SIGKILL after 5s) so the next // startHermes() spawns fresh instead of racing the dying one. Shared by the // connection-config and profile switch flows. -async function teardownPrimaryBackendAndWait({ soft = false } = {}) { - // Capture the reference before resetHermesConnection() invalidates it. - const hermesProcess = backendConnectionState.getProcess() - const dying = hermesProcess && !hermesProcess.killed ? hermesProcess : null +async function teardownPrimaryBackendAndWait({ soft = false }: { soft?: boolean } = {}): Promise { + const stopping = backendConnectionState.stopProcess(async child => { + stopBackendChild(child) + await waitForBackendExit(child) + }) if (soft) { softRehomeInProgress = true @@ -10902,7 +10906,7 @@ async function teardownPrimaryBackendAndWait({ soft = false } = {}) { try { resetHermesConnection({ soft }) - await waitForBackendExit(dying) + await stopping } finally { if (soft) { softRehomeInProgress = false @@ -10939,53 +10943,20 @@ function broadcastConnectionsChanged(payload: { connectionId: string; reason: 'r } } -async function waitForBackendExit(child, timeoutMs = 5000) { - if (!child || child.exitCode !== null || child.signalCode !== null) { - return - } - - const exited = () => child.exitCode !== null || child.signalCode !== null - - const wait = delay => - new Promise(resolve => { - if (exited()) { - resolve() - - return - } - - const timer = setTimeout(resolve, delay) - child.once('exit', () => { - clearTimeout(timer) - resolve() - }) - }) - - await wait(timeoutMs) - - if (exited()) { - return - } - - try { - if (IS_WINDOWS && Number.isInteger(child.pid)) { - forceKillProcessTree(child.pid) - } else if (Number.isInteger(child.pid)) { +async function waitForBackendExit(child: ChildProcess | null | undefined, timeoutMs = 5000): Promise { + await waitForBackendExitImpl(child, processToStop => { + if (IS_WINDOWS && Number.isInteger(processToStop.pid)) { + forceKillProcessTree(processToStop.pid) + } else if (Number.isInteger(processToStop.pid)) { try { - process.kill(-child.pid, 'SIGKILL') + process.kill(-processToStop.pid!, 'SIGKILL') } catch { - child.kill('SIGKILL') + processToStop.kill('SIGKILL') } } else { - child.kill('SIGKILL') + processToStop.kill('SIGKILL') } - } catch { - return - } - - // Await the escalation as well; do not let shutdown or failed adoption race - // a still-running backend. - await wait(1000) + }, timeoutMs) } // The profile the primary (window) backend runs as. readActiveDesktopProfile() @@ -12325,7 +12296,7 @@ async function spawnPoolBackend(profile, entry, opts: { forceLocal?: boolean; po // SIGTERM -> SIGKILL escalation in waitForBackendExit() resolves. Previously // SIGTERM + immediate entry delete dropped the handle and a slow child // survived detached under PID 1. -const poolStopper = createPoolStopper({ +const poolStopper = createPoolStopper({ pool: backendPool, stopChild: child => stopBackendChild(child), waitForExit: child => waitForBackendExit(child) @@ -12383,9 +12354,8 @@ function reapInstallRootedStragglers(excludePids: number[]): void { } const backendShutdown = createBackendShutdownCoordinator(async () => { - const primary = backendConnectionState.invalidate() - - stopBackendChild(primary) + const primary = backendConnectionState.getProcess() + const primaryStop = teardownPrimaryBackendAndWait() const pooledStops = stopAllPoolBackends() if (poolIdleReaper) { @@ -12393,7 +12363,7 @@ const backendShutdown = createBackendShutdownCoordinator(async () => { poolIdleReaper = null } - await Promise.all([waitForBackendExit(primary), pooledStops]) + await Promise.all([primaryStop, pooledStops]) reapInstallRootedStragglers(Number.isInteger(primary?.pid) ? [primary.pid] : []) }) @@ -12636,6 +12606,10 @@ async function startHermes() { const backendNonce = crypto.randomBytes(16).toString('hex') const parentIdentityEnv = parentWatchdogEnv(process.pid, parentStartMarker, backendNonce) + if (!backendConnectionState.isCurrentAttempt(connectionAttempt)) { + throw new Error('Hermes backend start was superseded by a newer connection attempt.') + } + const hermesProcess = spawn( backend.command, backend.args, @@ -12694,14 +12668,14 @@ async function startHermes() { // Mark handled so an early rejection (child dies during the claim) can't // surface as an unhandled rejection before the Promise.race below attaches. portAnnouncement.catch(() => {}) - await claimBackendChild( - hermesProcess, + + const processOwner = await backendConnectionState.claimProcess(connectionAttempt, hermesProcess, child => claimBackendChild( + child, `${backend.command} ${backend.args.join(' ')}`, profile, backendNonce, primaryOutputTail - ) - const processOwner = backendConnectionState.attachProcess(connectionAttempt, hermesProcess) + )) if (!processOwner) { stopBackendChild(hermesProcess) @@ -12841,9 +12815,10 @@ async function startHermes() { throw error } - const failedProcess = backendConnectionState.invalidate() - stopBackendChild(failedProcess) - await waitForBackendExit(failedProcess) + await backendConnectionState.stopProcess(async failedProcess => { + stopBackendChild(failedProcess) + await waitForBackendExit(failedProcess) + }) if (error instanceof FirstRunSetupResetError) { throw error diff --git a/apps/desktop/electron/pool-stop.test.ts b/apps/desktop/electron/pool-stop.test.ts index 55d6c40c87..395fec5664 100644 --- a/apps/desktop/electron/pool-stop.test.ts +++ b/apps/desktop/electron/pool-stop.test.ts @@ -89,10 +89,11 @@ test('stop of an unknown key resolves without signalling anything', async () => assert.equal(stopper.inFlight('ghost'), undefined) }) -test('stopAll stops every pooled backend and resolves after all exits', async () => { +test('stopAll waits for current and already-stopping backends', async () => { const { addChild, exitResolvers, pool, stopper } = harness() const a = addChild('a') const b = addChild('b') + const priorStop = stopper.stop('a') let settled = false @@ -104,12 +105,13 @@ test('stopAll stops every pooled backend and resolves after all exits', async () assert.equal(a.killed, true) assert.equal(b.killed, true) - exitResolvers.get(a)?.() + exitResolvers.get(b)?.() await Promise.resolve() + await new Promise(setImmediate) assert.equal(settled, false, 'must wait for EVERY child, not the first') - exitResolvers.get(b)?.() - await all + exitResolvers.get(a)?.() + await Promise.all([all, priorStop]) assert.equal(settled, true) }) @@ -136,3 +138,32 @@ test('a respawn can await the in-flight stop before reusing the key', async () = assert.deepEqual(order, ['exit-signal', 'spawn']) }) + +test('failed stops block respawn and retain the child for a later stop retry', async () => { + const child: Child = { exited: false, killed: false } + const pool = new Map([['profile', { process: child }]]) + const failure = new Error('child is still alive') + const attempts: Child[] = [] + let refuses = true + + const stopper = createPoolStopper({ + pool, + stopChild: current => { attempts.push(current!) }, + waitForExit: async current => { + if (refuses) { throw failure } + current!.exited = true + } + }) + + const failed = stopper.stop('profile') + await assert.rejects(failed, error => error === failure) + assert.equal(pool.has('profile'), false) + assert.equal(stopper.inFlight('profile'), failed) + await assert.rejects(stopper.inFlight('profile')!, error => error === failure) + + refuses = false + await stopper.stopAll() + assert.deepEqual(attempts, [child, child]) + assert.equal(child.exited, true) + assert.equal(stopper.inFlight('profile'), undefined) +}) diff --git a/apps/desktop/electron/pool-stop.ts b/apps/desktop/electron/pool-stop.ts index 767a65e174..15ee8e390d 100644 --- a/apps/desktop/electron/pool-stop.ts +++ b/apps/desktop/electron/pool-stop.ts @@ -22,17 +22,17 @@ * directly instead of grepping main.ts source text. */ -export interface PoolStopEntry { - process?: unknown +export interface PoolStopEntry { + process?: Process } -export interface PoolStopperDeps { +export interface PoolStopperDeps { /** The live backend pool. Entries are evicted synchronously on stop. */ - pool: Map + pool: Map> /** Signal the child (tree/group kill per platform). Synchronous. */ - stopChild: (child: unknown) => void + stopChild: (child: Process | undefined) => void /** Bounded wait: resolves when the child exits, escalating to SIGKILL. */ - waitForExit: (child: unknown) => Promise + waitForExit: (child: Process | undefined) => Promise } export interface PoolStopper { @@ -44,17 +44,23 @@ export interface PoolStopper { stopAll: () => Promise } -export function createPoolStopper(deps: PoolStopperDeps): PoolStopper { - const stops = new Map>() +interface PendingStop { + entry: PoolStopEntry + completion: Promise + failed: boolean +} + +export function createPoolStopper(deps: PoolStopperDeps): PoolStopper { + const stops = new Map>() function stop(key: string): Promise { const inFlight = stops.get(key) - if (inFlight) { - return inFlight + if (inFlight && !inFlight.failed) { + return inFlight.completion } - const entry = deps.pool.get(key) + const entry = inFlight?.entry ?? deps.pool.get(key) if (!entry) { return Promise.resolve() @@ -67,20 +73,27 @@ export function createPoolStopper(deps: PoolStopperDeps): PoolStopper { const stopping = (async () => { deps.stopChild(entry.process) await deps.waitForExit(entry.process) - })().finally(() => { - stops.delete(key) - }) + })().then( + () => { stops.delete(key) }, + error => { pending.failed = true; throw error } + ) - stops.set(key, stopping) + const pending: PendingStop = { entry, completion: stopping, failed: false } + + stops.set(key, pending) return stopping } return { - inFlight: key => stops.get(key), + inFlight: key => stops.get(key)?.completion, stop, stopAll: async () => { - await Promise.all([...deps.pool.keys()].map(stop)) + const pending = new Set([...deps.pool.keys(), ...stops.keys()]) + const results = await Promise.allSettled([...pending].map(stop)) + const errors = results.filter(result => result.status === 'rejected').map(result => result.reason) + + if (errors.length) { throw new AggregateError(errors, 'Backend pool shutdown failed') } } } } diff --git a/apps/desktop/electron/updater/app-installer-recovery.test.ts b/apps/desktop/electron/updater/app-installer-recovery.test.ts new file mode 100644 index 0000000000..b091aadaa2 --- /dev/null +++ b/apps/desktop/electron/updater/app-installer-recovery.test.ts @@ -0,0 +1,114 @@ +import assert from 'node:assert/strict' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { test } from 'vitest' + +import { AppInstallerStrategy, type AppInstallerStrategyDeps } from './app-installer' +import { PENDING_RELAUNCH_FILENAME, registerUpdateRelaunch } from './relaunch' + +for (const failureAt of ['prepare', 'register', 'teardown', 'open', 'none']) { + test(`App Installer restores only after teardown begins (${failureAt})`, async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'appinstaller-recovery-')) + const marker = path.join(home, PENDING_RELAUNCH_FILENAME) + const failure = new Error(`failed ${failureAt}`) + const calls: string[] = [] + const progress: string[] = [] + let running = true + + const act = (at: string): void => { + calls.push(at) + + if (at === failureAt) {throw failure} + } + + const deps: AppInstallerStrategyDeps = { + python: 'unused-checker', script: 'unused.py', + run: async () => { throw new Error('configured feed does not need the checker') }, + channel: 'stable', light: false, appVersion: '1.0', feedBaseUrl: 'https://example.invalid', + installer: { + prepare: async () => { + act('prepare') + + return 'fixture.appinstaller' + }, + open: async () => { + assert.equal(running, false) + act('open') + + return '' + } + }, + registerPendingRelaunch: version => registerUpdateRelaunch(home, version, { + relaunch: async () => { + act('register') + + return { cancel: async () => { calls.push('cancel') } } + } + }), + teardownBundledBackend: async () => { running = false; act('teardown') }, + restoreBundledBackend: async () => { running = true; calls.push('restore') }, + emitUpdateProgress: value => { progress.push(value.stage) }, + quit: () => { calls.push('quit') } + } + + try { + if (failureAt === 'none') { + const result = await new AppInstallerStrategy(deps).apply({}) + assert.equal(result.ok, true) + assert.equal(result.handedOff, true, 'keep backend restart blocked until the quitting app exits') + assert.deepEqual(calls, ['prepare', 'register', 'teardown', 'open', 'quit']) + assert.equal(fs.existsSync(marker), true) + } else { + await assert.rejects(new AppInstallerStrategy(deps).apply({}), error => error === failure) + assert.equal(running, true) + assert.equal(calls.includes('quit'), false) + assert.equal(calls.includes('restore'), ['teardown', 'open'].includes(failureAt)) + assert.equal(calls.includes('cancel'), ['teardown', 'open'].includes(failureAt)) + assert.equal(fs.existsSync(marker), false) + assert.equal(progress.at(-1), 'error') + } + } finally { + fs.rmSync(home, { recursive: true, force: true }) + } + }) +} + +test('handoff errors retain cleanup failures while still restoring the backend', async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'appinstaller-errors-')) + const original = new Error('descriptor open failed') + const cancellation = new Error('waiter stop failed') + const recovery = new Error('backend restart failed') + const order: string[] = [] + + const deps: AppInstallerStrategyDeps = { + python: 'unused', script: 'unused.py', run: async () => ({ code: 0, stdout: '' }), + channel: 'stable', light: false, appVersion: '1.0', feedBaseUrl: 'https://example.invalid', + installer: { prepare: async () => 'file', open: async () => { throw original } }, + registerPendingRelaunch: version => registerUpdateRelaunch(home, version, { + relaunch: async () => ({ cancel: async () => { order.push('cancel'); throw cancellation } }) + }), + teardownBundledBackend: async () => { order.push('stop') }, + restoreBundledBackend: async () => { order.push('restore'); throw recovery }, + emitUpdateProgress: event => { if (event.stage === 'error') {order.push('error')} }, + quit: () => { throw new Error('failed handoff must not quit') } + } + + try { + await assert.rejects(new AppInstallerStrategy(deps).apply({}), error => { + assert.ok(error instanceof AggregateError) + assert.equal(error.cause, original) + assert.equal(error.errors[0], original) + assert.ok(error.errors[1] instanceof AggregateError) + assert.equal(error.errors[1].errors[0], cancellation) + assert.equal(error.errors[2], recovery) + + return true + }) + assert.deepEqual(order, ['stop', 'cancel', 'restore', 'error']) + assert.equal(fs.existsSync(path.join(home, PENDING_RELAUNCH_FILENAME)), false) + } finally { + fs.rmSync(home, { recursive: true, force: true }) + } +}) diff --git a/apps/desktop/electron/updater/app-installer-strategy.test.ts b/apps/desktop/electron/updater/app-installer-strategy.test.ts index 7d33c93564..c169e28a18 100644 --- a/apps/desktop/electron/updater/app-installer-strategy.test.ts +++ b/apps/desktop/electron/updater/app-installer-strategy.test.ts @@ -1,6 +1,6 @@ // updater/app-installer-strategy.test.ts — the apply-flow contract: the // relaunch registration (marker + detached waiter) completes BEFORE the OS -// hand-off, teardown runs before the trigger, and quit is unconditional. +// hand-off. Teardown finishes before the descriptor opens. import { describe, expect, it } from 'vitest' @@ -26,12 +26,13 @@ function makeDeps(over: Partial = {}) { return '' } }, teardownBundledBackend: async () => { calls.push('teardown') }, + restoreBundledBackend: async () => { calls.push('restore') }, emitUpdateProgress: () => {}, appVersion: '0.18.2', quit: () => { calls.push('quit') }, registerPendingRelaunch: async () => { calls.push('relaunch-marker'); - return true }, + return { automatic: true, cancel: async () => {} } }, ...over } @@ -44,7 +45,7 @@ describe('AppInstallerStrategy.apply', () => { const strategy = new AppInstallerStrategy(deps) const result = await strategy.apply({}) - expect(result).toEqual({ ok: true, manual: false, bundled: true, mechanism: 'app-installer' }) + expect(result).toEqual({ ok: true, manual: false, bundled: true, handedOff: true, mechanism: 'app-installer' }) expect(calls).toEqual(['prepare', 'relaunch-marker', 'teardown', 'open', 'quit']) }) @@ -52,7 +53,7 @@ describe('AppInstallerStrategy.apply', () => { const progress: string[] = [] const { deps, calls } = makeDeps({ - registerPendingRelaunch: async () => false, + registerPendingRelaunch: async () => ({ automatic: false, cancel: async () => {} }), emitUpdateProgress: event => { progress.push(event.message) } }) diff --git a/apps/desktop/electron/updater/app-installer.ts b/apps/desktop/electron/updater/app-installer.ts index 4fe5c5acbc..2a5a6de545 100644 --- a/apps/desktop/electron/updater/app-installer.ts +++ b/apps/desktop/electron/updater/app-installer.ts @@ -19,6 +19,8 @@ import { win32AppInstallerFeedPath } from '../app-updater' +import type { RelaunchRegistration } from './relaunch' + import type { UpdaterApplyResultWire, UpdaterStatusWire } from './index' export interface AppInstallerStrategyDeps { @@ -35,20 +37,15 @@ export interface AppInstallerStrategyDeps { installer: { prepare: (url: string) => Promise; open: (file: string) => Promise } /** Graceful backend teardown before the package swap. */ teardownBundledBackend: () => void | Promise + restoreBundledBackend: () => Promise /** Progress emitter for the updates overlay. */ emitUpdateProgress: (payload: { stage: string; message: string; percent: number | null }) => void /** App version label for the status wire. */ appVersion: string /** Quit the app (after handing the swap to the OS). */ quit: () => void - /** - * Register a one-shot post-update relaunch before quitting, so Hermes - * reopens on the new version with no user action. Resolves true only once - * the relaunch mechanism has acknowledged (the waiter handshake) — the - * caller must await this BEFORE quitting. Resolves false when the - * mechanism could not be started (relaunch stays manual). - */ - registerPendingRelaunch: (targetVersion: string) => Promise + /** Retain marker and waiter ownership until the OS accepts the handoff. */ + registerPendingRelaunch: (fromVersion: string) => Promise } export interface CheckOutcome { @@ -109,29 +106,57 @@ export class AppInstallerStrategy { percent: 100 }) - await triggerAppInstallerUpdate( - feedBaseUrl, - this.deps.channel, - this.deps.light, - this.deps.installer, - async () => { - const registered = await this.deps.registerPendingRelaunch(this.deps.appVersion) + let registration: RelaunchRegistration | undefined + let teardownStarted = false - if (!registered) { - this.deps.emitUpdateProgress({ - stage: 'restart', percent: 100, - message: 'Automatic relaunch could not be registered. Reopen Hermes after App Installer finishes.' - }) + try { + await triggerAppInstallerUpdate( + feedBaseUrl, + this.deps.channel, + this.deps.light, + this.deps.installer, + async () => { + registration = await this.deps.registerPendingRelaunch(this.deps.appVersion) + + if (!registration.automatic) { + this.deps.emitUpdateProgress({ + stage: 'restart', percent: 100, + message: 'Automatic relaunch could not be registered. Reopen Hermes after App Installer finishes.' + }) + } + + teardownStarted = true + await this.deps.teardownBundledBackend() + }, + sourceUri + ) + + this.deps.quit() + } catch (error) { + const errors: unknown[] = [error] + + try { + await registration?.cancel() + } catch (cancelError) { + errors.push(cancelError) + } + + if (teardownStarted) { + try { + await this.deps.restoreBundledBackend() + } catch (restoreError) { + errors.push(restoreError) } + } - await this.deps.teardownBundledBackend() - }, - sourceUri - ) + const message = errors.map(item => item instanceof Error ? item.message : String(item)).join('; ') + this.deps.emitUpdateProgress({ stage: 'error', message, percent: null }) - this.deps.quit() + if (errors.length > 1) { throw new AggregateError(errors, message, { cause: error }) } + throw error + } - return { ok: true, manual: false, bundled: true, mechanism: this.mechanism } + return { ok: true, manual: false, bundled: true, handedOff: true, mechanism: this.mechanism } } } diff --git a/apps/desktop/electron/updater/relaunch-registration.test.ts b/apps/desktop/electron/updater/relaunch-registration.test.ts new file mode 100644 index 0000000000..46d4f7b1b8 --- /dev/null +++ b/apps/desktop/electron/updater/relaunch-registration.test.ts @@ -0,0 +1,83 @@ +import assert from 'node:assert/strict' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { test } from 'vitest' + +import { PENDING_RELAUNCH_FILENAME, registerUpdateRelaunch } from './relaunch' +import type { RelaunchWaiterHandle } from './relaunch-waiter' + +for (const automatic of [true, false]) { + test(`registration owns its marker and cancellation (automatic=${automatic})`, async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'relaunch-registration-')) + const marker = path.join(home, PENDING_RELAUNCH_FILENAME) + let resolveReady!: (handle: RelaunchWaiterHandle | undefined) => void + const ready = new Promise(resolve => { resolveReady = resolve }) + let cancelled = 0 + let registered = false + + try { + const pending = registerUpdateRelaunch(home, '1.0', { relaunch: () => ready }) + .then(result => { registered = true; + + return result }) + + await new Promise(setImmediate) + assert.equal(registered, false) + assert.equal(JSON.parse(fs.readFileSync(marker, 'utf8')).fromVersion, '1.0') + resolveReady(automatic ? { cancel: async () => { cancelled++ } } : undefined) + const registration = await pending + assert.equal(registration.automatic, automatic) + assert.equal(fs.existsSync(marker), true) + await Promise.all([registration.cancel(), registration.cancel()]) + await registration.cancel() + assert.equal(cancelled, automatic ? 1 : 0) + assert.equal(fs.existsSync(marker), false) + } finally { + fs.rmSync(home, { recursive: true, force: true }) + } + }) +} + +test('start and cancellation failures preserve the cause and still clean owned markers', async () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'relaunch-failures-')) + const marker = path.join(home, PENDING_RELAUNCH_FILENAME) + const failure = new Error('owned child refused to stop') + + try { + await assert.rejects(registerUpdateRelaunch(home, '1.0', { + relaunch: async () => { throw failure } + }), error => error === failure) + assert.equal(fs.existsSync(marker), false) + + let attempts = 0 + + const registration = await registerUpdateRelaunch(home, '1.0', { + relaunch: async () => ({ cancel: async () => { attempts++; throw failure } }) + }) + + await assert.rejects(registration.cancel(), error => error instanceof AggregateError && error.errors.includes(failure)) + await assert.rejects(registration.cancel(), error => error instanceof AggregateError && error.errors.includes(failure)) + assert.equal(attempts, 1) + assert.equal(fs.existsSync(marker), false) + + const blocked = await registerUpdateRelaunch(home, '1.0', { + relaunch: async () => ({ cancel: async () => { throw failure } }) + }) + + fs.unlinkSync(marker) + fs.mkdirSync(marker) + fs.writeFileSync(path.join(marker, 'foreign-data'), 'keep') + await assert.rejects(blocked.cancel(), error => { + assert.ok(error instanceof AggregateError) + assert.equal(error.errors[0], failure) + assert.equal(error.errors.length, 2) + + return true + }) + assert.equal(fs.readFileSync(path.join(marker, 'foreign-data'), 'utf8'), 'keep') + } finally { + fs.rmSync(home, { recursive: true, force: true }) + } +}) diff --git a/apps/desktop/electron/updater/relaunch-waiter-lifecycle.test.ts b/apps/desktop/electron/updater/relaunch-waiter-lifecycle.test.ts new file mode 100644 index 0000000000..a7b75f2f34 --- /dev/null +++ b/apps/desktop/electron/updater/relaunch-waiter-lifecycle.test.ts @@ -0,0 +1,115 @@ +import assert from 'node:assert/strict' +import { type ChildProcess, spawn } from 'node:child_process' +import { once } from 'node:events' +import fs from 'node:fs' +import path from 'node:path' + +import { test, vi } from 'vitest' + +import { type SpawnWaiter, startRelaunchWaiter } from './relaunch-waiter' + +const scriptPath = path.resolve(import.meta.dirname, '../../scripts/update-relaunch-waiter.ps1') + +async function stop(child: ChildProcess | undefined): Promise { + if (child?.pid && child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'close') + child.kill() + await exited + } +} + +for (const mode of [ + { name: 'ready', ready: true }, + { name: 'silent', ready: false }, + { name: 'spawn-error', ready: false, missing: true }, + { name: 'cancel-timeout', ready: true, refuseKill: true }, + { name: 'startup-cancel-timeout', ready: false, refuseKill: true }, + { name: 'cleanup-error', ready: true, refuseCleanup: true } +]) { + test(`waiter ownership survives its complete lifecycle (${mode.name})`, async () => { + let child: ChildProcess | undefined + let originalKill: ChildProcess['kill'] | undefined + let stage = '' + let closed = false + const cleanupError = new Error('fixture staging cleanup refused') + + const start: SpawnWaiter = (_command, args, options) => { + stage = options.cwd + const readyFile = args[args.indexOf('-ReadyFile') + 1] + child = spawn(mode.missing ? path.join(stage, 'missing.exe') : process.execPath, ['-e', ` + const fs = require('node:fs'); + if (process.env.TEST_READY === 'yes') fs.writeFileSync(process.env.READY_FILE, 'ready'); + setInterval(() => {}, 1000); + `], { + ...options, + env: { ...process.env, READY_FILE: readyFile, TEST_READY: mode.ready ? 'yes' : 'no' } + }) + originalKill = child.kill.bind(child) + + if (mode.refuseKill) { child.kill = () => false } + child.once('close', () => { closed = true }) + + return child + } + + try { + const starting = startRelaunchWaiter({ + processId: process.pid, + processStartTimeMs: Date.now(), + identityName: 'disposable-waiter-test', + scriptPath + }, { spawn: start, handshakeTimeoutMs: mode.ready ? 10_000 : 2_000, cancelTimeoutMs: 2_000, pollMs: 20 }) + + if (!mode.ready && mode.refuseKill) { + await assert.rejects(starting, /did not exit after cancellation/) + assert.equal(closed, false) + assert.equal(fs.existsSync(stage), true) + + return + } + + const handle = await starting + + if (mode.ready) { + assert.ok(handle && typeof handle === 'object', 'readiness must retain the cancellation handle') + assert.equal(closed, false) + + if (mode.refuseCleanup) { + const remove = fs.promises.rm.bind(fs.promises) + vi.spyOn(fs.promises, 'rm').mockImplementation((file, options) => { + return file === stage ? Promise.reject(cleanupError) : remove(file, options) + }) + } + + const cancelled = handle.cancel() + assert.equal(handle.cancel(), cancelled, 'concurrent cancellation shares its result') + + if (mode.refuseKill || mode.refuseCleanup) { + await assert.rejects(cancelled, error => mode.refuseCleanup + ? error === cleanupError + : error instanceof Error && error.message.includes('did not exit after cancellation')) + assert.equal(handle.cancel(), cancelled) + assert.equal(closed, !mode.refuseKill) + assert.equal(fs.existsSync(stage), true) + + return + } + + await cancelled + await handle.cancel() + } else { + assert.equal(handle, undefined, 'a failed start has no live mechanism') + } + + assert.equal(closed, true, 'the actual child has exited before the caller proceeds') + assert.equal(fs.existsSync(stage), false) + } finally { + vi.restoreAllMocks() + + if (child && originalKill) { child.kill = originalKill } + await stop(child) + + if (stage) {fs.rmSync(stage, { recursive: true, force: true })} + } + }, 20_000) +} diff --git a/apps/desktop/electron/updater/relaunch-waiter.test.ts b/apps/desktop/electron/updater/relaunch-waiter.test.ts index ac222af285..3913bb326d 100644 --- a/apps/desktop/electron/updater/relaunch-waiter.test.ts +++ b/apps/desktop/electron/updater/relaunch-waiter.test.ts @@ -45,9 +45,15 @@ function fakeSpawn(handshake: 'ready' | 'error' | 'exit-nonzero' | 'none'): { sp const child: any = new EventEmitter() + child.pid = handshake === 'error' ? undefined : 123 + child.unref = () => {} - child.kill = () => { seen.killed = true; child.emit('exit', null) } + child.kill = () => { + seen.killed = true + child.emit('exit', null) + child.emit('close', null) + } const readyFile = args[args.indexOf('-ReadyFile') + 1] @@ -57,8 +63,10 @@ function fakeSpawn(handshake: 'ready' | 'error' | 'exit-nonzero' | 'none'): { sp fs.writeFileSync(readyFile, '0.18.1|Family|App') } else if (handshake === 'error') { child.emit('error', new Error('spawn ENOENT')) + child.emit('close', -1) } else if (handshake === 'exit-nonzero') { child.emit('exit', 3) + child.emit('close', 3) } }) @@ -109,7 +117,7 @@ describe('startRelaunchWaiter', () => { const result = await startRelaunchWaiter({ ...OPTIONS, scriptPath }, { spawn, pollMs: 10 }) - expect(result).toBe(true) + expect(result).toHaveProperty('cancel') expect(seen.command).toBe(POWERSHELL_PATH) expect(seen.opts.detached).toBe(true) expect(seen.opts.stdio).toBe('ignore') @@ -123,34 +131,35 @@ describe('startRelaunchWaiter', () => { expect(fs.existsSync(stagedScript)).toBe(true) }) - it('resolves true on the ready-file handshake (the waiter snapshotted the OLD package)', async () => { + it('retains cancellation after the ready-file handshake', async () => { const scriptPath = await stageScript() const { spawn } = fakeSpawn('ready') const result = await startRelaunchWaiter({ ...OPTIONS, scriptPath }, { spawn, pollMs: 10 }) - expect(result).toBe(true) + expect(result).toHaveProperty('cancel') + await result!.cancel() }) - it('resolves false when the child errors before the handshake', async () => { + it('returns no handle after a failed spawn closes', async () => { const scriptPath = await stageScript() const { spawn } = fakeSpawn('error') const result = await startRelaunchWaiter({ ...OPTIONS, scriptPath }, { spawn, pollMs: 10 }) - expect(result).toBe(false) + expect(result).toBeUndefined() }) - it('resolves false on a non-zero early exit (waiter never signalled ready)', async () => { + it('returns no handle on an early exit without readiness', async () => { const scriptPath = await stageScript() const { spawn } = fakeSpawn('exit-nonzero') const result = await startRelaunchWaiter({ ...OPTIONS, scriptPath }, { spawn, pollMs: 10 }) - expect(result).toBe(false) + expect(result).toBeUndefined() }) - it('resolves false when the handshake never arrives within the deadline', async () => { + it('stops the child before returning when readiness times out', async () => { const scriptPath = await stageScript() const { spawn, seen } = fakeSpawn('none') @@ -159,12 +168,12 @@ describe('startRelaunchWaiter', () => { { spawn, handshakeTimeoutMs: 80, pollMs: 20 } ) - expect(result).toBe(false) + expect(result).toBeUndefined() expect(seen.killed).toBe(true) expect(fs.existsSync(seen.opts.cwd)).toBe(false) }, 5_000) - it('resolves false when staging fails (missing script), never throwing', async () => { + it('returns no handle when a missing script leaves no staging', async () => { const spawn = (() => { throw new Error('should not be reached') }) as unknown as SpawnWaiter @@ -174,7 +183,7 @@ describe('startRelaunchWaiter', () => { { spawn } ) - expect(result).toBe(false) + expect(result).toBeUndefined() }) it('the staged handshake file uses the reserved ready filename', async () => { diff --git a/apps/desktop/electron/updater/relaunch-waiter.ts b/apps/desktop/electron/updater/relaunch-waiter.ts index 21b140e698..9f0cc0c875 100644 --- a/apps/desktop/electron/updater/relaunch-waiter.ts +++ b/apps/desktop/electron/updater/relaunch-waiter.ts @@ -44,6 +44,7 @@ export interface RelaunchWaiterDeps { handshakeTimeoutMs?: number /** Poll interval for the ready file (tests shrink this). */ pollMs?: number + cancelTimeoutMs?: number } /** Pure: the exact argv the waiter is spawned with. */ @@ -68,38 +69,50 @@ export function buildRelaunchWaiterArgs(options: RelaunchWaiterOptions, readyFil ] } -/** Stage script + handshake file into a fresh temp dir outside the package. */ -export async function stageRelaunchWaiter(options: RelaunchWaiterOptions): Promise { - const stageDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'hermes-relaunch-')) +/** Stage outside the package so its replacement does not invalidate the waiter. */ +async function stageRelaunchWaiter(options: RelaunchWaiterOptions): Promise { + let stageDir: string + + try { + stageDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'hermes-relaunch-')) + } catch { + return undefined + } + const scriptPath = path.join(stageDir, RELAUNCH_WAITER_SCRIPT) const readyFile = path.join(stageDir, RELAUNCH_WAITER_READY_FILENAME) try { await fs.promises.copyFile(options.scriptPath, scriptPath) } catch (error) { - await fs.promises.rm(stageDir, { recursive: true, force: true }) - throw error + try { + await fs.promises.rm(stageDir, { recursive: true, force: true }) + } catch (cleanupError) { + throw new AggregateError([error, cleanupError], 'Relaunch waiter staging cleanup failed') + } + + return undefined } return { stageDir, scriptPath, readyFile } } -/** Return true only after the waiter confirms its package snapshot. */ +export interface RelaunchWaiterHandle { + cancel: () => Promise +} + +/** Return ownership after readiness, or no handle after a safely stopped failure. */ export async function startRelaunchWaiter( options: RelaunchWaiterOptions, deps: RelaunchWaiterDeps = {} -): Promise { +): Promise { const spawn = deps.spawn ?? (nodeSpawn as unknown as SpawnWaiter) const handshakeTimeoutMs = deps.handshakeTimeoutMs ?? DEFAULT_RELAUNCH_WAITER_HANDSHAKE_MS const pollMs = deps.pollMs ?? 250 + const cancelTimeoutMs = deps.cancelTimeoutMs ?? 10_000 + const staging = await stageRelaunchWaiter(options) - let staging: WaiterStaging - - try { - staging = await stageRelaunchWaiter(options) - } catch { - return false - } + if (!staging) { return undefined } const cleanup = () => fs.promises.rm(staging.stageDir, { recursive: true, force: true }) let child: ChildProcess @@ -108,13 +121,50 @@ export async function startRelaunchWaiter( child = spawn(POWERSHELL_PATH, buildRelaunchWaiterArgs({ ...options, scriptPath: staging.scriptPath }, staging.readyFile), { detached: true, stdio: 'ignore', windowsHide: true, cwd: staging.stageDir }) - } catch { - await cleanup() + } catch (error) { + try { + await cleanup() + } catch (cleanupError) { + throw new AggregateError([error, cleanupError], 'Relaunch waiter spawn cleanup failed') + } - return false + return undefined } - return new Promise(resolve => { + let closed = false + + const closedPromise = new Promise(resolve => { + child.once('close', () => { closed = true; resolve() }) + }) + + let cancellation: Promise | undefined + + const cancel = (): Promise => { + cancellation ??= (async () => { + if (!closed) { + if (child.pid !== undefined) { child.kill() } + + let timeout: ReturnType | undefined + + try { + await Promise.race([ + closedPromise, + new Promise((_, reject) => { + timeout = setTimeout(() => reject(new Error('Relaunch waiter did not exit after cancellation')), cancelTimeoutMs) + }) + ]) + } finally { + clearTimeout(timeout) + } + } + + await cleanup() + })() + + return cancellation + } + + const ready = await new Promise(resolve => { let settled = false let timer: ReturnType const deadline = Date.now() + handshakeTimeoutMs @@ -123,18 +173,11 @@ export async function startRelaunchWaiter( if (settled) { return } settled = true clearTimeout(timer) - - if (ready) { - child.unref() - resolve(true) - } else { - child.kill() - void cleanup().catch(() => {}).finally(() => resolve(false)) - } + resolve(ready) } child.once('error', () => finish(false)) - child.once('exit', () => finish(false)) + child.once('close', () => finish(false)) const check = () => { if (settled) { return } @@ -150,4 +193,14 @@ export async function startRelaunchWaiter( check() }) + + if (!ready) { + await cancel() + + return undefined + } + + child.unref() + + return { cancel } } diff --git a/apps/desktop/electron/updater/relaunch.ts b/apps/desktop/electron/updater/relaunch.ts index cba87f8d5d..7534c5a428 100644 --- a/apps/desktop/electron/updater/relaunch.ts +++ b/apps/desktop/electron/updater/relaunch.ts @@ -1,32 +1,12 @@ -// updater/relaunch.ts — unconditional relaunch after an App Installer swap. -// -// Contract: clicking Update always ends in Hermes reopening on the new -// version — no user action between quit and relaunch. Implementation: a -// one-shot pending-relaunch marker written before we quit. The OS App -// Installer swaps the package; on next launch (whenever it happens) Hermes -// reads the marker, shows the "updated" toast, and deletes it. -// -// The marker is a JSON file under HERMES_HOME (not a run-key / scheduled -// task): a registry entry can't be made one-shot safely from a dying process, -// and a file survives the package swap (HERMES_HOME lives outside the -// package). The actual relaunch is the detached waiter -// (relaunch-waiter.ts + scripts/update-relaunch-waiter.ps1): it waits for -// this process to exit, waits for the installed package version to change, -// and activates the NEW package. The marker's job is only to tell the NEW -// version it was an update relaunch, so it can toast + clean up. -// -// If the OS does NOT relaunch (update applied later, on next launch), the -// marker is still correct: the first launch after the swap detects it and -// toasts. A stale marker (update cancelled/failed) self-deletes when its -// expected-version no longer matches a later install... no — cancelled -// updates mean the version never changed, so we stamp with "expected NEW -// version unknown" instead: the marker only records that an update was -// STARTED; the new version decides "was it really me?" by comparing versions -// recorded at write time vs launch time. +// The marker survives a package swap and records the version that started it. +// The separate waiter owns automatic relaunch. Cancelling an update stops +// that waiter and removes its marker, including when startup stays manual. import * as fs from 'node:fs' import * as path from 'node:path' +import type { RelaunchWaiterHandle } from './relaunch-waiter' + export interface PendingRelaunchMarker { schemaVersion: 1 /** Version of the app that wrote the marker (pre-update). */ @@ -63,38 +43,74 @@ export function writePendingRelaunch( } } -/** - * Register the post-update relaunch: write the pending-relaunch marker AND - * start the relaunch mechanism. The mechanism itself is injected — the - * App Installer arm passes the detached relaunch waiter (relaunch-waiter.ts), - * which re-opens the NEW package after the OS swap; nothing here re-executes - * the current binary. The relaunched instance reads the marker, toasts, and - * cleans it up. Failure never blocks the update — relaunch just stays - * manual (the marker alone still produces the "updated" toast on the next - * manual launch). - */ export interface UpdateRelaunchDeps { - /** Start the relaunch mechanism (the detached relaunch waiter). */ - relaunch: () => boolean | void | Promise + /** Return a ready waiter, or no handle after a safely stopped failure. */ + relaunch: () => RelaunchWaiterHandle | undefined | Promise } +export interface RelaunchRegistration { + automatic: boolean + cancel: () => Promise +} + +/** Marker failure permits an update, but failed waiter cleanup must abort it. */ export async function registerUpdateRelaunch( hermesHome: string, fromVersion: string, deps: UpdateRelaunchDeps, writeFile: (file: string, contents: string) => void = (f, c) => fs.writeFileSync(f, c) -): Promise { - // Marker first: a relaunch that wins the race against the write still - // leaves the next manual launch marker-less (honest), but a marker without - // a started mechanism is the exact "marker is not a mechanism" gap. - writePendingRelaunch(hermesHome, fromVersion, writeFile) +): Promise { + const ownsMarker = writePendingRelaunch(hermesHome, fromVersion, writeFile) + + const removeMarker = (): void => { + if (!ownsMarker) { return } + + try { + fs.unlinkSync(markerPath(hermesHome)) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { throw error } + } + } + + let waiter: RelaunchWaiterHandle | undefined try { - // `undefined` (a fire-and-forget mechanism) counts as started; only an - // explicit false or a throw means registration failed. - return (await deps.relaunch()) !== false - } catch { - return false + waiter = await deps.relaunch() + } catch (error) { + try { + removeMarker() + } catch (cleanupError) { + throw new AggregateError([error, cleanupError], 'Relaunch registration cleanup failed', { cause: error }) + } + + throw error + } + + let cancellation: Promise | undefined + + return { + automatic: waiter !== undefined, + cancel: () => { + cancellation ??= (async () => { + const errors: unknown[] = [] + + try { + await waiter?.cancel() + } catch (error) { + errors.push(error) + } + + try { + removeMarker() + } catch (error) { + errors.push(error) + } + + if (errors.length) { throw new AggregateError(errors, 'Relaunch cancellation failed') } + })() + + return cancellation + } } } diff --git a/apps/desktop/electron/updater/updater.test.ts b/apps/desktop/electron/updater/updater.test.ts index 3f500ea312..6fd3ae1943 100644 --- a/apps/desktop/electron/updater/updater.test.ts +++ b/apps/desktop/electron/updater/updater.test.ts @@ -167,40 +167,29 @@ describe('registerUpdateRelaunch — the mechanism, not just the marker', () => const ok = await registerUpdateRelaunch( '/home', '0.18.2', - { relaunch: () => { started += 1 } }, + { relaunch: () => { started += 1; + + return { cancel: async () => {} } } }, (f, c) => { files[f] = c as string } ) - expect(ok).toBe(true) + expect(ok.automatic).toBe(true) expect(started).toBe(1) expect(Object.keys(files)).toHaveLength(1) }) - it('awaits the mechanism outcome: a waiter that reports false leaves the marker as the only trace', async () => { + it('retains the marker when a safely failed waiter requires manual relaunch', async () => { const files: Record = {} const ok = await registerUpdateRelaunch( '/home', '0.18.2', - { relaunch: async () => false }, + { relaunch: async () => undefined }, (f, c) => { files[f] = c as string } ) - expect(ok).toBe(false) + expect(ok.automatic).toBe(false) expect(Object.keys(files)).toHaveLength(1) }) - it('a mechanism throw never blocks the update (marker-only manual relaunch)', async () => { - const files: Record = {} - - const ok = await registerUpdateRelaunch( - '/home', - '0.18.2', - { relaunch: () => { throw new Error('spawn failed') } }, - (f, c) => { files[f] = c as string } - ) - - expect(ok).toBe(false) - expect(Object.keys(files)).toHaveLength(1) - }) })