skills/pbakaus_impeccable
1
0
Fork 0
mirror of https://github.com/pbakaus/impeccable.git synced 2026-09-11 13:46:32 +03:00
Code Issues Packages Projects Releases Wiki Activity

Tests: stop the harness leaking live-server processes (#718)

Browse Source 
* 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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<pid> 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Paul Bakaus
2026-09-03 19:21:30 -07:00
committed by GitHub
co-authored by Claude Fable 5.1
parent fbc5c95355
commit 4c5243fcd4
14 changed files with 1214 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CLAUDE.md
+13
View File
@@ -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.
package.json
+1
View File
@@ -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",
scripts/lib/live-server-processes.mjs
+258
View File
@@ -0,0 +1,258 @@
/**
* Finding and killing live servers a test run left behind.
*
* The harness starts live servers in two shapes and neither one dies with the
* process that started it:
*
* 1. a direct child (`node skill/scripts/live-server.mjs --port=N`, or
* `$IMPECCABLE_BIN live-server --port=N` once the engine is Rust), stopped
* by an HTTP `/stop` call in an `after()` hook;
* 2. a detached daemon (`live-server --background`, or a full `live` boot),
* which is orphaned to pid 1 by design and stopped by the `stop` verb.
*
* Both survive a runner that dies before teardown: a `SIGKILL`, a Ctrl-C, a
* `node:test` timeout that skips the `after()` hook. This module is the safety
* net. It identifies servers by an environment marker the runner exports, so a
* sweep can only ever match a server this repo's test harness started; nothing
* is matched by port, by name alone, or by "looks like impeccable".
*
* Every marker value is an opaque token: a random run or process id, or a hash
* of the checkout path. None of them is a path, and none can contain
* whitespace, which is what keeps the `ps -E` matching below exact.
*/
import { spawnSync } from 'node:child_process';
import { createHash, randomBytes } from 'node:crypto';
import { readFileSync, readdirSync, realpathSync } from 'node:fs';
import path from 'node:path';
/**
* The three matching markers. Every value is an opaque token from the alphabet
* below, never a path and never anything a user chose. That is what lets the
* `ps -E` matcher work on a "starts an entry, ends at whitespace or line end"
* rule with no ambiguous case left, and it is a load-bearing invariant rather
* than a formatting preference: a value free to contain a space and a
* `KEY=`-shaped token could impersonate the end of its own entry, and one
* checkout's cleanup could then reach a neighbouring checkout's servers.
*/
const MARKER_VALUE_RE = /^[A-Za-z0-9_-]+$/;
/** Env var carrying the id of one suite command. Descendants inherit it. */
export const RUN_ID_ENV = 'IMPECCABLE_TEST_RUN_ID';
/**
* Env var carrying the id of one test process. `node --test` runs files
* concurrently and they all inherit the same run id, so a per-process id is
* what lets one file's reaper kill that file's servers and not its siblings'.
*/
export const PROC_ID_ENV = 'IMPECCABLE_TEST_PROC_ID';
/**
* Env var carrying a hash of the checkout, so a cleanup can be scoped to it.
* The value is `repoMarker()`, not the path: two checkouts whose paths share a
* prefix get unrelated hashes, and no filesystem path can leak into matching.
*/
export const REPO_ENV = 'IMPECCABLE_TEST_REPO';
/**
* The checkout path in readable form, for a human looking at `ps -E` output or
* a stuck process. **Never used for matching**, and nothing should start: it is
* the one marker-adjacent value that can contain whitespace.
*/
export const REPO_PATH_ENV = 'IMPECCABLE_TEST_REPO_PATH';
/**
* A command line belonging to a live server. Matches the Node script
* (`.../live-server.mjs`) and the engine verb (`.../impeccable live-server`).
*/
const LIVE_SERVER_RE = /(^|[\s/\\])live-server(\.mjs|\.exe)?(\s|$)/;
/**
* A stable, opaque id for one checkout: the first 16 hex characters of the
* sha256 of its real path. Symlinked and `/private`-prefixed spellings of the
* same directory resolve to the same marker; `/work/impeccable` and
* `/work/impeccable-copy` do not share a prefix.
*/
export function repoMarker(repoRoot) {
// path.resolve first so a trailing slash or a `.` segment cannot change the
// marker for a directory that is not on disk (realpathSync throws for those).
let resolved = path.resolve(repoRoot);
try { resolved = realpathSync(resolved); } catch { /* not on disk; hash as normalized */ }
return createHash('sha256').update(resolved).digest('hex').slice(0, 16);
}
/**
* An id for one suite command. Random, so two runs of the same suite in the
* same second never collide, and hex/`-` only so it can never break out of its
* own environment entry.
*/
export function makeRunId(repoRoot = process.cwd()) {
return `${repoMarker(repoRoot)}-${randomBytes(8).toString('hex')}`;
}
/** An id for one test process. Same alphabet rule as the run id. */
export function makeProcId() {
return `p${process.pid}-${randomBytes(8).toString('hex')}`;
}
/**
* Live servers still running that carry one of the given markers.
*
* @param {object} opts
* Every marker is an environment entry the harness itself exported. There is
* deliberately no fallback that matches a command line under the checkout: a
* developer running `impeccable live` in this repo has exactly that command
* line, and a cleanup must never be able to kill their session.
*
* @param {string} [opts.runId] match `IMPECCABLE_TEST_RUN_ID=<runId>` exactly.
* @param {string} [opts.procId] match `IMPECCABLE_TEST_PROC_ID=<procId>` exactly.
* @param {string} [opts.repo] a checkout path; matched as its `repoMarker()`
* hash, never as the path itself.
* @returns {{pid:number, command:string}[]}
*/
export function findLiveServers({ runId, procId, repo } = {}) {
const markers = [];
if (runId) markers.push(`${RUN_ID_ENV}=${assertMarkerValue(runId)}`);
if (procId) markers.push(`${PROC_ID_ENV}=${assertMarkerValue(procId)}`);
if (repo) markers.push(`${REPO_ENV}=${assertMarkerValue(repoMarker(repo))}`);
if (!markers.length) return [];
const commands = listCommands();
if (!commands.size) return [];
const matched = new Set(pidsWithEnvMarker(markers, commands));
const out = [];
for (const pid of matched) {
if (pid === process.pid) continue;
const command = commands.get(pid);
if (!command || !LIVE_SERVER_RE.test(command)) continue;
out.push({ pid, command });
}
return out.sort((a, b) => a.pid - b.pid);
}
/**
* SIGTERM every process, then SIGKILL whatever is still alive after `graceMs`.
* Kills the process group too when the process leads one, so a server that
* spawned helpers does not leave them behind.
*
* @returns {number} how many processes were signalled.
*/
export function killLiveServers(procs, { graceMs = 400 } = {}) {
if (!procs.length) return 0;
for (const { pid } of procs) signal(pid, 'SIGTERM');
const deadline = Date.now() + graceMs;
// Busy-wait: this runs from `process.on('exit')` handlers, where the event
// loop is already closed and nothing asynchronous can be awaited.
while (Date.now() < deadline) {
if (!procs.some(({ pid }) => alive(pid))) return procs.length;
}
for (const { pid } of procs) {
if (alive(pid)) signal(pid, 'SIGKILL');
}
return procs.length;
}
export function alive(pid) {
try {
process.kill(pid, 0);
return true;
} catch (err) {
return err.code === 'EPERM';
}
}
function signal(pid, sig) {
// A process group id is always the pid of its leader, so `-pid` can only ever
// reach a group this process leads. When it leads none, the call is ESRCH and
// the plain kill below is what does the work.
try { process.kill(-pid, sig); } catch { /* not a group leader */ }
try { process.kill(pid, sig); } catch { /* already gone */ }
}
/**
* Whether a `ps -E` line contains `marker` as a complete environment entry.
*
* A plain substring test is wrong here: a marker is a substring of any longer
* value that starts with it, and matching that would let one checkout's cleanup
* kill a neighbouring checkout's servers. `ps` flattens the environment into
* whitespace-separated `KEY=VALUE` pairs, so an entry runs to the next
* whitespace or to the end of the line.
*
* That simple rule is sound only because every marker value is drawn from
* `MARKER_VALUE_RE` and can therefore never contain whitespace: no value can
* hide the end of its own entry, and there is no ambiguous case. Give a marker
* a free-form value and this becomes guesswork again, which is why
* `assertMarkerValue` refuses one.
*/
export function envLineHasEntry(line, marker) {
for (let from = 0; ; from += 1) {
const at = line.indexOf(marker, from);
if (at === -1) return false;
const startsEntry = at === 0 || /\s/.test(line[at - 1]);
const end = at + marker.length;
const endsEntry = end === line.length || /\s/.test(line[end]);
if (startsEntry && endsEntry) return true;
from = at;
}
}
/** Refuse a marker value that could break out of its own environment entry. */
function assertMarkerValue(value) {
if (!MARKER_VALUE_RE.test(value)) {
throw new Error(
`refusing to match on "${value}": a marker value must be [A-Za-z0-9_-] only, ` +
'so that an environment entry ends where the whitespace after it does. ' +
'Hash or tokenize the value before passing it (see repoMarker).',
);
}
return value;
}
/** pid -> full command line, for every process this user can see. */
function listCommands() {
const map = new Map();
if (process.platform === 'win32') return map; // no supported sweep yet
const res = spawnSync('ps', ['-A', '-ww', '-o', 'pid=,command='], { encoding: 'utf-8' });
if (res.status !== 0 || !res.stdout) return map;
for (const line of res.stdout.split('\n')) {
const m = /^\s*(\d+)\s+(.*)$/.exec(line);
if (m) map.set(Number(m[1]), m[2]);
}
return map;
}
/**
* Pids whose environment contains one of `markers`.
*
* Linux exposes `/proc/<pid>/environ` directly, so entries are compared whole.
* BSD/macOS `ps -E` appends the environment to the command column instead, so
* the marker is matched against that combined line through `envLineHasEntry`,
* which requires the same whole-entry boundary. The command itself is then read
* back from the marker-free listing, so an env value that happened to contain
* "live-server" cannot decide the match.
*/
function pidsWithEnvMarker(markers, commands) {
const hits = [];
if (process.platform === 'linux') {
let entries = [];
try { entries = readdirSync('/proc'); } catch { return hits; }
for (const entry of entries) {
if (!/^\d+$/.test(entry)) continue;
let environ = '';
try { environ = readFileSync(`/proc/${entry}/environ`, 'utf-8'); } catch { continue; }
const vars = environ.split('\0');
if (markers.some((marker) => vars.includes(marker))) hits.push(Number(entry));
}
return hits;
}
const res = spawnSync('ps', ['-A', '-E', '-ww', '-o', 'pid=,command='], { encoding: 'utf-8' });
if (res.status !== 0 || !res.stdout) return hits;
for (const line of res.stdout.split('\n')) {
const m = /^\s*(\d+)\s+(.*)$/.exec(line);
if (!m) continue;
const pid = Number(m[1]);
if (!commands.has(pid)) continue;
if (markers.some((marker) => envLineHasEntry(m[2], marker))) hits.push(pid);
}
return hits;
}
scripts/lib/process-group.mjs
+146
View File
@@ -0,0 +1,146 @@
/**
* Stopping a spawned process group without blocking the event loop.
*
* The test runner puts every suite command in its own process group so one
* signal can take down the runner, every test file it forked, and anything
* those forked. Ending that group has one non-obvious constraint: **the wait
* for the child to die cannot be a poll on liveness.**
*
* A dead child stays a zombie until its parent reaps it, and the parent here is
* this process, which reaps through libuv when the event loop runs. So a busy
* wait on `kill(pid, 0)` is doubly wrong: it blocks the very loop that would do
* the reaping, and it then reads the unreaped zombie as alive. The wait would
* always burn its full grace period and always end in a needless SIGKILL. There
* is no waitpid from JavaScript that would see through this, so the wait has to
* be asynchronous and keyed on the child's own `exit` event instead.
*/
/** Signal the whole group, then the child itself in case it leads no group. */
export function signalGroup(child, sig) {
try { process.kill(-child.pid, sig); } catch { /* group already gone */ }
try { child.kill(sig); } catch { /* already gone */ }
}
/**
* Wrap a spawned child so its exit is observable as both a flag and a promise.
* Register this before any other `exit` listener so `hasExited` is already true
* by the time the others run.
*
* @returns {{child: import('node:child_process').ChildProcess, exited: Promise<void>, hasExited: boolean}}
*/
export function trackChildExit(child) {
const running = { child, hasExited: false, exited: null };
running.exited = new Promise((resolve) => {
child.once('exit', () => {
running.hasExited = true;
resolve();
});
});
return running;
}
/**
* SIGTERM the group, wait for the child to actually exit, and escalate to
* SIGKILL only if it has not exited within `graceMs`. Returns as soon as the
* child is gone, so a suite that dies promptly costs milliseconds rather than
* the whole grace period.
*
* @returns {Promise<'exited'|'killed'|'already-gone'>}
*/
export async function stopGroup(running, { graceMs = 2000, killGraceMs = 500 } = {}) {
if (!running || running.hasExited) return 'already-gone';
signalGroup(running.child, 'SIGTERM');
if (await raceExit(running, graceMs)) return 'exited';
signalGroup(running.child, 'SIGKILL');
await raceExit(running, killGraceMs);
return 'killed';
}
/**
* The last resort, for `process.on('exit')` where the loop is closed and
* nothing can be awaited. It cannot wait, so it does not pretend to: SIGTERM
* for anything that handles it, then SIGKILL for anything that does not.
*/
export function killGroupSync(running) {
if (!running || running.hasExited) return false;
signalGroup(running.child, 'SIGTERM');
signalGroup(running.child, 'SIGKILL');
return true;
}
function raceExit(running, ms) {
return new Promise((resolve) => {
const timer = setTimeout(() => resolve(false), ms);
running.exited.then(() => {
clearTimeout(timer);
resolve(true);
});
});
}
/**
* The runner's shutdown state machine: which group is ours, and how it ends.
*
* Kept here rather than as loose module state in the runner because the
* interesting case is a state bug, not a signalling one. The first signal hands
* the group off from `current` to `stopping` and then awaits the grace period.
* A second signal has to be able to reach that same handle, or it kills nothing
* and `exit` abandons the escalation still in flight, leaving a detached suite
* running after the runner is gone. Clearing one reference without holding the
* other is exactly how that happens.
*
* @param {object} [opts]
* @param {(code: number) => void} [opts.exit] injectable for tests; in
* production this never returns, so a shutdown that has been overtaken
* by a second signal simply stops there.
*/
export function createGroupShutdown({ exit = (code) => process.exit(code), graceMs = 2000 } = {}) {
let current = null;
let stopping = null;
let shuttingDown = false;
return {
/** Adopt a freshly spawned group. */
track(running) {
current = running;
return running;
},
/** The group finished on its own; nothing left to end. */
release() {
current = null;
},
get shuttingDown() {
return shuttingDown;
},
/** A termination signal arrived. The second one stops waiting. */
async onSignal(exitCode) {
if (shuttingDown) {
// Still inside the first shutdown's grace period. Do not wait it out:
// kill the handle that shutdown is holding, which `current` no longer
// is, and leave.
killGroupSync(stopping);
exit(exitCode);
return;
}
shuttingDown = true;
stopping = current;
current = null;
await stopGroup(stopping, { graceMs });
stopping = null;
exit(exitCode);
},
/** `process.on('exit')`: the last resort, and the one that cannot wait. */
onExit() {
const running = current || stopping;
current = null;
stopping = null;
return killGroupSync(running);
},
};
}
scripts/lib/test-orphan-reaper.mjs
+55
View File
@@ -0,0 +1,55 @@
#!/usr/bin/env node
/**
* Kills the live servers of one test process once that process is gone.
*
* In-process cleanup (an `after()` hook, a `process.on('exit')` handler) cannot
* run when the process is `SIGKILL`ed, and that is exactly the case that left
* 197 orphaned servers on a laptop. So the last line of defence lives outside
* the process: this reaper is spawned detached, holding the write end of a pipe
* on its stdin. When its parent dies, for any reason at all, the pipe closes,
* the reaper wakes on EOF, and it kills every live server carrying the parent's
* process marker.
*
* Scope is the marker, never a name or a port: only servers this exact test
* process started are matched.
*
* node scripts/lib/test-orphan-reaper.mjs <procId> [maxLifetimeMs]
*
* Deliberately not named after the thing it kills: its own argv must not look
* like a live server to the sweep.
*/
import { findLiveServers, killLiveServers } from './live-server-processes.mjs';
const procId = process.argv[2];
const maxLifetimeMs = Number(process.argv[3]) || 6 * 60 * 60 * 1000;
if (!procId) {
console.error('usage: test-orphan-reaper.mjs <procId> [maxLifetimeMs]');
process.exit(2);
}
let done = false;
function sweepAndExit(code = 0) {
if (done) return;
done = true;
try {
const leaked = findLiveServers({ procId });
if (leaked.length) killLiveServers(leaked);
} catch { /* nothing useful to report: no one is reading our output */ }
process.exit(code);
}
// The parent holds the other end of stdin. EOF means the parent is gone.
process.stdin.resume();
process.stdin.on('end', () => sweepAndExit(0));
process.stdin.on('close', () => sweepAndExit(0));
process.stdin.on('error', () => sweepAndExit(0));
// A reaper whose parent somehow outlives the machine's patience still goes away.
setTimeout(() => sweepAndExit(0), maxLifetimeMs).unref();
for (const sig of ['SIGTERM', 'SIGINT', 'SIGHUP']) {
process.on(sig, () => sweepAndExit(0));
}
scripts/run-tests.mjs
+149 -24
View File
@@ -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() {
scripts/test-suites.mjs
+4
View File
@@ -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',
tests/lib/live-servers.mjs
+128
View File
@@ -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;
}
}
tests/live-e2e/session.mjs
+8 -2
View File
@@ -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 = [];
tests/live-poll-stream.test.mjs
+5 -2
View File
@@ -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();
tests/live-server-leak.test.mjs
+235
View File
@@ -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/,
);
});
});
tests/live-server.test.mjs
+7 -2
View File
@@ -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();
tests/live-target-context.test.mjs
+5
View File
@@ -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, '..');
tests/process-group.test.mjs
+200
View File
@@ -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;
});
});