diff --git a/packages/contracts/src/app-log-runtime.ts b/packages/contracts/src/app-log-runtime.ts index 35dcf7f4b8..4e0a2e56a0 100644 --- a/packages/contracts/src/app-log-runtime.ts +++ b/packages/contracts/src/app-log-runtime.ts @@ -110,13 +110,18 @@ export type AppLogBackgroundProcess = AsyncDisposable & export type AppLogProcessCommand = | Readonly<{ kind: 'host'; - request: HostCommandRequest; + /** + * A streamed log tail is stopped by its owner, so a host command here has no deadline to + * honour: the background exec drops `timeoutMs`. It stays out of the type so a producer + * cannot pass a budget that silently never fires. + */ + request: Omit; }> | Readonly<{ kind: 'android-adb'; serial: string; args: readonly string[]; - options?: Pick; + options?: Pick; }>; export type AppLogBackgroundProcessRequest = Readonly<{ diff --git a/packages/host-kit/src/internal/exec-boundary-faults.test.ts b/packages/host-kit/src/internal/exec-boundary-faults.test.ts index dd44fd9610..e35bd590f6 100644 --- a/packages/host-kit/src/internal/exec-boundary-faults.test.ts +++ b/packages/host-kit/src/internal/exec-boundary-faults.test.ts @@ -3,6 +3,10 @@ import { test } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; import { runCmd, + runCmdBackground, + runCmdDetached, + runCmdStreaming, + runCmdSync, withCommandExecutorOverride, type CommandExecutorOverride, } from '@agent-device/host-kit/command'; @@ -28,3 +32,27 @@ test('fail-Nth executor drives one deterministic command failure without hiding assert.deepEqual(calls, [['first'], ['second'], ['third']]); }); + +test('the override seam covers the foreground commands and no other spawn path', async () => { + const consulted: string[] = []; + + await withCommandExecutorOverride( + (command) => { + consulted.push(command); + return undefined; + }, + async () => { + runCmdSync(process.execPath, ['-e', 'process.stdout.write("sync")']); + const background = runCmdBackground(process.execPath, [ + '-e', + 'process.stdout.write("background")', + ]); + await background.wait; + runCmdDetached(process.execPath, ['-e', 'process.exit(0)']); + await runCmdStreaming(process.execPath, ['-e', 'process.stdout.write("streaming")']); + await runCmd(process.execPath, ['-e', 'process.stdout.write("foreground")']); + }, + ); + + assert.deepEqual(consulted, [process.execPath, process.execPath]); +}); diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts new file mode 100644 index 0000000000..86fd1493b8 --- /dev/null +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -0,0 +1,395 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'vitest'; +import { + isCommandTimeoutError, + runCmd, + runCmdBackground, + runCmdStreaming, + signalProcessGroupBestEffort, + type ExecBackgroundOptions, +} from './exec.ts'; +import { shellQuote } from './shell-quote.ts'; +import { sleep } from './timeouts.ts'; +import { mkdtempForTestSync } from './tmp-dir.fixtures.ts'; + +// A direct child can hand our stdout/stderr pipes to a descendant, and `close` waits +// for those pipes to drain: a command this module killed stayed unsettled — holding +// its request and the device lock it owns — until the descendant died by itself. +// `sh` and `sleep` start in milliseconds, so the deadline never races a runtime +// booting, and the leaked holder is a timer, not a runtime. + +const HOLDER_LIFETIME_SECONDS = 3; +const DEADLINE_MS = 400; + +function pipeHolderShellScript(pidFilePath: string): string { + return `sleep ${HOLDER_LIFETIME_SECONDS} & printf %s $! > ${shellQuote(pidFilePath)}; wait`; +} + +function holderPidFilePath(label: string): string { + return path.join(mkdtempForTestSync(`agent-device-exec-${label}-`), 'holder.pid'); +} + +function isProcessRunning(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM is a live process owned by someone else; only ESRCH is absence. + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +async function readRecordedHolderPid(pidFilePath: string): Promise { + const deadline = Date.now() + 2_000; + for (;;) { + try { + const recorded = fs.readFileSync(pidFilePath, 'utf8').trim(); + if (recorded) return Number(recorded); + } catch {} + if (Date.now() > deadline) throw new Error('the pipe holder never recorded its pid'); + await sleep(10); + } +} + +async function waitForProcessToExit(pid: number): Promise { + const deadline = Date.now() + 2_000; + while (isProcessRunning(pid)) { + if (Date.now() > deadline) return false; + await sleep(20); + } + return true; +} + +function settledRejection(promise: Promise): Promise<{ error: unknown } | null> { + return promise.then( + () => null, + (error: unknown) => ({ error }), + ); +} + +test.runIf(process.platform !== 'win32')( + 'runCmd killed at its deadline settles on the child exit, not on an inherited pipe', + async () => { + const pidFilePath = holderPidFilePath('timeout-holder'); + const killed = runCmd('/bin/sh', ['-c', pipeHolderShellScript(pidFilePath)], { + timeoutMs: DEADLINE_MS, + }); + const rejection = settledRejection(killed); + + const holderPid = await readRecordedHolderPid(pidFilePath); + const outcome = await rejection; + + assert.ok(outcome, 'a command killed at its deadline must not resolve'); + assert.ok(isCommandTimeoutError(outcome.error)); + assert.equal( + isProcessRunning(holderPid), + true, + 'settled only once the pipe holder died, which is the wedge this settles before', + ); + }, +); + +test.runIf(process.platform !== 'win32')( + 'a deadline on a detached command kills the descendant that inherited the pipes', + async () => { + const pidFilePath = holderPidFilePath('detached-holder'); + const killed = runCmd('/bin/sh', ['-c', pipeHolderShellScript(pidFilePath)], { + timeoutMs: DEADLINE_MS, + detached: true, + }); + const rejection = settledRejection(killed); + + const holderPid = await readRecordedHolderPid(pidFilePath); + const outcome = await rejection; + + assert.ok(outcome); + assert.ok(isCommandTimeoutError(outcome.error)); + assert.equal( + await waitForProcessToExit(holderPid), + true, + 'the process-group kill left the inherited-pipe holder running', + ); + }, +); + +test.runIf(process.platform !== 'win32')( + 'runCmdBackground killed by request cancellation settles on the child exit', + async () => { + const pidFilePath = holderPidFilePath('abort-holder'); + const controller = new AbortController(); + const { wait } = runCmdBackground('/bin/sh', ['-c', pipeHolderShellScript(pidFilePath)], { + signal: controller.signal, + captureOutput: false, + }); + const rejection = settledRejection(wait); + + const holderPid = await readRecordedHolderPid(pidFilePath); + controller.abort(); + const outcome = await rejection; + + assert.ok(outcome, 'a canceled background command must not resolve'); + assert.equal( + isProcessRunning(holderPid), + true, + 'settled only once the pipe holder died, which is the wedge this settles before', + ); + const details = (outcome.error as { details?: Record }).details; + assert.equal(details?.reason, 'request_canceled'); + }, +); + +test.runIf(process.platform !== 'win32')( + 'a deadline that fires after the child exited settles without waiting for the pipe holder', + async () => { + // The direct child is gone, so no kill can reach the descendant that inherited its + // pipes, and `close` only arrives when that descendant finishes. Settlement has to + // come from the deadline noticing an already-exited child. + const startedAt = Date.now(); + await assert.rejects( + () => runCmd('/bin/sh', ['-c', 'sleep 2 & exit 0'], { timeoutMs: 100 }), + (error: unknown) => { + assert.equal(isCommandTimeoutError(error), true); + return true; + }, + ); + assert.ok(Date.now() - startedAt < 1_000, 'settled only once the pipe holder finished'); + }, + 10_000, +); + +// A command this module asked to be killed is finished once its child is gone, without +// waiting for the stdio pipes to drain: a descendant that inherited them keeps `close` +// from arriving, and the request behind the command — and the device lock it holds — +// would wait forever. Whether the kill request or the child's exit arrives first is not a +// question the callers answer, so both report to one settlement. +// +// The kill paths below address a process group whose leader this worker already reaped, and +// the hermetic signal setup ends a worker's authority over a pid at that moment. So every group +// write is answered by `guardGroupWrites` below, which is the seam that setup points a real kill +// path at: it records what the kill aimed at and answers the way `process.kill` does — `true` for a +// write the kernel accepted, `ESRCH` for a group that is gone, `EPERM` for one that is not ours. + +type GroupWrite = { readonly pid: number; readonly signal: string | number }; + +/** How a guarded group write answers, matching what `process.kill` does with a negative pid. */ +type GroupWriteAnswer = 'delivered' | 'no-such-process' | 'not-permitted'; + +function guardGroupWrites(answer: GroupWriteAnswer = 'delivered'): { + restore: () => void; + writes: GroupWrite[]; +} { + const original = process.kill.bind(process); + const writes: GroupWrite[] = []; + process.kill = ((pid: number, signal: string | number = 'SIGTERM') => { + if (pid < 0) { + writes.push({ pid, signal }); + if (answer === 'no-such-process' || answer === 'not-permitted') { + const error = new Error( + answer === 'no-such-process' ? 'no such process' : 'operation not permitted', + ) as NodeJS.ErrnoException; + error.code = answer === 'no-such-process' ? 'ESRCH' : 'EPERM'; + throw error; + } + return true; + } + return original(pid, signal as NodeJS.Signals); + }) as typeof process.kill; + return { writes, restore: () => (process.kill = original) }; +} + +// One seam, one double. Every group write in this file — the direct calls below included — is answered +// by `guardGroupWrites`, so a hand-written spy beside it would be a second answer to the same question, +// and the two are free to drift from each other. +test('the group signal seam answers each way process.kill answers a negative pid', () => { + // `true` once the kernel accepted the write; `ESRCH` when no member is left and `EPERM` when a + // member belongs to another user. The two throws are the same answer to this seam — nothing was + // reached, so the caller must not keep waiting on a pipe holder it just asked to be killed. + const cases = [ + ['delivered', true], + ['no-such-process', false], + ['not-permitted', false], + ] as const; + for (const [answer, reported] of cases) { + const groupWrites = guardGroupWrites(answer); + try { + assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), reported, answer); + assert.deepEqual(groupWrites.writes, [{ pid: -101, signal: 'SIGKILL' }], answer); + } finally { + groupWrites.restore(); + } + } +}); + +test('an invalid pid is refused before anything is signalled', () => { + // A zero or negative pid would address this worker's own group, or every process the user owns. The + // guard records every write it is asked about, so an empty list is the proof none was attempted. + const groupWrites = guardGroupWrites(); + try { + assert.equal(signalProcessGroupBestEffort(0, 'SIGTERM'), false); + assert.equal(signalProcessGroupBestEffort(-1, 'SIGTERM'), false); + assert.equal(signalProcessGroupBestEffort(1.5, 'SIGTERM'), false); + assert.deepEqual(groupWrites.writes, []); + } finally { + groupWrites.restore(); + } +}); + +test.runIf(process.platform !== 'win32')( + 'a detached deadline still kills the group its reaped child left behind', + async () => { + const groupWrites = guardGroupWrites(); + let childPid = 0; + try { + const startedAt = Date.now(); + await assert.rejects( + () => + runCmdStreaming('/bin/sh', ['-c', 'sleep 2 & exit 0'], { + detached: true, + timeoutMs: 100, + onSpawn: (child) => { + childPid = child.pid ?? 0; + }, + }), + (error: unknown) => { + assert.equal(isCommandTimeoutError(error), true); + return true; + }, + ); + assert.ok(Date.now() - startedAt < 1_000, 'settled only once the pipe holder finished'); + assert.deepEqual(groupWrites.writes, [{ pid: -childPid, signal: 'SIGKILL' }]); + } finally { + groupWrites.restore(); + } + }, + 10_000, +); + +test.runIf(process.platform !== 'win32')( + 'a detached deadline whose group cannot be signalled still settles', + async () => { + // A vanished group answers the group write by throwing `ESRCH`, and a group owned by someone + // else by throwing `EPERM`; the seam swallows both. The command still cannot wait on a pipe + // holder it just asked to be killed. + const groupWrites = guardGroupWrites('no-such-process'); + try { + const startedAt = Date.now(); + await assert.rejects( + () => runCmd('/bin/sh', ['-c', 'sleep 2 & exit 0'], { detached: true, timeoutMs: 100 }), + (error: unknown) => { + assert.equal(isCommandTimeoutError(error), true); + return true; + }, + ); + assert.ok(Date.now() - startedAt < 1_000, 'settled only once the pipe holder finished'); + assert.equal(groupWrites.writes.length, 1); + } finally { + groupWrites.restore(); + } + }, + 10_000, +); + +test.runIf(process.platform !== 'win32')( + 'a request that was already canceled kills the command it arrives on', + async () => { + // The kill is issued before the caller finishes wiring, so a settlement that read + // the watcher mid-construction would fail here rather than at the next await. + const controller = new AbortController(); + controller.abort(); + await assert.rejects( + () => runCmd('/bin/sh', ['-c', 'sleep 5'], { signal: controller.signal }), + (error: unknown) => { + assert.equal( + (error as { details?: Record }).details?.reason, + 'request_canceled', + ); + return true; + }, + ); + }, + 5_000, +); + +test.runIf(process.platform !== 'win32')( + 'a background request that was already canceled ends its wait', + async () => { + const controller = new AbortController(); + controller.abort(); + const background = runCmdBackground('/bin/sh', ['-c', 'sleep 5'], { + signal: controller.signal, + }); + + await assert.rejects( + () => background.wait, + (error: unknown) => { + assert.equal( + (error as { details?: Record }).details?.reason, + 'request_canceled', + ); + return true; + }, + ); + }, + 5_000, +); + +test.runIf(process.platform !== 'win32')( + 'runCmd that was never killed still drains output a descendant writes after its parent exited', + async () => { + const result = await runCmd('/bin/sh', ['-c', 'printf head; { sleep 0.2; printf tail; } &']); + + assert.equal(result.stdout, 'headtail'); + }, +); + +test('a killed command still fails with its deadline even when it allowed failure', async () => { + const outcome = await runCmd(process.execPath, ['-e', 'setTimeout(() => {}, 10_000)'], { + timeoutMs: 60, + allowFailure: true, + }).then( + () => null, + (error: unknown) => error, + ); + + assert.ok(isCommandTimeoutError(outcome)); +}); + +test('binaryStdout returns every byte the command wrote', async () => { + const bytes = 4096; + const result = await runCmd( + process.execPath, + ['-e', 'process.stdout.write(Buffer.alloc(4096, 7))'], + { binaryStdout: true }, + ); + + assert.equal(result.stdout, ''); + assert.equal(result.stdoutBuffer?.length, bytes); +}); + +test('runCmdBackground captures the full stdout of a child that writes over a megabyte', async () => { + const bytes = 1_500_000; + const { wait } = runCmdBackground(process.execPath, [ + '-e', + `process.stdout.write("a".repeat(${bytes}))`, + ]); + + const result = await wait; + + assert.equal(result.stdout.length, bytes); +}); + +test('background exec arms no deadline when timeoutMs crosses an unchecked options spread', async () => { + const leakedOptions = { timeoutMs: 20 } as unknown as ExecBackgroundOptions; + + const { wait } = runCmdBackground( + process.execPath, + ['-e', 'setTimeout(() => process.exit(0), 150)'], + leakedOptions, + ); + + const result = await wait; + + assert.equal(result.exitCode, 0); +}); diff --git a/packages/host-kit/src/internal/exec.ts b/packages/host-kit/src/internal/exec.ts index 46d6d4bc6e..8c90a476d5 100644 --- a/packages/host-kit/src/internal/exec.ts +++ b/packages/host-kit/src/internal/exec.ts @@ -56,7 +56,15 @@ export type ExecDetachedProcess = { exited: Promise; }; -export type ExecBackgroundOptions = ExecOptions & { +/** + * Background runs have no `timeoutMs`: the callers are long-lived sessions (the + * Android snapshot helper, the keep-hot xcodebuild runner, app-log capture), and + * a deadline field the spawn path never armed was one plumbing change away from + * killing them. A background deadline belongs to its caller, which cancels it with + * `signal`; a caller that kills the child directly still waits for the streams to + * drain, exactly as before. + */ +export type ExecBackgroundOptions = Omit & { /** * Capture stdout/stderr into the wait result when the child has piped stdio. * Set false when the caller owns, ignores, or forwards the streams. @@ -152,13 +160,62 @@ function runSpawnedCommand( let stderr = ''; let didTimeout = false; const timeoutMs = normalizeTimeoutMs(options.timeoutMs); - const timeoutHandle = timeoutMs + let timeoutHandle: NodeJS.Timeout | null = null; + let settled = false; + function finish(): boolean { + if (settled) return false; + settled = true; + if (timeoutHandle) clearTimeout(timeoutHandle); + abort.dispose(); + destroyCommandStreams(child); + execTrace.emitForegroundCompletion(cmd, args); + return true; + } + function fail(error: AppError): void { + if (finish()) reject(error); + } + function settle(code: number | null): void { + if (!finish()) return; + const finalExitCode = code ?? 1; + if (!abort.didAbort && didTimeout && timeoutMs) { + reject(createTimeoutError(executable, cmd, args, timeoutMs, finalExitCode, stdout, stderr)); + return; + } + const failure = commandCloseFailure( + abort, + executable, + cmd, + args, + finalExitCode, + options.allowFailure, + stdout, + stderr, + ); + if (failure) { + reject(failure); + return; + } + resolve({ + stdout, + stderr, + exitCode: finalExitCode, + stdoutBuffer: stdoutChunks ? Buffer.concat(stdoutChunks) : undefined, + }); + } + // A deadline that fires after the child exited on its own still has to settle: the + // group kill that would have ended the pipe holder can no longer run through a child + // Node already reaped. + const settlement = createCommandKillSettlement({ + killProcessTree: () => killProcessTree(child, options.detached), + settle, + }); + const abort = watchCommandAbort(options, settlement.requestKill); + timeoutHandle = timeoutMs ? setTimeout(() => { didTimeout = true; - killProcessTree(child, options.detached); + settlement.requestKill(); }, timeoutMs) : null; - const abort = watchCommandAbort(child, options); if (!options.binaryStdout) child.stdout.setEncoding('utf8'); child.stderr.setEncoding('utf8'); @@ -166,7 +223,7 @@ function runSpawnedCommand( void writeChildStdin(child, options.stdin).catch((error: unknown) => { if (abort.didAbort || didTimeout) return; if (isEpipeError(error)) return; - reject(createStdinError(executable, cmd, args, error)); + fail(createStdinError(executable, cmd, args, error)); killProcessTree(child, options.detached); }); @@ -187,42 +244,11 @@ function runSpawnedCommand( }); child.on('error', (err) => { - if (timeoutHandle) clearTimeout(timeoutHandle); - abort.dispose(); - execTrace.emitForegroundCompletion(cmd, args); - reject(spawnRejectionError(abort, executable, cmd, args, err)); + fail(spawnRejectionError(abort, executable, cmd, args, err)); }); - child.on('close', (code) => { - if (timeoutHandle) clearTimeout(timeoutHandle); - abort.dispose(); - execTrace.emitForegroundCompletion(cmd, args); - const exitCode = code ?? 1; - if (!abort.didAbort && didTimeout && timeoutMs) { - reject(createTimeoutError(executable, cmd, args, timeoutMs, exitCode, stdout, stderr)); - return; - } - const failure = commandCloseFailure( - abort, - executable, - cmd, - args, - exitCode, - options.allowFailure, - stdout, - stderr, - ); - if (failure) { - reject(failure); - return; - } - resolve({ - stdout, - stderr, - exitCode, - stdoutBuffer: stdoutChunks ? Buffer.concat(stdoutChunks) : undefined, - }); - }); + child.once('exit', settlement.recordExit); + child.once('close', settle); }); } @@ -397,7 +423,6 @@ export function runCmdBackground( let stdout = ''; let stderr = ''; const captureOutput = options.captureOutput ?? true; - const abort = watchCommandAbort(child, options); if (captureOutput) { child.stdout?.setEncoding('utf8'); @@ -412,21 +437,24 @@ export function runCmdBackground( } const wait = new Promise((resolve, reject) => { - child.on('error', (err) => { + let settled = false; + function finish(event: 'error' | 'exit'): boolean { + if (settled) return false; + settled = true; abort.dispose(); - execTrace.emitBackgroundCompletion(cmd, args, 'error'); - reject(spawnRejectionError(abort, executable, cmd, args, err)); - }); - child.on('close', (code) => { - abort.dispose(); - execTrace.emitBackgroundCompletion(cmd, args, 'exit'); - const exitCode = code ?? 1; + destroyCommandStreams(child); + execTrace.emitBackgroundCompletion(cmd, args, event); + return true; + } + function settle(code: number | null): void { + if (!finish('exit')) return; + const finalExitCode = code ?? 1; const failure = commandCloseFailure( abort, executable, cmd, args, - exitCode, + finalExitCode, options.allowFailure, stdout, stderr, @@ -435,8 +463,18 @@ export function runCmdBackground( reject(failure); return; } - resolve({ stdout, stderr, exitCode }); + resolve({ stdout, stderr, exitCode: finalExitCode }); + } + const settlement = createCommandKillSettlement({ + killProcessTree: () => killProcessTree(child, options.detached), + settle, + }); + const abort = watchCommandAbort(options, settlement.requestKill); + child.on('error', (err) => { + if (finish('error')) reject(spawnRejectionError(abort, executable, cmd, args, err)); }); + child.once('exit', settlement.recordExit); + child.once('close', settle); }); return { child, wait }; @@ -769,14 +807,52 @@ function normalizeTimeoutMs(value: number | undefined): number | undefined { return timeout; } +/** + * A command this module asked to be killed is finished once its child is gone, without + * waiting for the stdio pipes to drain: a descendant that inherited them keeps `close` + * from arriving, and the request behind the command — and the device lock it holds — + * would wait forever. Whether the kill request or the child's exit arrives first is not + * a question each caller should answer, so both report here and settlement happens once. + */ +type CommandKillSettlement = { + /** Signals the command's process tree, then settles the command if its child is gone. */ + readonly requestKill: () => void; + /** Records the child's exit, then settles the command if a kill was already requested. */ + readonly recordExit: (code: number | null) => void; +}; + +function createCommandKillSettlement(input: { + readonly killProcessTree: () => void; + readonly settle: (exitCode: number | null) => void; +}): CommandKillSettlement { + let killRequested = false; + let exited = false; + let exitCode: number | null = null; + const settleIfKilledAndGone = (): void => { + if (killRequested && exited) input.settle(exitCode); + }; + return { + requestKill: () => { + killRequested = true; + input.killProcessTree(); + settleIfKilledAndGone(); + }, + recordExit: (code) => { + exited = true; + exitCode = code ?? 1; + settleIfKilledAndGone(); + }, + }; +} + function watchCommandAbort( - child: ChildProcess, options: Pick, + onKill: () => void, ): { readonly didAbort: boolean; dispose: () => void } { let didAbort = false; const onAbort = () => { didAbort = true; - killProcessTree(child, options.detached); + onKill(); }; if (options.signal?.aborted) { onAbort(); @@ -793,16 +869,57 @@ function watchCommandAbort( }; } +/** + * Signals the process group led by `pid` — the tree a detached child spawned — best-effort, + * and reports whether the write went through. One seam for every group kill in host-kit, so + * a caller outside this module can mock it instead of delivering a real signal to a + * fabricated pid (#1824). `host-process.ts` reaches it from here rather than the reverse: + * that module imports `exec.ts` for `runCmd`, and a value import back up would close a cycle + * the layering rules reject. + * + * A pid that is not a positive integer is refused without signalling: `0` would address this + * process's own group, and a negative one every process this user owns. + */ +export function signalProcessGroupBestEffort(pid: number, signal: NodeJS.Signals): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(-pid, signal); + return true; + } catch { + return false; + } +} + +/** + * A detached command owns a process group, and the descendants we are trying to reach are + * its members — which is what keeps the group id reserved. So the group is still signalled + * after the direct child is reaped: those members are holding the pipes this command is + * waiting on. The one group-signal seam reports whether anything was reached rather than + * throwing, and a group that is gone or not ours to signal is the case it reports false. + */ function killProcessTree(child: ChildProcess, detached: boolean | undefined): void { if (detached && child.pid && process.platform !== 'win32') { - try { - process.kill(-child.pid, 'SIGKILL'); - return; - } catch {} + signalProcessGroupBestEffort(child.pid, 'SIGKILL'); + return; } + // A non-detached child leaves its pid free for the kernel to hand to an unrelated + // process once Node has reaped it, so a late signal from a stale deadline could + // strike a stranger. Nothing waits for a kill of a child that is already gone: + // settlement happens on `exit`. + if (child.exitCode !== null || child.signalCode !== null) return; child.kill('SIGKILL'); } +/** + * A kill that cannot reach an inherited-pipe holder must at least stop this process + * from holding the other end of those pipes open after it has settled. + */ +function destroyCommandStreams(child: ChildProcess): void { + child.stdin?.destroy(); + child.stdout?.destroy(); + child.stderr?.destroy(); +} + async function writeChildStdin( child: ChildProcess, stdin: string | Buffer | undefined, diff --git a/packages/host-kit/src/internal/host-process.test.ts b/packages/host-kit/src/internal/host-process.test.ts index 6236d74b4d..887e494b1e 100644 --- a/packages/host-kit/src/internal/host-process.test.ts +++ b/packages/host-kit/src/internal/host-process.test.ts @@ -9,7 +9,6 @@ import { readProcessCommand, readProcessStartTime, signalPidsBestEffort, - signalProcessGroupBestEffort, stopPidsWithEscalation, uniquePositivePids, } from './host-process.ts'; @@ -123,40 +122,6 @@ test('best-effort signaling ignores invalid, current, and failed pids', () => { } }); -test('group signaling addresses the negative pid and reports delivery', () => { - const calls: Array<{ pid: number; signal: string | number | undefined }> = []; - const killSpy = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => { - calls.push({ pid: Number(pid), signal }); - return true; - }); - - try { - assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), true); - assert.deepEqual(calls, [{ pid: -101, signal: 'SIGKILL' }]); - } finally { - killSpy.mockRestore(); - } -}); - -test('group signaling reports a vanished group and never signals an invalid pid', () => { - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { - const error = new Error('not found') as NodeJS.ErrnoException; - error.code = 'ESRCH'; - throw error; - }); - - try { - assert.equal(signalProcessGroupBestEffort(101, 'SIGTERM'), false); - assert.equal(signalProcessGroupBestEffort(0, 'SIGTERM'), false); - assert.equal(signalProcessGroupBestEffort(-1, 'SIGTERM'), false); - // A zero or negative pid would address the caller's own group, or every - // process the user owns, so it must not reach process.kill at all. - assert.equal(killSpy.mock.calls.length, 1); - } finally { - killSpy.mockRestore(); - } -}); - test('pid escalation sends TERM, then KILL only to live pids', async () => { vi.useFakeTimers(); const alivePids = new Set([101, 202, 303]); diff --git a/packages/host-kit/src/internal/host-process.ts b/packages/host-kit/src/internal/host-process.ts index af6d8022ba..956eb5c706 100644 --- a/packages/host-kit/src/internal/host-process.ts +++ b/packages/host-kit/src/internal/host-process.ts @@ -267,23 +267,6 @@ export function signalPidsBestEffort( return signaled; } -/** - * Signals the process group led by `pid` (the tree a detached child spawned), - * best-effort. Lives beside `signalPidsBestEffort` so a runner-tree kill has one - * seam for both writes, and a unit test that mocks this module's liveness reads - * mocks the signal writes in the same place instead of delivering a real signal - * to a fabricated pid (#1824). - */ -export function signalProcessGroupBestEffort(pid: number, signal: NodeJS.Signals): boolean { - if (!Number.isInteger(pid) || pid <= 0) return false; - try { - process.kill(-pid, signal); - return true; - } catch { - return false; - } -} - export async function waitForProcessExit(pid: number, timeoutMs: number): Promise { if (!isProcessAlive(pid)) return true; const start = Date.now(); diff --git a/packages/host-kit/src/process.ts b/packages/host-kit/src/process.ts index e292cb0ef1..3504fdee89 100644 --- a/packages/host-kit/src/process.ts +++ b/packages/host-kit/src/process.ts @@ -19,12 +19,12 @@ export { readProcessIdentityFacts, readProcessStartTime, signalPidsBestEffort, - signalProcessGroupBestEffort, stopPidsWithEscalation, uniquePositivePids, waitForProcessExit, writeHostStderr, } from './internal/host-process.ts'; +export { signalProcessGroupBestEffort } from './internal/exec.ts'; export { reapOwnedProcessRecordsAtStartup } from './internal/owned-process-reaper.ts'; export { createOwnedProcessRecordStore, diff --git a/packages/platform-android/src/__tests__/test-utils/android-host-test-setup.ts b/packages/platform-android/src/__tests__/test-utils/android-host-test-setup.ts index e8718b4871..60e1f7f680 100644 --- a/packages/platform-android/src/__tests__/test-utils/android-host-test-setup.ts +++ b/packages/platform-android/src/__tests__/test-utils/android-host-test-setup.ts @@ -31,7 +31,8 @@ export function bindAndroidAdbTestHost() { void background.wait.catch(() => {}); return background.child; }, - execHostAdb: async (args, options) => await runCmd('adb', args, options), + execHostAdb: async (args, options) => + await runCmd('adb', args, { ...options, detached: process.platform !== 'win32' }), withAdbCommandExecutorOverride: withCommandExecutorOverride, withoutAdbCommandExecutorOverride: withoutCommandExecutorOverride, coerceAdbResult: coerceExecResult, diff --git a/packages/platform-android/src/adb-transport.ts b/packages/platform-android/src/adb-transport.ts index e6284b4b09..37945ebfdf 100644 --- a/packages/platform-android/src/adb-transport.ts +++ b/packages/platform-android/src/adb-transport.ts @@ -30,7 +30,13 @@ export type AndroidAdbExecutorResult = { /** Structural mirror of node's StdioOptions; R13 bars the child_process import that names it. */ type AndroidAdbStdioOption = 'overlapped' | 'pipe' | 'ignore' | 'inherit'; -export type AndroidAdbSpawnOptions = AndroidAdbExecutorOptions & { +/** + * A spawned adb process is long-lived — the snapshot helper session rides it for + * the whole session — so `timeoutMs` is not part of its options: background + * spawns arm no deadline, and a field that looked like one invited callers to + * kill their own helper. + */ +export type AndroidAdbSpawnOptions = Omit & { cwd?: string; detached?: boolean; /** Max stdout/stderr bytes for synchronous runs (default Node ~1MB). */ diff --git a/packages/platform-apple/src/runner/host.ts b/packages/platform-apple/src/runner/host.ts index c3f5b9c15b..20eea8b285 100644 --- a/packages/platform-apple/src/runner/host.ts +++ b/packages/platform-apple/src/runner/host.ts @@ -63,7 +63,8 @@ export type ExecBackgroundResult = { wait: Promise; }; -export type ExecBackgroundOptions = ExecOptions; +/** Mirrors host-kit: a background runner process is never on a spawn deadline. */ +export type ExecBackgroundOptions = Omit; export type Deadline = { remainingMs(nowMs?: number): number; diff --git a/src/platform-runtime-android-adb-host.test.ts b/src/platform-runtime-android-adb-host.test.ts index 27ff6068dc..1717c55988 100644 --- a/src/platform-runtime-android-adb-host.test.ts +++ b/src/platform-runtime-android-adb-host.test.ts @@ -7,148 +7,211 @@ import { createLocalAndroidAdbProvider, runAndroidHostAdb, } from '@agent-device/platform-android/mechanics'; +import { ANDROID_EMULATOR } from './__tests__/test-utils/device-fixtures.ts'; import { mkdtempForTestSync } from './__tests__/test-utils/tmp-dir.ts'; import './platform-runtime-android-adb-host.ts'; +/** + * Publishes a fake `adb` on PATH for the duration of `run`. Anything the script needs + * from the test — a path, a port — arrives through `env`, never spliced into the source + * the fake is built from. + */ +async function withFakeAdbOnPath( + scriptBody: string, + run: () => Promise, + env: Record = {}, +): Promise { + const tmpDir = mkdtempForTestSync('agent-device-adb-host-binding-'); + const adbPath = path.join(tmpDir, 'adb'); + fs.writeFileSync(adbPath, `#!/usr/bin/env node\n${scriptBody}`); + fs.chmodSync(adbPath, 0o755); + const previousPath = process.env.PATH; + const previousEnv = new Map( + Object.keys(env).map((key) => [key, process.env[key] as string | undefined]), + ); + process.env.PATH = `${tmpDir}${path.delimiter}${previousPath ?? ''}`; + for (const [key, value] of Object.entries(env)) process.env[key] = value; + try { + return await run(); + } finally { + if (previousPath === undefined) delete process.env.PATH; + else process.env.PATH = previousPath; + for (const [key, value] of previousEnv) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } +} + test.skipIf(process.platform === 'win32')( 'the local host binding classifies a real nonzero adb process result', async () => { - const tmpDir = mkdtempForTestSync('agent-device-adb-host-binding-'); - const adbPath = path.join(tmpDir, 'adb'); - fs.writeFileSync( - adbPath, - '#!/usr/bin/env node\nprocess.stderr.write("error: device offline\\n"); process.exit(1);', - ); - fs.chmodSync(adbPath, 0o755); - const previousPath = process.env.PATH; - process.env.PATH = `${tmpDir}${path.delimiter}${previousPath ?? ''}`; - try { - const error = await runAndroidHostAdb(['devices']).then( - () => assert.fail('expected local adb to reject'), - (error: unknown) => error, - ); + await withFakeAdbOnPath( + String.raw`process.stderr.write("error: device offline\n"); process.exit(1);`, + async () => { + const error = await runAndroidHostAdb(['devices']).then( + () => assert.fail('expected local adb to reject'), + (error: unknown) => error, + ); - assert.ok(error instanceof AppError); - assert.equal(error.details?.adbFailure, 'device_offline'); - assert.equal(error.details?.retriable, true); - assert.match(String(error.details?.hint), /adb reconnect/i); - } finally { - if (previousPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = previousPath; - } - } + assert.ok(error instanceof AppError); + assert.equal(error.details?.adbFailure, 'device_offline'); + assert.equal(error.details?.retriable, true); + assert.match(String(error.details?.hint), /adb reconnect/i); + }, + ); }, ); test.skipIf(process.platform === 'win32')( 'the root host lowers request-local adb server ports without mutating process env', async () => { - const tmpDir = mkdtempForTestSync('agent-device-adb-server-port-'); - const adbPath = path.join(tmpDir, 'adb'); - fs.writeFileSync( - adbPath, - '#!/usr/bin/env node\n' + - 'process.stdout.write(JSON.stringify({args: process.argv.slice(2), port: process.env.ANDROID_ADB_SERVER_PORT ?? null, address: process.env.ANDROID_ADB_SERVER_ADDRESS ?? null, socket: process.env.ADB_SERVER_SOCKET ?? null}));\n', - ); - fs.chmodSync(adbPath, 0o755); - const previousPath = process.env.PATH; const previousPort = process.env.ANDROID_ADB_SERVER_PORT; const previousSocket = process.env.ADB_SERVER_SOCKET; process.env.ADB_SERVER_SOCKET = 'tcp:inherited.example:9999'; - process.env.PATH = `${tmpDir}${path.delimiter}${previousPath ?? ''}`; try { - const provider = createLocalAndroidAdbProvider( - { - platform: 'android', - id: 'emulator-5554', - name: 'Pixel Emulator', - kind: 'emulator', - booted: true, + await withFakeAdbOnPath( + 'process.stdout.write(JSON.stringify({args: process.argv.slice(2), port: process.env.ANDROID_ADB_SERVER_PORT ?? null, address: process.env.ANDROID_ADB_SERVER_ADDRESS ?? null, socket: process.env.ADB_SERVER_SOCKET ?? null}));', + async () => { + const provider = createLocalAndroidAdbProvider(ANDROID_EMULATOR, { + serverPort: 15_037, + }); + const adb = provider.exec; + const serial = JSON.parse((await adb(['shell', 'id'])).stdout) as { + args: string[]; + port: string | null; + }; + const serialWithWrongPort = JSON.parse( + (await adb(['-P', '9999', 'shell', 'id'])).stdout, + ) as { args: string[]; port: string | null }; + const serialWithWrongEnvironment = JSON.parse( + ( + await adb(['shell', 'id'], { + env: { + ANDROID_ADB_SERVER_PORT: '9999', + ANDROID_ADB_SERVER_ADDRESS: 'foreign.example', + ADB_SERVER_SOCKET: 'tcp:foreign.example:9999', + }, + }) + ).stdout, + ) as { args: string[]; port: string | null }; + const host = JSON.parse( + (await runAndroidHostAdb(['-P', '9999', 'devices'], { serverPort: 15_038 })).stdout, + ) as { args: string[]; port: string | null }; + for (const selector of [ + ['-H', 'foreign.example'], + ['-L', 'tcp:foreign.example:5037'], + ['-t', '42'], + ['-s', 'foreign-device'], + ['-P9999'], + ['-d'], + ['-e'], + ['nodaemon', '-H', 'foreign.example'], + ['server', '-P', '9999'], + ['fork-server', '-s', 'foreign-device'], + ['kill-server'], + ['start-server'], + ['connect', 'foreign.example'], + ['disconnect'], + ['reconnect', 'offline'], + ['attach', 'foreign-device'], + ['detach', 'foreign-device'], + ['pair', 'foreign.example', '123456'], + ['wait-for-device', 'kill-server'], + ['wait-for-device', 'disconnect'], + ['wait-for-any-device', 'pair', 'foreign.example', '123456'], + ]) { + await assert.rejects(adb([...selector, 'shell', 'id']), { + details: { reason: 'managed-device-transport-mismatch' }, + }); + assert.throws(() => provider.spawn?.([...selector, 'shell', 'id']), { + details: { reason: 'managed-device-transport-mismatch' }, + }); + } + + assert.deepEqual(serial, { + args: ['-P', '15037', '-s', 'emulator-5554', 'shell', 'id'], + port: '15037', + address: '127.0.0.1', + socket: null, + }); + assert.deepEqual(serialWithWrongPort, serial); + assert.deepEqual(serialWithWrongEnvironment, serial); + const waited = JSON.parse((await adb(['wait-for-device', 'shell', 'id'])).stdout); + assert.deepEqual(waited, { + ...serial, + args: ['-P', '15037', '-s', 'emulator-5554', 'wait-for-device', 'shell', 'id'], + }); + assert.deepEqual(host, { + args: ['-P', '15038', 'devices'], + port: '15038', + address: '127.0.0.1', + socket: null, + }); + assert.equal(process.env.ANDROID_ADB_SERVER_PORT, previousPort); + assert.equal(process.env.ADB_SERVER_SOCKET, 'tcp:inherited.example:9999'); }, - { serverPort: 15_037 }, ); - const adb = provider.exec; - const serial = JSON.parse((await adb(['shell', 'id'])).stdout) as { - args: string[]; - port: string | null; - }; - const serialWithWrongPort = JSON.parse((await adb(['-P', '9999', 'shell', 'id'])).stdout) as { - args: string[]; - port: string | null; - }; - const serialWithWrongEnvironment = JSON.parse( - ( - await adb(['shell', 'id'], { - env: { - ANDROID_ADB_SERVER_PORT: '9999', - ANDROID_ADB_SERVER_ADDRESS: 'foreign.example', - ADB_SERVER_SOCKET: 'tcp:foreign.example:9999', - }, - }) - ).stdout, - ) as { args: string[]; port: string | null }; - const host = JSON.parse( - (await runAndroidHostAdb(['-P', '9999', 'devices'], { serverPort: 15_038 })).stdout, - ) as { args: string[]; port: string | null }; - for (const selector of [ - ['-H', 'foreign.example'], - ['-L', 'tcp:foreign.example:5037'], - ['-t', '42'], - ['-s', 'foreign-device'], - ['-P9999'], - ['-d'], - ['-e'], - ['nodaemon', '-H', 'foreign.example'], - ['server', '-P', '9999'], - ['fork-server', '-s', 'foreign-device'], - ['kill-server'], - ['start-server'], - ['connect', 'foreign.example'], - ['disconnect'], - ['reconnect', 'offline'], - ['attach', 'foreign-device'], - ['detach', 'foreign-device'], - ['pair', 'foreign.example', '123456'], - ['wait-for-device', 'kill-server'], - ['wait-for-device', 'disconnect'], - ['wait-for-any-device', 'pair', 'foreign.example', '123456'], - ]) { - await assert.rejects(adb([...selector, 'shell', 'id']), { - details: { reason: 'managed-device-transport-mismatch' }, - }); - assert.throws(() => provider.spawn?.([...selector, 'shell', 'id']), { - details: { reason: 'managed-device-transport-mismatch' }, - }); - } - - assert.deepEqual(serial, { - args: ['-P', '15037', '-s', 'emulator-5554', 'shell', 'id'], - port: '15037', - address: '127.0.0.1', - socket: null, - }); - assert.deepEqual(serialWithWrongPort, serial); - assert.deepEqual(serialWithWrongEnvironment, serial); - const waited = JSON.parse((await adb(['wait-for-device', 'shell', 'id'])).stdout); - assert.deepEqual(waited, { - ...serial, - args: ['-P', '15037', '-s', 'emulator-5554', 'wait-for-device', 'shell', 'id'], - }); - assert.deepEqual(host, { - args: ['-P', '15038', 'devices'], - port: '15038', - address: '127.0.0.1', - socket: null, - }); - assert.equal(process.env.ANDROID_ADB_SERVER_PORT, previousPort); - assert.equal(process.env.ADB_SERVER_SOCKET, 'tcp:inherited.example:9999'); } finally { if (previousSocket === undefined) delete process.env.ADB_SERVER_SOCKET; else process.env.ADB_SERVER_SOCKET = previousSocket; - if (previousPath === undefined) delete process.env.PATH; - else process.env.PATH = previousPath; } }, ); + +test.skipIf(process.platform === 'win32')( + 'host adb runs in its own process group so a deadline reaches adb fork-server descendants', + async () => { + // adb starts a fork-server and talks through it. Killing only the `adb` client + // leaves that server holding the inherited stdio pipes, so the group-wide kill a + // `detached` spawn enables is what ends the request. + const reported = await withFakeAdbOnPath( + [ + 'let ownGroup = false;', + 'try { process.kill(-process.pid, 0); ownGroup = true; } catch {}', + 'process.stdout.write(JSON.stringify({ ownGroup }));', + ].join('\n'), + async () => await runAndroidHostAdb(['devices'], { timeoutMs: 3_000 }), + ); + + assert.equal((JSON.parse(reported.stdout) as { ownGroup: boolean }).ownGroup, true); + }, +); + +test.skipIf(process.platform === 'win32')( + 'a background adb spawn outlives a timeoutMs that crossed the provider options spread', + async () => { + const markerPath = path.join( + mkdtempForTestSync('agent-device-adb-spawn-deadline-'), + 'helper-session-ended', + ); + const outcome = await withFakeAdbOnPath( + [ + 'const fs = require("node:fs");', + 'const markerPath = process.env.FAKE_ADB_MARKER_PATH;', + "setTimeout(() => { fs.writeFileSync(markerPath, 'ended'); }, 250);", + ].join('\n'), + async () => { + const provider = createLocalAndroidAdbProvider(ANDROID_EMULATOR); + const spawn = provider.spawn; + if (!spawn) throw new Error('the local adb provider must expose a background spawner'); + // A JavaScript provider or SDK caller can still hand a deadline across this + // unchecked boundary; the long-lived helper session it lands on must ignore it. + const leakedOptions = { timeoutMs: 20 } as unknown as NonNullable< + Parameters[1] + >; + const child = spawn(['shell', 'logcat'], leakedOptions); + return await new Promise<{ code: number | null; signal: string | null }>((resolve) => { + child.once('exit', (code, signal) => { + resolve({ code, signal }); + }); + }); + }, + { FAKE_ADB_MARKER_PATH: markerPath }, + ); + + assert.equal(outcome.signal, null); + assert.equal(outcome.code, 0); + assert.equal(fs.existsSync(markerPath), true); + }, +); diff --git a/src/platform-runtime-android-adb-host.ts b/src/platform-runtime-android-adb-host.ts index a3df819697..a26bfc57b9 100644 --- a/src/platform-runtime-android-adb-host.ts +++ b/src/platform-runtime-android-adb-host.ts @@ -104,7 +104,13 @@ bindAndroidAdbHost({ }, execHostAdb: async (args, options) => { const invocation = adbInvocation(args, options); - return await runCmd('adb', invocation.args, invocation.options); + return await runCmd('adb', invocation.args, { + ...invocation.options, + // adb's fork-server is a grandchild: without its own process group a + // deadline can only signal `adb` itself, and the server keeps the stdio + // pipes open behind it. + detached: process.platform !== 'win32', + }); }, withAdbCommandExecutorOverride: withCommandExecutorOverride, withoutAdbCommandExecutorOverride: withoutCommandExecutorOverride, diff --git a/src/platform-runtime-app-log-android-transport.ts b/src/platform-runtime-app-log-android-transport.ts index 8cb8245034..163ab5b703 100644 --- a/src/platform-runtime-app-log-android-transport.ts +++ b/src/platform-runtime-app-log-android-transport.ts @@ -29,7 +29,6 @@ export async function resolveAndroidAppLogProcessTransport( allowFailure: adb.options?.allowFailure, cwd: adb.options?.cwd, env: adb.options?.env ? { ...process.env, ...adb.options.env } : undefined, - timeoutMs: adb.options?.timeoutMs, captureOutput: false, signal, }); diff --git a/src/platform-runtime-app-log-process.ts b/src/platform-runtime-app-log-process.ts index 56bf8b2a01..9c1a326630 100644 --- a/src/platform-runtime-app-log-process.ts +++ b/src/platform-runtime-app-log-process.ts @@ -174,7 +174,6 @@ function launchLocalAppLogCommand( allowFailure: request.allowFailure, cwd: request.cwd, env: request.env ? { ...process.env, ...request.env } : undefined, - timeoutMs: request.timeoutMs, captureOutput: false, signal, });