From 5c1402607fcd64509fb86d3ac03202939ed53f10 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Thu, 24 Sep 2026 21:11:31 +0000 Subject: [PATCH 1/7] fix(worker): reap deterministic station descendants on timeout and exit (#10, #17) runDeterministic spawned without `detached` and relied on Bun.spawn's native timeout, which kills only the immediate child. A backgrounded descendant survived a timeout (#10) and a normal or nonzero exit (#17), and one that inherited stdout/stderr kept the drains pending, so the runner could block past its deadline or indefinitely. The command now runs as its own process group (`detached: true`). Our own timer SIGKILLs the group at the deadline, and the group is SIGKILLed again after the leader exits and before the pipe drains are awaited, so output already written is still captured. `timedOut` now requires that our timer fired and the leader died of SIGKILL, so a post-exit group kill never marks a normal exit as timed out. The kill is shared with the harness runner via src/worker/process-group.ts. Tests: the deterministic conformance registration drops `knownLeak` and opts into new exit-0 and nonzero-exit scenarios (fixture `--exit ` mode, opt-in so harness adapters are unaffected). New tests cover descendants holding the output pipes, and one regression per execution path: the synchronous executor and the subprocess worker entry. docs/harness-containment.md now states that the deterministic runner passes the suite. Closes #10 Closes #17 Co-Authored-By: Claude Opus 5.5 --- docs/harness-containment.md | 11 +- .../sync-deterministic-failure.test.ts | 59 ++++++++ src/worker/containment-fixture.sh | 12 ++ src/worker/deterministic.test.ts | 139 +++++++++++++++-- src/worker/deterministic.ts | 86 +++++++---- .../harness-containment.conformance.test.ts | 8 +- src/worker/harness-containment.conformance.ts | 143 ++++++++++++++---- src/worker/harness-runner.ts | 23 ++- src/worker/process-group.ts | 24 +++ src/worker/worker-entry.test.ts | 54 +++++++ 10 files changed, 467 insertions(+), 92 deletions(-) create mode 100644 src/worker/process-group.ts diff --git a/docs/harness-containment.md b/docs/harness-containment.md index d8b5484..0b710aa 100644 --- a/docs/harness-containment.md +++ b/docs/harness-containment.md @@ -53,10 +53,13 @@ hold it together: backgrounds a grandchild, and must show that the grandchild's pid is gone and its sentinel file stops changing after the timeout. `src/worker/harness-containment-registry.test.ts` fails if an adapter ships without a - conformance call. The deterministic station runner is registered against the same suite - but does not pass it yet ([#10](https://github.com/theaiteam-dev/conduit/issues/10), - [#17](https://github.com/theaiteam-dev/conduit/issues/17)); that is a separate path from - harness stations, and its test is marked as a known failure until those are fixed. + conformance call. The deterministic station runner, a separate path from harness + stations, passes the same suite. It also runs the suite's exit scenarios: when the + station's command exits 0 or nonzero before its timeout, the runner kills the command's + process group before it returns, so a grandchild does not outlive a station that finished + on its own ([#10](https://github.com/theaiteam-dev/conduit/issues/10), + [#17](https://github.com/theaiteam-dev/conduit/issues/17)). A harness adapter kills the + group on timeout only. Deployment guidance additionally recommends running the container as a dedicated non-root user for agentic/harness flows. - **The ADR-0003 container wall.** [ADR-0003](../adr/0003-packaging-and-distribution.md)'s diff --git a/src/controller/sync-deterministic-failure.test.ts b/src/controller/sync-deterministic-failure.test.ts index 045cc6c..cffabe1 100644 --- a/src/controller/sync-deterministic-failure.test.ts +++ b/src/controller/sync-deterministic-failure.test.ts @@ -23,6 +23,10 @@ * build TERMINATE (the no-progress watchdog halts it, leaving the card * un-scrapped) instead of hanging — so this test fails fast (red) pre-fix rather * than wedging the suite. The fixed build scraps well before the watchdog. + * + * The second describe covers containment on the same path (#17): a station + * whose command backgrounds a grandchild and exits 0 must not leave that + * grandchild running once the card has advanced. */ import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; @@ -36,6 +40,12 @@ import type { FlowConfig } from '../types/kernel'; import { runExecutor } from './executor'; import type { RunEngineArgs } from '../cli/main'; import type { ModelAdapter } from '../worker/adapter'; +import { + CONTAINMENT_FIXTURE, + containmentFixtureExitArgs, + expectGrandchildReaped, + killRecordedGrandchild, +} from '../worker/harness-containment.conformance'; const io = { out: (_l: string) => {}, err: (_l: string) => {} }; @@ -120,6 +130,7 @@ afterEach(() => { db.close(); db = null; } + killRecordedGrandchild(projectDir); process.chdir(originalCwd); rmSync(projectDir, { recursive: true, force: true }); }); @@ -170,3 +181,51 @@ describe('WI-686 — synchronous-path deterministic failure routes to scrap', () expect(runs).toBeLessThanOrEqual(MAX_ATTEMPTS); }); }); + +describe('#17: synchronous-path deterministic station reaps its descendants on exit 0', () => { + it('advances the card to done and leaves no backgrounded grandchild running', async () => { + db = openDb(); + // The containment fixture backgrounds a grandchild that touches a sentinel, + // waits for its first touch, then exits 0. It writes into its cwd, which + // the executor sets to the project root. + const args = [CONTAINMENT_FIXTURE, ...containmentFixtureExitArgs(0)].map((a) => JSON.stringify(a)); + writeFileSync( + join(projectDir, 'flow.yaml'), + ` +flow: sync-deterministic-reap +project_root: . +flow_version: 1 +terminal_lanes: [done, scrap, hold] +defaults: + cap_policy: scrap +budgets: + per_card: { max_execution_attempts: 1 } +stations: + - id: boom + next: done + worker: + kind: deterministic + role: spawner + command: sh + args: [${args.join(', ')}] + wip: 1 + inputs: [] + outputs: [] +`, + ); + const loaded = loadFlow(join(projectDir, 'flow.yaml')); + if (!loaded.ok) throw new Error(`fixture flow invalid: ${JSON.stringify(loaded.errors)}`); + seedBoomCard(db); + + await runExecutor({ + db, + flow: loaded.flow, + now: () => Math.floor(Date.now() / 1000), + adapter: throwingModel, + io, + } as RunEngineArgs); + + expect(db.getCard(DEFAULT_RUN_ID, 'entry')?.lane).toBe('done'); + await expectGrandchildReaped(projectDir); + }, 20_000); +}); diff --git a/src/worker/containment-fixture.sh b/src/worker/containment-fixture.sh index ec1c54d..fe35901 100755 --- a/src/worker/containment-fixture.sh +++ b/src/worker/containment-fixture.sh @@ -10,6 +10,12 @@ # until killed. It prints nothing, so the invocation can only end at its # timeout. # +# Exit mode: `containment-fixture.sh --exit ` waits until the grandchild +# has touched the sentinel once, then exits with instead of blocking. +# The suite uses it for the normal-exit and nonzero-exit scenarios, where the +# invocation ends on its own and the grandchild must still not outlive it. +# Only spawn paths that pass the suite's `fixtureArgs` through use this mode. +# # The grandchild's stdio goes to /dev/null so it does not hold the spawn # path's stdout/stderr pipes open. A path that fails to reap it then returns # at its timeout and fails the suite's assertions, instead of hanging. @@ -25,4 +31,10 @@ export PATH done ) /dev/null 2>&1 & echo "$!" > containment.pid +if [ "$1" = "--exit" ]; then + while [ ! -f containment.sentinel ]; do + sleep 0.05 + done + exit "${2:-0}" +fi wait diff --git a/src/worker/deterministic.test.ts b/src/worker/deterministic.test.ts index 2bc2444..c9a8f49 100644 --- a/src/worker/deterministic.test.ts +++ b/src/worker/deterministic.test.ts @@ -25,7 +25,7 @@ * // MUST call checkCommandAllowed first and THROW (refuse) before spawning if denied. */ import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; -import { existsSync, mkdtempSync, rmSync, writeFileSync, chmodSync } from 'node:fs'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync, chmodSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { @@ -193,7 +193,7 @@ describe('runDeterministic — timeout enforcement (punch-list #8)', () => { // Code-review fix #1 — a SIGKILL that is NOT from our timeout (here: the child // kills itself well before the deadline) must NOT be mislabeled `timedOut`. - // The elapsed-time guard keeps the timeout diagnostic honest. The self-kill + // `timedOut` requires that our own timer fired, which keeps the diagnostic honest. The self-kill // lives in a SCRIPT FILE (not argv) so the Law-lite metacharacter scan — which // only inspects argv — admits it; the path is plain safe-charset. it('does NOT report timedOut for a SIGKILL that landed well before the deadline', async () => { @@ -291,22 +291,137 @@ describe('runDeterministic — env injection', () => { }); // --------------------------------------------------------------------------- -// Containment conformance (issue #27). runDeterministic is the other spawn -// path, and it does not reap descendants yet: Bun.spawn's native timeout kills -// only the immediate child, so the fixture's grandchild survives (#10 for the -// timeout, #17 for a normal exit). The reaping test is registered with -// `test.failing` until that is fixed. It goes red once the grandchild dies, -// and the fix should then drop `knownLeak`. +// Containment conformance (issues #27, #10, #17). runDeterministic spawns the +// command as its own process group and SIGKILLs the group on timeout and after +// a normal or nonzero exit, so the fixture's grandchild dies on every path. +// `reapsOnExit` adds the exit-0 and nonzero-exit scenarios to the timeout one. // --------------------------------------------------------------------------- +/** + * Test-local label for `result.timedOut === true`. DeterministicResult carries + * a boolean, not a classification code, so the suite compares against this. + */ +const DETERMINISTIC_TIMEOUT_LABEL = 'timedOut'; + describeContainmentConformance( 'deterministic', - async ({ projectRoot, fixture, timeoutMs }) => { + async ({ projectRoot, fixture, timeoutMs, fixtureArgs }) => { const result = await runDeterministic( - { command: fixture, args: [] }, + { command: fixture, args: fixtureArgs }, { allowlist: [fixture], cwd: projectRoot, timeoutMs }, ); - return result.timedOut === true ? 'timedOut' : undefined; + return result.timedOut === true ? DETERMINISTIC_TIMEOUT_LABEL : undefined; }, - { timeoutClass: 'timedOut', knownLeak: '#10 and #17' }, + { timeoutClass: DETERMINISTIC_TIMEOUT_LABEL, reapsOnExit: true }, ); + +// --------------------------------------------------------------------------- +// #10 / #17: a descendant that keeps the inherited stdout/stderr pipes open. +// The conformance fixture sends its grandchild's stdio to /dev/null; these +// scripts do not, so a surviving grandchild would keep the drains pending. +// The group kill must close the pipes, and output written before the exit or +// the timeout must still be captured. +// --------------------------------------------------------------------------- + +describe('runDeterministic: descendants holding the output pipes (#10, #17)', () => { + /** Grandchild pids a test recorded; afterEach SIGKILLs any that survived. */ + const recorded: number[] = []; + + afterEach(() => { + // Single pids only, never a group: a leaked grandchild of an undetached + // spawn shares the test runner's process group. + for (const pid of recorded.splice(0)) { + try { + process.kill(pid, 'SIGKILL'); + } catch { + /* already gone: the expected case */ + } + } + }); + + /** + * A script that writes to stdout and stderr, backgrounds a `sleep 30` that + * inherits both pipes, records its pid, then runs `tail`. + */ + function pipeHoldingScript(tail: string): string { + const script = join(dir, 'hold.sh'); + writeFileSync( + script, + ['echo before-out', 'echo before-err >&2', 'sleep 30 &', 'echo "$!" > grandchild.pid', tail, ''].join('\n'), + ); + return script; + } + + function grandchildPid(): number { + const pid = Number(readFileSync(join(dir, 'grandchild.pid'), 'utf-8').trim()); + expect(Number.isInteger(pid) && pid > 1).toBe(true); + recorded.push(pid); + return pid; + } + + async function waitForPidGone(pid: number): Promise { + const deadline = Date.now() + 3_000; + while (Date.now() < deadline) { + try { + process.kill(pid, 0); + } catch { + return true; + } + await new Promise((r) => setTimeout(r, 25)); + } + return false; + } + + for (const [label, code] of [ + ['exits 0', 0], + ['exits nonzero', 4], + ] as const) { + it(`returns promptly with the output written before it ${label}, and the grandchild is gone`, async () => { + const script = pipeHoldingScript(`exit ${code}`); + const start = Date.now(); + const result = await runDeterministic({ command: 'sh', args: [script] }, { allowlist: ['sh'], cwd: dir }); + const elapsed = Date.now() - start; + + // Without the group kill the drains wait out the 30s sleep. + expect(elapsed).toBeLessThan(10_000); + expect(result.exitCode).toBe(code); + expect(result.ok).toBe(code === 0); + expect(result.timedOut).toBeFalsy(); + expect(result.stdout).toBe('before-out\n'); + expect(result.stderr).toBe('before-err\n'); + expect(await waitForPidGone(grandchildPid())).toBe(true); + }, 40_000); + } + + it('does not report a timeout when the group is killed after a normal exit', async () => { + const script = pipeHoldingScript('exit 0'); + const result = await runDeterministic( + { command: 'sh', args: [script] }, + { allowlist: ['sh'], cwd: dir, timeoutMs: 20_000 }, + ); + expect(result.ok).toBe(true); + expect(result.exitCode).toBe(0); + expect(result.timedOut).toBeFalsy(); + expect(result.stderr).not.toMatch(/exceeded its timeout/i); + expect(await waitForPidGone(grandchildPid())).toBe(true); + }, 40_000); + + it('returns promptly after the deadline and keeps the output written before the timeout', async () => { + const script = pipeHoldingScript('wait'); + const start = Date.now(); + const result = await runDeterministic( + { command: 'sh', args: [script] }, + { allowlist: ['sh'], cwd: dir, timeoutMs: 300 }, + ); + const elapsed = Date.now() - start; + + // #10's reproduction returned only when the descendant exited. + expect(elapsed).toBeLessThan(10_000); + expect(result.ok).toBe(false); + expect(result.timedOut).toBe(true); + expect(result.stdout).toBe('before-out\n'); + expect(result.stderr).toStartWith('before-err\n'); + expect(result.stderr).toMatch(/exceeded its timeout of 300ms/); + expect(await waitForPidGone(grandchildPid())).toBe(true); + }, 40_000); +}); diff --git a/src/worker/deterministic.ts b/src/worker/deterministic.ts index 69924e6..142c378 100644 --- a/src/worker/deterministic.ts +++ b/src/worker/deterministic.ts @@ -11,8 +11,14 @@ * * Denylist-based approaches are insufficient on an untrusted substrate — only a * positive allowlist guarantees the blast radius stays bounded. + * + * Containment (issues #10, #17): the command runs as its own process group, and + * the runner SIGKILLs that group on timeout and again after the command exits, + * so nothing the station started outlives it, whatever the exit reason. */ +import { killProcessGroup } from './process-group'; + // --------------------------------------------------------------------------- // Public types // --------------------------------------------------------------------------- @@ -29,8 +35,9 @@ export interface LawLiteConfig { cwd?: string; /** * Optional wall-clock timeout in MILLISECONDS (pre-launch punch-list #8). - * When set (> 0), the spawned command is killed (SIGKILL) if it exceeds the - * deadline and runDeterministic returns a timeout failure rather than hanging. + * When set (> 0), the spawned command's process group is killed (SIGKILL) if + * it exceeds the deadline and runDeterministic returns a timeout failure + * rather than hanging. * Absent / undefined → unbounded (today's behaviour). The executor converts a * station's `timeout_seconds` to ms and threads it here. */ @@ -162,6 +169,11 @@ export function deterministicCardEnv(reworkCount: number, attempt: number): Reco * * Uses Bun.spawn with an array (no shell) so no metacharacter expansion can * occur at the OS level even if the guard were somehow bypassed. + * + * The command is spawned detached (its own process group). The group is + * SIGKILLed when the timeout fires and again once the command has exited, so + * a descendant it backgrounded neither keeps running nor holds the output + * pipes open past the return. */ export async function runDeterministic( cmd: DeterministicCommand, @@ -174,13 +186,9 @@ export async function runDeterministic( ); } - // Punch-list #8 — enforce an optional wall-clock timeout. Bun.spawn's native - // `timeout` + `killSignal` kills the process at the deadline (no leaked timer - // to clear, unlike a manual setTimeout race) and resolves `proc.exited`, so - // the function returns at ~the timeout window rather than hanging. A timeout - // <= 0 or absent means unbounded (today's behaviour, byte-for-byte). + // Punch-list #8: an optional wall-clock timeout. A timeout <= 0 or absent + // means unbounded. const hasTimeout = typeof config.timeoutMs === 'number' && config.timeoutMs > 0; - const startedAt = Date.now(); // Env injection: when extra vars are supplied, spawn with an explicit env that // is the inherited process env with the injected vars layered on top. Bun.spawn @@ -193,32 +201,56 @@ export async function runDeterministic( const proc = Bun.spawn([cmd.command, ...cmd.args], { stdout: 'pipe', stderr: 'pipe', + // setsid(): the child becomes its own session/process-group leader, so + // `-proc.pid` addresses the whole group (grandchildren included) below. + detached: true, ...(config.cwd ? { cwd: config.cwd } : {}), ...(hasEnv ? { env: { ...process.env, ...config.env } } : {}), - ...(hasTimeout ? { timeout: config.timeoutMs, killSignal: 'SIGKILL' as const } : {}), }); - const [exitCode, stdout, stderr] = await Promise.all([ - proc.exited, - new Response(proc.stdout).text(), - new Response(proc.stderr as ReadableStream).text(), - ]); - - // Distinguish OUR timeout-kill from an unrelated SIGKILL (OOM-killer, a child - // that self-kills, a propagated signal). Bun exposes no "killed by my timeout" - // flag, so we require BOTH a SIGKILL signal AND that it landed at/after the - // deadline — a SIGKILL well before the timeout could not have been ours. This - // keeps the timeout diagnostic honest (never claim a timeout we can't prove). + // Our own timer instead of Bun.spawn's native `timeout`, which kills only the + // immediate child (#10). Killing the group also closes the pipes any + // descendant inherited, so the drains below settle at the deadline. + let timerFired = false; + const timer = hasTimeout + ? setTimeout(() => { + timerFired = true; + killProcessGroup(proc.pid); + }, config.timeoutMs) + : undefined; + + // Start draining now so a command that writes more than a pipe buffer is not + // blocked on a full pipe while we wait for it to exit. + const stdoutText = new Response(proc.stdout).text(); + const stderrText = new Response(proc.stderr as ReadableStream).text(); + + const exitCode = await proc.exited; + clearTimeout(timer); + + // #17: the command has exited, but a descendant it backgrounded may still be + // running and holding the pipes. Kill the group BEFORE awaiting the drains so + // they settle. Bytes already written stay readable, so no output is lost. + // The leader has already exited, so this does not change its exit status. + // While any member is alive the group id cannot be reused, so the signal + // reaches only this command's descendants; an empty group is ESRCH. + killProcessGroup(proc.pid); + + const [stdout, stderr] = await Promise.all([stdoutText, stderrText]); + + // Distinguish OUR timeout-kill from a normal exit or an unrelated SIGKILL + // (OOM-killer, a child that self-kills). Both must hold: our timer fired, and + // the leader died of SIGKILL. The timer can fire in the gap between a natural + // exit and the clearTimeout above; the leader then exited on its own, carries + // no SIGKILL signalCode, and is not reported as timed out. The post-exit + // group kill never reaches the leader, so it cannot mark a normal exit either. // Either way an unsuccessful exit is a failure (ok:false) on the same path; - // only the `timedOut` label + message are gated on this stricter check. - const elapsed = Date.now() - startedAt; - const timedOut = - hasTimeout && proc.signalCode === 'SIGKILL' && elapsed >= config.timeoutMs! * 0.9; + // only the `timedOut` label + message are gated on this check. + const timedOut = timerFired && proc.signalCode === 'SIGKILL'; if (timedOut) { - // On a Bun timeout-kill, `await proc.exited` resolves to 137 (128 + SIGKILL), - // even though the `proc.exitCode` getter reads null. We use the resolved - // value; SIGKILL_EXIT is a defensive fallback for that getter/promise skew. + // On a SIGKILL, `await proc.exited` resolves to 137 (128 + SIGKILL), even + // though the `proc.exitCode` getter reads null. We use the resolved value; + // SIGKILL_EXIT is a defensive fallback for that getter/promise skew. const SIGKILL_EXIT = 137; return { ok: false, diff --git a/src/worker/harness-containment.conformance.test.ts b/src/worker/harness-containment.conformance.test.ts index 1a42ecf..04e7d13 100644 --- a/src/worker/harness-containment.conformance.test.ts +++ b/src/worker/harness-containment.conformance.test.ts @@ -36,7 +36,7 @@ describe('harnessAdapterSpawnPath', () => { throw Object.assign(new Error('timed out'), { code: HARNESS_TIMEOUT_CLASS }); }); const spawnPath = harnessAdapterSpawnPath('fake-adapter', () => adapter); - const code = await spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000 }); + const code = await spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000, fixtureArgs: [] }); expect(code).toBe(HARNESS_TIMEOUT_CLASS); }); @@ -47,7 +47,7 @@ describe('harnessAdapterSpawnPath', () => { }); const spawnPath = harnessAdapterSpawnPath('fake-adapter', () => adapter); await expect( - spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000 }), + spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000, fixtureArgs: [] }), ).rejects.toBe(boom); }); @@ -57,14 +57,14 @@ describe('harnessAdapterSpawnPath', () => { }); const spawnPath = harnessAdapterSpawnPath('fake-adapter', () => adapter); await expect( - spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000 }), + spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000, fixtureArgs: [] }), ).rejects.toMatchObject({ code: 'harness-exit-nonzero' }); }); it('returns undefined when the invocation resolves', async () => { const adapter = makeFake('fake-adapter', async () => RESULT); const spawnPath = harnessAdapterSpawnPath('fake-adapter', () => adapter); - const code = await spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000 }); + const code = await spawnPath({ projectRoot: '/tmp/unused', fixture: '/bin/true', timeoutMs: 1000, fixtureArgs: [] }); expect(code).toBeUndefined(); }); }); diff --git a/src/worker/harness-containment.conformance.ts b/src/worker/harness-containment.conformance.ts index c3f1ed5..e5904ff 100644 --- a/src/worker/harness-containment.conformance.ts +++ b/src/worker/harness-containment.conformance.ts @@ -3,7 +3,9 @@ * * Every spawn path that runs a worker process must kill that worker's whole * process tree on timeout, so a grandchild the worker backgrounded (a shell, a - * browser, a language server) does not outlive the invocation. The guarantee + * browser, a language server) does not outlive the invocation. A spawn path + * that opts in with `reapsOnExit` must also kill it when the worker exits on + * its own, with status 0 or nonzero (issue #17). The guarantee * is implemented in `runHarnessProcess` (./harness-runner.ts) and proved there * by the runner's own AC3 test. That test calls the runner directly, so an * adapter that spawns some other way would drop the guarantee without anything @@ -22,7 +24,8 @@ * * The fixture (./containment-fixture.sh) stands in for the binary. It * backgrounds a grandchild that records its pid and touches a sentinel file - * every 100ms, then blocks. The suite proves the grandchild died two ways: the + * every 100ms, then blocks, or exits with a given code when run with + * `--exit `. The suite proves the grandchild died two ways: the * pid is gone, and the sentinel mtime stops advancing. The second check does * not depend on the pid, so a recycled pid cannot make a surviving grandchild * look dead. @@ -62,6 +65,12 @@ export interface ContainmentRun { fixture: string; /** Wall-clock bound to pass to the spawn path's own timeout mechanism. */ timeoutMs: number; + /** + * Arguments for the fixture. Empty for the timeout scenario. The exit + * scenarios, registered only with `reapsOnExit`, pass `--exit `, so a + * spawn path that opts in must hand these to the fixture unchanged. + */ + fixtureArgs: string[]; } /** @@ -77,15 +86,32 @@ export interface ContainmentConformanceOptions { timeoutClass: string; /** * Set when the path is known not to reap descendants yet, naming the open - * issue(s). The reaping test is then registered with `test.failing`: it - * reports as passing while the bug stands, and turns red the moment the - * path is fixed, so the marker has to be removed rather than left behind. - * A separate normal test still pins the timeout class and the fixture, so a - * broken fixture cannot hide behind the inverted test. + * issue(s). The reaping tests are then registered with `test.failing`: they + * report as passing while the bug stands, and turn red the moment the path + * is fixed, so the marker has to be removed rather than left behind. A + * separate normal test still pins the timeout class and the fixture, so a + * broken fixture cannot hide behind the inverted test. No shipped path sets + * it today; it is kept so a new spawn path can be registered before its fix. */ knownLeak?: string; + /** + * Also register the exit scenarios: the fixture backgrounds its grandchild + * and exits 0, or exits nonzero, well before the timeout. The spawn path + * must return without reporting a timeout and the grandchild must be gone. + * Opt-in because it requires the path to pass `fixtureArgs` through, and a + * harness adapter builds its own argv and treats a nonzero exit as an error. + */ + reapsOnExit?: boolean; } +/** + * The wall-clock bound for the exit scenarios. The fixture exits on its own + * long before this, so a path that returns near it waited on something the + * exit should have ended. + */ +const EXIT_SCENARIO_TIMEOUT_MS = 10_000; +const EXIT_SCENARIO_PROMPT_MS = 5_000; + /** True while `pid` exists (signal 0 checks for existence without delivering a signal). */ function isAlive(pid: number): boolean { try { @@ -118,7 +144,8 @@ function sentinelMtime(projectRoot: string): number | undefined { } } -function recordedPid(projectRoot: string): number | undefined { +/** The grandchild pid the fixture recorded in `projectRoot`, if it got that far. */ +export function recordedPid(projectRoot: string): number | undefined { const path = join(projectRoot, PID_FILE); if (!existsSync(path)) return undefined; const pid = Number(readFileSync(path, 'utf-8').trim()); @@ -143,6 +170,45 @@ async function watchSentinelAdvance(projectRoot: string, settled: () => boolean) return false; } +/** The fixture arguments that make it exit with `code` once its grandchild is running. */ +export function containmentFixtureExitArgs(code: number): string[] { + return ['--exit', String(code)]; +} + +/** + * SIGKILL the grandchild recorded in `projectRoot`, for an afterEach, so a + * failing test does not leave the fixture's loop running in the developer's + * session. Only the recorded pid: when the path did not detach the worker, + * the grandchild shares the test runner's process group, so a group kill here + * would take the runner down with it. + */ +export function killRecordedGrandchild(projectRoot: string): void { + const pid = recordedPid(projectRoot); + if (pid === undefined) return; + try { + process.kill(pid, 'SIGKILL'); + } catch { + /* already gone: the expected case */ + } +} + +/** + * Assert that the fixture's grandchild in `projectRoot` is dead, two ways: + * the recorded pid disappears, and the sentinel stops advancing. A live + * grandchild touches the sentinel every 100ms, so an unchanged mtime across + * the window means nothing is left running the loop, whatever the pid now + * refers to. + */ +export async function expectGrandchildReaped(projectRoot: string): Promise { + const pid = recordedPid(projectRoot); + expect(pid).toBeDefined(); + expect(await waitForPidGone(pid!, REAP_BUDGET_MS)).toBe(true); + + const before = sentinelMtime(projectRoot); + await sleep(STALL_WINDOW_MS); + expect(sentinelMtime(projectRoot)).toBe(before); +} + interface ObservedRun { timeoutClass: string | undefined; /** The sentinel advanced while the invocation was running. */ @@ -152,7 +218,12 @@ interface ObservedRun { async function runAndObserve(spawnPath: ContainmentSpawnPath, projectRoot: string): Promise { let done = false; - const invocation = spawnPath({ projectRoot, fixture: CONTAINMENT_FIXTURE, timeoutMs: TIMEOUT_MS }).finally( + const invocation = spawnPath({ + projectRoot, + fixture: CONTAINMENT_FIXTURE, + timeoutMs: TIMEOUT_MS, + fixtureArgs: [], + }).finally( () => { done = true; }, @@ -181,19 +252,7 @@ export function describeContainmentConformance( }); afterEach(() => { - // Kill a grandchild the spawn path failed to reap, so a failing test - // does not leave the fixture's loop running in the developer's session. - // Only the recorded pid: when the path did not detach the worker, the - // grandchild shares the test runner's process group, so a group kill - // here would take the runner down with it. - const pid = recordedPid(projectRoot); - if (pid !== undefined) { - try { - process.kill(pid, 'SIGKILL'); - } catch { - /* already gone: the expected case */ - } - } + killRecordedGrandchild(projectRoot); rmSync(projectRoot, { recursive: true, force: true }); }); @@ -223,18 +282,40 @@ export function describeContainmentConformance( expect(run.sawGrandchildLive).toBe(true); expect(run.grandchildPid).toBeDefined(); - // Proof one: the recorded pid disappears. - expect(await waitForPidGone(run.grandchildPid!, REAP_BUDGET_MS)).toBe(true); - - // Proof two: the sentinel stops advancing. A live grandchild touches - // it every 100ms, so an unchanged mtime across the window means - // nothing is left running the loop, whatever the pid now refers to. - const before = sentinelMtime(projectRoot); - await sleep(STALL_WINDOW_MS); - expect(sentinelMtime(projectRoot)).toBe(before); + await expectGrandchildReaped(projectRoot); }, TEST_TIMEOUT_MS, ); + + if (options.reapsOnExit === true) { + for (const [label, code] of [ + ['exits 0', 0], + ['exits nonzero', 3], + ] as const) { + reaps( + `kills a backgrounded grandchild when the worker ${label} before its timeout`, + async () => { + const startedAt = Date.now(); + const timeoutClass = await spawnPath({ + projectRoot, + fixture: CONTAINMENT_FIXTURE, + timeoutMs: EXIT_SCENARIO_TIMEOUT_MS, + fixtureArgs: containmentFixtureExitArgs(code), + }); + + expect(Date.now() - startedAt).toBeLessThan(EXIT_SCENARIO_PROMPT_MS); + // A worker that exited on its own was not timed out, even though + // the path killed its process group afterwards. + expect(timeoutClass).toBeUndefined(); + // The fixture exits only after the grandchild has touched the + // sentinel, so the grandchild was running when the worker exited. + expect(sentinelMtime(projectRoot)).toBeDefined(); + await expectGrandchildReaped(projectRoot); + }, + TEST_TIMEOUT_MS, + ); + } + } }); } diff --git a/src/worker/harness-runner.ts b/src/worker/harness-runner.ts index 58d6267..80e3cb2 100644 --- a/src/worker/harness-runner.ts +++ b/src/worker/harness-runner.ts @@ -7,12 +7,12 @@ * (not a lone pid) on expiry, and confines the child's cwd to inside the * project root. * - * Bun.spawn's native `timeout`/`killSignal` (used by runDeterministic in - * ./deterministic.ts) kills only the immediate child — a grandchild the - * harness backgrounds (a common agent-CLI pattern) reparents to init and - * survives. This runner instead spawns the child DETACHED (`setsid()`, so - * the child's pid becomes its own process-group id) and, on timeout, sends - * SIGKILL to the negative pid — the whole group, grandchildren included. + * Bun.spawn's native `timeout`/`killSignal` kills only the immediate child: a + * grandchild the harness backgrounds (a common agent-CLI pattern) reparents to + * init and survives. This runner instead spawns the child DETACHED (`setsid()`, + * so the child's pid becomes its own process-group id) and, on timeout, sends + * SIGKILL to the negative pid, which is the whole group, grandchildren included + * (`killProcessGroup` in ./process-group.ts, shared with runDeterministic). * * Do NOT apply a command allowlist here — the harness binary comes from * trusted engine config (WI-560); the `tools` allowlist is enforced @@ -22,6 +22,7 @@ import { resolve, sep } from 'node:path'; import { existsSync, statSync } from 'node:fs'; +import { killProcessGroup } from './process-group'; /** The command + argv to spawn (no shell — array form, per the Law-lite pattern). */ export interface HarnessCommand { @@ -177,12 +178,7 @@ export async function runHarnessProcess( }); const timer = setTimeout(() => { - try { - // Negative pid == the process GROUP, not just the immediate child. - process.kill(-proc.pid, 'SIGKILL'); - } catch { - /* group already exited — nothing to kill */ - } + killProcessGroup(proc.pid); }, config.timeoutMs); const [exitCode, stdout, stderr] = await Promise.all([ @@ -196,8 +192,7 @@ export async function runHarnessProcess( const durationMs = Date.now() - startedAt; - // Distinguish OUR timeout-kill from a normal exit (mirrors runDeterministic's - // discrimination in ./deterministic.ts). A shared flag set independently + // Distinguish OUR timeout-kill from a normal exit. A shared flag set independently // inside the setTimeout callback would race: when the child exits naturally // right around the deadline, the timer can still fire and attempt a kill — // harmless (it fails silently on an already-exited pid) but a flag set there diff --git a/src/worker/process-group.ts b/src/worker/process-group.ts new file mode 100644 index 0000000..7e89f41 --- /dev/null +++ b/src/worker/process-group.ts @@ -0,0 +1,24 @@ +/** + * Process-group termination shared by the deterministic station runner + * (./deterministic.ts) and the harness runner (./harness-runner.ts). + * + * Both spawn their child with `detached: true`, which calls setsid(): the + * child becomes its own session and process-group leader, so its pid is also + * the group id. Every descendant that does not call setsid() itself stays in + * that group, and a signal sent to the negative pid reaches all of them. This + * works on Linux and macOS without prctl or a subreaper. + */ + +/** + * SIGKILL every process in the group led by `pid`. ESRCH (the group is + * already empty) is the normal case after a clean exit and is ignored, as is + * any other kill error: there is nothing further the caller could do. + */ +export function killProcessGroup(pid: number): void { + try { + // Negative pid == the process GROUP, not just the immediate child. + process.kill(-pid, 'SIGKILL'); + } catch { + /* group already exited: nothing to kill */ + } +} diff --git a/src/worker/worker-entry.test.ts b/src/worker/worker-entry.test.ts index 39ee59c..0725d7b 100644 --- a/src/worker/worker-entry.test.ts +++ b/src/worker/worker-entry.test.ts @@ -15,6 +15,12 @@ import { tmpdir } from 'node:os'; import { runWorkerProcess, type WorkerProcessIO } from './worker-entry'; import { serializeWorkerMessage, parseWorkerMessage } from './ipc-protocol'; import type { WorkerMessage } from './ipc-protocol'; +import { + CONTAINMENT_FIXTURE, + containmentFixtureExitArgs, + expectGrandchildReaped, + killRecordedGrandchild, +} from './harness-containment.conformance'; // --------------------------------------------------------------------------- // Harness: a controllable IO seam capturing sends/exits and feeding raw input. @@ -228,6 +234,54 @@ stations: }); }); +describe('worker-entry #17: a pooled deterministic station reaps its descendants on exit 0', () => { + afterEach(() => { + killRecordedGrandchild(dir); + }); + + it('reports success and leaves no backgrounded grandchild running', async () => { + // The containment fixture backgrounds a grandchild that touches a sentinel, + // waits for its first touch, then exits 0. It writes into its cwd, which + // the worker sets to the project root. + const args = [CONTAINMENT_FIXTURE, ...containmentFixtureExitArgs(0)].map((a) => JSON.stringify(a)); + const reapFlow = join(dir, 'reap-flow.yaml'); + writeFileSync( + reapFlow, + ` +flow: worker-entry-reap-test +project_root: . +flow_version: 1 +budgets: + run: { wall_clock_minutes: 10, max_tokens: 1000 } + per_card: { max_execution_attempts: 1 } + liveness: { no_progress_minutes: 3 } +defaults: { cap_policy: scrap, on_dep_scrap: hold } +terminal_lanes: [done, scrap, hold] +security: + bash: + allow: ["sh"] +stations: + - id: spawner + worker: { kind: deterministic, command: "sh", args: [${args.join(', ')}] } + next: done +`, + ); + const h = makeIO(); + runWorkerProcess({ flowPath: reapFlow, projectRoot: dir }, h.io); + h.deliver(start('c1', 'spawner')); + + const deadline = Date.now() + 10_000; + while (h.sent.length === 0 && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 25)); + } + + const done = h.sent[0]; + if (done?.type !== 'MARK_DONE') throw new Error('expected a MARK_DONE'); + expect(done.outcome).toBe('success'); + await expectGrandchildReaped(dir); + }, 20_000); +}); + describe('worker-entry — NFR-3: the worker module does no state-DB I/O', () => { it('does not import the persistence DB module', async () => { // Static guard: the worker entry must never pull in the state DB (the kernel From cd021edd5f66e993edcf6053f49b9fc481477538 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Thu, 24 Sep 2026 21:16:20 +0000 Subject: [PATCH 2/7] fix(worker): kill live station process groups when the kernel is signalled or exits Spawning station commands detached (5c14026) moved them out of the terminal's foreground process group. `conduit run` installs no signal handler, so a Ctrl-C killed the kernel and its timeout timer while the station command ran on unbounded. runHarnessProcess already had this gap. src/worker/process-group.ts now keeps a registry of live station group ids. Both runners register the pid right after spawn and unregister in a finally after their final group kill. While the registry is non-empty, SIGINT, SIGTERM and SIGHUP handlers (prepended) and an exit handler are installed; they are removed when it empties, so an idle kernel keeps its default signal behaviour. On a signal the handler kills every live group, removes itself, and re-raises the signal when no other listener remains, so the default termination and exit status still happen. When another listener exists, such as conduit listen's graceful stop, termination is left to it. The exit handler kills live groups synchronously for a process.exit() mid-station. Tests spawn a real stand-in kernel (containment-signal-runner.ts) running the containment fixture and send it SIGINT or SIGTERM, or have it call process.exit(), then assert the kernel's default outcome and that the grandchild is gone by pid and sentinel. Unit tests pin that the handlers exist only while a group is live and that another listener suppresses the re-raise. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RbrsaSXMxkbrk8gdd1UeSt --- docs/harness-containment.md | 4 +- src/worker/containment-signal-runner.ts | 40 ++++++ src/worker/deterministic.ts | 29 +++-- src/worker/harness-runner.ts | 28 +++-- src/worker/process-group.test.ts | 154 ++++++++++++++++++++++++ src/worker/process-group.ts | 86 +++++++++++++ 6 files changed, 320 insertions(+), 21 deletions(-) create mode 100644 src/worker/containment-signal-runner.ts create mode 100644 src/worker/process-group.test.ts diff --git a/docs/harness-containment.md b/docs/harness-containment.md index 0b710aa..ca38a9d 100644 --- a/docs/harness-containment.md +++ b/docs/harness-containment.md @@ -59,7 +59,9 @@ hold it together: process group before it returns, so a grandchild does not outlive a station that finished on its own ([#10](https://github.com/theaiteam-dev/conduit/issues/10), [#17](https://github.com/theaiteam-dev/conduit/issues/17)). A harness adapter kills the - group on timeout only. + group on timeout only. Both runners also kill every live station process group when the + kernel receives SIGINT, SIGTERM or SIGHUP, or exits; a SIGKILLed kernel cannot do this, + which is why the container boundary below still matters. Deployment guidance additionally recommends running the container as a dedicated non-root user for agentic/harness flows. - **The ADR-0003 container wall.** [ADR-0003](../adr/0003-packaging-and-distribution.md)'s diff --git a/src/worker/containment-signal-runner.ts b/src/worker/containment-signal-runner.ts new file mode 100644 index 0000000..3cf701c --- /dev/null +++ b/src/worker/containment-signal-runner.ts @@ -0,0 +1,40 @@ +/** + * Stand-in kernel for the signal containment tests (./process-group.test.ts). + * + * bun containment-signal-runner.ts [exit] + * + * Runs the containment fixture (./containment-fixture.sh) through the named + * runner with a timeout far longer than any test, so only the kernel's own + * signal or exit handling can end the station. Prints `ready` once the fixture + * has recorded its grandchild's pid, by which point the runner has registered + * the group. With `exit`, it then calls process.exit(0) mid-station. + * + * This file is not a test file. Bun only runs it when a test spawns it. + */ +import { existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { runDeterministic } from './deterministic'; +import { runHarnessProcess } from './harness-runner'; + +const [runner, projectRoot, mode] = process.argv.slice(2); +if ((runner !== 'deterministic' && runner !== 'harness') || projectRoot === undefined) { + console.error('usage: containment-signal-runner.ts [exit]'); + process.exit(2); +} + +const fixture = join(import.meta.dir, 'containment-fixture.sh'); +const timeoutMs = 600_000; + +const poll = setInterval(() => { + if (!existsSync(join(projectRoot, 'containment.pid'))) return; + clearInterval(poll); + console.log('ready'); + if (mode === 'exit') process.exit(0); +}, 20); + +if (runner === 'deterministic') { + await runDeterministic({ command: fixture, args: [] }, { allowlist: [fixture], cwd: projectRoot, timeoutMs }); +} else { + await runHarnessProcess({ command: fixture, args: [] }, { projectRoot, timeoutMs }); +} +console.log('station returned'); diff --git a/src/worker/deterministic.ts b/src/worker/deterministic.ts index 142c378..f737780 100644 --- a/src/worker/deterministic.ts +++ b/src/worker/deterministic.ts @@ -17,7 +17,7 @@ * so nothing the station started outlives it, whatever the exit reason. */ -import { killProcessGroup } from './process-group'; +import { killProcessGroup, trackProcessGroup, untrackProcessGroup } from './process-group'; // --------------------------------------------------------------------------- // Public types @@ -207,6 +207,9 @@ export async function runDeterministic( ...(config.cwd ? { cwd: config.cwd } : {}), ...(hasEnv ? { env: { ...process.env, ...config.env } } : {}), }); + // The detached group no longer receives the terminal's Ctrl-C, so register + // it for the kernel's signal and exit handlers until the final kill below. + trackProcessGroup(proc.pid); // Our own timer instead of Bun.spawn's native `timeout`, which kills only the // immediate child (#10). Killing the group also closes the pipes any @@ -224,16 +227,20 @@ export async function runDeterministic( const stdoutText = new Response(proc.stdout).text(); const stderrText = new Response(proc.stderr as ReadableStream).text(); - const exitCode = await proc.exited; - clearTimeout(timer); - - // #17: the command has exited, but a descendant it backgrounded may still be - // running and holding the pipes. Kill the group BEFORE awaiting the drains so - // they settle. Bytes already written stay readable, so no output is lost. - // The leader has already exited, so this does not change its exit status. - // While any member is alive the group id cannot be reused, so the signal - // reaches only this command's descendants; an empty group is ESRCH. - killProcessGroup(proc.pid); + let exitCode: number; + try { + exitCode = await proc.exited; + } finally { + clearTimeout(timer); + // #17: the command has exited, but a descendant it backgrounded may still + // be running and holding the pipes. Kill the group BEFORE awaiting the + // drains so they settle. Bytes already written stay readable, so no output + // is lost. The leader has already exited, so this does not change its exit + // status. While any member is alive the group id cannot be reused, so the + // signal reaches only this command's descendants; an empty group is ESRCH. + killProcessGroup(proc.pid); + untrackProcessGroup(proc.pid); + } const [stdout, stderr] = await Promise.all([stdoutText, stderrText]); diff --git a/src/worker/harness-runner.ts b/src/worker/harness-runner.ts index 80e3cb2..a2b2c92 100644 --- a/src/worker/harness-runner.ts +++ b/src/worker/harness-runner.ts @@ -22,7 +22,7 @@ import { resolve, sep } from 'node:path'; import { existsSync, statSync } from 'node:fs'; -import { killProcessGroup } from './process-group'; +import { killProcessGroup, trackProcessGroup, untrackProcessGroup } from './process-group'; /** The command + argv to spawn (no shell — array form, per the Law-lite pattern). */ export interface HarnessCommand { @@ -176,19 +176,29 @@ export async function runHarnessProcess( // Never inherit the parent env wholesale — only the allowlisted names. env: childEnv, }); + // The detached group no longer receives the terminal's Ctrl-C, so register + // it for the kernel's signal and exit handlers (./process-group.ts). + trackProcessGroup(proc.pid); const timer = setTimeout(() => { killProcessGroup(proc.pid); }, config.timeoutMs); - const [exitCode, stdout, stderr] = await Promise.all([ - proc.exited, - config.stdoutLineFilter !== undefined - ? readKeptLines(proc.stdout as ReadableStream, config.stdoutLineFilter) - : new Response(proc.stdout).text(), - new Response(proc.stderr as ReadableStream).text(), - ]); - clearTimeout(timer); + let exitCode: number; + let stdout: string; + let stderr: string; + try { + [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + config.stdoutLineFilter !== undefined + ? readKeptLines(proc.stdout as ReadableStream, config.stdoutLineFilter) + : new Response(proc.stdout).text(), + new Response(proc.stderr as ReadableStream).text(), + ]); + } finally { + clearTimeout(timer); + untrackProcessGroup(proc.pid); + } const durationMs = Date.now() - startedAt; diff --git a/src/worker/process-group.test.ts b/src/worker/process-group.test.ts new file mode 100644 index 0000000..0c4de7d --- /dev/null +++ b/src/worker/process-group.test.ts @@ -0,0 +1,154 @@ +/** + * Tests for the live station group registry (./process-group.ts). + * + * Both runners spawn detached, so a Ctrl-C at the terminal reaches only the + * kernel. These tests prove that a kernel which receives SIGINT or SIGTERM, or + * calls process.exit() mid-station, still takes the station's process group + * with it, and that the handlers exist only while a group is live. + * + * The end-to-end tests spawn ./containment-signal-runner.ts as a real kernel + * stand-in and signal that process by its own pid. Cleanup kills recorded + * single pids only, never a group. + */ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { mkdtempSync, realpathSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { TERMINATING_SIGNALS, trackProcessGroup, untrackProcessGroup } from './process-group'; +import { expectGrandchildReaped, killRecordedGrandchild, recordedPid } from './harness-containment.conformance'; + +const RUNNER_SCRIPT = join(import.meta.dir, 'containment-signal-runner.ts'); +const READY_BUDGET_MS = 10_000; +const EXIT_BUDGET_MS = 10_000; +const TEST_TIMEOUT_MS = 30_000; + +async function sleep(ms: number): Promise { + await new Promise((r) => setTimeout(r, ms)); +} + +function listenerCounts(): number[] { + return [...TERMINATING_SIGNALS.map((s) => process.listenerCount(s)), process.listenerCount('exit')]; +} + +describe('process-group registry: handlers exist only while a group is live', () => { + // A pid far above any real one: tracking never signals it, and the test + // untracks before anything could. + const FAKE_PID = 2_147_000_000; + + it('installs the signal and exit handlers on the first tracked group and removes them when the last is untracked', () => { + const baseline = listenerCounts(); + + trackProcessGroup(FAKE_PID); + trackProcessGroup(FAKE_PID + 1); + expect(listenerCounts()).toEqual(baseline.map((n) => n + 1)); + + untrackProcessGroup(FAKE_PID); + expect(listenerCounts()).toEqual(baseline.map((n) => n + 1)); + + untrackProcessGroup(FAKE_PID + 1); + expect(listenerCounts()).toEqual(baseline); + }); + + it('kills live groups and leaves termination to another listener when one exists', async () => { + // A real detached group, so the kill is observable. Its pid is its group id. + const sleeper = Bun.spawn(['sleep', '30'], { detached: true, stdout: 'ignore', stderr: 'ignore' }); + const baseline = listenerCounts(); + let calls = 0; + const other = () => { + calls += 1; + }; + process.on('SIGINT', other); + try { + trackProcessGroup(sleeper.pid); + process.emit('SIGINT', 'SIGINT'); + + await sleeper.exited; + expect(sleeper.signalCode).toBe('SIGKILL'); + // Our handler removed itself. A re-raise would have reached `other` a + // second time, asynchronously, so give it the chance to show up. + await sleep(200); + expect(calls).toBe(1); + expect(process.listenerCount('SIGINT')).toBe(baseline[0]! + 1); + expect(process.listenerCount('exit')).toBe(baseline[3]!); + } finally { + process.off('SIGINT', other); + untrackProcessGroup(sleeper.pid); + try { + process.kill(sleeper.pid, 'SIGKILL'); + } catch { + /* already gone */ + } + } + }); +}); + +describe('process-group registry: the kernel takes live station groups with it', () => { + let projectRoot: string; + let kernel: Bun.Subprocess<'ignore', 'pipe', 'pipe'> | undefined; + + beforeEach(() => { + projectRoot = realpathSync(mkdtempSync(join(tmpdir(), 'conduit-signal-'))); + kernel = undefined; + }); + + afterEach(() => { + if (kernel !== undefined) { + try { + process.kill(kernel.pid, 'SIGKILL'); + } catch { + /* already gone: the expected case */ + } + } + killRecordedGrandchild(projectRoot); + rmSync(projectRoot, { recursive: true, force: true }); + }); + + /** Spawn the stand-in kernel and wait until its station's grandchild is running. */ + async function startKernel(runner: 'deterministic' | 'harness', mode?: 'exit'): Promise { + kernel = Bun.spawn(['bun', RUNNER_SCRIPT, runner, projectRoot, ...(mode ? [mode] : [])], { + stdin: 'ignore', + stdout: 'pipe', + stderr: 'pipe', + }); + const deadline = Date.now() + READY_BUDGET_MS; + while (recordedPid(projectRoot) === undefined && Date.now() < deadline) { + await sleep(20); + } + expect(recordedPid(projectRoot)).toBeDefined(); + } + + async function waitForKernelExit(): Promise { + const exited = await Promise.race([kernel!.exited.then(() => true), sleep(EXIT_BUDGET_MS).then(() => false)]); + expect(exited).toBe(true); + } + + for (const signal of ['SIGINT', 'SIGTERM'] as const) { + it(`deterministic: ${signal} to the kernel kills the station's grandchild and keeps the default outcome`, async () => { + await startKernel('deterministic'); + process.kill(kernel!.pid, signal); + await waitForKernelExit(); + + // The re-raised signal terminated the kernel, as it would with no handler. + expect(kernel!.signalCode).toBe(signal); + expect(await new Response(kernel!.stdout).text()).not.toContain('station returned'); + await expectGrandchildReaped(projectRoot); + }, TEST_TIMEOUT_MS); + } + + it('harness: SIGINT to the kernel kills the station grandchild and keeps the default outcome', async () => { + await startKernel('harness'); + process.kill(kernel!.pid, 'SIGINT'); + await waitForKernelExit(); + + expect(kernel!.signalCode).toBe('SIGINT'); + await expectGrandchildReaped(projectRoot); + }, TEST_TIMEOUT_MS); + + it('deterministic: process.exit() mid-station kills the station grandchild', async () => { + await startKernel('deterministic', 'exit'); + await waitForKernelExit(); + + expect(kernel!.exitCode).toBe(0); + await expectGrandchildReaped(projectRoot); + }, TEST_TIMEOUT_MS); +}); diff --git a/src/worker/process-group.ts b/src/worker/process-group.ts index 7e89f41..6be6abb 100644 --- a/src/worker/process-group.ts +++ b/src/worker/process-group.ts @@ -7,6 +7,13 @@ * the group id. Every descendant that does not call setsid() itself stays in * that group, and a signal sent to the negative pid reaches all of them. This * works on Linux and macOS without prctl or a subreaper. + * + * A detached child is outside the terminal's foreground process group, so a + * Ctrl-C reaches only the kernel, and the runner's timeout timer dies with it. + * The registry below closes that gap: while any station group is live, SIGINT, + * SIGTERM and SIGHUP handlers and an `exit` handler kill every live group + * before the kernel goes. A SIGKILLed kernel runs no handler, which is why the + * ADR-0003 container boundary still matters. */ /** @@ -22,3 +29,82 @@ export function killProcessGroup(pid: number): void { /* group already exited: nothing to kill */ } } + +/** Signals whose default action terminates the kernel, and so must take live groups with it. */ +export const TERMINATING_SIGNALS = ['SIGINT', 'SIGTERM', 'SIGHUP'] as const; +type TerminatingSignal = (typeof TERMINATING_SIGNALS)[number]; + +/** Group ids of station commands that are running now. */ +const liveGroups = new Set(); + +function killLiveGroups(): void { + for (const pid of liveGroups) killProcessGroup(pid); + liveGroups.clear(); +} + +/** + * Kill the live groups, then preserve the kernel's normal outcome for `signal`. + * When no other listener remains, re-raise the signal so the default action + * (termination, exit status 128 + signo) still happens. When another listener + * exists, such as `conduit listen`'s graceful stop, termination is its job. + */ +function onSignal(signal: TerminatingSignal): void { + killLiveGroups(); + uninstallHandlers(); + if (process.listenerCount(signal) === 0) { + process.kill(process.pid, signal); + } +} + +const signalHandlers: Record void> = { + SIGINT: () => onSignal('SIGINT'), + SIGTERM: () => onSignal('SIGTERM'), + SIGHUP: () => onSignal('SIGHUP'), +}; + +/** `process.exit()` mid-station: only synchronous work runs here, and kill is synchronous. */ +function onExit(): void { + killLiveGroups(); +} + +let installed = false; + +function installHandlers(): void { + if (installed) return; + installed = true; + for (const signal of TERMINATING_SIGNALS) { + // Prepended so it runs before any other listener. A listener that removes + // itself when it runs (conduit listen's does) would otherwise be gone by + // the time onSignal counts the remaining listeners. + process.prependListener(signal, signalHandlers[signal]); + } + process.on('exit', onExit); +} + +function uninstallHandlers(): void { + if (!installed) return; + installed = false; + for (const signal of TERMINATING_SIGNALS) { + process.off(signal, signalHandlers[signal]); + } + process.off('exit', onExit); +} + +/** + * Record a just-spawned detached child as a live station group. The first + * live group installs the signal and exit handlers. + */ +export function trackProcessGroup(pid: number): void { + liveGroups.add(pid); + installHandlers(); +} + +/** + * Forget a group once its runner has finished with it (after its final kill). + * The last one removes the handlers, so an idle kernel, and every test that + * spawns nothing, keeps its default signal behaviour. + */ +export function untrackProcessGroup(pid: number): void { + liveGroups.delete(pid); + if (liveGroups.size === 0) uninstallHandlers(); +} From e94da68fff5c2adfc923953681df4ac16a5cd1d3 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Fri, 25 Sep 2026 04:12:10 +0000 Subject: [PATCH 3/7] Address PR #65 review feedback (pass 1) - Fix: runHarnessProcess kills the process group after the harness leader exits, before awaiting the output drains, so a backgrounded descendant holding the pipes cannot stall a finished harness until its timeout (#17 on the harness path). - Test: register runHarnessProcess for the containment conformance exit scenarios, and add pipe-holding grandchild tests for exit 0 and nonzero. - Docs: harness-containment.md no longer says the harness runner kills the group on timeout only. Addresses review comments from github-actions (nitpick). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RbrsaSXMxkbrk8gdd1UeSt --- docs/harness-containment.md | 20 ++++--- src/worker/harness-runner.test.ts | 96 ++++++++++++++++++++++++++++++- src/worker/harness-runner.ts | 47 ++++++++++----- 3 files changed, 138 insertions(+), 25 deletions(-) diff --git a/docs/harness-containment.md b/docs/harness-containment.md index ca38a9d..e49b0f2 100644 --- a/docs/harness-containment.md +++ b/docs/harness-containment.md @@ -53,15 +53,17 @@ hold it together: backgrounds a grandchild, and must show that the grandchild's pid is gone and its sentinel file stops changing after the timeout. `src/worker/harness-containment-registry.test.ts` fails if an adapter ships without a - conformance call. The deterministic station runner, a separate path from harness - stations, passes the same suite. It also runs the suite's exit scenarios: when the - station's command exits 0 or nonzero before its timeout, the runner kills the command's - process group before it returns, so a grandchild does not outlive a station that finished - on its own ([#10](https://github.com/theaiteam-dev/conduit/issues/10), - [#17](https://github.com/theaiteam-dev/conduit/issues/17)). A harness adapter kills the - group on timeout only. Both runners also kill every live station process group when the - kernel receives SIGINT, SIGTERM or SIGHUP, or exits; a SIGKILLed kernel cannot do this, - which is why the container boundary below still matters. + conformance call. The deterministic station runner and the harness runner, two separate + spawn paths, both pass the same suite, and both also run its exit scenarios: when the + command exits 0 or nonzero before its timeout, the runner kills the command's process + group before it returns, so a grandchild does not outlive a station or a harness + invocation that finished on its own + ([#10](https://github.com/theaiteam-dev/conduit/issues/10), + [#17](https://github.com/theaiteam-dev/conduit/issues/17)). For the harness runner this + also keeps a grandchild that inherited the stdout/stderr pipes from stalling the output + drains past its actual finish. Both runners also kill every live station process group + when the kernel receives SIGINT, SIGTERM or SIGHUP, or exits; a SIGKILLed kernel cannot do + this, which is why the container boundary below still matters. Deployment guidance additionally recommends running the container as a dedicated non-root user for agentic/harness flows. - **The ADR-0003 container wall.** [ADR-0003](../adr/0003-packaging-and-distribution.md)'s diff --git a/src/worker/harness-runner.test.ts b/src/worker/harness-runner.test.ts index 6b22abb..7e75917 100644 --- a/src/worker/harness-runner.test.ts +++ b/src/worker/harness-runner.test.ts @@ -15,15 +15,20 @@ * 3. a child that spawns a grandchild is FULLY reaped on timeout — the recorded * grandchild pid is dead afterward (process-GROUP kill, not child.kill). * 4. cwd is set inside the project root; a cwd resolving OUTSIDE it is rejected. + * 5. (#17) a grandchild backgrounded by a harness that exits on its own (0 or + * nonzero, well before the timeout) is reaped too, and does not stall the + * output drains: the runner returns promptly with the output written + * before the exit, not at timeoutMs. */ import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; -import { mkdtempSync, mkdirSync, rmSync, readFileSync, realpathSync, existsSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, rmSync, readFileSync, writeFileSync, realpathSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { runHarnessProcess, type HarnessRunnerConfig, } from './harness-runner'; +import { describeContainmentConformance } from './harness-containment.conformance'; // --------------------------------------------------------------------------- // Fixtures: an isolated project root per test + tracked pids for cleanup so a @@ -334,3 +339,92 @@ describe('harness runner: stdout line filter', () => { expect(result.stdout).toBe('a\nb\n'); }); }); + +// --------------------------------------------------------------------------- +// Containment conformance (issue #17, mirroring runDeterministic). The runner +// spawns the harness as its own process group and must SIGKILL the group after +// the leader exits, not only on timeout, so the fixture's grandchild dies on +// every exit path. +// --------------------------------------------------------------------------- + +/** Test-local label for `result.timedOut === true`. HarnessSpawnResult carries a boolean. */ +const HARNESS_RUNNER_TIMEOUT_LABEL = 'timedOut'; + +describeContainmentConformance( + 'harness-runner', + async ({ projectRoot, fixture, timeoutMs, fixtureArgs }) => { + const result = await runHarnessProcess( + { command: fixture, args: fixtureArgs }, + { projectRoot, timeoutMs }, + ); + return result.timedOut === true ? HARNESS_RUNNER_TIMEOUT_LABEL : undefined; + }, + { timeoutClass: HARNESS_RUNNER_TIMEOUT_LABEL, reapsOnExit: true }, +); + +// --------------------------------------------------------------------------- +// #17: a descendant that keeps the inherited stdout/stderr pipes open. The +// conformance fixture above sends its grandchild's stdio to /dev/null, so it +// proves reaping but not the drain stall; this script does not redirect, so a +// surviving grandchild would keep `Promise.all`'s drains pending until +// timeoutMs. The post-exit group kill must close the pipes so the runner +// returns promptly, and output written before the exit must still be captured. +// --------------------------------------------------------------------------- + +describe('runHarnessProcess: descendants holding the output pipes (#17)', () => { + /** Grandchild pids a test recorded; afterEach SIGKILLs any that survived. */ + const recorded: number[] = []; + + afterEach(() => { + // Single pids only, never a group: a leaked grandchild of an undetached + // spawn shares the test runner's process group. + for (const pid of recorded.splice(0)) { + try { + process.kill(pid, 'SIGKILL'); + } catch { + /* already gone: the expected case */ + } + } + }); + + /** + * A script that writes to stdout and stderr, backgrounds a `sleep 30` that + * inherits both pipes, records its pid, then exits with `tail`. + */ + function pipeHoldingScript(tail: string): string { + const script = join(projectRoot, 'hold.sh'); + writeFileSync( + script, + ['echo before-out', 'echo before-err >&2', 'sleep 30 &', 'echo "$!" > grandchild.pid', tail, ''].join('\n'), + ); + return script; + } + + function grandchildPid(): number { + const pid = Number(readFileSync(join(projectRoot, 'grandchild.pid'), 'utf-8').trim()); + expect(Number.isInteger(pid) && pid > 1).toBe(true); + recorded.push(pid); + return pid; + } + + for (const [label, code] of [ + ['exits 0', 0], + ['exits nonzero', 4], + ] as const) { + it(`returns promptly with the output written before it ${label}, and the grandchild is gone`, async () => { + const script = pipeHoldingScript(`exit ${code}`); + const start = Date.now(); + // A generous timeout the fix must beat by a wide margin: without the + // post-exit group kill, the drains wait out the full 30s sleep instead. + const result = await runHarnessProcess({ command: 'sh', args: [script] }, config({ timeoutMs: 60_000 })); + const elapsed = Date.now() - start; + + expect(elapsed).toBeLessThan(10_000); + expect(result.exitCode).toBe(code); + expect(result.timedOut).toBe(false); + expect(result.stdout).toBe('before-out\n'); + expect(result.stderr).toBe('before-err\n'); + expect(await waitForPidGone(grandchildPid(), 3_000)).toBe(true); + }, 40_000); + } +}); diff --git a/src/worker/harness-runner.ts b/src/worker/harness-runner.ts index a2b2c92..699c917 100644 --- a/src/worker/harness-runner.ts +++ b/src/worker/harness-runner.ts @@ -10,9 +10,12 @@ * Bun.spawn's native `timeout`/`killSignal` kills only the immediate child: a * grandchild the harness backgrounds (a common agent-CLI pattern) reparents to * init and survives. This runner instead spawns the child DETACHED (`setsid()`, - * so the child's pid becomes its own process-group id) and, on timeout, sends - * SIGKILL to the negative pid, which is the whole group, grandchildren included - * (`killProcessGroup` in ./process-group.ts, shared with runDeterministic). + * so the child's pid becomes its own process-group id) and sends SIGKILL to the + * negative pid, which is the whole group, grandchildren included + * (`killProcessGroup` in ./process-group.ts, shared with runDeterministic): + * on timeout, AND again once the harness leader has exited on its own (#17): + * a backgrounded grandchild that inherited the stdout/stderr pipes would + * otherwise keep them open and stall the drains until the timeout fires. * * Do NOT apply a command allowlist here — the harness binary comes from * trusted engine config (WI-560); the `tools` allowlist is enforced @@ -154,9 +157,13 @@ function resolveConfinedCwd(projectRoot: string, cwd: string | undefined): strin } /** - * Spawn `cmd` bounded by `config.timeoutMs`. On timeout, SIGKILLs the whole - * process group (setsid-detached child) so backgrounded grandchildren are - * reaped too, and resolves with `timedOut: true` rather than a normal exit. + * Spawn `cmd` bounded by `config.timeoutMs`. SIGKILLs the whole process group + * (setsid-detached child) after the leader exits, on EVERY exit path, not only + * on timeout (#17): a harness that finishes on its own but left a grandchild + * backgrounded is reaped just the same, before the output drains are awaited, + * so that grandchild cannot stall the runner until timeoutMs by holding the + * inherited stdout/stderr pipes open. Only a genuine timeout resolves with + * `timedOut: true`; a post-exit kill never marks a normal exit as one. */ export async function runHarnessProcess( cmd: HarnessCommand, @@ -184,22 +191,32 @@ export async function runHarnessProcess( killProcessGroup(proc.pid); }, config.timeoutMs); + // Start draining now so a harness that writes more than a pipe buffer is not + // blocked on a full pipe while we wait for it to exit. + const stdoutText = + config.stdoutLineFilter !== undefined + ? readKeptLines(proc.stdout as ReadableStream, config.stdoutLineFilter) + : new Response(proc.stdout).text(); + const stderrText = new Response(proc.stderr as ReadableStream).text(); + let exitCode: number; - let stdout: string; - let stderr: string; try { - [exitCode, stdout, stderr] = await Promise.all([ - proc.exited, - config.stdoutLineFilter !== undefined - ? readKeptLines(proc.stdout as ReadableStream, config.stdoutLineFilter) - : new Response(proc.stdout).text(), - new Response(proc.stderr as ReadableStream).text(), - ]); + exitCode = await proc.exited; } finally { clearTimeout(timer); + // #17: the harness has exited, but a descendant it backgrounded may still + // be running and holding the pipes. Kill the group BEFORE awaiting the + // drains below so they settle. Bytes already written stay readable, so no + // output is lost. The leader has already exited, so this does not change + // its exit status or signalCode. While any member is alive the group id + // cannot be reused, so the signal reaches only this harness's descendants; + // an empty group is ESRCH. + killProcessGroup(proc.pid); untrackProcessGroup(proc.pid); } + const [stdout, stderr] = await Promise.all([stdoutText, stderrText]); + const durationMs = Date.now() - startedAt; // Distinguish OUR timeout-kill from a normal exit. A shared flag set independently From 7966713d6735f7b360d3955e90668283d8f4306f Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Fri, 25 Sep 2026 04:19:59 +0000 Subject: [PATCH 4/7] Address PR #65 review feedback (pass 2) - Test: the containment exit scenarios bound elapsed time by the scenario timeout (10s) instead of a separate 5s bound, so a loaded CI host cannot fail them while a path that waited on its timeout still does. Addresses review comments from github-actions (nitpick). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RbrsaSXMxkbrk8gdd1UeSt --- src/worker/harness-containment.conformance.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/worker/harness-containment.conformance.ts b/src/worker/harness-containment.conformance.ts index e5904ff..0c904a2 100644 --- a/src/worker/harness-containment.conformance.ts +++ b/src/worker/harness-containment.conformance.ts @@ -106,11 +106,11 @@ export interface ContainmentConformanceOptions { /** * The wall-clock bound for the exit scenarios. The fixture exits on its own - * long before this, so a path that returns near it waited on something the - * exit should have ended. + * long before this, so a path that takes this long waited on its timeout + * instead of returning when the worker exited. The elapsed-time check uses + * this bound, not a tighter one, so a loaded CI host cannot fail it. */ const EXIT_SCENARIO_TIMEOUT_MS = 10_000; -const EXIT_SCENARIO_PROMPT_MS = 5_000; /** True while `pid` exists (signal 0 checks for existence without delivering a signal). */ function isAlive(pid: number): boolean { @@ -303,7 +303,7 @@ export function describeContainmentConformance( fixtureArgs: containmentFixtureExitArgs(code), }); - expect(Date.now() - startedAt).toBeLessThan(EXIT_SCENARIO_PROMPT_MS); + expect(Date.now() - startedAt).toBeLessThan(EXIT_SCENARIO_TIMEOUT_MS); // A worker that exited on its own was not timed out, even though // the path killed its process group afterwards. expect(timeoutClass).toBeUndefined(); From 04f8e1c58b9c72ae37dcbe491c1fe08fe9330472 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Fri, 25 Sep 2026 04:30:34 +0000 Subject: [PATCH 5/7] Address PR #65 review feedback (pass 3) - Fix: runHarnessProcess and runDeterministic attach a rejection handler to the output drains as soon as they start, so a drain that rejects while the leader is still running (a throwing stdoutLineFilter) is rethrown after the group kill instead of surfacing as an unhandled rejection. - Test: a throwing stdoutLineFilter rejects runHarnessProcess with the filter's error and raises no unhandledRejection. Addresses review comments from coderabbitai. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RbrsaSXMxkbrk8gdd1UeSt --- src/worker/deterministic.ts | 6 +++- src/worker/harness-runner.test.ts | 47 +++++++++++++++++++++++++++++++ src/worker/harness-runner.ts | 7 ++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/worker/deterministic.ts b/src/worker/deterministic.ts index f737780..c041168 100644 --- a/src/worker/deterministic.ts +++ b/src/worker/deterministic.ts @@ -226,6 +226,10 @@ export async function runDeterministic( // blocked on a full pipe while we wait for it to exit. const stdoutText = new Response(proc.stdout).text(); const stderrText = new Response(proc.stderr as ReadableStream).text(); + // Attach a handler now: a drain that rejects while we await proc.exited + // would otherwise be an unhandled rejection. `await drains` below rethrows it. + const drains = Promise.all([stdoutText, stderrText]); + drains.catch(() => {}); let exitCode: number; try { @@ -242,7 +246,7 @@ export async function runDeterministic( untrackProcessGroup(proc.pid); } - const [stdout, stderr] = await Promise.all([stdoutText, stderrText]); + const [stdout, stderr] = await drains; // Distinguish OUR timeout-kill from a normal exit or an unrelated SIGKILL // (OOM-killer, a child that self-kills). Both must hold: our timer fired, and diff --git a/src/worker/harness-runner.test.ts b/src/worker/harness-runner.test.ts index 7e75917..397b29a 100644 --- a/src/worker/harness-runner.test.ts +++ b/src/worker/harness-runner.test.ts @@ -428,3 +428,50 @@ describe('runHarnessProcess: descendants holding the output pipes (#17)', () => }, 40_000); } }); + +// --------------------------------------------------------------------------- +// A `stdoutLineFilter` that throws before `proc.exited` resolves. The filter +// runs inside `readKeptLines`'s `for await` loop as lines arrive, so a filter +// that throws on an early line rejects the stdout drain promise while the +// child is still running (the `sleep` below keeps `proc.exited` pending). +// Before the fix, that promise had no handler attached until after the +// process-group kill, so the rejection could go unhandled in the gap; bun:test +// treats an unhandled rejection as a failure independent of what this function +// returns. +// --------------------------------------------------------------------------- + +describe('runHarnessProcess: a throwing stdoutLineFilter does not produce an unhandled rejection', () => { + it('rejects with the filter error, and the drain rejection is never unhandled', async () => { + const filterError = new Error('stdoutLineFilter boom'); + const throwingFilter = (): boolean => { + throw filterError; + }; + + let unhandled: unknown; + const onUnhandledRejection = (reason: unknown) => { + unhandled = reason; + }; + process.on('unhandledRejection', onUnhandledRejection); + + try { + await expect( + runHarnessProcess( + // Prints a line immediately (the filter throws on it), then keeps the + // process alive briefly so proc.exited has not resolved yet when the + // filter throws. + { command: 'sh', args: ['-c', 'echo x; sleep 1'] }, + config({ timeoutMs: 5_000, stdoutLineFilter: throwingFilter }), + ), + ).rejects.toBe(filterError); + + // Let the event loop settle so a rejection that only becomes unhandled + // after this test's assertions (e.g. once the group kill finally runs) + // has had a chance to fire the listener above. + await new Promise((resolve) => setTimeout(resolve, 200)); + + expect(unhandled).toBeUndefined(); + } finally { + process.off('unhandledRejection', onUnhandledRejection); + } + }); +}); diff --git a/src/worker/harness-runner.ts b/src/worker/harness-runner.ts index 699c917..2433257 100644 --- a/src/worker/harness-runner.ts +++ b/src/worker/harness-runner.ts @@ -198,6 +198,11 @@ export async function runHarnessProcess( ? readKeptLines(proc.stdout as ReadableStream, config.stdoutLineFilter) : new Response(proc.stdout).text(); const stderrText = new Response(proc.stderr as ReadableStream).text(); + // Attach a handler now: a throwing `stdoutLineFilter` rejects its drain while + // proc.exited is still pending, which would otherwise be an unhandled + // rejection. `await drains` below rethrows it after the group kill. + const drains = Promise.all([stdoutText, stderrText]); + drains.catch(() => {}); let exitCode: number; try { @@ -215,7 +220,7 @@ export async function runHarnessProcess( untrackProcessGroup(proc.pid); } - const [stdout, stderr] = await Promise.all([stdoutText, stderrText]); + const [stdout, stderr] = await drains; const durationMs = Date.now() - startedAt; From 32046e3416a903be8e27e787682d1be89372a689 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Fri, 25 Sep 2026 04:37:57 +0000 Subject: [PATCH 6/7] Address PR #65 review feedback (pass 4) - Test: the "does not report a timeout after a normal exit" test asserts the runner returns well before its 20s timeout, so it fails when the post-exit group kill is removed. Before, every assertion also held on the timer-kill path. Addresses review comments from github-actions (nitpick). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RbrsaSXMxkbrk8gdd1UeSt --- src/worker/deterministic.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/worker/deterministic.test.ts b/src/worker/deterministic.test.ts index c9a8f49..18fdff5 100644 --- a/src/worker/deterministic.test.ts +++ b/src/worker/deterministic.test.ts @@ -395,10 +395,16 @@ describe('runDeterministic: descendants holding the output pipes (#10, #17)', () it('does not report a timeout when the group is killed after a normal exit', async () => { const script = pipeHoldingScript('exit 0'); + const start = Date.now(); const result = await runDeterministic( { command: 'sh', args: [script] }, { allowlist: ['sh'], cwd: dir, timeoutMs: 20_000 }, ); + + // Without the post-exit group kill the drains wait for the 20s timer, + // which reaps the grandchild and reports no timeout, so only the elapsed + // time tells the two apart. + expect(Date.now() - start).toBeLessThan(10_000); expect(result.ok).toBe(true); expect(result.exitCode).toBe(0); expect(result.timedOut).toBeFalsy(); From 606be4a38cc1c5c9810e1f242e92046225157981 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Fri, 25 Sep 2026 04:45:35 +0000 Subject: [PATCH 7/7] Address PR #65 review feedback (pass 5) - Fix: containment-signal-runner clears its pid poll once the station returns, so a station that returns without the fixture recording its pid lets the stand-in kernel exit and the test fail fast. Addresses review comments from github-actions (nitpick). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RbrsaSXMxkbrk8gdd1UeSt --- src/worker/containment-signal-runner.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/worker/containment-signal-runner.ts b/src/worker/containment-signal-runner.ts index 3cf701c..2d425dc 100644 --- a/src/worker/containment-signal-runner.ts +++ b/src/worker/containment-signal-runner.ts @@ -37,4 +37,7 @@ if (runner === 'deterministic') { } else { await runHarnessProcess({ command: fixture, args: [] }, { projectRoot, timeoutMs }); } +// A fixture that never recorded its pid leaves the poll running, which would +// keep this process alive after the station returned. +clearInterval(poll); console.log('station returned');