diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 9bd5f7ef8..b3c7f9ae8 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -100,6 +100,23 @@ the runtime seam existed; retiring `dispatchCommand('snapshot')` surfaced them o time. Do not add to that set, and when a command migrates (`docs/agents/adr-0019-unit.md`), its tests move to the runtime seam in the same PR. +Signals are hermetic too. A vitest worker may signal only itself and its own **direct children**; +`src/__tests__/hermetic-signal-setup.ts` refuses every other process-table write and fails the +sending test by name. It covers both ways out of the process, because the runner-disposal family +uses both: `process.kill` (signal 0, the liveness probe, stays free) and a spawned +`kill`/`pkill`/`killall`, whose `-P` and `-f` forms reach processes the worker never spawned at all. +The refused pid is usually one the test made up (`child: { pid: 4242 }`), and on a real host that +number can belong to anyone — on CI it periodically belonged to a sibling fork, which died mid-file +with no test attributed ("Worker exited unexpectedly", #1824); a `pkill -f 'xcodebuild.*'` from a +unit test would find a developer's live runner. + +If a test drives a real kill path against a fabricated pid, mock the seam where it already mocks the +liveness reads: `signalPidsBestEffort` / `signalProcessGroupBestEffort` in `src/utils/host-process.ts` +for direct writes, the exec or tool-provider seam for a spawned `pkill`. Killing a daemon or Metro +fixture the test itself spawned is fine — that pid is the worker's own. *Direct* is literal: a +grandchild started through a shell or `npx` wrapper is not tracked, so signal the direct child, or +its process group through the negative pid, rather than the grandchild's pid. + Keep tests behavioral. Do not assert shapes or cases TypeScript already proves. A test added as a regression pin must be shown to fail without the change it pins — vacuity is the diff --git a/src/__tests__/hermetic-signal-setup.test.ts b/src/__tests__/hermetic-signal-setup.test.ts new file mode 100644 index 000000000..697ab000c --- /dev/null +++ b/src/__tests__/hermetic-signal-setup.test.ts @@ -0,0 +1,94 @@ +import assert from 'node:assert/strict'; +import { execFile, spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { promisify } from 'node:util'; +import { afterEach, test } from 'vitest'; +import { drainRefusedWritesForTest } from './hermetic-signal-setup.ts'; + +// The guard in hermetic-signal-setup.ts is what keeps a unit test from reaching +// a process it does not own (#1824). These drive its refusal paths deliberately, +// so each one drains the record the guard would otherwise fail the test with. + +const execFileAsync = promisify(execFile); + +afterEach(() => { + assert.deepEqual(drainRefusedWritesForTest(), [], 'a test left an unasserted refusal behind'); +}); + +function spawnSleeper() { + return spawn(process.execPath, ['-e', 'setTimeout(() => {}, 10_000)'], { stdio: 'ignore' }); +} + +test('a live direct child may be signalled, and its process group with it', async () => { + const child = spawnSleeper(); + const pid = child.pid; + assert.ok(pid); + + process.kill(pid, 'SIGTERM'); + await once(child, 'exit'); + + assert.deepEqual(drainRefusedWritesForTest(), []); +}); + +test('authority ends when a child is reaped, so its recycled pid is refused', async () => { + const child = spawnSleeper(); + const pid = child.pid; + assert.ok(pid); + child.kill('SIGKILL'); + await once(child, 'exit'); + + // The pid is now free for the kernel to hand to an unrelated process, so the + // worker no longer owns it — even though this worker did spawn it once. + assert.throws( + () => process.kill(pid, 'SIGTERM'), + (error: NodeJS.ErrnoException) => + error.code === 'ESRCH' && /did not spawn it/.test(error.message), + ); + + const [record] = drainRefusedWritesForTest(); + assert.match(record ?? '', new RegExp(`SIGTERM -> pid ${pid}`)); +}); + +test('a live process this worker never spawned is refused', () => { + // The vitest main process: alive, not ours. + assert.throws( + () => process.kill(process.ppid, 'SIGTERM'), + (error: NodeJS.ErrnoException) => error.code === 'ESRCH', + ); + assert.equal(drainRefusedWritesForTest().length, 1); +}); + +test('a liveness probe stays free', () => { + assert.equal(process.kill(process.ppid, 0), true); + assert.deepEqual(drainRefusedWritesForTest(), []); +}); + +test('a promisified execFile cannot smuggle a pkill past the guard', async () => { + // promisify() resolves through the custom-promisify symbol rather than calling + // the wrapper, so this path needs its own interception. + await assert.rejects( + execFileAsync('pkill', ['-f', 'xcodebuild.*AgentDeviceRunner']), + (error: NodeJS.ErrnoException) => + error.code === 'ENOENT' && /Refusing to spawn/.test(error.message), + ); + + const [record] = drainRefusedWritesForTest(); + assert.match(record ?? '', /spawn pkill -f xcodebuild/); +}); + +test('a promisified execFile still tracks the child it starts', async () => { + const pending = execFileAsync(process.execPath, ['-e', 'setTimeout(() => {}, 10_000)']); + const pid = pending.child.pid; + assert.ok(pid); + + process.kill(pid, 'SIGKILL'); + await pending.catch(() => {}); + + assert.deepEqual(drainRefusedWritesForTest(), []); +}); + +test('a spawned kill binary is refused whichever entry point starts it', () => { + assert.throws(() => spawn('pkill', ['-TERM', '-P', '4242']), /Refusing to spawn/); + assert.throws(() => execFile('/usr/bin/killall', ['node']), /Refusing to spawn/); + assert.equal(drainRefusedWritesForTest().length, 2); +}); diff --git a/src/__tests__/hermetic-signal-setup.ts b/src/__tests__/hermetic-signal-setup.ts new file mode 100644 index 000000000..2fbaf0903 --- /dev/null +++ b/src/__tests__/hermetic-signal-setup.ts @@ -0,0 +1,170 @@ +import childProcess from 'node:child_process'; +import module from 'node:module'; +import util from 'node:util'; +import { afterAll, afterEach, expect } from 'vitest'; + +// Unit tests must be hermetic with respect to the host's process table: a +// worker may signal only itself and its own direct children. Anything else is +// a pid the test made up (`child: { pid: 4242 }`), and on a real host that +// number can belong to anyone — on CI it periodically belonged to a sibling +// vitest fork, which died mid-file with no test attributed +// ("Worker exited unexpectedly", #1824). Refusing the write here, in every +// worker, turns that silent fork death into a named failure of the test that +// sent it, on any host, deterministically. +// +// Both ways out of the process are covered, because the runner-disposal family +// uses both: `process.kill` (including the negative pid that addresses a +// child's process group) and a spawned `kill`/`pkill`/`killall`, whose `-P` +// and `-f` forms reach processes this worker never spawned at all. +// +// Signal 0 is a liveness probe (`isProcessAlive`), not a signal; it stays free. +// Tests that drive a real kill path against a fabricated pid mock the seam — +// `signalPidsBestEffort` / `signalProcessGroupBestEffort` in +// `src/utils/host-process.ts` for the direct writes, the exec/tool-provider +// seam for a spawned `pkill` — the same way they already mock the liveness +// reads. + +/** + * Direct, *live* children only. + * + * A grandchild (a server started through a shell or `npx` wrapper) is not + * tracked, so signalling its pid is refused even though the test legitimately + * owns it; signal the direct child, or its process group via the negative pid, + * or mock the seam. + * + * Authority ends when the child is reaped: a pid is only ever a claim on a slot + * in the host's process table, and the kernel hands that slot to someone else + * once it is free. So synchronous spawns (`spawnSync`, `execFileSync`) are never + * remembered — they have already exited when the call returns — and async + * children are dropped on `exit`. Cleanup that signals a child must gate on + * `isProcessAlive` (as `stopProcess` in `client-metro.test.ts` already does); + * "I spawned this pid once" is not ownership of whatever holds it now, which is + * the exact hazard this file exists to close. + */ +const ownPids = new Set([process.pid]); + +const KILL_BINARIES = new Set(['kill', 'pkill', 'killall']); + +const refused: string[] = []; + +function record(what: string): void { + const state = expect.getState(); + const where = new Error().stack?.split('\n').slice(3, 7).join('\n') ?? ''; + refused.push(`${what} (${state.currentTestName ?? 'outside a test'})\n${where}`); +} + +function rememberChild(child: unknown): void { + const handle = child as { pid?: unknown; once?: (event: string, cb: () => void) => void } | null; + const pid = handle?.pid; + if (typeof pid !== 'number') return; + ownPids.add(pid); + // Both events are attached because a child that is never spawned at all + // ('error') emits close without exit. + handle?.once?.('exit', () => ownPids.delete(pid)); + handle?.once?.('close', () => ownPids.delete(pid)); +} + +function killBinaryName(command: unknown): string | undefined { + // `exec` takes a shell command line; the rest take an executable path. + const first = String(command).trim().split(/\s+/)[0] ?? ''; + const name = first.split('/').pop() ?? ''; + return KILL_BINARIES.has(name) ? name : undefined; +} + +function refuseSpawnedKill(command: unknown, args: unknown): Error | undefined { + const name = killBinaryName(command); + if (!name) return undefined; + const rendered = [String(command), ...(Array.isArray(args) ? args.map(String) : [])].join(' '); + record(`spawn ${rendered}`); + // Shaped like the binary being unavailable, which every caller of these + // best-effort kills already tolerates. + const error = new Error( + `Refusing to spawn \`${rendered}\`: a unit test may not signal processes through ${name} (see afterEach failure).`, + ) as NodeJS.ErrnoException; + error.code = 'ENOENT'; + return error; +} + +function wrapSpawner unknown>(real: T, remember: boolean): T { + const wrapped = ((...args: Parameters) => { + const refusal = refuseSpawnedKill(args[0], args[1]); + if (refusal) throw refusal; + const child = real(...args); + if (remember) rememberChild(child); + return child; + }) as unknown as T; + // `util.promisify(execFile)` does not call the function at all: it resolves + // through the custom-promisify symbol, so copying the original across would + // hand every promisified caller an unguarded execFile. Wrap that path too. + const custom = (real as unknown as Record)[util.promisify.custom] as + | ((...args: unknown[]) => unknown) + | undefined; + if (custom) { + const wrappedCustom = (...args: unknown[]) => { + const refusal = refuseSpawnedKill(args[0], args[1]); + if (refusal) return Promise.reject(refusal); + const promise = custom(...args); + if (remember) rememberChild((promise as { child?: unknown } | null)?.child); + return promise; + }; + Object.defineProperty(wrapped, util.promisify.custom, { value: wrappedCustom }); + } + return wrapped; +} + +for (const name of ['spawn', 'fork', 'exec', 'execFile'] as const) { + (childProcess as unknown as Record)[name] = wrapSpawner( + childProcess[name], + true, + ); +} +for (const name of ['spawnSync', 'execFileSync', 'execSync'] as const) { + (childProcess as unknown as Record)[name] = wrapSpawner( + childProcess[name], + false, + ); +} +// Named ESM imports of the builtin (`import { spawn } from 'node:child_process'`) +// bind to the CJS exports only after they are re-synced. +module.syncBuiltinESMExports(); + +const realKill = process.kill.bind(process); +process.kill = ((pid: number, signal: string | number = 'SIGTERM') => { + if (signal === 0 || ownPids.has(Math.abs(pid))) return realKill(pid, signal as NodeJS.Signals); + record(`${String(signal)} -> pid ${pid}`); + // Behave like a dead pid: the caller's ESRCH handling runs, no signal leaves + // this worker, and the record above fails the test in afterEach below. + const error = new Error( + `Refusing to send ${String(signal)} to pid ${pid}: this vitest worker did not spawn it (see afterEach failure).`, + ) as NodeJS.ErrnoException; + error.code = 'ESRCH'; + throw error; +}) as typeof process.kill; + +// Best-effort kill sites swallow the errors above, so the refusal is reported +// here, attributed to the test (or file) that caused it, and never lost. +function failOnRefusedSignals(scope: string): void { + if (refused.length === 0) return; + const count = refused.length; + const report = refused.splice(0).join('\n'); + throw new Error( + `${scope} tried to signal ${count} process(es) this vitest worker did not spawn. ` + + 'A unit test may signal only itself and its own direct children (and their process ' + + 'groups); mock the seam instead — signalPidsBestEffort / signalProcessGroupBestEffort in ' + + 'src/utils/host-process.ts, the exec or tool-provider seam for a spawned pkill, or ' + + `vi.spyOn(process, 'kill'). If the pid is a descendant this test really owns, signal the ` + + `direct child (or its group via the negative pid) rather than the grandchild.\n${report}`, + ); +} + +afterEach(() => failOnRefusedSignals('This test')); +afterAll(() => failOnRefusedSignals('This file')); + +/** + * Takes the pending refusals so a test can assert on them without the hooks + * above failing it. Only `hermetic-signal-setup.test.ts`, which drives the + * guard's own refusal paths on purpose, should call this. + */ +export function drainRefusedWritesForTest(): string[] { + return refused.splice(0); +} diff --git a/src/__tests__/test-file-size-ratchet.test.ts b/src/__tests__/test-file-size-ratchet.test.ts index 81cb97ea9..92b77c979 100644 --- a/src/__tests__/test-file-size-ratchet.test.ts +++ b/src/__tests__/test-file-size-ratchet.test.ts @@ -36,8 +36,8 @@ const PINNED_TEST_FILE_LINES: Readonly> = Object.freeze({ 'src/__tests__/remote-connection.test.ts': 2973, 'src/daemon/handlers/__tests__/snapshot-handler.test.ts': 2654, 'src/commands/interaction/runtime/settle.test.ts': 2361, - 'src/platforms/apple/core/__tests__/runner-session.test.ts': 2083, 'src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts': 2031, + 'src/platforms/apple/core/__tests__/runner-session.test.ts': 2001, 'src/utils/__tests__/daemon-client.test.ts': 1910, 'src/utils/__tests__/output.test.ts': 1861, 'src/platforms/android/__tests__/snapshot.test.ts': 1660, diff --git a/src/daemon/__tests__/request-router-open.test.ts b/src/daemon/__tests__/request-router-open.test.ts index 5ce399b05..23ce5a295 100644 --- a/src/daemon/__tests__/request-router-open.test.ts +++ b/src/daemon/__tests__/request-router-open.test.ts @@ -12,6 +12,19 @@ vi.mock('../../utils/host-process.ts', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, readProcessStartTime: vi.fn(() => 'test-process-start') }; }); +// Opening a session runs the owned-lease cleanup, which pattern-kills stale +// xcodebuild runners with a real `pkill -f`. The session id here is fabricated, +// so on a host with a live Apple runner that write would reach a process this +// test does not own; stub the tool seam the way the runner tests stub the +// signal seam (#1824). +vi.mock('../../platforms/apple/core/tool-provider.ts', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + runAppleToolCommand: vi.fn(async () => ({ exitCode: 0, stdout: '', stderr: '' })), + }; +}); import { dispatchCommand } from '../../core/dispatch.ts'; import { diff --git a/src/platforms/apple/core/__tests__/runner-disposal.test.ts b/src/platforms/apple/core/__tests__/runner-disposal.test.ts index 2c1b91e3d..da2056e9d 100644 --- a/src/platforms/apple/core/__tests__/runner-disposal.test.ts +++ b/src/platforms/apple/core/__tests__/runner-disposal.test.ts @@ -14,6 +14,7 @@ const { mockRunAppleToolCommand, mockRunXcrun, mockSignalPidsBestEffort, + mockSignalProcessGroupBestEffort, } = vi.hoisted(() => ({ mockCleanupTempFile: vi.fn(), mockIsProcessAlive: vi.fn(), @@ -21,6 +22,7 @@ const { mockRunAppleToolCommand: vi.fn(), mockRunXcrun: vi.fn(), mockSignalPidsBestEffort: vi.fn(), + mockSignalProcessGroupBestEffort: vi.fn(), })); vi.mock('../../../../utils/host-process.ts', async (importOriginal) => { @@ -30,6 +32,7 @@ vi.mock('../../../../utils/host-process.ts', async (importOriginal) => { isProcessAlive: mockIsProcessAlive, isProcessGroupAlive: mockIsProcessGroupAlive, signalPidsBestEffort: mockSignalPidsBestEffort, + signalProcessGroupBestEffort: mockSignalProcessGroupBestEffort, }; }); @@ -51,7 +54,6 @@ import { abortRunnerSessionsAndPrepProcesses } from '../runner/runner-disposal.t beforeEach(() => { vi.useFakeTimers(); - vi.spyOn(process, 'kill').mockImplementation(() => true); mockIsProcessAlive.mockReturnValue(true); mockIsProcessGroupAlive.mockReturnValue(false); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); @@ -144,9 +146,8 @@ function makeRunnerSession( } function runnerSignals(session: RunnerSession): NodeJS.Signals[] { - return vi - .mocked(process.kill) - .mock.calls.filter(([pid]) => pid === -(session.child.pid ?? 0)) + return mockSignalProcessGroupBestEffort.mock.calls + .filter(([pid]) => pid === session.child.pid) .map(([, signal]) => signal as NodeJS.Signals); } diff --git a/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts b/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts index e0c10caea..bf1a48b55 100644 --- a/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts +++ b/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts @@ -18,6 +18,8 @@ const { mockRunAppleToolCommand, mockRunCmdBackground, mockRunXcrun, + mockSignalPidsBestEffort, + mockSignalProcessGroupBestEffort, mockWaitForRunner, mockRedirectRelease, } = vi.hoisted(() => ({ @@ -32,6 +34,9 @@ const { mockRunAppleToolCommand: vi.fn(), mockRunCmdBackground: vi.fn(), mockRunXcrun: vi.fn(), + // Runner child pids here are fabricated (4141..4444); see runner-session.test.ts. + mockSignalPidsBestEffort: vi.fn(), + mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), mockRedirectRelease: vi.fn(), })); @@ -54,6 +59,8 @@ vi.mock('../../../../utils/host-process.ts', async () => { ...actual, isProcessAlive: mockIsProcessAlive, isProcessGroupAlive: mockIsProcessGroupAlive, + signalPidsBestEffort: mockSignalPidsBestEffort, + signalProcessGroupBestEffort: mockSignalProcessGroupBestEffort, }; }); diff --git a/src/platforms/apple/core/__tests__/runner-session-fixtures.ts b/src/platforms/apple/core/__tests__/runner-session-fixtures.ts new file mode 100644 index 000000000..85be667d9 --- /dev/null +++ b/src/platforms/apple/core/__tests__/runner-session-fixtures.ts @@ -0,0 +1,111 @@ +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; +import fs from 'node:fs'; +import { IOS_SIMULATOR } from '../../../../__tests__/test-utils/index.ts'; +import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; +import { + flushDiagnosticsToSessionFile, + withDiagnosticsScope, +} from '../../../../utils/diagnostics.ts'; +import { RUNNER_OWNER_START_TIME, type RunnerLease } from '../runner/runner-lease.ts'; +import type { RunnerSession } from '../runner/runner-session-types.ts'; + +// Fabricated runner sessions, leases, background children, and transport +// payloads shared by the runner-session tests. The child pids here are made up +// (`4242`): nothing in a test may deliver a real signal to them, so the owning +// tests mock the signal seam in `src/utils/host-process.ts` — see +// `src/__tests__/hermetic-signal-setup.ts` and #1824. + +export function makeRunnerSession(overrides: Partial = {}): RunnerSession { + return { + sessionId: `session-${overrides.port ?? 8100}`, + device: IOS_SIMULATOR, + deviceId: IOS_SIMULATOR.id, + port: 8100, + xctestrunPath: '/tmp/runner.xctestrun', + jsonPath: '/tmp/runner.json', + testPromise: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), + child: { pid: 1234, exitCode: null }, + ready: true, + ...overrides, + } as RunnerSession; +} + +export function makeRunnerLease( + overrides: Partial & { deviceId: string; ownerToken?: string | undefined }, +): RunnerLease { + const ownerToken = overrides.ownerToken ?? `owner-${process.pid}-test`; + const lease: RunnerLease = { + schemaVersion: 1, + deviceId: overrides.deviceId, + ownerToken, + ownerPid: process.pid, + ownerStartTime: RUNNER_OWNER_START_TIME, + sessionId: `session-${overrides.deviceId}`, + runnerPid: 4242, + port: 8123, + xctestrunPath: `/tmp/AgentDeviceRunner.env.session-${overrides.deviceId}-${ownerToken}-8123.xctestrun`, + jsonPath: `/tmp/AgentDeviceRunner.env.session-${overrides.deviceId}-${ownerToken}-8123.json`, + createdAtMs: Date.now(), + }; + return { ...lease, ...overrides, ownerToken }; +} + +export function makeBackgroundRunner(pid: number) { + return { + child: { + pid, + exitCode: null, + stdout: new EventEmitter(), + stderr: new EventEmitter(), + }, + wait: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), + }; +} + +export function runnerResponse(data: Record): Response { + return new Response(JSON.stringify({ ok: true, data })); +} + +export function runnerError(error: { code: string; message: string }): Response { + return new Response(JSON.stringify({ ok: false, error })); +} + +export async function captureDiagnostics(callback: () => Promise): Promise { + const previousHome = process.env.HOME; + process.env.HOME = mkdtempForTestSync('agent-device-runner-diag-'); + try { + return await withDiagnosticsScope( + { session: 'runner-session-test', requestId: 'request-1', command: 'tap' }, + async () => { + await callback(); + const diagnosticsPath = flushDiagnosticsToSessionFile({ force: true })?.path; + assert.ok(diagnosticsPath); + return fs.readFileSync(diagnosticsPath, 'utf8'); + }, + ); + } finally { + process.env.HOME = previousHome; + } +} + +export function assertRunnerCommand( + actual: unknown, + expected: Record, + options: { commandId?: boolean } = {}, +): asserts actual is Record { + assert.equal(typeof actual, 'object'); + assert.notEqual(actual, null); + const command = actual as Record; + const commandId = command.commandId; + if (options.commandId === false) { + assert.equal(commandId, undefined); + assert.deepEqual(command, expected); + return; + } + if (typeof commandId !== 'string') { + assert.fail('expected runner commandId'); + } + assert.match(commandId, /^runner-/); + assert.deepEqual({ ...command, commandId: undefined }, { ...expected, commandId: undefined }); +} diff --git a/src/platforms/apple/core/__tests__/runner-session.test.ts b/src/platforms/apple/core/__tests__/runner-session.test.ts index 920fc9b50..b36b600ab 100644 --- a/src/platforms/apple/core/__tests__/runner-session.test.ts +++ b/src/platforms/apple/core/__tests__/runner-session.test.ts @@ -1,6 +1,5 @@ import type { RequestProgressEvent } from '@agent-device/contracts/progress'; import assert from 'node:assert/strict'; -import { EventEmitter } from 'node:events'; import fs from 'node:fs'; import path from 'node:path'; import { beforeEach, test, vi } from 'vitest'; @@ -8,10 +7,14 @@ import { IOS_DEVICE, IOS_SIMULATOR } from '../../../../__tests__/test-utils/inde import { withRequestProgressSink } from '../../../../request/progress.ts'; import { AppError } from '@agent-device/kernel/errors'; import { - flushDiagnosticsToSessionFile, - withDiagnosticsScope, -} from '../../../../utils/diagnostics.ts'; -import type { RunnerSession } from '../runner/runner-session-types.ts'; + assertRunnerCommand, + captureDiagnostics, + makeBackgroundRunner, + makeRunnerLease, + makeRunnerSession, + runnerError, + runnerResponse, +} from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; const { @@ -30,6 +33,8 @@ const { mockRunCmdBackground, mockRunXcrun, mockSendRunnerCommandOnce, + mockSignalPidsBestEffort, + mockSignalProcessGroupBestEffort, mockWaitForRunner, mockRedirectRelease, } = vi.hoisted(() => ({ @@ -53,6 +58,11 @@ const { mockRunCmdBackground: vi.fn(), mockRunXcrun: vi.fn(), mockSendRunnerCommandOnce: vi.fn(), + // The runner child pid below is fabricated (4242), so the signal writes are + // mocked next to the liveness reads: a real signal to a made-up pid can hit a + // sibling vitest fork (#1824), and the shared setup refuses it outright. + mockSignalPidsBestEffort: vi.fn(), + mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), mockRedirectRelease: vi.fn(), })); @@ -77,6 +87,8 @@ vi.mock('../../../../utils/host-process.ts', async () => { isProcessGroupAlive: mockIsProcessGroupAlive, readProcessCommand: mockReadProcessCommand, readProcessStartTime: mockReadProcessStartTime, + signalPidsBestEffort: mockSignalPidsBestEffort, + signalProcessGroupBestEffort: mockSignalProcessGroupBestEffort, }; }); @@ -1987,97 +1999,3 @@ test('runner session invalidates when the runner reports abandoned main-thread w assert.equal(getRunnerSessionSnapshot(device.id), null); }); - -function makeRunnerSession(overrides: Partial = {}): RunnerSession { - return { - sessionId: `session-${overrides.port ?? 8100}`, - device: IOS_SIMULATOR, - deviceId: IOS_SIMULATOR.id, - port: 8100, - xctestrunPath: '/tmp/runner.xctestrun', - jsonPath: '/tmp/runner.json', - testPromise: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), - child: { pid: 1234, exitCode: null }, - ready: true, - ...overrides, - } as RunnerSession; -} - -function makeRunnerLease( - overrides: Partial & { deviceId: string; ownerToken?: string | undefined }, -): RunnerLease { - const ownerToken = overrides.ownerToken ?? `owner-${process.pid}-test`; - const lease: RunnerLease = { - schemaVersion: 1, - deviceId: overrides.deviceId, - ownerToken, - ownerPid: process.pid, - ownerStartTime: RUNNER_OWNER_START_TIME, - sessionId: `session-${overrides.deviceId}`, - runnerPid: 4242, - port: 8123, - xctestrunPath: `/tmp/AgentDeviceRunner.env.session-${overrides.deviceId}-${ownerToken}-8123.xctestrun`, - jsonPath: `/tmp/AgentDeviceRunner.env.session-${overrides.deviceId}-${ownerToken}-8123.json`, - createdAtMs: Date.now(), - }; - return { ...lease, ...overrides, ownerToken }; -} - -function makeBackgroundRunner(pid: number) { - return { - child: { - pid, - exitCode: null, - stdout: new EventEmitter(), - stderr: new EventEmitter(), - }, - wait: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), - }; -} - -function runnerResponse(data: Record): Response { - return new Response(JSON.stringify({ ok: true, data })); -} - -function runnerError(error: { code: string; message: string }): Response { - return new Response(JSON.stringify({ ok: false, error })); -} - -async function captureDiagnostics(callback: () => Promise): Promise { - const previousHome = process.env.HOME; - process.env.HOME = mkdtempForTestSync('agent-device-runner-diag-'); - try { - return await withDiagnosticsScope( - { session: 'runner-session-test', requestId: 'request-1', command: 'tap' }, - async () => { - await callback(); - const diagnosticsPath = flushDiagnosticsToSessionFile({ force: true })?.path; - assert.ok(diagnosticsPath); - return fs.readFileSync(diagnosticsPath, 'utf8'); - }, - ); - } finally { - process.env.HOME = previousHome; - } -} - -function assertRunnerCommand( - actual: unknown, - expected: Record, - options: { commandId?: boolean } = {}, -): asserts actual is Record { - assert.equal(typeof actual, 'object'); - assert.notEqual(actual, null); - const command = actual as Record; - const commandId = command.commandId; - if (options.commandId === false) { - assert.equal(commandId, undefined); - assert.deepEqual(command, expected); - return; - } - if (typeof commandId !== 'string') { - assert.fail('expected runner commandId'); - } - assert.match(commandId, /^runner-/); - assert.deepEqual({ ...command, commandId: undefined }, { ...expected, commandId: undefined }); -} diff --git a/src/platforms/apple/core/runner/runner-disposal.ts b/src/platforms/apple/core/runner/runner-disposal.ts index 01a31763c..75ef4e4c2 100644 --- a/src/platforms/apple/core/runner/runner-disposal.ts +++ b/src/platforms/apple/core/runner/runner-disposal.ts @@ -4,6 +4,7 @@ import { isProcessAlive, isProcessGroupAlive, signalPidsBestEffort, + signalProcessGroupBestEffort, } from '../../../../utils/host-process.ts'; import type { ExecBackgroundResult } from '../../../../utils/exec.ts'; import { cleanupTempFile } from './runner-io.ts'; @@ -246,9 +247,7 @@ async function killRunnerProcessTree( signal: 'SIGINT' | 'SIGTERM' | 'SIGKILL', ): Promise { if (!pid || pid <= 0) return; - try { - process.kill(-pid, signal); - } catch {} + signalProcessGroupBestEffort(pid, signal); signalPidsBestEffort([pid], signal); const pkillSignal = signal === 'SIGINT' ? 'INT' : signal === 'SIGTERM' ? 'TERM' : 'KILL'; try { diff --git a/src/utils/__tests__/host-process.test.ts b/src/utils/__tests__/host-process.test.ts index 9f6dac3fd..3ced0c86b 100644 --- a/src/utils/__tests__/host-process.test.ts +++ b/src/utils/__tests__/host-process.test.ts @@ -9,6 +9,7 @@ import { readProcessCommand, readProcessStartTime, signalPidsBestEffort, + signalProcessGroupBestEffort, stopPidsWithEscalation, uniquePositivePids, } from '../host-process.ts'; @@ -122,6 +123,40 @@ 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/src/utils/host-process.ts b/src/utils/host-process.ts index 812beb0ab..5ce210c8d 100644 --- a/src/utils/host-process.ts +++ b/src/utils/host-process.ts @@ -181,6 +181,23 @@ 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/vitest.config.ts b/vitest.config.ts index 21f5a9b0e..e15533161 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -17,7 +17,13 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = [ 'scripts/fuzz/corpus-replay.test.ts', ]; -const SETUP_FILES = ['src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts']; +// Imported by vitest.mutation.config.ts so the two lanes cannot drift: a guard +// added here must reach the Stryker sandbox too. +export const SETUP_FILES = [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/hermetic-signal-setup.ts', + 'src/__tests__/process-memo-setup.ts', +]; export default defineConfig({ test: { diff --git a/vitest.mutation.config.ts b/vitest.mutation.config.ts index 4795070f8..09e3b2bca 100644 --- a/vitest.mutation.config.ts +++ b/vitest.mutation.config.ts @@ -3,7 +3,7 @@ import { fileURLToPath } from 'node:url'; import { defineConfig } from 'vitest/config'; import { readTestScope, threadHostileTestFiles } from './scripts/mutation/test-scope.ts'; import { workspaceSourceAliases } from './scripts/mutation/workspace-aliases.ts'; -import { SUBPROCESS_STUB_TESTS } from './vitest.config.ts'; +import { SETUP_FILES, SUBPROCESS_STUB_TESTS } from './vitest.config.ts'; const repoRoot = path.dirname(fileURLToPath(import.meta.url)); @@ -36,6 +36,6 @@ export default defineConfig({ test: { include: scope ?? ['src/**/*.test.ts', 'packages/*/src/**/*.test.ts'], exclude: [...SUBPROCESS_STUB_TESTS, ...threadHostileTestFiles(repoRoot), '**/node_modules/**'], - setupFiles: ['src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts'], + setupFiles: [...SETUP_FILES], }, });