diff --git a/bin/gstack-detach b/bin/gstack-detach index 101e86eee..938a17e8c 100755 --- a/bin/gstack-detach +++ b/bin/gstack-detach @@ -106,10 +106,18 @@ def child_run(args, log): except Exception: pass time.sleep(5) + # Group SIGKILL after the grace, not a direct-child kill: + # eval runs spawn claude/codex grandchildren that survive a + # proc.kill() and burn cores + API for hours (the observed + # 15-hour-orphan class). ESRCH here just means the group + # honored the SIGTERM. try: - proc.kill() + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) except Exception: - pass + try: + proc.kill() + except Exception: + pass code = "timeout" else: code = proc.wait() diff --git a/test/helpers/codex-session-runner.ts b/test/helpers/codex-session-runner.ts index 7d24248fd..e6ddf5837 100644 --- a/test/helpers/codex-session-runner.ts +++ b/test/helpers/codex-session-runner.ts @@ -15,8 +15,11 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; +import { spawn } from 'child_process'; +import { Readable } from 'node:stream'; import { hermeticChildEnv } from './hermetic-env'; import { extractSkillSections } from './skill-fixture'; +import { killProcessGroup } from '../../scripts/test-strict-output'; // --- Interfaces --- @@ -232,28 +235,42 @@ export async function runCodexSkill(opts: { // Hermetic scrub (test/helpers/hermetic-env.ts) with codex's auth surface // re-admitted: codex auths from $HOME/.codex (copied into tempHome above) // plus OPENAI_API_KEY/CODEX_* when present. HOME override merges last. - const proc = Bun.spawn(['codex', ...args], { + // node:child_process spawn with `detached` (own process group) — mirrors + // session-runner.ts. Bun.spawn's bare proc.kill() signalled only codex + // itself; command subprocesses codex spawned survived as orphans holding + // our pipes open (the same blocked-drain hang the claude runner fixed — + // this copy never inherited that fix until now). + const proc = spawn('codex', args, { cwd: cwd || skillDir, - stdout: 'pipe', - stderr: 'pipe', + stdio: ['ignore', 'pipe', 'pipe'], + detached: process.platform !== 'win32', env: hermeticChildEnv( { HOME: tempHome, CODEX_HOME: tempCodexDir }, { extraAllow: ['OPENAI_API_KEY', 'CODEX_*'] }, ), }); + const stdoutWeb = Readable.toWeb(proc.stdout!) as ReadableStream; + const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream; + const procExited: Promise = new Promise((resolve) => { + proc.on('close', (code) => resolve(code ?? 1)); + proc.on('error', () => resolve(1)); + }); // Race against timeout let timedOut = false; const timeoutId = setTimeout(() => { timedOut = true; - proc.kill(); + // Group SIGKILL + reader cancel: kill the whole tree AND unblock the + // read loop even if a stray grandchild survives the group kill. + killProcessGroup(proc, 'SIGKILL'); + reader.cancel().catch(() => { /* stream already closed */ }); }, timeoutMs); // Stream and collect JSONL from stdout const collectedLines: string[] = []; - const stderrPromise = new Response(proc.stderr).text(); + const stderrPromise = new Response(stderrWeb).text(); - const reader = proc.stdout.getReader(); + const reader = stdoutWeb.getReader(); const decoder = new TextDecoder(); let buf = ''; @@ -291,8 +308,18 @@ export async function runCodexSkill(opts: { collectedLines.push(buf); } - const stderr = await stderrPromise; - const exitCode = await proc.exited; + // Same orphan hazard as stdout: a grandchild holding stderr open would + // block this drain forever. Race it against child exit + a short grace + // window (ported from session-runner.ts — the codex copy lacked it). + const stderr = await Promise.race([ + stderrPromise, + (async () => { + await procExited; + await new Promise((r) => setTimeout(r, 5_000)); + return ''; + })(), + ]); + const exitCode = await procExited; clearTimeout(timeoutId); const durationMs = Date.now() - startTime; diff --git a/test/helpers/gemini-session-runner.ts b/test/helpers/gemini-session-runner.ts index 2b080265d..f9d9c478b 100644 --- a/test/helpers/gemini-session-runner.ts +++ b/test/helpers/gemini-session-runner.ts @@ -15,7 +15,10 @@ */ import * as path from 'path'; +import { spawn } from 'child_process'; +import { Readable } from 'node:stream'; import { hermeticChildEnv } from './hermetic-env'; +import { killProcessGroup } from '../../scripts/test-strict-output'; // --- Interfaces --- @@ -130,27 +133,40 @@ export async function runGeminiSkill(opts: { // Spawn gemini — uses real HOME for auth (~/.gemini; HOME is allowlisted), // cwd for skill discovery. Hermetic scrub with gemini's auth surface // re-admitted (previously this spawn inherited the full operator env). - const proc = Bun.spawn(['gemini', ...args], { + // node:child_process spawn with `detached` (own process group) — mirrors + // session-runner.ts. A bare kill signalled only gemini itself; tool + // subprocesses survived as orphans holding our pipes open (the same + // blocked-drain hang the claude runner fixed — this copy lacked it). + const proc = spawn('gemini', args, { cwd: cwd || process.cwd(), - stdout: 'pipe', - stderr: 'pipe', + stdio: ['ignore', 'pipe', 'pipe'], + detached: process.platform !== 'win32', env: hermeticChildEnv(undefined, { extraAllow: ['GEMINI_API_KEY', 'GOOGLE_API_KEY', 'GOOGLE_APPLICATION_CREDENTIALS', 'GOOGLE_CLOUD_*', 'GEMINI_*'], }), }); + const stdoutWeb = Readable.toWeb(proc.stdout!) as ReadableStream; + const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream; + const procExited: Promise = new Promise((resolve) => { + proc.on('close', (code) => resolve(code ?? 1)); + proc.on('error', () => resolve(1)); + }); // Race against timeout let timedOut = false; const timeoutId = setTimeout(() => { timedOut = true; - proc.kill(); + // Group SIGKILL + reader cancel: kill the whole tree AND unblock the + // read loop even if a stray grandchild survives the group kill. + killProcessGroup(proc, 'SIGKILL'); + reader.cancel().catch(() => { /* stream already closed */ }); }, timeoutMs); // Stream and collect JSONL from stdout const collectedLines: string[] = []; - const stderrPromise = new Response(proc.stderr).text(); + const stderrPromise = new Response(stderrWeb).text(); - const reader = proc.stdout.getReader(); + const reader = stdoutWeb.getReader(); const decoder = new TextDecoder(); let buf = ''; @@ -185,8 +201,18 @@ export async function runGeminiSkill(opts: { collectedLines.push(buf); } - const stderr = await stderrPromise; - const exitCode = await proc.exited; + // Same orphan hazard as stdout: a grandchild holding stderr open would + // block this drain forever. Race against child exit + a short grace window + // (ported from session-runner.ts — the gemini copy lacked it). + const stderr = await Promise.race([ + stderrPromise, + (async () => { + await procExited; + await new Promise((r) => setTimeout(r, 5_000)); + return ''; + })(), + ]); + const exitCode = await procExited; clearTimeout(timeoutId); const durationMs = Date.now() - startTime; diff --git a/test/helpers/session-runner.ts b/test/helpers/session-runner.ts index f433332f5..34e62b325 100644 --- a/test/helpers/session-runner.ts +++ b/test/helpers/session-runner.ts @@ -9,8 +9,11 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; +import { spawn } from 'child_process'; +import { Readable } from 'node:stream'; import { getProjectEvalDir } from './eval-store'; import { hermeticChildEnv, isHermeticEnabled } from './hermetic-env'; +import { killProcessGroup } from '../../scripts/test-strict-output'; const GSTACK_DEV_DIR = path.join(os.homedir(), '.gstack-dev'); const HEARTBEAT_PATH = path.join(GSTACK_DEV_DIR, 'e2e-live.json'); // heartbeat stays global @@ -174,8 +177,13 @@ export async function runSkillTest(options: { if (isHermeticEnabled()) args.push('--strict-mcp-config'); // Spawn claude directly with array-form args (no shell interpolation). - // Prompt is piped via stdin using a Blob to avoid temp files and shell escaping. - const proc = Bun.spawn(['claude', ...args], { + // node:child_process spawn (not Bun.spawn): `detached` puts the child in + // its OWN process group, so the timeout handler can killpg the whole tree. + // Bun.spawn has no detached option, and its bare proc.kill() signalled only + // claude itself — tool subprocesses claude spawned survived as orphans + // burning shared API rate for the rest of the shard's lifetime. + // Prompt is piped via stdin to avoid temp files and shell escaping. + const proc = spawn('claude', args, { cwd: workingDirectory, // Hermetic by default (see test/helpers/hermetic-env.ts): operator // session context (CONDUCTOR_*, CLAUDECODE, ~/.claude config, ~/.gstack) @@ -185,9 +193,17 @@ export async function runSkillTest(options: { // suite exercising the INTERACTIVE prose-fallback path opts out by passing // `env: { GSTACK_HEADLESS: '' }` — extraEnv wins because it spreads last. env: hermeticChildEnv({ GSTACK_HEADLESS: '1', ...extraEnv }), - stdin: new Blob([prompt]), - stdout: 'pipe', - stderr: 'pipe', + stdio: ['pipe', 'pipe', 'pipe'], + detached: process.platform !== 'win32', + }); + proc.stdin!.on('error', () => { /* child died before reading the prompt — exit handling reports it */ }); + proc.stdin!.write(prompt); + proc.stdin!.end(); + const stdoutWeb = Readable.toWeb(proc.stdout!) as ReadableStream; + const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream; + const procExited: Promise = new Promise((resolve) => { + proc.on('close', (code) => resolve(code ?? 1)); + proc.on('error', () => resolve(1)); }); // Race against timeout @@ -197,13 +213,14 @@ export async function runSkillTest(options: { const timeoutId = setTimeout(() => { timedOut = true; - proc.kill(); - // proc.kill() signals claude itself (direct spawn, no shell wrapper), - // but tool subprocesses claude spawned can survive as orphans that - // inherited our stdout/stderr pipes, so without cancel() the read loop - // below blocks until the orphan finally exits (observed: a 600s timeout - // stretching past 1400s and tripping bun's per-test timeout instead of - // returning a result). + // Group SIGKILL (mirrors runShardChild): claude AND every tool + // subprocess it spawned die together — a bare proc.kill() left orphans + // that inherited our stdout/stderr pipes and kept the API burning + // (observed: a 600s timeout stretching past 1400s while an orphan held + // the pipes open). + killProcessGroup(proc, 'SIGKILL'); + // Belt and braces with the group kill: even if an orphan survives (EPERM + // fallback path), cancel() unblocks the read loop below. reader.cancel().catch(() => { /* stream already closed */ }); }, timeout); @@ -214,9 +231,9 @@ export async function runSkillTest(options: { let firstResponseMs = 0; let lastToolTime = 0; let maxInterTurnMs = 0; - const stderrPromise = new Response(proc.stderr).text(); + const stderrPromise = new Response(stderrWeb).text(); - const reader = proc.stdout.getReader(); + const reader = stdoutWeb.getReader(); const decoder = new TextDecoder(); let buf = ''; @@ -304,12 +321,12 @@ export async function runSkillTest(options: { stderr = await Promise.race([ stderrPromise, (async () => { - await proc.exited; + await procExited; await new Promise((r) => setTimeout(r, 5_000)); return ''; })(), ]); - const exitCode = await proc.exited; + const exitCode = await procExited; clearTimeout(timeoutId); if (timedOut) { diff --git a/test/session-runner-groupkill.test.ts b/test/session-runner-groupkill.test.ts new file mode 100644 index 000000000..eb7faf3be --- /dev/null +++ b/test/session-runner-groupkill.test.ts @@ -0,0 +1,103 @@ +/** + * Group-kill regression pin for the provider session runners (F7 in the + * test-infra audit): a timed-out `claude -p` used to get a bare proc.kill() + * — the direct child died but tool subprocesses it had spawned survived as + * orphans holding our pipes open and burning shared API rate (observed: a + * 600s timeout stretching past 1400s; a stalled legacy run once burned a + * core for 15 hours). The fix: node:child_process spawn with `detached` + * (child leads its own process group) + killProcessGroup(SIGKILL) in the + * timeout handler, mirroring runShardChild's proven pattern. + * + * The behavioral test drives the REAL runSkillTest against a fake `claude` + * shim (PATH override — hermeticChildEnv allowlists PATH through) that + * spawns a grandchild and never exits: the run must classify as timeout + * within its budget AND leave neither shim nor grandchild alive. + * + * Windows note: the shim is a '/bin/bash' shebang script, which the free + * runner's Windows curation auto-excludes (CreateProcess cannot exec + * shebangs) — this literal mention is what trips the content scan. + */ +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { spawnSync } from 'child_process'; +import { runSkillTest } from './helpers/session-runner'; + +const ROOT = path.resolve(import.meta.dir, '..'); + +function aliveWithArg(marker: string): boolean { + const result = spawnSync('pgrep', ['-f', marker], { stdio: 'pipe', timeout: 5_000 }); + return result.status === 0; +} + +describe('session-runner timeout kills the whole process group', () => { + test('fake claude + its grandchild are both dead after a timeout', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'groupkill-')); + const shimDir = path.join(dir, 'bin'); + fs.mkdirSync(shimDir); + // Unique-ish sleep durations double as pgrep markers: they appear only + // in the shim's children's argv, never in this test process's cmdline. + const shim = [ + '#!/bin/bash', + 'sleep 6041 &', // the orphan-candidate grandchild + 'exec sleep 6042', // the shim itself, wedged forever, no NDJSON + ].join('\n'); + fs.writeFileSync(path.join(shimDir, 'claude'), `${shim}\n`, { mode: 0o755 }); + + const realPath = process.env.PATH; + process.env.PATH = `${shimDir}:${realPath}`; + try { + const started = Date.now(); + const result = await runSkillTest({ + prompt: 'irrelevant — the shim never reads it', + workingDirectory: dir, + maxTurns: 1, + allowedTools: ['Bash'], + timeout: 3_000, + testName: 'groupkill-probe', + }); + const wall = Date.now() - started; + + expect(result.exitReason).toBe('timeout'); + // The old bug's signature was the drain blocking long past the budget + // (600s -> 1400s). Generous 10x bound: timeout 3s + the 5s stderr + // grace race must return promptly once the group is dead. + expect(wall).toBeLessThan(30_000); + + // The kill is SIGKILL on the GROUP: give the OS a beat to reap, then + // require both the wedged shim and its grandchild gone. + await new Promise((r) => setTimeout(r, 1_000)); + expect(aliveWithArg('sleep 6042'), 'the fake claude itself survived the timeout kill').toBe(false); + expect(aliveWithArg('sleep 6041'), 'the grandchild ORPHANED — group kill regressed to a direct-child kill').toBe(false); + } finally { + process.env.PATH = realPath; + // Belt and braces: never leak the markers into later tests even on + // assertion failure. + spawnSync('pkill', ['-f', 'sleep 604[12]'], { stdio: 'ignore', timeout: 5_000 }); + fs.rmSync(dir, { recursive: true, force: true }); + } + }, 60_000); +}); + +describe('all three provider runners carry the group-kill wiring', () => { + // Source pin, not behavior: codex/gemini need their real binaries for a + // behavioral run, but the kill wiring is identical code — a runner that + // drops `detached` or reverts to a bare kill() re-opens the orphan class. + const runners = [ + 'test/helpers/session-runner.ts', + 'test/helpers/codex-session-runner.ts', + 'test/helpers/gemini-session-runner.ts', + ]; + for (const rel of runners) { + test(`${path.basename(rel)}: detached spawn + killProcessGroup, no bare timeout kill`, () => { + const source = fs.readFileSync(path.join(ROOT, rel), 'utf-8'); + expect(source).toContain("detached: process.platform !== 'win32'"); + expect(source).toContain('killProcessGroup(proc'); + expect(source, `${rel} reverted to Bun.spawn for the provider child — detached group-kill is impossible there`) + .not.toMatch(/Bun\.spawn\(\[['"](?:claude|codex|gemini)['"]/); + expect(source, `${rel} has a bare proc.kill() in a timeout handler`) + .not.toMatch(/timedOut = true;\s*\n\s*proc\.kill\(\)/); + }); + } +});