mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-11 07:29:00 +02:00
fix: provider-runner timeouts kill the whole process GROUP; codex/gemini inherit the orphan-drain hardening
All three provider runners (claude/codex/gemini) killed only the direct child on timeout: tool subprocesses the CLI spawned survived as orphans holding our pipes open and burning shared API rate (observed: a 600s timeout stretching past 1400s; a stalled run once burned a core for 15 hours). gstack-detach's watchdog had the same shape one level up — killpg SIGTERM, 5s grace, then a direct-child proc.kill() that orphaned grandchildren. Fix: spawn provider children via node:child_process with detached (own process group) and killProcessGroup(SIGKILL) in the timeout handler — runShardChild's proven pattern, EPERM/ESRCH fallbacks included. The codex and gemini copies also gain the reader.cancel() + stderr Promise.race hardening only the claude copy had (they still carried the blocked-drain hang it fixed). gstack-detach's watchdog now group-SIGKILLs after the grace. Regression net: test/session-runner-groupkill.test.ts drives the REAL runSkillTest against a fake claude shim (PATH override) that spawns a grandchild and wedges — the run must classify timeout within budget and leave neither shim nor grandchild alive — plus source pins on all three runners (detached + killProcessGroup, no bare timeout kill, no Bun.spawn reversion). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
07c2452e6d
commit
2e53b18670
@@ -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<Uint8Array>;
|
||||
const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream<Uint8Array>;
|
||||
const procExited: Promise<number> = 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;
|
||||
|
||||
@@ -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<Uint8Array>;
|
||||
const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream<Uint8Array>;
|
||||
const procExited: Promise<number> = 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;
|
||||
|
||||
@@ -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<Uint8Array>;
|
||||
const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream<Uint8Array>;
|
||||
const procExited: Promise<number> = 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) {
|
||||
|
||||
Reference in New Issue
Block a user