diff --git a/.changeset/sunny-wasps-cheer.md b/.changeset/sunny-wasps-cheer.md new file mode 100644 index 000000000..9cf1ec984 --- /dev/null +++ b/.changeset/sunny-wasps-cheer.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4615 +--- +**A hung bounded test check no longer leaks a permanent CPU-pegging orphan process.** `node --test`'s per-file worker subprocess (the process default since Node 22) survived a timed-out check's own kill signal, which only reached the direct runner -- the worker was reparented to PID 1 and could busy-loop forever, consuming a full core, with no visible indication anything was wrong. The bounded check now reaps the whole process tree (POSIX process-group SIGKILL, Windows `taskkill /T /F`) when its own timeout fires. (#3660) diff --git a/src/prohibition-enforcement.cts b/src/prohibition-enforcement.cts index e12e8c5e4..467c8683b 100644 --- a/src/prohibition-enforcement.cts +++ b/src/prohibition-enforcement.cts @@ -42,7 +42,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { execFileSync } from 'node:child_process'; +import { execFileSync, spawnSync } from 'node:child_process'; // Import the leaf I/O module directly (core.cjs re-export spine retired in epic #1267). // eslint-disable-next-line @typescript-eslint/no-require-imports import io = require('./io.cjs'); @@ -450,6 +450,95 @@ const NODE_TEST_TIMEOUT_MS = 30_000; const ESLINT_TIMEOUT_MS = 60_000; const CHECK_MAX_BUFFER = 16 * 1024 * 1024; +/** Windows `taskkill` resolved by ABSOLUTE path — never a bare PATH-resolved name. A project + * directory used as the child's `cwd` could otherwise contain a planted `taskkill.exe`/`.bat` + * that Windows executable resolution picks up ahead of the real one (#3660 review, minor-9). + * Returns null (never a hardcoded fallback, per tests/hardcoded-paths.test.cjs) when neither env + * var is set -- not expected on a real Windows host (both are set by the OS itself), but a + * hostile/stripped env should degrade to "skip the reap" rather than guess a system path. */ +function taskkillPath(): string | null { + const root = process.env.SystemRoot || process.env.windir; + return root ? path.join(root, 'System32', 'taskkill.exe') : null; +} + +/** + * Reap the process TREE rooted at `pid` after THIS call's own bound killed it (#3660: `node --test` + * forks a per-file WORKER by default since Node 22 — `execFileSync`'s `timeout` signals only the + * direct child/runner, never the worker, which is reparented to PID 1 and can busy-loop forever). + * + * POSIX: the child was spawned `detached` (its own process group, pgid === pid), so `-pid` addresses + * the whole group. ESRCH (group already gone) is swallowed — the subject may have exited on its own + * between the timeout firing and this call. + * + * Windows has no process-group equivalent; `taskkill /PID /T /F` walks the live process tree by + * parent-PID instead, which does not require the parent PID to still be alive. A non-zero exit means + * "nothing left to kill" (already gone, or never had descendants) — not a failure, so it is never + * escalated; there is no portable stronger primitive to escalate TO. + * + * Never throws — this runs from a `catch`/`finally` path and must not itself become the error. + */ +function reapDescendants(pid: number | undefined): void { + if (typeof pid !== 'number' || pid <= 0) return; // defensive: never signal pid 0 (self) or negative + if (process.platform === 'win32') { + const exe = taskkillPath(); + if (!exe) return; // no safe absolute path available -- best-effort, skip rather than guess + try { + // 5s bound (DEFECT.UNBOUNDED-SUBPROCESS): a local OS command, not a network call -- if + // taskkill itself hangs, this function's own "never throws" contract already treats that + // identically to any other failure (swallowed below), so a bounded timeout costs nothing + // and just prevents a stuck taskkill from blocking the caller forever. + spawnSync(exe, ['/PID', String(pid), '/T', '/F'], { stdio: 'ignore', windowsHide: true, timeout: 5_000 }); + } catch { + // best-effort: a missing taskkill.exe (or a timeout) is not this call's problem to escalate + } + return; + } + try { + process.kill(-pid, 'SIGKILL'); + } catch { + // Swallows ESRCH (group already gone -- nothing to reap) and any other errno (e.g. EPERM) -- + // this helper never throws regardless of cause; see the function's own doc comment above. + } +} + +/** + * `execFileSync`, with descendant reaping layered on top. Same contract (same return value, throws + * the identical error) EXCEPT that when — and ONLY when — this call's OWN timeout killed the child, + * any descendants the child forked are also reaped. + * + * Gated on `error.code === 'ETIMEDOUT'`, NOT `error.signal`. `signal` is the field Node's own docs + * describe for this purpose, but it is not reliably populated across platforms/Node versions: on one + * real Linux CI run (Node 24) a genuine timeout-kill threw `{ signal: null, code: 'ETIMEDOUT', + * status: 7 }` — `signal` was simply absent, `code` was the only reliable marker (confirmed empirically + * before landing this; a macOS/Node run separately showed `signal: 'SIGTERM'` for the identical + * scenario, so neither field alone is safe to rely on everywhere — `code` was the one constant). + * Gating strictly on the timeout code (rather than reaping on every throw) matters: an ordinary + * non-zero exit (a real test/lint failure) has no `ETIMEDOUT` code — the child exited on its own, so a + * reap there would fire on every red run for no reason and, on POSIX, risks signalling a process group + * whose pgid was *already* recycled by something unrelated in the time since (the #3660 review's + * Blocker-3 defect in the prior attempt at this fix, PR #3681). Only the timeout-kill path is targeted. + * + * Spawns `detached` on POSIX so the reap above can address the whole process group; omitted on + * Windows (no such flag there — `@types/node`'s `ExecFileSyncOptions` doesn't declare `detached` + * either, hence the cast below, though libuv honors it identically to `spawnSync`). + */ +function execFileSyncReaping( + file: string, + args: readonly string[], + options: { cwd: string; encoding: 'utf-8'; stdio: ['ignore', 'pipe', 'pipe']; windowsHide: true; env: NodeJS.ProcessEnv; timeout: number; maxBuffer: number }, +): string { + const spawnOptions = process.platform === 'win32' ? options : ({ ...options, detached: true } as typeof options & { detached: true }); + try { + return execFileSync(file, args, spawnOptions); + } catch (e) { + const err = e as { pid?: unknown; code?: unknown }; + if (typeof err.pid === 'number' && err.code === 'ETIMEDOUT') { + reapDescendants(err.pid); + } + throw e; + } +} + /** Resolve the effective timeout: only a POSITIVE override is honored — `0` (which Node treats as * "no timeout") or a negative value falls back to the bounded default, so the subprocess is ALWAYS * bounded (a `timeoutMs: 0` injection can never disable the bound). */ @@ -467,7 +556,7 @@ function posTimeout(timeoutMs: number | undefined, def: number): number { */ function runNodeTestWithSubject(check: CheckDescriptor, cwd: string, subject: string, timeoutMs?: number): string { try { - return execFileSync(process.execPath, buildNodeTestArgs(check), { + return execFileSyncReaping(process.execPath, buildNodeTestArgs(check), { cwd, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], @@ -487,7 +576,7 @@ function defaultRunCheck(check: CheckDescriptor, cwd: string, timeoutMs?: number if (check.kind === 'node-test') { let out = ''; try { - out = execFileSync(process.execPath, buildNodeTestArgs(check), { + out = execFileSyncReaping(process.execPath, buildNodeTestArgs(check), { cwd, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], @@ -509,7 +598,7 @@ function defaultRunCheck(check: CheckDescriptor, cwd: string, timeoutMs?: number if (!eslintCli) return { passed: false }; // eslint not installed -> fail closed, never throw let json = ''; try { - json = execFileSync(process.execPath, [eslintCli, ...buildLintArgs(check)], { + json = execFileSyncReaping(process.execPath, [eslintCli, ...buildLintArgs(check)], { cwd, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], @@ -566,7 +655,7 @@ function defaultProveFailFirst(check: CheckDescriptor, cwd: string, timeoutMs?: if (!eslintCli) return { provenFailFirst: false }; // eslint not installed -> fail closed, never throw let json = ''; try { - json = execFileSync(process.execPath, [eslintCli, ...buildLintArgs({ ...check, target: fixture })], { + json = execFileSyncReaping(process.execPath, [eslintCli, ...buildLintArgs({ ...check, target: fixture })], { cwd, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], diff --git a/tests/prohibition-enforcement.test.cjs b/tests/prohibition-enforcement.test.cjs index 7700fb4a7..94587f7bf 100644 --- a/tests/prohibition-enforcement.test.cjs +++ b/tests/prohibition-enforcement.test.cjs @@ -500,6 +500,7 @@ describe('prohibition-enforcement real-runner helpers (#1259)', () => { describe('prohibition-enforcement REAL runner end-to-end (#1259)', () => { const fs = require('node:fs'); const { spawn } = require('node:child_process'); + const { setTimeout: sleep } = require('node:timers/promises'); // The hang fixture served to the bounded-timeout test below, hoisted so the #4104 self-exit // regression cannot drift from the body it guards. Parks on a SETTLING 10s timer: still "hung" @@ -777,6 +778,172 @@ describe('prohibition-enforcement REAL runner end-to-end (#1259)', () => { assert.equal(result.evidence.length, 0); }); + // ─── #3660 regression: a HANGING node-test's per-file WORKER must not be orphaned ────────────── + // `node --test` forks a per-file worker subprocess by default (Node 22+, `--test-isolation=process`); + // `execFileSync`'s `timeout` only signals the direct child (the runner), never the worker. These + // exercise the REAL, uninjected `defaultRunCheck` -> `execFileSyncReaping` path (no `runCheck` + // injected) and observe a real OS-level pid, so the fix (`reapDescendants`) is proven, not a mock. + + /** Poll for the fixture's pidfile with bounded retry-with-backoff (no fixed sleep) — the pidfile + * write happens inside the spawned worker, which may take a beat to start. */ + async function readPidWithRetry(pidfilePath, { attempts = 30, delayMs = 150 } = {}) { + for (let i = 0; i < attempts; i += 1) { + if (fs.existsSync(pidfilePath)) { + const txt = fs.readFileSync(pidfilePath, 'utf-8').trim(); + if (txt) return Number(txt); + } + await sleep(delayMs); + } + throw new Error(`pidfile ${pidfilePath} was never written within the retry budget`); + } + + /** Liveness probe. `process.kill(pid, 0)` alone cannot distinguish a genuinely-running process + * from an already-killed ZOMBIE stuck unreaped in a container with no init process to collect + * orphans (a real, confirmed condition on this repo's own Linux CI bench) -- both report "exists" + * with no throw. On Linux, read /proc//stat's process-state field (3rd whitespace-separated + * token, inside the trailing `)` after the command name, which itself may contain spaces/parens) + * and treat state 'Z' (zombie) as DEAD -- it is no longer executing or consuming CPU, which is + * the actual thing #3660 cares about. Falls back to the plain kill(pid,0) probe on non-Linux + * platforms (no /proc there) and if /proc//stat is unreadable for any reason (already fully + * gone, permissions, etc. -- ENOENT there means genuinely dead too). */ + function isAlive(pid) { + if (process.platform === 'linux') { + let stat; + try { + stat = fs.readFileSync(`/proc/${pid}/stat`, 'utf-8'); + } catch (err) { + // ENOENT: /proc/ genuinely gone -- fully reaped, no zombie remnant. Any OTHER read + // error (EACCES, EIO, ...) is inconclusive -- report "alive" rather than risk a false + // "dead" that would silently mask a real regression (a liveness check should fail loud + // via a longer retry loop, not fail quiet via a wrong verdict). + if (err && err.code === 'ENOENT') return false; + return true; + } + // Format: "pid (comm) state ...". comm may contain spaces/parens, so split on the LAST ')'. + const afterComm = stat.slice(stat.lastIndexOf(')') + 1).trim(); + const state = afterComm.split(/\s+/)[0]; + if (state === 'Z') return false; // zombie: already dead, just not yet reaped by its parent + return true; + } + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } + } + + /** Bounded retry-with-backoff until `isAlive(pid)` reports false, or the budget is exhausted. */ + async function waitUntilDead(pid, { attempts = 30, delayMs = 100 } = {}) { + let alive = isAlive(pid); + for (let i = 0; i < attempts && alive; i += 1) { + await sleep(delayMs); + alive = isAlive(pid); + } + return alive; + } + + test('isAlive(pid) correctly reports TRUE for a genuinely running process (own pid) -- closes the vacuous-test gap: without this, a probe that always returned false would pass every #3660 test below trivially', () => { + assert.equal(isAlive(process.pid), true, + 'isAlive must report this test\'s own (unambiguously running) process as alive'); + }); + + test('a HANGING node-test leaves no orphaned descendant behind (#3660: worker survives runner-only kill)', async (t) => { + const enforce = require(ENFORCEMENT_LIB); + const dir = createTempDir('prohib-orphan-hang-'); + t.after(() => cleanup(dir)); + const pidfilePath = path.join(dir, 'worker.pid'); + const tf = path.join(dir, 'hang-pid.test.cjs'); + // Blocks via Atomics.wait (NOT a busy `while(true)`) so this test does not peg a CPU core; the + // deadline (10s) is far longer than the check's own timeoutMs (1200ms) below. The worker writes + // its OWN pid before blocking, matching the maintainer-blessed fixture design (no pgrep/procps). + fs.writeFileSync(tf, + "const { test } = require('node:test');\n" + + "const fs = require('node:fs');\n" + + "test('blocks forever (#3660 regression fixture)', () => {\n" + + " fs.writeFileSync(process.env.GSD_TEST_PIDFILE, String(process.pid));\n" + + " Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10_000);\n" + + "});\n"); + const prevPidfileEnv = process.env.GSD_TEST_PIDFILE; + process.env.GSD_TEST_PIDFILE = pidfilePath; + t.after(() => { + if (prevPidfileEnv === undefined) delete process.env.GSD_TEST_PIDFILE; + else process.env.GSD_TEST_PIDFILE = prevPidfileEnv; + }); + // Real, UNINJECTED path: no runCheck/proveFailFirst override -> defaultRunCheck -> + // execFileSyncReaping runs the fixture for real. The short timeoutMs keeps this test fast. + const result = enforce.runProhibitionEnforcement( + TEST_TIER, + { kind: 'node-test', target: tf, failFirst: true }, + { cwd: dir, timeoutMs: 1200 }, + ); + assert.notEqual(result.status, 'green', 'a hung check must fail closed (unchanged pre-existing contract)'); + const workerPid = await readPidWithRetry(pidfilePath); + assert.ok(Number.isInteger(workerPid) && workerPid > 0, 'worker pid must be a real positive pid'); + const stillAlive = await waitUntilDead(workerPid); + assert.equal(stillAlive, false, + `the node --test worker (pid ${workerPid}) must be reaped, not orphaned (#3660)`); + }); + + test('control: a CLEAN node-test subject\'s worker exits on its own (no reap needed; proves the liveness probe is meaningful)', async (t) => { + const enforce = require(ENFORCEMENT_LIB); + const dir = createTempDir('prohib-orphan-control-'); + t.after(() => cleanup(dir)); + const pidfilePath = path.join(dir, 'worker.pid'); + const tf = path.join(dir, 'clean-pid.test.cjs'); + // Same fixture SHAPE (writes its own pid) but does NOT block — it exits on its own. This proves + // the isAlive/waitUntilDead probe can observe a live-then-dead transition at all, so the hang + // test's "not alive" assertion above is meaningful, not vacuously true. + fs.writeFileSync(tf, + "const { test } = require('node:test');\n" + + "const fs = require('node:fs');\n" + + "test('exits immediately, no hang', () => {\n" + + " fs.writeFileSync(process.env.GSD_TEST_PIDFILE, String(process.pid));\n" + + "});\n"); + const prevPidfileEnv = process.env.GSD_TEST_PIDFILE; + process.env.GSD_TEST_PIDFILE = pidfilePath; + t.after(() => { + if (prevPidfileEnv === undefined) delete process.env.GSD_TEST_PIDFILE; + else process.env.GSD_TEST_PIDFILE = prevPidfileEnv; + }); + enforce.runProhibitionEnforcement( + TEST_TIER, + { kind: 'node-test', target: tf, failFirst: true }, + { cwd: dir }, + ); + const workerPid = await readPidWithRetry(pidfilePath); + assert.ok(Number.isInteger(workerPid) && workerPid > 0, 'worker pid must be a real positive pid'); + const stillAlive = await waitUntilDead(workerPid); + assert.equal(stillAlive, false, + `control: the clean-exit worker (pid ${workerPid}) must be observably dead shortly after — proves the probe works`); + }); + + test('an ORDINARY FAILING node-test (no hang) fails closed exactly as before (#3660 non-regression: reap-gating does not alter the normal-failure path)', (t) => { + const enforce = require(ENFORCEMENT_LIB); + const dir = createTempDir('prohib-fail-ordinary-'); + t.after(() => cleanup(dir)); + const tf = path.join(dir, 'fails.test.cjs'); + fs.writeFileSync(tf, + "const { test } = require('node:test');\n" + + "const assert = require('node:assert');\n" + + "test('fails immediately, no hang', () => {\n" + + " assert.fail('deliberate ordinary failure (#3660 non-regression control)');\n" + + "});\n"); + const result = enforce.runProhibitionEnforcement( + TEST_TIER, + { kind: 'node-test', target: tf, failFirst: true }, + { cwd: dir }, + ); + // Same return-shape assertions as the pre-existing EMPTY-file fail-closed test above — the + // reap-gating change (gated strictly on `err.code === 'ETIMEDOUT'`, i.e. a timeout-kill) must + // not alter the ordinary non-zero-exit path's observable result. No wall-clock assertion + // (clock-seam rule): the absence of a hang is proven by this synchronous call returning at + // all, not by timing it. + assert.notEqual(result.status, 'green', 'an ordinary failing node-test must fail closed exactly as before this fix'); + assert.equal(result.located, true, 'the check was located; it just did not pass'); + assert.equal(result.evidence.length, 0); + }); + test('a clean in-tree target greens the lint-rule kind via the real eslint + real prover (SF-01: plugin loads)', () => { const enforce = require(ENFORCEMENT_LIB); // Migrated to the SHIPPING prover (#1279): the default real prover lints the committed