mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix: adversarial-review runtime fixes across the telemetry + kill paths
- 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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
5b04f05ba4
commit
513111a166
+10
-3
@@ -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()
|
||||
|
||||
@@ -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<string, TestSeries> {
|
||||
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();
|
||||
|
||||
@@ -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<number> {
|
||||
// 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<number> {
|
||||
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(),
|
||||
);
|
||||
|
||||
@@ -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<void>((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<number> {
|
||||
// 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 });
|
||||
|
||||
@@ -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 `<evalDir>/shards/<slug>/`
|
||||
* 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
|
||||
|
||||
@@ -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<Uint8Array>;
|
||||
const stderrWeb = Readable.toWeb(proc.stderr!) as ReadableStream<Uint8Array>;
|
||||
// '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<number> = 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<typeof setTimeout>;
|
||||
|
||||
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).
|
||||
|
||||
Reference in New Issue
Block a user