Skip to content

Commit 73ad6cf

Browse files
thymikeethymikee
committed
fix(daemon): own daemon-exit waiting as identity, not as a bare pid
Review feedback on #2083: the smoke lane had grown its own identity-aware poll beside the owner it already imported, and the owner itself was weaker than the copy. Add `waitForDaemonExit(identity, { timeoutMs, pollMs })` to daemon-process, the module that owns daemon lifecycle policy, and consume it from the web shutdown smoke, `stopProcessForTakeover` and `stopDaemon`. The test-local helper and its paragraph of rationale are gone; the invariant now lives in the interface. This closes a real defect in `stopProcessForTakeover`. It verified identity only before SIGTERM, then waited on the bare pid: a daemon that exited and had its pid reused read as "still running", and the takeover escalated SIGKILL onto whatever now held that number. Escalation is no longer a rule each call site remembers — `signalDaemonIdentity` re-reads identity immediately before every signal, so a recycled pid cannot be signaled at all. `stopDaemon` already re-verified before SIGKILL; it now also stops misreporting a recycled pid as "did not exit". `classifyDaemonPid` names the four states a pid can be in, including the one a bare liveness read cannot express: a process that has died but has not been reaped answers kill(pid, 0) and still reports its original start time, while ps shows its command as `<defunct>`. Reading that as a recycled pid would hand the number back while it is still taken, so it counts as neither ending and the wait keeps polling — preserving the guarantee callers had before. Deterministic coverage in daemon-exit-wait.test.ts: identity changing mid-wait, a zombie waited through to its reap, no SIGKILL after a recycled grace wait, and escalation still reached for a daemon that genuinely survives SIGTERM. Verified planted-red — reverting stopProcessForTakeover to the bare-pid wait fails the recycled-pid test alone, on its assertion, inside the unit lane's clock budget.
1 parent 554d6c5 commit 73ad6cf

5 files changed

Lines changed: 255 additions & 61 deletions

File tree

