From 45862c2b40d1e0505f41ac25ca282cd4a3a77cc7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 15 Oct 2026 03:41:43 -0700 Subject: [PATCH] Fix terminal daemon restart leaving macOS service registered (#29130) --- .../reference/macos-daemon-tcc-attribution.md | 6 +- src/main/daemon/daemon-entry.ts | 33 ++++--- src/main/daemon/daemon-main.ts | 2 + .../daemon-rpc-shutdown-deadline.test.ts | 93 +++++++++++++++++++ src/main/daemon/daemon-server-lifecycle.ts | 2 + src/main/daemon/daemon-server-options.ts | 1 + src/main/daemon/daemon-server.ts | 3 +- src/main/daemon/daemon-shutdown-deadline.ts | 28 ++++++ .../daemon/macos-daemon-job-state.test.ts | 35 +++++++ src/main/daemon/macos-daemon-job-state.ts | 13 ++- src/main/daemon/macos-daemon-launchd.test.ts | 10 +- src/main/daemon/macos-daemon-launchd.ts | 9 +- 12 files changed, 211 insertions(+), 24 deletions(-) create mode 100644 src/main/daemon/daemon-rpc-shutdown-deadline.test.ts create mode 100644 src/main/daemon/daemon-shutdown-deadline.ts diff --git a/docs/reference/macos-daemon-tcc-attribution.md b/docs/reference/macos-daemon-tcc-attribution.md index 26772dda46f..f180c029e44 100644 --- a/docs/reference/macos-daemon-tcc-attribution.md +++ b/docs/reference/macos-daemon-tcc-attribution.md @@ -31,7 +31,8 @@ So a packaged macOS GUI app starts its daemon from a private copy that the updat non-persistent launchd job, so the daemon has Orca's own identity rather than the UI process's replaceable path. The label is fixed per profile and protocol (`com.stablyai.orca.terminal..v`). Before bootstrap a stopped job with - that label is booted out; a running or unreadable one is fatal. `--entry-path` stays the installed + that label is booted out. A running job gets a bounded wait for shutdown and launchd retirement; + a job still running afterward, or an unreadable one, is fatal. `--entry-path` stays the installed app's entry, which the replacement preflight compares. The job file is 0600 and deleted right after bootstrap, because the environment may hold credentials. It sets `AssociatedBundleIdentifiers` to `com.stablyai.orca`, which is how a launchd job not installed @@ -122,6 +123,9 @@ keeps about 124 MiB. ## Existing sessions and status +RPC and signal shutdown share a five-second process-exit deadline, armed before terminal teardown. +A stalled teardown must not leave the endpoint retired while the launchd job stays alive forever. + Running old daemons keep their terminals; settings reports app and terminal-host Full Disk Access separately, and a denied terminal is recovered by closing it or restarting the daemon. The packaging hook also gives the main bundle and every Helper the Desktop, Documents and Downloads descriptions, diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index 7ef5053f840..3f3a7c80b52 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -31,6 +31,7 @@ import { isNativePtyException } from './daemon-native-pty-exception' import { startDaemonScopeDeathWatch } from './daemon-scope-death-watch' import { isWindowsProcessTableAvailable } from '../windows/windows-process-table' import { leaseAppImageGenerationForCurrentProcess } from '../cli/appimage-generation-lease' +import { createDaemonShutdownDeadline } from './daemon-shutdown-deadline' export type ParsedDaemonArgs = { socketPath: string @@ -181,35 +182,33 @@ async function main(): Promise { let daemon: DaemonHandle | null = null let deathWatch: MacosLoginSessionDeathWatch | null = null let shuttingDown = false - // Bound the wait so a wedged native shutdown can't leave the daemon running - // forever on SIGTERM/SIGINT (it would then survive a real quit, not just updates). - const SHUTDOWN_TIMEOUT_MS = 5000 + const shutdownDeadline = createDaemonShutdownDeadline(daemonLog, () => process.exit(0)) + const beginShutdown = (): void => { + shuttingDown = true + shutdownDeadline.start() + // Why before the async teardown: a scope stop kills the AppImage mount alongside us, and a + // teardown cut short by SIGBUS must not read as a crash that pauses the next start's restores. + daemon?.recordStopRequested() + deathWatch?.stop() + } const shutdown = async (reason: string): Promise => { // SIGTERM and SIGINT can both fire; guard against a double daemon.shutdown(). if (shuttingDown) { return } - shuttingDown = true - // Why before the async teardown: a scope stop kills the AppImage mount alongside us, and a - // teardown cut short by SIGBUS must not read as a crash that pauses the next start's restores. - daemon?.recordStopRequested() - deathWatch?.stop() + beginShutdown() daemonLog.log('shutdown', { reason }) try { if (daemon) { - await Promise.race([ - daemon.shutdown(), - new Promise((_, reject) => - setTimeout(() => reject(new Error('shutdown timeout')), SHUTDOWN_TIMEOUT_MS) - ) - ]) + await daemon.shutdown() daemon = null } } catch (err) { // Never let a rejected shutdown() escape as an unhandled rejection and skip exit. daemonLog.log('shutdown-error', { message: (err as Error)?.message }) } finally { + shutdownDeadline.cancel() daemonLog.close() process.exit(0) } @@ -329,11 +328,11 @@ async function main(): Promise { process.exit(0) }, onRpcShutdown: () => { - deathWatch?.stop() - shuttingDown = true + shutdownDeadline.cancel() daemonLog.close() process.exit(0) - } + }, + onShutdownRequested: beginShutdown }) deathWatch?.start() diff --git a/src/main/daemon/daemon-main.ts b/src/main/daemon/daemon-main.ts index 89d9627dd91..c8e591116d8 100644 --- a/src/main/daemon/daemon-main.ts +++ b/src/main/daemon/daemon-main.ts @@ -27,6 +27,7 @@ export type DaemonStartOptions = { hostTerminalLedgerDir?: string onIdleShutdown?: () => void onRpcShutdown?: () => void + onShutdownRequested?: () => void initialAdoptionTestConfig?: DaemonServerOptions['initialAdoptionTestConfig'] } @@ -74,6 +75,7 @@ export async function startDaemon(opts: DaemonStartOptions): Promise void) | undefined +let cancelDeadline: (() => void) | undefined + +beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'orca-shutdown-deadline-')) +}) + +afterEach(async () => { + cancelDeadline?.() + releaseDisposal?.() + client?.disconnect() + vi.useRealTimers() + await daemon?.shutdown() + vi.restoreAllMocks() + await rm(directory, { recursive: true, force: true }) +}) + +async function start(): Promise<{ + exit: ReturnType + log: ReturnType + requested: ReturnType +}> { + const exit = vi.fn() + const log = vi.fn() + const deadline = createDaemonShutdownDeadline({ log, close: () => {} }, exit) + cancelDeadline = deadline.cancel + const requested = vi.fn(deadline.start) + const socketPath = getDaemonSocketPath(directory) + const tokenPath = join(directory, 'daemon.token') + daemon = await startDaemon({ + socketPath, + tokenPath, + spawnSubprocess: () => { + throw new Error('No PTY expected') + }, + onShutdownRequested: requested, + onRpcShutdown: deadline.cancel + }) + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + return { exit, log, requested } +} + +it('bounds RPC shutdown before terminal disposal, even when disposal never settles', async () => { + const { exit, log, requested } = await start() + const stalled = new Promise((resolve) => { + releaseDisposal = resolve + }) + const dispose = vi.spyOn(TerminalHost.prototype, 'dispose').mockReturnValue(stalled) + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const shutdown = client.request('shutdown', { killSessions: true }, 30_000).catch(() => {}) + await vi.waitFor(() => expect(dispose).toHaveBeenCalled()) + expect(requested).toHaveBeenCalledOnce() + + // A second stop must not extend the first request's deadline. + await vi.advanceTimersByTimeAsync(4_000) + const secondShutdown = daemon.shutdown() + await vi.advanceTimersByTimeAsync(DAEMON_SHUTDOWN_TIMEOUT_MS - 4_000) + expect(exit).toHaveBeenCalledOnce() + expect(log).toHaveBeenCalledWith('shutdown-error', { message: 'shutdown timeout' }) + + releaseDisposal?.() + client.disconnect() + await shutdown + await secondShutdown +}) + +it('cancels the deadline when the RPC completes normally', async () => { + const { exit, requested } = await start() + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + await client.request('shutdown', { killSessions: true }) + await vi.waitFor(() => expect(vi.getTimerCount()).toBe(0)) + expect(requested).toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(DAEMON_SHUTDOWN_TIMEOUT_MS) + expect(exit).not.toHaveBeenCalled() +}) diff --git a/src/main/daemon/daemon-server-lifecycle.ts b/src/main/daemon/daemon-server-lifecycle.ts index 0d4379b95e0..f2ce63314e5 100644 --- a/src/main/daemon/daemon-server-lifecycle.ts +++ b/src/main/daemon/daemon-server-lifecycle.ts @@ -19,6 +19,7 @@ type DaemonServerLifecycleOptions = { disposeResources: () => Promise onIdleShutdown: () => void onRpcShutdown: () => void + onShutdownRequested: () => void } export class DaemonServerLifecycle { @@ -148,6 +149,7 @@ export class DaemonServerLifecycle { } beginOrdinaryShutdownFence(): Promise { + this.options.onShutdownRequested() this.state = 'shutting-down' this.cancelInitialAdoptionTimer() this.ordinaryShutdownServerClose ??= this.beginServerClose() diff --git a/src/main/daemon/daemon-server-options.ts b/src/main/daemon/daemon-server-options.ts index fe7724c62db..ca08680f26b 100644 --- a/src/main/daemon/daemon-server-options.ts +++ b/src/main/daemon/daemon-server-options.ts @@ -15,6 +15,7 @@ export type DaemonServerOptions = { protocolVersion?: number onIdleShutdown?: () => void onRpcShutdown?: () => void + onShutdownRequested?: () => void initialAdoptionTestConfig?: { timeoutMs: number clock: { diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 20c45d3304c..95b1f363902 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -198,7 +198,8 @@ export class DaemonServer { isIdle: () => this.isIdle(), disposeResources: () => this.disposeResources(), onIdleShutdown: options.onIdleShutdown ?? (() => {}), - onRpcShutdown: options.onRpcShutdown ?? (() => {}) + onRpcShutdown: options.onRpcShutdown ?? (() => {}), + onShutdownRequested: options.onShutdownRequested ?? (() => {}) }) this.admission = new DaemonTerminalAdmission({ host: this.host, diff --git a/src/main/daemon/daemon-shutdown-deadline.ts b/src/main/daemon/daemon-shutdown-deadline.ts new file mode 100644 index 00000000000..6e46de0a893 --- /dev/null +++ b/src/main/daemon/daemon-shutdown-deadline.ts @@ -0,0 +1,28 @@ +import type { DaemonFileLog } from './daemon-file-log' + +export const DAEMON_SHUTDOWN_TIMEOUT_MS = 5_000 + +/** Bounds every process shutdown, including RPC teardown that never settles. */ +export function createDaemonShutdownDeadline( + log: DaemonFileLog, + exit: () => void +): { start: () => void; cancel: () => void } { + let timer: ReturnType | undefined + return { + start: () => { + if (timer) { + return + } + timer = setTimeout(() => { + log.log('shutdown-error', { message: 'shutdown timeout' }) + log.close() + exit() + }, DAEMON_SHUTDOWN_TIMEOUT_MS) + timer.unref() + }, + cancel: () => { + clearTimeout(timer) + timer = undefined + } + } +} diff --git a/src/main/daemon/macos-daemon-job-state.test.ts b/src/main/daemon/macos-daemon-job-state.test.ts index af9da9e3c74..40f24884320 100644 --- a/src/main/daemon/macos-daemon-job-state.test.ts +++ b/src/main/daemon/macos-daemon-job-state.test.ts @@ -97,3 +97,38 @@ it.each([ expect(await releaseMacDaemonJob('gui/501/owned')).toBe(released) } ) + +it.each(['stopped', 'missing'] as const)( + 'waits for a retiring running job to become %s before releasing its label', + async (state) => { + run + .mockResolvedValueOnce(lingering) + .mockResolvedValueOnce(lingering) + .mockResolvedValueOnce( + state === 'stopped' ? { ...result, stdout: '\tstate = not running' } : missing + ) + .mockResolvedValueOnce(result) + .mockResolvedValueOnce(missing) + expect(await releaseMacDaemonJob('gui/501/owned', 1_000)).toBe(true) + expect(clock.now).toBe(500) + expect(run.mock.calls.filter(([spec]) => spec.args?.[0] === 'bootout')).toHaveLength( + state === 'stopped' ? 1 : 0 + ) + } +) + +it('bounds the retirement wait and never stops a job that remains running', async () => { + run.mockResolvedValue(lingering) + expect(await releaseMacDaemonJob('gui/501/owned', 125)).toBe(false) + expect(clock.now).toBe(125) + expect(run.mock.calls.every(([spec]) => spec.args?.[0] === 'print')).toBe(true) +}) + +it('does not wait or boot out when a retiring job becomes unreadable', async () => { + run + .mockResolvedValueOnce(lingering) + .mockResolvedValue({ ...result, code: 1, stderr: 'Operation not permitted' }) + expect(await releaseMacDaemonJob('gui/501/owned', 1_000)).toBe(false) + expect(clock.now).toBe(250) + expect(run.mock.calls.every(([spec]) => spec.args?.[0] === 'print')).toBe(true) +}) diff --git a/src/main/daemon/macos-daemon-job-state.ts b/src/main/daemon/macos-daemon-job-state.ts index 00fd0f726b0..9e6b8d8f456 100644 --- a/src/main/daemon/macos-daemon-job-state.ts +++ b/src/main/daemon/macos-daemon-job-state.ts @@ -3,6 +3,7 @@ import { runProcess } from '@orca/process-host' const JOB_REMOVAL_TIMEOUT_MS = 3_000 const JOB_REMOVAL_POLL_MS = 50 +const JOB_RETIREMENT_POLL_MS = 250 export type MacDaemonJobState = 'running' | 'stopped' | 'missing' | 'unverifiable' @@ -54,8 +55,16 @@ export async function stopMacDaemonJob(service: string): Promise { } /** True once no process can start under `service`: missing, or stopped and booted out. */ -export async function releaseMacDaemonJob(service: string): Promise { - const state = await readMacDaemonJobState(service) +export async function releaseMacDaemonJob( + service: string, + waitUntilMs = Date.now() +): Promise { + let state = await readMacDaemonJobState(service) + // Endpoint retirement can precede process exit; never boot out a running or unreadable job. + while (state === 'running' && Date.now() < waitUntilMs) { + await delay(Math.max(1, Math.min(JOB_RETIREMENT_POLL_MS, waitUntilMs - Date.now()))) + state = await readMacDaemonJobState(service) + } if (state !== 'stopped') { return state === 'missing' } diff --git a/src/main/daemon/macos-daemon-launchd.test.ts b/src/main/daemon/macos-daemon-launchd.test.ts index 1912ec71612..e0be3a76fb4 100644 --- a/src/main/daemon/macos-daemon-launchd.test.ts +++ b/src/main/daemon/macos-daemon-launchd.test.ts @@ -263,12 +263,20 @@ it("boots out an earlier daemon's stopped job before starting the new one", asyn expect(verbs).toEqual(['bootout', 'bootstrap']) }) +it('lets a restarting daemon exit before removing its registration and bootstrapping', async () => { + launchctl.print = [running, stopped] + await expect(launchMacDaemonFromStableBundle(options, roomyDeadline())).resolves.not.toBeNull() + expect(launchctlCalls('bootout')).toHaveLength(1) + expect(launchctlCalls('bootstrap')).toHaveLength(1) +}) + it.each([ ['running', running], ['unverifiable', { ...ok, code: 1, stderr: 'Operation not permitted' }] ])('never starts beside an earlier job that reads %s', async (_state, print) => { launchctl.print = [print] - const error = await launchFailure() + // No retirement wait may consume the time reserved for bootstrap and readiness. + const error = await launchFailure(Date.now() + 20_000) expect(error).toBeInstanceOf(Error) expect(launchctlCalls('bootstrap')).toHaveLength(0) expect(launchctlCalls('bootout')).toHaveLength(0) diff --git a/src/main/daemon/macos-daemon-launchd.ts b/src/main/daemon/macos-daemon-launchd.ts index e878fd41838..2feb0cf4d66 100644 --- a/src/main/daemon/macos-daemon-launchd.ts +++ b/src/main/daemon/macos-daemon-launchd.ts @@ -13,6 +13,7 @@ import { DaemonEndpointOwnershipError, holdDaemonAdoptionLease } from './daemon- import { buildDaemonScriptArgs, type DaemonChildSpawnOptions } from './daemon-launched-child-spawn' import { PROTOCOL_VERSION } from './daemon-protocol-version' import type { DaemonProcessHandle } from './daemon-spawner' +import { DAEMON_SHUTDOWN_TIMEOUT_MS } from './daemon-shutdown-deadline' import { getMacDaemonBundleRoot, materializeMacDaemonBundle, @@ -139,8 +140,12 @@ export async function launchMacDaemonFromStableBundle( return fallBackToFork(reason, error, deadlineMs) } - // A stopped job from an earlier daemon holds the label; a running one may own the endpoint. - if (!(await releaseMacDaemonJob(service))) { + // Reserve bootstrap/readiness time while shutdown and launchd finish retiring the old job. + const releaseByMs = Math.min( + Date.now() + DAEMON_SHUTDOWN_TIMEOUT_MS + 2_000, + deadlineMs - BOOTSTRAP_TIMEOUT_MS - STARTUP_TIMEOUT_MS + ) + if (!(await releaseMacDaemonJob(service, releaseByMs))) { throw new Error('A macOS terminal service from an earlier launch is still registered') } // The inherited environment can contain credentials; never leave it on disk after bootstrap.