diff --git a/packages/host-kit/src/internal/host-process.ts b/packages/host-kit/src/internal/host-process.ts index 0f5f3c99b4..fdde3acb22 100644 --- a/packages/host-kit/src/internal/host-process.ts +++ b/packages/host-kit/src/internal/host-process.ts @@ -606,6 +606,38 @@ export function describeUnconfirmedProcessStop(pid: number, claimedAs: string): return `pid ${pid} could not be confirmed as ${claimedAs}, so no signal was sent to it`; } +/** Which verdict a retained stop recorded, as it travels through untyped error `details`. */ +type ConfirmedProcessStopReason = Extract['reason']; + +/** + * Whether `reason` names a retained stop verdict. Forwarding gates on this rather than on error text, + * so an unrelated failure's details cannot be forwarded as one. + */ +function isConfirmedProcessStopReason(value: unknown): value is ConfirmedProcessStopReason { + return value === 'signal-failed' || value === 'exit-timeout' || value === 'identity-unverified'; +} + +/** + * The verdict a nested layer published, or nothing when the error carries no stop verdict. A wrapper + * that flattens the inner message keeps the verdict in `details` and drops the inner surface's file + * paths and advice, which describe a file the caller of the wrapper was not asked about. + */ +export function readConfirmedProcessStopDetails( + details: Record | undefined, +): ConfirmedProcessStopDetails | undefined { + const reason = details?.reason; + const pid = details?.pid; + if (!isConfirmedProcessStopReason(reason) || typeof pid !== 'number') return undefined; + const signal = details?.signal; + const errno = details?.errno; + return { + reason, + signal: signal === 'SIGTERM' || signal === 'SIGKILL' ? signal : undefined, + errno: reason === 'signal-failed' && typeof errno === 'string' ? errno : undefined, + pid, + }; +} + /** * The process to stop, and what the caller recorded about it. A stop signals only a process it can * name: an `identity` that claims nothing is unverifiable, and so is one whose claims the host will diff --git a/packages/host-kit/src/process.ts b/packages/host-kit/src/process.ts index 878bf3c910..0b62bce639 100644 --- a/packages/host-kit/src/process.ts +++ b/packages/host-kit/src/process.ts @@ -1,7 +1,7 @@ export { type ConfirmedProcessStop, - type ConfirmedProcessStopDetails, confirmedProcessStopDetails, + type ConfirmedProcessStopDetails, describeUnconfirmedProcessStop, expandProcessTree, hostCurrentWorkingDirectory, @@ -20,6 +20,7 @@ export { type ProcessStopBudget, type ProcessStopIdentity, type ProcessStopTarget, + readConfirmedProcessStopDetails, readHostCpuArch, readHostCpuArchSync, readHostEnvironmentVariable, diff --git a/src/__tests__/client-metro-auto-companion.test.ts b/src/__tests__/client-metro-auto-companion.test.ts index a8a2a10263..1d41106cd6 100644 --- a/src/__tests__/client-metro-auto-companion.test.ts +++ b/src/__tests__/client-metro-auto-companion.test.ts @@ -3,6 +3,7 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { mkdtempForTestSync } from '@agent-device/test-kit/tmp-dir'; +import { AppError } from '@agent-device/kernel/errors'; vi.mock('../metro/client-metro-companion.ts', () => ({ ensureMetroCompanion: vi.fn(), @@ -224,7 +225,7 @@ test('prepareMetroRuntime rejects bridged descriptors without iOS bundle URLs', } }); -test('prepareMetroRuntime preserves the initial bridge error if companion startup fails', async () => { +test('prepareMetroRuntime preserves the initial bridge error and companion stop verdict', async () => { const tempRoot = mkdtempForTestSync('agent-device-metro-companion-error-'); const projectRoot = path.join(tempRoot, 'project'); fs.mkdirSync(path.join(projectRoot, 'node_modules'), { recursive: true }); @@ -239,7 +240,21 @@ test('prepareMetroRuntime preserves the initial bridge error if companion startu }), ); - vi.mocked(ensureMetroCompanion).mockRejectedValue(new Error('companion startup failed')); + vi.mocked(ensureMetroCompanion).mockRejectedValue( + // A companion stop that could not confirm the exit arrives typed, and the wrapper flattens the + // message into a `bridgeError=` fragment: the verdict has to travel in `details` to be usable. + // The companion's own artifact paths and advice travel beside it, and must not be forwarded: + // they describe a file this caller did not ask about. + new AppError('COMMAND_FAILED', 'companion startup failed', { + reason: 'signal-failed', + signal: 'SIGTERM', + errno: 'EPERM', + pid: 4101, + statePath: '/tmp/project/.agent-device/metro-companion.json', + logPath: '/tmp/project/.agent-device/metro-companion.log', + hint: 'a retry acts on that same companion', + }), + ); const fetchMock = vi.fn(); fetchMock.mockResolvedValueOnce({ @@ -269,6 +284,14 @@ test('prepareMetroRuntime preserves the initial bridge error if companion startu assert.match(error.message, /bridgeError=companion startup failed/); assert.match(error.message, /initialBridgeError=initial bridge auth failed/); assert.doesNotMatch(error.message, /metroCompanionLog=/); + const details = (error as AppError).details; + assert.equal(details?.reason, 'signal-failed'); + assert.equal(details?.signal, 'SIGTERM'); + assert.equal(details?.errno, 'EPERM'); + assert.equal(details?.pid, 4101); + assert.equal(details?.statePath, undefined); + assert.equal(details?.logPath, undefined); + assert.equal(details?.hint, undefined); return true; }, ); diff --git a/src/__tests__/client-metro-startup-cleanup.test.ts b/src/__tests__/client-metro-startup-cleanup.test.ts index dba3f56b74..d3d869dcbc 100644 --- a/src/__tests__/client-metro-startup-cleanup.test.ts +++ b/src/__tests__/client-metro-startup-cleanup.test.ts @@ -3,19 +3,27 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { mkdtempForTestSync } from '@agent-device/test-kit/tmp-dir'; +import type { AppError } from '@agent-device/kernel/errors'; vi.mock('@agent-device/host-kit/command', async (importOriginal) => ({ ...(await importOriginal()), runCmdDetached: vi.fn(), runCmdSync: vi.fn(), })); +// Host-kit's own suite proves what each host answer means; this one owns the Metro half: the +// budgets it hands over, and what the readiness failure says about what the stop came back with. vi.mock('@agent-device/host-kit/process', async (importOriginal) => ({ ...(await importOriginal()), - waitForProcessExit: vi.fn(), + readProcessStartTime: vi.fn(), + stopProcessAndConfirmExit: vi.fn(), })); import { runCmdDetached } from '@agent-device/host-kit/command'; -import { waitForProcessExit } from '@agent-device/host-kit/process'; +import { + readProcessStartTime, + stopProcessAndConfirmExit, + type ConfirmedProcessStop, +} from '@agent-device/host-kit/process'; import { prepareMetroRuntime } from '../metro/client-metro.ts'; @@ -26,6 +34,10 @@ afterEach(() => { vi.unstubAllGlobals(); }); +const SPAWNED_METRO_PID = 987; +const SPAWNED_METRO_START = 'Mon Oct 13 00:00:00 2026'; +const METRO_STOP_BUDGET = { termTimeoutMs: 1_000, killTimeoutMs: 1_000 }; + function createMetroProject(label: string): string { const tempRoot = mkdtempForTestSync(`agent-device-metro-startup-cleanup-${label}-`); const projectRoot = path.join(tempRoot, 'project'); @@ -56,8 +68,11 @@ function metroPrepareInput(projectRoot: string) { } as const; } -/** Starts a prepare whose Metro never becomes ready, letting readiness time out under fake timers. */ -async function runMetroPrepareThroughTimeout(projectRoot: string): Promise { +/** + * Runs a prepare whose Metro never becomes ready and hands back the failure it rejected with, so a + * case can read the cleanup verdict off the readiness failure. Fake timers keep the budget virtual. + */ +async function runMetroPrepareThroughTimeout(projectRoot: string): Promise { const fetchMock = vi.fn().mockResolvedValue({ ok: true, status: 200, @@ -72,142 +87,119 @@ async function runMetroPrepareThroughTimeout(projectRoot: string): Promise ); await vi.advanceTimersByTimeAsync(30_000); await expectedFailure; + return (await preparePromise.catch((error: unknown) => error)) as AppError; } -type KillSignal = 'SIGTERM' | 'SIGKILL'; - -function spyOnKill(behavior: (signal: KillSignal) => boolean): { - signals: Array<[number, string]>; -} { - const signals: Array<[number, string]> = []; - vi.spyOn(process, 'kill').mockImplementation(((pid: number, signal?: string | number) => { - const name = (signal ?? 'SIGTERM') as KillSignal; - signals.push([pid, name]); - return behavior(name) as true; - }) as typeof process.kill); - return { signals }; -} +/** What the readiness failure's `details` must carry; undefined keys pin what must stay absent. */ +const NO_CLEANUP = { reason: undefined, signal: undefined, errno: undefined, pid: undefined }; -function killError(code: string): NodeJS.ErrnoException { - return Object.assign(new Error(`kill failed: ${code}`), { code }); +function cleanupDetails(error: AppError): Record { + const details = error.details ?? {}; + return { reason: details.reason, signal: details.signal, errno: details.errno, pid: details.pid }; } -test('prepareMetroRuntime stops a spawned Metro process when startup readiness times out', async () => { - const projectRoot = createMetroProject('timeout'); - vi.mocked(runCmdDetached).mockReturnValue(987); - vi.mocked(waitForProcessExit).mockResolvedValue(true); - const kill = spyOnKill(() => true); - - try { - await runMetroPrepareThroughTimeout(projectRoot); - assert.equal(vi.mocked(runCmdDetached).mock.calls.length, 1); - assert.deepEqual(kill.signals, [[987, 'SIGTERM']]); - assert.deepEqual(vi.mocked(waitForProcessExit).mock.calls, [[987, 1_000]]); - } finally { - fs.rmSync(path.dirname(projectRoot), { recursive: true, force: true }); - } -}); - -test('stopSpawnedMetroProcess gives up cleanup (best effort) when SIGTERM throws ESRCH or EPERM', async () => { - for (const code of ['ESRCH', 'EPERM']) { - vi.clearAllMocks(); - const projectRoot = createMetroProject(`term-${code}`); - vi.mocked(runCmdDetached).mockReturnValue(987); - const kill = spyOnKill(() => { - throw killError(code); - }); - - try { - await runMetroPrepareThroughTimeout(projectRoot); - assert.deepEqual(kill.signals, [[987, 'SIGTERM']], `${code} on SIGTERM must stop cleanup`); - assert.equal(vi.mocked(waitForProcessExit).mock.calls.length, 0, code); - } finally { - fs.rmSync(path.dirname(projectRoot), { recursive: true, force: true }); - } - } -}); - -test('stopSpawnedMetroProcess escalates to SIGKILL when SIGTERM does not exit the process', async () => { - const projectRoot = createMetroProject('escalate'); - vi.mocked(runCmdDetached).mockReturnValue(987); - vi.mocked(waitForProcessExit).mockResolvedValue(false); - const kill = spyOnKill(() => true); - - try { - await runMetroPrepareThroughTimeout(projectRoot); - assert.deepEqual(kill.signals, [ - [987, 'SIGTERM'], - [987, 'SIGKILL'], - ]); - assert.deepEqual(vi.mocked(waitForProcessExit).mock.calls, [ - [987, 1_000], - [987, 1_000], - ]); - } finally { - fs.rmSync(path.dirname(projectRoot), { recursive: true, force: true }); - } -}); - -test('stopSpawnedMetroProcess gives up cleanup (best effort) when SIGKILL throws ESRCH or EPERM', async () => { - for (const code of ['ESRCH', 'EPERM']) { - vi.clearAllMocks(); - const projectRoot = createMetroProject(`kill-${code}`); - vi.mocked(runCmdDetached).mockReturnValue(987); - vi.mocked(waitForProcessExit).mockResolvedValue(false); - const kill = spyOnKill((signal) => { - if (signal === 'SIGKILL') throw killError(code); - return true; - }); +/** + * Every cleanup outcome, read off the readiness failure it decorates: the message names the pid that + * still holds the port and `details` is what a caller dispatches on. + */ +test.each([ + { + name: 'the cleanup confirmed the exit', + stop: { status: 'stopped' } satisfies ConfirmedProcessStop, + says: '', + details: NO_CLEANUP, + }, + { + name: 'a signal was refused', + stop: { + status: 'retained', + reason: 'signal-failed', + signal: 'SIGTERM', + errno: 'EPERM', + } satisfies ConfirmedProcessStop, + says: 'Dev server pid 987 is still running and refused SIGTERM (EPERM)', + details: { reason: 'signal-failed', signal: 'SIGTERM', errno: 'EPERM', pid: SPAWNED_METRO_PID }, + }, + { + // SIGTERM is accepted, then refused on the escalation: the verdict carries the later signal. + name: 'only the escalation was refused', + stop: { + status: 'retained', + reason: 'signal-failed', + signal: 'SIGKILL', + errno: 'EPERM', + } satisfies ConfirmedProcessStop, + says: 'Dev server pid 987 is still running and refused SIGKILL (EPERM)', + details: { reason: 'signal-failed', signal: 'SIGKILL', errno: 'EPERM', pid: SPAWNED_METRO_PID }, + }, + { + // The host would not say when the child started, so nothing proves the pid is still ours: the + // stop retains it instead of signaling whatever process has inherited the pid by now. + name: 'the cleanup could not establish the identity', + stop: { + status: 'retained', + reason: 'identity-unverified', + } satisfies ConfirmedProcessStop, + says: 'could not be confirmed as the process this run started', + details: { + reason: 'identity-unverified', + signal: undefined, + errno: undefined, + pid: SPAWNED_METRO_PID, + }, + }, + { + name: 'the process outlived both budgets', + stop: { + status: 'retained', + reason: 'exit-timeout', + signal: 'SIGKILL', + } satisfies ConfirmedProcessStop, + says: 'Dev server pid 987 is still running', + details: { + reason: 'exit-timeout', + signal: 'SIGKILL', + errno: undefined, + pid: SPAWNED_METRO_PID, + }, + }, +])( + 'prepareMetroRuntime reports a dev server to reap when $name', + async ({ stop, says, details }) => { + const projectRoot = createMetroProject('outcome'); + vi.mocked(runCmdDetached).mockReturnValue(SPAWNED_METRO_PID); + vi.mocked(readProcessStartTime).mockReturnValue(SPAWNED_METRO_START); + vi.mocked(stopProcessAndConfirmExit).mockResolvedValue(stop); try { - await runMetroPrepareThroughTimeout(projectRoot); - assert.deepEqual(kill.signals, [ - [987, 'SIGTERM'], - [987, 'SIGKILL'], + const failure = await runMetroPrepareThroughTimeout(projectRoot); + if (says) assert.ok(failure.message.includes(says), failure.message); + else assert.doesNotMatch(failure.message, /still running/); + assert.deepEqual(cleanupDetails(failure), details); + assert.deepEqual(vi.mocked(stopProcessAndConfirmExit).mock.calls, [ + [ + { pid: SPAWNED_METRO_PID, identity: { startTime: SPAWNED_METRO_START } }, + METRO_STOP_BUDGET, + ], ]); - assert.deepEqual( - vi.mocked(waitForProcessExit).mock.calls, - [[987, 1_000]], - `${code} on SIGKILL means the process is already gone; no second exit wait`, - ); } finally { fs.rmSync(path.dirname(projectRoot), { recursive: true, force: true }); } - } -}); + }, +); -test('prepareMetroRuntime reports the readiness timeout when the escalated SIGKILL is refused with EACCES', async () => { - const projectRoot = createMetroProject('kill-eacces'); - vi.mocked(runCmdDetached).mockReturnValue(987); - vi.mocked(waitForProcessExit).mockResolvedValue(false); - const kill = spyOnKill((signal) => { - if (signal === 'SIGKILL') throw killError('EACCES'); - return true; - }); - - try { - await runMetroPrepareThroughTimeout(projectRoot); - assert.deepEqual(kill.signals, [ - [987, 'SIGTERM'], - [987, 'SIGKILL'], - ]); - assert.deepEqual(vi.mocked(waitForProcessExit).mock.calls, [[987, 1_000]]); - } finally { - fs.rmSync(path.dirname(projectRoot), { recursive: true, force: true }); - } -}); - -test('prepareMetroRuntime reports the readiness timeout when SIGTERM is refused with EACCES', async () => { - const projectRoot = createMetroProject('term-eacces'); - vi.mocked(runCmdDetached).mockReturnValue(987); - const kill = spyOnKill(() => { - throw killError('EACCES'); - }); +// Cleanup stays best effort: an unclassifiable host fault is the cause, never a replacement for the +// readiness failure and its log path, which is what the user acts on. +test('prepareMetroRuntime keeps the readiness failure when cleanup faults', async () => { + const projectRoot = createMetroProject('host-fault'); + const fault = new TypeError('kill refused: bad signal'); + vi.mocked(runCmdDetached).mockReturnValue(SPAWNED_METRO_PID); + vi.mocked(stopProcessAndConfirmExit).mockRejectedValue(fault); try { - await runMetroPrepareThroughTimeout(projectRoot); - assert.deepEqual(kill.signals, [[987, 'SIGTERM']]); - assert.equal(vi.mocked(waitForProcessExit).mock.calls.length, 0); + const failure = await runMetroPrepareThroughTimeout(projectRoot); + assert.equal(failure.cause, fault); + assert.deepEqual(cleanupDetails(failure), NO_CLEANUP); } finally { fs.rmSync(path.dirname(projectRoot), { recursive: true, force: true }); } diff --git a/src/metro/client-metro.ts b/src/metro/client-metro.ts index d64ad0cbc8..b6d7a1a0fa 100644 --- a/src/metro/client-metro.ts +++ b/src/metro/client-metro.ts @@ -11,7 +11,14 @@ import type { import fs from 'node:fs'; import path from 'node:path'; import { runCmdSync, runCmdDetached } from '@agent-device/host-kit/command'; -import { waitForProcessExit } from '@agent-device/host-kit/process'; +import { + confirmedProcessStopDetails, + describeUnconfirmedProcessStop, + readConfirmedProcessStopDetails, + readProcessStartTime, + stopProcessAndConfirmExit, + type ConfirmedProcessStop, +} from '@agent-device/host-kit/process'; import { sleep } from '@agent-device/host-kit/retry'; import { ensureMetroCompanion } from './client-metro-companion.ts'; import type { @@ -53,6 +60,8 @@ type PackageManagerConfig = { type MetroProcessResult = { pid: number; + /** The lifetime the host reported at spawn; a stale pid cannot imitate it. */ + startTime?: string; }; type ResolvedMetroPrepareSettings = { @@ -406,27 +415,34 @@ function startMetroProcess( return { pid, + startTime: readProcessStartTime(pid) ?? undefined, }; } -async function stopSpawnedMetroProcess(pid: number): Promise { - if (!Number.isInteger(pid) || pid <= 0) return; - try { - process.kill(pid, 'SIGTERM'); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code === 'ESRCH' || code === 'EPERM') return; - throw error; - } - if (await waitForProcessExit(pid, METRO_TERM_TIMEOUT_MS)) return; - try { - process.kill(pid, 'SIGKILL'); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code === 'ESRCH' || code === 'EPERM') return; - throw error; - } - await waitForProcessExit(pid, METRO_KILL_TIMEOUT_MS); +/** + * Readiness failure, carrying what the cleanup proved: a dev server still live afterwards names its + * pid, signal and typed `details.reason`, so the user learns what still holds the port. + */ +function metroStartupFailure( + statusUrl: string, + settings: ResolvedMetroPrepareSettings, + pid: number, + stop: ConfirmedProcessStop | { throws: unknown }, +): AppError { + const message = `React Native dev server did not become ready at ${statusUrl} within ${settings.startupTimeoutMs}ms. Check ${settings.logPath}.`; + // Cleanup stays best effort: an unclassifiable fault travels as the cause, not as the failure. + if ('throws' in stop) + return new AppError('COMMAND_FAILED', message, { logPath: settings.logPath }, stop.throws); + if (stop.status === 'stopped') + return new AppError('COMMAND_FAILED', message, { logPath: settings.logPath }); + const kept = + stop.reason === 'identity-unverified' + ? describeUnconfirmedProcessStop(pid, 'the process this run started') + : `pid ${pid} is still running${stop.reason === 'signal-failed' ? ` and refused ${stop.signal} (${stop.errno})` : ''}`; + return new AppError('COMMAND_FAILED', `${message} Dev server ${kept}; stop it before retrying.`, { + logPath: settings.logPath, + ...confirmedProcessStopDetails(stop, pid), + }); } function createProxyHeaders(baseUrl: string, bearerToken: string): Record { @@ -848,12 +864,16 @@ async function ensureMetroProcessReady( return { started: true, reused: false, pid: startedProcess.pid, statusUrl }; } - await stopSpawnedMetroProcess(startedProcess.pid).catch(() => {}); - throw new AppError( - 'COMMAND_FAILED', - `React Native dev server did not become ready at ${statusUrl} within ${settings.startupTimeoutMs}ms. Check ${settings.logPath}.`, - { logPath: settings.logPath }, - ); + // The recorded start time is what keeps this from signaling whatever process inherits the pid + // after a slow readiness timeout. + const stop = await stopProcessAndConfirmExit( + { pid: startedProcess.pid, identity: { startTime: startedProcess.startTime } }, + { + termTimeoutMs: METRO_TERM_TIMEOUT_MS, + killTimeoutMs: METRO_KILL_TIMEOUT_MS, + }, + ).catch((error: unknown) => ({ throws: error })); + throw metroStartupFailure(statusUrl, settings, startedProcess.pid, stop); } async function configureProxyBridgeForRuntime( @@ -917,6 +937,9 @@ async function configureProxyBridgeViaCompanion( }); companionLogPath = companion.logPath; } catch (error) { + // The wrapper flattens the cause into an undispatchable `bridgeError=`, so the verdict travels + // in `details`. Only the verdict: the companion's own log path and advice describe a surface + // this caller did not ask about, and would outrank this command's own `details`. throw new AppError( 'COMMAND_FAILED', describeBridgeFailure( @@ -925,7 +948,7 @@ async function configureProxyBridgeViaCompanion( bridge, initialBridgeError, ), - undefined, + error instanceof AppError ? readConfirmedProcessStopDetails(error.details) : undefined, error, ); }