* test(#3660): prove a bounded node-test check orphans its worker on timeout Regression test only, no fix yet: `execFileSync`'s timeout kills the direct `node --test` runner but never the per-file worker it forks by default since Node 22 (`--test-isolation=process`). The worker is reparented to PID 1 and can busy-loop forever while the bounded-check verdict still reports a clean fail-closed timeout. Adds three tests driven through the real, uninjected defaultRunCheck path: a hanging subject's worker must not survive the call, a control proving the liveness probe can actually distinguish alive-vs-dead, and a non-hanging failure proving the reap-gating logic added by the next commit doesn't change the ordinary-failure return shape. Expected RED on this commit (src/prohibition-enforcement.cts is unchanged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): reap a bounded check's descendant worker after its own timeout kill execFileSync's timeout only signals the direct child (the node --test runner); since Node 22, node --test forks a per-file WORKER by default (--test-isolation=process), so a hung subject's worker survives the bound, gets reparented to PID 1, and busy-loops forever while the verdict still reports a clean fail-closed timeout. Adds execFileSyncReaping (wraps execFileSync, detached:true on POSIX) and reapDescendants(pid): POSIX process.kill(-pid, 'SIGKILL') against the process group, Windows an absolute-path taskkill /PID <pid> /T /F (never a bare PATH-resolved name -- PR #3681 review minor-9). The reap fires ONLY when this call's own timeout killed the child (the thrown error carries a signal) -- an ordinary non-zero-exit failure has signal:null and is left alone, which is the fix for PR #3681's Blocker-3 (that attempt reaped on every throw, risking a PGID-reuse collateral kill on a ordinary red run). All four execFileSync(process.execPath, ...) call sites now route through execFileSyncReaping: runNodeTestWithSubject, defaultRunCheck's node-test and lint-rule arms, defaultProveFailFirst's lint-rule arm (its node-test arm reuses runNodeTestWithSubject). RED proven on 0bd741fbc2b2ad0792fdf2361de14b68b2e3aea3 (test-only commit, gsd-test outcome:failed, exactly the new orphan-detection test failing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): address code-review nits on the reap doc comments - Clarify execFileSyncReaping's gate covers a maxBuffer-triggered kill too, not just a timeout -- both set .signal, both are "this call's own bound". - Note reapDescendants' POSIX catch swallows any errno, not only ESRCH. No behavior change (tsc --noEmit clean, no-op for the compiler). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * debug(#3660): fix hardcoded Windows path + add temp diagnostics for CI reap failure Real defect #1 (fixed for good): taskkillPath() had a 'C:\Windows' literal fallback, tripping tests/hardcoded-paths.test.cjs's repo-wide scanner. Now returns null when neither SystemRoot nor windir is set, and the caller skips the Windows reap rather than guessing a path. Real defect #2 (under investigation): the prior GREEN gsd-test run showed the #3660 orphan-detection test STILL failing on linux-node24 even with the fix applied -- the worker survived. Isolated diagnostic scripts against the exact same execFileSync({detached:true})+process.kill(-pid) mechanism, including one using a REAL node --test worker, both confirm the mechanism works correctly on macOS (group-kill reaches the worker). This commit adds TEMPORARY stderr instrumentation (GSD-DEBUG-3660 tags) around the reap attempt to get direct evidence from the actual Linux CI environment before guessing further. Will be removed once the root cause is confirmed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): gate the reap on err.code === 'ETIMEDOUT', not err.signal Root cause of the prior GREEN run's failure, confirmed with real evidence from linux-node24 CI: execFileSync's thrown error on a genuine timeout-kill does NOT reliably set `.signal` -- on that environment it came back `signal: null, code: 'ETIMEDOUT', status: 7`, so the reap gate never fired. A separate macOS/Node run of the identical scenario showed `signal: 'SIGTERM'` for the same case -- neither field alone is safe across platforms/versions, but `code === 'ETIMEDOUT'` was present and correct in both. Verified via temporary stderr instrumentation on a real gsd-test run (now removed) before landing this, rather than guessing from the macOS-only result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * debug(#3660): round-2 instrumentation -- ETIMEDOUT gate fix alone didn't work The err.code === 'ETIMEDOUT' gate fix (previous commit) did not resolve the failure -- same test still red on real Linux CI. Adding probes around the actual process.kill(-pid, 'SIGKILL') call itself to see whether it throws, and whether the group is observably alive/dead before and after, since the gate may now be firing correctly but the kill may not be reaching the worker's process group on this environment. Temporary, will be removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): the fix was already correct -- the TEST's liveness probe was not Root cause of the two prior red rounds, confirmed via process-group probes on real Linux CI: process.kill(-pid, 'SIGKILL') succeeds (no throw) every time the ETIMEDOUT gate fires -- the worker genuinely IS killed. But process.kill(pid, 0) cannot tell a truly-running process from an already-killed ZOMBIE stuck unreaped: this bench's container has no init process collecting arbitrary orphans, so a killed worker (reparented to PID 1 on death) sits as a zombie forever, still answering kill(pid,0) with "exists" even though it is fully dead and burning zero CPU -- which is the actual harm #3660 is about. Test now reads /proc/<pid>/stat's process-state field on Linux and treats 'Z' (zombie) as dead, falling back to the plain kill(pid,0) probe elsewhere (no /proc on macOS/Windows). Also strips the round-2 GSD-DEBUG-3660b instrumentation now that its evidence has been used and the real root cause is fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): merge duplicate doc comment, fix stale err.signal reference Leftover artifacts from the multi-round debugging: taskkillPath had two stacked doc comments (an edit only replaced the function body, not the original comment above it); a test comment still said "err.signal" after the gate was changed to err.code === 'ETIMEDOUT'. Comment-only, no behavior change (tsc --noEmit no-op). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): close a vacuous-test gap and harden isAlive's error handling Code review finding (major): none of the three #3660 regression tests ever asserted isAlive(pid) === true for a genuinely running process -- the real code path is fully synchronous, so there's no natural window to observe "alive" before "dead" inside those tests. A probe that always returned false would have passed all three vacuously. Added a standalone test proving isAlive(process.pid) reports true, using this test's own unambiguously-alive process, running before the three existing tests. Also hardened isAlive's /proc read-failure handling (minor finding): only ENOENT (process genuinely gone) now means "dead"; any other read error (EACCES, EIO, ...) reports "alive" (inconclusive) rather than risking a false "dead" that would silently mask a real regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): add changeset fragment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): bound the taskkill spawnSync with a timeout CI caught it: local/require-subprocess-timeout (DEFECT.UNBOUNDED-SUBPROCESS) flagged the new spawnSync(taskkill, ...) call in reapDescendants' Windows branch for having no timeout. 5s bound -- a local OS command, not a network call; reapDescendants already treats any failure (including a hypothetical hang) identically via its existing try/catch, so the bound costs nothing and just prevents a stuck taskkill from blocking the caller forever. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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 <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'],
|
||||
|
||||
Reference in New Issue
Block a user