Skip to content

Commit 090cfb9

Browse files
thymikeethymikee
committed
test: refuse spawned kill/pkill writes too, and share the guard with the mutation lane
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 <pid>` 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
1 parent 3e7e227 commit 090cfb9

5 files changed

Lines changed: 115 additions & 36 deletions

File tree

‎docs/agents/testing.md‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -100,15 +100,22 @@ the runtime seam existed; retiring `dispatchCommand('snapshot')` surfaced them o
100100
time. Do not add to that set, and when a command migrates (`docs/agents/adr-0019-unit.md`), its
101101
tests move to the runtime seam in the same PR.
102102

103-
Signals are hermetic too. A vitest worker may signal only itself and the processes it spawned;
104-
`src/__tests__/hermetic-signal-setup.ts` refuses every other `process.kill` (signal 0, the liveness
105-
probe, stays free) and fails the sending test by name. The refused pid is one the test made up
106-
(`child: { pid: 4242 }`), and on a real host that number can belong to anyone — on CI it periodically
107-
belonged to a sibling fork, which died mid-file with no test attributed ("Worker exited
108-
unexpectedly", #1824). If a test drives a real kill path against a fabricated pid, mock the signal
109-
writes where it already mocks the liveness reads: `signalPidsBestEffort` and
110-
`signalProcessGroupBestEffort` in `src/utils/host-process.ts` (or `vi.spyOn(process, 'kill')`).
111-
Killing a daemon or Metro fixture the test itself spawned is fine — that pid is the worker's own.
103+
Signals are hermetic too. A vitest worker may signal only itself and its own **direct children**;
104+
`src/__tests__/hermetic-signal-setup.ts` refuses every other process-table write and fails the
105+
sending test by name. It covers both ways out of the process, because the runner-disposal family
106+
uses both: `process.kill` (signal 0, the liveness probe, stays free) and a spawned
107+
`kill`/`pkill`/`killall`, whose `-P` and `-f` forms reach processes the worker never spawned at all.
108+
The refused pid is usually one the test made up (`child: { pid: 4242 }`), and on a real host that
109+
number can belong to anyone — on CI it periodically belonged to a sibling fork, which died mid-file
110+
with no test attributed ("Worker exited unexpectedly", #1824); a `pkill -f 'xcodebuild.*'` from a
111+
unit test would find a developer's live runner.
112+
113+
If a test drives a real kill path against a fabricated pid, mock the seam where it already mocks the
114+
liveness reads: `signalPidsBestEffort` / `signalProcessGroupBestEffort` in `src/utils/host-process.ts`
115+
for direct writes, the exec or tool-provider seam for a spawned `pkill`. Killing a daemon or Metro
116+
fixture the test itself spawned is fine — that pid is the worker's own. *Direct* is literal: a
117+
grandchild started through a shell or `npx` wrapper is not tracked, so signal the direct child, or
118+
its process group through the negative pid, rather than the grandchild's pid.
112119

113120
Keep tests behavioral. Do not assert shapes or cases TypeScript already proves.
114121

‎src/__tests__/hermetic-signal-setup.ts‎

Lines changed: 81 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,33 +4,85 @@ import util from 'node:util';
44
import { afterAll, afterEach, expect } from 'vitest';
55

66
// Unit tests must be hermetic with respect to the host's process table: a
7-
// worker may signal only itself and the processes it spawned. Anything else is
7+
// worker may signal only itself and its own direct children. Anything else is
88
// a pid the test made up (`child: { pid: 4242 }`), and on a real host that
99
// number can belong to anyone — on CI it periodically belonged to a sibling
1010
// vitest fork, which died mid-file with no test attributed
11-
// ("Worker exited unexpectedly", #1824). Refusing the signal here, in every
11+
// ("Worker exited unexpectedly", #1824). Refusing the write here, in every
1212
// worker, turns that silent fork death into a named failure of the test that
1313
// sent it, on any host, deterministically.
1414
//
15+
// Both ways out of the process are covered, because the runner-disposal family
16+
// uses both: `process.kill` (including the negative pid that addresses a
17+
// child's process group) and a spawned `kill`/`pkill`/`killall`, whose `-P`
18+
// and `-f` forms reach processes this worker never spawned at all.
19+
//
1520
// Signal 0 is a liveness probe (`isProcessAlive`), not a signal; it stays free.
16-
// A negative pid addresses a process group; it is allowed exactly when the
17-
// group leader is a process this worker spawned (`runCmd`'s tree kill).
18-
// Tests that drive a real kill path against a fabricated pid mock the signal
19-
// seam (`signalPidsBestEffort` / `signalProcessGroupBestEffort` in
20-
// `src/utils/host-process.ts`, or `vi.spyOn(process, 'kill')`) the same way
21-
// they already mock the liveness reads.
21+
// Tests that drive a real kill path against a fabricated pid mock the seam —
22+
// `signalPidsBestEffort` / `signalProcessGroupBestEffort` in
23+
// `src/utils/host-process.ts` for the direct writes, the exec/tool-provider
24+
// seam for a spawned `pkill` — the same way they already mock the liveness
25+
// reads.
2226

27+
/**
28+
* Direct children only. A grandchild (a server started through a shell or
29+
* `npx` wrapper) is not tracked, so signalling its pid is refused even though
30+
* the test legitimately owns it; signal the direct child, or its process group
31+
* via the negative pid, or mock the seam.
32+
*
33+
* Synchronous spawns (`spawnSync`, `execFileSync`) are deliberately not
34+
* remembered: they have already exited by the time the call returns, so
35+
* remembering them would license a signal to whatever inherits that pid next —
36+
* the very hazard this file closes. Asynchronous children are remembered for
37+
* the worker's lifetime rather than evicted on exit, because tests legitimately
38+
* signal a child they have already reaped (idempotent cleanup); with
39+
* `isolate: true` a worker runs one file, so the set stays small and
40+
* short-lived.
41+
*/
2342
const ownPids = new Set<number>([process.pid]);
2443

44+
const KILL_BINARIES = new Set(['kill', 'pkill', 'killall']);
45+
46+
const refused: string[] = [];
47+
48+
function record(what: string): void {
49+
const state = expect.getState();
50+
const where = new Error().stack?.split('\n').slice(3, 7).join('\n') ?? '';
51+
refused.push(`${what} (${state.currentTestName ?? 'outside a test'})\n${where}`);
52+
}
53+
2554
function rememberChild(child: unknown): void {
2655
const pid = (child as { pid?: unknown } | null)?.pid;
2756
if (typeof pid === 'number') ownPids.add(pid);
2857
}
2958

30-
function wrapSpawner<T extends (...args: never[]) => unknown>(real: T): T {
59+
function killBinaryName(command: unknown): string | undefined {
60+
// `exec` takes a shell command line; the rest take an executable path.
61+
const first = String(command).trim().split(/\s+/)[0] ?? '';
62+
const name = first.split('/').pop() ?? '';
63+
return KILL_BINARIES.has(name) ? name : undefined;
64+
}
65+
66+
function refuseSpawnedKill(command: unknown, args: unknown): Error | undefined {
67+
const name = killBinaryName(command);
68+
if (!name) return undefined;
69+
const rendered = [String(command), ...(Array.isArray(args) ? args.map(String) : [])].join(' ');
70+
record(`spawn ${rendered}`);
71+
// Shaped like the binary being unavailable, which every caller of these
72+
// best-effort kills already tolerates.
73+
const error = new Error(
74+
`Refusing to spawn \`${rendered}\`: a unit test may not signal processes through ${name} (see afterEach failure).`,
75+
) as NodeJS.ErrnoException;
76+
error.code = 'ENOENT';
77+
return error;
78+
}
79+
80+
function wrapSpawner<T extends (...args: never[]) => unknown>(real: T, remember: boolean): T {
3181
const wrapped = ((...args: Parameters<T>) => {
82+
const refusal = refuseSpawnedKill(args[0], args[1]);
83+
if (refusal) throw refusal;
3284
const child = real(...args);
33-
rememberChild(child);
85+
if (remember) rememberChild(child);
3486
return child;
3587
}) as unknown as T;
3688
// `util.promisify(execFile)` resolves through the custom-promisify symbol.
@@ -39,23 +91,26 @@ function wrapSpawner<T extends (...args: never[]) => unknown>(real: T): T {
3991
return wrapped;
4092
}
4193

42-
for (const name of ['spawn', 'spawnSync', 'fork', 'exec', 'execFile'] as const) {
43-
(childProcess as unknown as Record<string, unknown>)[name] = wrapSpawner(childProcess[name]);
94+
for (const name of ['spawn', 'fork', 'exec', 'execFile'] as const) {
95+
(childProcess as unknown as Record<string, unknown>)[name] = wrapSpawner(
96+
childProcess[name],
97+
true,
98+
);
99+
}
100+
for (const name of ['spawnSync', 'execFileSync', 'execSync'] as const) {
101+
(childProcess as unknown as Record<string, unknown>)[name] = wrapSpawner(
102+
childProcess[name],
103+
false,
104+
);
44105
}
45106
// Named ESM imports of the builtin (`import { spawn } from 'node:child_process'`)
46107
// bind to the CJS exports only after they are re-synced.
47108
module.syncBuiltinESMExports();
48109

49-
const refused: string[] = [];
50-
51110
const realKill = process.kill.bind(process);
52111
process.kill = ((pid: number, signal: string | number = 'SIGTERM') => {
53112
if (signal === 0 || ownPids.has(Math.abs(pid))) return realKill(pid, signal as NodeJS.Signals);
54-
const state = expect.getState();
55-
const where = new Error().stack?.split('\n').slice(2, 6).join('\n') ?? '';
56-
refused.push(
57-
`${String(signal)} -> pid ${pid} (${state.currentTestName ?? 'outside a test'})\n${where}`,
58-
);
113+
record(`${String(signal)} -> pid ${pid}`);
59114
// Behave like a dead pid: the caller's ESRCH handling runs, no signal leaves
60115
// this worker, and the record above fails the test in afterEach below.
61116
const error = new Error(
@@ -65,17 +120,19 @@ process.kill = ((pid: number, signal: string | number = 'SIGTERM') => {
65120
throw error;
66121
}) as typeof process.kill;
67122

68-
// Best-effort kill sites swallow the ESRCH above, so the refusal is reported
123+
// Best-effort kill sites swallow the errors above, so the refusal is reported
69124
// here, attributed to the test (or file) that caused it, and never lost.
70125
function failOnRefusedSignals(scope: string): void {
71126
if (refused.length === 0) return;
72127
const count = refused.length;
73128
const report = refused.splice(0).join('\n');
74129
throw new Error(
75-
`${scope} tried to send ${count} real signal(s) to a pid this vitest worker did not spawn. ` +
76-
'A unit test may signal only its own children (and their process groups); mock the signal ' +
77-
'seam in src/utils/host-process.ts (signalPidsBestEffort, signalProcessGroupBestEffort) or ' +
78-
`vi.spyOn(process, 'kill') instead.\n${report}`,
130+
`${scope} tried to signal ${count} process(es) this vitest worker did not spawn. ` +
131+
'A unit test may signal only itself and its own direct children (and their process ' +
132+
'groups); mock the seam instead — signalPidsBestEffort / signalProcessGroupBestEffort in ' +
133+
'src/utils/host-process.ts, the exec or tool-provider seam for a spawned pkill, or ' +
134+
`vi.spyOn(process, 'kill'). If the pid is a descendant this test really owns, signal the ` +
135+
`direct child (or its group via the negative pid) rather than the grandchild.\n${report}`,
79136
);
80137
}
81138

‎src/daemon/__tests__/request-router-open.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,19 @@ vi.mock('../../utils/host-process.ts', async (importOriginal) => {
1212
const actual = await importOriginal<typeof import('../../utils/host-process.ts')>();
1313
return { ...actual, readProcessStartTime: vi.fn(() => 'test-process-start') };
1414
});
15+
// Opening a session runs the owned-lease cleanup, which pattern-kills stale
16+
// xcodebuild runners with a real `pkill -f`. The session id here is fabricated,
17+
// so on a host with a live Apple runner that write would reach a process this
18+
// test does not own; stub the tool seam the way the runner tests stub the
19+
// signal seam (#1824).
20+
vi.mock('../../platforms/apple/core/tool-provider.ts', async (importOriginal) => {
21+
const actual =
22+
await importOriginal<typeof import('../../platforms/apple/core/tool-provider.ts')>();
23+
return {
24+
...actual,
25+
runAppleToolCommand: vi.fn(async () => ({ exitCode: 0, stdout: '', stderr: '' })),
26+
};
27+
});
1528

1629
import { dispatchCommand } from '../../core/dispatch.ts';
1730
import {

‎vitest.config.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = [
1717
'scripts/fuzz/corpus-replay.test.ts',
1818
];
1919

20-
const SETUP_FILES = [
20+
// Imported by vitest.mutation.config.ts so the two lanes cannot drift: a guard
21+
// added here must reach the Stryker sandbox too.
22+
export const SETUP_FILES = [
2123
'src/__tests__/hermetic-env-setup.ts',
2224
'src/__tests__/hermetic-signal-setup.ts',
2325
'src/__tests__/process-memo-setup.ts',

‎vitest.mutation.config.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { fileURLToPath } from 'node:url';
33
import { defineConfig } from 'vitest/config';
44
import { readTestScope, threadHostileTestFiles } from './scripts/mutation/test-scope.ts';
55
import { workspaceSourceAliases } from './scripts/mutation/workspace-aliases.ts';
6-
import { SUBPROCESS_STUB_TESTS } from './vitest.config.ts';
6+
import { SETUP_FILES, SUBPROCESS_STUB_TESTS } from './vitest.config.ts';
77

88
const repoRoot = path.dirname(fileURLToPath(import.meta.url));
99

@@ -36,6 +36,6 @@ export default defineConfig({
3636
test: {
3737
include: scope ?? ['src/**/*.test.ts', 'packages/*/src/**/*.test.ts'],
3838
exclude: [...SUBPROCESS_STUB_TESTS, ...threadHostileTestFiles(repoRoot), '**/node_modules/**'],
39-
setupFiles: ['src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts'],
39+
setupFiles: [...SETUP_FILES],
4040
},
4141
});

0 commit comments

Comments
 (0)