From 513111a1665181081298faaae6912b310c90fe48 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 31 Aug 2026 05:38:26 +0000 Subject: [PATCH] fix: adversarial-review runtime fixes across the telemetry + kill paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - session-runner: exit-labeling keys off 'exit', not 'close' — an orphan holding the pipes could relabel a REAL exit (auth failure) as 'timeout_startup' availability noise; the kill path still always group-kills and cancels the reader (labeling and unblocking are separate concerns). Work phase arms on a flag, not firstResponseMs===0 (a same-ms first byte left the startup timer live all run). The CI startup grace is now a real FLOOR (Math.max), matching its name and pinning test. - gstack-detach: pgid captured AT SPAWN (== child pid under start_new_session) — resolving it after the grace raised ESRCH once the leader died on SIGTERM, orphaning TERM-immune grandchildren forever. - test-free-shards: ledger entries carry branch + git_sha (rev-parse split: '--abbrev-ref HEAD HEAD' printed the branch twice and recorded it as the sha); local ledger default is per-PROJECT, not the machine-global tmpdir. - eval-flake-rank: per-LINE ledger parse (one torn JSONL line vanished the whole series), 60-day recency bound (transcript-bearing files are MBs), shared isFinalizedEvalResultFile predicate (the artifact-taxonomy rule lived in three places); eval-store exports the predicate and finalize stops computing flakyRetries twice; paid-shards cleanup uses async rm (a SIGKILLed shard's git-workspace teardown blocked every sibling's stream classification on the parent event loop). Co-Authored-By: Claude Fable 5 --- bin/gstack-detach | 13 +++++++++--- scripts/eval-flake-rank.ts | 31 ++++++++++++++++++++------- scripts/test-free-shards.ts | 27 +++++++++++++++++++++++- scripts/test-paid-shards.ts | 10 ++++++--- test/helpers/eval-store.ts | 18 +++++++++++++++- test/helpers/session-runner.ts | 38 ++++++++++++++++++++++++++++------ 6 files changed, 116 insertions(+), 21 deletions(-) diff --git a/bin/gstack-detach b/bin/gstack-detach index 938a17e8c..1f27f9246 100755 --- a/bin/gstack-detach +++ b/bin/gstack-detach @@ -96,13 +96,19 @@ def child_run(args, log): proc = subprocess.Popen( cmd, stdout=f, stderr=subprocess.STDOUT, stdin=subprocess.DEVNULL, start_new_session=True ) + # Capture the PGID AT SPAWN (== proc.pid: start_new_session makes the + # child a session/group leader). Resolving it later via + # os.getpgid(proc.pid) raises ESRCH once the leader exits — a leader + # that died on the SIGTERM while a TERM-immune grandchild survived + # left that grandchild alive forever (codex adversarial finding). + pgid = proc.pid if args.timeout and args.timeout > 0: try: code = proc.wait(timeout=args.timeout) except subprocess.TimeoutExpired: log_line(log, f"### gstack-detach WATCHDOG fired after {args.timeout}s — killing ### {_now()}") try: - os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + os.killpg(pgid, signal.SIGTERM) except Exception: pass time.sleep(5) @@ -110,9 +116,10 @@ def child_run(args, log): # 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. + # honored the SIGTERM. Uses the SAVED pgid so a dead leader + # cannot orphan its group. try: - os.killpg(os.getpgid(proc.pid), signal.SIGKILL) + os.killpg(pgid, signal.SIGKILL) except Exception: try: proc.kill() diff --git a/scripts/eval-flake-rank.ts b/scripts/eval-flake-rank.ts index 0885124ad..b6698ae48 100644 --- a/scripts/eval-flake-rank.ts +++ b/scripts/eval-flake-rank.ts @@ -21,7 +21,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; -import { getProjectEvalDir, isPartialEval, type EvalResult } from '../test/helpers/eval-store'; +import { getProjectEvalDir, isPartialEval, isFinalizedEvalResultFile, type EvalResult } from '../test/helpers/eval-store'; import { flakeLedgerPath, type FlakeLedgerEntry } from './test-free-shards'; interface TestSeries { @@ -73,21 +73,36 @@ export function aggregate(evalFiles: string[]): Map { return series; } -export function collectEvalFiles(dir: string): string[] { +export function collectEvalFiles(dir: string, sinceDays = 60): string[] { if (!fs.existsSync(dir)) return []; + const cutoff = Date.now() - sinceDays * 86_400_000; const out: string[] = []; for (const name of fs.readdirSync(dir, { recursive: true }) as string[]) { - if (!name.endsWith('.json') || path.basename(name).startsWith('_partial')) continue; - out.push(path.join(dir, name)); + if (!isFinalizedEvalResultFile(name)) continue; + const full = path.join(dir, name); + try { + // Recency bound (review finding): E2E results embed full transcripts + // (MBs each) and the scan is otherwise unbounded over all-time history. + if (fs.statSync(full).mtimeMs < cutoff) continue; + } catch { continue; } + out.push(full); } return out; } function readFreeLedger(): FlakeLedgerEntry[] { + // Per-LINE parse: one malformed JSONL line (torn write, manual edit) must + // drop that line, never vanish the whole series (codex adversarial finding). + let raw: string; try { - return fs.readFileSync(flakeLedgerPath(), 'utf-8') - .split('\n').filter(Boolean).map((l) => JSON.parse(l)); + raw = fs.readFileSync(flakeLedgerPath(), 'utf-8'); } catch { return []; } + const out: FlakeLedgerEntry[] = []; + for (const line of raw.split('\n')) { + if (!line.trim()) continue; + try { out.push(JSON.parse(line)); } catch { /* torn line — skip */ } + } + return out; } if (import.meta.main) { @@ -95,8 +110,10 @@ if (import.meta.main) { const dirFlag = argv.indexOf('--dir'); const dir = dirFlag !== -1 ? argv[dirFlag + 1] : getProjectEvalDir(); const asJson = argv.includes('--json'); + const sinceFlag = argv.indexOf('--since-days'); + const sinceDays = sinceFlag !== -1 ? Number(argv[sinceFlag + 1]) || 60 : 60; - const files = collectEvalFiles(dir); + const files = collectEvalFiles(dir, sinceDays); const series = [...aggregate(files).values()] .sort((a, b) => b.retriedPasses - a.retriedPasses || (b.fails / b.runs) - (a.fails / a.runs)); const ledger = readFreeLedger(); diff --git a/scripts/test-free-shards.ts b/scripts/test-free-shards.ts index 9b010405e..06aa2c215 100755 --- a/scripts/test-free-shards.ts +++ b/scripts/test-free-shards.ts @@ -1037,10 +1037,28 @@ export interface FlakeLedgerEntry { file: string; /** Shard the original failure surfaced in, when attributable. */ shard?: number; + /** Code-state attribution (review finding): without branch/sha the series + * can't tie an entry to the state that produced it, and the WS16 + * promotion evidence needs exactly that. */ + branch?: string; + git_sha?: string; } export function flakeLedgerPath(env: NodeJS.ProcessEnv = process.env): string { - return env.GSTACK_FLAKE_LEDGER || path.join(os.tmpdir(), 'gstack-flake-ledger.jsonl'); + if (env.GSTACK_FLAKE_LEDGER) return env.GSTACK_FLAKE_LEDGER; + // Local default: per-PROJECT, not the machine-global tmpdir — sibling + // Conductor worktrees of DIFFERENT repos must not interleave into one + // series (review finding). CI always sets GSTACK_FLAKE_LEDGER explicitly. + try { + const slug = spawnSync('bash', ['-c', '~/.claude/skills/gstack/bin/gstack-slug 2>/dev/null'], { stdio: 'pipe', timeout: 3000 }) + .stdout?.toString().match(/^SLUG=(.+)$/m)?.[1]; + if (slug) { + const dir = path.join(os.homedir(), '.gstack', 'projects', slug); + fs.mkdirSync(dir, { recursive: true }); + return path.join(dir, 'flake-ledger.jsonl'); + } + } catch { /* fall through */ } + return path.join(os.tmpdir(), 'gstack-flake-ledger.jsonl'); } export function appendFlakeLedger( @@ -1552,6 +1570,11 @@ async function main(): Promise { // Durable record (WS1): console lines vanish with the scrollback; the // ledger makes repeat offenders rankable across runs (eval:flake-rank). const ts = new Date().toISOString(); + // Two separate calls: `rev-parse --abbrev-ref HEAD HEAD` abbreviates + // BOTH revs, printing the branch twice — git_sha recorded the branch + // name (codex adversarial finding). + const ledgerBranch = (spawnSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 5000 }).stdout ?? '').trim(); + const ledgerSha = (spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 5000 }).stdout ?? '').trim(); appendFlakeLedger( flakyFiles.map((file) => ({ ts, @@ -1559,6 +1582,8 @@ async function main(): Promise { kind: 'flaky-pass' as const, file, shard: outcomes.find((o) => o.failingFiles.includes(file))?.shard, + ...(ledgerBranch ? { branch: ledgerBranch } : {}), + ...(ledgerSha ? { git_sha: ledgerSha.slice(0, 12) } : {}), })), flakeLedgerPath(), ); diff --git a/scripts/test-paid-shards.ts b/scripts/test-paid-shards.ts index 8a2874e33..cd274743f 100644 --- a/scripts/test-paid-shards.ts +++ b/scripts/test-paid-shards.ts @@ -64,7 +64,7 @@ import { } from './test-strict-output'; import { PAID_TEST_GLOBS, isPaidTestFile } from '../test/helpers/paid-test-set'; import { PERIODIC_CI_EXCLUDE } from '../test/helpers/periodic-exclude-data'; -import { getProjectEvalDir, getClaudeCliVersion } from '../test/helpers/eval-store'; +import { getProjectEvalDir, getClaudeCliVersion, isFinalizedEvalResultFile } from '../test/helpers/eval-store'; import { preflightAnthropicApi } from '../test/helpers/anthropic-preflight'; import { detectBaseBranch, @@ -575,7 +575,11 @@ export async function runPaidShard( // Close the spool even when the spawn itself failed. await new Promise((resolve) => logStream.end(() => resolve())); try { - fs.rmSync(stateDir, { recursive: true, force: true }); + // async rm: a SIGKILLed shard can leave a full git workspace + Chromium + // profile here; a synchronous recursive delete on the parent's event + // loop would stall every sibling shard's stream classification and + // wall timers for seconds (review finding). + await fs.promises.rm(stateDir, { recursive: true, force: true }); } catch { // Best-effort: a locked file must not turn a real verdict into an // exception (same posture as the free runner's cleanup). @@ -1035,7 +1039,7 @@ async function main(): Promise { // Source: the finalized eval-store JSONs inside the slice artifacts. const flaky: Array<{ name: string; attempts: number; file: string }> = []; for (const name of fs.readdirSync(options.reportDir, { recursive: true }) as string[]) { - if (!/\.json$/.test(name) || /manifest\.json$|slice-\d+\.json$|^_partial|\/_partial/.test(name)) continue; + if (!isFinalizedEvalResultFile(name)) continue; try { const parsed = JSON.parse(fs.readFileSync(path.join(options.reportDir, name), 'utf-8')) as { flaky_retries?: Array<{ name: string; attempts: number }> }; for (const f of parsed.flaky_retries ?? []) flaky.push({ ...f, file: name }); diff --git a/test/helpers/eval-store.ts b/test/helpers/eval-store.ts index 1ca3edbf0..4542cbbd3 100644 --- a/test/helpers/eval-store.ts +++ b/test/helpers/eval-store.ts @@ -192,6 +192,21 @@ export function isPartialEval(data: unknown, filename: string): boolean { return Boolean((data as { _partial?: unknown } | null)?._partial); } +/** + * Is this path a FINALIZED eval-store result file? Single owner of the + * filename taxonomy (manifest.json / slice-N.json are runner artifacts, + * _partial* are in-progress accumulators) — the paid runner's report mode + * and eval-flake-rank both consume this instead of re-encoding the rule + * (review finding: the rule lived in three places). + */ +export function isFinalizedEvalResultFile(relPath: string): boolean { + const base = path.basename(relPath); + if (!base.endsWith('.json')) return false; + if (base === 'manifest.json' || /^slice-\d+\.json$/.test(base)) return false; + if (base.startsWith('_partial')) return false; + return true; +} + /** * List eval JSON files in `evalDir` plus one level of `/shards//` * subdirectories (where the sharded paid runner points each shard's collector). @@ -900,6 +915,7 @@ export class EvalCollector { const totalDuration = this.tests.reduce((s, t) => s + t.duration_ms, 0); const passed = this.tests.filter(t => t.passed).length; + const flaky = this.flakyRetries(); const result: EvalResult = { schema_version: SCHEMA_VERSION, version, @@ -917,7 +933,7 @@ export class EvalCollector { wall_clock_ms: Date.now() - this.createdAt, tests: this.tests, ...(this.shard ? { shard: this.shard } : {}), - ...(this.flakyRetries().length > 0 ? { flaky_retries: this.flakyRetries() } : {}), + ...(flaky.length > 0 ? { flaky_retries: flaky } : {}), }; // Write eval file diff --git a/test/helpers/session-runner.ts b/test/helpers/session-runner.ts index 58071d163..2bbababea 100644 --- a/test/helpers/session-runner.ts +++ b/test/helpers/session-runner.ts @@ -162,8 +162,13 @@ export async function runSkillTest(options: { runId, env: extraEnv, } = options; + // The CI floor is a FLOOR, not a default: an explicit startupGraceMs below + // 300s in CI would re-open the queueing-becomes-false-red hole the floor + // exists for (review finding — the name promised a clamp the code lacked). + // Local runs honor the caller verbatim; timeout still caps everything. + const requestedGrace = options.startupGraceMs ?? (process.env.CI ? STARTUP_GRACE_CI_FLOOR_MS : STARTUP_GRACE_MS); const startupGraceMs = Math.min( - options.startupGraceMs ?? (process.env.CI ? STARTUP_GRACE_CI_FLOOR_MS : STARTUP_GRACE_MS), + process.env.CI ? Math.max(requestedGrace, STARTUP_GRACE_CI_FLOOR_MS) : requestedGrace, timeout, ); const model = options.model ?? process.env.EVALS_MODEL ?? 'claude-sonnet-4-6'; @@ -222,9 +227,17 @@ export async function runSkillTest(options: { proc.stdin!.end(); const stdoutWeb = Readable.toWeb(proc.stdout!) as ReadableStream; const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream; + // 'exit' vs 'close' matters here: 'close' waits for stdout/stderr to + // drain, which an orphaned grandchild can hold open long after claude + // itself died with a REAL exit code — labeling must key off 'exit' or an + // auth failure gets triaged as 'timeout_startup' availability noise + // (claude adversarial finding). procExited stays 'close'-based (streams + // complete) for the drain race below. + let childExited = false; const procExited: Promise = new Promise((resolve) => { - proc.on('close', (code) => resolve(code ?? 1)); - proc.on('error', () => resolve(1)); + proc.on('exit', () => { childExited = true; }); + proc.on('close', (code) => { childExited = true; resolve(code ?? 1); }); + proc.on('error', () => { childExited = true; resolve(1); }); }); // Two-phase timeout. Phase 1 (startup): no NDJSON byte yet — a shorter @@ -240,8 +253,16 @@ export async function runSkillTest(options: { let phaseTimer: ReturnType; const killRun = (startupPhase: boolean): void => { - timedOut = true; - timedOutInStartup = startupPhase; + // Labeling and unblocking are SEPARATE concerns: a timer firing after + // the child already exited must not relabel a real exit (auth error, + // crash) as a timeout — but it must STILL group-kill and cancel the + // reader, or an orphan holding the pipes re-creates the exact + // blocked-drain hang this runner fixed (an early `return` here was the + // bug the adversarial pass caught in the first version of this guard). + if (!childExited) { + timedOut = true; + timedOutInStartup = startupPhase; + } // 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 @@ -264,6 +285,7 @@ export async function runSkillTest(options: { let liveTurnCount = 0; let liveToolCount = 0; let firstResponseMs = 0; + let workPhaseArmed = false; let lastToolTime = 0; let maxInterTurnMs = 0; const stderrPromise = new Response(stderrWeb).text(); @@ -284,7 +306,11 @@ export async function runSkillTest(options: { collectedLines.push(line); // Track time to first NDJSON line (measures latency from spawn to first Claude response) - if (firstResponseMs === 0) { + if (!workPhaseArmed) { + // Flag, not `firstResponseMs === 0`: a first line landing in the + // same millisecond as spawn would read as "not yet seen" and leave + // the startup timer live for the whole run (claude adversarial). + workPhaseArmed = true; firstResponseMs = Date.now() - startTime; // First byte: startup phase over — arm the work phase for the // REMAINING budget (total wall stays <= timeout).