From d669aee18cbb1ce0cf89f7ebc8e8e22799a6d72a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 16:18:17 +0200 Subject: [PATCH 1/8] fix(host-kit): a killed command settles on exit; background exec cannot take a timeout A command killed by its own deadline or by request cancellation settled on 'close', which waits for the stdio pipes to drain. A descendant that inherited those pipes keeps them open after the direct child is gone, wedging the request and the device lock it owns. Both the foreground promise and the background wait now settle on 'exit' once this module asked for the kill, and still wait for a full drain on 'close' when it did not. One finish() owns the timer clear, the abort-listener release, and the trace emit, so the stdin-failure rejection stops bypassing them. killProcessTree no longer signals a child Node already reaped: its pid, and with it the process-group id a detached spawn handed out, is reusable by then. execHostAdb spawns detached like execSerialAdb already does, so a deadline can signal the group instead of only the client. Background runs lose the timeoutMs field they never armed: ExecBackgroundOptions and AndroidAdbSpawnOptions omit it, the two app-log call sites that forwarded it are dropped, and the app-log adb command contract no longer offers it. --- packages/contracts/src/app-log-runtime.ts | 2 +- .../src/internal/exec-boundary-faults.test.ts | 28 ++ .../src/internal/exec-kill-settle.test.ts | 229 ++++++++++++++ packages/host-kit/src/internal/exec.ts | 124 +++++--- .../test-utils/android-host-test-setup.ts | 3 +- .../platform-android/src/adb-transport.ts | 8 +- packages/platform-apple/src/runner/host.ts | 3 +- src/platform-runtime-android-adb-host.test.ts | 285 ++++++++++-------- src/platform-runtime-android-adb-host.ts | 8 +- ...tform-runtime-app-log-android-transport.ts | 1 - src/platform-runtime-app-log-process.ts | 1 - 11 files changed, 524 insertions(+), 168 deletions(-) create mode 100644 packages/host-kit/src/internal/exec-kill-settle.test.ts diff --git a/packages/contracts/src/app-log-runtime.ts b/packages/contracts/src/app-log-runtime.ts index 35dcf7f4b8..4113b2795f 100644 --- a/packages/contracts/src/app-log-runtime.ts +++ b/packages/contracts/src/app-log-runtime.ts @@ -116,7 +116,7 @@ export type AppLogProcessCommand = 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..df6afefd14 --- /dev/null +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -0,0 +1,229 @@ +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, + 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 cancellation arriving after the child was reaped signals no process group', + async () => { + // The direct child exits at once and leaves `sleep 0.4` draining the streams. + // Its pid — and therefore its process-group id — is reusable from the moment it + // is reaped, so the cancellation must not address that group any more. + let childReaped: () => void = () => {}; + const reaped = new Promise((resolve) => { + childReaped = resolve; + }); + const controller = new AbortController(); + const running = runCmdStreaming('/bin/sh', ['-c', 'sleep 0.4 &'], { + signal: controller.signal, + detached: true, + onSpawn: (child) => { + child.once('exit', childReaped); + }, + }); + const rejection = settledRejection(running); + + await reaped; + controller.abort(); + const outcome = await rejection; + + assert.ok(outcome, 'a canceled command must not resolve'); + const details = (outcome.error as { details?: Record }).details; + assert.equal(details?.reason, 'request_canceled'); + }, +); + +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..e922043043 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. @@ -159,6 +167,52 @@ function runSpawnedCommand( }, timeoutMs) : null; const abort = watchCommandAbort(child, options); + // One settlement, whichever termination reaches it first. `close` waits for the + // stdio pipes to drain, and a descendant that inherited them keeps them open + // after the direct child is gone, which would wedge the request and the device + // lock it holds. Once this module asked for the kill there is nothing left to + // drain: the command has already failed on our deadline or the request's + // cancellation. + let settled = false; + const finish = (): boolean => { + if (settled) return false; + settled = true; + if (timeoutHandle) clearTimeout(timeoutHandle); + abort.dispose(); + execTrace.emitForegroundCompletion(cmd, args); + return true; + }; + const fail = (error: AppError): void => { + if (finish()) reject(error); + }; + const settle = (code: number | null): void => { + if (!finish()) return; + 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, + }); + }; if (!options.binaryStdout) child.stdout.setEncoding('utf8'); child.stderr.setEncoding('utf8'); @@ -166,7 +220,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 +241,13 @@ 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', (code) => { + if (didTimeout || abort.didAbort) settle(code); }); + child.once('close', settle); }); } @@ -412,12 +437,13 @@ export function runCmdBackground( } const wait = new Promise((resolve, reject) => { - child.on('error', (err) => { - abort.dispose(); - execTrace.emitBackgroundCompletion(cmd, args, 'error'); - reject(spawnRejectionError(abort, executable, cmd, args, err)); - }); - child.on('close', (code) => { + let settled = false; + // Same rule as the foreground: a kill this module issued ends the wait on + // `exit`, because a descendant that inherited the pipes can hold `close` + // open indefinitely and the cancellation would never reach its caller. + const settle = (code: number | null): void => { + if (settled) return; + settled = true; abort.dispose(); execTrace.emitBackgroundCompletion(cmd, args, 'exit'); const exitCode = code ?? 1; @@ -436,7 +462,18 @@ export function runCmdBackground( return; } resolve({ stdout, stderr, exitCode }); + }; + child.on('error', (err) => { + if (settled) return; + settled = true; + abort.dispose(); + execTrace.emitBackgroundCompletion(cmd, args, 'error'); + reject(spawnRejectionError(abort, executable, cmd, args, err)); + }); + child.once('exit', (code) => { + if (abort.didAbort) settle(code); }); + child.once('close', settle); }); return { child, wait }; @@ -794,6 +831,11 @@ function watchCommandAbort( } function killProcessTree(child: ChildProcess, detached: boolean | undefined): void { + // A child Node already reaped leaves its pid — and therefore its process-group id + // — free for the kernel to hand to an unrelated process, so a late group signal + // from a stale deadline could strike a stranger. Nothing waits for a kill of a + // child that is already gone: the settlement happens on `exit`. + if (child.exitCode !== null || child.signalCode !== null) return; if (detached && child.pid && process.platform !== 'win32') { try { process.kill(-child.pid, 'SIGKILL'); 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..6fdbcf65e6 100644 --- a/src/platform-runtime-android-adb-host.test.ts +++ b/src/platform-runtime-android-adb-host.test.ts @@ -7,148 +7,193 @@ 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`. */ +async function withFakeAdbOnPath(scriptBody: string, run: () => Promise): 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; + process.env.PATH = `${tmpDir}${path.delimiter}${previousPath ?? ''}`; + try { + return await run(); + } finally { + if (previousPath === undefined) delete process.env.PATH; + else process.env.PATH = previousPath; + } +} + 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");', + `setTimeout(() => { fs.writeFileSync(${JSON.stringify(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 }); + }); + }); + }, + ); + + 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, }); From 707047f9e83515caf51347473ba969968657e936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 17:55:08 +0200 Subject: [PATCH 2/8] fix(host-kit): both the kill request and the child exit can settle a command A deadline that fired after the child had already exited left the command waiting for a `close` its pipe-holding descendant would never release: the group kill that ends that descendant cannot run through a child Node already reaped. Kill requests and child exits now report to one settlement that does not care which arrives first, a detached process group is still killed while its members are reachable, and settling closes our end of the pipes instead of holding them open for a stranger. The fake adb reads its marker path from the environment instead of having it spliced into the source it is generated from. Co-authored-by: Apex by Callstack --- .../internal/command-kill-settlement.test.ts | 54 ++++++ .../src/internal/command-kill-settlement.ts | 37 +++++ .../src/internal/exec-kill-settle.test.ts | 154 +++++++++++++++--- packages/host-kit/src/internal/exec.ts | 144 +++++++++------- src/platform-runtime-android-adb-host.test.ts | 24 ++- 5 files changed, 334 insertions(+), 79 deletions(-) create mode 100644 packages/host-kit/src/internal/command-kill-settlement.test.ts create mode 100644 packages/host-kit/src/internal/command-kill-settlement.ts diff --git a/packages/host-kit/src/internal/command-kill-settlement.test.ts b/packages/host-kit/src/internal/command-kill-settlement.test.ts new file mode 100644 index 0000000000..bfd57315c9 --- /dev/null +++ b/packages/host-kit/src/internal/command-kill-settlement.test.ts @@ -0,0 +1,54 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { createCommandKillSettlement } from './command-kill-settlement.ts'; + +function buildSettlement(): { + state: { kills: number; settled: (number | null)[] }; + settlement: ReturnType; +} { + const state = { kills: 0, settled: [] as (number | null)[] }; + const settlement = createCommandKillSettlement({ + killProcessTree: () => { + state.kills += 1; + }, + settle: (code) => state.settled.push(code), + }); + return { state, settlement }; +} + +test('a kill request waits for the child it signalled', () => { + const { state, settlement } = buildSettlement(); + settlement.requestKill(); + assert.equal(state.kills, 1); + assert.deepEqual(state.settled, []); +}); + +test('a kill request settles the child exit that follows it', () => { + const { state, settlement } = buildSettlement(); + settlement.requestKill(); + settlement.recordExit(137); + assert.equal(state.kills, 1); + assert.deepEqual(state.settled, [137]); +}); + +test('a child that exited on its own settles at the kill request that follows', () => { + const { state, settlement } = buildSettlement(); + settlement.recordExit(0); + settlement.requestKill(); + assert.equal(state.kills, 1); + assert.deepEqual(state.settled, [0]); +}); + +test('an exit with no kill request behind it settles nothing', () => { + const { state, settlement } = buildSettlement(); + settlement.recordExit(0); + assert.equal(state.kills, 0); + assert.deepEqual(state.settled, []); +}); + +test('a missing exit code reaches settlement as a failure', () => { + const { state, settlement } = buildSettlement(); + settlement.recordExit(null); + settlement.requestKill(); + assert.deepEqual(state.settled, [1]); +}); diff --git a/packages/host-kit/src/internal/command-kill-settlement.ts b/packages/host-kit/src/internal/command-kill-settlement.ts new file mode 100644 index 0000000000..a993508095 --- /dev/null +++ b/packages/host-kit/src/internal/command-kill-settlement.ts @@ -0,0 +1,37 @@ +/** + * 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. + */ +export 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; +}; + +export 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(); + }, + }; +} diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts index df6afefd14..10990b81ed 100644 --- a/packages/host-kit/src/internal/exec-kill-settle.test.ts +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -140,33 +140,147 @@ test.runIf(process.platform !== 'win32')( ); test.runIf(process.platform !== 'win32')( - 'a cancellation arriving after the child was reaped signals no process group', + 'a deadline that fires after the child exited settles without waiting for the pipe holder', async () => { - // The direct child exits at once and leaves `sleep 0.4` draining the streams. - // Its pid — and therefore its process-group id — is reusable from the moment it - // is reaped, so the cancellation must not address that group any more. - let childReaped: () => void = () => {}; - const reaped = new Promise((resolve) => { - childReaped = resolve; - }); + // 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, +); + +// 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 the group writes are intercepted here, which is the seam that setup points at for +// a real kill path, and the probe answer is what each test is choosing between. + +type GroupWrite = { readonly pid: number; readonly signal: string | number }; + +function interceptGroupWrites(probeAnswer: 'reachable' | 'gone'): { + restore: () => void; + signals: GroupWrite[]; +} { + const original = process.kill.bind(process); + const signals: GroupWrite[] = []; + process.kill = ((pid: number, signal: string | number = 'SIGTERM') => { + if (pid >= 0) return original(pid, signal as NodeJS.Signals); + if (signal === 0) { + if (probeAnswer === 'gone') { + throw Object.assign(new Error('no such process group'), { code: 'ESRCH' }); + } + return true; + } + signals.push({ pid, signal }); + return true; + }) as typeof process.kill; + return { signals, restore: () => (process.kill = original) }; +} + +test.runIf(process.platform !== 'win32')( + 'a detached deadline still kills the group its reaped child left behind', + async () => { + const groupWrites = interceptGroupWrites('reachable'); + 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.signals, [{ pid: -childPid, signal: 'SIGKILL' }]); + } finally { + groupWrites.restore(); + } + }, + 10_000, +); + +test.runIf(process.platform !== 'win32')( + 'a detached deadline whose group is already gone signals nothing and still settles', + async () => { + // A group with no members left has an id the kernel can hand to anyone, so a stale + // deadline must not aim a signal at it. + const groupWrites = interceptGroupWrites('gone'); + 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.deepEqual(groupWrites.signals, []); + } 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(); - const running = runCmdStreaming('/bin/sh', ['-c', 'sleep 0.4 &'], { - signal: controller.signal, - detached: true, - onSpawn: (child) => { - child.once('exit', childReaped); + 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; }, - }); - const rejection = settledRejection(running); + ); + }, + 5_000, +); - await reaped; +test.runIf(process.platform !== 'win32')( + 'a background request that was already canceled ends its wait', + async () => { + const controller = new AbortController(); controller.abort(); - const outcome = await rejection; + const background = runCmdBackground('/bin/sh', ['-c', 'sleep 5'], { + signal: controller.signal, + }); - assert.ok(outcome, 'a canceled command must not resolve'); - const details = (outcome.error as { details?: Record }).details; - assert.equal(details?.reason, 'request_canceled'); + 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')( diff --git a/packages/host-kit/src/internal/exec.ts b/packages/host-kit/src/internal/exec.ts index e922043043..947e69fd3a 100644 --- a/packages/host-kit/src/internal/exec.ts +++ b/packages/host-kit/src/internal/exec.ts @@ -6,6 +6,7 @@ import { spawn, spawnSync, type ChildProcess, type StdioOptions } from 'node:chi import { Readable } from 'node:stream'; import { pipeline } from 'node:stream/promises'; import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; +import { createCommandKillSettlement } from './command-kill-settlement.ts'; import { emitDiagnostic, getDiagnosticsMeta, updateDiagnosticsScope } from './diagnostics.ts'; import { parseBooleanLiteral } from '@agent-device/kernel/source-value'; @@ -160,36 +161,25 @@ function runSpawnedCommand( let stderr = ''; let didTimeout = false; const timeoutMs = normalizeTimeoutMs(options.timeoutMs); - const timeoutHandle = timeoutMs - ? setTimeout(() => { - didTimeout = true; - killProcessTree(child, options.detached); - }, timeoutMs) - : null; - const abort = watchCommandAbort(child, options); - // One settlement, whichever termination reaches it first. `close` waits for the - // stdio pipes to drain, and a descendant that inherited them keeps them open - // after the direct child is gone, which would wedge the request and the device - // lock it holds. Once this module asked for the kill there is nothing left to - // drain: the command has already failed on our deadline or the request's - // cancellation. + let timeoutHandle: NodeJS.Timeout | null = null; let settled = false; - const finish = (): boolean => { + function finish(): boolean { if (settled) return false; settled = true; if (timeoutHandle) clearTimeout(timeoutHandle); abort.dispose(); + destroyCommandStreams(child); execTrace.emitForegroundCompletion(cmd, args); return true; - }; - const fail = (error: AppError): void => { + } + function fail(error: AppError): void { if (finish()) reject(error); - }; - const settle = (code: number | null): void => { + } + function settle(code: number | null): void { if (!finish()) return; - const exitCode = code ?? 1; + const finalExitCode = code ?? 1; if (!abort.didAbort && didTimeout && timeoutMs) { - reject(createTimeoutError(executable, cmd, args, timeoutMs, exitCode, stdout, stderr)); + reject(createTimeoutError(executable, cmd, args, timeoutMs, finalExitCode, stdout, stderr)); return; } const failure = commandCloseFailure( @@ -197,7 +187,7 @@ function runSpawnedCommand( executable, cmd, args, - exitCode, + finalExitCode, options.allowFailure, stdout, stderr, @@ -209,10 +199,24 @@ function runSpawnedCommand( resolve({ stdout, stderr, - exitCode, + 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; + settlement.requestKill(); + }, timeoutMs) + : null; if (!options.binaryStdout) child.stdout.setEncoding('utf8'); child.stderr.setEncoding('utf8'); @@ -244,9 +248,7 @@ function runSpawnedCommand( fail(spawnRejectionError(abort, executable, cmd, args, err)); }); - child.once('exit', (code) => { - if (didTimeout || abort.didAbort) settle(code); - }); + child.once('exit', settlement.recordExit); child.once('close', settle); }); } @@ -422,7 +424,6 @@ export function runCmdBackground( let stdout = ''; let stderr = ''; const captureOutput = options.captureOutput ?? true; - const abort = watchCommandAbort(child, options); if (captureOutput) { child.stdout?.setEncoding('utf8'); @@ -438,21 +439,23 @@ export function runCmdBackground( const wait = new Promise((resolve, reject) => { let settled = false; - // Same rule as the foreground: a kill this module issued ends the wait on - // `exit`, because a descendant that inherited the pipes can hold `close` - // open indefinitely and the cancellation would never reach its caller. - const settle = (code: number | null): void => { - if (settled) return; + function finish(event: 'error' | 'exit'): boolean { + if (settled) return false; settled = true; 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, @@ -461,18 +464,17 @@ export function runCmdBackground( reject(failure); return; } - resolve({ stdout, stderr, exitCode }); - }; - child.on('error', (err) => { - if (settled) return; - settled = true; - abort.dispose(); - execTrace.emitBackgroundCompletion(cmd, args, 'error'); - reject(spawnRejectionError(abort, executable, cmd, args, err)); + resolve({ stdout, stderr, exitCode: finalExitCode }); + } + const settlement = createCommandKillSettlement({ + killProcessTree: () => killProcessTree(child, options.detached), + settle, }); - child.once('exit', (code) => { - if (abort.didAbort) settle(code); + 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); }); @@ -807,13 +809,13 @@ function normalizeTimeoutMs(value: number | undefined): number | undefined { } 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(); @@ -830,21 +832,51 @@ function watchCommandAbort( }; } +/** + * 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. An empty group's id is not reserved, and a group id that no + * longer resolves tells us the members are gone, so the signal is skipped rather than + * aimed at whatever process holds that id now. + */ function killProcessTree(child: ChildProcess, detached: boolean | undefined): void { - // A child Node already reaped leaves its pid — and therefore its process-group id - // — free for the kernel to hand to an unrelated process, so a late group signal - // from a stale deadline could strike a stranger. Nothing waits for a kill of a - // child that is already gone: the settlement happens on `exit`. - if (child.exitCode !== null || child.signalCode !== null) return; if (detached && child.pid && process.platform !== 'win32') { - try { - process.kill(-child.pid, 'SIGKILL'); - return; - } catch {} + if (isProcessGroupReachable(child.pid)) { + try { + process.kill(-child.pid, 'SIGKILL'); + } catch {} + } + 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'); } +function isProcessGroupReachable(pid: number): boolean { + try { + process.kill(-pid, 0); + return true; + } catch (error) { + // EPERM means the group exists and simply isn't ours to signal. + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +/** + * 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/src/platform-runtime-android-adb-host.test.ts b/src/platform-runtime-android-adb-host.test.ts index 6fdbcf65e6..1717c55988 100644 --- a/src/platform-runtime-android-adb-host.test.ts +++ b/src/platform-runtime-android-adb-host.test.ts @@ -11,19 +11,35 @@ 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`. */ -async function withFakeAdbOnPath(scriptBody: string, run: () => Promise): Promise { +/** + * 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; + } } } @@ -172,7 +188,8 @@ test.skipIf(process.platform === 'win32')( const outcome = await withFakeAdbOnPath( [ 'const fs = require("node:fs");', - `setTimeout(() => { fs.writeFileSync(${JSON.stringify(markerPath)}, 'ended'); }, 250);`, + 'const markerPath = process.env.FAKE_ADB_MARKER_PATH;', + "setTimeout(() => { fs.writeFileSync(markerPath, 'ended'); }, 250);", ].join('\n'), async () => { const provider = createLocalAndroidAdbProvider(ANDROID_EMULATOR); @@ -190,6 +207,7 @@ test.skipIf(process.platform === 'win32')( }); }); }, + { FAKE_ADB_MARKER_PATH: markerPath }, ); assert.equal(outcome.signal, null); From a33043dd29067cf3cbc4cfb9ff8225d1673e888d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 18:27:51 +0200 Subject: [PATCH 3/8] fix(host-kit): the kill settlement stays inside the closure its callers already pay The Coverage job's eager-closure gate measured 27 entries evaluating one more module once `command-kill-settlement.ts` landed, and its verdict names the remedy: a small module that every affected entry already evaluates belongs inside that module rather than behind a new static edge. The factory now sits in `exec.ts` next to the two commands that construct it; the ordering behavior and its tests are unchanged. Co-authored-by: Apex by Callstack --- .../internal/command-kill-settlement.test.ts | 54 ------------------- .../src/internal/command-kill-settlement.ts | 37 ------------- .../src/internal/exec-kill-settle.test.ts | 6 +++ packages/host-kit/src/internal/exec.ts | 39 +++++++++++++- 4 files changed, 44 insertions(+), 92 deletions(-) delete mode 100644 packages/host-kit/src/internal/command-kill-settlement.test.ts delete mode 100644 packages/host-kit/src/internal/command-kill-settlement.ts diff --git a/packages/host-kit/src/internal/command-kill-settlement.test.ts b/packages/host-kit/src/internal/command-kill-settlement.test.ts deleted file mode 100644 index bfd57315c9..0000000000 --- a/packages/host-kit/src/internal/command-kill-settlement.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -import assert from 'node:assert/strict'; -import { test } from 'vitest'; -import { createCommandKillSettlement } from './command-kill-settlement.ts'; - -function buildSettlement(): { - state: { kills: number; settled: (number | null)[] }; - settlement: ReturnType; -} { - const state = { kills: 0, settled: [] as (number | null)[] }; - const settlement = createCommandKillSettlement({ - killProcessTree: () => { - state.kills += 1; - }, - settle: (code) => state.settled.push(code), - }); - return { state, settlement }; -} - -test('a kill request waits for the child it signalled', () => { - const { state, settlement } = buildSettlement(); - settlement.requestKill(); - assert.equal(state.kills, 1); - assert.deepEqual(state.settled, []); -}); - -test('a kill request settles the child exit that follows it', () => { - const { state, settlement } = buildSettlement(); - settlement.requestKill(); - settlement.recordExit(137); - assert.equal(state.kills, 1); - assert.deepEqual(state.settled, [137]); -}); - -test('a child that exited on its own settles at the kill request that follows', () => { - const { state, settlement } = buildSettlement(); - settlement.recordExit(0); - settlement.requestKill(); - assert.equal(state.kills, 1); - assert.deepEqual(state.settled, [0]); -}); - -test('an exit with no kill request behind it settles nothing', () => { - const { state, settlement } = buildSettlement(); - settlement.recordExit(0); - assert.equal(state.kills, 0); - assert.deepEqual(state.settled, []); -}); - -test('a missing exit code reaches settlement as a failure', () => { - const { state, settlement } = buildSettlement(); - settlement.recordExit(null); - settlement.requestKill(); - assert.deepEqual(state.settled, [1]); -}); diff --git a/packages/host-kit/src/internal/command-kill-settlement.ts b/packages/host-kit/src/internal/command-kill-settlement.ts deleted file mode 100644 index a993508095..0000000000 --- a/packages/host-kit/src/internal/command-kill-settlement.ts +++ /dev/null @@ -1,37 +0,0 @@ -/** - * 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. - */ -export 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; -}; - -export 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(); - }, - }; -} diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts index 10990b81ed..4ea7f69d9e 100644 --- a/packages/host-kit/src/internal/exec-kill-settle.test.ts +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -158,6 +158,12 @@ test.runIf(process.platform !== 'win32')( 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 the group writes are intercepted here, which is the seam that setup points at for diff --git a/packages/host-kit/src/internal/exec.ts b/packages/host-kit/src/internal/exec.ts index 947e69fd3a..209e8ab320 100644 --- a/packages/host-kit/src/internal/exec.ts +++ b/packages/host-kit/src/internal/exec.ts @@ -6,7 +6,6 @@ import { spawn, spawnSync, type ChildProcess, type StdioOptions } from 'node:chi import { Readable } from 'node:stream'; import { pipeline } from 'node:stream/promises'; import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; -import { createCommandKillSettlement } from './command-kill-settlement.ts'; import { emitDiagnostic, getDiagnosticsMeta, updateDiagnosticsScope } from './diagnostics.ts'; import { parseBooleanLiteral } from '@agent-device/kernel/source-value'; @@ -808,6 +807,44 @@ 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. + */ +export 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; +}; + +export 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( options: Pick, onKill: () => void, From fe243fc8ce1d0b6bf157fd5a30cab78fab1228c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 18:33:46 +0200 Subject: [PATCH 4/8] fix(host-kit): the kill settlement is private to the command executor Co-authored-by: Apex by Callstack --- packages/host-kit/src/internal/exec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/host-kit/src/internal/exec.ts b/packages/host-kit/src/internal/exec.ts index 209e8ab320..2936930ef5 100644 --- a/packages/host-kit/src/internal/exec.ts +++ b/packages/host-kit/src/internal/exec.ts @@ -814,14 +814,14 @@ function normalizeTimeoutMs(value: number | undefined): number | undefined { * 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. */ -export type CommandKillSettlement = { +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; }; -export function createCommandKillSettlement(input: { +function createCommandKillSettlement(input: { readonly killProcessTree: () => void; readonly settle: (exitCode: number | null) => void; }): CommandKillSettlement { From 3471f6acb51e9cb51998fc3c474d35b54ab9243b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 20:13:30 +0200 Subject: [PATCH 5/8] fix(host-kit): one group-signal seam, and no deadline an app-log tail will not honour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `killProcessTree` probed a group with `process.kill(-pid, 0)` and then wrote to it by hand, so host-kit had two group-signal paths and the one every runner-tree kill already used was the one this file could not mock. The probe also decided nothing: EPERM made it report the group reachable and the following write was refused all the same. It is gone, and the detached branch is one call to `signalProcessGroupBestEffort`. That seam now lives in `exec.ts`, below the module that already reached it. `host-process.ts` imports `exec.ts` for `runCmd`, so importing its signal helper back up would close a production value-import cycle, which `check:layering` R4 rejects outright; and giving the two of them a new shared module below both is a module every one of those entries starts evaluating, which the eager-closure budgets reject. `@agent-device/host-kit/process` exports the same name from the new home, and nothing outside host-kit noticed. Two tests moved with the function. The two group-kill tests now answer writes at a guard which records what the kill aimed at and refuses to deliver it — the seam the hermetic signal setup points real kill paths at — and a source-shape test fails if a second `process.kill(-…)` ever appears in this module beside the seam. Reverting the detached branch to a raw group write turns that one red. `AppLogProcessCommand`'s host variant takes `Omit`. The background exec it feeds passes `allowFailure`, `cwd`, and `env` and cannot pass a timeout, so a producer that wrote one was writing a budget that never fires. Co-authored-by: Apex by Callstack --- packages/contracts/src/app-log-runtime.ts | 7 +- .../src/internal/exec-kill-settle.test.ts | 92 +++++++++++++------ packages/host-kit/src/internal/exec.ts | 48 +++++----- .../src/internal/host-process.test.ts | 35 ------- .../host-kit/src/internal/host-process.ts | 17 ---- packages/host-kit/src/process.ts | 2 +- 6 files changed, 100 insertions(+), 101 deletions(-) diff --git a/packages/contracts/src/app-log-runtime.ts b/packages/contracts/src/app-log-runtime.ts index 4113b2795f..4e0a2e56a0 100644 --- a/packages/contracts/src/app-log-runtime.ts +++ b/packages/contracts/src/app-log-runtime.ts @@ -110,7 +110,12 @@ 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'; diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts index 4ea7f69d9e..f05c18b219 100644 --- a/packages/host-kit/src/internal/exec-kill-settle.test.ts +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -1,12 +1,13 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; -import { test } from 'vitest'; +import { test, vi } from 'vitest'; import { isCommandTimeoutError, runCmd, runCmdBackground, runCmdStreaming, + signalProcessGroupBestEffort, type ExecBackgroundOptions, } from './exec.ts'; import { shellQuote } from './shell-quote.ts'; @@ -164,37 +165,75 @@ test.runIf(process.platform !== 'win32')( // 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 the group writes are intercepted here, which is the seam that setup points at for -// a real kill path, and the probe answer is what each test is choosing between. +// 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 refuses to deliver it. type GroupWrite = { readonly pid: number; readonly signal: string | number }; -function interceptGroupWrites(probeAnswer: 'reachable' | 'gone'): { - restore: () => void; - signals: GroupWrite[]; -} { +function guardGroupWrites(): { restore: () => void; writes: GroupWrite[] } { const original = process.kill.bind(process); - const signals: GroupWrite[] = []; + const writes: GroupWrite[] = []; process.kill = ((pid: number, signal: string | number = 'SIGTERM') => { - if (pid >= 0) return original(pid, signal as NodeJS.Signals); - if (signal === 0) { - if (probeAnswer === 'gone') { - throw Object.assign(new Error('no such process group'), { code: 'ESRCH' }); - } - return true; + if (pid < 0) { + writes.push({ pid, signal }); + return false; } - signals.push({ pid, signal }); - return true; + return original(pid, signal as NodeJS.Signals); }) as typeof process.kill; - return { signals, restore: () => (process.kill = original) }; + return { writes, restore: () => (process.kill = original) }; } +test('group signaling addresses the negative pid and reports delivery', () => { + const calls: GroupWrite[] = []; + const killSpy = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => { + calls.push({ pid: Number(pid), signal: signal ?? '' }); + return true; + }); + + try { + assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), true); + assert.deepEqual(calls, [{ pid: -101, signal: 'SIGKILL' }]); + } finally { + killSpy.mockRestore(); + } +}); + +test('a group write in this module can only come from the seam', () => { + // A second group-signal path in here would be the second seam the callers were written + // against once more, and nothing at runtime distinguishes the two. + const source = fs.readFileSync(new URL('./exec.ts', import.meta.url), 'utf8'); + const seam = source.indexOf('export function signalProcessGroupBestEffort'); + const writes = [...source.matchAll(/process\.kill\(-/g)].map((match) => match.index ?? -1); + + assert.ok(seam >= 0); + assert.deepEqual(writes, [seam + source.slice(seam).indexOf('process.kill(-')]); +}); + +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.runIf(process.platform !== 'win32')( 'a detached deadline still kills the group its reaped child left behind', async () => { - const groupWrites = interceptGroupWrites('reachable'); + const groupWrites = guardGroupWrites(); let childPid = 0; try { const startedAt = Date.now(); @@ -213,7 +252,7 @@ test.runIf(process.platform !== 'win32')( }, ); assert.ok(Date.now() - startedAt < 1_000, 'settled only once the pipe holder finished'); - assert.deepEqual(groupWrites.signals, [{ pid: -childPid, signal: 'SIGKILL' }]); + assert.deepEqual(groupWrites.writes, [{ pid: -childPid, signal: 'SIGKILL' }]); } finally { groupWrites.restore(); } @@ -222,11 +261,12 @@ test.runIf(process.platform !== 'win32')( ); test.runIf(process.platform !== 'win32')( - 'a detached deadline whose group is already gone signals nothing and still settles', + 'a detached deadline whose group cannot be signalled still settles', async () => { - // A group with no members left has an id the kernel can hand to anyone, so a stale - // deadline must not aim a signal at it. - const groupWrites = interceptGroupWrites('gone'); + // A vanished group, an empty group, and a group owned by someone else all answer this + // write with nothing, and the seam swallows that. The command still cannot wait on a pipe + // holder it just asked to be killed. + const groupWrites = guardGroupWrites(); try { const startedAt = Date.now(); await assert.rejects( @@ -237,7 +277,7 @@ test.runIf(process.platform !== 'win32')( }, ); assert.ok(Date.now() - startedAt < 1_000, 'settled only once the pipe holder finished'); - assert.deepEqual(groupWrites.signals, []); + assert.equal(groupWrites.writes.length, 1); } finally { groupWrites.restore(); } diff --git a/packages/host-kit/src/internal/exec.ts b/packages/host-kit/src/internal/exec.ts index 2936930ef5..8c90a476d5 100644 --- a/packages/host-kit/src/internal/exec.ts +++ b/packages/host-kit/src/internal/exec.ts @@ -870,20 +870,36 @@ function watchCommandAbort( } /** - * 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. An empty group's id is not reserved, and a group id that no - * longer resolves tells us the members are gone, so the signal is skipped rather than - * aimed at whatever process holds that id now. + * 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') { - if (isProcessGroupReachable(child.pid)) { - try { - process.kill(-child.pid, 'SIGKILL'); - } catch {} - } + signalProcessGroupBestEffort(child.pid, 'SIGKILL'); return; } // A non-detached child leaves its pid free for the kernel to hand to an unrelated @@ -894,16 +910,6 @@ function killProcessTree(child: ChildProcess, detached: boolean | undefined): vo child.kill('SIGKILL'); } -function isProcessGroupReachable(pid: number): boolean { - try { - process.kill(-pid, 0); - return true; - } catch (error) { - // EPERM means the group exists and simply isn't ours to signal. - return (error as NodeJS.ErrnoException).code === 'EPERM'; - } -} - /** * 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. 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, From 957fdea21f56e42a9fc39d3db71c71974caca495 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 07:46:00 +0200 Subject: [PATCH 6/8] test(host-kit): a vanished group answers with ESRCH, and no source-shape proxy --- .../src/internal/exec-kill-settle.test.ts | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts index f05c18b219..1424be992b 100644 --- a/packages/host-kit/src/internal/exec-kill-settle.test.ts +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -167,17 +167,29 @@ test.runIf(process.platform !== 'win32')( // // 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 refuses to deliver it. +// 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 a real group would, either +// a delivery nothing was reached for or the `ESRCH` a vanished group throws. type GroupWrite = { readonly pid: number; readonly signal: string | number }; -function guardGroupWrites(): { restore: () => void; writes: GroupWrite[] } { +/** How a guarded group write answers, matching what a real group would do. */ +type GroupWriteAnswer = 'no-group-reached' | 'no-such-process'; + +function guardGroupWrites(answer: GroupWriteAnswer = 'no-group-reached'): { + 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') { + const error = new Error('no such process') as NodeJS.ErrnoException; + error.code = 'ESRCH'; + throw error; + } return false; } return original(pid, signal as NodeJS.Signals); @@ -200,17 +212,6 @@ test('group signaling addresses the negative pid and reports delivery', () => { } }); -test('a group write in this module can only come from the seam', () => { - // A second group-signal path in here would be the second seam the callers were written - // against once more, and nothing at runtime distinguishes the two. - const source = fs.readFileSync(new URL('./exec.ts', import.meta.url), 'utf8'); - const seam = source.indexOf('export function signalProcessGroupBestEffort'); - const writes = [...source.matchAll(/process\.kill\(-/g)].map((match) => match.index ?? -1); - - assert.ok(seam >= 0); - assert.deepEqual(writes, [seam + source.slice(seam).indexOf('process.kill(-')]); -}); - 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; @@ -263,10 +264,10 @@ test.runIf(process.platform !== 'win32')( test.runIf(process.platform !== 'win32')( 'a detached deadline whose group cannot be signalled still settles', async () => { - // A vanished group, an empty group, and a group owned by someone else all answer this - // write with nothing, and the seam swallows that. The command still cannot wait on a pipe + // 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(); + const groupWrites = guardGroupWrites('no-such-process'); try { const startedAt = Date.now(); await assert.rejects( From 725a22531d249e08d3568f3ddecdd571d2a29e04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 10:52:58 +0200 Subject: [PATCH 7/8] test(host-kit): a guarded group write answers the way process.kill does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default mode returned `false` for a write the kernel took, and the comment above it described that as a group with no reachable members. `process.kill` has three answers to a negative pid and none of them is `false`: `true` once the write is accepted, `ESRCH` when no member is left, `EPERM` when a member belongs to someone else. The assertions survived because the seam reads the throw and ignores the return value, so nothing was wrong with the behavior — but the next test written against this helper would copy an answer the kernel never gives, and the comment would keep promising a state the seam cannot observe. The mode is now `delivered` and returns `true`, and `EPERM` is a mode of its own, which is also the test that branch never had: `signalProcessGroupBestEffort` has one catch for both errno values, and only `ESRCH` was ever exercised. Making that catch rethrow `EPERM` turns the new test red and nothing else. --- .../src/internal/exec-kill-settle.test.ts | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts index 1424be992b..3245ecbe00 100644 --- a/packages/host-kit/src/internal/exec-kill-settle.test.ts +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -166,17 +166,17 @@ test.runIf(process.platform !== 'win32')( // 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 a real group would, either -// a delivery nothing was reached for or the `ESRCH` a vanished group throws. +// 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 a real group would do. */ -type GroupWriteAnswer = 'no-group-reached' | 'no-such-process'; +/** 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 = 'no-group-reached'): { +function guardGroupWrites(answer: GroupWriteAnswer = 'delivered'): { restore: () => void; writes: GroupWrite[]; } { @@ -185,12 +185,14 @@ function guardGroupWrites(answer: GroupWriteAnswer = 'no-group-reached'): { process.kill = ((pid: number, signal: string | number = 'SIGTERM') => { if (pid < 0) { writes.push({ pid, signal }); - if (answer === 'no-such-process') { - const error = new Error('no such process') as NodeJS.ErrnoException; - error.code = 'ESRCH'; + 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 false; + return true; } return original(pid, signal as NodeJS.Signals); }) as typeof process.kill; @@ -231,6 +233,22 @@ test('group signaling reports a vanished group and never signals an invalid pid' } }); +test('a group that is gone and a group that is not ours to signal both report nothing reached', () => { + // `process.kill` answers a negative pid in exactly three ways: `true` once the kernel accepted the + // write, `ESRCH` when no member is left, and `EPERM` when a member belongs to another user. The + // second and third 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 — and only the first of them was tested. + for (const answer of ['no-such-process', 'not-permitted'] as const) { + const groupWrites = guardGroupWrites(answer); + try { + assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), false); + assert.deepEqual(groupWrites.writes, [{ pid: -101, signal: 'SIGKILL' }]); + } finally { + groupWrites.restore(); + } + } +}); + test.runIf(process.platform !== 'win32')( 'a detached deadline still kills the group its reaped child left behind', async () => { From 0089fb123b6cf67ffccfe17922ccff5172259eca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 15:45:46 +0200 Subject: [PATCH 8/8] test(host-kit): one double answers every group write in these tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `guardGroupWrites` had been introduced as the seam every group write is answered at, and then two tests beside it went on answering the same question with hand-written `vi.spyOn(process, 'kill')` doubles. Two doubles for one seam is a slow disagreement waiting to happen: the spy said delivery and the guard said delivery differently, and only one of them was checked against what `process.kill` really answers. Both are built on the guard now, and the three answers it can give — `true` for a delivered write, `ESRCH`, `EPERM` — are one table with the report each must produce, so the delivered case and the two refusals cannot drift apart or be edited separately. The invalid-pid checks moved into the same shape rather than staying beside it, and their proof got stronger: the guard records every write it is asked about, so "nothing was signalled" is now an empty list rather than a spy call count, which is a claim about what the seam did rather than about how the spy was wired. The comment clause claiming only one of the three answers was tested is gone, since it was written before the third arrived. Mutations, one at a time: making the catch in `signalProcessGroupBestEffort` rethrow `EPERM` reddens the table test and nothing else; replacing its pid refusal with a NaN check reddens the invalid-pid test through that empty list. Running the second is safe precisely because the guard answers a negative pid itself and never forwards one to the kernel. --- .../src/internal/exec-kill-settle.test.ts | 69 ++++++++----------- 1 file changed, 28 insertions(+), 41 deletions(-) diff --git a/packages/host-kit/src/internal/exec-kill-settle.test.ts b/packages/host-kit/src/internal/exec-kill-settle.test.ts index 3245ecbe00..86fd1493b8 100644 --- a/packages/host-kit/src/internal/exec-kill-settle.test.ts +++ b/packages/host-kit/src/internal/exec-kill-settle.test.ts @@ -1,7 +1,7 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; -import { test, vi } from 'vitest'; +import { test } from 'vitest'; import { isCommandTimeoutError, runCmd, @@ -199,53 +199,40 @@ function guardGroupWrites(answer: GroupWriteAnswer = 'delivered'): { return { writes, restore: () => (process.kill = original) }; } -test('group signaling addresses the negative pid and reports delivery', () => { - const calls: GroupWrite[] = []; - const killSpy = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => { - calls.push({ pid: Number(pid), signal: signal ?? '' }); - return true; - }); - - try { - assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), true); - assert.deepEqual(calls, [{ pid: -101, signal: 'SIGKILL' }]); - } finally { - killSpy.mockRestore(); +// 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('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; - }); - +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(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); + assert.equal(signalProcessGroupBestEffort(1.5, 'SIGTERM'), false); + assert.deepEqual(groupWrites.writes, []); } finally { - killSpy.mockRestore(); - } -}); - -test('a group that is gone and a group that is not ours to signal both report nothing reached', () => { - // `process.kill` answers a negative pid in exactly three ways: `true` once the kernel accepted the - // write, `ESRCH` when no member is left, and `EPERM` when a member belongs to another user. The - // second and third 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 — and only the first of them was tested. - for (const answer of ['no-such-process', 'not-permitted'] as const) { - const groupWrites = guardGroupWrites(answer); - try { - assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), false); - assert.deepEqual(groupWrites.writes, [{ pid: -101, signal: 'SIGKILL' }]); - } finally { - groupWrites.restore(); - } + groupWrites.restore(); } });