‎src/daemon/__tests__/daemon-exit-wait.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { afterEach, beforeEach, expect, test, vi } from 'vitest';
2+
import { stopProcessForTakeover, waitForDaemonExit } from '../daemon-process.ts';
3+
4+
const DAEMON_COMMAND = '/opt/checkout/dist/src/internal/daemon.js';
5+
const OURS = 'Mon Aug 24 10:00:00 2026';
6+
const RECYCLED = 'Mon Aug 24 10:00:07 2026';
7+
const PID = 4242;
8+
// Small enough that a regression that waits the budget out still lands inside the
9+
// unit lane's wall-clock gate and fails on its assertion rather than on the clock.
10+
const TIMEOUT_MS = 1_000;
11+
const POLL_MS = 5;
12+
13+
// A live process cannot be made to change identity mid-wait, so the host reads are
14+
// mocked: a recycled pid is the same live pid reporting a different start time,
15+
// which is what the host presents after reuse.
16+
const state = vi.hoisted(() => ({
17+
alive: new Map<number, boolean>(),
18+
starts: new Map<number, string>(),
19+
states: new Map<number, string>(),
20+
commands: new Map<number, string>(),
21+
}));
22+
23+
vi.mock('../../utils/host-process.ts', async (importOriginal) => ({
24+
...(await importOriginal<typeof import('../../utils/host-process.ts')>()),
25+
isProcessAlive: (pid: number) => state.alive.get(pid) ?? false,
26+
readProcessStartTime: (pid: number) => state.starts.get(pid) ?? null,
27+
readProcessCommand: (pid: number) => state.commands.get(pid) ?? null,
28+
readHostProcessIdentityObservations: (pids: Iterable<number>) => {
29+
const observations = new Map<number, { state: string; startTime: string }>();
30+
for (const pid of pids) {
31+
const startTime = state.starts.get(pid);
32+
if (startTime === undefined) continue;
33+
observations.set(pid, { state: state.states.get(pid) ?? 'S', startTime });
34+
}
35+
return observations;
36+
},
37+
}));
38+
39+
/** Delivered signals only; a `0` probe is a liveness read, not a write. */
40+
const signals: NodeJS.Signals[] = [];
41+
let onSignal: (signal: NodeJS.Signals) => void = () => {};
42+
43+
beforeEach(() => {
44+
state.alive.clear();
45+
state.starts.clear();
46+
state.states.clear();
47+
state.commands.clear();
48+
signals.length = 0;
49+
onSignal = () => {};
50+
state.alive.set(PID, true);
51+
state.starts.set(PID, OURS);
52+
state.states.set(PID, 'S');
53+
state.commands.set(PID, DAEMON_COMMAND);
54+
vi.spyOn(process, 'kill').mockImplementation(((pid: number, signal: NodeJS.Signals | 0) => {
55+
if (pid !== PID || signal === 0) return true;
56+
signals.push(signal);
57+
onSignal(signal);
58+
return true;
59+
}) as typeof process.kill);
60+
});
61+
62+
afterEach(() => {
63+
vi.restoreAllMocks();
64+
});
65+
66+
test('waitForDaemonExit reports a pid recycled mid-wait as exited, without burning the deadline', async () => {
67+
setTimeout(() => state.starts.set(PID, RECYCLED), 20);
68+
const wait = await waitForDaemonExit(
69+
{ pid: PID, startTime: OURS },
70+
{ timeoutMs: TIMEOUT_MS, pollMs: POLL_MS },
71+
);
72+
expect(wait.exited).toBe(true);
73+
expect(wait.elapsedMs).toBeLessThan(TIMEOUT_MS / 2);
74+
});
75+
76+
test('waitForDaemonExit reports a daemon that keeps its identity as not exited', async () => {
77+
const wait = await waitForDaemonExit(
78+
{ pid: PID, startTime: OURS },
79+
{ timeoutMs: 40, pollMs: POLL_MS },
80+
);
81+
expect(wait.exited).toBe(false);
82+
});
83+
84+
// A daemon killed by its own parent lingers as a zombie: kill(pid, 0) still succeeds
85+
// and the start time still matches, while ps reports the command as `<defunct>`.
86+
// Reading that as a recycled pid would hand the number back to callers while it is
87+
// still taken, so it must count as neither ending until the reap lands.
88+
test('waitForDaemonExit keeps waiting through a zombie until the pid is reaped', async () => {
89+
state.states.set(PID, 'Z+');
90+
state.commands.set(PID, '<defunct>');
91+
const stillTaken = await waitForDaemonExit(
92+
{ pid: PID, startTime: OURS },
93+
{ timeoutMs: 40, pollMs: POLL_MS },
94+
);
95+
expect(stillTaken.exited).toBe(false);
96+
97+
state.alive.set(PID, false);
98+
const reaped = await waitForDaemonExit(
99+
{ pid: PID, startTime: OURS },
100+
{ timeoutMs: TIMEOUT_MS, pollMs: POLL_MS },
101+
);
102+
expect(reaped.exited).toBe(true);
103+
});
104+
105+
// Planted red for the defect this pair exists to prevent: the grace wait used to poll
106+
// the bare pid, so a daemon that exited and had its pid reused read as "still running"
107+
// and the takeover escalated SIGKILL onto whatever now held that number.
108+
test('stopProcessForTakeover does not SIGKILL a pid recycled during the grace wait', async () => {
109+
onSignal = (signal) => {
110+
if (signal === 'SIGTERM') state.starts.set(PID, RECYCLED);
111+
};
112+
await stopProcessForTakeover(PID, {
113+
termTimeoutMs: TIMEOUT_MS,
114+
killTimeoutMs: 40,
115+
expectedStartTime: OURS,
116+
});
117+
expect(signals).toEqual(['SIGTERM']);
118+
});
119+
120+
test('stopProcessForTakeover still escalates to SIGKILL for a daemon that survives SIGTERM', async () => {
121+
onSignal = (signal) => {
122+
if (signal === 'SIGKILL') state.alive.set(PID, false);
123+
};
124+
await stopProcessForTakeover(PID, {
125+
termTimeoutMs: 40,
126+
killTimeoutMs: 40,
127+
expectedStartTime: OURS,
128+
});
129+
expect(signals).toEqual(['SIGTERM', 'SIGKILL']);
130+
});

