mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 22:26:38 +03:00
Merge origin/main into rust-swap (#718 live-server leak guard)
Brings the leak guard from #718 onto the branch and makes its guarantee hold
for the Rust engine instead of the Node scripts it was written against.
Conflicts and how each was resolved:
- tests/live-poll-stream.test.mjs, tests/live-server.test.mjs,
tests/live-target-context.test.mjs (modify/delete): kept deleted. They drove
skill/scripts/live-server.mjs, which does not exist here; the verb behavior
they covered is the oracle's job now. Their entries came out of
test-suites.mjs along with the rest of main's live list, which is Node-script
coverage this branch already retired.
- scripts/test-suites.mjs: took main's two new entries that still apply,
process-group.test.mjs into core and live-server-leak.test.mjs into live, plus
the infra trigger patterns for the three new scripts/lib modules. Dropped
main's pin.test.mjs (no such file here).
- package.json: kept test:cleanup, dropped test:cli-e2e (no cli-e2e suite here).
- scripts/run-tests.mjs: rewritten to hold both sides rather than picking one.
From #718: the createGroupShutdown state machine, the per-suite run-id marker
env, the post-suite leak check, and --cleanup. From 47f18713: the per-command
wall-clock cap with its per-suite wallClockMs override and
IMPECCABLE_TEST_WALL_CLOCK_MS, plus the killed-by-signal report. The two agree
on the detached process group, so they compose: the cap SIGKILLs that group
when a command wedges, the shutdown handler ends it on a signal, and both now
sweep for leaked servers before exiting. #718's handler replaces the old raw
signal forwarding, which sent one signal and never escalated.
- tests/live-e2e/session.mjs: kept both sides. The binary-driven boot
(runEngineSync, requireEngineBin, engineEnv) stands, with armLiveServerReaper
at module scope and trackServerChild around the fixture dev server.
Ported to the rest of the branch:
- tests/oracle/lib.mjs arms the reaper and tracks the daemon child. Its daemon
steps spawn live-server detached, so a SIGKILLed oracle run used to strand
one; buildInvocation already inherits process.env, so the marker reaches it.
- tests/live-server-leak.test.mjs now boots the engine binary through
tests/lib/engine-bin.mjs and skips cleanly without one.
No crate change was needed. The daemon spawn does env_clear().envs(env) against
Io::stdio()'s env, which is std::env::vars(), so the detached Rust process
carries the parent environment and the markers reach it. Verified against a
real --background daemon: found by run id and by repo marker, not found by an
adjacent checkout's marker. CLAUDE.md now says so, since narrowing that env
would make the guard silently blind.
Verified with a fresh cargo build --release -p impeccable:
- IMPECCABLE_BIN=... bun run test green end to end: core 90, oracle 1 (zero
unreviewed differences), detector 1, live 159 (157 pass, 2 skipped),
framework 186, plugin-e2e 4. Zero servers left.
- SIGKILL repro against impeccable live-server --background: 1 daemon up, 0
after with the reaper, 1 surviving with parent pid 1 under
IMPECCABLE_NO_TEST_REAPER=1. bun run test:cleanup then kills exactly that one.
- The leak test fails under IMPECCABLE_NO_TEST_REAPER=1 and passes with it.
- IMPECCABLE_E2E_ONLY=vite8-react-plain bun run test:live-e2e 4/4.
- bun run build green.
AI assistance: prepared by Claude Code under pbakaus's direction.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY
This commit is contained in:
Paul Bakaus
Claude Code
@@ -0,0 +1,258 @@
|
||||
/**
|
||||
* Finding and killing live servers a test run left behind.
|
||||
*
|
||||
* The harness starts live servers in two shapes and neither one dies with the
|
||||
* process that started it:
|
||||
*
|
||||
* 1. a direct child (`node skill/scripts/live-server.mjs --port=N`, or
|
||||
* `$IMPECCABLE_BIN live-server --port=N` once the engine is Rust), stopped
|
||||
* by an HTTP `/stop` call in an `after()` hook;
|
||||
* 2. a detached daemon (`live-server --background`, or a full `live` boot),
|
||||
* which is orphaned to pid 1 by design and stopped by the `stop` verb.
|
||||
*
|
||||
* Both survive a runner that dies before teardown: a `SIGKILL`, a Ctrl-C, a
|
||||
* `node:test` timeout that skips the `after()` hook. This module is the safety
|
||||
* net. It identifies servers by an environment marker the runner exports, so a
|
||||
* sweep can only ever match a server this repo's test harness started; nothing
|
||||
* is matched by port, by name alone, or by "looks like impeccable".
|
||||
*
|
||||
* Every marker value is an opaque token: a random run or process id, or a hash
|
||||
* of the checkout path. None of them is a path, and none can contain
|
||||
* whitespace, which is what keeps the `ps -E` matching below exact.
|
||||
*/
|
||||
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { createHash, randomBytes } from 'node:crypto';
|
||||
import { readFileSync, readdirSync, realpathSync } from 'node:fs';
|
||||
import path from 'node:path';
|
||||
|
||||
/**
|
||||
* The three matching markers. Every value is an opaque token from the alphabet
|
||||
* below, never a path and never anything a user chose. That is what lets the
|
||||
* `ps -E` matcher work on a "starts an entry, ends at whitespace or line end"
|
||||
* rule with no ambiguous case left, and it is a load-bearing invariant rather
|
||||
* than a formatting preference: a value free to contain a space and a
|
||||
* `KEY=`-shaped token could impersonate the end of its own entry, and one
|
||||
* checkout's cleanup could then reach a neighbouring checkout's servers.
|
||||
*/
|
||||
const MARKER_VALUE_RE = /^[A-Za-z0-9_-]+$/;
|
||||
|
||||
/** Env var carrying the id of one suite command. Descendants inherit it. */
|
||||
export const RUN_ID_ENV = 'IMPECCABLE_TEST_RUN_ID';
|
||||
/**
|
||||
* Env var carrying the id of one test process. `node --test` runs files
|
||||
* concurrently and they all inherit the same run id, so a per-process id is
|
||||
* what lets one file's reaper kill that file's servers and not its siblings'.
|
||||
*/
|
||||
export const PROC_ID_ENV = 'IMPECCABLE_TEST_PROC_ID';
|
||||
/**
|
||||
* Env var carrying a hash of the checkout, so a cleanup can be scoped to it.
|
||||
* The value is `repoMarker()`, not the path: two checkouts whose paths share a
|
||||
* prefix get unrelated hashes, and no filesystem path can leak into matching.
|
||||
*/
|
||||
export const REPO_ENV = 'IMPECCABLE_TEST_REPO';
|
||||
/**
|
||||
* The checkout path in readable form, for a human looking at `ps -E` output or
|
||||
* a stuck process. **Never used for matching**, and nothing should start: it is
|
||||
* the one marker-adjacent value that can contain whitespace.
|
||||
*/
|
||||
export const REPO_PATH_ENV = 'IMPECCABLE_TEST_REPO_PATH';
|
||||
|
||||
/**
|
||||
* A command line belonging to a live server. Matches the Node script
|
||||
* (`.../live-server.mjs`) and the engine verb (`.../impeccable live-server`).
|
||||
*/
|
||||
const LIVE_SERVER_RE = /(^|[\s/\\])live-server(\.mjs|\.exe)?(\s|$)/;
|
||||
|
||||
/**
|
||||
* A stable, opaque id for one checkout: the first 16 hex characters of the
|
||||
* sha256 of its real path. Symlinked and `/private`-prefixed spellings of the
|
||||
* same directory resolve to the same marker; `/work/impeccable` and
|
||||
* `/work/impeccable-copy` do not share a prefix.
|
||||
*/
|
||||
export function repoMarker(repoRoot) {
|
||||
// path.resolve first so a trailing slash or a `.` segment cannot change the
|
||||
// marker for a directory that is not on disk (realpathSync throws for those).
|
||||
let resolved = path.resolve(repoRoot);
|
||||
try { resolved = realpathSync(resolved); } catch { /* not on disk; hash as normalized */ }
|
||||
return createHash('sha256').update(resolved).digest('hex').slice(0, 16);
|
||||
}
|
||||
|
||||
/**
|
||||
* An id for one suite command. Random, so two runs of the same suite in the
|
||||
* same second never collide, and hex/`-` only so it can never break out of its
|
||||
* own environment entry.
|
||||
*/
|
||||
export function makeRunId(repoRoot = process.cwd()) {
|
||||
return `${repoMarker(repoRoot)}-${randomBytes(8).toString('hex')}`;
|
||||
}
|
||||
|
||||
/** An id for one test process. Same alphabet rule as the run id. */
|
||||
export function makeProcId() {
|
||||
return `p${process.pid}-${randomBytes(8).toString('hex')}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Live servers still running that carry one of the given markers.
|
||||
*
|
||||
* @param {object} opts
|
||||
* Every marker is an environment entry the harness itself exported. There is
|
||||
* deliberately no fallback that matches a command line under the checkout: a
|
||||
* developer running `impeccable live` in this repo has exactly that command
|
||||
* line, and a cleanup must never be able to kill their session.
|
||||
*
|
||||
* @param {string} [opts.runId] match `IMPECCABLE_TEST_RUN_ID=<runId>` exactly.
|
||||
* @param {string} [opts.procId] match `IMPECCABLE_TEST_PROC_ID=<procId>` exactly.
|
||||
* @param {string} [opts.repo] a checkout path; matched as its `repoMarker()`
|
||||
* hash, never as the path itself.
|
||||
* @returns {{pid:number, command:string}[]}
|
||||
*/
|
||||
export function findLiveServers({ runId, procId, repo } = {}) {
|
||||
const markers = [];
|
||||
if (runId) markers.push(`${RUN_ID_ENV}=${assertMarkerValue(runId)}`);
|
||||
if (procId) markers.push(`${PROC_ID_ENV}=${assertMarkerValue(procId)}`);
|
||||
if (repo) markers.push(`${REPO_ENV}=${assertMarkerValue(repoMarker(repo))}`);
|
||||
if (!markers.length) return [];
|
||||
|
||||
const commands = listCommands();
|
||||
if (!commands.size) return [];
|
||||
|
||||
const matched = new Set(pidsWithEnvMarker(markers, commands));
|
||||
|
||||
const out = [];
|
||||
for (const pid of matched) {
|
||||
if (pid === process.pid) continue;
|
||||
const command = commands.get(pid);
|
||||
if (!command || !LIVE_SERVER_RE.test(command)) continue;
|
||||
out.push({ pid, command });
|
||||
}
|
||||
return out.sort((a, b) => a.pid - b.pid);
|
||||
}
|
||||
|
||||
/**
|
||||
* SIGTERM every process, then SIGKILL whatever is still alive after `graceMs`.
|
||||
* Kills the process group too when the process leads one, so a server that
|
||||
* spawned helpers does not leave them behind.
|
||||
*
|
||||
* @returns {number} how many processes were signalled.
|
||||
*/
|
||||
export function killLiveServers(procs, { graceMs = 400 } = {}) {
|
||||
if (!procs.length) return 0;
|
||||
for (const { pid } of procs) signal(pid, 'SIGTERM');
|
||||
const deadline = Date.now() + graceMs;
|
||||
// Busy-wait: this runs from `process.on('exit')` handlers, where the event
|
||||
// loop is already closed and nothing asynchronous can be awaited.
|
||||
while (Date.now() < deadline) {
|
||||
if (!procs.some(({ pid }) => alive(pid))) return procs.length;
|
||||
}
|
||||
for (const { pid } of procs) {
|
||||
if (alive(pid)) signal(pid, 'SIGKILL');
|
||||
}
|
||||
return procs.length;
|
||||
}
|
||||
|
||||
export function alive(pid) {
|
||||
try {
|
||||
process.kill(pid, 0);
|
||||
return true;
|
||||
} catch (err) {
|
||||
return err.code === 'EPERM';
|
||||
}
|
||||
}
|
||||
|
||||
function signal(pid, sig) {
|
||||
// A process group id is always the pid of its leader, so `-pid` can only ever
|
||||
// reach a group this process leads. When it leads none, the call is ESRCH and
|
||||
// the plain kill below is what does the work.
|
||||
try { process.kill(-pid, sig); } catch { /* not a group leader */ }
|
||||
try { process.kill(pid, sig); } catch { /* already gone */ }
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a `ps -E` line contains `marker` as a complete environment entry.
|
||||
*
|
||||
* A plain substring test is wrong here: a marker is a substring of any longer
|
||||
* value that starts with it, and matching that would let one checkout's cleanup
|
||||
* kill a neighbouring checkout's servers. `ps` flattens the environment into
|
||||
* whitespace-separated `KEY=VALUE` pairs, so an entry runs to the next
|
||||
* whitespace or to the end of the line.
|
||||
*
|
||||
* That simple rule is sound only because every marker value is drawn from
|
||||
* `MARKER_VALUE_RE` and can therefore never contain whitespace: no value can
|
||||
* hide the end of its own entry, and there is no ambiguous case. Give a marker
|
||||
* a free-form value and this becomes guesswork again, which is why
|
||||
* `assertMarkerValue` refuses one.
|
||||
*/
|
||||
export function envLineHasEntry(line, marker) {
|
||||
for (let from = 0; ; from += 1) {
|
||||
const at = line.indexOf(marker, from);
|
||||
if (at === -1) return false;
|
||||
const startsEntry = at === 0 || /\s/.test(line[at - 1]);
|
||||
const end = at + marker.length;
|
||||
const endsEntry = end === line.length || /\s/.test(line[end]);
|
||||
if (startsEntry && endsEntry) return true;
|
||||
from = at;
|
||||
}
|
||||
}
|
||||
|
||||
/** Refuse a marker value that could break out of its own environment entry. */
|
||||
function assertMarkerValue(value) {
|
||||
if (!MARKER_VALUE_RE.test(value)) {
|
||||
throw new Error(
|
||||
`refusing to match on "${value}": a marker value must be [A-Za-z0-9_-] only, ` +
|
||||
'so that an environment entry ends where the whitespace after it does. ' +
|
||||
'Hash or tokenize the value before passing it (see repoMarker).',
|
||||
);
|
||||
}
|
||||
return value;
|
||||
}
|
||||
|
||||
/** pid -> full command line, for every process this user can see. */
|
||||
function listCommands() {
|
||||
const map = new Map();
|
||||
if (process.platform === 'win32') return map; // no supported sweep yet
|
||||
const res = spawnSync('ps', ['-A', '-ww', '-o', 'pid=,command='], { encoding: 'utf-8' });
|
||||
if (res.status !== 0 || !res.stdout) return map;
|
||||
for (const line of res.stdout.split('\n')) {
|
||||
const m = /^\s*(\d+)\s+(.*)$/.exec(line);
|
||||
if (m) map.set(Number(m[1]), m[2]);
|
||||
}
|
||||
return map;
|
||||
}
|
||||
|
||||
/**
|
||||
* Pids whose environment contains one of `markers`.
|
||||
*
|
||||
* Linux exposes `/proc/<pid>/environ` directly, so entries are compared whole.
|
||||
* BSD/macOS `ps -E` appends the environment to the command column instead, so
|
||||
* the marker is matched against that combined line through `envLineHasEntry`,
|
||||
* which requires the same whole-entry boundary. The command itself is then read
|
||||
* back from the marker-free listing, so an env value that happened to contain
|
||||
* "live-server" cannot decide the match.
|
||||
*/
|
||||
function pidsWithEnvMarker(markers, commands) {
|
||||
const hits = [];
|
||||
if (process.platform === 'linux') {
|
||||
let entries = [];
|
||||
try { entries = readdirSync('/proc'); } catch { return hits; }
|
||||
for (const entry of entries) {
|
||||
if (!/^\d+$/.test(entry)) continue;
|
||||
let environ = '';
|
||||
try { environ = readFileSync(`/proc/${entry}/environ`, 'utf-8'); } catch { continue; }
|
||||
const vars = environ.split('\0');
|
||||
if (markers.some((marker) => vars.includes(marker))) hits.push(Number(entry));
|
||||
}
|
||||
return hits;
|
||||
}
|
||||
|
||||
const res = spawnSync('ps', ['-A', '-E', '-ww', '-o', 'pid=,command='], { encoding: 'utf-8' });
|
||||
if (res.status !== 0 || !res.stdout) return hits;
|
||||
for (const line of res.stdout.split('\n')) {
|
||||
const m = /^\s*(\d+)\s+(.*)$/.exec(line);
|
||||
if (!m) continue;
|
||||
const pid = Number(m[1]);
|
||||
if (!commands.has(pid)) continue;
|
||||
if (markers.some((marker) => envLineHasEntry(m[2], marker))) hits.push(pid);
|
||||
}
|
||||
return hits;
|
||||
}
|
||||
@@ -0,0 +1,146 @@
|
||||
/**
|
||||
* Stopping a spawned process group without blocking the event loop.
|
||||
*
|
||||
* The test runner puts every suite command in its own process group so one
|
||||
* signal can take down the runner, every test file it forked, and anything
|
||||
* those forked. Ending that group has one non-obvious constraint: **the wait
|
||||
* for the child to die cannot be a poll on liveness.**
|
||||
*
|
||||
* A dead child stays a zombie until its parent reaps it, and the parent here is
|
||||
* this process, which reaps through libuv when the event loop runs. So a busy
|
||||
* wait on `kill(pid, 0)` is doubly wrong: it blocks the very loop that would do
|
||||
* the reaping, and it then reads the unreaped zombie as alive. The wait would
|
||||
* always burn its full grace period and always end in a needless SIGKILL. There
|
||||
* is no waitpid from JavaScript that would see through this, so the wait has to
|
||||
* be asynchronous and keyed on the child's own `exit` event instead.
|
||||
*/
|
||||
|
||||
/** Signal the whole group, then the child itself in case it leads no group. */
|
||||
export function signalGroup(child, sig) {
|
||||
try { process.kill(-child.pid, sig); } catch { /* group already gone */ }
|
||||
try { child.kill(sig); } catch { /* already gone */ }
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrap a spawned child so its exit is observable as both a flag and a promise.
|
||||
* Register this before any other `exit` listener so `hasExited` is already true
|
||||
* by the time the others run.
|
||||
*
|
||||
* @returns {{child: import('node:child_process').ChildProcess, exited: Promise<void>, hasExited: boolean}}
|
||||
*/
|
||||
export function trackChildExit(child) {
|
||||
const running = { child, hasExited: false, exited: null };
|
||||
running.exited = new Promise((resolve) => {
|
||||
child.once('exit', () => {
|
||||
running.hasExited = true;
|
||||
resolve();
|
||||
});
|
||||
});
|
||||
return running;
|
||||
}
|
||||
|
||||
/**
|
||||
* SIGTERM the group, wait for the child to actually exit, and escalate to
|
||||
* SIGKILL only if it has not exited within `graceMs`. Returns as soon as the
|
||||
* child is gone, so a suite that dies promptly costs milliseconds rather than
|
||||
* the whole grace period.
|
||||
*
|
||||
* @returns {Promise<'exited'|'killed'|'already-gone'>}
|
||||
*/
|
||||
export async function stopGroup(running, { graceMs = 2000, killGraceMs = 500 } = {}) {
|
||||
if (!running || running.hasExited) return 'already-gone';
|
||||
|
||||
signalGroup(running.child, 'SIGTERM');
|
||||
if (await raceExit(running, graceMs)) return 'exited';
|
||||
|
||||
signalGroup(running.child, 'SIGKILL');
|
||||
await raceExit(running, killGraceMs);
|
||||
return 'killed';
|
||||
}
|
||||
|
||||
/**
|
||||
* The last resort, for `process.on('exit')` where the loop is closed and
|
||||
* nothing can be awaited. It cannot wait, so it does not pretend to: SIGTERM
|
||||
* for anything that handles it, then SIGKILL for anything that does not.
|
||||
*/
|
||||
export function killGroupSync(running) {
|
||||
if (!running || running.hasExited) return false;
|
||||
signalGroup(running.child, 'SIGTERM');
|
||||
signalGroup(running.child, 'SIGKILL');
|
||||
return true;
|
||||
}
|
||||
|
||||
function raceExit(running, ms) {
|
||||
return new Promise((resolve) => {
|
||||
const timer = setTimeout(() => resolve(false), ms);
|
||||
running.exited.then(() => {
|
||||
clearTimeout(timer);
|
||||
resolve(true);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* The runner's shutdown state machine: which group is ours, and how it ends.
|
||||
*
|
||||
* Kept here rather than as loose module state in the runner because the
|
||||
* interesting case is a state bug, not a signalling one. The first signal hands
|
||||
* the group off from `current` to `stopping` and then awaits the grace period.
|
||||
* A second signal has to be able to reach that same handle, or it kills nothing
|
||||
* and `exit` abandons the escalation still in flight, leaving a detached suite
|
||||
* running after the runner is gone. Clearing one reference without holding the
|
||||
* other is exactly how that happens.
|
||||
*
|
||||
* @param {object} [opts]
|
||||
* @param {(code: number) => void} [opts.exit] injectable for tests; in
|
||||
* production this never returns, so a shutdown that has been overtaken
|
||||
* by a second signal simply stops there.
|
||||
*/
|
||||
export function createGroupShutdown({ exit = (code) => process.exit(code), graceMs = 2000 } = {}) {
|
||||
let current = null;
|
||||
let stopping = null;
|
||||
let shuttingDown = false;
|
||||
|
||||
return {
|
||||
/** Adopt a freshly spawned group. */
|
||||
track(running) {
|
||||
current = running;
|
||||
return running;
|
||||
},
|
||||
|
||||
/** The group finished on its own; nothing left to end. */
|
||||
release() {
|
||||
current = null;
|
||||
},
|
||||
|
||||
get shuttingDown() {
|
||||
return shuttingDown;
|
||||
},
|
||||
|
||||
/** A termination signal arrived. The second one stops waiting. */
|
||||
async onSignal(exitCode) {
|
||||
if (shuttingDown) {
|
||||
// Still inside the first shutdown's grace period. Do not wait it out:
|
||||
// kill the handle that shutdown is holding, which `current` no longer
|
||||
// is, and leave.
|
||||
killGroupSync(stopping);
|
||||
exit(exitCode);
|
||||
return;
|
||||
}
|
||||
shuttingDown = true;
|
||||
stopping = current;
|
||||
current = null;
|
||||
await stopGroup(stopping, { graceMs });
|
||||
stopping = null;
|
||||
exit(exitCode);
|
||||
},
|
||||
|
||||
/** `process.on('exit')`: the last resort, and the one that cannot wait. */
|
||||
onExit() {
|
||||
const running = current || stopping;
|
||||
current = null;
|
||||
stopping = null;
|
||||
return killGroupSync(running);
|
||||
},
|
||||
};
|
||||
}
|
||||
@@ -0,0 +1,55 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* Kills the live servers of one test process once that process is gone.
|
||||
*
|
||||
* In-process cleanup (an `after()` hook, a `process.on('exit')` handler) cannot
|
||||
* run when the process is `SIGKILL`ed, and that is exactly the case that left
|
||||
* 197 orphaned servers on a laptop. So the last line of defence lives outside
|
||||
* the process: this reaper is spawned detached, holding the write end of a pipe
|
||||
* on its stdin. When its parent dies, for any reason at all, the pipe closes,
|
||||
* the reaper wakes on EOF, and it kills every live server carrying the parent's
|
||||
* process marker.
|
||||
*
|
||||
* Scope is the marker, never a name or a port: only servers this exact test
|
||||
* process started are matched.
|
||||
*
|
||||
* node scripts/lib/test-orphan-reaper.mjs <procId> [maxLifetimeMs]
|
||||
*
|
||||
* Deliberately not named after the thing it kills: its own argv must not look
|
||||
* like a live server to the sweep.
|
||||
*/
|
||||
|
||||
import { findLiveServers, killLiveServers } from './live-server-processes.mjs';
|
||||
|
||||
const procId = process.argv[2];
|
||||
const maxLifetimeMs = Number(process.argv[3]) || 6 * 60 * 60 * 1000;
|
||||
|
||||
if (!procId) {
|
||||
console.error('usage: test-orphan-reaper.mjs <procId> [maxLifetimeMs]');
|
||||
process.exit(2);
|
||||
}
|
||||
|
||||
let done = false;
|
||||
|
||||
function sweepAndExit(code = 0) {
|
||||
if (done) return;
|
||||
done = true;
|
||||
try {
|
||||
const leaked = findLiveServers({ procId });
|
||||
if (leaked.length) killLiveServers(leaked);
|
||||
} catch { /* nothing useful to report: no one is reading our output */ }
|
||||
process.exit(code);
|
||||
}
|
||||
|
||||
// The parent holds the other end of stdin. EOF means the parent is gone.
|
||||
process.stdin.resume();
|
||||
process.stdin.on('end', () => sweepAndExit(0));
|
||||
process.stdin.on('close', () => sweepAndExit(0));
|
||||
process.stdin.on('error', () => sweepAndExit(0));
|
||||
|
||||
// A reaper whose parent somehow outlives the machine's patience still goes away.
|
||||
setTimeout(() => sweepAndExit(0), maxLifetimeMs).unref();
|
||||
|
||||
for (const sig of ['SIGTERM', 'SIGINT', 'SIGHUP']) {
|
||||
process.on(sig, () => sweepAndExit(0));
|
||||
}
|
||||
+163
-44
@@ -1,6 +1,22 @@
|
||||
#!/usr/bin/env node
|
||||
import { spawn } from 'node:child_process';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { DEFAULT_SUITES, OPT_IN_SUITES, SUITES, expandSuites } from './test-suites.mjs';
|
||||
import { createGroupShutdown, trackChildExit } from './lib/process-group.mjs';
|
||||
import {
|
||||
REPO_ENV,
|
||||
REPO_PATH_ENV,
|
||||
RUN_ID_ENV,
|
||||
alive,
|
||||
findLiveServers,
|
||||
killLiveServers,
|
||||
makeRunId,
|
||||
repoMarker,
|
||||
} from './lib/live-server-processes.mjs';
|
||||
|
||||
const REPO_ROOT = path.resolve(fileURLToPath(new URL('..', import.meta.url)));
|
||||
|
||||
// Global wall-clock backstop for any one command. Even with per-test timeouts
|
||||
// and client-side network deadlines in place, a wedged tool or an orphaned
|
||||
@@ -20,6 +36,32 @@ if (args.includes('--list')) {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
if (args.includes('--cleanup')) {
|
||||
process.exit(cleanupRepoServers());
|
||||
}
|
||||
|
||||
/**
|
||||
* Suite commands run in their own process group, which buys two things: the
|
||||
* wall-clock cap can SIGKILL a wedged tree whole (the runner, its per-file
|
||||
* `node --test` workers, and any grandchildren or browsers they left open),
|
||||
* and a Ctrl-C can end that same tree deterministically instead of orphaning
|
||||
* it. Nothing in this file may use spawnSync: a blocked event loop cannot run
|
||||
* the signal handlers that make either guarantee, and cannot reap the child it
|
||||
* is waiting on either.
|
||||
*/
|
||||
const shutdown = createGroupShutdown();
|
||||
|
||||
for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) {
|
||||
process.on(sig, () => { void shutdown.onSignal(exitCodeForSignal(sig)); });
|
||||
}
|
||||
// Nothing can be awaited here, so this is the one path that does not wait.
|
||||
process.on('exit', () => shutdown.onExit());
|
||||
|
||||
/** The shell convention for "killed by signal N": 130 SIGINT, 143 SIGTERM, 129 SIGHUP. */
|
||||
function exitCodeForSignal(sig) {
|
||||
return 128 + (os.constants.signals[sig] ?? 0);
|
||||
}
|
||||
|
||||
const requestedSuites = args.filter((arg) => !arg.startsWith('-'));
|
||||
let suites;
|
||||
try {
|
||||
@@ -37,57 +79,64 @@ async function main() {
|
||||
console.log(`\n## test:${suiteName}`);
|
||||
console.log(suite.description);
|
||||
for (const command of suite.commands) {
|
||||
await runCommand(command);
|
||||
await runCommand(command, suiteName);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async function runCommand(command) {
|
||||
const env = { ...process.env, ...(command.env || {}) };
|
||||
async function runCommand(command, suiteName) {
|
||||
const runId = makeRunId(REPO_ROOT);
|
||||
const env = {
|
||||
...process.env,
|
||||
[RUN_ID_ENV]: runId,
|
||||
// The hash is what matching uses; the path rides along for a human reading
|
||||
// `ps -E` output and is never matched on.
|
||||
[REPO_ENV]: repoMarker(REPO_ROOT),
|
||||
[REPO_PATH_ENV]: REPO_ROOT,
|
||||
...(command.env || {}),
|
||||
};
|
||||
const wallClockMs = command.wallClockMs ?? DEFAULT_WALL_CLOCK_MS;
|
||||
|
||||
if (command.runner === 'bun') {
|
||||
await runProcess('bun', ['test', ...command.files], { env, wallClockMs });
|
||||
return;
|
||||
}
|
||||
|
||||
if (command.runner === 'node') {
|
||||
} else if (command.runner === 'node') {
|
||||
// One invocation for the whole file list: node --test runs each file in
|
||||
// its own child process regardless, so isolation is unchanged, but the
|
||||
// runner-per-file spawn overhead is gone and files execute concurrently.
|
||||
// Measured on the live suite (38 files): 52s serial-per-file vs 18s
|
||||
// batched at concurrency 4. Suites can pin `concurrency: 1` if their
|
||||
// tests ever contend for a shared resource.
|
||||
const args = ['--test', `--test-concurrency=${command.concurrency ?? 4}`];
|
||||
if (command.timeoutMs) args.push(`--test-timeout=${command.timeoutMs}`);
|
||||
if (command.forceExit) args.push('--test-force-exit');
|
||||
args.push(...command.files);
|
||||
await runProcess(process.execPath, args, { env, wallClockMs });
|
||||
return;
|
||||
const nodeArgs = ['--test', `--test-concurrency=${command.concurrency ?? 4}`];
|
||||
if (command.timeoutMs) nodeArgs.push(`--test-timeout=${command.timeoutMs}`);
|
||||
if (command.forceExit) nodeArgs.push('--test-force-exit');
|
||||
nodeArgs.push(...command.files);
|
||||
await runProcess(process.execPath, nodeArgs, { env, wallClockMs });
|
||||
} else {
|
||||
throw new Error(`Unsupported test runner "${command.runner}"`);
|
||||
}
|
||||
|
||||
throw new Error(`Unsupported test runner "${command.runner}"`);
|
||||
await assertNoLeakedServers(runId, suiteName);
|
||||
}
|
||||
|
||||
// Spawn as a detached process-group leader so the wall-clock cap can SIGKILL
|
||||
// the entire tree — the runner, its per-file `node --test` workers, and any
|
||||
// grandchildren or browsers they left open — not just the top process. A
|
||||
// blocked spawnSync inside a test can't be reached by node's `--test-timeout`;
|
||||
// this group kill is the guaranteed cleanup that lets the sweep always end.
|
||||
function runProcess(cmd, args, { env, wallClockMs }) {
|
||||
console.log(`$ ${formatCommand(cmd, args)}`);
|
||||
return new Promise((resolve) => {
|
||||
console.log(`$ ${formatCommand(cmd, args)}`);
|
||||
const child = spawn(cmd, args, { stdio: 'inherit', env, detached: true });
|
||||
const child = spawn(cmd, args, {
|
||||
// Own process group: the wall-clock cap and the shutdown handler can
|
||||
// then take down the runner, every test file it forked, and anything
|
||||
// those forked, in one signal.
|
||||
detached: true,
|
||||
// stdin is deliberately not inherited. A detached child is a background
|
||||
// process group on the terminal, and a background read of the tty stops
|
||||
// the process with SIGTTIN. No suite reads the runner's stdin.
|
||||
stdio: ['ignore', 'inherit', 'inherit'],
|
||||
env,
|
||||
});
|
||||
// Registered before the handlers below, so `hasExited` is already set by
|
||||
// the time they run and a shutdown mid-exit does not signal a dead pid.
|
||||
const running = shutdown.track(trackChildExit(child));
|
||||
|
||||
let timedOut = false;
|
||||
// A detached child is its own group leader, so an interactive Ctrl-C on
|
||||
// the runner no longer reaches it. Forward the interrupt to the group so
|
||||
// the whole tree is torn down instead of orphaned.
|
||||
const forward = (signal) => () => {
|
||||
try { process.kill(-child.pid, signal); } catch { try { child.kill(signal); } catch { /* gone */ } }
|
||||
};
|
||||
const onInt = forward('SIGINT');
|
||||
const onTerm = forward('SIGTERM');
|
||||
process.on('SIGINT', onInt);
|
||||
process.on('SIGTERM', onTerm);
|
||||
const timer = wallClockMs
|
||||
? setTimeout(() => {
|
||||
timedOut = true;
|
||||
@@ -95,30 +144,99 @@ function runProcess(cmd, args, { env, wallClockMs }) {
|
||||
`\n[run-tests] wall-clock cap of ${wallClockMs}ms exceeded for "${formatCommand(cmd, args)}"; ` +
|
||||
'killing the process group (SIGKILL).',
|
||||
);
|
||||
try { process.kill(-child.pid, 'SIGKILL'); }
|
||||
catch { try { child.kill('SIGKILL'); } catch { /* already gone */ } }
|
||||
// A test blocked in a synchronous spawnSync cannot be reached by
|
||||
// node's --test-timeout, so this is the guaranteed end of the tree.
|
||||
// No graceful phase: the cap has already been generous.
|
||||
try { process.kill(-running.child.pid, 'SIGKILL'); }
|
||||
catch { try { running.child.kill('SIGKILL'); } catch { /* already gone */ } }
|
||||
}, wallClockMs)
|
||||
: null;
|
||||
const cleanup = () => {
|
||||
if (timer) clearTimeout(timer);
|
||||
process.off('SIGINT', onInt);
|
||||
process.off('SIGTERM', onTerm);
|
||||
};
|
||||
|
||||
child.on('error', (err) => {
|
||||
cleanup();
|
||||
if (timer) clearTimeout(timer);
|
||||
shutdown.release();
|
||||
console.error(err.message);
|
||||
process.exit(1);
|
||||
});
|
||||
child.on('exit', (code, signal) => {
|
||||
cleanup();
|
||||
if (timedOut) process.exit(1);
|
||||
if (signal) { console.error(`[run-tests] "${formatCommand(cmd, args)}" killed by signal ${signal}`); process.exit(1); }
|
||||
if (code !== 0) process.exit(code || 1);
|
||||
if (timer) clearTimeout(timer);
|
||||
shutdown.release();
|
||||
if (shutdown.shuttingDown) return;
|
||||
if (timedOut) {
|
||||
// A wedged suite is one of the ways servers are left behind, so sweep
|
||||
// before reporting rather than walking away from them.
|
||||
assertNoLeakedServers(env[RUN_ID_ENV], null).finally(() => process.exit(1));
|
||||
return;
|
||||
}
|
||||
if (signal) {
|
||||
console.error(`[run-tests] "${formatCommand(cmd, args)}" killed by signal ${signal}`);
|
||||
assertNoLeakedServers(env[RUN_ID_ENV], null).finally(() => process.exit(1));
|
||||
return;
|
||||
}
|
||||
if (code !== 0) {
|
||||
// Leaked servers are still worth reporting on a failing suite: a
|
||||
// failure before teardown is one of the ways they are left behind.
|
||||
assertNoLeakedServers(env[RUN_ID_ENV], null).finally(() => process.exit(code || 1));
|
||||
return;
|
||||
}
|
||||
resolve();
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Fail the run when a suite left live servers behind.
|
||||
*
|
||||
* The whole point of the guard is that a leak shows up in the run that caused
|
||||
* it rather than as a wedged port days later, so it is an error, not a warning.
|
||||
* The leaked servers are killed either way, so the next suite still gets its
|
||||
* ports.
|
||||
*/
|
||||
async function assertNoLeakedServers(runId, suiteName) {
|
||||
if (!runId || process.env.IMPECCABLE_SKIP_LEAK_CHECK === '1') return;
|
||||
// A server asked to stop needs a moment to actually go.
|
||||
let leaked = [];
|
||||
for (let attempt = 0; attempt < 10; attempt += 1) {
|
||||
leaked = findLiveServers({ runId });
|
||||
if (!leaked.length) return;
|
||||
await new Promise((r) => setTimeout(r, 200));
|
||||
}
|
||||
|
||||
killLiveServers(leaked);
|
||||
const label = suiteName ? `test:${suiteName}` : 'the suite';
|
||||
console.error(`\nLeaked live servers: ${label} left ${leaked.length} live server process(es) running.`);
|
||||
for (const { pid, command } of leaked) console.error(` pid ${pid} ${command}`);
|
||||
console.error('They have been killed. A live server outliving its suite means a teardown path');
|
||||
console.error('was skipped; see tests/lib/live-servers.mjs for how servers are meant to be tracked.');
|
||||
console.error('Set IMPECCABLE_SKIP_LEAK_CHECK=1 to bypass this check.');
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
/**
|
||||
* `bun run test:cleanup`: kill live servers this checkout's tests left behind.
|
||||
*
|
||||
* Scoped to servers carrying this checkout's `IMPECCABLE_TEST_REPO` marker, so
|
||||
* a live session the developer started themselves in this same repo is not a
|
||||
* candidate. A server from a run that predates the marker is not found here and
|
||||
* has to be killed by hand.
|
||||
*/
|
||||
function cleanupRepoServers() {
|
||||
const leaked = findLiveServers({ repo: REPO_ROOT });
|
||||
if (!leaked.length) {
|
||||
console.log('No leftover live servers from this repo\'s test runs.');
|
||||
return 0;
|
||||
}
|
||||
for (const { pid, command } of leaked) console.log(`killing pid ${pid} ${command}`);
|
||||
killLiveServers(leaked);
|
||||
const survivors = leaked.filter(({ pid }) => alive(pid));
|
||||
if (survivors.length) {
|
||||
console.error(`Could not kill ${survivors.length} process(es): ${survivors.map((p) => p.pid).join(', ')}`);
|
||||
return 1;
|
||||
}
|
||||
console.log(`Killed ${leaked.length} leftover live server process(es).`);
|
||||
return 0;
|
||||
}
|
||||
|
||||
function formatCommand(cmd, args) {
|
||||
const bin = cmd === process.execPath ? 'node' : cmd;
|
||||
return [bin, ...args].join(' ');
|
||||
@@ -132,7 +250,8 @@ Aliases:
|
||||
all-local ${DEFAULT_SUITES.join(', ')}
|
||||
all ${[...DEFAULT_SUITES, ...OPT_IN_SUITES].join(', ')}
|
||||
|
||||
Run with --list to see suite contents.`);
|
||||
Run with --list to see suite contents.
|
||||
Run with --cleanup to kill live servers a previous run left behind.`);
|
||||
}
|
||||
|
||||
function printSuites() {
|
||||
|
||||
@@ -17,6 +17,10 @@ const COMMON_INFRA_PATTERNS = [
|
||||
/^scripts\/run-tests\.mjs$/,
|
||||
/^scripts\/test-suites\.mjs$/,
|
||||
/^scripts\/ci-test-plan\.mjs$/,
|
||||
/^scripts\/lib\/(live-server-processes|process-group|test-orphan-reaper)\.mjs$/,
|
||||
/^tests\/lib\/live-servers\.mjs$/,
|
||||
/^scripts\/lib\/(live-server-processes|process-group|test-orphan-reaper)\.mjs$/,
|
||||
/^tests\/lib\/live-servers\.mjs$/,
|
||||
/^\.github\/workflows\/ci\.yml$/,
|
||||
];
|
||||
|
||||
@@ -62,6 +66,7 @@ export const SUITES = {
|
||||
'tests/github-sheriff.test.mjs',
|
||||
'tests/hook-build.test.mjs',
|
||||
'tests/openai-plugin.test.mjs',
|
||||
'tests/process-group.test.mjs',
|
||||
'tests/release.test.mjs',
|
||||
'tests/skill-reference.test.mjs',
|
||||
'tests/readme-gitignore.test.mjs',
|
||||
@@ -128,6 +133,7 @@ export const SUITES = {
|
||||
'tests/live-e2e-llm-agent.test.mjs',
|
||||
'tests/live-e2e-steer-agent.test.mjs',
|
||||
'tests/live-e2e/agent-insert.test.mjs',
|
||||
'tests/live-server-leak.test.mjs',
|
||||
],
|
||||
},
|
||||
],
|
||||
|
||||
Reference in New Issue
Block a user