Fix terminal daemon restart leaving macOS service registered (#29130)

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.<profile hash>.v<protocol>`). 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,

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<void> {
   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<void> => {
     // 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<void> {
       process.exit(0)
     },
     onRpcShutdown: () => {
-      deathWatch?.stop()
-      shuttingDown = true
+      shutdownDeadline.cancel()
       daemonLog.close()
       process.exit(0)
-    }
+    },
+    onShutdownRequested: beginShutdown
   })
   deathWatch?.start()
 

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<DaemonHandl
     ...(hostTerminalLedgerDir ? { hostTerminalLedgerDir } : {}),
     ...(opts.onIdleShutdown ? { onIdleShutdown: opts.onIdleShutdown } : {}),
     ...(opts.onRpcShutdown ? { onRpcShutdown: opts.onRpcShutdown } : {}),
+    ...(opts.onShutdownRequested ? { onShutdownRequested: opts.onShutdownRequested } : {}),
     ...(opts.initialAdoptionTestConfig
       ? { initialAdoptionTestConfig: opts.initialAdoptionTestConfig }
       : {})

src/main/daemon/daemon-rpc-shutdown-deadline.test.ts

@@ -0,0 +1,93 @@
+import './mock-descendant-sweep'
+import { mkdtemp, rm } from 'node:fs/promises'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { afterEach, beforeEach, expect, it, vi } from 'vitest'
+import { DaemonClient } from './client'
+import { startDaemon, type DaemonHandle } from './daemon-main'
+import {
+  createDaemonShutdownDeadline,
+  DAEMON_SHUTDOWN_TIMEOUT_MS
+} from './daemon-shutdown-deadline'
+import { getDaemonSocketPath } from './daemon-spawner'
+import { TerminalHost } from './terminal-host'
+
+let directory: string
+let daemon: DaemonHandle
+let client: DaemonClient
+let releaseDisposal: (() => 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<typeof vi.fn>
+  log: ReturnType<typeof vi.fn>
+  requested: ReturnType<typeof vi.fn>
+}> {
+  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<void>((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()
+})

src/main/daemon/daemon-server-lifecycle.ts

@@ -19,6 +19,7 @@ type DaemonServerLifecycleOptions = {
   disposeResources: () => Promise<void>
   onIdleShutdown: () => void
   onRpcShutdown: () => void
+  onShutdownRequested: () => void
 }
 
 export class DaemonServerLifecycle {
@@ -148,6 +149,7 @@ export class DaemonServerLifecycle {
   }
 
   beginOrdinaryShutdownFence(): Promise<void> {
+    this.options.onShutdownRequested()
     this.state = 'shutting-down'
     this.cancelInitialAdoptionTimer()
     this.ordinaryShutdownServerClose ??= this.beginServerClose()

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: {

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,

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<typeof setTimeout> | 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
+    }
+  }
+}

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)
+})

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<void> {
 }
 
 /** True once no process can start under `service`: missing, or stopped and booted out. */
-export async function releaseMacDaemonJob(service: string): Promise<boolean> {
-  const state = await readMacDaemonJobState(service)
+export async function releaseMacDaemonJob(
+  service: string,
+  waitUntilMs = Date.now()
+): Promise<boolean> {
+  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'
   }

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)

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.