From a8a020099c756a3f4d9d510cb5d2abf33fa063af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 19:17:27 +0200 Subject: [PATCH 1/5] =?UTF-8?q?ci:=20w3-1824=20experiment=20=E2=80=94=20tr?= =?UTF-8?q?ace=20fork=20signals=20and=20plant=20pid=20sentinels=20in=20the?= =?UTF-8?q?=20Coverage=20job?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Temporary instrumentation for #1824. Every vitest fork logs each real process.kill it sends to a foreign pid (and every kill/pkill it spawns); the Coverage job parks sentinel processes on the pids the Apple runner tests fabricate (4141/4242/4343/4444) and reports which of them survive the run. Reverted before this PR leaves draft. --- .github/workflows/ci.yml | 10 ++++ vitest.config.ts | 7 ++- w3-1824-kill-trace-setup.ts | 91 +++++++++++++++++++++++++++++++++++++ w3-1824-plant-sentinels.sh | 49 ++++++++++++++++++++ w3-1824-report.sh | 56 +++++++++++++++++++++++ w3-1824-sentinel.cjs | 15 ++++++ 6 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 w3-1824-kill-trace-setup.ts create mode 100644 w3-1824-plant-sentinels.sh create mode 100644 w3-1824-report.sh create mode 100644 w3-1824-sentinel.cjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 015540029..3e004f553 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -338,12 +338,22 @@ jobs: uses: ./.github/actions/run-gate with: { gate: coverage-model } + # w3-1824 EXPERIMENT (temporary): sentinels on the pids the runner tests + # fabricate, plus a per-fork kill tracer wired through vitest.config.ts. + - name: w3-1824 plant sentinels + run: bash ./w3-1824-plant-sentinels.sh /tmp/w3-1824 + - name: Run coverage id: run-coverage env: OUTPUT_ECONOMY_BASE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} + W3_1824_KILL_TRACE: /tmp/w3-1824/kill-trace.ndjson uses: ./.github/actions/run-gate with: { gate: unit-ci } + + - name: w3-1824 report + if: always() + run: bash ./w3-1824-report.sh /tmp/w3-1824 /tmp/w3-1824/kill-trace.ndjson # The TMPDIR redirection both test lanes depend on (#1593/#1595). The check is a real # package script that no workflow ran: it is reachable only through `check:unit`, an # aggregate CI never invokes, so a leak regression could not fail a PR. Placed here diff --git a/vitest.config.ts b/vitest.config.ts index 21f5a9b0e..7a88354e5 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -17,7 +17,12 @@ 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']; +const SETUP_FILES = [ + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', + // w3-1824 EXPERIMENT (temporary): per-fork kill tracer. + './w3-1824-kill-trace-setup.ts', +]; export default defineConfig({ test: { diff --git a/w3-1824-kill-trace-setup.ts b/w3-1824-kill-trace-setup.ts new file mode 100644 index 000000000..9dc347a1a --- /dev/null +++ b/w3-1824-kill-trace-setup.ts @@ -0,0 +1,91 @@ +// fallow-ignore-file unused-file +// SCRATCH (w3-1824): traces every real signal a vitest fork sends to a foreign +// pid, plus every `kill`/`pkill` subprocess it spawns. Appends NDJSON lines to +// $W3_1824_KILL_TRACE. Never committed. +import childProcess from 'node:child_process'; +import fs from 'node:fs'; +import module from 'node:module'; +import util from 'node:util'; +import { beforeAll, expect } from 'vitest'; + +const out = process.env.W3_1824_KILL_TRACE; + +function currentTest(): { file?: string; name?: string } { + try { + const state = expect.getState(); + return { file: state.testPath, name: state.currentTestName }; + } catch { + return {}; + } +} + +function readPgid(): number | undefined { + try { + // /proc/self/stat: pid (comm) state ppid pgrp ... + const stat = fs.readFileSync('/proc/self/stat', 'utf8'); + const afterComm = stat.slice(stat.lastIndexOf(')') + 2).split(' '); + return Number(afterComm[2]); + } catch { + return undefined; + } +} + +function record(entry: Record): void { + if (!out) return; + const line = JSON.stringify({ + ts: new Date().toISOString(), + workerPid: process.pid, + workerPpid: process.ppid, + workerPgid: readPgid(), + ...currentTest(), + ...entry, + }); + fs.appendFileSync(out, `${line}\n`); +} + +const realKill = process.kill.bind(process); +process.kill = ((pid: number, signal?: string | number) => { + // signal 0 is a liveness probe; ignore self-signals. + if (signal !== 0 && pid !== process.pid) { + record({ + kind: 'process.kill', + pid, + signal: signal ?? 'SIGTERM', + stack: new Error().stack?.split('\n').slice(2, 9).join(' | '), + }); + } + return realKill(pid, signal as never); +}) as typeof process.kill; + +const KILL_BINARIES = new Set(['kill', 'pkill', 'killall']); + +function traceSpawn(cmd: unknown, args: unknown): void { + const command = String(cmd); + if (!KILL_BINARIES.has(command.split('/').pop() ?? '')) return; + record({ + kind: 'spawn', + command, + args: Array.isArray(args) ? args : [], + stack: new Error().stack?.split('\n').slice(3, 10).join(' | '), + }); +} + +for (const name of ['spawn', 'spawnSync', 'execFile', 'execFileSync'] as const) { + const real = childProcess[name] as (...a: unknown[]) => unknown; + const wrapped = (...a: unknown[]) => { + traceSpawn(a[0], a[1]); + return real(...a); + }; + // util.promisify(execFile) relies on the custom promisify symbol; keep it. + const custom = (real as unknown as Record)[util.promisify.custom]; + if (custom) Object.defineProperty(wrapped, util.promisify.custom, { value: custom }); + (childProcess as unknown as Record)[name] = wrapped; +} + +// Named ESM imports of the builtin (`import { spawn } from 'node:child_process'`) +// only see the patched functions after the CJS exports are re-synced. +module.syncBuiltinESMExports(); +record({ kind: 'worker-start' }); +beforeAll(() => { + record({ kind: 'file-start' }); +}); diff --git a/w3-1824-plant-sentinels.sh b/w3-1824-plant-sentinels.sh new file mode 100644 index 000000000..c19d1a8fe --- /dev/null +++ b/w3-1824-plant-sentinels.sh @@ -0,0 +1,49 @@ +#!/usr/bin/env bash +# SCRATCH (w3-1824): park a sentinel process on each pid the runner-session and +# runner-request-cancellation tests fabricate (4141, 4242, 4343, 4444). Linux +# allocates pids sequentially, so the script spawns throwaway subshells until +# the counter sits just below each target and then starts the sentinel there. +# No privileges, no kernel knobs: just process creation. +set -euo pipefail +LOG_DIR="${1:?log dir}" +mkdir -p "$LOG_DIR" +echo "nproc=$(nproc) mem=$(free -m | awk '/Mem:/ {print $2}')MB pid_max=$(cat /proc/sys/kernel/pid_max)" +echo "step shell: pid/ppid/pgid/sid = $(ps -o pid=,ppid=,pgid=,sid= -p $$)" +SENTINEL="$(cd "$(dirname "$0")" && pwd)/w3-1824-sentinel.cjs" + +for target in 4141 4242 4343 4444; do + if kill -0 "$target" 2>/dev/null; then + echo "pid $target already taken by: $(ps -o cmd= -p "$target")" + continue + fi + spawned=0 + while :; do + ( : ) & + p=$! + wait "$p" || true + spawned=$((spawned + 1)) + if [ "$p" -eq $((target - 1)) ]; then break; fi + if [ "$p" -gt $((target - 1)) ]; then + echo "missed $target (counter already at $p after $spawned spawns)" + p="" + break + fi + if [ "$spawned" -gt 20000 ]; then + echo "gave up on $target after $spawned spawns (counter at $p)" + p="" + break + fi + done + [ -n "$p" ] || continue + node "$SENTINEL" "$LOG_DIR/sentinel-$target.log" & + pid=$! + if [ "$pid" -eq "$target" ]; then + echo "sentinel planted at pid $pid after $spawned spawns: $(ps -o pid=,ppid=,pgid=,sid=,cmd= -p "$pid")" + echo "$pid" >> "$LOG_DIR/sentinels.txt" + else + echo "sentinel landed on $pid instead of $target; killing it" + kill "$pid" 2>/dev/null || true + fi +done +disown -a 2>/dev/null || true +echo "sentinels: $(cat "$LOG_DIR/sentinels.txt" 2>/dev/null | tr '\n' ' ')" diff --git a/w3-1824-report.sh b/w3-1824-report.sh new file mode 100644 index 000000000..e1f70ce05 --- /dev/null +++ b/w3-1824-report.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# SCRATCH (w3-1824): after the coverage run, report which sentinels survived, +# what signals they saw, whether the kernel OOM-killed anything, and every real +# signal a vitest fork sent to a foreign pid (from the kill tracer). +set -uo pipefail +LOG_DIR="${1:?log dir}" +TRACE="${2:?trace file}" + +echo "== sentinels" +if [ -f "$LOG_DIR/sentinels.txt" ]; then + while read -r pid; do + if kill -0 "$pid" 2>/dev/null; then + echo "pid $pid: ALIVE ($(ps -o cmd= -p "$pid" | cut -c1-80))" + else + echo "pid $pid: DEAD" + fi + cat "$LOG_DIR/sentinel-$pid.log" 2>/dev/null | sed 's/^/ /' + done < "$LOG_DIR/sentinels.txt" +else + echo "(none planted)" +fi + +echo "== kernel OOM / kill lines" +(sudo dmesg 2>/dev/null || dmesg 2>/dev/null) | grep -iE 'out of memory|oom-kill|killed process' || echo "(none)" + +echo "== kill trace" +if [ -f "$TRACE" ]; then + node - "$TRACE" <<'EOF' +const fs = require('node:fs'); +const lines = fs.readFileSync(process.argv[2], 'utf8').trim().split('\n').filter(Boolean).map((l) => JSON.parse(l)); +const rel = (f) => (f || '?').replace(/.*\/agent-device\/agent-device\//, ''); +const starts = lines.filter((e) => e.kind === 'file-start'); +console.log(`${lines.length} events, ${starts.length} files; worker pids ${Math.min(...starts.map((e) => e.workerPid))}..${Math.max(...starts.map((e) => e.workerPid))}; pgid(s) ${[...new Set(starts.map((e) => e.workerPgid))].join(',')}`); +const firstUnit = starts.find((e) => rel(e.file).startsWith('src/')); +if (firstUnit) console.log(`first src/ file ${rel(firstUnit.file)} started at ${firstUnit.ts} in worker pid ${firstUnit.workerPid}`); +const kills = lines.filter((e) => e.kind === 'process.kill' || e.kind === 'spawn'); +const byFile = new Map(); +for (const e of kills) { + const key = rel(e.file); + const what = e.kind === 'spawn' ? `spawn ${e.command} ${e.args.join(' ')}` : `kill ${e.pid} ${e.signal}`; + const m = byFile.get(key) ?? new Map(); + m.set(what, (m.get(what) ?? 0) + 1); + byFile.set(key, m); +} +for (const [file, m] of byFile) { + console.log(`-- ${file}`); + for (const [what, n] of m) console.log(` ${n}x ${what}`); +} +const forkPids = new Set(starts.map((e) => e.workerPid)); +const hits = kills.filter((e) => e.kind === 'process.kill' && forkPids.has(Math.abs(e.pid))); +console.log(`== signals aimed at a pid that was also a vitest fork in this run: ${hits.length}`); +for (const e of hits) console.log(` ${e.ts} ${rel(e.file)} -> kill ${e.pid} ${e.signal} (fork ran ${rel(starts.find((s) => s.workerPid === Math.abs(e.pid))?.file)})`); +EOF +else + echo "(no trace file at $TRACE)" +fi diff --git a/w3-1824-sentinel.cjs b/w3-1824-sentinel.cjs new file mode 100644 index 000000000..8815deb0e --- /dev/null +++ b/w3-1824-sentinel.cjs @@ -0,0 +1,15 @@ +// fallow-ignore-file unused-file +// SCRATCH (w3-1824): a process that sits at a chosen pid and records which +// signal reached it. SIGKILL cannot be logged; its death is the record. +const fs = require('node:fs'); +const out = process.argv[2]; +const log = (line) => + fs.appendFileSync(out, `${new Date().toISOString()} pid=${process.pid} ${line}\n`); +for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP']) { + process.on(signal, () => { + log(`received ${signal}`); + process.exit(0); + }); +} +log('started'); +setInterval(() => {}, 1_000_000); From 84ffcc744b22857c850635ee2fa25a35066f4534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 19:50:43 +0200 Subject: [PATCH 2/5] test: refuse foreign-pid signals from unit-test workers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vitest worker may signal only itself and the processes it spawned. src/__tests__/hermetic-signal-setup.ts records any other process.kill, answers it with ESRCH (so best-effort kill paths proceed as if the pid were dead), and fails the sending test by name in afterEach. The senders this catches today are the Apple runner tests, which fabricate runner child pids (4242, 4141, 4343, 4444) and mocked the liveness reads in host-process.ts but not the signal writes: killRunnerProcessTree delivered real SIGINT/SIGTERM/SIGKILL to those pids and their process groups — 146 signals per run of runner-session.test.ts. On the CI runner the sibling vitest forks live in that pid band, so the Coverage job periodically lost one fork mid-file with no test attributed (issue #1824, 6 of the last 40 red CI runs). The group-signal write moves behind signalProcessGroupBestEffort in host-process.ts, next to signalPidsBestEffort, so the runner tests mock the signal seam in the same place they already mock the liveness reads. Refs #1824 --- docs/agents/testing.md | 10 ++ src/__tests__/hermetic-signal-setup.ts | 83 +++++++++++++ src/__tests__/test-file-size-ratchet.test.ts | 2 +- .../core/__tests__/runner-disposal.test.ts | 9 +- .../runner-request-cancellation.test.ts | 7 ++ .../core/__tests__/runner-session-fixtures.ts | 111 +++++++++++++++++ .../core/__tests__/runner-session.test.ts | 116 +++--------------- .../apple/core/runner/runner-disposal.ts | 5 +- src/utils/__tests__/host-process.test.ts | 35 ++++++ src/utils/host-process.ts | 17 +++ vitest.config.ts | 1 + 11 files changed, 289 insertions(+), 107 deletions(-) create mode 100644 src/__tests__/hermetic-signal-setup.ts create mode 100644 src/platforms/apple/core/__tests__/runner-session-fixtures.ts diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 9bd5f7ef8..86b9b5cb4 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -100,6 +100,16 @@ 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 the processes it spawned; +`src/__tests__/hermetic-signal-setup.ts` refuses every other `process.kill` (signal 0, the liveness +probe, stays free) and fails the sending test by name. The refused pid is 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). If a test drives a real kill path against a fabricated pid, mock the signal +writes where it already mocks the liveness reads: `signalPidsBestEffort` and +`signalProcessGroupBestEffort` in `src/utils/host-process.ts` (or `vi.spyOn(process, 'kill')`). +Killing a daemon or Metro fixture the test itself spawned is fine — that pid is the worker's own. + 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.ts b/src/__tests__/hermetic-signal-setup.ts new file mode 100644 index 000000000..94a69eedf --- /dev/null +++ b/src/__tests__/hermetic-signal-setup.ts @@ -0,0 +1,83 @@ +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 the processes it spawned. 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 signal here, in every +// worker, turns that silent fork death into a named failure of the test that +// sent it, on any host, deterministically. +// +// Signal 0 is a liveness probe (`isProcessAlive`), not a signal; it stays free. +// A negative pid addresses a process group; it is allowed exactly when the +// group leader is a process this worker spawned (`runCmd`'s tree kill). +// Tests that drive a real kill path against a fabricated pid mock the signal +// seam (`signalPidsBestEffort` / `signalProcessGroupBestEffort` in +// `src/utils/host-process.ts`, or `vi.spyOn(process, 'kill')`) the same way +// they already mock the liveness reads. + +const ownPids = new Set([process.pid]); + +function rememberChild(child: unknown): void { + const pid = (child as { pid?: unknown } | null)?.pid; + if (typeof pid === 'number') ownPids.add(pid); +} + +function wrapSpawner unknown>(real: T): T { + const wrapped = ((...args: Parameters) => { + const child = real(...args); + rememberChild(child); + return child; + }) as unknown as T; + // `util.promisify(execFile)` resolves through the custom-promisify symbol. + const custom = (real as unknown as Record)[util.promisify.custom]; + if (custom) Object.defineProperty(wrapped, util.promisify.custom, { value: custom }); + return wrapped; +} + +for (const name of ['spawn', 'spawnSync', 'fork', 'exec', 'execFile'] as const) { + (childProcess as unknown as Record)[name] = wrapSpawner(childProcess[name]); +} +// 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 refused: string[] = []; + +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); + const state = expect.getState(); + const where = new Error().stack?.split('\n').slice(2, 6).join('\n') ?? ''; + refused.push( + `${String(signal)} -> pid ${pid} (${state.currentTestName ?? 'outside a test'})\n${where}`, + ); + // 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 ESRCH 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 send ${count} real signal(s) to a pid this vitest worker did not spawn. ` + + 'A unit test may signal only its own children (and their process groups); mock the signal ' + + 'seam in src/utils/host-process.ts (signalPidsBestEffort, signalProcessGroupBestEffort) or ' + + `vi.spyOn(process, 'kill') instead.\n${report}`, + ); +} + +afterEach(() => failOnRefusedSignals('This test')); +afterAll(() => failOnRefusedSignals('This file')); 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/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 7a88354e5..8ca027b57 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -19,6 +19,7 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = [ const SETUP_FILES = [ 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/hermetic-signal-setup.ts', 'src/__tests__/process-memo-setup.ts', // w3-1824 EXPERIMENT (temporary): per-fork kill tracer. './w3-1824-kill-trace-setup.ts', From 3e7e22724e39f59f448b243e95f6b33f22ecd618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 08:22:44 +0200 Subject: [PATCH 3/5] =?UTF-8?q?Revert=20"ci:=20w3-1824=20experiment=20?= =?UTF-8?q?=E2=80=94=20trace=20fork=20signals=20and=20plant=20pid=20sentin?= =?UTF-8?q?els=20in=20the=20Coverage=20job"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit a8a020099c756a3f4d9d510cb5d2abf33fa063af. --- .github/workflows/ci.yml | 10 ---- vitest.config.ts | 2 - w3-1824-kill-trace-setup.ts | 91 ------------------------------------- w3-1824-plant-sentinels.sh | 49 -------------------- w3-1824-report.sh | 56 ----------------------- w3-1824-sentinel.cjs | 15 ------ 6 files changed, 223 deletions(-) delete mode 100644 w3-1824-kill-trace-setup.ts delete mode 100644 w3-1824-plant-sentinels.sh delete mode 100644 w3-1824-report.sh delete mode 100644 w3-1824-sentinel.cjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3e004f553..015540029 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -338,22 +338,12 @@ jobs: uses: ./.github/actions/run-gate with: { gate: coverage-model } - # w3-1824 EXPERIMENT (temporary): sentinels on the pids the runner tests - # fabricate, plus a per-fork kill tracer wired through vitest.config.ts. - - name: w3-1824 plant sentinels - run: bash ./w3-1824-plant-sentinels.sh /tmp/w3-1824 - - name: Run coverage id: run-coverage env: OUTPUT_ECONOMY_BASE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} - W3_1824_KILL_TRACE: /tmp/w3-1824/kill-trace.ndjson uses: ./.github/actions/run-gate with: { gate: unit-ci } - - - name: w3-1824 report - if: always() - run: bash ./w3-1824-report.sh /tmp/w3-1824 /tmp/w3-1824/kill-trace.ndjson # The TMPDIR redirection both test lanes depend on (#1593/#1595). The check is a real # package script that no workflow ran: it is reachable only through `check:unit`, an # aggregate CI never invokes, so a leak regression could not fail a PR. Placed here diff --git a/vitest.config.ts b/vitest.config.ts index 8ca027b57..821932dfb 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -21,8 +21,6 @@ const SETUP_FILES = [ 'src/__tests__/hermetic-env-setup.ts', 'src/__tests__/hermetic-signal-setup.ts', 'src/__tests__/process-memo-setup.ts', - // w3-1824 EXPERIMENT (temporary): per-fork kill tracer. - './w3-1824-kill-trace-setup.ts', ]; export default defineConfig({ diff --git a/w3-1824-kill-trace-setup.ts b/w3-1824-kill-trace-setup.ts deleted file mode 100644 index 9dc347a1a..000000000 --- a/w3-1824-kill-trace-setup.ts +++ /dev/null @@ -1,91 +0,0 @@ -// fallow-ignore-file unused-file -// SCRATCH (w3-1824): traces every real signal a vitest fork sends to a foreign -// pid, plus every `kill`/`pkill` subprocess it spawns. Appends NDJSON lines to -// $W3_1824_KILL_TRACE. Never committed. -import childProcess from 'node:child_process'; -import fs from 'node:fs'; -import module from 'node:module'; -import util from 'node:util'; -import { beforeAll, expect } from 'vitest'; - -const out = process.env.W3_1824_KILL_TRACE; - -function currentTest(): { file?: string; name?: string } { - try { - const state = expect.getState(); - return { file: state.testPath, name: state.currentTestName }; - } catch { - return {}; - } -} - -function readPgid(): number | undefined { - try { - // /proc/self/stat: pid (comm) state ppid pgrp ... - const stat = fs.readFileSync('/proc/self/stat', 'utf8'); - const afterComm = stat.slice(stat.lastIndexOf(')') + 2).split(' '); - return Number(afterComm[2]); - } catch { - return undefined; - } -} - -function record(entry: Record): void { - if (!out) return; - const line = JSON.stringify({ - ts: new Date().toISOString(), - workerPid: process.pid, - workerPpid: process.ppid, - workerPgid: readPgid(), - ...currentTest(), - ...entry, - }); - fs.appendFileSync(out, `${line}\n`); -} - -const realKill = process.kill.bind(process); -process.kill = ((pid: number, signal?: string | number) => { - // signal 0 is a liveness probe; ignore self-signals. - if (signal !== 0 && pid !== process.pid) { - record({ - kind: 'process.kill', - pid, - signal: signal ?? 'SIGTERM', - stack: new Error().stack?.split('\n').slice(2, 9).join(' | '), - }); - } - return realKill(pid, signal as never); -}) as typeof process.kill; - -const KILL_BINARIES = new Set(['kill', 'pkill', 'killall']); - -function traceSpawn(cmd: unknown, args: unknown): void { - const command = String(cmd); - if (!KILL_BINARIES.has(command.split('/').pop() ?? '')) return; - record({ - kind: 'spawn', - command, - args: Array.isArray(args) ? args : [], - stack: new Error().stack?.split('\n').slice(3, 10).join(' | '), - }); -} - -for (const name of ['spawn', 'spawnSync', 'execFile', 'execFileSync'] as const) { - const real = childProcess[name] as (...a: unknown[]) => unknown; - const wrapped = (...a: unknown[]) => { - traceSpawn(a[0], a[1]); - return real(...a); - }; - // util.promisify(execFile) relies on the custom promisify symbol; keep it. - const custom = (real as unknown as Record)[util.promisify.custom]; - if (custom) Object.defineProperty(wrapped, util.promisify.custom, { value: custom }); - (childProcess as unknown as Record)[name] = wrapped; -} - -// Named ESM imports of the builtin (`import { spawn } from 'node:child_process'`) -// only see the patched functions after the CJS exports are re-synced. -module.syncBuiltinESMExports(); -record({ kind: 'worker-start' }); -beforeAll(() => { - record({ kind: 'file-start' }); -}); diff --git a/w3-1824-plant-sentinels.sh b/w3-1824-plant-sentinels.sh deleted file mode 100644 index c19d1a8fe..000000000 --- a/w3-1824-plant-sentinels.sh +++ /dev/null @@ -1,49 +0,0 @@ -#!/usr/bin/env bash -# SCRATCH (w3-1824): park a sentinel process on each pid the runner-session and -# runner-request-cancellation tests fabricate (4141, 4242, 4343, 4444). Linux -# allocates pids sequentially, so the script spawns throwaway subshells until -# the counter sits just below each target and then starts the sentinel there. -# No privileges, no kernel knobs: just process creation. -set -euo pipefail -LOG_DIR="${1:?log dir}" -mkdir -p "$LOG_DIR" -echo "nproc=$(nproc) mem=$(free -m | awk '/Mem:/ {print $2}')MB pid_max=$(cat /proc/sys/kernel/pid_max)" -echo "step shell: pid/ppid/pgid/sid = $(ps -o pid=,ppid=,pgid=,sid= -p $$)" -SENTINEL="$(cd "$(dirname "$0")" && pwd)/w3-1824-sentinel.cjs" - -for target in 4141 4242 4343 4444; do - if kill -0 "$target" 2>/dev/null; then - echo "pid $target already taken by: $(ps -o cmd= -p "$target")" - continue - fi - spawned=0 - while :; do - ( : ) & - p=$! - wait "$p" || true - spawned=$((spawned + 1)) - if [ "$p" -eq $((target - 1)) ]; then break; fi - if [ "$p" -gt $((target - 1)) ]; then - echo "missed $target (counter already at $p after $spawned spawns)" - p="" - break - fi - if [ "$spawned" -gt 20000 ]; then - echo "gave up on $target after $spawned spawns (counter at $p)" - p="" - break - fi - done - [ -n "$p" ] || continue - node "$SENTINEL" "$LOG_DIR/sentinel-$target.log" & - pid=$! - if [ "$pid" -eq "$target" ]; then - echo "sentinel planted at pid $pid after $spawned spawns: $(ps -o pid=,ppid=,pgid=,sid=,cmd= -p "$pid")" - echo "$pid" >> "$LOG_DIR/sentinels.txt" - else - echo "sentinel landed on $pid instead of $target; killing it" - kill "$pid" 2>/dev/null || true - fi -done -disown -a 2>/dev/null || true -echo "sentinels: $(cat "$LOG_DIR/sentinels.txt" 2>/dev/null | tr '\n' ' ')" diff --git a/w3-1824-report.sh b/w3-1824-report.sh deleted file mode 100644 index e1f70ce05..000000000 --- a/w3-1824-report.sh +++ /dev/null @@ -1,56 +0,0 @@ -#!/usr/bin/env bash -# SCRATCH (w3-1824): after the coverage run, report which sentinels survived, -# what signals they saw, whether the kernel OOM-killed anything, and every real -# signal a vitest fork sent to a foreign pid (from the kill tracer). -set -uo pipefail -LOG_DIR="${1:?log dir}" -TRACE="${2:?trace file}" - -echo "== sentinels" -if [ -f "$LOG_DIR/sentinels.txt" ]; then - while read -r pid; do - if kill -0 "$pid" 2>/dev/null; then - echo "pid $pid: ALIVE ($(ps -o cmd= -p "$pid" | cut -c1-80))" - else - echo "pid $pid: DEAD" - fi - cat "$LOG_DIR/sentinel-$pid.log" 2>/dev/null | sed 's/^/ /' - done < "$LOG_DIR/sentinels.txt" -else - echo "(none planted)" -fi - -echo "== kernel OOM / kill lines" -(sudo dmesg 2>/dev/null || dmesg 2>/dev/null) | grep -iE 'out of memory|oom-kill|killed process' || echo "(none)" - -echo "== kill trace" -if [ -f "$TRACE" ]; then - node - "$TRACE" <<'EOF' -const fs = require('node:fs'); -const lines = fs.readFileSync(process.argv[2], 'utf8').trim().split('\n').filter(Boolean).map((l) => JSON.parse(l)); -const rel = (f) => (f || '?').replace(/.*\/agent-device\/agent-device\//, ''); -const starts = lines.filter((e) => e.kind === 'file-start'); -console.log(`${lines.length} events, ${starts.length} files; worker pids ${Math.min(...starts.map((e) => e.workerPid))}..${Math.max(...starts.map((e) => e.workerPid))}; pgid(s) ${[...new Set(starts.map((e) => e.workerPgid))].join(',')}`); -const firstUnit = starts.find((e) => rel(e.file).startsWith('src/')); -if (firstUnit) console.log(`first src/ file ${rel(firstUnit.file)} started at ${firstUnit.ts} in worker pid ${firstUnit.workerPid}`); -const kills = lines.filter((e) => e.kind === 'process.kill' || e.kind === 'spawn'); -const byFile = new Map(); -for (const e of kills) { - const key = rel(e.file); - const what = e.kind === 'spawn' ? `spawn ${e.command} ${e.args.join(' ')}` : `kill ${e.pid} ${e.signal}`; - const m = byFile.get(key) ?? new Map(); - m.set(what, (m.get(what) ?? 0) + 1); - byFile.set(key, m); -} -for (const [file, m] of byFile) { - console.log(`-- ${file}`); - for (const [what, n] of m) console.log(` ${n}x ${what}`); -} -const forkPids = new Set(starts.map((e) => e.workerPid)); -const hits = kills.filter((e) => e.kind === 'process.kill' && forkPids.has(Math.abs(e.pid))); -console.log(`== signals aimed at a pid that was also a vitest fork in this run: ${hits.length}`); -for (const e of hits) console.log(` ${e.ts} ${rel(e.file)} -> kill ${e.pid} ${e.signal} (fork ran ${rel(starts.find((s) => s.workerPid === Math.abs(e.pid))?.file)})`); -EOF -else - echo "(no trace file at $TRACE)" -fi diff --git a/w3-1824-sentinel.cjs b/w3-1824-sentinel.cjs deleted file mode 100644 index 8815deb0e..000000000 --- a/w3-1824-sentinel.cjs +++ /dev/null @@ -1,15 +0,0 @@ -// fallow-ignore-file unused-file -// SCRATCH (w3-1824): a process that sits at a chosen pid and records which -// signal reached it. SIGKILL cannot be logged; its death is the record. -const fs = require('node:fs'); -const out = process.argv[2]; -const log = (line) => - fs.appendFileSync(out, `${new Date().toISOString()} pid=${process.pid} ${line}\n`); -for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP']) { - process.on(signal, () => { - log(`received ${signal}`); - process.exit(0); - }); -} -log('started'); -setInterval(() => {}, 1_000_000); From 090cfb9bbf27f22fefb2fe17278b4ea4ac0be3aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 09:45:13 +0200 Subject: [PATCH 4/5] test: refuse spawned kill/pkill writes too, and share the guard with the mutation lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #1854 found three gaps in the first pass: - The guard intercepted process.kill only, so the spawned half of the same function family was unguarded: runner-disposal spawns `pkill -P ` and `pkill -f 'xcodebuild.*AgentDeviceRunner.env.session-...'`, and request-router-open.test.ts fired that pattern kill twice per suite run. On a developer machine with a live Apple runner, `pnpm test` could reach it. The setup file now refuses kill/pkill/killall spawns with ENOENT — which the best-effort callers already tolerate — and records them the same way; that test stubs the Apple tool seam. - vitest.mutation.config.ts hard-coded its own setupFiles list, so the Stryker lane ran without the guard. SETUP_FILES is now exported from vitest.config.ts and imported there, next to the SUBPROCESS_STUB_TESTS import that already crossed the same boundary. - 'processes it spawned' meant direct children only; a grandchild started through a shell wrapper was refused with advice that did not fit. The docs and the failure message now say direct children and name the remedy. Synchronous spawns are no longer remembered as own pids: spawnSync and execFileSync have already exited when they return, so keeping their pids would license a signal to whatever inherits them next. Refs #1824 --- docs/agents/testing.md | 25 +++-- src/__tests__/hermetic-signal-setup.ts | 105 ++++++++++++++---- .../__tests__/request-router-open.test.ts | 13 +++ vitest.config.ts | 4 +- vitest.mutation.config.ts | 4 +- 5 files changed, 115 insertions(+), 36 deletions(-) diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 86b9b5cb4..b3c7f9ae8 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -100,15 +100,22 @@ 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 the processes it spawned; -`src/__tests__/hermetic-signal-setup.ts` refuses every other `process.kill` (signal 0, the liveness -probe, stays free) and fails the sending test by name. The refused pid is 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). If a test drives a real kill path against a fabricated pid, mock the signal -writes where it already mocks the liveness reads: `signalPidsBestEffort` and -`signalProcessGroupBestEffort` in `src/utils/host-process.ts` (or `vi.spyOn(process, 'kill')`). -Killing a daemon or Metro fixture the test itself spawned is fine — that pid is the worker's own. +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. diff --git a/src/__tests__/hermetic-signal-setup.ts b/src/__tests__/hermetic-signal-setup.ts index 94a69eedf..8b57560db 100644 --- a/src/__tests__/hermetic-signal-setup.ts +++ b/src/__tests__/hermetic-signal-setup.ts @@ -4,33 +4,85 @@ 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 the processes it spawned. Anything else is +// 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 signal here, in every +// ("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. -// A negative pid addresses a process group; it is allowed exactly when the -// group leader is a process this worker spawned (`runCmd`'s tree kill). -// Tests that drive a real kill path against a fabricated pid mock the signal -// seam (`signalPidsBestEffort` / `signalProcessGroupBestEffort` in -// `src/utils/host-process.ts`, or `vi.spyOn(process, 'kill')`) the same way -// they already mock the liveness reads. +// 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 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. + * + * Synchronous spawns (`spawnSync`, `execFileSync`) are deliberately not + * remembered: they have already exited by the time the call returns, so + * remembering them would license a signal to whatever inherits that pid next — + * the very hazard this file closes. Asynchronous children are remembered for + * the worker's lifetime rather than evicted on exit, because tests legitimately + * signal a child they have already reaped (idempotent cleanup); with + * `isolate: true` a worker runs one file, so the set stays small and + * short-lived. + */ 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 pid = (child as { pid?: unknown } | null)?.pid; if (typeof pid === 'number') ownPids.add(pid); } -function wrapSpawner unknown>(real: T): T { +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); - rememberChild(child); + if (remember) rememberChild(child); return child; }) as unknown as T; // `util.promisify(execFile)` resolves through the custom-promisify symbol. @@ -39,23 +91,26 @@ function wrapSpawner unknown>(real: T): T { return wrapped; } -for (const name of ['spawn', 'spawnSync', 'fork', 'exec', 'execFile'] as const) { - (childProcess as unknown as Record)[name] = wrapSpawner(childProcess[name]); +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 refused: string[] = []; - 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); - const state = expect.getState(); - const where = new Error().stack?.split('\n').slice(2, 6).join('\n') ?? ''; - refused.push( - `${String(signal)} -> pid ${pid} (${state.currentTestName ?? 'outside a test'})\n${where}`, - ); + 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( @@ -65,17 +120,19 @@ process.kill = ((pid: number, signal: string | number = 'SIGTERM') => { throw error; }) as typeof process.kill; -// Best-effort kill sites swallow the ESRCH above, so the refusal is reported +// 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 send ${count} real signal(s) to a pid this vitest worker did not spawn. ` + - 'A unit test may signal only its own children (and their process groups); mock the signal ' + - 'seam in src/utils/host-process.ts (signalPidsBestEffort, signalProcessGroupBestEffort) or ' + - `vi.spyOn(process, 'kill') instead.\n${report}`, + `${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}`, ); } 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/vitest.config.ts b/vitest.config.ts index 821932dfb..e15533161 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -17,7 +17,9 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = [ 'scripts/fuzz/corpus-replay.test.ts', ]; -const SETUP_FILES = [ +// 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', 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], }, }); From 10c4db8292ed1424993607820d98b813af496360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 13:24:24 +0200 Subject: [PATCH 5/5] test: end signal authority at child exit, and guard the promisified execFile path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of #1854 found two holes in the guard itself, both the class it exists to close: - An async child's pid stayed authorized for the worker's lifetime after it was reaped. A pid is a claim on a process-table slot, and the kernel reissues that slot once it is free, so 'I spawned this pid once' licensed a signal to whatever holds it now. Authority now ends on exit/close. Cleanup that signals a child must gate on isProcessAlive, which is what the existing cleanup paths already do. - wrapSpawner copied execFile's original util.promisify.custom onto the wrapper, and promisify() resolves through that symbol instead of calling the function — so every promisified caller got an unguarded execFile, bypassing both the kill-binary refusal and child tracking. That path is now wrapped too. hermetic-signal-setup.test.ts covers both, plus the allowed cases they could regress into: a reaped child's pid is refused, a promisified execFile cannot smuggle a pkill, a promisified child is still tracked, a live foreign pid is refused, and signal 0 stays free. Reverting either fix reds three of them. Refs #1824 --- src/__tests__/hermetic-signal-setup.test.ts | 94 +++++++++++++++++++++ src/__tests__/hermetic-signal-setup.ts | 64 ++++++++++---- 2 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 src/__tests__/hermetic-signal-setup.test.ts 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 index 8b57560db..2fbaf0903 100644 --- a/src/__tests__/hermetic-signal-setup.ts +++ b/src/__tests__/hermetic-signal-setup.ts @@ -25,19 +25,21 @@ import { afterAll, afterEach, expect } from 'vitest'; // reads. /** - * Direct 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. + * Direct, *live* children only. * - * Synchronous spawns (`spawnSync`, `execFileSync`) are deliberately not - * remembered: they have already exited by the time the call returns, so - * remembering them would license a signal to whatever inherits that pid next — - * the very hazard this file closes. Asynchronous children are remembered for - * the worker's lifetime rather than evicted on exit, because tests legitimately - * signal a child they have already reaped (idempotent cleanup); with - * `isolate: true` a worker runs one file, so the set stays small and - * short-lived. + * 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]); @@ -52,8 +54,14 @@ function record(what: string): void { } function rememberChild(child: unknown): void { - const pid = (child as { pid?: unknown } | null)?.pid; - if (typeof pid === 'number') ownPids.add(pid); + 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 { @@ -85,9 +93,22 @@ function wrapSpawner unknown>(real: T, remember: if (remember) rememberChild(child); return child; }) as unknown as T; - // `util.promisify(execFile)` resolves through the custom-promisify symbol. - const custom = (real as unknown as Record)[util.promisify.custom]; - if (custom) Object.defineProperty(wrapped, util.promisify.custom, { value: custom }); + // `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; } @@ -138,3 +159,12 @@ function failOnRefusedSignals(scope: string): void { 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); +}