mirror of https://github.com/garrytan/gstack.git
fix(test-runner): cancellation terminates the run; win32 kills the whole tree
Installing SIGINT/SIGTERM forwarders suppresses Node's default terminate-on-signal, so a cancelled run killed the current child and kept LAUNCHING shards — observed as paid runs continuing to burn API spend after Ctrl-C (codex adversarial, repro'd ALIVE_AFTER_SIGTERM). The first signal now also schedules the parent's own exit after the children's SIGKILL grace, and both shard pools consult isTerminationRequested() before taking new work. On win32, killProcessGroup uses taskkill /T /F — detached:true creates no killable group there, and a bare child.kill orphaned every grandchild (ports, locks, and the inherited pipes that kept close from firing). Also: the tree-mutating serial shard prints dirty generated artifacts when it dies mid-regeneration, and --shard CI-matrix mode gets the same size-scaled wall deadline as full-suite mode.
This commit is contained in:
parent
84a93766f2
commit
9e72b4ac7f
|
|
@ -77,13 +77,14 @@
|
||||||
import * as fs from 'fs';
|
import * as fs from 'fs';
|
||||||
import * as os from 'os';
|
import * as os from 'os';
|
||||||
import * as path from 'path';
|
import * as path from 'path';
|
||||||
import { spawn } from 'child_process';
|
import { spawn, spawnSync } from 'child_process';
|
||||||
import { StringDecoder } from 'node:string_decoder';
|
import { StringDecoder } from 'node:string_decoder';
|
||||||
import { isPaidTestFile } from '../test/helpers/paid-test-set';
|
import { isPaidTestFile } from '../test/helpers/paid-test-set';
|
||||||
import {
|
import {
|
||||||
BunTestOutputClassifier,
|
BunTestOutputClassifier,
|
||||||
exactTestFileSelectors,
|
exactTestFileSelectors,
|
||||||
installChildSignalForwarding,
|
installChildSignalForwarding,
|
||||||
|
isTerminationRequested,
|
||||||
killProcessGroup,
|
killProcessGroup,
|
||||||
strictTestExitCode,
|
strictTestExitCode,
|
||||||
stripAnsiLine,
|
stripAnsiLine,
|
||||||
|
|
@ -1081,16 +1082,38 @@ async function main(): Promise<number> {
|
||||||
})),
|
})),
|
||||||
);
|
);
|
||||||
let worst = Math.max(...outcomes.map((o) => exitCodeFor(o.status)));
|
let worst = Math.max(...outcomes.map((o) => exitCodeFor(o.status)));
|
||||||
if (mutators.length > 0) {
|
// Cancellation stops the run: don't launch the serial tree-mutating shard
|
||||||
|
// after a SIGINT/SIGTERM already killed the parallel phase.
|
||||||
|
if (mutators.length > 0 && !isTerminationRequested()) {
|
||||||
const mutatorOutcome = await runFreeShard(mutators, totalShards, totalShards, {
|
const mutatorOutcome = await runFreeShard(mutators, totalShards, totalShards, {
|
||||||
wallTimeoutMs: shardTimeout(mutators.length),
|
wallTimeoutMs: shardTimeout(mutators.length),
|
||||||
verbose: options.verbose,
|
verbose: options.verbose,
|
||||||
});
|
});
|
||||||
worst = Math.max(worst, exitCodeFor(mutatorOutcome.status));
|
worst = Math.max(worst, exitCodeFor(mutatorOutcome.status));
|
||||||
|
if (mutatorOutcome.status !== 'passed') {
|
||||||
|
// Mutator safety rests on each test restoring default state itself; a
|
||||||
|
// SIGKILL at the wall deadline (or a mid-regeneration crash) defeats
|
||||||
|
// that by construction. Say so, loudly, before someone commits
|
||||||
|
// regenerated SKILL.md / .agents artifacts by accident.
|
||||||
|
const dirty = spawnSyncGitStatusGenerated();
|
||||||
|
if (dirty.length > 0) {
|
||||||
|
console.error('[test:free] ⚠ tree-mutating shard did not finish cleanly — generated artifacts may be mid-regeneration:');
|
||||||
|
for (const line of dirty.slice(0, 20)) console.error(`[test:free] ${line}`);
|
||||||
|
console.error('[test:free] restore with: bun run gen:skill-docs (or git checkout -- <paths>)');
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return worst;
|
return worst;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Dirty generated artifacts (SKILL.md / host outputs) after a failed mutator shard. */
|
||||||
|
function spawnSyncGitStatusGenerated(): string[] {
|
||||||
|
const result = spawnSync('git', ['status', '--porcelain'], { cwd: ROOT, encoding: 'utf8' });
|
||||||
|
if (result.status !== 0 || !result.stdout) return [];
|
||||||
|
return result.stdout.split('\n').filter((line) =>
|
||||||
|
/SKILL\.md$/.test(line) || line.includes('.agents/') || line.includes('.factory/'));
|
||||||
|
}
|
||||||
|
|
||||||
if (import.meta.main) {
|
if (import.meta.main) {
|
||||||
try {
|
try {
|
||||||
process.exitCode = await main();
|
process.exitCode = await main();
|
||||||
|
|
|
||||||
|
|
@ -59,6 +59,7 @@ import {
|
||||||
exactTestFileSelectors,
|
exactTestFileSelectors,
|
||||||
forwardAndClassify,
|
forwardAndClassify,
|
||||||
installChildSignalForwarding,
|
installChildSignalForwarding,
|
||||||
|
isTerminationRequested,
|
||||||
killProcessGroup,
|
killProcessGroup,
|
||||||
strictTestExitCode,
|
strictTestExitCode,
|
||||||
} from './test-strict-output';
|
} from './test-strict-output';
|
||||||
|
|
@ -517,6 +518,10 @@ export async function runPaidShards(
|
||||||
let next = 0;
|
let next = 0;
|
||||||
const worker = async (): Promise<void> => {
|
const worker = async (): Promise<void> => {
|
||||||
while (true) {
|
while (true) {
|
||||||
|
// Cancellation (SIGINT/SIGTERM) must stop the RUN: the signal
|
||||||
|
// forwarders kill in-flight children, and this guard stops the pool
|
||||||
|
// from launching replacement shards that would keep burning API spend.
|
||||||
|
if (isTerminationRequested()) return;
|
||||||
const index = next;
|
const index = next;
|
||||||
next += 1;
|
next += 1;
|
||||||
if (index >= shards.length) return;
|
if (index >= shards.length) return;
|
||||||
|
|
|
||||||
|
|
@ -51,25 +51,69 @@ const DEFAULT_TERMINATION_TIMER: TerminationTimerApi = {
|
||||||
cancel: (handle) => clearTimeout(handle as ReturnType<typeof setTimeout>),
|
cancel: (handle) => clearTimeout(handle as ReturnType<typeof setTimeout>),
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Per-source termination bookkeeping, shared across every forwarder bound to
|
||||||
|
* the same source. Installing ANY signal listener suppresses Node's default
|
||||||
|
* terminate-on-SIGINT/SIGTERM, so without this the parent runner survived
|
||||||
|
* cancellation: it killed the current child, then kept LAUNCHING new shards
|
||||||
|
* (observed: paid runs continuing to burn API spend after Ctrl-C). The first
|
||||||
|
* signal now also schedules the parent's own exit after the children's
|
||||||
|
* SIGKILL grace, and runners consult isTerminationRequested() before
|
||||||
|
* launching more work.
|
||||||
|
*/
|
||||||
|
interface SourceTerminationState {
|
||||||
|
requested: boolean;
|
||||||
|
exitScheduled: boolean;
|
||||||
|
}
|
||||||
|
const SOURCE_TERMINATION_STATE = new WeakMap<TerminationSignalSource, SourceTerminationState>();
|
||||||
|
function terminationStateFor(source: TerminationSignalSource): SourceTerminationState {
|
||||||
|
let state = SOURCE_TERMINATION_STATE.get(source);
|
||||||
|
if (!state) {
|
||||||
|
state = { requested: false, exitScheduled: false };
|
||||||
|
SOURCE_TERMINATION_STATE.set(source, state);
|
||||||
|
}
|
||||||
|
return state;
|
||||||
|
}
|
||||||
|
export function isTerminationRequested(source: TerminationSignalSource = process): boolean {
|
||||||
|
return SOURCE_TERMINATION_STATE.get(source)?.requested ?? false;
|
||||||
|
}
|
||||||
|
const signalExitCode = (signal: ForwardedTerminationSignal): number =>
|
||||||
|
128 + (signal === 'SIGINT' ? 2 : 15);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Bind one active child to the parent's termination lifecycle. SIGINT and
|
* Bind one active child to the parent's termination lifecycle. SIGINT and
|
||||||
* SIGTERM get a grace period so Bun can clean up; a repeated signal, timeout,
|
* SIGTERM get a grace period so Bun can clean up; a repeated signal, timeout,
|
||||||
* or synchronous parent exit uses SIGKILL so the child cannot be orphaned.
|
* or synchronous parent exit uses SIGKILL so the child cannot be orphaned.
|
||||||
|
* The parent itself exits shortly after the grace window (or immediately on
|
||||||
|
* a repeated signal) — cancellation must terminate the RUN, not just the
|
||||||
|
* currently-running children.
|
||||||
*/
|
*/
|
||||||
export function installChildSignalForwarding(
|
export function installChildSignalForwarding(
|
||||||
child: Pick<ChildProcess, 'kill'>,
|
child: Pick<ChildProcess, 'kill'>,
|
||||||
source: TerminationSignalSource = process,
|
source: TerminationSignalSource = process,
|
||||||
timer: TerminationTimerApi = DEFAULT_TERMINATION_TIMER,
|
timer: TerminationTimerApi = DEFAULT_TERMINATION_TIMER,
|
||||||
graceMs = 5_000,
|
graceMs = 5_000,
|
||||||
|
exitImpl: (code: number) => void = (code) => process.exit(code),
|
||||||
): ChildSignalForwarding {
|
): ChildSignalForwarding {
|
||||||
let receivedSignal: ForwardedTerminationSignal | null = null;
|
let receivedSignal: ForwardedTerminationSignal | null = null;
|
||||||
let forceTimer: unknown = null;
|
let forceTimer: unknown = null;
|
||||||
let disposed = false;
|
let disposed = false;
|
||||||
|
|
||||||
|
const scheduleParentExit = (signal: ForwardedTerminationSignal, delayMs: number): void => {
|
||||||
|
const state = terminationStateFor(source);
|
||||||
|
state.requested = true;
|
||||||
|
if (state.exitScheduled) return;
|
||||||
|
state.exitScheduled = true;
|
||||||
|
// Never cancelled by dispose(): once cancellation is requested, the run
|
||||||
|
// is going down even if this particular shard finishes cleanly first.
|
||||||
|
timer.schedule(() => exitImpl(signalExitCode(signal)), delayMs);
|
||||||
|
};
|
||||||
|
|
||||||
const forward = (signal: ForwardedTerminationSignal): void => {
|
const forward = (signal: ForwardedTerminationSignal): void => {
|
||||||
if (disposed) return;
|
if (disposed) return;
|
||||||
if (receivedSignal !== null) {
|
if (receivedSignal !== null) {
|
||||||
child.kill('SIGKILL');
|
child.kill('SIGKILL');
|
||||||
|
scheduleParentExit(signal, 0);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
receivedSignal = signal;
|
receivedSignal = signal;
|
||||||
|
|
@ -78,6 +122,8 @@ export function installChildSignalForwarding(
|
||||||
forceTimer = null;
|
forceTimer = null;
|
||||||
child.kill('SIGKILL');
|
child.kill('SIGKILL');
|
||||||
}, graceMs);
|
}, graceMs);
|
||||||
|
// Exit AFTER the children's SIGKILL grace so the group kills land first.
|
||||||
|
scheduleParentExit(signal, graceMs + 1_000);
|
||||||
};
|
};
|
||||||
const onSigint = () => forward('SIGINT');
|
const onSigint = () => forward('SIGINT');
|
||||||
const onSigterm = () => forward('SIGTERM');
|
const onSigterm = () => forward('SIGTERM');
|
||||||
|
|
|
||||||
|
|
@ -8,7 +8,14 @@
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, expect, it } from 'bun:test';
|
import { describe, expect, it } from 'bun:test';
|
||||||
import { BunTestOutputClassifier, strictTestExitCode } from '../scripts/test-strict-output';
|
import {
|
||||||
|
BunTestOutputClassifier,
|
||||||
|
installChildSignalForwarding,
|
||||||
|
isTerminationRequested,
|
||||||
|
strictTestExitCode,
|
||||||
|
type TerminationSignalSource,
|
||||||
|
type TerminationTimerApi,
|
||||||
|
} from '../scripts/test-strict-output';
|
||||||
|
|
||||||
describe('strictTestExitCode', () => {
|
describe('strictTestExitCode', () => {
|
||||||
it('trusts a clean zero exit when the expected file count ran', () => {
|
it('trusts a clean zero exit when the expected file count ran', () => {
|
||||||
|
|
@ -83,3 +90,74 @@ describe('BunTestOutputClassifier', () => {
|
||||||
expect(strictTestExitCode(0, summary, 2)).toBe(0);
|
expect(strictTestExitCode(0, summary, 2)).toBe(0);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('installChildSignalForwarding — cancellation terminates the RUN', () => {
|
||||||
|
// Installing any SIGINT/SIGTERM listener suppresses Node's default
|
||||||
|
// terminate-on-signal. Pre-fix, the forwarder killed the current child and
|
||||||
|
// the parent LIVED ON — the paid worker pool kept launching API-burning
|
||||||
|
// shards after Ctrl-C. The parent must schedule its own exit and expose
|
||||||
|
// isTerminationRequested() so launch loops stop taking new work.
|
||||||
|
type Handler = () => void;
|
||||||
|
const makeFakes = () => {
|
||||||
|
const listeners = new Map<string, Handler[]>();
|
||||||
|
const source: TerminationSignalSource = {
|
||||||
|
on: (event, listener) => {
|
||||||
|
listeners.set(event, [...(listeners.get(event) ?? []), listener]);
|
||||||
|
},
|
||||||
|
off: (event, listener) => {
|
||||||
|
listeners.set(event, (listeners.get(event) ?? []).filter((l) => l !== listener));
|
||||||
|
},
|
||||||
|
};
|
||||||
|
const emit = (event: string) => (listeners.get(event) ?? []).forEach((l) => l());
|
||||||
|
const scheduled: Array<{ callback: () => void; delayMs: number; cancelled: boolean }> = [];
|
||||||
|
const timer: TerminationTimerApi = {
|
||||||
|
schedule: (callback, delayMs) => {
|
||||||
|
const handle = { callback, delayMs, cancelled: false };
|
||||||
|
scheduled.push(handle);
|
||||||
|
return handle;
|
||||||
|
},
|
||||||
|
cancel: (handle) => {
|
||||||
|
(handle as { cancelled: boolean }).cancelled = true;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
const kills: string[] = [];
|
||||||
|
const child = { kill: (sig?: unknown) => { kills.push(String(sig)); return true; } };
|
||||||
|
const exits: number[] = [];
|
||||||
|
return { source, emit, timer, scheduled, kills, child, exits, exit: (code: number) => { exits.push(code); } };
|
||||||
|
};
|
||||||
|
|
||||||
|
it('first signal kills the child, marks termination, and schedules parent exit after the grace', () => {
|
||||||
|
const f = makeFakes();
|
||||||
|
installChildSignalForwarding(f.child, f.source, f.timer, 5_000, f.exit);
|
||||||
|
expect(isTerminationRequested(f.source)).toBe(false);
|
||||||
|
f.emit('SIGTERM');
|
||||||
|
expect(f.kills).toEqual(['SIGTERM']);
|
||||||
|
expect(isTerminationRequested(f.source)).toBe(true);
|
||||||
|
// Two timers: child SIGKILL grace (5s) and parent exit (grace + 1s).
|
||||||
|
const delays = f.scheduled.map((s) => s.delayMs);
|
||||||
|
expect(delays).toContain(5_000);
|
||||||
|
expect(delays).toContain(6_000);
|
||||||
|
const parentExit = f.scheduled.find((s) => s.delayMs === 6_000)!;
|
||||||
|
parentExit.callback();
|
||||||
|
expect(f.exits).toEqual([143]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('parent exit fires even when the shard disposes cleanly first', () => {
|
||||||
|
const f = makeFakes();
|
||||||
|
const forwarding = installChildSignalForwarding(f.child, f.source, f.timer, 5_000, f.exit);
|
||||||
|
f.emit('SIGINT');
|
||||||
|
forwarding.dispose();
|
||||||
|
const parentExit = f.scheduled.find((s) => s.delayMs === 6_000)!;
|
||||||
|
expect(parentExit.cancelled).toBe(false);
|
||||||
|
parentExit.callback();
|
||||||
|
expect(f.exits).toEqual([130]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('one parent exit across many concurrent forwarders on the same source', () => {
|
||||||
|
const f = makeFakes();
|
||||||
|
installChildSignalForwarding(f.child, f.source, f.timer, 5_000, f.exit);
|
||||||
|
installChildSignalForwarding({ kill: () => true }, f.source, f.timer, 5_000, f.exit);
|
||||||
|
f.emit('SIGTERM');
|
||||||
|
expect(f.scheduled.filter((s) => s.delayMs === 6_000).length).toBe(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue