diff --git a/packages/host-kit/src/internal/host-process-stop-confirm.test.ts b/packages/host-kit/src/internal/host-process-stop-confirm.test.ts new file mode 100644 index 0000000000..b94647530c --- /dev/null +++ b/packages/host-kit/src/internal/host-process-stop-confirm.test.ts @@ -0,0 +1,313 @@ +import assert from 'node:assert/strict'; +import { afterEach, test, vi } from 'vitest'; + +// Every claim the stopper makes is about what the host answers, so one double answers both syscalls it +// reads: `kill(pid, signal)` for the signals and liveness probes, and `ps` for the identity fields. +// One host state drives both, so no case can be live while its identity says otherwise. +const { mockRunCmd } = vi.hoisted(() => ({ mockRunCmd: vi.fn() })); +vi.mock('./exec.ts', async () => ({ + ...(await vi.importActual('./exec.ts')), + runCmd: mockRunCmd, +})); + +import { + stopProcessAndConfirmExit, + type ConfirmedProcessStop, + type ProcessStopTarget, +} from './host-process.ts'; + +const PID = 4242; +const BUDGET = { termTimeoutMs: 1_000, killTimeoutMs: 500 }; +type Signal = 'SIGTERM' | 'SIGKILL'; + +const START = 'Mon Oct 12 00:00:00 2026'; +const OTHER_START = 'Tue Oct 13 09:00:00 2026'; +const COMMAND = '/usr/bin/node /app/dist/src/client/companion-tunnel.js --run-metro-companion'; + +const STOPPED: ConfirmedProcessStop = { status: 'stopped' }; +/** `ps` found the row and told nothing for that column, which is unknown rather than absent. */ +const EMPTY = { stdout: '', stderr: '', exitCode: 0 }; +const PS_REFUSED = { stdout: '', stderr: 'ps: not permitted', exitCode: 1 }; +const psAnswer = (value: string) => ({ stdout: `${value}\n`, stderr: '', exitCode: 0 }); +const refused = (signal: Signal): ConfirmedProcessStop => ({ + status: 'retained', + reason: 'signal-failed', + signal, + errno: 'EPERM', +}); + +/** What the host will say about the pid, overriding what the recorded process would show. */ +type HostIdentity = Readonly<{ startTime?: string | null; command?: string | null }>; + +type HostCase = Readonly<{ + name: string; + /** + * Which claims the caller recorded about the pid. Defaults to having recorded both; `'none'` is a + * caller that stored only the pid. + */ + claims?: 'startTime' | 'matches' | 'both' | 'none'; + /** What the host answers each signal with: a string is the refusing errno, absent accepts. */ + answers?: Partial>; + /** The signal after which the process has run its course, when it ends because it was asked. */ + exitsOnSignal?: Signal; + /** How many liveness probes it survives before somebody else ends it. */ + exitsAfterProbes?: number; + /** + * How a process that has run its course keeps answering to its pid, which is where liveness and + * identity disagree: a POSIX zombie answers `EPERM` beside a `Z` row. `'held'` never lets go. + */ + afterExit?: 'zombie' | 'held'; + identity?: HostIdentity; + /** A `ps` the host refuses outright, so identity cannot be read at all. */ + psFails?: boolean; + /** The signals the stop must spend, in order, and the verdict each host answer earns. */ + expect: { outcome: ConfirmedProcessStop; signals: Signal[] }; +}>; + +type HostModel = Omit; +type Expect = HostCase['expect']; +const HELD = { afterExit: 'held' } satisfies HostModel; +const DENIED_ON_TERM = { answers: { SIGTERM: 'EPERM' } } satisfies HostModel; +const UNREADABLE = { identity: { startTime: null, command: null } } satisfies HostModel; +/** The process has run its course: liveness stops answering for it before anything is signaled. */ +const GONE = { exitsAfterProbes: 0 } satisfies HostModel; +const ZOMBIE = { ...GONE, afterExit: 'zombie' } satisfies HostModel; +const STOPPED_BY_TERM: Expect = { outcome: STOPPED, signals: ['SIGTERM'] }; +const STOPPED_BY_KILL: Expect = { outcome: STOPPED, signals: ['SIGTERM', 'SIGKILL'] }; +const NEVER_SIGNALED: Expect = { outcome: STOPPED, signals: [] }; +const REFUSED_ON_TERM: Expect = { outcome: refused('SIGTERM'), signals: ['SIGTERM'] }; +const REFUSED_ON_KILL: Expect = { outcome: refused('SIGKILL'), signals: STOPPED_BY_KILL.signals }; +const EXIT_TIMED_OUT: Expect = { + outcome: { status: 'retained', reason: 'exit-timeout', signal: 'SIGKILL' }, + signals: STOPPED_BY_KILL.signals, +}; +const UNVERIFIED: Expect = { + outcome: { status: 'retained', reason: 'identity-unverified' }, + signals: [], +}; + +const STOP_CASES: HostCase[] = [ + // A caller that recorded nothing about the pid cannot name the process at all, so no signal is + // spent on it: the pid is retained rather than handed to whatever holds it now. + { name: 'is a pid nobody recorded anything about', claims: 'none', ...HELD, expect: UNVERIFIED }, + // Confirming one claim is not enough when the other is the one the host will not answer: the + // readable fact has to be the recorded one, or a silent column authorises a blind signal. + { + name: 'has only a command matcher and the host withholds the command line', + claims: 'matches', + identity: { command: null }, + ...HELD, + expect: UNVERIFIED, + }, + { + name: 'has only a recorded start time and the host withholds the start time', + claims: 'startTime', + identity: { startTime: null }, + ...HELD, + expect: UNVERIFIED, + }, + + // Escalation, refusal and the two ways a signal stage proves the exit. + { name: 'leaves on SIGTERM', exitsOnSignal: 'SIGTERM', expect: STOPPED_BY_TERM }, + { + name: 'ignores SIGTERM and takes SIGKILL', + exitsOnSignal: 'SIGKILL', + expect: STOPPED_BY_KILL, + }, + { name: 'was gone before anything was signaled', ...GONE, expect: NEVER_SIGNALED }, + { + name: 'is held by another owner', + ...DENIED_ON_TERM, + ...HELD, + expect: REFUSED_ON_TERM, + }, + { + name: 'refuses SIGKILL', + answers: { SIGKILL: 'EPERM' }, + ...HELD, + expect: REFUSED_ON_KILL, + }, + { + name: 'is released inside the budget', + ...DENIED_ON_TERM, + exitsAfterProbes: 3, + expect: STOPPED_BY_TERM, + }, + { + // Liveness keeps answering for a zombie that outlives its budget, so only the `Z` row proves + // the recorded process is gone, and it does that after the wait rather than before the signal. + name: 'leaves a zombie the budget cannot wait out', + exitsOnSignal: 'SIGTERM', + afterExit: 'zombie', + expect: STOPPED_BY_TERM, + }, + { name: 'outlives both budgets', ...HELD, expect: EXIT_TIMED_OUT }, + + // What the recorded identity buys: a stale pid is proven without signaling it, and a host that + // will not describe the pid stops the stopper instead of authorizing a blind signal. + { + name: 'left a zombie before anything was signaled', + ...ZOMBIE, + expect: NEVER_SIGNALED, + }, + { + // Windows hides only the command line of an elevated or other-user process, and that is the #3540 + // companion: it must be signaled and retained, never reported stopped. + name: 'recorded process hides its command line, as an elevated one does', + identity: { command: null }, + ...DENIED_ON_TERM, + ...HELD, + expect: REFUSED_ON_TERM, + }, + { + name: 'recorded process hides its start time', + identity: { startTime: null }, + ...DENIED_ON_TERM, + ...HELD, + expect: REFUSED_ON_TERM, + }, + { + // POSIX `hidepid`, a `ps` that times out, or a Windows process that exited while its object still + // had handles: the pid answers `kill(pid, 0)` and nothing else, which is unknown, not absent. + name: 'live recorded pid the host will not describe', + ...UNREADABLE, + ...HELD, + expect: UNVERIFIED, + }, + { + name: 'live recorded pid whose process tool the host refuses', + psFails: true, + ...HELD, + expect: UNVERIFIED, + }, + { + name: 'pid the host has handed to another lifetime', + identity: { startTime: OTHER_START }, + expect: NEVER_SIGNALED, + }, + { + name: 'pid whose command line names a different program', + identity: { command: '/usr/bin/other --serve' }, + expect: NEVER_SIGNALED, + }, +]; + +function driveHost(model: HostCase): { signals: Signal[] } { + const signals: Signal[] = []; + let ended = false; + let probes = 0; + const hasEnded = () => + ended || + (model.afterExit !== 'held' && probes >= (model.exitsAfterProbes ?? Number.POSITIVE_INFINITY)); + vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal: string | number) => { + if (signal === 0) { + probes += 1; + // A released pid answers `ESRCH`. A zombie, and on Windows a process whose object still has + // handles, both keep answering `EPERM`, and only the process table tells those apart. + if (hasEnded() && model.afterExit === undefined) + throw Object.assign(new Error('no such process'), { code: 'ESRCH' }); + if (hasEnded()) throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + return true; + } + const name = signal as Signal; + signals.push(name); + const errno = model.answers?.[name]; + if (errno) throw Object.assign(new Error(`kill refused: ${errno}`), { code: errno }); + if (model.exitsOnSignal === name) ended = true; + return true; + }) as typeof process.kill); + + // Absent means the recorded process, `null` means the host withholds the column, and a string is + // what it says instead. + const identity = { + startTime: model.identity?.startTime === undefined ? START : model.identity.startTime, + command: model.identity?.command === undefined ? COMMAND : model.identity.command, + }; + mockRunCmd.mockImplementation(async (_command: string, args: readonly string[]) => { + if (model.psFails) return PS_REFUSED; + // A process the host has fully released, and one that left no row, both answer no fields. + if (hasEnded() && model.afterExit !== 'zombie') return EMPTY; + const field = String(args[3]); + if (field === 'state=') return psAnswer(hasEnded() && model.afterExit === 'zombie' ? 'Z' : 'S'); + // Withheld is its own answer: `ps` finds the row and reports nothing for that column, which is + // how Windows answers `CommandLine` for another user's process. + const column = field === 'lstart=' ? identity.startTime : identity.command; + return column === null ? EMPTY : psAnswer(column); + }); + return { signals }; +} + +function targetOf(model: HostCase): ProcessStopTarget { + const claims = model.claims ?? 'both'; + return { + pid: PID, + identity: { + startTime: claims === 'startTime' || claims === 'both' ? START : undefined, + matches: claims === 'matches' || claims === 'both' ? (c) => c === COMMAND : undefined, + }, + }; +} + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + mockRunCmd.mockReset(); +}); + +// The host model above answers with POSIX `ps` fields; a Windows host answers the same questions from +// CIM, which never reports a zombie state. win32 branches live in host-process-windows.test.ts. +test.skipIf(process.platform === 'win32').each(STOP_CASES)( + 'stopProcessAndConfirmExit $name', + async (testCase) => { + const host = driveHost(testCase); + // Both stage budgets, advanced on a fake clock: a retained outcome must not cost real time. + vi.useFakeTimers(); + const stop = stopProcessAndConfirmExit(targetOf(testCase), BUDGET); + await vi.advanceTimersByTimeAsync(BUDGET.termTimeoutMs + BUDGET.killTimeoutMs); + + assert.deepEqual(await stop, testCase.expect.outcome); + assert.deepEqual(host.signals, testCase.expect.signals); + }, +); + +// `kill(0, SIGTERM)` addresses this very process group, so an unvalidated pid must not reach it. +test.each([0, -1, 1.5, Number.NaN])( + 'never signals a pid that is not a process: %s', + async (pid) => { + const signals: string[] = []; + vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal: string) => { + signals.push(signal); + throw Object.assign(new Error('no such process'), { code: 'ESRCH' }); + }) as typeof process.kill); + assert.deepEqual(await stopProcessAndConfirmExit({ pid, identity: {} }, BUDGET), STOPPED); + assert.deepEqual(signals, []); + }, +); + +// Stale or malformed process state can name the caller's own pid, and SIGTERM would end the very +// process trying to stop it. +test('refuses to stop the current process', async () => { + const signals: string[] = []; + vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal: string) => { + signals.push(signal); + return true; + }) as typeof process.kill); + await assert.rejects( + () => stopProcessAndConfirmExit({ pid: process.pid, identity: {} }, BUDGET), + RangeError, + ); + assert.deepEqual(signals, []); +}); + +// Node's own argument faults carry a `code` that is not an errno, and reading one as a refusal would +// send the caller off to stop a process that never existed. The fault itself has to reach it. +test('propagates an argument fault instead of reporting it as a host refusal', async () => { + const failure = Object.assign(new TypeError('bad signal'), { code: 'ERR_INVALID_ARG_TYPE' }); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw failure; + }); + const settled = await stopProcessAndConfirmExit({ pid: PID, identity: {} }, BUDGET).catch( + (error: unknown) => error, + ); + assert.equal(settled, failure); +}); diff --git a/packages/host-kit/src/internal/host-process.ts b/packages/host-kit/src/internal/host-process.ts index 1073b401dc..0f5f3c99b4 100644 --- a/packages/host-kit/src/internal/host-process.ts +++ b/packages/host-kit/src/internal/host-process.ts @@ -554,6 +554,213 @@ export async function waitForProcessExit(pid: number, timeoutMs: number): Promis return !isProcessAlive(pid); } +type StopSignal = 'SIGTERM' | 'SIGKILL'; + +/** + * The outcome of a stop that a caller may act on. A process tracked elsewhere (a state file, a + * registration) may be forgotten only on `stopped`. + * + * - `stopped`: the host released the pid, or proved the pid now names a different process. + * - `retained` + `signal-failed`: `signal` was refused by `errno` while the process was there. + * `EPERM` usually means another owner holds it, and later signals are refused by the same check. + * - `retained` + `exit-timeout`: SIGKILL was accepted and the process outlived its budget. + * - `retained` + `identity-unverified`: no recorded claim could be confirmed against a readable fact, + * so nothing proves which process a signal would reach. + */ +export type ConfirmedProcessStop = + | Readonly<{ status: 'stopped' }> + | Readonly<{ status: 'retained'; reason: 'signal-failed'; signal: StopSignal; errno: string }> + | Readonly<{ status: 'retained'; reason: 'exit-timeout'; signal: 'SIGKILL' }> + | Readonly<{ status: 'retained'; reason: 'identity-unverified' }>; + +/** The verdict keys a retained stop publishes as error `details`, and nothing else. */ +export type ConfirmedProcessStopDetails = Readonly<{ + reason: Extract['reason']; + signal?: StopSignal; + errno?: string; + pid: number; +}>; + +/** + * Projects a retained verdict onto the keys a caller may publish. A caller adds its own artifact + * paths beside these; a layer that forwards the verdict forwards only these, so an inner surface's + * file paths and advice cannot ride along under an outer surface's message. + */ +export function confirmedProcessStopDetails( + retained: Extract, + pid: number, +): ConfirmedProcessStopDetails { + return { + reason: retained.reason, + signal: 'signal' in retained ? retained.signal : undefined, + errno: retained.reason === 'signal-failed' ? retained.errno : undefined, + pid, + }; +} + +/** + * Names a pid the stop left alone. This is the one verdict that must not say the process is still + * running: the host refused to describe it, so nothing was ever signaled to it. + */ +export function describeUnconfirmedProcessStop(pid: number, claimedAs: string): string { + return `pid ${pid} could not be confirmed as ${claimedAs}, so no signal was sent to it`; +} + +/** + * 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 + * not confirm. + */ +export type ProcessStopTarget = Readonly<{ + pid: number; + identity: ProcessStopIdentity; +}>; + +export type ProcessStopIdentity = Readonly<{ + /** The start time recorded for this pid; a differing one proves the pid changed hands. */ + startTime?: string; + /** Whether `command` names the process this stop is meant for. */ + matches?: (command: string) => boolean; +}>; + +/** How long each signal stage may wait for the exit, on the same keys `stopPidsWithEscalation` takes. */ +export type ProcessStopBudget = Pick< + StopPidsWithEscalationOptions, + 'termTimeoutMs' | 'killTimeoutMs' +>; + +/** + * Signals one process TERM then KILL and proves which way the recorded process ended, so a caller + * only forgets one it watched leave. Identity settles it both ways: unreadable identity retains + * rather than signaling a process this caller cannot name, while a released pid, a zombie, or a + * start time the host contradicts proves it gone. A refusal is not absence, so each stage waits. + */ +export async function stopProcessAndConfirmExit( + target: ProcessStopTarget, + budget: ProcessStopBudget, +): Promise { + // Stale or malformed process state must never aim a signal at the caller itself. + if (target.pid === process.pid) throw new RangeError('Cannot stop the current process'); + const identity = await proveStopTargetIdentity(target); + if (identity === 'gone') return { status: 'stopped' }; + if (identity === 'unverified') return { status: 'retained', reason: 'identity-unverified' }; + const term = await proveSignalStage(target, 'SIGTERM', budget.termTimeoutMs); + if (term.proof === 'exited') return { status: 'stopped' }; + // A refused SIGTERM is the whole verdict: the host refuses SIGKILL by the same permission check, + // so escalating would spend a second budget repeating it. + if (term.proof === 'refused') return retainedSignalFailure('SIGTERM', term.errno); + const kill = await proveSignalStage(target, 'SIGKILL', budget.killTimeoutMs); + if (kill.proof === 'exited') return { status: 'stopped' }; + if (kill.proof === 'refused') return retainedSignalFailure('SIGKILL', kill.errno); + return { status: 'retained', reason: 'exit-timeout', signal: 'SIGKILL' }; +} + +/** + * Whether the live pid still names the process the caller recorded, read once before anything is + * signaled. `ours` needs a claim the host confirmed: two recorded facts that no readable fact + * contradicts only prove the host is silent, and silence about a pid this caller may not own is + * exactly what must not become permission to signal it. + */ +async function proveStopTargetIdentity( + target: ProcessStopTarget, +): Promise<'ours' | 'gone' | 'unverified'> { + if (!isStopPidLive(target.pid)) return 'gone'; + const facts = await readProcessIdentityFacts(target.pid); + if (facts.zombie) return 'gone'; + if (target.identity.startTime && facts.startTime && facts.startTime !== target.identity.startTime) + return 'gone'; + if (target.identity.matches && facts.command && !target.identity.matches(facts.command)) + return 'gone'; + const confirmedStartTime = Boolean(target.identity.startTime && facts.startTime); + const confirmedCommand = Boolean(target.identity.matches && facts.command); + return confirmedStartTime || confirmedCommand ? 'ours' : 'unverified'; +} + +function retainedSignalFailure(signal: StopSignal, errno: string): ConfirmedProcessStop { + return { status: 'retained', reason: 'signal-failed', signal, errno }; +} + +/** What one signal stage proved, within the budget that stage was given. */ +type SignalStageProof = + | Readonly<{ proof: 'exited' }> + | Readonly<{ proof: 'refused'; errno: string }> + | Readonly<{ proof: 'survived' }>; + +async function proveSignalStage( + target: ProcessStopTarget, + signal: StopSignal, + timeoutMs: number, +): Promise { + const errno = signalProcessForStop(target.pid, signal); + // `ESRCH` is absence; any other refusal still leaves a live pid worth waiting on, because the + // owner may release it inside this budget, and a released pid is what the caller may forget. + if (errno === 'ESRCH') return { proof: 'exited' }; + if (await waitForStopPidRelease(target.pid, timeoutMs)) return { proof: 'exited' }; + if (await proveRecordedProcessGone(target)) return { proof: 'exited' }; + return errno === undefined ? { proof: 'survived' } : { proof: 'refused', errno }; +} + +const STOP_LIVENESS_POLL_MS = 50; + +/** + * Whether `kill(pid, 0)` finds the process. `isProcessAlive` reads every answer but `EPERM` as + * absence, which suits a best-effort sweep and not a stop: here only `ESRCH` releases a pid, and an + * answer that is not about the process at all must not become permission to forget it. + */ +function isStopPidLive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ESRCH') return false; + if (code === 'EPERM') return true; + throw error; + } +} + +async function waitForStopPidRelease(pid: number, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + for (;;) { + if (!isStopPidLive(pid)) return true; + if (Date.now() >= deadline) return false; + await sleep(Math.min(STOP_LIVENESS_POLL_MS, Math.max(0, deadline - Date.now()))); + } +} + +/** + * Whether the pid outliving its budget still proves the recorded process is gone, which the answer to + * `kill(pid, 0)` cannot: `EPERM` also comes back for a zombie awaiting its parent, and on Windows for + * a process that exited while its object still had handles. A start time the host contradicts proves + * the pid changed hands; the process table holding no row does not, because a hidden or unreadable + * process is missing from it too. + */ +async function proveRecordedProcessGone(target: ProcessStopTarget): Promise { + const facts = await readProcessIdentityFacts(target.pid); + if (facts.zombie) return true; + const startTime = target.identity.startTime; + return Boolean(startTime && facts.startTime && facts.startTime !== startTime); +} + +/** The errno that refused the signal, or `undefined` when the host accepted it. */ +function signalProcessForStop(pid: number, signal: StopSignal): string | undefined { + // A non-positive pid is no process at all: `kill(0, …)` would address our own process group. + if (!Number.isInteger(pid) || pid <= 0) return 'ESRCH'; + try { + process.kill(pid, signal); + return undefined; + } catch (error) { + // Only an errno classifies a stop outcome: Node's argument faults carry a `code` too, and one + // reported as a refusal would invent a process for the caller to go and stop. + const code = (error as NodeJS.ErrnoException).code; + if (typeof code === 'string' && ERRNO_CODE.test(code)) return code; + throw error; + } +} + +const ERRNO_CODE = /^E[A-Z0-9]+$/; + export async function stopPidsWithEscalation( options: StopPidsWithEscalationOptions, ): Promise { diff --git a/packages/host-kit/src/process.ts b/packages/host-kit/src/process.ts index f4cc43fd5a..878bf3c910 100644 --- a/packages/host-kit/src/process.ts +++ b/packages/host-kit/src/process.ts @@ -1,4 +1,8 @@ export { + type ConfirmedProcessStop, + type ConfirmedProcessStopDetails, + confirmedProcessStopDetails, + describeUnconfirmedProcessStop, expandProcessTree, hostCurrentWorkingDirectory, hostEnvironment, @@ -13,6 +17,9 @@ export { isProcessGroupAlive, isProcessZombie, listHostProcesses, + type ProcessStopBudget, + type ProcessStopIdentity, + type ProcessStopTarget, readHostCpuArch, readHostCpuArchSync, readHostEnvironmentVariable, @@ -23,6 +30,7 @@ export { readProcessStartTime, signalPidsBestEffort, stopPidsWithEscalation, + stopProcessAndConfirmExit, uniquePositivePids, waitForProcessExit, writeHostStderr, diff --git a/src/__tests__/client-metro-companion.test.ts b/src/__tests__/client-metro-companion.test.ts index b4863247dd..d32d8cc209 100644 --- a/src/__tests__/client-metro-companion.test.ts +++ b/src/__tests__/client-metro-companion.test.ts @@ -3,17 +3,20 @@ 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(), })); +// `stopProcessAndConfirmExit` is the seam this suite owns: what it proved is decided by the case +// table, and these tests assert what the companion layer does with the verdict. vi.mock('@agent-device/host-kit/process', async (importOriginal) => ({ ...(await importOriginal()), isProcessAlive: vi.fn(), readProcessCommand: vi.fn(), readProcessStartTime: vi.fn(), - waitForProcessExit: vi.fn(), + stopProcessAndConfirmExit: vi.fn(), })); import { runCmdDetached } from '@agent-device/host-kit/command'; @@ -21,11 +24,13 @@ import { isProcessAlive, readProcessCommand, readProcessStartTime, - waitForProcessExit, + stopProcessAndConfirmExit, + type ConfirmedProcessStop, } from '@agent-device/host-kit/process'; import { ensureMetroCompanion, stopMetroCompanion } from '../metro/client-metro-companion.ts'; import { ensureReactDevtoolsCompanion } from '../client/client-react-devtools-companion.ts'; +import { METRO_COMPANION_RUN_ARG } from '../client/client-companion-tunnel-contract.ts'; const TEST_BRIDGE_SCOPE = { tenantId: 'tenant-1', @@ -55,6 +60,32 @@ function assertCompanionRunArg(callIndex: number, runArg: string): void { assert.equal(call[1].at(-1), runArg); } +/** The stops the code asked host-kit for: the identity recorded beside the pid, and the budget. */ +const COMPANION_STOP_BUDGET = { termTimeoutMs: 1_000, killTimeoutMs: 1_000 }; +const COMPANION_TUNNEL_COMMAND = `${process.execPath} src/client/companion-tunnel.ts ${METRO_COMPANION_RUN_ARG}`; +const UNRELATED_COMMAND = '/usr/bin/nginx -g daemon off;'; +// A predicate compares by identity, so deepEqual cannot see whether it would accept the tunnel or +// refuse a stranger. Projecting its verdicts is what makes a dropped or inverted matcher fail here. +const stopRequests = () => + vi.mocked(stopProcessAndConfirmExit).mock.calls.map(([target, budget]) => ({ + pid: target.pid, + startTime: target.identity.startTime, + namesCompanion: target.identity.matches?.(COMPANION_TUNNEL_COMMAND) ?? null, + namesStranger: target.identity.matches?.(UNRELATED_COMMAND) ?? null, + budget, + })); + +const STOPPED: ConfirmedProcessStop = { status: 'stopped' }; + +/** One projected stop request, with the matcher having accepted the tunnel and refused a stranger. */ +const companionStop = (pid: number, startTime: string) => ({ + pid, + startTime, + namesCompanion: true, + namesStranger: false, + budget: COMPANION_STOP_BUDGET, +}); + test('companion ownership is profile-scoped and consumer-counted', async () => { const projectRoot = mkdtempForTestSync('agent-device-metro-companion-state-'); try { @@ -66,8 +97,7 @@ test('companion ownership is profile-scoped and consumer-counted', async () => { vi.mocked(readProcessCommand).mockImplementation( () => `${process.execPath} src/client/companion-tunnel.ts --agent-device-run-metro-companion`, ); - vi.mocked(waitForProcessExit).mockResolvedValue(true); - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true); + vi.mocked(stopProcessAndConfirmExit).mockResolvedValue(STOPPED); const stagingFirst = await ensureMetroCompanion({ projectRoot, @@ -119,7 +149,7 @@ test('companion ownership is profile-scoped and consumer-counted', async () => { consumerKey: 'session-a', }); assert.equal(partialStop.stopped, false); - assert.equal(killSpy.mock.calls.length, 0); + assert.equal(vi.mocked(stopProcessAndConfirmExit).mock.calls.length, 0); const remainingState = JSON.parse(fs.readFileSync(stagingFirst.statePath, 'utf8')) as { consumers: string[]; @@ -132,8 +162,7 @@ test('companion ownership is profile-scoped and consumer-counted', async () => { consumerKey: 'session-b', }); assert.equal(finalStop.stopped, true); - assert.equal(killSpy.mock.calls.length, 1); - assert.deepEqual(killSpy.mock.calls[0], [111, 'SIGTERM']); + assert.deepEqual(stopRequests(), [companionStop(111, 'start-111')]); assert.equal(fs.existsSync(stagingFirst.statePath), false); assert.equal(fs.existsSync(stagingFirst.logPath), false); @@ -143,8 +172,10 @@ test('companion ownership is profile-scoped and consumer-counted', async () => { consumerKey: 'session-prod', }); assert.equal(prodStop.stopped, true); - assert.equal(killSpy.mock.calls.length, 2); - assert.deepEqual(killSpy.mock.calls[1], [222, 'SIGTERM']); + assert.deepEqual(stopRequests(), [ + companionStop(111, 'start-111'), + companionStop(222, 'start-222'), + ]); assert.equal(fs.existsSync(prod.statePath), false); assert.equal(fs.existsSync(prod.logPath), false); } finally { @@ -163,8 +194,7 @@ test('launchUrl changes force a companion respawn for the same profile', async ( vi.mocked(readProcessCommand).mockImplementation( () => `${process.execPath} src/client/companion-tunnel.ts --agent-device-run-metro-companion`, ); - vi.mocked(waitForProcessExit).mockResolvedValue(true); - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true); + vi.mocked(stopProcessAndConfirmExit).mockResolvedValue(STOPPED); const first = await ensureMetroCompanion({ projectRoot, @@ -190,9 +220,8 @@ test('launchUrl changes force a companion respawn for the same profile', async ( assert.equal(first.spawned, true); assert.equal(second.spawned, true); assert.equal(vi.mocked(runCmdDetached).mock.calls.length, 2); - assert.equal(killSpy.mock.calls.length, 1); + assert.deepEqual(stopRequests(), [companionStop(333, 'start-333')]); assertCompanionSpawnTarget(); - assert.deepEqual(killSpy.mock.calls[0], [333, 'SIGTERM']); const state = JSON.parse(fs.readFileSync(second.statePath, 'utf8')) as { launchUrl?: string; @@ -328,6 +357,7 @@ function writeOwnCompanionState(projectRoot: string, pid: number): string { statePath, `${JSON.stringify({ pid, + startTime: `start-${pid}`, serverBaseUrl: 'https://bridge.example.test', localBaseUrl: 'http://127.0.0.1:8081', tokenHash: 'token-hash', @@ -337,207 +367,202 @@ function writeOwnCompanionState(projectRoot: string, pid: number): string { return statePath; } -function errnoException(code: string): NodeJS.ErrnoException { - return Object.assign(new Error(`simulated ${code}`), { code }); -} - -const COMPANION_TUNNEL_COMMAND = `${process.execPath} src/client/companion-tunnel.ts --agent-device-run-metro-companion`; - type CompanionTeardownCase = Readonly<{ name: string; pid: number; processAlive: boolean; processCommand: string | null; - /** One entry per `process.kill` call: an errno to throw, or true when the signal is accepted. */ - killOutcomes: Array; - /** One entry per `waitForProcessExit` call: whether the companion exited in that window. */ - exitOutcomes: boolean[]; - expect: Readonly<{ - rejection?: string; - stopped?: boolean; - killCalls: Array<[number, 'SIGTERM' | 'SIGKILL']>; - exitWaitCalls: Array<[number, number]>; - keepsState: boolean; - }>; + /** What host-kit's stop proves, or the host fault it propagates. */ + stop: ConfirmedProcessStop | { throws: Error }; + /** What the operator has to be able to read off the failure for this verdict. */ + says?: string[]; + keepsState: boolean; }>; +const RETAINED_ON_TERM: ConfirmedProcessStop = { + status: 'retained', + reason: 'signal-failed', + signal: 'SIGTERM', + errno: 'EPERM', +}; + +const IDENTITY_UNVERIFIED: ConfirmedProcessStop = { + status: 'retained', + reason: 'identity-unverified', +}; + const COMPANION_TEARDOWN_CASES: CompanionTeardownCase[] = [ { - // The recorded pid is already gone: nothing to signal, and the artifacts are cleared. - name: 'clears artifacts when the recorded pid is already gone', + name: 'clears artifacts when the host proves the recorded process is gone', pid: 4001, processAlive: false, processCommand: COMPANION_TUNNEL_COMMAND, - killOutcomes: [], - exitOutcomes: [], - expect: { stopped: true, killCalls: [], exitWaitCalls: [], keepsState: false }, + stop: STOPPED, + keepsState: false, }, { - // The process is alive but its command line is unreadable: never signal an unknown pid. - name: 'never signals a pid whose command line is unreadable', + // The #3540 companion: an elevated process this profile cannot describe. Dropping the state here + // is what let a second companion start beside a live one, so the reason has to reach the operator. + name: 'keeps the state when the host cannot verify the companion identity', pid: 4002, processAlive: true, processCommand: null, - killOutcomes: [], - exitOutcomes: [], - expect: { stopped: true, killCalls: [], exitWaitCalls: [], keepsState: false }, + stop: IDENTITY_UNVERIFIED, + says: ['could not be confirmed as the Metro companion', 'no signal was sent'], + keepsState: true, }, { - // SIGTERM is refused (EPERM). The shared stopper reads "signal refused" as already-exited, so - // teardown reports stopped, skips the exit wait and SIGKILL, and drops the state file without - // confirming an exit. EPERM can also mean a companion still running under another owner, which - // is why the daemon's own stopper keeps its registration instead (`signal-failed` in - // src/daemon/daemon-stop.ts); companion teardown keeps the weaker local-dev contract that a pid - // we cannot signal is not ours to keep tracking. - name: 'reads a refused SIGTERM as already exited', + // Reporting `stopped` on EPERM is what used to let a second companion start beside this one. + name: 'keeps the state when the stop reports a refused signal', pid: 4003, processAlive: true, processCommand: COMPANION_TUNNEL_COMMAND, - killOutcomes: ['EPERM'], - exitOutcomes: [], - expect: { - stopped: true, - killCalls: [[4003, 'SIGTERM']], - exitWaitCalls: [], - keepsState: false, - }, - }, - { - // SIGTERM is ignored: escalate to SIGKILL and wait for the exit again. - name: 'escalates an ignored SIGTERM to SIGKILL', - pid: 4004, - processAlive: true, - processCommand: COMPANION_TUNNEL_COMMAND, - killOutcomes: [true, true], - exitOutcomes: [false, true], - expect: { - stopped: true, - killCalls: [ - [4004, 'SIGTERM'], - [4004, 'SIGKILL'], - ], - exitWaitCalls: [ - [4004, 1_000], - [4004, 1_000], - ], - keepsState: false, - }, + stop: RETAINED_ON_TERM, + says: ['refused SIGTERM (EPERM)', 'sudo kill 4003'], + keepsState: true, }, { - // SIGKILL loses the race (ESRCH): stop without waiting on a pid that is already gone. - name: 'skips the second exit wait when SIGKILL loses the race', - pid: 4005, - processAlive: true, - processCommand: COMPANION_TUNNEL_COMMAND, - killOutcomes: [true, 'ESRCH'], - exitOutcomes: [false], - expect: { - stopped: true, - killCalls: [ - [4005, 'SIGTERM'], - [4005, 'SIGKILL'], - ], - exitWaitCalls: [[4005, 1_000]], - keepsState: false, - }, - }, - { - // A signalling failure that is not a race is reported, and the state file survives so the - // companion can be retried instead of being orphaned silently. - name: 'reports a non-race SIGTERM failure and keeps the state for retry', + // A fault that is not a stop outcome is not swallowed either: the state stays for the retry. + name: 'propagates a host fault that carries no errno and keeps the state', pid: 4006, processAlive: true, processCommand: COMPANION_TUNNEL_COMMAND, - killOutcomes: ['EACCES'], - exitOutcomes: [], - expect: { - rejection: 'simulated EACCES', - killCalls: [[4006, 'SIGTERM']], - exitWaitCalls: [], - keepsState: true, - }, - }, - { - // The same holds when only the escalation fails for a non-race reason: the retry stops the - // same still-recorded companion and fails on SIGKILL. - name: 'reports a non-race SIGKILL escalation failure and keeps the state for retry', - pid: 4007, - processAlive: true, - processCommand: COMPANION_TUNNEL_COMMAND, - killOutcomes: [true, 'EACCES'], - exitOutcomes: [false], - expect: { - rejection: 'simulated EACCES', - killCalls: [ - [4007, 'SIGTERM'], - [4007, 'SIGKILL'], - ], - exitWaitCalls: [[4007, 1_000]], - keepsState: true, - }, + stop: { throws: new TypeError('kill refused: bad signal') }, + keepsState: true, }, ]; -// Each case owns its projectRoot, state file, pid, and mock outcomes, so one failure cannot hide -// the later scenarios the way the original chained test allowed. test.each(COMPANION_TEARDOWN_CASES)( - 'stopping a companion survives the teardown race: $name', - async ({ - pid, - processAlive, - processCommand, - killOutcomes, - exitOutcomes, - expect: expectation, - }) => { + 'stopping a companion confirms the exit before dropping state: $name', + async ({ pid, processAlive, processCommand, stop, says, keepsState }) => { const projectRoot = mkdtempForTestSync('agent-device-metro-companion-teardown-'); const statePath = writeOwnCompanionState(projectRoot, pid); try { vi.mocked(isProcessAlive).mockReturnValue(processAlive); vi.mocked(readProcessCommand).mockReturnValue(processCommand); - let killIndex = 0; - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { - const outcome = killOutcomes[killIndex]; - if (outcome === undefined) { - throw new Error(`unexpected process.kill call #${killIndex + 1}`); - } - killIndex += 1; - if (outcome !== true) throw errnoException(outcome); - return true; - }); - let exitIndex = 0; - vi.mocked(waitForProcessExit).mockImplementation(async () => { - const outcome = exitOutcomes[exitIndex]; - if (outcome === undefined) { - throw new Error(`unexpected waitForProcessExit call #${exitIndex + 1}`); - } - exitIndex += 1; - return outcome; - }); - - if (expectation.rejection !== undefined) { + const stopMock = vi.mocked(stopProcessAndConfirmExit); + if ('throws' in stop) stopMock.mockRejectedValue(stop.throws); + else stopMock.mockResolvedValue(stop); + + if ('throws' in stop) { + await assert.rejects(() => stopMetroCompanion({ projectRoot }), stop.throws); + } else if (stop.status === 'retained') { await assert.rejects( () => stopMetroCompanion({ projectRoot }), - new RegExp(expectation.rejection), + (error: unknown) => { + const failure = error as AppError; + const details = failure.details; + assert.equal(details?.reason, stop.reason); + assert.equal(details?.signal, 'signal' in stop ? stop.signal : undefined); + assert.equal(details?.errno, 'errno' in stop ? stop.errno : undefined); + assert.equal(details?.pid, pid); + for (const said of says ?? []) + assert.ok(failure.message.includes(said), failure.message); + return true; + }, ); } else { - assert.equal( - (await stopMetroCompanion({ projectRoot })).stopped, - expectation.stopped === true, - ); + assert.equal((await stopMetroCompanion({ projectRoot })).stopped, true); } - assert.deepEqual(killSpy.mock.calls, expectation.killCalls); - assert.deepEqual( - vi.mocked(waitForProcessExit).mock.calls.map((call) => [call[0], call[1]]), - expectation.exitWaitCalls, - ); - assert.equal(fs.existsSync(statePath), expectation.keepsState); + // Whatever the host went on to prove, the caller handed over the identity it recorded. + assert.deepEqual(stopRequests(), [companionStop(pid, `start-${pid}`)]); + assert.equal(fs.existsSync(statePath), keepsState); } finally { fs.rmSync(projectRoot, { recursive: true, force: true }); } }, ); +test('a companion whose stop was refused keeps its state and is not replaced by a second one', async () => { + // ensure stops before spawning, so an unconfirmed stop aborts the respawn with the state tracked. + const pid = 4101; + const projectRoot = mkdtempForTestSync('agent-device-metro-companion-refused-ensure-'); + const statePath = writeOwnCompanionState(projectRoot, pid); + const before = fs.readFileSync(statePath, 'utf8'); + try { + // The state does not match these options (different launchUrl), so ensure must stop it first. + vi.mocked(isProcessAlive).mockReturnValue(true); + vi.mocked(readProcessCommand).mockReturnValue(COMPANION_TUNNEL_COMMAND); + vi.mocked(readProcessStartTime).mockReturnValue('start-4101'); + vi.mocked(stopProcessAndConfirmExit).mockResolvedValue(RETAINED_ON_TERM); + + const failure = (await ensureMetroCompanion({ + projectRoot, + serverBaseUrl: 'https://bridge.example.test', + bearerToken: 'token', + localBaseUrl: 'http://127.0.0.1:8081', + bridgeScope: TEST_BRIDGE_SCOPE, + launchUrl: 'myapp://second', + consumerKey: 'session-a', + }).catch((error: unknown) => error)) as AppError; + assert.equal(failure.details?.reason, 'signal-failed', String(failure)); + assert.equal(vi.mocked(runCmdDetached).mock.calls.length, 0, 'no second companion was spawned'); + assert.equal(fs.readFileSync(statePath, 'utf8'), before, 'the tracked state is untouched'); + } finally { + fs.rmSync(projectRoot, { recursive: true, force: true }); + } +}); + +test('a half-spawned companion whose stop cannot be confirmed keeps its record', async () => { + // The state write fails once after the spawn produced a pid, so the stop verdict decides whether + // the record goes: an unconfirmed exit keeps it, with the spawn failure as the cause. + const pid = 4201; + const projectRoot = mkdtempForTestSync('agent-device-metro-companion-spawn-cleanup-'); + const statePath = path.join(projectRoot, '.agent-device', 'metro-companion.json'); + try { + vi.mocked(isProcessAlive).mockReturnValue(true); + vi.mocked(readProcessStartTime).mockReturnValue('start-4201'); + vi.mocked(readProcessCommand).mockReturnValue(COMPANION_TUNNEL_COMMAND); + vi.mocked(stopProcessAndConfirmExit).mockResolvedValue(RETAINED_ON_TERM); + vi.mocked(runCmdDetached).mockReturnValue(pid); + const writeFault = Object.assign(new Error('simulated EIO'), { code: 'EIO' }); + const realWriteFileSync = fs.writeFileSync; + let faultPending = true; + vi.spyOn(fs, 'writeFileSync').mockImplementation((( + target: fs.PathOrFileDescriptor, + ...rest + ) => { + if (faultPending && String(target) === statePath) { + faultPending = false; + throw writeFault; + } + return realWriteFileSync(target, ...rest); + }) as typeof fs.writeFileSync); + + const failure = (await ensureMetroCompanion({ + projectRoot, + serverBaseUrl: 'https://bridge.example.test', + bearerToken: 'token', + localBaseUrl: 'http://127.0.0.1:8081', + bridgeScope: TEST_BRIDGE_SCOPE, + consumerKey: 'session-a', + }).catch((error: unknown) => error)) as AppError; + assert.equal(failure.details?.reason, 'signal-failed', String(failure)); + assert.equal(failure.cause, writeFault, 'the spawn failure stays the cause'); + const kept = JSON.parse(fs.readFileSync(statePath, 'utf8')) as { pid: number }; + assert.equal(kept.pid, pid, 'the profile keeps tracking the surviving pid'); + assert.equal(vi.mocked(runCmdDetached).mock.calls.length, 1, 'nothing was spawned twice'); + + // What the kept record is for: a later ensure stops that same pid instead of registering a + // second companion beside the one that refused to die. + vi.mocked(stopProcessAndConfirmExit).mockClear(); + const retry = (await ensureMetroCompanion({ + projectRoot, + serverBaseUrl: 'https://bridge.example.test', + bearerToken: 'token', + localBaseUrl: 'http://127.0.0.1:8081', + bridgeScope: TEST_BRIDGE_SCOPE, + launchUrl: 'myapp://second', + consumerKey: 'session-a', + }).catch((error: unknown) => error)) as AppError; + assert.equal(retry.details?.reason, 'signal-failed', String(retry)); + assert.deepEqual(stopRequests(), [companionStop(pid, `start-${pid}`)]); + assert.equal(vi.mocked(runCmdDetached).mock.calls.length, 1, 'the retry did not spawn either'); + } finally { + fs.rmSync(projectRoot, { recursive: true, force: true }); + } +}); + test('legacy state without bridge scope is stopped before respawn', async () => { const projectRoot = mkdtempForTestSync('agent-device-metro-companion-legacy-'); const statePath = path.join(projectRoot, '.agent-device', 'metro-companion.json'); @@ -562,8 +587,7 @@ test('legacy state without bridge scope is stopped before respawn', async () => vi.mocked(readProcessCommand).mockReturnValue( `${process.execPath} src/client/companion-tunnel.ts --agent-device-run-metro-companion`, ); - vi.mocked(waitForProcessExit).mockResolvedValue(true); - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true); + vi.mocked(stopProcessAndConfirmExit).mockResolvedValue(STOPPED); const spawned = await ensureMetroCompanion({ projectRoot, @@ -577,7 +601,7 @@ test('legacy state without bridge scope is stopped before respawn', async () => assert.equal(spawned.spawned, true); assert.equal(spawned.pid, 666); assert.equal(vi.mocked(runCmdDetached).mock.calls.length, 1); - assert.deepEqual(killSpy.mock.calls[0], [555, 'SIGTERM']); + assert.deepEqual(stopRequests(), [companionStop(555, 'start-555')]); const state = JSON.parse(fs.readFileSync(spawned.statePath, 'utf8')) as { bridgeScope?: unknown; }; diff --git a/src/client/client-companion-tunnel.ts b/src/client/client-companion-tunnel.ts index cbaa9182f3..ca3a83ec23 100644 --- a/src/client/client-companion-tunnel.ts +++ b/src/client/client-companion-tunnel.ts @@ -20,11 +20,15 @@ import { import { normalizeBaseUrl } from './base-url.ts'; import { runCmdDetached } from '@agent-device/host-kit/command'; import { + confirmedProcessStopDetails, + describeUnconfirmedProcessStop, isProcessAlive, readProcessCommand, readProcessStartTime, - waitForProcessExit, + stopProcessAndConfirmExit, + type ConfirmedProcessStop, } from '@agent-device/host-kit/process'; +import { AppError } from '@agent-device/kernel/errors'; const COMPANION_TUNNEL_TERM_TIMEOUT_MS = 1_000; const COMPANION_TUNNEL_KILL_TIMEOUT_MS = 1_000; @@ -280,29 +284,64 @@ function withoutConsumer( }; } +/** Stops the recorded companion, throwing unless the host proves it left. */ async function stopCompanionProcess( state: CompanionTunnelState, definition: CompanionTunnelDefinition, + paths: { statePath: string; logPath: string }, ): Promise { - if (!isProcessAlive(state.pid)) return; - const command = readProcessCommand(state.pid); - if (!command || !isCompanionTunnelCommand(command, definition)) return; - try { - process.kill(state.pid, 'SIGTERM'); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code === 'ESRCH' || code === 'EPERM') return; - throw error; + const outcome = await stopProcessAndConfirmExit( + { + pid: state.pid, + identity: { + startTime: state.startTime, + matches: (command) => isCompanionTunnelCommand(command, definition), + }, + }, + { + termTimeoutMs: COMPANION_TUNNEL_TERM_TIMEOUT_MS, + killTimeoutMs: COMPANION_TUNNEL_KILL_TIMEOUT_MS, + }, + ); + if (outcome.status === 'retained') { + throw companionStopFailure({ ...outcome, pid: state.pid }, definition, paths); } - if (await waitForProcessExit(state.pid, COMPANION_TUNNEL_TERM_TIMEOUT_MS)) return; +} + +/** A companion whose exit could not be confirmed: state stays, so a retry stops the same process. */ +function companionStopFailure( + retained: { pid: number } & Extract, + definition: CompanionTunnelDefinition, + paths: { statePath: string; logPath: string }, +): AppError { + // `identity-unverified` is the one verdict that cannot say "still running": the pid answers + // `kill(pid, 0)`, and on Windows an exited process does too while its object keeps handles. + const why = + retained.reason === 'identity-unverified' + ? `${definition.displayName} ${describeUnconfirmedProcessStop(retained.pid, `the ${definition.displayName} this profile started`)}. Stop it yourself, as its owner if needed.` + : `${definition.displayName} pid ${retained.pid} is still running${ + retained.reason === 'exit-timeout' + ? `: it survived ${retained.signal}.` + : retained.errno === 'EPERM' + ? `: it refused ${retained.signal} (${retained.errno}), so it belongs to another user or was started with elevated privileges. Stop it as that user, e.g. \`sudo kill ${retained.pid}\`.` + : `: it refused ${retained.signal} (${retained.errno}).` + }`; + return new AppError('COMMAND_FAILED', why, { + ...confirmedProcessStopDetails(retained, retained.pid), + statePath: paths.statePath, + logPath: paths.logPath, + hint: `${paths.statePath} still tracks this pid, so a retry acts on that same companion instead of starting a second one.`, + }); +} + +/** Records a half-spawned companion a stop could not confirm, so a retry stops the same pid. */ +function persistUnstoppedCompanion(statePath: string, state: CompanionTunnelState): void { try { - process.kill(state.pid, 'SIGKILL'); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code === 'ESRCH' || code === 'EPERM') return; - throw error; + writeCompanionState(statePath, state); + } catch { + // A pid this host will not record cannot be tracked on a retry; the refusal names it, and the + // caller dispatches on that verdict. } - await waitForProcessExit(state.pid, COMPANION_TUNNEL_KILL_TIMEOUT_MS); } function setOrDelete(env: NodeJS.ProcessEnv, key: string, value: string | undefined): void { @@ -428,8 +467,10 @@ export async function ensureCompanionTunnel( }; } + // Stopping before spawning is what keeps one bridge scope down to one companion: an unconfirmed + // exit aborts here instead of registering a second one beside the first. if (existing) { - await stopCompanionProcess(existing, options.definition); + await stopCompanionProcess(existing, options.definition, paths); clearCompanionArtifacts(paths, options.definition); } @@ -439,8 +480,16 @@ export async function ensureCompanionTunnel( spawned = spawnCompanionProcess(options, paths.logPath); writeCompanionState(paths.statePath, withConsumer(spawned, consumerKey)); } catch (error) { + // An unconfirmed exit of our own half-spawned companion outranks the spawn failure. if (spawned) { - await stopCompanionProcess(spawned, options.definition).catch(() => {}); + try { + await stopCompanionProcess(spawned, options.definition, paths); + } catch (stopError) { + // The failure promises a retry stops the same pid, and the sentinel carries none. + persistUnstoppedCompanion(paths.statePath, spawned); + if (stopError instanceof AppError) stopError.cause = error; + throw stopError; + } } clearCompanionArtifacts(paths, options.definition); throw error; @@ -473,7 +522,7 @@ export async function stopCompanionTunnel( writeCompanionState(paths.statePath, nextState); return { stopped: false, statePath: paths.statePath }; } - await stopCompanionProcess(existing, options.definition); + await stopCompanionProcess(existing, options.definition, paths); clearCompanionArtifacts(paths, options.definition); return { stopped: true, statePath: paths.statePath }; } diff --git a/website/docs/docs/build-an-integration.md b/website/docs/docs/build-an-integration.md index 5146bdd398..0d4c814463 100644 --- a/website/docs/docs/build-an-integration.md +++ b/website/docs/docs/build-an-integration.md @@ -177,6 +177,8 @@ await stopMetroTunnel({ Use `agent-device/remote-config` for profile loading and path resolution, `agent-device/metro` for Metro preparation, reload, and tunnel lifecycle, and `agent-device/contracts` when a server consumer needs daemon request or runtime contract types. For bridged remote Metro, `proxyBaseUrl` is the bridge origin and `publicBaseUrl` is optional; the bridge descriptor supplies cloud iOS wildcard HTTPS hints and Android runtime-route hints. `reloadMetro()` calls Metro's `/reload` endpoint, matching the terminal `r` reload path for connected React Native apps. +`stopMetroTunnel()` releases a companion once per consumer, so it only returns once the companion is gone when it releases the last one. If that companion belongs to another user — you started it with `sudo`, for example — the call fails with its pid, and the profile keeps tracking it so a later stop retries the same process instead of starting a second companion beside it. Stop the surviving process as its owner (`sudo kill `) and run `stopMetroTunnel()` again. + ## Selector helpers Use `agent-device/selectors` to parse and match selector expressions in a remote daemon or bridge. The `role=` term matches the platform-neutral `kind` vocabulary that `snapshot --json` publishes (see [Snapshots](./snapshots.md#structured-node-fields---json)): pass the nodes as captured, and a node whose `kind` is `text` always matches `role=text`. Separately, the deprecated raw leaf spellings (`statictext`, `edittext`, `textview`, …) also match, each on the nodes that actually carried that class. A leaf spelling matches beside such a node's `kind`, not in place of it — `role=webarea` resolves a macOS-helper `AXWebArea` node whose `kind` is `axwebarea`, for example. Matching stays platform-aware because editability checks differ by backend. diff --git a/website/docs/docs/commands.md b/website/docs/docs/commands.md index 7291f3c47d..c7954643dc 100644 --- a/website/docs/docs/commands.md +++ b/website/docs/docs/commands.md @@ -841,6 +841,7 @@ agent-device react-devtools profile report @c5 - `agent-device` global flags work before or after `react-devtools`. Put `--` before downstream flags only when they share an `agent-device` global flag name. - Development builds connect to the DevTools daemon on port 8097. If an Android emulator or device cannot reach the host, run `adb reverse tcp:8097 tcp:8097`. Android `open` of a local Metro URL with a port sets up host reachability; for app/package launches, run `adb reverse tcp:8081 tcp:8081` if the app cannot reach Metro. - Remote bridge sessions (Android and iOS) work through the same command; the CLI keeps a tunnel to the remote DevTools daemon alive until `react-devtools stop` or `disconnect`. See [Remote bridge sessions](/docs/debugging-profiling#remote-bridge-sessions) for the iOS relaunch order and supported React Native versions. +- `react-devtools stop` reports the tunnel's pid and keeps tracking it when it cannot confirm the tunnel exited — it belongs to another user, for example — instead of starting a second one on the next run. Stop that process as its owner and run `react-devtools stop` again. - For cross-platform validation, use separate sessions/devices and restart `react-devtools` between iOS and Android runs. ```bash