From 4c5243fcd42d39c1fc281adcaf10be0913095f74 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 3 Sep 2026 22:21:30 -0400 Subject: [PATCH] Tests: stop the harness leaking live-server processes (#718) * Tests: stop the harness leaking live-server processes Nothing owned a live server past the exit paths JavaScript can observe. The live unit tests spawn the server as a direct child and stop it with an HTTP /stop plus proc.kill() inside an after() hook; the e2e session and the target-context tests boot it through `live-server --background` / live.mjs, which spawns a detached, unref'd daemon that only the `stop` verb ever ends. A POSIX child does not die with its parent, and a detached daemon is orphaned to pid 1 from birth, so any exit that skipped teardown (a node:test timeout, a SIGKILL of the runner, a Ctrl-C, an assertion that threw before the hook) left the server listening on a fixed live-suite port for good. scripts/run-tests.mjs did not compensate: it used blocking spawnSync, so no signal handler could run; it left suite commands in its own process group with nothing that could kill that group; and it never checked afterwards whether anything survived. Days of local runs accumulated 197 orphans on one machine, the oldest four days old, until `bun run test:live` could not claim its ports. The fix is structural rather than a cleanup sweep bolted on the end, and it is deliberately implementation-agnostic so it holds for the Node scripts here and for the Rust `impeccable live-server` on rust-swap: - tests/lib/live-servers.mjs. armLiveServerReaper(), called once at module scope by every test file that starts a server, stamps the process env with a unique marker, installs exit and signal handlers, and spawns a detached reaper holding a pipe to the process. SIGKILL the process and the pipe closes, the reaper wakes on EOF and kills the servers carrying that marker. That is the one case no in-process cleanup can reach. trackServerChild() also registers direct children (live servers and fixture dev servers) so the ordinary exits are a cheap kill by handle. - scripts/lib/live-server-processes.mjs. The scan and kill primitives, shared by the reaper and the runner. Processes are matched by the environment marker the harness exported, never by name or port, so a sweep can only ever reach a server this repo's tests started. - scripts/run-tests.mjs. Each suite command now runs as its own process-group leader with SIGINT/SIGTERM/SIGHUP forwarded to the group, and after every suite the runner checks for live servers carrying that suite's run id. A survivor is killed and fails the run, so the next leak surfaces in the run that caused it instead of on a laptop days later. IMPECCABLE_SKIP_LEAK_CHECK=1 bypasses it. `bun run test:cleanup` sweeps leftovers from earlier runs. - tests/live-server-leak.test.mjs pins the guarantee: it boots a real server under a process it then SIGKILLs, and fails if the server outlives it. With IMPECCABLE_NO_TEST_REAPER=1 the test fails, which is what makes it a regression test rather than a tautology. Verified: bun run test:live green with zero survivors; scoped live-e2e (vite8-react-plain) matches pristine main test for test; the SIGKILL repro goes from 2 orphans to 0; SIGINT and SIGKILL of the runner itself both leave nothing behind; bun run build green. Fixes #717 AI assistance: prepared by Claude Code under pbakaus's direction. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY * Review fixes: scope the sweep to whole env entries only Five review findings on #718, all in the matching layer that decides which processes a sweep may touch. The repository-path fallback is gone (Greptile P1). `bun run test:cleanup` passed REPO_ROOT to findLiveServers, which then also matched any live-server command line under the checkout, marker or not. A developer running `impeccable live` in this repo has exactly that command line, so the cleanup could have killed their own session. The PR promised matching on the exported environment marker and nothing else; now it does. The cost is that a server from a run predating the marker is no longer found and has to be killed by hand, which is the right trade. Environment entries are compared whole on macOS and BSD (Greptile P1). `ps -E` flattens the environment into the command column, and that line was searched with a plain substring test, so IMPECCABLE_TEST_REPO=/work/impeccable also matched /work/impeccable-copy and one checkout's cleanup could reach a neighbouring checkout's servers. envLineHasEntry() now requires the marker to start an entry (line start or whitespace) and to end one (line end, or whitespace followed by the next KEY=), which is the same whole-entry comparison the Linux /proc branch already did. Six unit tests cover it, including the adjacent-path negative case, and a live probe against real `ps -E` output confirms an exact repo matches while /work/impeccable-copy and a run-id prefix do not. The SIGKILL regression test now skips on win32 with a stated reason (Copilot). The reaper is a POSIX mechanism and armLiveServerReaper() does not arm it there, so the test asserted a guarantee Windows does not make yet. Signal exits use the shell convention 128 + signum in both the runner and the test helper (Copilot, two threads). SIGHUP returned 143; it is 129. Read from os.constants.signals rather than a hand-written table. Verified: leak test 7/7 (2 guard, 5 matcher); bun run test:live 895 tests, 0 fail, 0 survivors; scoped live-e2e (vite8-react-plain) 3 pass / 1 fail, matching pristine main; SIGKILL repro 3 servers up, 0 after; bun run build green. AI assistance: prepared by Claude Code under pbakaus's direction. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY * Review fix: make marker values opaque so the matcher has no ambiguous case Greptile's follow-up P1 on the parser was right, and the parser was the wrong place to answer it. envLineHasEntry ended an entry at "whitespace followed by the next KEY=", so a checkout path that extended another one with whitespace plus a KEY=-shaped token still defeated it, which is exactly the ambiguity the docblock admitted to. A format that cannot be parsed unambiguously should not be handed ambiguous input. So the fix is at the source: no marker value is a path any more. IMPECCABLE_TEST_REPO now carries repoMarker(), the first 16 hex characters of the sha256 of the checkout's real path, and the runner and the cleanup command both compute it the same way from REPO_ROOT. Two checkouts whose paths share a prefix get unrelated hashes, so a substring cannot arise in the first place, and every spelling of one checkout (trailing slash, `.` segment, symlink, /private prefix) resolves to one marker. The run id is now repoMarker plus 8 random bytes of hex, and the process id p plus the same, both from a whitespace-free alphabet. With every value fixed-alphabet, envLineHasEntry needs only "starts an entry and ends at whitespace or line end". The KEY= lookahead is gone and so is the documented unresolvable case. assertMarkerValue keeps the invariant honest: it refuses any value outside [A-Za-z0-9_-] with a message that says to hash it, so a future caller that passes a path gets a loud error instead of a silent mismatch. The readable path is still available for a human reading `ps -E` output, exported separately as IMPECCABLE_TEST_REPO_PATH, which nothing matches on and the docblock says so. Matcher tests: the space-in-value case is gone, since that value can no longer exist. Added a strict-prefix case (a longer hash-shaped value starting with the marker), an adjacent-checkout case asserting the two hashes do not even share a prefix, a symlink/trailing-slash case against real directories, an alphabet check on all three generators, and one asserting assertMarkerValue throws. Verified: leak test 10/10; bun run test:live 898 tests, 0 fail, 0 survivors; scoped live-e2e (vite8-react-plain) 3 pass / 1 fail, matching pristine main; SIGKILL repro 1 server up, 0 after; bun run build green. A probe against real `ps -E` output with a hashed marker: this checkout 1 match, its trailing-slash spelling 1, an adjacent checkout 0, exact run id 1, a run-id prefix 0. AI assistance: prepared by Claude Code under pbakaus's direction. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY * Review fixes: async group shutdown, and a Windows-safe symlink test Two Cursor Bugbot findings, both real. killCurrentGroup busy-waited on alive(child.pid) after sending SIGTERM, which could never work. A dead child stays a zombie until its parent reaps it, the parent here is the runner, and the runner reaps through libuv when the event loop runs. The spin blocked the very loop that would have done the reaping and then read the unreaped zombie as alive, so every SIGINT, SIGTERM and SIGHUP burned the full 2s grace and ended in a needless SIGKILL. There is no waitpid from JavaScript that sees through this, so the wait is now asynchronous and keyed on the child's own exit event. The logic moved to scripts/lib/process-group.mjs: trackChildExit exposes the exit as a flag and a promise, stopGroup races that promise against the grace period and escalates to SIGKILL only if it loses, and killGroupSync stays synchronous for process.on('exit'), where nothing can be awaited, so it sends SIGTERM then SIGKILL without pretending to wait. A second Ctrl-C now skips the grace period entirely rather than queueing behind it. Measured on a real SIGINT to a running live suite: 2027ms before, 34ms after. tests/process-group.test.mjs pins both halves, including the escalation path against a child that traps SIGTERM, which is not otherwise reachable from a registered suite. The repoMarker symlink test called symlinkSync with no type, which throws EPERM on Windows without Developer Mode. It now passes 'junction' there and 'dir' elsewhere, the same shape tests/concept-seed.test.mjs uses, and the trailing-slash and dot-segment cases split into their own test so they keep running on every platform regardless. Merged origin/main (through #716) to re-level the branch. Verified: leak and process-group tests 16/16; bun run test:live 900 tests, 0 fail, 0 survivors; scoped live-e2e (vite8-react-plain) now 4/4, with the orphaned-session test that #716 fixed passing in 7.2s; bun run build green. AI assistance: prepared by Claude Code under pbakaus's direction. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY * Review fix: a second Ctrl-C must reach the group the first one is stopping Cursor Bugbot caught a bug I introduced with the async shutdown, and it is the same class of leak this PR exists to close. The signal handler cleared currentChild before awaiting stopGroup, so a second Ctrl-C read a null handle: killGroupSync did nothing, process.exit walked away from the SIGKILL escalation still in flight, and because the suite is spawned detached it kept running after the runner was gone. Impatience with a stuck suite produced exactly the orphan the change is supposed to prevent. The shutdown state machine moved into scripts/lib/process-group.mjs as createGroupShutdown, which holds the group in `stopping` for as long as it is being ended rather than dropping the only reference to it. A second signal kills that handle and leaves; process.on('exit') looks at `current` or `stopping`, so the last-resort path reaches a group mid-shutdown too. The runner keeps no shutdown state of its own now, which is what made the bug possible to write in the first place. The extraction is what makes it testable: `exit` is injectable, so tests/process-group.test.mjs can drive two signals at a stubborn child that traps SIGTERM and assert the group dies in under 2s against a 30s grace. Point that test at the old logic (killGroupSync on the cleared reference) and it hangs out the full grace and fails, which is the check that it pins something real. Five cases in all, including the exit-handler path and the no-child case. Verified: process-group 10/10, live-server-leak 11/11; real double SIGINT to a running live suite exits in 24ms with zero group members and zero servers left; bun run test:live 900 tests, 0 fail, 0 survivors; scoped live-e2e (vite8-react-plain) 4/4; bun run build green. The core suite wedged twice locally in tests/build-phase.test.mjs, the pre-existing unbounded-spawnSync hang noted in the PR description that rust-swap's 47f18713 fixes. Unrelated to this change: CI is green on both Node versions, and process-group.test.mjs passes inside that batch. AI assistance: prepared by Claude Code under pbakaus's direction. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY --------- Co-authored-by: Claude Fable 5.1 --- CLAUDE.md | 13 ++ package.json | 1 + scripts/lib/live-server-processes.mjs | 258 ++++++++++++++++++++++++++ scripts/lib/process-group.mjs | 146 +++++++++++++++ scripts/lib/test-orphan-reaper.mjs | 55 ++++++ scripts/run-tests.mjs | 173 ++++++++++++++--- scripts/test-suites.mjs | 4 + tests/lib/live-servers.mjs | 128 +++++++++++++ tests/live-e2e/session.mjs | 10 +- tests/live-poll-stream.test.mjs | 7 +- tests/live-server-leak.test.mjs | 235 +++++++++++++++++++++++ tests/live-server.test.mjs | 9 +- tests/live-target-context.test.mjs | 5 + tests/process-group.test.mjs | 200 ++++++++++++++++++++ 14 files changed, 1214 insertions(+), 30 deletions(-) create mode 100644 scripts/lib/live-server-processes.mjs create mode 100644 scripts/lib/process-group.mjs create mode 100644 scripts/lib/test-orphan-reaper.mjs create mode 100644 tests/lib/live-servers.mjs create mode 100644 tests/live-server-leak.test.mjs create mode 100644 tests/process-group.test.mjs 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; + }); +});