diff --git a/CLAUDE.md b/CLAUDE.md index dc3dae89a..b068c399d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -140,10 +140,23 @@ bun run test # Default suite: unit + static framework fixtures bun run test:live-e2e # Opt-in: full-cycle live-mode E2E across framework fixtures bun run test:skill-behavior # Opt-in: LLM-backed checks that the skill text actually drives the agent's setup flow bun run test:plugin-e2e # Just the plugin loader E2E (also part of the default suite) +bun run test:cleanup # Kill live servers a previous run of THIS checkout left behind ``` Unit tests (build orchestration, detector logic) run via `bun test`. Fixture tests (jsdom-based HTML detection) run via `node --test` because bun is too slow with jsdom. The `test` script handles this split automatically. +### Live servers must not outlive their test process + +A live server does not die with the process that started it: a direct child survives its parent, and `live-server --background` is orphaned to pid 1 by design. Teardown in an `after()` hook or a `finally` covers only the exits JavaScript can observe, so a `SIGKILL`, a Ctrl-C, or a wedged runner used to leave servers squatting the live suite's fixed ports for days (issue #717). + +Three pieces keep that from recurring, and a new test that starts a server owes the first one: + +- **`armLiveServerReaper()`** (`tests/lib/live-servers.mjs`), called once at module scope by any test file that starts a live server. It stamps the process environment with a unique marker, installs exit and signal handlers, and spawns a detached reaper holding a pipe to the process. When the process dies for any reason at all, the pipe closes and the reaper kills the servers carrying that marker. Wrap direct children in `trackServerChild()` so the common case is a cheap `child.kill()`. This is deliberately implementation-agnostic: it works the same for the Node scripts and for the Rust `impeccable live-server`. +- **The runner guard.** `scripts/run-tests.mjs` runs each suite command as its own process-group leader, forwards `SIGINT` / `SIGTERM` to the group, and after every suite checks whether any live server carrying that suite's run id is still alive. If one is, it kills it and fails the run. Bypass with `IMPECCABLE_SKIP_LEAK_CHECK=1`. +- **`bun run test:cleanup`.** A one-shot sweep for leftovers from earlier runs. + +**Everything that kills is scoped by an environment marker this repo's harness exported**, never by process name, port, or path. A sweep can never touch a live server that another checkout, or the user's own session, is running. Keep it that way, and keep marker values opaque: every one is a random token or a hash of the checkout path (`repoMarker()`), drawn from `[A-Za-z0-9_-]` so it can never contain whitespace. `ps -E` flattens the environment into one whitespace-separated line, so a value free to hold a space could hide the end of its own entry and let one checkout's cleanup reach another's servers. `assertMarkerValue` refuses such a value; the readable path travels separately as `IMPECCABLE_TEST_REPO_PATH`, which nothing matches on. + ### Which opt-in suite a change owes The default suite does not cover everything. When a change touches one of these areas, run the matching opt-in suite before shipping. The canonical mapping is the `triggers` lists in `scripts/test-suites.mjs`; this table mirrors it for the areas that need a manual run. diff --git a/package.json b/package.json index 6cdb56656..962a311a7 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ "test:detector": "node scripts/run-tests.mjs detector", "test:framework": "node scripts/run-tests.mjs framework", "test:live": "node scripts/run-tests.mjs live", + "test:cleanup": "node scripts/run-tests.mjs --cleanup", "test:cli-e2e": "node scripts/run-tests.mjs cli-e2e", "test:cli-remote-e2e": "node scripts/run-tests.mjs cli-remote-e2e", "test:plugin-e2e": "node scripts/run-tests.mjs plugin-e2e", diff --git a/scripts/lib/live-server-processes.mjs b/scripts/lib/live-server-processes.mjs new file mode 100644 index 000000000..0ff2378c0 --- /dev/null +++ b/scripts/lib/live-server-processes.mjs @@ -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=` exactly. + * @param {string} [opts.procId] match `IMPECCABLE_TEST_PROC_ID=` 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//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; +} diff --git a/scripts/lib/process-group.mjs b/scripts/lib/process-group.mjs new file mode 100644 index 000000000..d899b8d33 --- /dev/null +++ b/scripts/lib/process-group.mjs @@ -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, 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); + }, + }; +} diff --git a/scripts/lib/test-orphan-reaper.mjs b/scripts/lib/test-orphan-reaper.mjs new file mode 100644 index 000000000..fb0bc7ba1 --- /dev/null +++ b/scripts/lib/test-orphan-reaper.mjs @@ -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 [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 [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)); +} diff --git a/scripts/run-tests.mjs b/scripts/run-tests.mjs index 556d5a54e..3b02e5427 100644 --- a/scripts/run-tests.mjs +++ b/scripts/run-tests.mjs @@ -1,7 +1,22 @@ #!/usr/bin/env node -import { spawnSync } from 'node:child_process'; +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))); const args = process.argv.slice(2); if (args.includes('--help') || args.includes('-h')) { @@ -14,6 +29,29 @@ if (args.includes('--list')) { process.exit(0); } +if (args.includes('--cleanup')) { + process.exit(cleanupRepoServers()); +} + +/** + * Suite commands run in their own process group so a Ctrl-C, a timeout, or an + * exiting runner can take the whole tree down at once. Nothing else in this + * file may use spawnSync: a blocked event loop cannot run the signal handlers + * that make that 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 { @@ -28,46 +66,132 @@ for (const suiteName of suites) { console.log(`\n## test:${suiteName}`); console.log(suite.description); for (const command of suite.commands) { - runCommand(command); + await runCommand(command, suiteName); } } -function runCommand(command) { - const env = { ...process.env, ...(command.env || {}) }; - if (command.runner === 'bun') { - runProcess('bun', ['test', ...command.files], { env }); - return; - } +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 || {}), + }; - if (command.runner === 'node') { + if (command.runner === 'bun') { + await runProcess('bun', ['test', ...command.files], { env }); + } 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); - runProcess(process.execPath, args, { env }); - 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 }); + } else { + throw new Error(`Unsupported test runner "${command.runner}"`); } - throw new Error(`Unsupported test runner "${command.runner}"`); + await assertNoLeakedServers(runId, suiteName); } function runProcess(cmd, args, { env }) { console.log(`$ ${formatCommand(cmd, args)}`); - const result = spawnSync(cmd, args, { - stdio: 'inherit', - env, + return new Promise((resolve) => { + const child = spawn(cmd, args, { + // Own process group: killCurrentGroup() 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. + shutdown.track(trackChildExit(child)); + + child.on('error', (err) => { + shutdown.release(); + console.error(err.message); + process.exit(1); + }); + child.on('exit', (code) => { + shutdown.release(); + if (shutdown.shuttingDown) 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(); + }); }); - if (result.error) { - console.error(result.error.message); - process.exit(1); +} + +/** + * 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)); } - if (result.status !== 0) process.exit(result.status || 1); + + 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) { @@ -83,7 +207,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() { diff --git a/scripts/test-suites.mjs b/scripts/test-suites.mjs index aae63d64d..ad13084d7 100644 --- a/scripts/test-suites.mjs +++ b/scripts/test-suites.mjs @@ -17,6 +17,8 @@ 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$/, /^\.github\/workflows\/ci\.yml$/, ]; @@ -74,6 +76,7 @@ export const SUITES = { 'tests/impeccable-paths.test.mjs', 'tests/openai-plugin.test.mjs', 'tests/pin.test.mjs', + 'tests/process-group.test.mjs', 'tests/release.test.mjs', 'tests/doctor.test.mjs', 'tests/staleness.test.mjs', @@ -170,6 +173,7 @@ export const SUITES = { 'tests/live-reference.test.mjs', 'tests/live-roots.test.mjs', 'tests/live-server.test.mjs', + 'tests/live-server-leak.test.mjs', 'tests/live-session-store.test.mjs', 'tests/live-source-lock.test.mjs', 'tests/live-source-search.test.mjs', diff --git a/tests/lib/live-servers.mjs b/tests/lib/live-servers.mjs new file mode 100644 index 000000000..8b911aefa --- /dev/null +++ b/tests/lib/live-servers.mjs @@ -0,0 +1,128 @@ +/** + * Test-side guard against leaked live servers. + * + * Any test file that starts a live server (directly, through + * `live-server --background`, or through a full `live` boot) imports this and + * calls `armLiveServerReaper()` once at module scope. That does three things: + * + * 1. stamps this process's environment with a unique marker, so every server + * it starts from then on is identifiable as belonging to this process; + * 2. installs exit and signal handlers that kill those servers on the ways + * out that JavaScript can still observe (assertion failure, thrown hook, + * Ctrl-C, `SIGTERM`); + * 3. spawns a detached reaper holding a pipe to this process, which covers + * the way out that JavaScript cannot observe: `SIGKILL`, a runner killed + * mid-test, a `node:test` timeout that never reaches the `after()` hook. + * + * Servers spawned as direct children are also tracked by handle so the common + * case is a cheap `child.kill()` rather than a process-table sweep. + */ + +import { spawn } from 'node:child_process'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + PROC_ID_ENV, + REPO_ENV, + REPO_PATH_ENV, + RUN_ID_ENV, + alive, + findLiveServers, + killLiveServers, + makeProcId, + makeRunId, + repoMarker, +} from '../../scripts/lib/live-server-processes.mjs'; + +const REPO_ROOT = path.resolve(fileURLToPath(new URL('../..', import.meta.url))); +const REAPER = path.join(REPO_ROOT, 'scripts', 'lib', 'test-orphan-reaper.mjs'); + +const trackedChildren = new Set(); +let procId = null; +let reaper = null; + +/** + * Idempotent. Safe to call from several modules in the same process. + * @returns {string} the process marker every server started after this inherits. + */ +export function armLiveServerReaper() { + if (procId) return procId; + + procId = makeProcId(); + process.env[PROC_ID_ENV] = procId; + // Standalone `node --test tests/live-server.test.mjs` gets the same coverage + // as a run through scripts/run-tests.mjs. + if (!process.env[RUN_ID_ENV]) process.env[RUN_ID_ENV] = makeRunId(REPO_ROOT); + if (!process.env[REPO_ENV]) process.env[REPO_ENV] = repoMarker(REPO_ROOT); + if (!process.env[REPO_PATH_ENV]) process.env[REPO_PATH_ENV] = REPO_ROOT; + + if (process.platform !== 'win32' && process.env.IMPECCABLE_NO_TEST_REAPER !== '1') { + try { + reaper = spawn(process.execPath, [REAPER, procId], { + detached: true, + stdio: ['pipe', 'ignore', 'ignore'], + }); + reaper.unref(); + reaper.on('error', () => { reaper = null; }); + } catch { + reaper = null; + } + } + + process.on('exit', () => cleanupSync()); + for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) { + process.on(sig, () => { + cleanupSync(); + // The shell convention for "killed by signal N": 130 SIGINT, 143 SIGTERM, + // 129 SIGHUP. + process.exit(128 + (os.constants.signals[sig] ?? 0)); + }); + } + // An uncaught exception still fires 'exit', so no separate handler is owed. + return procId; +} + +/** + * Register a live server started as a direct child, so it is killed by handle + * on the way out instead of waiting for a process-table sweep. + */ +export function trackServerChild(child) { + if (!child || typeof child.kill !== 'function') return child; + trackedChildren.add(child); + child.once('exit', () => trackedChildren.delete(child)); + return child; +} + +export function untrackServerChild(child) { + trackedChildren.delete(child); +} + +/** Live servers this process started that are still running. */ +export function findLeakedLiveServers() { + if (!procId) return []; + return findLiveServers({ procId }); +} + +/** + * Synchronous best-effort cleanup. Called from `process.on('exit')`, where the + * event loop is closed, so everything here has to be synchronous. + */ +export function cleanupSync() { + for (const child of trackedChildren) { + try { if (child.exitCode == null && child.signalCode == null) child.kill('SIGKILL'); } catch { /* gone */ } + } + trackedChildren.clear(); + + if (procId) { + try { + const leaked = findLiveServers({ procId }).filter(({ pid }) => alive(pid)); + if (leaked.length) killLiveServers(leaked, { graceMs: 250 }); + } catch { /* the sweep is a safety net, not a requirement */ } + } + + if (reaper) { + try { reaper.stdin?.end(); } catch { /* already closed */ } + reaper = null; + } +} diff --git a/tests/live-e2e/session.mjs b/tests/live-e2e/session.mjs index 04f87df08..38fe35779 100644 --- a/tests/live-e2e/session.mjs +++ b/tests/live-e2e/session.mjs @@ -22,12 +22,18 @@ import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; import { runAgentLoop } from './agent.mjs'; +import { armLiveServerReaper, trackServerChild } from '../lib/live-servers.mjs'; const __dirname = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = join(__dirname, '..', '..'); const SCRIPTS_DIR = join(REPO_ROOT, 'skill', 'scripts'); const FIXTURES_DIR = join(REPO_ROOT, 'tests', 'framework-fixtures'); +// Live servers here are detached daemons (`live-server --background`, or a full +// `live` boot), orphaned to pid 1 by design; teardown() is the only thing that +// stops them. The reaper covers the runs where teardown never happens. +armLiveServerReaper(); + export { SCRIPTS_DIR, FIXTURES_DIR, REPO_ROOT }; // --------------------------------------------------------------------------- @@ -198,14 +204,14 @@ export function runInject(tmp, port, token) { export function startDevServer(tmp, runtime) { const [cmd, ...args] = runtime.devCommand; - const child = spawn(cmd, args, { + const child = trackServerChild(spawn(cmd, args, { cwd: tmp, // runtime.env lets a fixture pin framework behavior. Astro 7 needs // ASTRO_DEV_BACKGROUND set: it auto-detects AI-agent environments and // daemonizes `astro dev`, which the harness reads as a crashed server. env: { ...process.env, FORCE_COLOR: '0', NO_COLOR: '1', ...(runtime.env || {}) }, stdio: ['ignore', 'pipe', 'pipe'], - }); + })); const readyRe = new RegExp(runtime.readyPattern); const bufLog = []; diff --git a/tests/live-poll-stream.test.mjs b/tests/live-poll-stream.test.mjs index 55c802e97..c45cc9993 100644 --- a/tests/live-poll-stream.test.mjs +++ b/tests/live-poll-stream.test.mjs @@ -11,6 +11,9 @@ import { spawn } from 'node:child_process'; import { tmpdir } from 'node:os'; import { getLiveServerPath } from '../skill/scripts/lib/impeccable-paths.mjs'; import { postReply } from '../skill/scripts/live-poll.mjs'; +import { armLiveServerReaper, trackServerChild } from './lib/live-servers.mjs'; + +armLiveServerReaper(); const REPO_ROOT = process.cwd(); const SERVER_SCRIPT = join(REPO_ROOT, 'skill/scripts/live-server.mjs'); @@ -18,11 +21,11 @@ const POLL_SCRIPT = join(REPO_ROOT, 'skill/scripts/live-poll.mjs'); function startServer(port = 8498, { cwd = REPO_ROOT } = {}) { return new Promise((resolve, reject) => { - const proc = spawn('node', [SERVER_SCRIPT, '--port=' + port], { + const proc = trackServerChild(spawn('node', [SERVER_SCRIPT, '--port=' + port], { cwd, stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env }, - }); + })); let output = ''; proc.stdout.on('data', (d) => { output += d.toString(); diff --git a/tests/live-server-leak.test.mjs b/tests/live-server-leak.test.mjs new file mode 100644 index 000000000..b709315cf --- /dev/null +++ b/tests/live-server-leak.test.mjs @@ -0,0 +1,235 @@ +/** + * The leak guard itself. + * + * A live server must not outlive the test process that started it, even when + * that process is SIGKILLed and no `after()` hook, no `finally`, and no + * `process.on('exit')` handler ever runs. That is the shape that left 197 + * orphaned servers squatting the live suite's ports (issue: live-server leak). + * + * Run with: node --test tests/live-server-leak.test.mjs + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fileURLToPath } from 'node:url'; +import { + PROC_ID_ENV, + REPO_ENV, + RUN_ID_ENV, + alive, + envLineHasEntry, + findLiveServers, + killLiveServers, + makeProcId, + makeRunId, + repoMarker, +} from '../scripts/lib/live-server-processes.mjs'; + +// The reaper is a POSIX mechanism (a detached process holding a pipe, killed by +// signal). armLiveServerReaper() does not arm it on Windows, so the guarantee it +// pins is not one Windows makes yet. +const WINDOWS = process.platform === 'win32'; + +const REPO_ROOT = fileURLToPath(new URL('..', import.meta.url)); +const PORT = 8591; + +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +async function waitUntil(predicate, { timeoutMs = 10_000, stepMs = 100 } = {}) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (await predicate()) return true; + await sleep(stepMs); + } + return false; +} + +/** + * A stand-in for a test file: arms the reaper, starts a detached live server + * exactly as `live-server --background` does, prints its pid, then blocks + * forever so the caller can decide how it dies. + */ +const VICTIM = ` +import { spawn } from 'node:child_process'; +import { armLiveServerReaper } from ${JSON.stringify(join(REPO_ROOT, 'tests/lib/live-servers.mjs'))}; + +armLiveServerReaper(); + +const child = spawn(process.execPath, [ + ${JSON.stringify(join(REPO_ROOT, 'skill/scripts/live-server.mjs'))}, + '--port=${PORT}', +], { detached: true, stdio: 'ignore', cwd: process.cwd() }); +child.unref(); + +// Wait until it is actually listening before reporting, so the assertion below +// is about a running server rather than a pid that never came up. +const deadline = Date.now() + 15000; +while (Date.now() < deadline) { + try { + const res = await fetch('http://127.0.0.1:${PORT}/health'); + if (res.ok || res.status === 401 || res.status === 404) break; + } catch {} + await new Promise((r) => setTimeout(r, 50)); +} +console.log(JSON.stringify({ serverPid: child.pid })); +setInterval(() => {}, 1000); +`; + +function startVictim(cwd) { + const script = join(cwd, 'victim.mjs'); + writeFileSync(script, VICTIM); + const proc = spawn(process.execPath, [script], { + cwd, + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...process.env, [RUN_ID_ENV]: makeRunId(REPO_ROOT), [PROC_ID_ENV]: '' }, + }); + const ready = new Promise((resolve, reject) => { + let out = ''; + let err = ''; + const timer = setTimeout( + () => reject(new Error(`victim never reported a server\nstdout: ${out}\nstderr: ${err}`)), + 25_000, + ); + timer.unref(); + proc.stdout.on('data', (d) => { + out += d.toString(); + const line = out.split('\n').find((l) => l.trim().startsWith('{')); + if (line) { + clearTimeout(timer); + resolve(JSON.parse(line)); + } + }); + proc.stderr.on('data', (d) => { err += d.toString(); }); + proc.on('exit', (code) => { + clearTimeout(timer); + reject(new Error(`victim exited early (code=${code})\n${err}`)); + }); + }); + return { proc, ready }; +} + +describe('live server leak guard', () => { + it('kills the live server when the test process is SIGKILLed', { + skip: WINDOWS ? 'the reaper is POSIX-only; armLiveServerReaper() does not arm it on win32' : false, + }, async () => { + const cwd = mkdtempSync(join(tmpdir(), 'impeccable-leak-')); + let victim; + let serverPid; + try { + victim = startVictim(cwd); + ({ serverPid } = await victim.ready); + assert.ok(alive(serverPid), 'server should be running before the kill'); + + // The case no in-process cleanup can cover. + victim.proc.kill('SIGKILL'); + + const reaped = await waitUntil(() => !alive(serverPid), { timeoutMs: 15_000 }); + assert.equal(reaped, true, `live server pid ${serverPid} outlived the SIGKILLed test process`); + } finally { + if (victim?.proc && victim.proc.exitCode == null) victim.proc.kill('SIGKILL'); + if (serverPid && alive(serverPid)) killLiveServers([{ pid: serverPid, command: '' }]); + rmSync(cwd, { recursive: true, force: true }); + } + }); + + it('scopes a sweep to the marker, never to the name alone', () => { + // No marker, no match: a sweep can never take out a live server that some + // other checkout, or the user's own session, is running. + assert.deepEqual(findLiveServers({}), []); + assert.deepEqual(findLiveServers({ runId: 'no-such-run-id-' + Date.now() }), []); + }); +}); + +describe('envLineHasEntry', () => { + // Marker values are opaque tokens, never paths: repoMarker() hashes the + // checkout so two adjacent checkouts get unrelated values, and the run and + // process ids are random hex. Nothing a marker can hold contains whitespace, + // which is the invariant this matcher rests on. + const hash = repoMarker('/work/impeccable'); + const marker = `${REPO_ENV}=${hash}`; + + it('matches the entry at the end of the line and between other entries', () => { + assert.equal(envLineHasEntry(`node live-server.mjs PATH=/usr/bin ${marker}`, marker), true); + assert.equal(envLineHasEntry(`node live-server.mjs ${marker} PATH=/usr/bin`, marker), true); + assert.equal(envLineHasEntry(`node x ${marker} ${RUN_ID_ENV}=abc PATH=/usr/bin`, marker), true); + }); + + it('does not match a value the marker is a strict prefix of', () => { + // The shape the hash rules out at the source, asserted anyway: a longer + // value starting with this one must not count as this entry. + assert.equal(envLineHasEntry(`node x ${marker}ff PATH=/usr/bin`, marker), false); + assert.equal(envLineHasEntry(`node x ${marker}-2`, marker), false); + assert.equal(envLineHasEntry(`node x ${REPO_ENV}=${hash}0123456789abcdef`, marker), false); + }); + + it('gives adjacent checkouts unrelated markers', () => { + // The bug this all exists for: /work/impeccable is a substring of + // /work/impeccable-copy. Hashing means the two markers no longer share a + // prefix at all, so a substring can never arise in the first place. + const neighbour = repoMarker('/work/impeccable-copy'); + assert.notEqual(neighbour, hash); + assert.equal(neighbour.startsWith(hash), false); + assert.equal(envLineHasEntry(`node x ${REPO_ENV}=${neighbour} PATH=/usr/bin`, marker), false); + }); + + it('resolves one checkout to one marker through trailing slashes and dot segments', () => { + const real = mkdtempSync(join(tmpdir(), 'impeccable-marker-')); + try { + const expected = repoMarker(real); + for (const spelling of [`${real}/`, `${real}/.`, join(real, 'sub', '..')]) { + assert.equal(repoMarker(spelling), expected, `${spelling} should hash like ${real}`); + } + } finally { + rmSync(real, { recursive: true, force: true }); + } + }); + + it('resolves a symlinked checkout to the same marker as its target', () => { + const real = mkdtempSync(join(tmpdir(), 'impeccable-marker-')); + const link = join(mkdtempSync(join(tmpdir(), 'impeccable-link-')), 'checkout'); + try { + // Windows refuses a directory symlink without Developer Mode; a junction + // is the equivalent it does allow. Same call shape as concept-seed's. + symlinkSync(real, link, process.platform === 'win32' ? 'junction' : 'dir'); + assert.equal(repoMarker(link), repoMarker(real)); + assert.equal(repoMarker(`${link}/`), repoMarker(real)); + } finally { + rmSync(link, { force: true, recursive: true }); + rmSync(real, { recursive: true, force: true }); + } + }); + + it('does not match when the entry name only ends with the marker name', () => { + // The marker is a substring here, but it does not start an entry. + assert.equal(envLineHasEntry(`node x MY_${marker} PATH=/usr/bin`, marker), false); + assert.equal(envLineHasEntry(`node x X${marker}`, marker), false); + }); + + it('returns false when the marker is absent', () => { + assert.equal(envLineHasEntry('node live-server.mjs PATH=/usr/bin', marker), false); + assert.equal(envLineHasEntry('', marker), false); + }); +}); + +describe('marker values', () => { + it('generates run and process ids from a whitespace-free alphabet', () => { + for (const value of [makeRunId('/work/impeccable'), makeProcId(), repoMarker('/work/impeccable')]) { + assert.match(value, /^[A-Za-z0-9_-]+$/, `marker value "${value}" must not need quoting`); + } + }); + + it('refuses to match on a value that could break out of its entry', () => { + // The guard that keeps the matcher's invariant honest if someone later + // passes a path where a token is expected. + assert.throws( + () => findLiveServers({ runId: '/work/my repo PATH=x' }), + /marker value must be/, + ); + }); +}); diff --git a/tests/live-server.test.mjs b/tests/live-server.test.mjs index be0a0c17d..db40a6088 100644 --- a/tests/live-server.test.mjs +++ b/tests/live-server.test.mjs @@ -19,6 +19,11 @@ import { removeAllSvelteComponentSessions, sweepInactiveSvelteComponentSessions, } from '../skill/scripts/live/svelte-component.mjs'; +import { armLiveServerReaper, trackServerChild } from './lib/live-servers.mjs'; + +// Every server this file starts is killed even if the process is SIGKILLed or +// a test times out before its after() hook runs. See tests/lib/live-servers.mjs. +armLiveServerReaper(); const REPO_ROOT = process.cwd(); const SERVER_SCRIPT = join(REPO_ROOT, 'skill/scripts/live-server.mjs'); @@ -29,11 +34,11 @@ const COMPLETE_SCRIPT = join(REPO_ROOT, 'skill/scripts/live-complete.mjs'); function startServer(port = 8499, { cwd = REPO_ROOT, env = {} } = {}) { return new Promise((resolve, reject) => { - const proc = spawn('node', [SERVER_SCRIPT, '--port=' + port], { + const proc = trackServerChild(spawn('node', [SERVER_SCRIPT, '--port=' + port], { cwd, stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, IMPECCABLE_LIVE_COPY_AGENT: 'off', ...env }, - }); + })); let output = ''; proc.stdout.on('data', (d) => { output += d.toString(); diff --git a/tests/live-target-context.test.mjs b/tests/live-target-context.test.mjs index 9cd3156f5..1ab6f84c7 100644 --- a/tests/live-target-context.test.mjs +++ b/tests/live-target-context.test.mjs @@ -5,6 +5,11 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, import { join, dirname } from 'node:path'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; +import { armLiveServerReaper } from './lib/live-servers.mjs'; + +// These tests boot detached live servers through live.mjs; the reaper kills any +// the `stop` calls miss when the process dies before them. +armLiveServerReaper(); const __dirname = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = join(__dirname, '..'); diff --git a/tests/process-group.test.mjs b/tests/process-group.test.mjs new file mode 100644 index 000000000..09b1d16d3 --- /dev/null +++ b/tests/process-group.test.mjs @@ -0,0 +1,200 @@ +/** + * The runner's group shutdown. + * + * Two properties, and the first is the one that regressed: ending a group must + * return as soon as the child is actually gone. A wait implemented as a poll on + * `kill(pid, 0)` cannot do that, because a dead child is a zombie until this + * process reaps it and a blocked event loop never reaps anything. Such a wait + * always burns its whole grace period and always ends in a needless SIGKILL. + * + * Run with: node --test tests/process-group.test.mjs + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { + createGroupShutdown, + killGroupSync, + stopGroup, + trackChildExit, +} from '../scripts/lib/process-group.mjs'; + +const POSIX = process.platform !== 'win32'; + +/** A child in its own process group that exits on SIGTERM, the ordinary case. */ +function spawnObedient() { + return spawnDetached('setInterval(() => {}, 1000);'); +} + +/** A child that traps SIGTERM and keeps running, so only SIGKILL ends it. */ +function spawnStubborn() { + return spawnDetached("process.on('SIGTERM', () => {}); setInterval(() => {}, 1000);"); +} + +function spawnDetached(source) { + const child = spawn(process.execPath, ['-e', source], { + detached: true, + stdio: 'ignore', + }); + return trackChildExit(child); +} + +function isAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return err.code === 'EPERM'; + } +} + +describe('stopGroup', () => { + it('returns as soon as the child exits, not after the grace period', { + skip: POSIX ? false : 'process groups and SIGTERM are POSIX-only', + }, async () => { + const running = spawnObedient(); + const startedAt = Date.now(); + const outcome = await stopGroup(running, { graceMs: 5_000 }); + const elapsed = Date.now() - startedAt; + + assert.equal(outcome, 'exited'); + // The regression this pins: a poll on liveness could not see the reaped + // child and would have taken the full 5s. + assert.ok(elapsed < 1_000, `expected a prompt return, took ${elapsed}ms`); + assert.equal(running.hasExited, true); + }); + + it('escalates to SIGKILL when the child ignores SIGTERM', { + skip: POSIX ? false : 'process groups and SIGTERM are POSIX-only', + }, async () => { + const running = spawnStubborn(); + const { pid } = running.child; + // Give the trap a moment to be installed, so the SIGTERM lands on a child + // that really is ignoring it. + await new Promise((r) => setTimeout(r, 250)); + + const startedAt = Date.now(); + const outcome = await stopGroup(running, { graceMs: 400, killGraceMs: 2_000 }); + const elapsed = Date.now() - startedAt; + + assert.equal(outcome, 'killed'); + assert.ok(elapsed >= 400, `should have waited out the grace period, took ${elapsed}ms`); + assert.equal(running.hasExited, true); + assert.equal(isAlive(pid), false); + }); + + it('is a no-op for a child that has already exited', async () => { + const running = spawnObedient(); + await stopGroup(running, { graceMs: 5_000 }); + assert.equal(await stopGroup(running, { graceMs: 5_000 }), 'already-gone'); + }); +}); + +describe('killGroupSync', () => { + it('ends a stubborn child without awaiting anything', { + skip: POSIX ? false : 'process groups and SIGTERM are POSIX-only', + }, async () => { + const running = spawnStubborn(); + const { pid } = running.child; + await new Promise((r) => setTimeout(r, 250)); + + assert.equal(killGroupSync(running), true); + // It cannot wait, so the death is observed here rather than there. + await running.exited; + assert.equal(isAlive(pid), false); + }); + + it('reports nothing to do when the child has already exited', async () => { + const running = spawnObedient(); + await stopGroup(running, { graceMs: 5_000 }); + assert.equal(killGroupSync(running), false); + }); +}); + +describe('createGroupShutdown', () => { + const posixOnly = { skip: POSIX ? false : 'process groups and SIGTERM are POSIX-only' }; + + function spy() { + const codes = []; + return { codes, exit: (code) => codes.push(code) }; + } + + it('a second signal kills the group instead of abandoning the escalation', posixOnly, async () => { + // The regression: the first handler used to clear the only reference to + // the group before awaiting, so the second signal killed nothing and its + // exit walked away from an escalation that was still in flight. Because + // the suite is spawned detached, it then outlived the runner, which is the + // exact leak this whole change exists to close. + const running = spawnStubborn(); + const { pid } = running.child; + await new Promise((r) => setTimeout(r, 250)); + + const { codes, exit } = spy(); + const shutdown = createGroupShutdown({ exit, graceMs: 30_000 }); + shutdown.track(running); + + const first = shutdown.onSignal(130); + await new Promise((r) => setTimeout(r, 150)); + + const startedAt = Date.now(); + await shutdown.onSignal(130); + await running.exited; + const elapsed = Date.now() - startedAt; + + assert.equal(isAlive(pid), false, 'the second signal must end the group'); + assert.ok(elapsed < 2_000, `should not wait out the 30s grace, took ${elapsed}ms`); + assert.equal(codes[0], 130); + await first; + }); + + it('ends the group on the first signal when it exits promptly', posixOnly, async () => { + const running = spawnObedient(); + const { pid } = running.child; + const { codes, exit } = spy(); + const shutdown = createGroupShutdown({ exit, graceMs: 5_000 }); + shutdown.track(running); + + const startedAt = Date.now(); + await shutdown.onSignal(143); + assert.ok(Date.now() - startedAt < 1_000); + assert.deepEqual(codes, [143]); + assert.equal(isAlive(pid), false); + }); + + it('onExit still reaches a group a shutdown is in the middle of stopping', posixOnly, async () => { + const running = spawnStubborn(); + const { pid } = running.child; + await new Promise((r) => setTimeout(r, 250)); + + const { exit } = spy(); + const shutdown = createGroupShutdown({ exit, graceMs: 30_000 }); + shutdown.track(running); + const inFlight = shutdown.onSignal(130); + await new Promise((r) => setTimeout(r, 150)); + + // `current` is null by now; the handle lives in `stopping`. + assert.equal(shutdown.onExit(), true); + await running.exited; + assert.equal(isAlive(pid), false); + await inFlight; + }); + + it('exits cleanly when no group is running', async () => { + const { codes, exit } = spy(); + const shutdown = createGroupShutdown({ exit }); + await shutdown.onSignal(129); + assert.deepEqual(codes, [129]); + assert.equal(shutdown.onExit(), false); + }); + + it('reports that it is shutting down, so a normal exit is not misread', posixOnly, async () => { + const running = spawnObedient(); + const shutdown = createGroupShutdown({ exit: () => {}, graceMs: 5_000 }); + shutdown.track(running); + assert.equal(shutdown.shuttingDown, false); + const done = shutdown.onSignal(130); + assert.equal(shutdown.shuttingDown, true); + await done; + }); +});