diff --git a/docs/harness-containment.md b/docs/harness-containment.md index d8b5484..e49b0f2 100644 --- a/docs/harness-containment.md +++ b/docs/harness-containment.md @@ -53,10 +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 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 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/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/containment-signal-runner.ts b/src/worker/containment-signal-runner.ts new file mode 100644 index 0000000..2d425dc --- /dev/null +++ b/src/worker/containment-signal-runner.ts @@ -0,0 +1,43 @@ +/** + * 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 }); +} +// 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'); diff --git a/src/worker/deterministic.test.ts b/src/worker/deterministic.test.ts index 2bc2444..18fdff5 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,143 @@ 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 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(); + 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..c041168 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, trackProcessGroup, untrackProcessGroup } 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,67 @@ 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 } : {}), }); + // 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 + // 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(); + // 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 { + 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 drains; - 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). + // 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..0c904a2 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 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; + /** 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_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(); + // 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.test.ts b/src/worker/harness-runner.test.ts index 6b22abb..397b29a 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,139 @@ 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); + } +}); + +// --------------------------------------------------------------------------- +// 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 58d6267..2433257 100644 --- a/src/worker/harness-runner.ts +++ b/src/worker/harness-runner.ts @@ -7,12 +7,15 @@ * (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 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 @@ -22,6 +25,7 @@ import { resolve, sep } from 'node:path'; import { existsSync, statSync } from 'node:fs'; +import { killProcessGroup, trackProcessGroup, untrackProcessGroup } from './process-group'; /** The command + argv to spawn (no shell — array form, per the Law-lite pattern). */ export interface HarnessCommand { @@ -153,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, @@ -175,29 +183,48 @@ 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(() => { - 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([ - proc.exited, + // 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(), - new Response(proc.stderr as ReadableStream).text(), - ]); - clearTimeout(timer); + : 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 { + 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 drains; 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.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 new file mode 100644 index 0000000..6be6abb --- /dev/null +++ b/src/worker/process-group.ts @@ -0,0 +1,110 @@ +/** + * 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. + * + * 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. + */ + +/** + * 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 */ + } +} + +/** 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(); +} 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