diff --git a/src/daemon/__tests__/daemon-exit-wait.test.ts b/src/daemon/__tests__/daemon-exit-wait.test.ts new file mode 100644 index 000000000..47a1e5f8b --- /dev/null +++ b/src/daemon/__tests__/daemon-exit-wait.test.ts @@ -0,0 +1,117 @@ +import { afterEach, beforeEach, expect, test, vi } from 'vitest'; +import { stopProcessForTakeover, waitForDaemonExit } from '../daemon-process.ts'; + +const DAEMON_COMMAND = '/opt/checkout/dist/src/internal/daemon.js'; +const OURS = 'Mon Aug 24 10:00:00 2026'; +const RECYCLED = 'Mon Aug 24 10:00:07 2026'; +const PID = 4242; +const TIMEOUT_MS = 1_000; +const POLL_MS = 5; + +const state = vi.hoisted(() => ({ + alive: new Map(), + starts: new Map(), + states: new Map(), + commands: new Map(), +})); + +vi.mock('../../utils/host-process.ts', async (importOriginal) => ({ + ...(await importOriginal()), + isProcessAlive: (pid: number) => state.alive.get(pid) ?? false, + readProcessStartTime: (pid: number) => state.starts.get(pid) ?? null, + readProcessCommand: (pid: number) => state.commands.get(pid) ?? null, + readHostProcessIdentityObservations: (pids: Iterable) => { + const observations = new Map(); + for (const pid of pids) { + const startTime = state.starts.get(pid); + if (startTime === undefined) continue; + observations.set(pid, { state: state.states.get(pid) ?? 'S', startTime }); + } + return observations; + }, +})); + +const signals: NodeJS.Signals[] = []; +let onSignal: (signal: NodeJS.Signals) => void = () => {}; + +beforeEach(() => { + state.alive.clear(); + state.starts.clear(); + state.states.clear(); + state.commands.clear(); + signals.length = 0; + onSignal = () => {}; + state.alive.set(PID, true); + state.starts.set(PID, OURS); + state.states.set(PID, 'S'); + state.commands.set(PID, DAEMON_COMMAND); + vi.spyOn(process, 'kill').mockImplementation(((pid: number, signal: NodeJS.Signals | 0) => { + if (pid !== PID || signal === 0) return true; + signals.push(signal); + onSignal(signal); + return true; + }) as typeof process.kill); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +test('waitForDaemonExit reports a pid recycled mid-wait as exited, without burning the deadline', async () => { + setTimeout(() => state.starts.set(PID, RECYCLED), 20); + const wait = await waitForDaemonExit( + { pid: PID, startTime: OURS }, + { timeoutMs: TIMEOUT_MS, pollMs: POLL_MS }, + ); + expect(wait.exited).toBe(true); + expect(wait.elapsedMs).toBeLessThan(TIMEOUT_MS / 2); +}); + +test('waitForDaemonExit reports a daemon that keeps its identity as not exited', async () => { + const wait = await waitForDaemonExit( + { pid: PID, startTime: OURS }, + { timeoutMs: 40, pollMs: POLL_MS }, + ); + expect(wait.exited).toBe(false); +}); + +test('waitForDaemonExit keeps waiting through a zombie until the pid is reaped', async () => { + state.states.set(PID, 'Z+'); + state.commands.set(PID, ''); + const stillTaken = await waitForDaemonExit( + { pid: PID, startTime: OURS }, + { timeoutMs: 40, pollMs: POLL_MS }, + ); + expect(stillTaken.exited).toBe(false); + + state.alive.set(PID, false); + const reaped = await waitForDaemonExit( + { pid: PID, startTime: OURS }, + { timeoutMs: TIMEOUT_MS, pollMs: POLL_MS }, + ); + expect(reaped.exited).toBe(true); +}); + +test('stopProcessForTakeover does not SIGKILL a pid recycled during the grace wait', async () => { + onSignal = (signal) => { + if (signal === 'SIGTERM') state.starts.set(PID, RECYCLED); + }; + await stopProcessForTakeover(PID, { + termTimeoutMs: TIMEOUT_MS, + killTimeoutMs: 40, + expectedStartTime: OURS, + }); + expect(signals).toEqual(['SIGTERM']); +}); + +test('stopProcessForTakeover still escalates to SIGKILL for a daemon that survives SIGTERM', async () => { + onSignal = (signal) => { + if (signal === 'SIGKILL') state.alive.set(PID, false); + }; + await stopProcessForTakeover(PID, { + termTimeoutMs: 40, + killTimeoutMs: 40, + expectedStartTime: OURS, + }); + expect(signals).toEqual(['SIGTERM', 'SIGKILL']); +}); diff --git a/src/daemon/__tests__/daemon-stop.test.ts b/src/daemon/__tests__/daemon-stop.test.ts index 0cd194892..cceec7ebd 100644 --- a/src/daemon/__tests__/daemon-stop.test.ts +++ b/src/daemon/__tests__/daemon-stop.test.ts @@ -8,16 +8,16 @@ const mocks = vi.hoisted(() => ({ isProcessAlive: vi.fn(), sleep: vi.fn(async () => undefined), trySignalProcess: vi.fn(), - waitForProcessExit: vi.fn(), + waitForDaemonExit: vi.fn(), })); vi.mock('../daemon-process.ts', () => ({ isAgentDeviceDaemonProcess: mocks.isAgentDeviceDaemonProcess, trySignalProcess: mocks.trySignalProcess, + waitForDaemonExit: mocks.waitForDaemonExit, })); vi.mock('../../utils/host-process.ts', () => ({ isProcessAlive: mocks.isProcessAlive, - waitForProcessExit: mocks.waitForProcessExit, })); vi.mock('../../utils/timeouts.ts', () => ({ sleep: mocks.sleep })); @@ -102,9 +102,9 @@ test('reports graceful cleanup after SIGTERM exits the verified daemon', async ( const paths = createDaemonPaths(); mocks.isAgentDeviceDaemonProcess.mockReturnValue(true); mocks.trySignalProcess.mockReturnValue(true); - mocks.waitForProcessExit.mockImplementation(async () => { + mocks.waitForDaemonExit.mockImplementation(async () => { fs.rmSync(paths.infoPath, { force: true }); - return true; + return { exited: true, elapsedMs: 0 }; }); try { @@ -125,7 +125,9 @@ test('re-verifies identity before SIGKILL and reports forced cleanup as unknown' const paths = createDaemonPaths(); mocks.isAgentDeviceDaemonProcess.mockReturnValue(true); mocks.trySignalProcess.mockReturnValue(true); - mocks.waitForProcessExit.mockResolvedValueOnce(false).mockResolvedValueOnce(true); + mocks.waitForDaemonExit + .mockResolvedValueOnce({ exited: false, elapsedMs: 0 }) + .mockResolvedValueOnce({ exited: true, elapsedMs: 0 }); try { const result = await stopDaemon({ paths }); @@ -146,7 +148,9 @@ test('does not send SIGKILL if the daemon identity changes during the graceful w const paths = createDaemonPaths(); mocks.isAgentDeviceDaemonProcess.mockReturnValueOnce(true).mockReturnValueOnce(false); mocks.trySignalProcess.mockReturnValue(true); - mocks.waitForProcessExit.mockResolvedValueOnce(false).mockResolvedValueOnce(true); + mocks.waitForDaemonExit + .mockResolvedValueOnce({ exited: false, elapsedMs: 0 }) + .mockResolvedValueOnce({ exited: true, elapsedMs: 0 }); try { await stopDaemon({ paths }); diff --git a/src/daemon/daemon-process.ts b/src/daemon/daemon-process.ts index 9e2facd69..0aea115ba 100644 --- a/src/daemon/daemon-process.ts +++ b/src/daemon/daemon-process.ts @@ -1,9 +1,10 @@ import { isProcessAlive, + readHostProcessIdentityObservations, readProcessCommand, readProcessStartTime, - waitForProcessExit, } from '../utils/host-process.ts'; +import { sleep } from '../utils/timeouts.ts'; const DAEMON_COMMAND_PATTERNS = [ /\/dist\/src\/daemon\.js($|[\s"'])/, @@ -48,6 +49,62 @@ export function trySignalProcess(pid: number, signal: NodeJS.Signals): boolean { } } +/** A daemon pinned to one process lifetime, never a bare pid. */ +export type DaemonProcessIdentity = { + pid: number; + startTime: string; +}; + +export type DaemonExitWait = { + /** The pid was released, or the host handed it to a different process. */ + exited: boolean; + elapsedMs: number; +}; + +const DAEMON_EXIT_POLL_MS = 100; + +type DaemonPidState = 'ours' | 'exiting' | 'released' | 'recycled'; + +function classifyDaemonPid(identity: DaemonProcessIdentity): DaemonPidState { + if (!isProcessAlive(identity.pid)) return 'released'; + // A terminated pid awaiting reap answers kill(pid, 0), keeps its start time, and + // reports its command as ``; only the process state distinguishes it. + const observed = readHostProcessIdentityObservations([identity.pid]).get(identity.pid); + if (!observed || observed.state.startsWith('Z')) return 'exiting'; + if (observed.startTime !== identity.startTime) return 'recycled'; + const command = readProcessCommand(identity.pid); + if (!command) return 'exiting'; + return isAgentDeviceDaemonCommand(command) ? 'ours' : 'recycled'; +} + +/** + * Resolves once `identity` has left the host — released or recycled. A pid still + * being torn down is neither, so the wait continues until the number is free. + */ +export async function waitForDaemonExit( + identity: DaemonProcessIdentity, + options: { timeoutMs: number; pollMs?: number }, +): Promise { + const startedAt = Date.now(); + const deadline = startedAt + options.timeoutMs; + const pollMs = options.pollMs ?? DAEMON_EXIT_POLL_MS; + const hasExited = (): boolean => { + const state = classifyDaemonPid(identity); + return state === 'released' || state === 'recycled'; + }; + let exited = hasExited(); + while (!exited && Date.now() < deadline) { + await sleep(pollMs); + exited = hasExited(); + } + return { exited, elapsedMs: Date.now() - startedAt }; +} + +function signalDaemonIdentity(identity: DaemonProcessIdentity, signal: NodeJS.Signals): boolean { + if (!isAgentDeviceDaemonProcess(identity.pid, identity.startTime)) return false; + return trySignalProcess(identity.pid, signal); +} + export async function stopProcessForTakeover( pid: number, options: { @@ -56,9 +113,10 @@ export async function stopProcessForTakeover( expectedStartTime: string | undefined; }, ): Promise { - if (!isAgentDeviceDaemonProcess(pid, options.expectedStartTime)) return; - if (!trySignalProcess(pid, 'SIGTERM')) return; - if (await waitForProcessExit(pid, options.termTimeoutMs)) return; - if (!trySignalProcess(pid, 'SIGKILL')) return; - await waitForProcessExit(pid, options.killTimeoutMs); + if (!options.expectedStartTime) return; + const identity: DaemonProcessIdentity = { pid, startTime: options.expectedStartTime }; + if (!signalDaemonIdentity(identity, 'SIGTERM')) return; + if ((await waitForDaemonExit(identity, { timeoutMs: options.termTimeoutMs })).exited) return; + if (!signalDaemonIdentity(identity, 'SIGKILL')) return; + await waitForDaemonExit(identity, { timeoutMs: options.killTimeoutMs }); } diff --git a/src/daemon/daemon-stop.ts b/src/daemon/daemon-stop.ts index ec6f7aaf5..ac9667bc5 100644 --- a/src/daemon/daemon-stop.ts +++ b/src/daemon/daemon-stop.ts @@ -1,7 +1,12 @@ import fs from 'node:fs'; import { AppError } from '@agent-device/kernel/errors'; -import { isAgentDeviceDaemonProcess, trySignalProcess } from './daemon-process.ts'; -import { isProcessAlive, waitForProcessExit } from '../utils/host-process.ts'; +import { + isAgentDeviceDaemonProcess, + trySignalProcess, + waitForDaemonExit, + type DaemonProcessIdentity, +} from './daemon-process.ts'; +import { isProcessAlive } from '../utils/host-process.ts'; import { sleep } from '../utils/timeouts.ts'; import type { DaemonPaths } from './config.ts'; import { readRegisteredDaemonIdentity } from './daemon-registration.ts'; @@ -56,11 +61,11 @@ export async function stopDaemon(params: { ); } + const identity: DaemonProcessIdentity = { pid: info.pid, startTime: info.startTime }; if (!signalDaemonProcess(info.pid, 'SIGTERM')) return notRunningResult(); - const graceful = await waitForProcessExit( - info.pid, - params.graceTimeoutMs ?? DAEMON_STOP_GRACE_TIMEOUT_MS, - ); + const { exited: graceful } = await waitForDaemonExit(identity, { + timeoutMs: params.graceTimeoutMs ?? DAEMON_STOP_GRACE_TIMEOUT_MS, + }); if (graceful) { await waitForDaemonMetadataRemoval(params.paths, DAEMON_STOP_METADATA_WAIT_MS); return { @@ -80,10 +85,9 @@ export async function stopDaemon(params: { if (isAgentDeviceDaemonProcess(info.pid, info.startTime)) { signalDaemonProcess(info.pid, 'SIGKILL'); } - const stopped = await waitForProcessExit( - info.pid, - params.killTimeoutMs ?? DAEMON_STOP_KILL_TIMEOUT_MS, - ); + const { exited: stopped } = await waitForDaemonExit(identity, { + timeoutMs: params.killTimeoutMs ?? DAEMON_STOP_KILL_TIMEOUT_MS, + }); if (!stopped) { throw new AppError('COMMAND_FAILED', 'Daemon did not exit after SIGKILL.', { pid: info.pid }); } diff --git a/test/integration/smoke-web-platform.test.ts b/test/integration/smoke-web-platform.test.ts index 13a5d4778..29a054f23 100644 --- a/test/integration/smoke-web-platform.test.ts +++ b/test/integration/smoke-web-platform.test.ts @@ -17,7 +17,11 @@ import { getManagedAgentBrowserStatus, type AgentBrowserToolStatus, } from '../../src/platforms/web/agent-browser-tool.ts'; -import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; +import { + stopProcessForTakeover, + waitForDaemonExit, + type DaemonProcessIdentity, +} from '../../src/daemon/daemon-process.ts'; import { expandProcessTree, isProcessAlive, @@ -40,11 +44,6 @@ const WEB_SHUTDOWN_CLEANUP_TIMEOUT_MS = 5_000; // actively closed the browser, not that agent-browser's own idle timer beat this test's own wait. const WEB_SHUTDOWN_IDLE_TIMEOUT_MS = WEB_SHUTDOWN_SETTLE_TIMEOUT_MS + 60_000; -type WebShutdownDaemonIdentity = { - pid: number; - startTime: string; -}; - test('web shutdown cleanup reaps the exact daemon that survived graceful shutdown', async (t) => { const root = mkdtempSync('/tmp/agent-device-web-shutdown-cleanup-'); const entryPath = path.join(root, 'src', 'daemon.ts'); @@ -168,7 +167,7 @@ async function runWebShutdownSmoke(context: WebSmokeContext): Promise { // Cleanup authority lives entirely in `finally`, driven by these two, so a failed assertion // above (including the very failure this test exists to catch: processes still alive when the // fix regresses) can never leave a daemon or a Chrome fleet running on the host afterward. - let daemonIdentity: WebShutdownDaemonIdentity | undefined; + let daemonIdentity: DaemonProcessIdentity | undefined; let status: AgentBrowserToolStatus | undefined; try { await runStep(context, 'set up managed web backend', ['web', 'setup', '--json']); @@ -194,16 +193,22 @@ async function runWebShutdownSmoke(context: WebSmokeContext): Promise { // orchestrator shutting the container down would, rather than going through `close`. process.kill(daemonPid, 'SIGTERM'); - const after = await settleManagedBrowserProcesses(status); + const [after, daemonExit] = await Promise.all([ + settleManagedBrowserProcesses(status), + waitForDaemonExit(daemonIdentity, { + timeoutMs: WEB_SHUTDOWN_SETTLE_TIMEOUT_MS, + pollMs: WEB_SHUTDOWN_SETTLE_POLL_MS, + }), + ]); assert.equal( after.count, 0, `expected zero owned Chrome processes after daemon shutdown, found: ${formatProcessSummary(after)}`, ); assert.equal( - isProcessAlive(daemonPid), - false, - 'expected the daemon process itself to have exited after SIGTERM', + daemonExit.exited, + true, + `expected the daemon process itself to have exited after SIGTERM, still alive ${daemonExit.elapsedMs}ms later`, ); // #1781 B1: the daemon leak oracle this fix unblocks wiring to a web lane — no stray @@ -222,7 +227,7 @@ async function runWebShutdownSmoke(context: WebSmokeContext): Promise { // alongside. async function cleanupWebShutdownSmoke( context: WebSmokeContext, - daemonIdentity: WebShutdownDaemonIdentity | undefined, + daemonIdentity: DaemonProcessIdentity | undefined, status: AgentBrowserToolStatus | undefined, timeouts = { termTimeoutMs: WEB_SHUTDOWN_CLEANUP_TIMEOUT_MS,