‎src/daemon/__tests__/daemon-stop.test.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,16 @@ const mocks = vi.hoisted(() => ({
88
isProcessAlive: vi.fn(),
99
sleep: vi.fn(async () => undefined),
1010
trySignalProcess: vi.fn(),
11-
waitForProcessExit: vi.fn(),
11+
waitForDaemonExit: vi.fn(),
1212
}));
1313

1414
vi.mock('../daemon-process.ts', () => ({
1515
isAgentDeviceDaemonProcess: mocks.isAgentDeviceDaemonProcess,
1616
trySignalProcess: mocks.trySignalProcess,
17+
waitForDaemonExit: mocks.waitForDaemonExit,
1718
}));
1819
vi.mock('../../utils/host-process.ts', () => ({
1920
isProcessAlive: mocks.isProcessAlive,
20-
waitForProcessExit: mocks.waitForProcessExit,
2121
}));
2222
vi.mock('../../utils/timeouts.ts', () => ({ sleep: mocks.sleep }));
2323

@@ -102,9 +102,9 @@ test('reports graceful cleanup after SIGTERM exits the verified daemon', async (
102102
const paths = createDaemonPaths();
103103
mocks.isAgentDeviceDaemonProcess.mockReturnValue(true);
104104
mocks.trySignalProcess.mockReturnValue(true);
105-
mocks.waitForProcessExit.mockImplementation(async () => {
105+
mocks.waitForDaemonExit.mockImplementation(async () => {
106106
fs.rmSync(paths.infoPath, { force: true });
107-
return true;
107+
return { exited: true, elapsedMs: 0 };
108108
});
109109

110110
try {
@@ -125,7 +125,9 @@ test('re-verifies identity before SIGKILL and reports forced cleanup as unknown'
125125
const paths = createDaemonPaths();
126126
mocks.isAgentDeviceDaemonProcess.mockReturnValue(true);
127127
mocks.trySignalProcess.mockReturnValue(true);
128-
mocks.waitForProcessExit.mockResolvedValueOnce(false).mockResolvedValueOnce(true);
128+
mocks.waitForDaemonExit
129+
.mockResolvedValueOnce({ exited: false, elapsedMs: 0 })
130+
.mockResolvedValueOnce({ exited: true, elapsedMs: 0 });
129131

130132
try {
131133
const result = await stopDaemon({ paths });
@@ -146,7 +148,9 @@ test('does not send SIGKILL if the daemon identity changes during the graceful w
146148
const paths = createDaemonPaths();
147149
mocks.isAgentDeviceDaemonProcess.mockReturnValueOnce(true).mockReturnValueOnce(false);
148150
mocks.trySignalProcess.mockReturnValue(true);
149-
mocks.waitForProcessExit.mockResolvedValueOnce(false).mockResolvedValueOnce(true);
151+
mocks.waitForDaemonExit
152+
.mockResolvedValueOnce({ exited: false, elapsedMs: 0 })
153+
.mockResolvedValueOnce({ exited: true, elapsedMs: 0 });
150154

151155
try {
152156
await stopDaemon({ paths });

‎src/daemon/daemon-process.ts‎

Lines changed: 89 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
import {
22
isProcessAlive,
3+
readHostProcessIdentityObservations,
34
readProcessCommand,
45
readProcessStartTime,
5-
waitForProcessExit,
66
} from '../utils/host-process.ts';
7+
import { sleep } from '../utils/timeouts.ts';
78

89
const DAEMON_COMMAND_PATTERNS = [
910
/\/dist\/src\/daemon\.js($|[\s"'])/,
@@ -48,6 +49,87 @@ export function trySignalProcess(pid: number, signal: NodeJS.Signals): boolean {
4849
}
4950
}
5051

52+
/** A daemon pinned to one process lifetime, never a bare pid. */
53+
export type DaemonProcessIdentity = {
54+
pid: number;
55+
startTime: string;
56+
};
57+
58+
export type DaemonExitWait = {
59+
/** The pid no longer belongs to the identity: it exited, or the host recycled it. */
60+
exited: boolean;
61+
elapsedMs: number;
62+
};
63+
64+
/**
65+
* Poll for {@link waitForDaemonExit}. {@link classifyDaemonPid} reads `ps` only
66+
* once the cheap liveness check says the pid is still taken, so a daemon that has
67+
* already gone costs no subprocess at all.
68+
*/
69+
const DAEMON_EXIT_POLL_MS = 100;
70+
71+
/**
72+
* What a pid is doing relative to the identity that claimed it. `exiting` is the
73+
* state a bare liveness read cannot express: `kill(pid, 0)` still succeeds for a
74+
* process that has died but has not been reaped yet, while `ps` has already
75+
* dropped its row — a pid on its way out, not a pid handed to somebody else.
76+
*/
77+
type DaemonPidState = 'ours' | 'exiting' | 'released' | 'recycled';
78+
79+
function classifyDaemonPid(identity: DaemonProcessIdentity): DaemonPidState {
80+
if (!isProcessAlive(identity.pid)) return 'released';
81+
// State and start time in one `ps` read. A process that has died but has not
82+
// been reaped answers kill(pid, 0) AND still reports its original start time,
83+
// so the state is the only field that separates it from one still running —
84+
// and its command reads `<defunct>`, which would otherwise look like a pid
85+
// handed to a different program.
86+
const observed = readHostProcessIdentityObservations([identity.pid]).get(identity.pid);
87+
if (!observed || observed.state.startsWith('Z')) return 'exiting';
88+
if (observed.startTime !== identity.startTime) return 'recycled';
89+
const command = readProcessCommand(identity.pid);
90+
if (!command) return 'exiting';
91+
return isAgentDeviceDaemonCommand(command) ? 'ours' : 'recycled';
92+
}
93+
94+
/**
95+
* Waits for a daemon identity to leave the host. Two endings count as exited: the
96+
* pid was released, or the host handed it to someone else. Recycling is the one a
97+
* bare liveness wait gets wrong — it reports a stranger's pid as "still running",
98+
* which is what lets a grace wait escalate a SIGKILL onto an unrelated process —
99+
* so escalating callers must branch on this result, never on bare liveness. A pid
100+
* still being torn down is neither ending, so the wait keeps polling and callers
101+
* retain the old guarantee that the number is free before they act on it.
102+
*/
103+
export async function waitForDaemonExit(
104+
identity: DaemonProcessIdentity,
105+
options: { timeoutMs: number; pollMs?: number },
106+
): Promise<DaemonExitWait> {
107+
const startedAt = Date.now();
108+
const deadline = startedAt + options.timeoutMs;
109+
const pollMs = options.pollMs ?? DAEMON_EXIT_POLL_MS;
110+
const hasExited = (): boolean => {
111+
const state = classifyDaemonPid(identity);
112+
return state === 'released' || state === 'recycled';
113+
};
114+
let exited = hasExited();
115+
while (!exited && Date.now() < deadline) {
116+
await sleep(pollMs);
117+
exited = hasExited();
118+
}
119+
return { exited, elapsedMs: Date.now() - startedAt };
120+
}
121+
122+
/**
123+
* The only way this module signals a daemon: identity is re-read immediately
124+
* before the write, so a pid recycled since the last observation cannot be
125+
* signaled at all. Escalation safety is then a property of the call, not a rule
126+
* each call site has to remember.
127+
*/
128+
function signalDaemonIdentity(identity: DaemonProcessIdentity, signal: NodeJS.Signals): boolean {
129+
if (!isAgentDeviceDaemonProcess(identity.pid, identity.startTime)) return false;
130+
return trySignalProcess(identity.pid, signal);
131+
}
132+
51133
export async function stopProcessForTakeover(
52134
pid: number,
53135
options: {
@@ -56,9 +138,10 @@ export async function stopProcessForTakeover(
56138
expectedStartTime: string | undefined;
57139
},
58140
): Promise<void> {
59-
if (!isAgentDeviceDaemonProcess(pid, options.expectedStartTime)) return;
60-
if (!trySignalProcess(pid, 'SIGTERM')) return;
61-
if (await waitForProcessExit(pid, options.termTimeoutMs)) return;
62-
if (!trySignalProcess(pid, 'SIGKILL')) return;
63-
await waitForProcessExit(pid, options.killTimeoutMs);
141+
if (!options.expectedStartTime) return;
142+
const identity: DaemonProcessIdentity = { pid, startTime: options.expectedStartTime };
143+
if (!signalDaemonIdentity(identity, 'SIGTERM')) return;
144+
if ((await waitForDaemonExit(identity, { timeoutMs: options.termTimeoutMs })).exited) return;
145+
if (!signalDaemonIdentity(identity, 'SIGKILL')) return;
146+
await waitForDaemonExit(identity, { timeoutMs: options.killTimeoutMs });
64147
}

‎src/daemon/daemon-stop.ts‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
import fs from 'node:fs';
22
import { AppError } from '@agent-device/kernel/errors';
3-
import { isAgentDeviceDaemonProcess, trySignalProcess } from './daemon-process.ts';
4-
import { isProcessAlive, waitForProcessExit } from '../utils/host-process.ts';
3+
import {
4+
isAgentDeviceDaemonProcess,
5+
trySignalProcess,
6+
waitForDaemonExit,
7+
type DaemonProcessIdentity,
8+
} from './daemon-process.ts';
9+
import { isProcessAlive } from '../utils/host-process.ts';
510
import { sleep } from '../utils/timeouts.ts';
611
import type { DaemonPaths } from './config.ts';
712
import { readRegisteredDaemonIdentity } from './daemon-registration.ts';
@@ -56,11 +61,11 @@ export async function stopDaemon(params: {
5661
);
5762
}
5863

64+
const identity: DaemonProcessIdentity = { pid: info.pid, startTime: info.startTime };
5965
if (!signalDaemonProcess(info.pid, 'SIGTERM')) return notRunningResult();
60-
const graceful = await waitForProcessExit(
61-
info.pid,
62-
params.graceTimeoutMs ?? DAEMON_STOP_GRACE_TIMEOUT_MS,
63-
);
66+
const { exited: graceful } = await waitForDaemonExit(identity, {
67+
timeoutMs: params.graceTimeoutMs ?? DAEMON_STOP_GRACE_TIMEOUT_MS,
68+
});
6469
if (graceful) {
6570
await waitForDaemonMetadataRemoval(params.paths, DAEMON_STOP_METADATA_WAIT_MS);
6671
return {
@@ -80,10 +85,9 @@ export async function stopDaemon(params: {
8085
if (isAgentDeviceDaemonProcess(info.pid, info.startTime)) {
8186
signalDaemonProcess(info.pid, 'SIGKILL');
8287
}
83-
const stopped = await waitForProcessExit(
84-
info.pid,
85-
params.killTimeoutMs ?? DAEMON_STOP_KILL_TIMEOUT_MS,
86-
);
88+
const { exited: stopped } = await waitForDaemonExit(identity, {
89+
timeoutMs: params.killTimeoutMs ?? DAEMON_STOP_KILL_TIMEOUT_MS,
90+
});
8791
if (!stopped) {
8892
throw new AppError('COMMAND_FAILED', 'Daemon did not exit after SIGKILL.', { pid: info.pid });
8993
}

0 commit comments

Comments
 (0)