diff --git a/packages/host-kit/src/internal/owner-identity-liveness.test.ts b/packages/host-kit/src/internal/owner-identity-liveness.test.ts index fd5278afa8..772d29cac5 100644 --- a/packages/host-kit/src/internal/owner-identity-liveness.test.ts +++ b/packages/host-kit/src/internal/owner-identity-liveness.test.ts @@ -59,3 +59,11 @@ test('a missing entry in a completed process snapshot stays fail-closed without assert.equal(mockIsProcessZombie.mock.calls.length, 0); assert.equal(mockReadProcessStartTime.mock.calls.length, 0); }); + +test('a pid outside the native range is unknown without a liveness probe', () => { + assert.equal( + classifyOwnerLiveness({ owner: { pid: 2_147_483_648, startTime: 'start-a' } }), + 'unknown', + ); + assert.equal(mockIsProcessAlive.mock.calls.length, 0); +}); diff --git a/packages/host-kit/src/internal/owner-identity.ts b/packages/host-kit/src/internal/owner-identity.ts index a7b1352b35..552a030ea7 100644 --- a/packages/host-kit/src/internal/owner-identity.ts +++ b/packages/host-kit/src/internal/owner-identity.ts @@ -11,6 +11,11 @@ export type OwnerIdentity = { startTime: string | null; }; +/** A process id accepted by Node's native signal API. */ +export function isProcessPid(value: unknown): value is number { + return typeof value === 'number' && Number.isInteger(value) && value > 0 && value <= 0x7fff_ffff; +} + export type OwnerLiveness = | 'live' | 'owner-process-dead' @@ -75,6 +80,7 @@ export function classifyOwnerLivenessFromObservation( observation?: HostProcessIdentityObservation | null, ): OwnerLiveness { const { owner, stateDir } = params; + if (!isProcessPid(owner.pid)) return 'unknown'; if (!isProcessAlive(owner.pid)) return 'owner-process-dead'; if (observation !== undefined ? observation?.state.startsWith('Z') : isProcessZombie(owner.pid)) { return 'owner-process-dead'; @@ -88,7 +94,10 @@ export function classifyOwnerLivenessFromObservation( return 'owner-process-reused'; } } - if (!stateDir) return 'live'; + return stateDir ? classifyOwnerStateDirectory(stateDir) : 'live'; +} + +function classifyOwnerStateDirectory(stateDir: string): OwnerLiveness { try { fs.statSync(stateDir); return 'live'; diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index 4ca663e6d9..e0f795c599 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -6,6 +6,7 @@ import { publishFileSync } from './atomic-file.ts'; import { emitDiagnostic } from './diagnostics.ts'; import { classifyOwnerLiveness, + isProcessPid, ownerIdentityMatches, type OwnerLiveness, } from './owner-identity.ts'; @@ -439,7 +440,7 @@ function parseProcessLockOwner(contents: string): ProcessLockOwnerRecord | null const PROCESS_LOCK_OWNER_FIELD_SHAPES: Record boolean> = { - pid: (value) => typeof value === 'number' && Number.isInteger(value) && value > 0, + pid: isProcessPid, acquiredAtMs: (value) => typeof value === 'number' && Number.isFinite(value), startTime: (value) => value === undefined || value === null || typeof value === 'string', }; diff --git a/packages/host-kit/src/process.ts b/packages/host-kit/src/process.ts index 2ed4c2c06d..6c061dd11f 100644 --- a/packages/host-kit/src/process.ts +++ b/packages/host-kit/src/process.ts @@ -36,6 +36,7 @@ export { export { classifyOwnerLiveness, classifyOwnerLivenessFromObservation, + isProcessPid, type OwnerIdentity, ownerIdentityDiffers, ownerIdentityMatches, diff --git a/src/__tests__/daemon-exit-wait.test.ts b/src/__tests__/daemon-exit-wait.test.ts index 2111bc2f3b..f2840761a1 100644 --- a/src/__tests__/daemon-exit-wait.test.ts +++ b/src/__tests__/daemon-exit-wait.test.ts @@ -57,6 +57,16 @@ afterEach(() => { vi.restoreAllMocks(); }); +test.each([0, -1, 1.5, 2_147_483_648, Number.MAX_SAFE_INTEGER])( + 'an invalid native pid %s cannot prove exit during recovery', + async (pid) => { + expect(await waitForDaemonExit({ pid, startTime: OURS }, { timeoutMs: 0 })).toEqual({ + exited: false, + elapsedMs: 0, + }); + }, +); + 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( diff --git a/src/__tests__/daemon-process-takeover.test.ts b/src/__tests__/daemon-process-takeover.test.ts index 78156ffc64..99292988f6 100644 --- a/src/__tests__/daemon-process-takeover.test.ts +++ b/src/__tests__/daemon-process-takeover.test.ts @@ -12,6 +12,19 @@ const TAKEOVER_TIMEOUTS = { termTimeoutMs: 5_000, killTimeoutMs: 2_000 }; const spawnedChildren: { child: ChildProcess; exited: Promise }[] = []; const spawnedRoots: string[] = []; +test.each([0, -1, 1.5, Number.NaN, '123', 2_147_483_648, Number.MAX_SAFE_INTEGER])( + 'an invalid daemon pid %s cannot prove exit', + async (pid) => { + assert.deepEqual( + await stopDaemonProcess( + { pid: pid as number, startTime: 'captured-birth' }, + { mode: 'force', termTimeoutMs: 0, killTimeoutMs: 0 }, + ), + { status: 'retained', reason: 'identity-unverified' }, + ); + }, +); + afterEach(async () => { for (const { child, exited } of spawnedChildren.splice(0)) { if (child.exitCode === null && child.signalCode === null) child.kill('SIGKILL'); diff --git a/src/__tests__/test-utils/daemon-http-fixture.ts b/src/__tests__/test-utils/daemon-http-fixture.ts index 4712a049c6..485bba4e7e 100644 --- a/src/__tests__/test-utils/daemon-http-fixture.ts +++ b/src/__tests__/test-utils/daemon-http-fixture.ts @@ -16,6 +16,7 @@ export type HttpDaemonFixture = { export async function startHttpDaemonFixture( responseData: Record, + options: { ready?: () => boolean } = {}, ): Promise { const seenPaths: string[] = []; const rpcRequests: Record[] = []; @@ -24,7 +25,7 @@ export async function startHttpDaemonFixture( seenPaths.push(`${req.method ?? 'GET'} ${url.pathname}`); if (req.method === 'GET' && url.pathname === '/health') { - res.writeHead(200); + res.writeHead(options.ready?.() === false ? 503 : 200); res.end('ok'); return; } diff --git a/src/__tests__/test-utils/registered-daemon-fixture.ts b/src/__tests__/test-utils/registered-daemon-fixture.ts index ea80ae08b6..6c99455a4e 100644 --- a/src/__tests__/test-utils/registered-daemon-fixture.ts +++ b/src/__tests__/test-utils/registered-daemon-fixture.ts @@ -2,7 +2,8 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { vi } from 'vitest'; -import type { runCmdDetachedMonitored } from '@agent-device/host-kit/command'; +import type { runCmdDetachedMonitored, ExecDetachedExit } from '@agent-device/host-kit/command'; +import { readDaemonInfo, type DaemonInfo } from '../../daemon-client/daemon-client-metadata.ts'; import { readProcessStartTime } from '@agent-device/host-kit/process'; import { stopDaemonProcess } from '../../daemon-process.ts'; import type { DaemonPaths } from '../../daemon-resolution.ts'; @@ -76,6 +77,24 @@ export function spawnRegisteredDaemonFixture( return child; } +export async function waitForRegisteredDaemonFixture( + paths: DaemonPaths, + child: ReturnType, +): Promise { + let exit: ExecDetachedExit | undefined; + void child.exited.then((result) => { + exit = result; + }); + for (let attempt = 0; attempt < 400; attempt += 1) { + if (exit) + throw new Error(`Registered child exited before publication: ${JSON.stringify(exit)}`); + const info = readDaemonInfo(paths.infoPath); + if (info?.pid === child.pid) return info; + await new Promise((resolve) => setTimeout(resolve, 10)); + } + throw new Error(`Registered child ${child.pid} did not publish ${paths.infoPath} within 4s`); +} + export async function finishRegisteredDaemonFixture(stateDir: string): Promise { for (const owned of children.get(stateDir) ?? []) { const child = owned.launch; diff --git a/src/daemon-client/__tests__/daemon-client-lifecycle.test.ts b/src/daemon-client/__tests__/daemon-client-lifecycle.test.ts index 487df065d2..9c67e88a31 100644 --- a/src/daemon-client/__tests__/daemon-client-lifecycle.test.ts +++ b/src/daemon-client/__tests__/daemon-client-lifecycle.test.ts @@ -8,6 +8,7 @@ import { afterEach, test, vi } from 'vitest'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { spawnRegisteredDaemonFixture, + waitForRegisteredDaemonFixture, finishRegisteredDaemonFixture, finishRegisteredDaemonFixtures, } from '../../__tests__/test-utils/registered-daemon-fixture.ts'; @@ -27,7 +28,6 @@ import { resolveDaemonPaths, type DaemonPaths } from '../../daemon-resolution.ts import { sendToDaemon, type DaemonRequest, type DaemonResponse } from '../daemon-client.ts'; import { attachActiveSessionAddressHint } from '../daemon-client-lifecycle.ts'; import { sendRequest } from '../daemon-client-transport.ts'; -import { readDaemonInfo } from '../daemon-client-metadata.ts'; import type { DaemonRetirementResult } from '../../daemon-registration-owner.ts'; import { closeLoopbackServer, @@ -646,12 +646,7 @@ test('sendRequest timeout cleanup uses resolved daemon paths instead of request }; try { - let info = readDaemonInfo(daemonPaths.infoPath); - for (let attempt = 0; !info && attempt < 200; attempt += 1) { - await actualRetry.sleep(10); - info = readDaemonInfo(daemonPaths.infoPath); - } - assert.ok(info); + const info = await waitForRegisteredDaemonFixture(daemonPaths, child); let thrown: unknown; try { await sendRequest(info, request, 'http', daemonPaths, 50); diff --git a/src/daemon-client/__tests__/daemon-client-startup-race.test.ts b/src/daemon-client/__tests__/daemon-client-startup-race.test.ts index d5a2b7ede4..8788a8c887 100644 --- a/src/daemon-client/__tests__/daemon-client-startup-race.test.ts +++ b/src/daemon-client/__tests__/daemon-client-startup-race.test.ts @@ -203,6 +203,46 @@ for (const exit of [ }); } +test('a joined busy contender waits for a published winner to become ready without signaling it', async (t) => { + if (!(await supportsLoopbackBind())) return t.skip('loopback unavailable'); + const paths = resolveDaemonPaths(mkdtempForTestSync('daemon-start-delayed-ready-')); + let probes = 0; + const http = await startHttpDaemonFixture({ devices: [] }, { ready: () => ++probes >= 12 }); + const deferred = path.join(paths.baseDir, 'defer-publication'); + fs.writeFileSync(deferred, 'wait'); + const winner = spawnRegisteredDaemonFixture(paths, fields(http.port), { stdio: 'ignore' }); + await awaitFile(path.join(paths.baseDir, 'registration-held')); + let joined = false; + spawn.mockImplementation((_command, _args, options) => { + const child = spawnRegisteredDaemonFixture(paths, fields(http.port), options); + void child.exited.then((exit) => { + assert.equal(exit.exitCode, DAEMON_STARTUP_EXIT_CODES.busy); + joined = true; + }); + return child; + }); + pause.mockImplementation(async () => { + if (joined) fs.rmSync(deferred, { force: true }); + await actualRetry.sleep(10); + }); + const signal = vi.spyOn(process, 'kill'); + try { + assert.equal((await sendToDaemon(request(paths))).ok, true); + assert.equal(joined, true); + assert.ok(probes >= 12); + assert.equal(spawn.mock.calls.length, 1); + assert.equal(http.rpcRequests.length, 1); + assert.equal(isProcessAlive(winner.pid), true); + assert.equal( + signal.mock.calls.some(([pid, kind]) => pid === winner.pid && kind !== 0), + false, + ); + } finally { + signal.mockRestore(); + await closeLoopbackServer(http.server); + } +}); + for (const held of [true, false]) { test(`startup uses one deadline when the claim is ${held ? 'held' : 'released for relaunch'}`, async () => { const paths = resolveDaemonPaths(mkdtempForTestSync('daemon-start-budget-')); @@ -215,6 +255,7 @@ for (const held of [true, false]) { let now = Date.now(); const started = now; let released = false; + let advanced = false; let finishPending: () => void = () => {}; const nativeTimeout = globalThis.setTimeout; vi.spyOn(globalThis, 'setTimeout').mockImplementation((handler, ms, ...args) => @@ -231,9 +272,12 @@ for (const held of [true, false]) { }), })); pause.mockImplementation(async (ms) => { - if (!held && !released) { - await claim.acquisition.release(); - released = true; + if (!advanced) { + if (!held) { + await claim.acquisition.release(); + released = true; + } + advanced = true; now += 14_750; } else now += ms; }); @@ -291,7 +335,14 @@ test.for([ const notice = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); try { const pending = sendToDaemon(request(paths)); - if (expected.alive) await assert.rejects(pending); + if (expected.alive) + await assert.rejects(pending, (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.kind, 'daemon_startup_failed'); + assert.equal(error.details?.startupAttempts, 1); + assert.equal(error.details?.startupTimeoutMs, 15_000); + return true; + }); else { assert.equal((await pending).ok, true); await winner.exited; diff --git a/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts b/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts index 04d4154403..c28d397d1d 100644 --- a/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts +++ b/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts @@ -41,14 +41,16 @@ vi.mock('@agent-device/host-kit/command', async () => { import { AppError, normalizeError } from '@agent-device/kernel/errors'; import { sleep } from '@agent-device/host-kit/retry'; +import { withDiagnosticsScope } from '@agent-device/host-kit/diagnostics'; import { sendRequest } from '../daemon-client-transport.ts'; import type { DaemonRequest } from '../../daemon/daemon-request.ts'; -import { readDaemonInfo, type DaemonInfo } from '../daemon-client-metadata.ts'; +import type { DaemonInfo } from '../daemon-client-metadata.ts'; import { resolveDaemonPaths, type DaemonPaths } from '../../daemon-resolution.ts'; import type { DaemonRetirementResult } from '../../daemon-registration-owner.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { spawnRegisteredDaemonFixture, + waitForRegisteredDaemonFixture, finishRegisteredDaemonFixture, finishRegisteredDaemonFixtures, } from '../../__tests__/test-utils/registered-daemon-fixture.ts'; @@ -284,13 +286,29 @@ test('remote HTTP timeout never runs the Apple pkill cleanup and uses the remote assert.equal(mockRunCmdSync.mock.calls.length, 0); }); -async function publishedInfo(paths: DaemonPaths): Promise { - for (let attempt = 0; attempt < 200; attempt += 1) { - const info = readDaemonInfo(paths.infoPath); - if (info) return info; - await sleep(10); +async function waitForForceStop(requested: Promise): Promise { + let timer: ReturnType | undefined; + try { + await Promise.race([ + requested, + new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new Error('force stop was not requested')), 1_500); + }), + ]); + } finally { + clearTimeout(timer); } - throw new Error('registered child did not publish'); +} + +function timeoutDiagnostic(paths: DaemonPaths): Record { + const events = fs + .readFileSync(path.join(paths.baseDir, 'timeout-diagnostics.ndjson'), 'utf8') + .trim() + .split('\n') + .map((line) => JSON.parse(line)); + const event = events.find((entry) => entry.phase === 'daemon_request_timeout'); + assert.ok(event); + return event.data; } for (const transport of ['socket', 'http'] as const) { @@ -331,13 +349,17 @@ for (const transport of ['socket', 'http'] as const) { return actualKill(pid, signal); }); try { - const info = await publishedInfo(paths); - outcome = sendRequest( - info, - { ...buildRequest(undefined), command: 'open' }, - transport, - paths, - TIMEOUT_MS, + const info = await waitForRegisteredDaemonFixture(paths, child); + outcome = withDiagnosticsScope( + { debug: true, logPath: path.join(paths.baseDir, 'timeout-diagnostics.ndjson') }, + () => + sendRequest( + info, + { ...buildRequest(undefined), command: 'open' }, + transport, + paths, + TIMEOUT_MS, + ), ).then( () => assert.fail('hanging request unexpectedly succeeded'), (error: unknown) => { @@ -345,10 +367,7 @@ for (const transport of ['socket', 'http'] as const) { return error; }, ); - await Promise.race([ - requested, - sleep(1_500).then(() => assert.fail('force stop was not requested')), - ]); + await waitForForceStop(requested); await sleep(30); assert.equal(actualKill(child.pid, 0), true); assert.equal(settled, false, 'request must remain pending while the daemon is alive'); @@ -364,6 +383,7 @@ for (const transport of ['socket', 'http'] as const) { assert.equal(fs.existsSync(paths.infoPath), false); assert.equal(fs.existsSync(paths.lockPath), false); assert.equal(fs.existsSync(paths.baseDir), true); + assert.equal(timeoutDiagnostic(paths).daemonPreservedAfterTimeout, false); assert.equal(mockRunCmdSync.mock.calls.filter(([cmd]) => cmd === 'pkill').length, 3); } finally { kill.mockRestore(); @@ -394,18 +414,22 @@ test('timeout retains a live registration without captured birth proof and repor ); const kill = vi.spyOn(process, 'kill'); try { - const info = await publishedInfo(paths); + const info = await waitForRegisteredDaemonFixture(paths, child); const before = fs.readFileSync(paths.infoPath, 'utf8'); const lockBefore = fs .readdirSync(paths.lockPath) .map((name) => [name, fs.readFileSync(path.join(paths.lockPath, name), 'utf8')]); await assert.rejects( - sendRequest( - { ...info, processStartTime: undefined }, - { ...buildRequest(undefined), command: 'open' }, - 'http', - paths, - TIMEOUT_MS, + withDiagnosticsScope( + { debug: true, logPath: path.join(paths.baseDir, 'timeout-diagnostics.ndjson') }, + () => + sendRequest( + { ...info, processStartTime: undefined }, + { ...buildRequest(undefined), command: 'open' }, + 'http', + paths, + TIMEOUT_MS, + ), ), (error: unknown) => { assert.ok(error instanceof AppError); @@ -419,6 +443,7 @@ test('timeout retains a live registration without captured birth proof and repor return true; }, ); + assert.equal(timeoutDiagnostic(paths).daemonPreservedAfterTimeout, true); assert.equal(process.kill(child.pid, 0), true); assert.equal(fs.readFileSync(paths.infoPath, 'utf8'), before); assert.deepEqual( diff --git a/src/daemon-client/daemon-client-lifecycle.ts b/src/daemon-client/daemon-client-lifecycle.ts index 7b25dd630c..1d76d43ecf 100644 --- a/src/daemon-client/daemon-client-lifecycle.ts +++ b/src/daemon-client/daemon-client-lifecycle.ts @@ -190,13 +190,20 @@ async function ensureRemoteDaemon(settings: DaemonClientSettings): Promise { + const { deadline } = options; const inspection = inspectProcessLock(settings.paths.lockPath); if (inspection.state === 'unproven') throw daemonRegistrationUnprovenError(settings, inspection); const existing = readDaemonInfo(settings.paths.infoPath); if (!existing) return null; if (!registrationAllowsDaemonObservation(inspection, existing)) return null; + if ( + options.waitForLiveStartup && + isProcessAlive(existing.pid) && + !(await canConnect(existing, 'auto', remainingStartupBudget(deadline))) + ) + return null; const decision = await resolveDaemonTakeover(existing, { onClientTransport: () => @@ -769,7 +776,7 @@ async function observeContendingDaemon( ): Promise { let winner: DaemonInfo | null; try { - winner = await readReusableLocalDaemon(settings, deadline); + winner = await readReusableLocalDaemon(settings, { deadline, waitForLiveStartup: true }); } catch (error) { if (error instanceof AppError && error.details?.reason === 'daemon_registration_unproven') return { kind: 'unproven', error }; diff --git a/src/daemon-client/daemon-client-metadata.ts b/src/daemon-client/daemon-client-metadata.ts index d517da18e4..718ed7b1cc 100644 --- a/src/daemon-client/daemon-client-metadata.ts +++ b/src/daemon-client/daemon-client-metadata.ts @@ -1,4 +1,5 @@ import fs from 'node:fs'; +import { isProcessPid } from '@agent-device/host-kit/process'; import type { DaemonCodeOrigin } from '@agent-device/host-kit/code-signature'; @@ -42,7 +43,7 @@ export function readDaemonInfo(infoPath: string): DaemonInfo | null { token, ...ports, transport: readDaemonInfoTransport(parsed.transport), - pid: readPositiveInteger(parsed.pid) ?? 0, + pid: isProcessPid(parsed.pid) ? parsed.pid : 0, version: readOptionalString(parsed.version), codeOrigin: readDaemonInfoCodeOrigin(parsed.codeOrigin), codeSignature: readOptionalString(parsed.codeSignature), diff --git a/src/daemon-client/daemon-client-timeout.ts b/src/daemon-client/daemon-client-timeout.ts index c27c95b1b7..f7132c4f87 100644 --- a/src/daemon-client/daemon-client-timeout.ts +++ b/src/daemon-client/daemon-client-timeout.ts @@ -73,10 +73,7 @@ export async function handleRequestTimeout( mode: 'force', }); } - const forcedKill = - retirement?.status !== 'absent' && - retirement?.termination?.status === 'exited' && - retirement.termination.mode === 'forced'; + const retained = retirement?.status === 'retained'; // The HINT, unlike cleanup, may only name Apple-runner involvement on // evidence this call site actually has: an explicitly declared Apple // platform selector, or the cleanup itself having terminated a matching @@ -95,9 +92,9 @@ export async function handleRequestTimeout( timedOutRunnerPidsTerminated: cleanup.terminated, timedOutRunnerCleanupError: cleanup.error, daemonPidReset: retirement?.status === 'retired' ? info.pid : undefined, - daemonPidForceKilled: resetDaemon ? forcedKill : undefined, + daemonPidForceKilled: resetDaemon ? daemonWasForceKilled(retirement) : undefined, daemonRetirement: retirement, - daemonPreservedAfterTimeout: !remote && !resetDaemon, + daemonPreservedAfterTimeout: retained || (!remote && !resetDaemon), daemonBaseUrl: info.baseUrl, }, }); @@ -120,6 +117,12 @@ export async function handleRequestTimeout( }); } +function daemonWasForceKilled(retirement: DaemonRetirementResult | undefined): boolean { + if (!retirement || retirement.status === 'absent') return false; + const termination = retirement.termination; + return termination?.status === 'exited' && termination.mode === 'forced'; +} + // Whether a timed-out request tears down the local daemon is declared on the // command's descriptor (ADR 0008, `timeoutPolicy.onTimeout`): read-only // capture/polling commands preserve the daemon so sessions survive and evidence diff --git a/src/daemon-process.ts b/src/daemon-process.ts index fee88d3622..28869a01b5 100644 --- a/src/daemon-process.ts +++ b/src/daemon-process.ts @@ -1,5 +1,6 @@ import { isProcessAlive, + isProcessPid, readHostProcessIdentityObservations, readProcessCommand, readProcessStartTime, @@ -72,6 +73,7 @@ export async function waitForDaemonExit( identity: DaemonProcessIdentity, options: { timeoutMs: number; pollMs?: number }, ): Promise { + if (!isProcessPid(identity.pid)) return { exited: false, elapsedMs: 0 }; const startedAt = Date.now(); const deadline = startedAt + options.timeoutMs; const pollMs = options.pollMs ?? DAEMON_EXIT_POLL_MS; @@ -114,6 +116,7 @@ export async function stopDaemonProcess( killTimeoutMs: number; }, ): Promise { + if (!isProcessPid(observed.pid)) return { status: 'retained', reason: 'identity-unverified' }; if (!observed.startTime?.trim()) { if (!isProcessAlive(observed.pid)) return { status: 'not-running' }; return { status: 'retained', reason: 'missing-start-time' }; diff --git a/src/daemon-registration.ts b/src/daemon-registration.ts index ad598afc3c..22f892971b 100644 --- a/src/daemon-registration.ts +++ b/src/daemon-registration.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import { ownerIdentityDiffers, + isProcessPid, ownerIdentityMatches, type OwnerIdentity, } from '@agent-device/host-kit/process'; @@ -53,7 +54,7 @@ function parseRegistration(parsed: { processStartTime?: unknown; }): ParsedRegistration { const pid = parsed.pid; - if (typeof pid !== 'number' || !Number.isInteger(pid) || pid <= 0) { + if (!isProcessPid(pid)) { return { pid: null, startTime: null }; } return { pid, startTime: readableStartTime(parsed.processStartTime) }; diff --git a/src/daemon/daemon-stop.ts b/src/daemon/daemon-stop.ts index 0f1ae4e1d0..c5e53eefcf 100644 --- a/src/daemon/daemon-stop.ts +++ b/src/daemon/daemon-stop.ts @@ -1,5 +1,5 @@ import { AppError } from '@agent-device/kernel/errors'; -import { stopAndRetireDaemon, type DaemonRetirementResult } from '../daemon-registration-owner.ts'; +import type { DaemonRetirementResult } from '../daemon-registration-owner.ts'; import type { OwnerIdentity } from '@agent-device/host-kit/process'; import type { DaemonPaths } from '../daemon-resolution.ts'; @@ -43,6 +43,7 @@ export async function stopDaemon(params: { }): Promise { const info = readRegisteredDaemonIdentity(params.paths.infoPath); if (!info) return notRunningResult(); + const { stopAndRetireDaemon } = await import('../daemon-registration-owner.ts'); const retirement = await stopAndRetireDaemon({ paths: params.paths, observed: info, diff --git a/src/daemon/handlers/__tests__/session-device-claims.test.ts b/src/daemon/handlers/__tests__/session-device-claims.test.ts index d45941b2c1..bb60b6ec2f 100644 --- a/src/daemon/handlers/__tests__/session-device-claims.test.ts +++ b/src/daemon/handlers/__tests__/session-device-claims.test.ts @@ -75,7 +75,6 @@ const mockResolveTargetDevice = vi.mocked(resolveTargetDevice); const mockEnsureDeviceReady = vi.mocked(ensureDeviceReady); const mockApplyRuntimeHints = vi.mocked(applyRuntimeHintValues); const mockResolveAndroidPackage = vi.mocked(resolveAndroidPackageForOpen); -const roots: string[] = []; const reconcileOrphanedDeviceClaim = async () => ({ status: 'retained' as const, reason: 'test-no-recovery', @@ -115,14 +114,12 @@ afterEach(() => { mockApplyRuntimeHints.mockResolvedValue(undefined); mockResolveAndroidPackage.mockResolvedValue(undefined); delete process.env.AGENT_DEVICE_CLAIMS_DIR; - for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true }); }); function setup(): { store: SessionStore; stateDir: string } { const stateDir = mkdtempForTestSync('agent-device-session-device-claim-'); const claimsDir = path.join(stateDir, 'claims'); process.env.AGENT_DEVICE_CLAIMS_DIR = claimsDir; - roots.push(stateDir); return { store: new SessionStore(path.join(stateDir, 'sessions')), stateDir }; } diff --git a/test/integration/support/daemon-test-cleanup.test.ts b/test/integration/support/daemon-test-cleanup.test.ts new file mode 100644 index 0000000000..a83780c9d5 --- /dev/null +++ b/test/integration/support/daemon-test-cleanup.test.ts @@ -0,0 +1,77 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import { test, vi } from 'vitest'; +import { + isProcessAlive, + readHostProcessIdentityObservations, +} from '@agent-device/host-kit/process'; +import { cleanupDaemonTestState } from './daemon-test-cleanup.ts'; +import { resolveDaemonPaths } from '../../../src/daemon-resolution.ts'; +import { stopDaemonProcess } from '../../../src/daemon-process.ts'; +import { mkdtempForTestSync } from '../../../src/__tests__/test-utils/tmp-dir.ts'; +import { + spawnRegisteredDaemonFixture, + waitForRegisteredDaemonFixture, + finishRegisteredDaemonFixture, +} from '../../../src/__tests__/test-utils/registered-daemon-fixture.ts'; + +const fields = { + httpPort: 4210, + token: 'fixture', + version: 'test', + codeOrigin: 'checkout' as const, + codeSignature: 'fixture', +}; + +test.each(['string', 'out-of-range'])( + 'malformed %s registration retains the directory and live daemon', + async (kind) => { + const paths = resolveDaemonPaths(mkdtempForTestSync('daemon-test-invalid-registration-')); + const child = spawnRegisteredDaemonFixture(paths, fields, undefined); + const warnings = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const info = await waitForRegisteredDaemonFixture(paths, child); + const malformed = JSON.stringify({ + ...info, + pid: kind === 'string' ? String(info.pid) : 2_147_483_648, + }); + fs.writeFileSync(paths.infoPath, malformed); + await cleanupDaemonTestState(paths.baseDir, null); + assert.equal(fs.readFileSync(paths.infoPath, 'utf8'), malformed); + assert.equal(isProcessAlive(child.pid), true); + assert.equal(warnings.mock.calls.length, 1); + } finally { + warnings.mockRestore(); + await finishRegisteredDaemonFixture(paths.baseDir); + } + }, +); + +test('cleanup stops the registered replacement when its observation still names the exited original', async () => { + const paths = resolveDaemonPaths(mkdtempForTestSync('daemon-test-replaced-registration-')); + const original = spawnRegisteredDaemonFixture(paths, fields, undefined); + try { + const observed = await waitForRegisteredDaemonFixture(paths, original); + const stopped = await stopDaemonProcess( + { pid: original.pid, startTime: observed.processStartTime ?? null }, + { mode: 'force', termTimeoutMs: 0, killTimeoutMs: 1_000 }, + ); + assert.equal(stopped.status, 'exited'); + await original.exited; + const replacement = spawnRegisteredDaemonFixture(paths, fields, undefined); + await waitForRegisteredDaemonFixture(paths, replacement); + await cleanupDaemonTestState(paths.baseDir, observed); + assert.ok( + !isProcessAlive(replacement.pid) || + readHostProcessIdentityObservations([replacement.pid]) + .get(replacement.pid) + ?.state.startsWith('Z'), + 'the registered replacement must be dead before cleanup returns', + ); + await replacement.exited; + assert.equal(isProcessAlive(replacement.pid), false); + assert.equal(fs.existsSync(paths.baseDir), false); + } finally { + await finishRegisteredDaemonFixture(paths.baseDir); + } +}); diff --git a/test/integration/support/daemon-test-cleanup.ts b/test/integration/support/daemon-test-cleanup.ts index 371159c521..848a90d450 100644 --- a/test/integration/support/daemon-test-cleanup.ts +++ b/test/integration/support/daemon-test-cleanup.ts @@ -2,6 +2,11 @@ import fs from 'node:fs'; import path from 'node:path'; import { normalizeError } from '@agent-device/kernel/errors'; import { stopDaemonProcess } from '../../../src/daemon-process.ts'; +import { + readRegisteredDaemonIdentity, + readRegisteredDaemonOwnership, +} from '../../../src/daemon-registration.ts'; +import { ownerIdentityMatches, type OwnerIdentity } from '@agent-device/host-kit/process'; type TestDaemonIdentity = { pid: number; processStartTime?: string }; @@ -11,29 +16,35 @@ export async function cleanupDaemonTestState( observed: TestDaemonIdentity | null, ): Promise { try { - const identity = observed ?? readIdentity(stateDir); - if (!identity) throw new Error('No daemon lifetime was observed'); - const termination = await stopDaemonProcess( - { pid: identity.pid, startTime: identity.processStartTime ?? null }, - { mode: 'graceful', termTimeoutMs: 1_500, killTimeoutMs: 1_500 }, - ); - if (termination.status !== 'exited') { - console.warn('Daemon test cleanup retained state:', stateDir, termination); - return; + const identities = [ + observed ? { pid: observed.pid, startTime: observed.processStartTime ?? null } : null, + readIdentity(stateDir), + ].filter((identity): identity is OwnerIdentity => identity !== null); + if (identities.length === 0) throw new Error('No daemon lifetime was observed'); + for (const identity of identities) { + const termination = await stopDaemonProcess(identity, { + mode: 'graceful', + termTimeoutMs: 1_500, + killTimeoutMs: 1_500, + }); + if (termination.status !== 'exited') { + console.warn('Daemon test cleanup retained state:', stateDir, termination); + return; + } } + const current = readIdentity(stateDir); + if (current && !identities.some((identity) => ownerIdentityMatches(identity, current))) + throw new Error('Daemon registration changed during test cleanup'); fs.rmSync(stateDir, { recursive: true, force: true }); } catch (error) { console.warn('Daemon test cleanup retained state:', stateDir, normalizeError(error)); } } -function readIdentity(stateDir: string): TestDaemonIdentity | null { - try { - return JSON.parse( - fs.readFileSync(path.join(stateDir, 'daemon.json'), 'utf8'), - ) as TestDaemonIdentity; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; - throw error; - } +function readIdentity(stateDir: string): OwnerIdentity | null { + const infoPath = path.join(stateDir, 'daemon.json'); + if (readRegisteredDaemonOwnership(infoPath, null).state === 'absent') return null; + const identity = readRegisteredDaemonIdentity(infoPath); + if (!identity) throw new Error('Daemon registration identity is invalid or unreadable'); + return identity; } diff --git a/vitest.config.ts b/vitest.config.ts index 1b4390fc19..bb9a0a0046 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -178,6 +178,7 @@ export default defineConfig({ // decisions over fixture state-dir listings, so they need no daemon, // device, or subprocess. 'test/integration/support/daemon-leak-model.test.ts', + 'test/integration/support/daemon-test-cleanup.test.ts', // The Android failed-step evidence reader: it replays adb output through the probe // seam, so the crash/process/activity selectors need no emulator to be pinned. 'test/integration/android-emulator-e2e/device-evidence.test.ts',