From 72be41d96defcd819eba0658358e0c0e2b15e56b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 15 Sep 2026 18:31:05 -0400 Subject: [PATCH] fix(#4601): reap the whole process tree on run-with-timeout's Windows force stage (#4775) * test(#4601): pin the run-with-timeout tree reap on win32 * docs(#4601): add changeset for the win32 tree reap * fix(#4601): reap the wrapped command's whole tree on Windows timeouts * docs(#4601): backfill changeset PR number * fix(#4601): keep the mediated shim alive without depending on stdin --------- Co-authored-by: sim --- .changeset/steady-herons-wander.md | 5 ++ gsd-core/bin/gsd-tools.cjs | 31 ++++++++-- tests/run-with-timeout.test.cjs | 99 ++++++++++++++++++++++++++++-- 3 files changed, 125 insertions(+), 10 deletions(-) create mode 100644 .changeset/steady-herons-wander.md diff --git a/.changeset/steady-herons-wander.md b/.changeset/steady-herons-wander.md new file mode 100644 index 000000000..7e7873099 --- /dev/null +++ b/.changeset/steady-herons-wander.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4775 +--- +**run-with-timeout now kills the whole process tree on Windows** — a timed-out command's descendants (e.g. a model CLI wrapped via workflow.cross_ai_command) no longer survive the timeout and keep running unbounded; POSIX behavior is unchanged. (#4601) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 0e8ea64ed..8a774a65a 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -4543,7 +4543,7 @@ async function dispatchHostCommand({ command, args, cwd, raw, error, defaultValu // keep working. No shell is spawned (argv array) — no injection surface beyond // the old `timeout … bash -c "$CMD"`. function runWithTimeout(argv) { - const { spawn } = require('node:child_process'); + const { spawn, spawnSync } = require('node:child_process'); const os = require('node:os'); const USAGE = 'Usage: gsd_run run-with-timeout [--] [args...]'; @@ -4568,9 +4568,11 @@ function runWithTimeout(argv) { const isWin = process.platform === 'win32'; // Detached (own process group) on POSIX so a timeout can reap the WHOLE tree — - // a bare child.kill() misses grandchildren (e.g. a test runner's workers) and - // would not actually bound the wall clock. Windows has no POSIX process - // groups; a direct kill is the best portable option there. + // a bare child.kill() misses grandchildren (e.g. a test runner's workers). + // Windows has no POSIX process groups and process.kill(-pid) is unsupported + // there, so EVERY killTree attempt on Windows tree-kills via + // `taskkill /PID /T /F` while the root is alive (see killTree) — by the + // time the direct child exits, its descendants are already orphaned. const detached = !isWin && secs > 0; const spawnFailureCode = (err) => (err && err.code === 'ENOENT' ? 127 : err && err.code === 'EACCES' ? 126 : 125); @@ -4623,6 +4625,27 @@ function runWithTimeout(argv) { if (detached && child.pid) { try { process.kill(-child.pid, signal); return; } catch { /* group already gone */ } } + if (isWin && child.pid) { + // #4601: Windows has no POSIX process groups, so the tree kill rides + // on `taskkill /T`, which walks the child's descendants the way + // `process.kill(-pid)` reaches a POSIX group — this is what bounds + // the wall clock when the direct child mediates (cmd.exe /c shim) or + // spawns its own children. Deliberately NOT gated on the SIGKILL + // stage: child.kill on Windows is TerminateProcess regardless of + // signal, so by the time the direct child exits its descendants are + // orphaned and no taskkill can reach them — the tree kill must ride + // the FIRST attempt, while the root is still alive. /F is required: + // without it taskkill posts WM_CLOSE, which a headless CLI never + // pumps. Spawned as an argv array per the no-shell-for-argv-array + // contract, and bounded — a non-zero/absent status means taskkill + // lost a race with an exiting process, and we fall through to the + // direct kill so the attempt is never weaker than before. + const reap = spawnSync('taskkill', ['/PID', String(child.pid), '/T', '/F'], { + encoding: 'utf8', + timeout: 15000, // taskkill /T is sub-second in practice; bounded so a wedged taskkill can't hang the gate + }); + if (reap.status === 0) return; + } child.kill(signal); } catch { /* already exited */ } }; diff --git a/tests/run-with-timeout.test.cjs b/tests/run-with-timeout.test.cjs index 37f32bfa6..b1cc9ae37 100644 --- a/tests/run-with-timeout.test.cjs +++ b/tests/run-with-timeout.test.cjs @@ -35,12 +35,13 @@ const { createTempDir, cleanup } = require('./helpers.cjs'); const RUN_WITH_TIMEOUT_HARNESS_BACKSTOP_MS = 30000; /** - * Tighter backstop for the C1 reap test specifically: its own verb-internal - * budget is 3s plus ~900ms of heartbeat-settle waits, so this keeps a - * smaller margin than the file default while still comfortably covering it. - * Pre-existing value, unchanged. + * Tighter backstop for the tree-reap tests (the C1 POSIX group reap and the + * #4601 win32 tree kill): their own verb-internal budgets are 2-3s plus ~900ms + * of heartbeat-settle waits, so this keeps a smaller margin than the file + * default while still comfortably covering them. Pre-existing value, unchanged + * (generalized from C1-only to both reap tests by #4601). */ -const RUN_WITH_TIMEOUT_C1_REAP_BACKSTOP_MS = 20000; +const RUN_WITH_TIMEOUT_REAP_BACKSTOP_MS = 20000; const ROOT = path.join(__dirname, '..'); const GSD_TOOLS = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -200,7 +201,7 @@ describe('#2351 run-with-timeout — kill semantics (POSIX process groups)', () 'process.on("SIGTERM", () => process.exit(0));', 'setInterval(() => {}, 1000);', ].join('\n')); - const r = runVerb(['3', '--', NODE, parentFile, hbFile], { timeout: RUN_WITH_TIMEOUT_C1_REAP_BACKSTOP_MS }); + const r = runVerb(['3', '--', NODE, parentFile, hbFile], { timeout: RUN_WITH_TIMEOUT_REAP_BACKSTOP_MS }); assert.equal(r.status, 124, 'must report a timeout (124), not hang'); assert.ok(fs.existsSync(hbFile), 'child heartbeat should exist'); await sleep(300); // let any in-flight write settle after the SIGKILL @@ -216,6 +217,92 @@ describe('#2351 run-with-timeout — kill semantics (POSIX process groups)', () }); }); +describe('#4601 run-with-timeout — tree reap (Windows has no process groups)', () => { + // The #4601 defect: on Windows `detached` is always false, so killTree fell + // through to a bare child.kill(), which terminates the DIRECT child only — + // any process the child launched survived the timeout and kept running + // (typically a model CLI wrapped via workflow.cross_ai_command, calling a + // paid API with nothing bounding it). The fix tree-kills via + // `taskkill /PID /T /F` on the FIRST kill attempt — it must run while + // the root is alive, because child.kill on Windows is TerminateProcess and + // the moment the direct child exits its descendants are orphaned and no + // taskkill can reach them (the issue's graceful-then-forceful staging prose + // is unsatisfiable there; its own snippet applies taskkill on every call). + // Methodology mirrors the C1 test above: HEARTBEAT liveness, not + // kill(pid,0); first sample written synchronously at startup; bounded + // settle waits >> the 100ms tick. + // + // KNOWN RUNNER MASKING, recorded deliberately: on GitHub's windows runners + // the test harness reaps orphaned descendants shortly after the verb exits, + // so the freeze assertion can pass even against UNFIXED code (observed: the + // direct tree test below was green on the unfixed RED head's windows tier). + // The environment where the bug manifests is a user's real Windows session, + // per the reporter's repro. These tests pin the contract; they are not a + // full differential witness on CI runners. + const isWin = process.platform === 'win32'; + + test("a timed-out command's descendant stops ticking — the tree dies, not just the direct child", { skip: !isWin ? 'win32-only' : false }, async (t) => { + const dir = createTempDir('rwt-4601-tree'); + t.after(() => cleanup(dir)); + const parentFile = path.join(dir, 'parent.js'); + const hbFile = path.join(dir, 'heartbeat'); + fs.writeFileSync(parentFile, [ + 'const cp = require("child_process");', + "const childCode = 'const fs=require(\"fs\");const hb=process.argv[1];const tick=()=>fs.writeFileSync(hb,String(Date.now()));tick();setInterval(tick,100);';", + 'cp.spawn(process.execPath, ["-e", childCode, process.argv[2]], { stdio: "ignore" });', + // The parent does NOT trap or exit on SIGTERM: on Windows the graceful + // stage is TerminateProcess on the direct child either way — what #4601 + // tests is that the DESCENDANT (which no direct kill can reach) is gone. + 'setInterval(() => {}, 1000);', + ].join('\n')); + const r = runVerb(['2', '--', NODE, parentFile, hbFile], { timeout: RUN_WITH_TIMEOUT_REAP_BACKSTOP_MS }); + assert.equal(r.status, 124, 'must report a timeout (124), not hang'); + assert.ok(fs.existsSync(hbFile), 'descendant heartbeat should exist'); + await sleep(300); // let any in-flight write settle after the tree kill + const first = fs.readFileSync(hbFile, 'utf8'); + await sleep(600); // >> the 100ms heartbeat interval + const second = fs.readFileSync(hbFile, 'utf8'); + assert.equal(second, first, 'descendant must be tree-killed (heartbeat frozen), not orphaned and still ticking'); + }); + + test('a fast-exiting command still exits with its own code — the win32 branch never perturbs the normal path', { skip: !isWin ? 'win32-only' : false }, () => { + // Negative-space guard: for a fast command no timer fires and killTree is + // never invoked — the taskkill branch must only ever attach to killTree + // attempts, never to the timerless normal path. + const r = runVerb(['5', '--', NODE, '-e', 'process.exit(7)']); + assert.equal(r.status, 7); + }); + + test('a timed-out .cmd-mediated command has its whole subtree reaped (the model-CLI shape)', { skip: !isWin ? 'win32-only' : false }, async (t) => { + // The #2667 mediation makes cmd.exe the direct child, so the real command + // is a GRANDCHILD even with no further nesting — this is the shape of + // workflow.cross_ai_command wrapping a model CLI .cmd shim (the paid-orphan + // case from the issue). taskkill /T from the cmd.exe pid must reach it. + // The shim stays alive with a ping sleep-loop, NOT `pause`: pause blocks on + // stdin, which spawnSync-harness semantics resolve differently (observed on + // a real runner: the shim quit early and the verb exited 0, no timeout). + const dir = createTempDir('rwt-4601-cmd'); + t.after(() => cleanup(dir)); + const shim = path.join(dir, 'slow.cmd'); + const hbFile = path.join(dir, 'heartbeat'); + fs.writeFileSync(shim, [ + '@echo off', + `start "" /b "${NODE}" -e "const fs=require('fs');const hb=process.argv[1];const tick=()=>fs.writeFileSync(hb,String(Date.now()));tick();setInterval(tick,100);" "${hbFile}"`, + ':loop', + 'ping -n 2 127.0.0.1 > NUL', + 'goto :loop', + ].join('\r\n'), 'utf8'); + const r = runVerb(['2', '--', shim], { timeout: RUN_WITH_TIMEOUT_REAP_BACKSTOP_MS }); + assert.equal(r.status, 124, 'the mediated .cmd must time out (124), not exit early or hang'); + assert.ok(fs.existsSync(hbFile), 'grandchild heartbeat should exist'); + await sleep(300); // let any in-flight write settle after the tree kill + const first = fs.readFileSync(hbFile, 'utf8'); + await sleep(600); // >> the 100ms heartbeat interval + const second = fs.readFileSync(hbFile, 'utf8'); + assert.equal(second, first, 'the .cmd grandchild must be tree-killed (heartbeat frozen), not orphaned and still ticking'); + }); +}); + describe('#2667 run-with-timeout — Windows .cmd/.bat/.exe spawn mediation (CVE-2024-27980)', () => { // Node's CVE-2024-27980 hardening throws EINVAL when child_process.spawn is // given a .cmd/.bat without a shell. run-with-timeout now mediates .cmd/.bat