From 9d7f73b5f0e8c055174118bcc639b176fa1b7e2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Kwa=C5=9Bniewski?= Date: Tue, 13 Oct 2026 12:40:51 +0200 Subject: [PATCH] fix(client): confirm a companion exit before dropping its state The host refusing SIGTERM was reported as a completed stop, so the profile forgot a live companion and a second one could start beside it. An unreadable identity had the same effect through a different door: on Windows an elevated process answers the command-line query with nothing, and the stop path read that as 'not our companion' and cleared the state. Signals now settle against the host in one place. stopProcessAndConfirmExit takes the identity the caller recorded - the start time and a command matcher - and reports how the process ended: only a released pid, a zombie, or an identity the host contradicts proves the exit; a refusal is retained with its errno. A pid is signalled only where a claim the caller recorded is confirmed by a fact the host will actually say, so two claims resting on a silent host are retained as identity-unverified instead. The companion state is dropped only on 'stopped', and the failure names the pid, the reason, and the state file so a retry stops the same process. The verdict's error projection and the wording a caller must use when nothing was signalled leave the companion builder and sit beside the verdict, so a second caller reports it the same way instead of paraphrasing it. --- .../host-process-stop-confirm.test.ts | 313 +++++++++++++++ .../host-kit/src/internal/host-process.ts | 207 ++++++++++ packages/host-kit/src/process.ts | 8 + src/__tests__/client-metro-companion.test.ts | 364 ++++++++++-------- src/client/client-companion-tunnel.ts | 89 ++++- website/docs/docs/build-an-integration.md | 2 + website/docs/docs/commands.md | 1 + 7 files changed, 794 insertions(+), 190 deletions(-) create mode 100644 packages/host-kit/src/internal/host-process-stop-confirm.test.ts 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