From a16305a05886eeea9580fcaed739c4b162ab95a5 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 09:00:09 -0700 Subject: [PATCH 1/5] fix(seats): the supervisor kills a stalled turn's whole process group killChildren() sent SIGKILL to the direct child only, so an agent CLI's own children (MCP servers started through npx, the native binary behind codex's node launcher) survived the kill and were orphaned on the shared host. - process-group.js: spawnInOwnGroup starts a child detached and marks it (LEADS_PROCESS_GROUP). killProcessTree signals -pid only for a marked child that has not been reaped, and otherwise falls back to child.kill. Leading a group is never inferred. - The supervisor passes ctx.ownProcessGroup = true and kills through killProcessTree. - The claude and codex adapters spawn through spawnInOwnGroup and ask for a group only when ctx.ownProcessGroup === true. Unsupervised, the options object reaches spawn untouched, so live seats spawn exactly as before. - Wrapper exit: a detached group is outside the seat's launchd group. While a supervised group is live, the wrapper kills every tracked group on SIGTERM, SIGINT, SIGHUP and exit. When nothing else handles the signal, it re-raises the signal so the wrapper still dies by it. Tests use real processes: a grandchild left by `sleep 300 &` is gone after a stall kill and after the wrapper is signalled or exits, a child that does not lead a group gets child.kill with no negative pid, and the unsupervised claude/codex spawn carries no detached option. Mutation check: reverting killChildren to child.kill alone fails the grandchild test. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu --- cli/__tests__/adapters.process-group.test.mjs | 67 +++++ cli/src/lib/adapters/claude.js | 11 +- cli/src/lib/adapters/codex.js | 7 +- packages/seats/src/index.js | 4 + packages/seats/src/process-group.js | 136 ++++++++++ packages/seats/src/supervisor.js | 18 +- .../seats/test/supervisor-group-kill.test.js | 239 ++++++++++++++++++ 7 files changed, 474 insertions(+), 8 deletions(-) create mode 100644 cli/__tests__/adapters.process-group.test.mjs create mode 100644 packages/seats/src/process-group.js create mode 100644 packages/seats/test/supervisor-group-kill.test.js diff --git a/cli/__tests__/adapters.process-group.test.mjs b/cli/__tests__/adapters.process-group.test.mjs new file mode 100644 index 0000000000..715b5becd4 --- /dev/null +++ b/cli/__tests__/adapters.process-group.test.mjs @@ -0,0 +1,67 @@ +/** + * The claude and codex adapters start their child in its own process group + * only when the run supervisor asks (ctx.ownProcessGroup), because only the + * supervisor kills the group. Live seats run unsupervised, and their spawn + * options must stay exactly what they were. + */ +import { jest } from '@jest/globals'; +import { EventEmitter } from 'events'; + +await jest.unstable_mockModule('child_process', () => ({ + spawnSync: jest.fn(() => ({ status: 1, stdout: '', stderr: '' })), + spawn: jest.fn(), +})); + +const claude = (await import('../src/lib/adapters/claude.js')).default; +const codex = (await import('../src/lib/adapters/codex.js')).default; +const { LEADS_PROCESS_GROUP, OWN_PROCESS_GROUP } = await import('@commonlyai/seats/process-group'); + +const makeSpawnImpl = () => { + const calls = []; + const impl = (cmd, args, opts) => { + const proc = new EventEmitter(); + proc.stdout = new EventEmitter(); + proc.stderr = new EventEmitter(); + proc.kill = jest.fn(); + calls.push({ cmd, args, opts, proc }); + setTimeout(() => { + proc.stdout.emit('data', Buffer.from('ok')); + proc.emit('close', 0); + }, 0); + return proc; + }; + return { impl, calls }; +}; + +const ADAPTERS = [['claude', claude], ['codex', codex]]; +const POSIX = process.platform !== 'win32'; + +describe.each(ADAPTERS)('%s adapter — process group', (name, adapter) => { + test('unsupervised: spawns with exactly cwd, env and stdio, no detached, no marker', async () => { + const { impl, calls } = makeSpawnImpl(); + await adapter.spawn('hi', { environment: { model: 'test-model' }, sessionId: null, _spawnImpl: impl }); + expect(calls).toHaveLength(1); + expect(Object.keys(calls[0].opts).sort()).toEqual(['cwd', 'env', 'stdio']); + expect(calls[0].opts.stdio).toEqual(['ignore', 'pipe', 'pipe']); + expect(calls[0].proc[LEADS_PROCESS_GROUP]).toBeUndefined(); + }); + + (POSIX ? test : test.skip)('supervised: spawns detached and marks the child as leading its group', async () => { + const { impl, calls } = makeSpawnImpl(); + await adapter.spawn('hi', { + environment: { model: 'test-model' }, sessionId: null, _spawnImpl: impl, [OWN_PROCESS_GROUP]: true, + }); + expect(calls).toHaveLength(1); + expect(calls[0].opts.detached).toBe(true); + expect(Object.keys(calls[0].opts).sort()).toEqual(['cwd', 'detached', 'env', 'stdio']); + expect(calls[0].proc[LEADS_PROCESS_GROUP]).toBe(true); + }); + + test('a truthy non-true value does not opt in', async () => { + const { impl, calls } = makeSpawnImpl(); + await adapter.spawn('hi', { + environment: { model: 'test-model' }, sessionId: null, _spawnImpl: impl, [OWN_PROCESS_GROUP]: 'yes', + }); + expect(calls[0].opts).not.toHaveProperty('detached'); + }); +}); diff --git a/cli/src/lib/adapters/claude.js b/cli/src/lib/adapters/claude.js index d7e0b7abad..02fab7bf77 100644 --- a/cli/src/lib/adapters/claude.js +++ b/cli/src/lib/adapters/claude.js @@ -1,5 +1,6 @@ import { claudeRuntimeInventory } from '../native-runtime-inventory.js'; import { observeSpawn, controlsFromArguments } from '../runtime-observation.js'; +import { spawnInOwnGroup, wantsOwnProcessGroup } from '@commonlyai/seats/process-group'; /** * claude adapter — wraps the local `claude` CLI as a Commonly agent. * @@ -291,12 +292,14 @@ const publicSkillPromptArgs = async (skillSources) => { return { args: text ? ['--append-system-prompt', text] : [], included }; }; -const runClaude = ({ cmd, args, cwd, env, timeoutMs, credentials = [], spawnImpl = childSpawn, onSpawn, controls = {} }) => new Promise((resolve, reject) => { - const proc = spawnImpl(cmd, args, { +// `ownGroup` is set only under the run supervisor, which kills the child's +// whole group; unsupervised, the spawn options are exactly what they were. +const runClaude = ({ cmd, args, cwd, env, timeoutMs, credentials = [], spawnImpl = childSpawn, onSpawn, controls = {}, ownGroup = false }) => new Promise((resolve, reject) => { + const proc = spawnInOwnGroup(spawnImpl, cmd, args, { cwd, env, stdio: ['ignore', 'pipe', 'pipe'], - }); + }, { ownGroup }); observeSpawn(proc, onSpawn, controls); let stdout = ''; let stderr = ''; @@ -906,6 +909,7 @@ export default { // the value a tail can echo back. credentials: spawnCredentials(spawnCtx), spawnImpl: ctx._spawnImpl, onSpawn: ctx.onSpawn, controls: controlsFromArguments(modelArgs, { modelFlag: '--model', effortFlag: '--effort' }), // test seam only — do not use in production + ownGroup: wantsOwnProcessGroup(ctx), }); // TASK-440: the context this turn left, read from the transcript the // child wrote under its own env (an isolated HOME for public seats). @@ -957,6 +961,7 @@ export default { timeoutMs: ctx.timeoutMs || DEFAULT_TIMEOUT_MS, credentials: spawnCredentials(retryCtx), spawnImpl: ctx._spawnImpl, onSpawn: ctx.onSpawn, controls: controlsFromArguments(modelArgs, { modelFlag: '--model', effortFlag: '--effort' }), + ownGroup: wantsOwnProcessGroup(ctx), }); const contextTokens = claudeContextTokens({ cwd: ctx.cwd, sessionId: freshId, diff --git a/cli/src/lib/adapters/codex.js b/cli/src/lib/adapters/codex.js index f55cd44100..48588bca5f 100644 --- a/cli/src/lib/adapters/codex.js +++ b/cli/src/lib/adapters/codex.js @@ -1,5 +1,6 @@ import { codexRuntimeInventory } from '../native-runtime-inventory.js'; import { observeSpawn, controlsFromArguments } from '../runtime-observation.js'; +import { spawnInOwnGroup, wantsOwnProcessGroup } from '@commonlyai/seats/process-group'; /** * codex adapter — wraps the local `codex` CLI as a Commonly agent. * @@ -572,14 +573,15 @@ const CODEX_PROVIDER_KEY_ENVS = [ 'CODEX_API_KEY', ]; -const runCodex = ({ args, cwd, env, timeoutMs, credentials = [], spawnImpl = childSpawn, onSpawn, controls = {} }) => new Promise((resolve, reject) => { // stdio: ['ignore', 'pipe', 'pipe'] — without this, child_process.spawn +// `ownGroup`: see runClaude in claude.js. +const runCodex = ({ args, cwd, env, timeoutMs, credentials = [], spawnImpl = childSpawn, onSpawn, controls = {}, ownGroup = false }) => new Promise((resolve, reject) => { // stdio: ['ignore', 'pipe', 'pipe'] — without this, child_process.spawn // defaults stdin to a fresh pipe. Codex 0.125.0's `exec` then blocks on // `Reading additional input from stdin...` because it sees an open pipe // and waits for input that never arrives. Interactive runs are fine because // codex detects a TTY and uses the argv prompt directly. Setting stdin to // `'ignore'` gives codex /dev/null → immediate EOF → it falls back to the // argv prompt as intended. Surfaced live during ADR-005 Phase 2 smoke. - const proc = spawnImpl('codex', args, { cwd, env, stdio: ['ignore', 'pipe', 'pipe'] }); + const proc = spawnInOwnGroup(spawnImpl, 'codex', args, { cwd, env, stdio: ['ignore', 'pipe', 'pipe'] }, { ownGroup }); observeSpawn(proc, onSpawn, controls); let stderr = ''; let timedOut = false; @@ -740,6 +742,7 @@ export default { // nothing left to compare a tail against. credentials: spawnCredentials(ctx, CODEX_PROVIDER_KEY_ENVS), spawnImpl: ctx._spawnImpl, onSpawn: ctx.onSpawn, controls: controlsFromArguments(args.slice(args.indexOf('exec') + (args[args.indexOf('exec') + 1] === 'resume' ? 3 : 1), -1), { modelFlag: '--model', valueFlags: ['-c', '-o', '--permission-profile'], codex: true }), // test seam only — do not use in production + ownGroup: wantsOwnProcessGroup(ctx), }); let text = ''; diff --git a/packages/seats/src/index.js b/packages/seats/src/index.js index 6f0be90d3c..9ee054c4e0 100644 --- a/packages/seats/src/index.js +++ b/packages/seats/src/index.js @@ -1,6 +1,10 @@ export { createRegistry, providerKeyEnvNames } from './contract.js'; export { requireSeatModel } from './seat-model.js'; export { controlsFromArguments, observeSpawn } from './runtime-observation.js'; +export { + LEADS_PROCESS_GROUP, OWN_PROCESS_GROUP, spawnInOwnGroup, wantsOwnProcessGroup, leadsOwnGroup, killProcessTree, + trackSupervisedGroup, killSupervisedGroups, +} from './process-group.js'; export { adapterFailure, spawnCredentials } from './upstream-refusal.js'; export { default as stub } from './adapters/stub.js'; export { createHermesAdapter } from './adapters/hermes.js'; diff --git a/packages/seats/src/process-group.js b/packages/seats/src/process-group.js new file mode 100644 index 0000000000..fea2b03c83 --- /dev/null +++ b/packages/seats/src/process-group.js @@ -0,0 +1,136 @@ +/** + * Process groups for supervised children. + * + * An agent CLI spawns its own children: MCP servers through `npx`, a native + * binary behind a node launcher, shells for tools. Killing only the CLI leaves + * those running, holding the run's work and keys on a shared host. A child the + * supervisor will kill is therefore started as the leader of its own process + * group, and killed as a group. + * + * Leading a group is never inferred. The spawner that passed `detached` marks + * the child, and only a marked child that is still alive is ever signalled by + * negative pid: signalling `-pid` for a process that does not lead its group + * can reach an unrelated group. + * + * A child in its own group is outside the wrapper's group, so launchd's kill + * of the seat's group on exit no longer reaches it. `trackSupervisedGroup` + * covers that: while any tracked group is live, the wrapper kills them all + * when it receives SIGTERM, SIGINT or SIGHUP, and when it exits. + */ + +/** Set on a child by the code that spawned it with `detached: true`. */ +export const LEADS_PROCESS_GROUP = Symbol.for('@commonlyai/seats.leadsProcessGroup'); + +/** Whether a child can lead its own group here (POSIX only). */ +export const PROCESS_GROUPS_SUPPORTED = process.platform !== 'win32'; + +/** + * The ctx key the supervisor sets to tell an adapter it will kill the child's + * group, so the adapter may start the child in one. Unset, an adapter spawns + * exactly as it would unsupervised. + */ +export const OWN_PROCESS_GROUP = 'ownProcessGroup'; + +export const wantsOwnProcessGroup = (ctx) => ctx?.[OWN_PROCESS_GROUP] === true && PROCESS_GROUPS_SUPPORTED; + +/** + * Spawn through `spawnImpl`. With `ownGroup`, the child is started detached + * (it calls setsid, so it leads a new group whose id is its pid) and marked. + * Without it, `options` reach `spawnImpl` untouched. + */ +export const spawnInOwnGroup = (spawnImpl, command, args, options, { ownGroup = false } = {}) => { + if (!ownGroup || !PROCESS_GROUPS_SUPPORTED) return spawnImpl(command, args, options); + const child = spawnImpl(command, args, { ...options, detached: true }); + if (child && typeof child === 'object') child[LEADS_PROCESS_GROUP] = true; + return child; +}; + +const isAlive = (child) => child.exitCode === null && child.signalCode === null; + +/** + * True only for a child its spawner marked, with a real pid, that has not been + * reaped. Node sets exitCode/signalCode in the same tick it reaps, so while + * this holds the pid (and so the group id) cannot have been reused. + */ +export const leadsOwnGroup = (child) => !!child + && child[LEADS_PROCESS_GROUP] === true + && Number.isInteger(child.pid) && child.pid > 0 + && isAlive(child); + +/** + * Kill a child, and its whole group when it leads one. Falls back to + * `child.kill` when the child does not lead a group, or the group signal fails. + * Returns whether a signal was delivered. + */ +export const killProcessTree = (child, signal = 'SIGKILL') => { + if (leadsOwnGroup(child)) { + try { process.kill(-child.pid, signal); return true; } catch { /* fall back to the leader */ } + } + try { return child.kill(signal) !== false; } catch { return false; } +}; + +// ── Wrapper-exit cleanup ──────────────────────────────────────────────────── + +const EXIT_SIGNALS = ['SIGTERM', 'SIGINT', 'SIGHUP']; +const tracked = new Set(); +const handlers = new Map(); +let onExit = null; + +/** SIGKILL every tracked group still led by a live child. Synchronous. */ +export const killSupervisedGroups = (signal = 'SIGKILL') => { + let killed = 0; + for (const child of tracked) { + if (!leadsOwnGroup(child)) continue; + try { process.kill(-child.pid, signal); killed += 1; } catch { /* already gone */ } + } + return killed; +}; + +const uninstall = () => { + for (const [signal, handler] of handlers) process.removeListener(signal, handler); + handlers.clear(); + if (onExit) process.removeListener('exit', onExit); + onExit = null; +}; + +const install = () => { + if (onExit) return; + onExit = () => { killSupervisedGroups(); }; + process.on('exit', onExit); + for (const signal of EXIT_SIGNALS) { + const handler = () => { + killSupervisedGroups(); + // Listening for a signal replaces its default action. When nothing else + // handles it, restore that action and re-raise, so the wrapper still + // terminates exactly as it would have without this module. + if (process.listenerCount(signal) === 1) { + uninstall(); + process.kill(process.pid, signal); + } + }; + handlers.set(signal, handler); + process.on(signal, handler); + } +}; + +const untrack = (child) => { + tracked.delete(child); + if (tracked.size === 0) uninstall(); +}; + +/** + * Track a group-leading child until it exits. The handlers exist only while a + * tracked child is live, so a wrapper with no supervised group in flight keeps + * its signal behaviour unchanged. A child that does not lead a group is + * ignored: launchd's group kill already reaches it. + */ +export const trackSupervisedGroup = (child) => { + if (!leadsOwnGroup(child) || tracked.has(child)) return false; + tracked.add(child); + install(); + child.once?.('exit', () => untrack(child)); + return true; +}; + +/** For tests: how many groups are tracked. */ +export const trackedGroupCount = () => tracked.size; diff --git a/packages/seats/src/supervisor.js b/packages/seats/src/supervisor.js index 513a420432..3604a271ed 100644 --- a/packages/seats/src/supervisor.js +++ b/packages/seats/src/supervisor.js @@ -8,9 +8,16 @@ * the adapter emits, and any byte on the stdout/stderr of a child the adapter * reports through `ctx.onSpawn`. A run that is busy but quiet past the window * is indistinguishable from a hung one, which is why the window is long. + * + * The adapter is told (ctx.ownProcessGroup) that it may start its child as the + * leader of a new process group; a child it marks as one is killed as a group, + * so the CLI's own children (MCP servers, a native binary behind a launcher) + * die with it. See process-group.js, which also kills those groups when the + * wrapper itself is signalled or exits. */ import { RunFailure, runFailure, failureKindOf, FAILURE_KINDS } from './failures.js'; import { redactKnownCredentials, scrubAdapterFailure, spawnCredentials } from './upstream-refusal.js'; +import { killProcessTree, trackSupervisedGroup, OWN_PROCESS_GROUP } from './process-group.js'; export const SUPERVISOR_DEFAULTS = Object.freeze({ // Longer than the quiet stretches a healthy turn has: a long-context first @@ -110,7 +117,7 @@ const runAttempt = ({ adapter, prompt, ctx, run, settings, attempt, sessionId, e let killed = 0; for (const child of children) { if (!isAlive(child)) continue; - try { child.kill('SIGKILL'); killed += 1; } catch { /* already gone */ } + if (killProcessTree(child, 'SIGKILL')) killed += 1; } return killed; }; @@ -180,7 +187,11 @@ const runAttempt = ({ adapter, prompt, ctx, run, settings, attempt, sessionId, e }; // Wraps the caller's onSpawn: every argument reaches it unchanged. + // A group-leading child is tracked for the wrapper's exit even when it + // arrives after this attempt ended: it is outside the seat's own group, so + // nothing else would kill it. const onSpawn = (...args) => { + for (const child of args.filter(isChild)) trackSupervisedGroup(child); if (live) { touch(); for (const child of args.filter(isChild)) { @@ -206,7 +217,7 @@ const runAttempt = ({ adapter, prompt, ctx, run, settings, attempt, sessionId, e let pending; try { pending = Promise.resolve(adapter.spawn(prompt, { - ...ctx, ...(sessionId ? { sessionId } : {}), trace, signal: controller.signal, onSpawn, + ...ctx, ...(sessionId ? { sessionId } : {}), trace, signal: controller.signal, onSpawn, [OWN_PROCESS_GROUP]: true, })); } catch (error) { pending = Promise.reject(error); @@ -225,7 +236,8 @@ const runAttempt = ({ adapter, prompt, ctx, run, settings, attempt, sessionId, e * @param {object} args * @param {import('./contract.js').Adapter} args.adapter * @param {string} args.prompt - * @param {object} [args.ctx] passed to adapter.spawn; `trace`, `signal` and `onSpawn` are wrapped. + * @param {object} [args.ctx] passed to adapter.spawn; `trace`, `signal` and `onSpawn` are wrapped, + * and `ownProcessGroup` is set to true. * `ctx.signal`, when given, cancels the run (outcome `cancelled`). * @param {ReturnType} args.store * @param {object} [args.policy] diff --git a/packages/seats/test/supervisor-group-kill.test.js b/packages/seats/test/supervisor-group-kill.test.js new file mode 100644 index 0000000000..7323c469a8 --- /dev/null +++ b/packages/seats/test/supervisor-group-kill.test.js @@ -0,0 +1,239 @@ +// A stalled turn's kill has to reach what the agent CLI spawned, not only the +// CLI. Each test here uses real processes: a shell that starts a grandchild +// (`sleep 300 &`), writes its pid, and waits on it, which is the shape of a CLI +// that started an MCP server. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { superviseRun } from '../src/supervisor.js'; +import { createTraceStore } from '../src/trace/store.js'; +import { runFailure } from '../src/failures.js'; +import { observeSpawn } from '../src/runtime-observation.js'; +import { + LEADS_PROCESS_GROUP, OWN_PROCESS_GROUP, leadsOwnGroup, spawnInOwnGroup, trackedGroupCount, wantsOwnProcessGroup, +} from '../src/process-group.js'; + +const SRC = fileURLToPath(new URL('../src/', import.meta.url)); +const POSIX = process.platform !== 'win32'; +const STDIO = ['ignore', 'pipe', 'pipe']; + +// A zombie still answers kill(pid, 0); on Linux it is read as gone, because a +// SIGKILLed orphan waits there until init reaps it. +const isRunning = (pid) => { + try { process.kill(pid, 0); } catch (error) { return error.code !== 'ESRCH'; } + try { return !/^\d+ \(.*\) Z/.test(readFileSync(`/proc/${pid}/stat`, 'utf8')); } catch { return true; } +}; + +const waitFor = async (predicate, ms = 5000) => { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (await predicate()) return true; + await new Promise((r) => { setTimeout(r, 25); }); + } + return predicate(); +}; + +const readPid = async (file) => { + assert.ok(await waitFor(() => existsSync(file) && /^\d+\s*$/.test(readFileSync(file, 'utf8'))), `no pid in ${file}`); + return Number(readFileSync(file, 'utf8')); +}; + +const reap = (pid) => { try { process.kill(pid, 'SIGKILL'); } catch { /* gone */ } }; + +/** + * An adapter shaped like the real ones: it spawns through spawnInOwnGroup, + * asking for a group only when the supervisor offered one, and reports the + * child through onSpawn. It ignores the abort, so the supervisor's kill is the + * only thing that ends the turn. + */ +const grandchildAdapter = ({ pidFile, children, honourGroup = true }) => ({ + name: 'sh-grandchild', + spawn(prompt, ctx) { + return new Promise((resolve, reject) => { + const child = spawnInOwnGroup(spawn, 'sh', ['-c', `sleep 300 & echo $! > '${pidFile}'; wait`], { stdio: STDIO }, { + ownGroup: honourGroup && wantsOwnProcessGroup(ctx), + }); + children.push(child); + observeSpawn(child, ctx.onSpawn, {}); + child.on('close', (code, signal) => { + if (ctx.signal.aborted) reject(ctx.signal.reason); + else reject(runFailure('exited', `sh exited ${code ?? signal}`)); + }); + }); + }, +}); + +const withHome = async (fn) => { + const home = mkdtempSync(join(tmpdir(), 'seats-group-kill-')); + try { return await fn(home); } finally { rmSync(home, { recursive: true, force: true }); } +}; + +const STALL = { stallMs: 300, cancelGraceMs: 300, timeoutMs: 20000 }; + +test('the supervisor tells the adapter it may lead its own group', async () => withHome(async (home) => { + let seen; + const adapter = { name: 'spy', spawn: async (prompt, ctx) => { seen = ctx; return { text: 'ok' }; } }; + await superviseRun({ adapter, prompt: 'p', ctx: {}, store: createTraceStore({ root: join(home, 'trace') }) }); + assert.equal(seen[OWN_PROCESS_GROUP], true); +})); + +test('unasked, spawnInOwnGroup hands the options through untouched', () => { + const options = { cwd: '/x', env: { A: '1' }, stdio: STDIO }; + const calls = []; + const child = spawnInOwnGroup((...args) => { calls.push(args); return {}; }, 'cmd', ['a'], options); + assert.equal(calls[0][2], options); + assert.equal(Object.hasOwn(calls[0][2], 'detached'), false); + assert.equal(child[LEADS_PROCESS_GROUP], undefined); +}); + +test('a group-leading child is never inferred: detached without the marker is not a leader', { skip: !POSIX }, async () => { + const child = spawn('sleep', ['300'], { detached: true, stdio: 'ignore' }); + try { + await once(child, 'spawn'); + assert.equal(leadsOwnGroup(child), false); + child[LEADS_PROCESS_GROUP] = true; + assert.equal(leadsOwnGroup(child), true); + } finally { + child.kill('SIGKILL'); + await once(child, 'exit'); + } + assert.equal(leadsOwnGroup(child), false, 'a reaped child leads nothing, marked or not'); +}); + +test('a stalled turn\'s kill reaches the grandchild of a group-leading child', { skip: !POSIX }, async () => withHome(async (home) => { + const pidFile = join(home, 'grandchild.pid'); + const children = []; + let grandchild; + try { + const run = superviseRun({ + adapter: grandchildAdapter({ pidFile, children }), prompt: 'p', ctx: {}, + store: createTraceStore({ root: join(home, 'trace') }), policy: STALL, + }); + grandchild = await readPid(pidFile); + assert.equal(leadsOwnGroup(children[0]), true); + assert.equal(isRunning(grandchild), true); + const out = await run; + assert.equal(out.outcome, 'stuck'); + assert.ok(await waitFor(() => !isRunning(grandchild)), `grandchild ${grandchild} survived the kill`); + assert.ok(await waitFor(() => children[0].exitCode !== null || children[0].signalCode !== null), 'the leader survived'); + assert.ok(await waitFor(() => trackedGroupCount() === 0), 'the exited group is still tracked'); + } finally { + if (grandchild) reap(grandchild); + for (const child of children) child.kill('SIGKILL'); + } +})); + +test('a child that does not lead its group is killed with child.kill, and no negative pid is signalled', { skip: !POSIX }, async (t) => withHome(async (home) => { + const pidFile = join(home, 'grandchild.pid'); + const children = []; + const groupSignals = []; + const realKill = process.kill; + t.mock.method(process, 'kill', function kill(pid, signal) { + if (Number(pid) < 0) groupSignals.push([pid, signal]); + return realKill.call(process, pid, signal); + }); + let grandchild; + try { + const out = await (async () => { + const run = superviseRun({ + adapter: grandchildAdapter({ pidFile, children, honourGroup: false }), prompt: 'p', ctx: {}, + store: createTraceStore({ root: join(home, 'trace') }), policy: STALL, + }); + grandchild = await readPid(pidFile); + return run; + })(); + assert.equal(out.outcome, 'stuck'); + assert.equal(leadsOwnGroup(children[0]), false); + assert.ok(await waitFor(() => children[0].signalCode !== null), 'child.kill did not reach the child'); + assert.equal(children[0].signalCode, 'SIGKILL'); + assert.deepEqual(groupSignals, []); + } finally { + if (grandchild) reap(grandchild); + for (const child of children) child.kill('SIGKILL'); + } +})); + +// ── The wrapper's own exit ────────────────────────────────────────────────── +// A group-leading child is outside the wrapper's group, so launchd's kill of +// the seat's group no longer reaches it. The wrapper itself has to kill it. + +const WRAPPER = (pidFile, mode) => ` +import { spawn } from 'node:child_process'; +import { existsSync, readFileSync } from 'node:fs'; +import { superviseRun } from ${JSON.stringify(pathToFileURL(join(SRC, 'supervisor.js')).href)}; +import { createTraceStore } from ${JSON.stringify(pathToFileURL(join(SRC, 'trace/store.js')).href)}; +import { observeSpawn } from ${JSON.stringify(pathToFileURL(join(SRC, 'runtime-observation.js')).href)}; +import { spawnInOwnGroup, wantsOwnProcessGroup } from ${JSON.stringify(pathToFileURL(join(SRC, 'process-group.js')).href)}; +const mode = ${JSON.stringify(mode)}; +if (mode === 'host-handler') { + // Like 'agent run': the host handles the signal and decides when to exit. + process.on('SIGINT', () => { process.stdout.write('host-handled\\n'); setTimeout(() => process.exit(7), 300); }); +} +const adapter = { name: 'sh', spawn: (prompt, ctx) => new Promise(() => { + const child = spawnInOwnGroup(spawn, 'sh', ['-c', "sleep 300 & echo $! > '${pidFile}'; wait"], { stdio: ['ignore', 'pipe', 'pipe'] }, { ownGroup: wantsOwnProcessGroup(ctx) }); + observeSpawn(child, ctx.onSpawn, {}); + child.once('spawn', () => { + const poll = setInterval(() => { + if (!existsSync('${pidFile}') || !/^\\d+\\s*$/.test(readFileSync('${pidFile}', 'utf8'))) return; + clearInterval(poll); + process.stdout.write('ready ' + child.pid + '\\n'); + if (mode === 'exit') process.exit(3); + }, 20); + }); +}) }; +superviseRun({ adapter, prompt: 'p', ctx: {}, store: createTraceStore({ root: ${JSON.stringify(`${pidFile}.trace`)} }), + policy: { stallMs: 0, timeoutMs: 0 } }); +`; + +const runWrapper = async (home, mode, signal) => { + const pidFile = join(home, `grandchild-${mode}-${signal || 'none'}.pid`); + const script = join(home, `wrapper-${mode}-${signal || 'none'}.mjs`); + writeFileSync(script, WRAPPER(pidFile, mode)); + const wrapper = spawn(process.execPath, [script], { stdio: ['ignore', 'pipe', 'inherit'] }); + let out = ''; + wrapper.stdout.on('data', (chunk) => { out += chunk; }); + const exited = once(wrapper, 'exit'); + let leader; + let grandchild; + try { + assert.ok(await waitFor(() => /ready \d+/.test(out)), `wrapper never got ready: ${out}`); + leader = Number(out.match(/ready (\d+)/)[1]); + grandchild = await readPid(pidFile); + if (signal) { + assert.equal(isRunning(grandchild), true); + wrapper.kill(signal); + } + const [code, how] = await Promise.race([exited, new Promise((r) => { setTimeout(() => r([null, 'timeout']), 5000); })]); + assert.ok(await waitFor(() => !isRunning(grandchild)), `grandchild ${grandchild} survived the wrapper (${mode}, ${signal})`); + assert.ok(await waitFor(() => !isRunning(leader)), `leader ${leader} survived the wrapper (${mode}, ${signal})`); + return { code, how, out }; + } finally { + if (grandchild) reap(grandchild); + if (leader) reap(leader); + wrapper.kill('SIGKILL'); + } +}; + +for (const signal of ['SIGTERM', 'SIGINT', 'SIGHUP']) { + test(`a wrapper killed by ${signal} takes its supervised groups with it, and still dies by ${signal}`, { skip: !POSIX }, async () => withHome(async (home) => { + const { code, how } = await runWrapper(home, 'plain', signal); + assert.equal(code, null); + assert.equal(how, signal, 'the wrapper no longer dies by the signal it was sent'); + })); +} + +test('a wrapper that exits takes its supervised groups with it', { skip: !POSIX }, async () => withHome(async (home) => { + const { code } = await runWrapper(home, 'exit', null); + assert.equal(code, 3); +})); + +test('a wrapper that handles the signal itself still loses its groups, and exits on its own terms', { skip: !POSIX }, async () => withHome(async (home) => { + const { code, out } = await runWrapper(home, 'host-handler', 'SIGINT'); + assert.match(out, /host-handled/); + assert.equal(code, 7); +})); From 49119bec4249f49caedd0e7c0ad7b94ca205e847 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 09:06:25 -0700 Subject: [PATCH 2/5] fix(seats): walk a killed child's descendants; a host-handled SIGTERM keeps the turn A group kill misses descendants that moved to a group of their own. Live codex seats put every MCP server and tool command in a group of its own, and claude's Bash tool shells do the same. - Descendant walk: before any signal, snapshot `ps -A -o pid=,ppid=,lstart=`. Then signal the child's descendants deepest first, re-reading each pid right before its signal. A pid is killed only if its ppid and start time still match the snapshot, so a pid reused by another process on the host is left alone. The walk runs before the group kill, so each pid's parent is still alive when its ppid is checked. Nothing loosens the ppid match. - SIGTERM/SIGINT/SIGHUP: when a host handles the signal itself, the registry no longer kills groups, so the in-flight turn survives. With no host handler, it still kills them and re-raises. - armSupervisedGroupDeadline(ms): the host kills the groups by a deadline it sets before launchd's SIGKILL, which no handler can catch. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu --- packages/seats/src/index.js | 3 +- packages/seats/src/process-group.js | 153 ++++++++++-- packages/seats/test-support/processes.js | 30 +++ .../seats/test/supervisor-group-kill.test.js | 27 +-- .../test/supervisor-tree-and-deadline.test.js | 224 ++++++++++++++++++ 5 files changed, 394 insertions(+), 43 deletions(-) create mode 100644 packages/seats/test-support/processes.js create mode 100644 packages/seats/test/supervisor-tree-and-deadline.test.js diff --git a/packages/seats/src/index.js b/packages/seats/src/index.js index 9ee054c4e0..47e30b6def 100644 --- a/packages/seats/src/index.js +++ b/packages/seats/src/index.js @@ -3,7 +3,8 @@ export { requireSeatModel } from './seat-model.js'; export { controlsFromArguments, observeSpawn } from './runtime-observation.js'; export { LEADS_PROCESS_GROUP, OWN_PROCESS_GROUP, spawnInOwnGroup, wantsOwnProcessGroup, leadsOwnGroup, killProcessTree, - trackSupervisedGroup, killSupervisedGroups, + trackSupervisedGroup, killSupervisedGroups, armSupervisedGroupDeadline, + readProcessTable, readProcess, descendantsOf, killDescendants, } from './process-group.js'; export { adapterFailure, spawnCredentials } from './upstream-refusal.js'; export { default as stub } from './adapters/stub.js'; diff --git a/packages/seats/src/process-group.js b/packages/seats/src/process-group.js index fea2b03c83..b016d0704d 100644 --- a/packages/seats/src/process-group.js +++ b/packages/seats/src/process-group.js @@ -12,12 +12,27 @@ * negative pid: signalling `-pid` for a process that does not lead its group * can reach an unrelated group. * + * A group kill misses a descendant that moved to a group of its own (codex + * starts every MCP server and tool command that way; claude's Bash tool does + * too). So before any signal, the kill also snapshots the process table and + * walks the child's descendants, deepest first. Each pid is re-read right + * before it is signalled and killed only if its parent and start time still + * match the snapshot: a pid that exited and was reused by another process on + * this host is left alone. Leaves go first so a pid's parent is still alive, + * and its ppid still meaningful, when it is checked. + * * A child in its own group is outside the wrapper's group, so launchd's kill * of the seat's group on exit no longer reaches it. `trackSupervisedGroup` * covers that: while any tracked group is live, the wrapper kills them all - * when it receives SIGTERM, SIGINT or SIGHUP, and when it exits. + * when it exits, and on SIGTERM, SIGINT or SIGHUP when nothing else handles + * the signal. A host that handles the signal itself (`agent run` finishes the + * in-flight turn on SIGTERM) owns the timing: it calls + * `armSupervisedGroupDeadline` so the groups die before launchd's SIGKILL, + * which no handler can catch, and the exit hook covers the clean case. */ +import { spawnSync } from 'node:child_process'; + /** Set on a child by the code that spawned it with `detached: true`. */ export const LEADS_PROCESS_GROUP = Symbol.for('@commonlyai/seats.leadsProcessGroup'); @@ -57,35 +72,135 @@ export const leadsOwnGroup = (child) => !!child && Number.isInteger(child.pid) && child.pid > 0 && isAlive(child); +// ── The process table ────────────────────────────────────────────────────── + +const PS_ENV = { ...process.env, LC_ALL: 'C' }; + +const parseTable = (text) => { + const rows = []; + for (const line of String(text || '').split('\n')) { + const match = /^\s*(\d+)\s+(\d+)\s+(\S.*?)\s*$/.exec(line); + if (match) rows.push({ pid: Number(match[1]), ppid: Number(match[2]), started: match[3] }); + } + return rows; +}; + +const ps = (args) => { + try { + const out = spawnSync('ps', args, { encoding: 'utf8', env: PS_ENV, timeout: 5000 }); + return out.error ? null : parseTable(out.stdout); + } catch { return null; } +}; + +/** Every process: { pid, ppid, started }. null when ps is unavailable. */ +export const readProcessTable = () => ps(['-A', '-o', 'pid=,ppid=,lstart=']); + +/** One process as it is now, or null when it is gone. */ +export const readProcess = (pid) => (ps(['-o', 'pid=,ppid=,lstart=', '-p', String(pid)]) || []) + .find((row) => row.pid === pid) || null; + +/** The descendants of `root` in `table`, deepest first. The root is not included. */ +export const descendantsOf = (table, root) => { + const byParent = new Map(); + for (const row of table || []) { + if (row.pid === row.ppid) continue; + if (!byParent.has(row.ppid)) byParent.set(row.ppid, []); + byParent.get(row.ppid).push(row); + } + const out = []; + const seen = new Set([root]); + let level = [root]; + for (let depth = 1; level.length; depth += 1) { + const next = []; + for (const parent of level) { + for (const row of byParent.get(parent) || []) { + if (seen.has(row.pid)) continue; + seen.add(row.pid); + out.push({ ...row, depth }); + next.push(row.pid); + } + } + level = next; + } + return out.sort((a, b) => b.depth - a.depth); +}; + +const DEFAULT_IO = Object.freeze({ table: readProcessTable, read: readProcess }); + /** - * Kill a child, and its whole group when it leads one. Falls back to - * `child.kill` when the child does not lead a group, or the group signal fails. - * Returns whether a signal was delivered. + * Signal each snapshotted descendant, deepest first, re-reading it just + * before: it is signalled only while its ppid and start time still match. + * Returns the pids signalled. */ -export const killProcessTree = (child, signal = 'SIGKILL') => { +export const killDescendants = (descendants, signal = 'SIGKILL', { read = readProcess } = {}) => { + const killed = []; + for (const row of descendants) { + const now = read(row.pid); + if (!now || now.ppid !== row.ppid || now.started !== row.started) continue; + try { process.kill(row.pid, signal); killed.push(row.pid); } catch { /* gone */ } + } + return killed; +}; + +const liveRoot = (child) => !!child && Number.isInteger(child.pid) && child.pid > 0 && isAlive(child); + +const killOne = (child, signal, table, io) => { + // The root must be alive, so unreaped, so its pid still names this child. + if (table && liveRoot(child)) killDescendants(descendantsOf(table, child.pid), signal, io); if (leadsOwnGroup(child)) { try { process.kill(-child.pid, signal); return true; } catch { /* fall back to the leader */ } } try { return child.kill(signal) !== false; } catch { return false; } }; +/** + * Kill a child, its verified descendants, and its whole group when it leads + * one. The table is read before any signal. Falls back to `child.kill` when + * the child does not lead a group, or the group signal fails. Returns whether + * the child was signalled. `io` is a test seam ({ table, read }). + */ +export const killProcessTree = (child, signal = 'SIGKILL', io = {}) => { + const seam = { ...DEFAULT_IO, ...io }; + return killOne(child, signal, liveRoot(child) ? seam.table() : null, seam); +}; + // ── Wrapper-exit cleanup ──────────────────────────────────────────────────── const EXIT_SIGNALS = ['SIGTERM', 'SIGINT', 'SIGHUP']; const tracked = new Set(); const handlers = new Map(); let onExit = null; - -/** SIGKILL every tracked group still led by a live child. Synchronous. */ -export const killSupervisedGroups = (signal = 'SIGKILL') => { +let deadline = null; + +/** SIGKILL every tracked group still led by a live child, with its verified descendants. Synchronous. */ +export const killSupervisedGroups = (signal = 'SIGKILL', io = {}) => { + const live = [...tracked].filter(leadsOwnGroup); + if (live.length === 0) return 0; + const seam = { ...DEFAULT_IO, ...io }; + const table = seam.table(); let killed = 0; - for (const child of tracked) { - if (!leadsOwnGroup(child)) continue; - try { process.kill(-child.pid, signal); killed += 1; } catch { /* already gone */ } - } + for (const child of live) if (killOne(child, signal, table, seam)) killed += 1; return killed; }; +/** + * Kill the tracked groups in `ms`, for a host that keeps running after a + * signal (to finish a turn) but will be SIGKILLed at a known deadline. The + * earliest deadline armed wins. The timer does not hold the process open. + * Returns a function that disarms it. + */ +export const armSupervisedGroupDeadline = (ms) => { + const wait = Math.max(0, Number(ms) || 0); + const at = Date.now() + wait; + if (deadline && deadline.at <= at) return deadline.cancel; + if (deadline) deadline.cancel(); + const timer = setTimeout(() => { deadline = null; killSupervisedGroups(); }, wait); + timer.unref?.(); + const entry = { at, cancel: () => { clearTimeout(timer); if (deadline === entry) deadline = null; } }; + deadline = entry; + return entry.cancel; +}; + const uninstall = () => { for (const [signal, handler] of handlers) process.removeListener(signal, handler); handlers.clear(); @@ -99,14 +214,16 @@ const install = () => { process.on('exit', onExit); for (const signal of EXIT_SIGNALS) { const handler = () => { + // A host that handles this signal has taken over shutdown: it may be + // finishing the turn these groups belong to, so they are left alone and + // the host's deadline or exit kills them. + if (process.listenerCount(signal) !== 1) return; killSupervisedGroups(); - // Listening for a signal replaces its default action. When nothing else - // handles it, restore that action and re-raise, so the wrapper still + // Listening for a signal replaces its default action. With nothing else + // handling it, restore that action and re-raise, so the wrapper still // terminates exactly as it would have without this module. - if (process.listenerCount(signal) === 1) { - uninstall(); - process.kill(process.pid, signal); - } + uninstall(); + process.kill(process.pid, signal); }; handlers.set(signal, handler); process.on(signal, handler); diff --git a/packages/seats/test-support/processes.js b/packages/seats/test-support/processes.js new file mode 100644 index 0000000000..5e2811f8c4 --- /dev/null +++ b/packages/seats/test-support/processes.js @@ -0,0 +1,30 @@ +// Real-process helpers for the kill tests. +import assert from 'node:assert/strict'; +import { existsSync, readFileSync } from 'node:fs'; + +// A zombie still answers kill(pid, 0); on Linux it is read as gone, because a +// SIGKILLed orphan waits there until init reaps it. +export const isRunning = (pid) => { + try { process.kill(pid, 0); } catch (error) { return error.code !== 'ESRCH'; } + try { return !/^\d+ \(.*\) Z/.test(readFileSync(`/proc/${pid}/stat`, 'utf8')); } catch { return true; } +}; + +export const waitFor = async (predicate, ms = 5000) => { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (await predicate()) return true; + await new Promise((r) => { setTimeout(r, 25); }); + } + return predicate(); +}; + +/** The comma-separated pids a process wrote to `file`, once it has written them. */ +export const readPids = async (file, count = 1) => { + const ready = () => existsSync(file) && new RegExp(`^\\d+(,\\d+){${count - 1}}\\s*$`).test(readFileSync(file, 'utf8')); + assert.ok(await waitFor(ready), `no ${count} pid(s) in ${file}`); + return readFileSync(file, 'utf8').trim().split(',').map(Number); +}; + +export const readPid = async (file) => (await readPids(file, 1))[0]; + +export const reap = (pid) => { try { process.kill(pid, 'SIGKILL'); } catch { /* gone */ } }; diff --git a/packages/seats/test/supervisor-group-kill.test.js b/packages/seats/test/supervisor-group-kill.test.js index 7323c469a8..4de7ab9211 100644 --- a/packages/seats/test/supervisor-group-kill.test.js +++ b/packages/seats/test/supervisor-group-kill.test.js @@ -6,7 +6,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { spawn } from 'node:child_process'; import { once } from 'node:events'; -import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -14,6 +14,7 @@ import { superviseRun } from '../src/supervisor.js'; import { createTraceStore } from '../src/trace/store.js'; import { runFailure } from '../src/failures.js'; import { observeSpawn } from '../src/runtime-observation.js'; +import { isRunning, readPid, reap, waitFor } from '../test-support/processes.js'; import { LEADS_PROCESS_GROUP, OWN_PROCESS_GROUP, leadsOwnGroup, spawnInOwnGroup, trackedGroupCount, wantsOwnProcessGroup, } from '../src/process-group.js'; @@ -22,28 +23,6 @@ const SRC = fileURLToPath(new URL('../src/', import.meta.url)); const POSIX = process.platform !== 'win32'; const STDIO = ['ignore', 'pipe', 'pipe']; -// A zombie still answers kill(pid, 0); on Linux it is read as gone, because a -// SIGKILLed orphan waits there until init reaps it. -const isRunning = (pid) => { - try { process.kill(pid, 0); } catch (error) { return error.code !== 'ESRCH'; } - try { return !/^\d+ \(.*\) Z/.test(readFileSync(`/proc/${pid}/stat`, 'utf8')); } catch { return true; } -}; - -const waitFor = async (predicate, ms = 5000) => { - const deadline = Date.now() + ms; - while (Date.now() < deadline) { - if (await predicate()) return true; - await new Promise((r) => { setTimeout(r, 25); }); - } - return predicate(); -}; - -const readPid = async (file) => { - assert.ok(await waitFor(() => existsSync(file) && /^\d+\s*$/.test(readFileSync(file, 'utf8'))), `no pid in ${file}`); - return Number(readFileSync(file, 'utf8')); -}; - -const reap = (pid) => { try { process.kill(pid, 'SIGKILL'); } catch { /* gone */ } }; /** * An adapter shaped like the real ones: it spawns through spawnInOwnGroup, @@ -232,7 +211,7 @@ test('a wrapper that exits takes its supervised groups with it', { skip: !POSIX assert.equal(code, 3); })); -test('a wrapper that handles the signal itself still loses its groups, and exits on its own terms', { skip: !POSIX }, async () => withHome(async (home) => { +test('a wrapper that handles the signal itself keeps its groups through the signal, and loses them when it exits', { skip: !POSIX }, async () => withHome(async (home) => { const { code, out } = await runWrapper(home, 'host-handler', 'SIGINT'); assert.match(out, /host-handled/); assert.equal(code, 7); diff --git a/packages/seats/test/supervisor-tree-and-deadline.test.js b/packages/seats/test/supervisor-tree-and-deadline.test.js new file mode 100644 index 0000000000..1b1d9830eb --- /dev/null +++ b/packages/seats/test/supervisor-tree-and-deadline.test.js @@ -0,0 +1,224 @@ +// Two layers on top of the group kill, with real processes. +// +// The descendant walk: a CLI's child that moved to a group of its own (codex +// does this for every MCP server and tool command) is outside the group kill, +// so the kill also walks the process tree. A pid is signalled only if its ppid +// and start time still match the snapshot, so a reused pid is left alone. +// +// The SIGTERM deadline: a host that keeps the in-flight turn on SIGTERM keeps +// the groups too, and kills them at a deadline set before launchd's SIGKILL. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { superviseRun } from '../src/supervisor.js'; +import { createTraceStore } from '../src/trace/store.js'; +import { runFailure } from '../src/failures.js'; +import { observeSpawn } from '../src/runtime-observation.js'; +import { + descendantsOf, killProcessTree, leadsOwnGroup, readProcess, readProcessTable, spawnInOwnGroup, wantsOwnProcessGroup, +} from '../src/process-group.js'; +import { isRunning, readPids, reap, waitFor } from '../test-support/processes.js'; + +const SRC = fileURLToPath(new URL('../src/', import.meta.url)); +const POSIX = process.platform !== 'win32'; + +const withHome = async (fn) => { + const home = mkdtempSync(join(tmpdir(), 'seats-tree-')); + try { return await fn(home); } finally { rmSync(home, { recursive: true, force: true }); } +}; + +// A child that starts `count` grandchildren, each the leader of a new group +// (detached), writes their pids, and stays alive. +const NEW_GROUP_PARENT = ` +const { spawn } = require('node:child_process'); +const pids = []; +for (let i = 0; i < Number(process.argv[2]); i += 1) { + pids.push(spawn('sleep', ['300'], { detached: true, stdio: 'ignore' }).pid); +} +require('node:fs').writeFileSync(process.argv[1], pids.join(',')); +setInterval(() => {}, 1e6); +`; + +const startParent = (pidFile, count, ownGroup) => spawnInOwnGroup( + spawn, process.execPath, ['-e', NEW_GROUP_PARENT, pidFile, String(count)], { stdio: ['ignore', 'pipe', 'pipe'] }, { ownGroup }, +); + +test('the descendant table is read deepest first', () => { + const table = [ + { pid: 10, ppid: 1, started: 'a' }, { pid: 11, ppid: 10, started: 'b' }, { pid: 12, ppid: 11, started: 'c' }, + { pid: 13, ppid: 10, started: 'd' }, { pid: 20, ppid: 1, started: 'e' }, + ]; + assert.deepEqual(descendantsOf(table, 10).map((row) => [row.pid, row.depth]), [[12, 2], [11, 1], [13, 1]]); +}); + +test('a stall kill reaches a grandchild that started a group of its own', { skip: !POSIX }, async () => withHome(async (home) => { + const pidFile = join(home, 'grandchild.pid'); + const children = []; + let grandchild; + try { + const adapter = { + name: 'new-group-parent', + spawn: (prompt, ctx) => new Promise((resolve, reject) => { + const child = startParent(pidFile, 1, wantsOwnProcessGroup(ctx)); + children.push(child); + observeSpawn(child, ctx.onSpawn, {}); + child.on('close', () => reject(ctx.signal.aborted ? ctx.signal.reason : runFailure('exited', 'parent exited'))); + }), + }; + const run = superviseRun({ + adapter, prompt: 'p', ctx: {}, store: createTraceStore({ root: join(home, 'trace') }), + policy: { stallMs: 300, cancelGraceMs: 300, timeoutMs: 20000 }, + }); + [grandchild] = await readPids(pidFile, 1); + assert.equal(leadsOwnGroup(children[0]), true); + assert.equal(isRunning(grandchild), true); + assert.equal((await run).outcome, 'stuck'); + assert.ok(await waitFor(() => !isRunning(grandchild)), `grandchild ${grandchild} in its own group survived the kill`); + } finally { + if (grandchild) reap(grandchild); + for (const child of children) child.kill('SIGKILL'); + } +})); + +test('a pid reused by another process is left alone; the descendant the snapshot named is killed', { skip: !POSIX }, async () => withHome(async (home) => { + const pidFile = join(home, 'grandchildren.pid'); + const parent = startParent(pidFile, 2, true); + // An unrelated process: not in the parent's tree at all. + const unrelated = spawn('sleep', ['300'], { stdio: 'ignore' }); + let real; + let reused; + try { + [real, reused] = await readPids(pidFile, 2); + await once(unrelated, 'spawn'); + const table = readProcessTable(); + assert.ok(table, 'ps is unavailable, so the walk cannot run'); + assert.ok(readProcess(reused), 'the reused stand-in is not running'); + // The snapshot as it would read if each pid had belonged to an earlier + // process: `reused` is the right parent's child but started at another + // time, and `unrelated` is named as the parent's child though it is not. + const stub = table.map((row) => (row.pid === reused ? { ...row, started: 'Thu Jan 1 00:00:00 1970' } : row)); + stub.push({ pid: unrelated.pid, ppid: parent.pid, started: readProcess(unrelated.pid).started }); + + killProcessTree(parent, 'SIGKILL', { table: () => stub }); + + assert.ok(await waitFor(() => !isRunning(real)), 'the walk did not kill the descendant the snapshot named'); + assert.ok(await waitFor(() => parent.signalCode !== null), 'the parent survived'); + await new Promise((r) => { setTimeout(r, 200); }); + assert.equal(isRunning(reused), true, 'a pid whose start time changed was signalled'); + assert.equal(isRunning(unrelated.pid), true, 'a pid whose parent changed was signalled'); + } finally { + for (const pid of [real, reused]) if (pid) reap(pid); + unrelated.kill('SIGKILL'); + parent.kill('SIGKILL'); + } +})); + +// ── SIGTERM in a host that keeps the turn ───────────────────────────────────── + +// Shaped like `agent run`: SIGTERM asks for a stop after the current turn and +// arms the deadline; when the turn is over, the host's exit path kills the +// groups. The turn ends when the test creates `go`; its child then says +// `done` and lingers, as a CLI shutting down can. +const HOST = ({ pidFile, goFile, deadlineMs }) => ` +import { spawn } from 'node:child_process'; +import { existsSync, readFileSync } from 'node:fs'; +import { superviseRun } from ${JSON.stringify(pathToFileURL(join(SRC, 'supervisor.js')).href)}; +import { createTraceStore } from ${JSON.stringify(pathToFileURL(join(SRC, 'trace/store.js')).href)}; +import { observeSpawn } from ${JSON.stringify(pathToFileURL(join(SRC, 'runtime-observation.js')).href)}; +import { + armSupervisedGroupDeadline, killSupervisedGroups, spawnInOwnGroup, wantsOwnProcessGroup, +} from ${JSON.stringify(pathToFileURL(join(SRC, 'process-group.js')).href)}; +let stopping = false; +process.on('SIGTERM', () => { + stopping = true; + process.stdout.write('stopping\\n'); + armSupervisedGroupDeadline(${deadlineMs}); +}); +const script = "sleep 300 & echo $! > '${pidFile}'; while [ ! -f '${goFile}' ]; do sleep 0.05; done; echo done; wait"; +const adapter = { name: 'sh', spawn: (prompt, ctx) => new Promise((resolve, reject) => { + const child = spawnInOwnGroup(spawn, 'sh', ['-c', script], { stdio: ['ignore', 'pipe', 'pipe'] }, { ownGroup: wantsOwnProcessGroup(ctx) }); + observeSpawn(child, ctx.onSpawn, {}); + child.stdout.on('data', (chunk) => { if (String(chunk).includes('done')) resolve({ text: 'done' }); }); + child.on('close', (code, signal) => reject(new Error('sh ended ' + (code ?? signal)))); + child.once('spawn', () => { + const poll = setInterval(() => { + if (!existsSync('${pidFile}') || !/^\\d+\\s*$/.test(readFileSync('${pidFile}', 'utf8'))) return; + clearInterval(poll); + process.stdout.write('ready ' + child.pid + '\\n'); + }, 20); + }); +}) }; +const out = await superviseRun({ adapter, prompt: 'p', ctx: {}, store: createTraceStore({ root: ${JSON.stringify(`${pidFile}.trace`)} }), + policy: { stallMs: 0, timeoutMs: 0 } }); +process.stdout.write('turn ' + out.outcome + '\\n'); +if (stopping) { killSupervisedGroups(); process.exit(0); } +`; + +const startHost = async (home, deadlineMs) => { + const pidFile = join(home, 'grandchild.pid'); + const goFile = join(home, 'go'); + const script = join(home, 'host.mjs'); + writeFileSync(script, HOST({ pidFile, goFile, deadlineMs })); + const host = spawn(process.execPath, [script], { stdio: ['ignore', 'pipe', 'inherit'] }); + const state = { host, goFile, out: '' }; + host.stdout.on('data', (chunk) => { state.out += chunk; }); + state.exited = once(host, 'exit'); + assert.ok(await waitFor(() => /ready \d+/.test(state.out)), `host never got ready: ${state.out}`); + state.leader = Number(state.out.match(/ready (\d+)/)[1]); + [state.grandchild] = await readPids(pidFile, 1); + return state; +}; + +const cleanup = (state) => { + if (!state) return; + reap(state.grandchild); + reap(state.leader); + state.host.kill('SIGKILL'); +}; + +test('SIGTERM mid-turn: the turn completes, then the groups die', { skip: !POSIX }, async () => withHome(async (home) => { + let state; + try { + state = await startHost(home, 10000); + state.host.kill('SIGTERM'); + assert.ok(await waitFor(() => /stopping/.test(state.out)), 'the host never saw SIGTERM'); + await new Promise((r) => { setTimeout(r, 400); }); + assert.equal(isRunning(state.grandchild), true, 'SIGTERM killed the turn\'s group'); + assert.equal(isRunning(state.leader), true, 'SIGTERM killed the turn\'s child'); + writeFileSync(state.goFile, ''); + const [code] = await Promise.race([state.exited, new Promise((r) => { setTimeout(() => r(['timeout']), 5000); })]); + assert.match(state.out, /turn ok/); + assert.equal(code, 0); + assert.ok(await waitFor(() => !isRunning(state.grandchild)), 'the group outlived the host'); + assert.ok(await waitFor(() => !isRunning(state.leader)), 'the leader outlived the host'); + } finally { + cleanup(state); + } +})); + +test('SIGTERM with the turn still running: the deadline kills the groups before ExitTimeOut', { skip: !POSIX }, async () => withHome(async (home) => { + // Stands in for launchd: ExitTimeOut 2 s, deadline 1 s (the CLI derives the + // real one from the seat's plist). + const exitTimeOutMs = 2000; + const deadlineMs = 1000; + let state; + try { + state = await startHost(home, deadlineMs); + const sent = Date.now(); + state.host.kill('SIGTERM'); + assert.ok(await waitFor(() => /stopping/.test(state.out)), 'the host never saw SIGTERM'); + await new Promise((r) => { setTimeout(r, deadlineMs / 2); }); + assert.equal(isRunning(state.grandchild), true, 'the group died before its deadline'); + assert.ok(await waitFor(() => !isRunning(state.grandchild), exitTimeOutMs), 'the group outlived ExitTimeOut'); + const diedAfter = Date.now() - sent; + assert.ok(diedAfter < exitTimeOutMs, `the group died ${diedAfter}ms after SIGTERM, past ExitTimeOut`); + assert.ok(await waitFor(() => !isRunning(state.leader), 1000), 'the leader outlived its group'); + } finally { + cleanup(state); + } +})); From f83fe372892b7bd0aeaa9962014d107036b35bb6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 09:06:25 -0700 Subject: [PATCH 3/5] fix(cli): agent run kills supervised groups before the seat's ExitTimeOut On SIGTERM, `agent run` still stops polling and finishes the in-flight turn. It now also arms a supervised-group kill a margin before ExitTimeOut, which it reads (only) from ~/Library/LaunchAgents/me.commonly.seat-.plist. An unset, zero or unreadable value means launchd's 20 s default. The margin is min(5 s, a quarter), so the default kill lands at 15 s. Its exit path calls killSupervisedGroups() for groups still live once the turn ends. With no supervised group live, both calls do nothing, so today's seats are unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu --- cli/__tests__/seat-stop.test.mjs | 113 +++++++++++++++++++++++++++++++ cli/src/commands/agent.js | 9 ++- cli/src/lib/seat-stop.js | 78 +++++++++++++++++++++ 3 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 cli/__tests__/seat-stop.test.mjs create mode 100644 cli/src/lib/seat-stop.js diff --git a/cli/__tests__/seat-stop.test.mjs b/cli/__tests__/seat-stop.test.mjs new file mode 100644 index 0000000000..e75fbb39a9 --- /dev/null +++ b/cli/__tests__/seat-stop.test.mjs @@ -0,0 +1,113 @@ +/** + * seat-stop: SIGTERM keeps the turn, and arms a supervised-group kill a margin + * before the seat plist's ExitTimeOut (launchd's SIGKILL). The plist is only + * read. The process-level behaviour is covered with real processes in + * packages/seats/test/supervisor-tree-and-deadline.test.js. + */ +import { jest } from '@jest/globals'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { + DEFAULT_EXIT_TIMEOUT_S, createSeatStop, groupKillDeadlineMs, readExitTimeOut, seatPlistPath, +} from '../src/lib/seat-stop.js'; + +const plist = (body) => ` + + + + Label + me.commonly.seat-anvil + KeepAlive + +${body} + + +`; + +let home; +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'seat-stop-')); + mkdirSync(join(home, 'Library', 'LaunchAgents'), { recursive: true }); +}); +afterEach(() => { rmSync(home, { recursive: true, force: true }); }); + +const writePlist = (name, body) => { + const path = seatPlistPath(name, home); + writeFileSync(path, plist(body)); + return path; +}; + +describe('seat-stop — ExitTimeOut', () => { + test('the plist path is the seat launch agent', () => { + expect(seatPlistPath('anvil', home)).toBe(join(home, 'Library', 'LaunchAgents', 'me.commonly.seat-anvil.plist')); + expect(seatPlistPath('../x', home)).toBeNull(); + expect(seatPlistPath('a/b', home)).toBeNull(); + }); + + test('reads ExitTimeOut from an XML plist, and only reads it', () => { + const path = writePlist('anvil', ' ExitTimeOut\n 40'); + const before = readFileSync(path, 'utf8'); + expect(readExitTimeOut(path)).toBe(40); + expect(readFileSync(path, 'utf8')).toBe(before); + }); + + test('unset, zero, missing file and no path all read as unset', () => { + expect(readExitTimeOut(writePlist('a', ''))).toBeNull(); + expect(readExitTimeOut(writePlist('b', ' ExitTimeOut\n 0'))).toBeNull(); + expect(readExitTimeOut(join(home, 'nope.plist'))).toBeNull(); + expect(readExitTimeOut(null)).toBeNull(); + }); + + test('a binary plist is read through plutil -extract', () => { + const run = jest.fn(() => ({ status: 0, stdout: '30\n' })); + const value = readExitTimeOut('/x.plist', { readFile: () => Buffer.from('bplist00...'), run }); + expect(value).toBe(30); + expect(run).toHaveBeenCalledWith('plutil', ['-extract', 'ExitTimeOut', 'raw', '-o', '-', '/x.plist'], expect.any(Object)); + }); + + test('the deadline is ExitTimeOut less min(5 s, a quarter); unset means 20 s', () => { + expect(DEFAULT_EXIT_TIMEOUT_S).toBe(20); + expect(groupKillDeadlineMs(null)).toBe(15000); + expect(groupKillDeadlineMs(40)).toBe(35000); + expect(groupKillDeadlineMs(4)).toBe(3000); + for (const s of [1, 4, 20, 40, 120]) expect(groupKillDeadlineMs(s)).toBeLessThan(s * 1000); + }); +}); + +describe('seat-stop — requestStop', () => { + const make = (name = 'anvil') => { + const stop = jest.fn(); + const arm = jest.fn(); + const kill = jest.fn(); + return { stop, arm, kill, seatStop: createSeatStop({ name, stop, home, deps: { arm, kill } }) }; + }; + + test('SIGTERM stops the loop and arms the group kill before ExitTimeOut', () => { + writePlist('anvil', ' ExitTimeOut\n 8'); + const { stop, arm, kill, seatStop } = make(); + expect(seatStop.requestStop('SIGTERM')).toBe(6000); + expect(stop).toHaveBeenCalledTimes(1); + expect(arm).toHaveBeenCalledWith(6000); + expect(kill).not.toHaveBeenCalled(); + }); + + test('SIGTERM on a seat with no plist uses the 20 s default', () => { + const { arm, seatStop } = make('daemon-seat'); + seatStop.requestStop('SIGTERM'); + expect(arm).toHaveBeenCalledWith(15000); + }); + + test('SIGINT stops the loop and arms nothing', () => { + const { stop, arm, seatStop } = make(); + expect(seatStop.requestStop('SIGINT')).toBeNull(); + expect(stop).toHaveBeenCalledTimes(1); + expect(arm).not.toHaveBeenCalled(); + }); + + test('the exit path kills the supervised groups', () => { + const { kill, seatStop } = make(); + seatStop.onExit(); + expect(kill).toHaveBeenCalledTimes(1); + }); +}); diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index fcd038aff7..b2e95554e4 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -83,6 +83,7 @@ import { detectBwrap } from '../lib/sandbox/bwrap.js'; import { resolvePublicSandboxMode } from '../lib/sandbox/mode.js'; import { hostSandboxRefusal, windowsSandboxMessage } from '../lib/sandbox/host-check.js'; import { prepareMcpBeforeFirstPoll } from '../lib/mcp-first-run.js'; +import { createSeatStop } from '../lib/seat-stop.js'; import { detectSeatbelt } from '../lib/sandbox/seatbelt.js'; import { describeUpstreamRefusal } from '../lib/upstream-refusal.js'; import { @@ -3612,12 +3613,16 @@ Docs: // SIGTERM is the daemon's idle-boundary handoff signal. `stop()` stops // future polls but lets the in-flight turn finish and ack before the // event loop drains; exiting here would cut a model turn in half. - const requestStop = () => { + // seat-stop also arms the supervised-group deadline before launchd's + // SIGKILL, and kills any group still live when the process exits. + const seatStop = createSeatStop({ name, stop }); + const requestStop = (signal) => { console.log(`\n${stamp()} [${name}] stopping after the current turn...`); - stop(); + seatStop.requestStop(signal); }; process.on('SIGINT', requestStop); process.on('SIGTERM', requestStop); + process.once('exit', seatStop.onExit); }); // ── detach (ADR-005) ────────────────────────────────────────────────────── diff --git a/cli/src/lib/seat-stop.js b/cli/src/lib/seat-stop.js new file mode 100644 index 0000000000..78ceb4f452 --- /dev/null +++ b/cli/src/lib/seat-stop.js @@ -0,0 +1,78 @@ +/** + * How `agent run` stops on a signal while supervised process groups may be + * live (see @commonlyai/seats/process-group). + * + * SIGTERM keeps the in-flight turn: the run loop stops polling and lets the + * turn finish. A launchd seat is SIGKILLed ExitTimeOut seconds after SIGTERM, + * and that SIGKILL cannot be caught, so a supervised group (outside the seat's + * own group, which launchd kills) would outlive the seat. So SIGTERM also arms + * a deadline a margin before ExitTimeOut, and the exit path kills whatever + * groups are left when the turn ends first. + * + * ExitTimeOut is read from the seat's plist and never written. Unset, zero or + * unreadable, it is launchd's default of 20 s. + */ +import { spawnSync } from 'child_process'; +import { readFileSync } from 'fs'; +import { homedir } from 'os'; +import { join } from 'path'; +import { armSupervisedGroupDeadline, killSupervisedGroups } from '@commonlyai/seats/process-group'; + +export const DEFAULT_EXIT_TIMEOUT_S = 20; +const MAX_MARGIN_MS = 5000; +const SEAT_NAME = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; + +/** The seat's launchd plist path, or null for a name that cannot be a file name. */ +export const seatPlistPath = (name, home = homedir()) => (typeof name === 'string' && SEAT_NAME.test(name) + ? join(home, 'Library', 'LaunchAgents', `me.commonly.seat-${name}.plist`) + : null); + +const positive = (value) => (Number.isFinite(value) && value > 0 ? value : null); + +/** + * ExitTimeOut in seconds, or null when the plist is missing, has none, or + * holds a value launchd would not use as a timeout. An XML plist is read + * directly; a binary one through `plutil -extract` (read-only). + */ +export const readExitTimeOut = (path, { readFile = readFileSync, run = spawnSync } = {}) => { + if (!path) return null; + let raw; + try { raw = readFile(path); } catch { return null; } + const text = Buffer.isBuffer(raw) ? raw.toString('utf8') : String(raw); + if (text.startsWith('bplist')) { + try { + const out = run('plutil', ['-extract', 'ExitTimeOut', 'raw', '-o', '-', path], { encoding: 'utf8', timeout: 5000 }); + return out.status === 0 ? positive(Number(String(out.stdout).trim())) : null; + } catch { return null; } + } + const match = /\s*ExitTimeOut\s*<\/key>\s*<(integer|real)>\s*([0-9.]+)\s*<\/\1>/.exec(text); + return match ? positive(Number(match[2])) : null; +}; + +/** Milliseconds after SIGTERM to kill the groups: ExitTimeOut less a margin of 5 s or a quarter, whichever is less. */ +export const groupKillDeadlineMs = (exitTimeOutS) => { + const totalMs = (positive(exitTimeOutS) ?? DEFAULT_EXIT_TIMEOUT_S) * 1000; + return Math.floor(totalMs - Math.min(MAX_MARGIN_MS, totalMs / 4)); +}; + +/** + * `requestStop(signal)` stops the run loop and, on SIGTERM, arms the group + * deadline; `onExit` kills the groups left at exit. `deps` is a test seam. + */ +export const createSeatStop = ({ name, stop, home, deps = {} } = {}) => { + const { + arm = armSupervisedGroupDeadline, + kill = killSupervisedGroups, + readTimeout = readExitTimeOut, + } = deps; + return { + requestStop(signal) { + stop(); + if (signal !== 'SIGTERM') return null; + const deadlineMs = groupKillDeadlineMs(readTimeout(seatPlistPath(name, home))); + arm(deadlineMs); + return deadlineMs; + }, + onExit() { kill(); }, + }; +}; From 59879d66fc8b941c7d9420460b5aacfcb598adb1 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 09:32:50 -0700 Subject: [PATCH 4/5] test(seats): wait for the unrelated process's spawn event before it can fire unseen The pid-reuse test awaited `once(unrelated, 'spawn')` only after reading the grandchildren's pids. By then the event had already fired, so the test hung until node's test timeout. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu --- packages/seats/test/supervisor-tree-and-deadline.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/seats/test/supervisor-tree-and-deadline.test.js b/packages/seats/test/supervisor-tree-and-deadline.test.js index 1b1d9830eb..eb2aee56a5 100644 --- a/packages/seats/test/supervisor-tree-and-deadline.test.js +++ b/packages/seats/test/supervisor-tree-and-deadline.test.js @@ -90,11 +90,12 @@ test('a pid reused by another process is left alone; the descendant the snapshot const parent = startParent(pidFile, 2, true); // An unrelated process: not in the parent's tree at all. const unrelated = spawn('sleep', ['300'], { stdio: 'ignore' }); + const unrelatedStarted = once(unrelated, 'spawn'); let real; let reused; try { [real, reused] = await readPids(pidFile, 2); - await once(unrelated, 'spawn'); + await unrelatedStarted; const table = readProcessTable(); assert.ok(table, 'ps is unavailable, so the walk cannot run'); assert.ok(readProcess(reused), 'the reused stand-in is not running'); From 21ccefa87760e38c1d1d2d1c348f795b65b8eb9f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 09:55:20 -0700 Subject: [PATCH 5/5] fix(cli): arm the supervised-group deadline only for a seat with a plist The daemon spawns some seats, and those have no plist. The daemon stops a seat with SIGTERM and never sends SIGKILL, so nothing there needs launchd's 20 s fallback. Arming it meant a reassignment or config change cut an in-flight turn's tools at 15 s. - readSeatPlist reports whether the seat's plist exists. With no plist, SIGTERM arms nothing and the exit path alone kills the supervised groups. A plist that exists but has no ExitTimeOut, or can't be read, keeps launchd's 20 s default, because launchd will SIGKILL that seat. - process-group.js header: document the walk's two known gaps. A descendant reparented to 1 mid-walk is skipped. A process started after the snapshot is missed, and so is one that calls setsid after it. Tests use real processes. With a plist, the groups die before ExitTimeOut while the turn is still running. With no plist, the turn runs past 15 s and the groups die on exit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu --- cli/__tests__/seat-stop.process.test.mjs | 136 +++++++++++++++++++++++ cli/__tests__/seat-stop.test.mjs | 23 +++- cli/src/lib/seat-stop.js | 38 +++++-- packages/seats/src/process-group.js | 8 ++ 4 files changed, 191 insertions(+), 14 deletions(-) create mode 100644 cli/__tests__/seat-stop.process.test.mjs diff --git a/cli/__tests__/seat-stop.process.test.mjs b/cli/__tests__/seat-stop.process.test.mjs new file mode 100644 index 0000000000..d4433402f4 --- /dev/null +++ b/cli/__tests__/seat-stop.process.test.mjs @@ -0,0 +1,136 @@ +/** + * seat-stop with real processes, wired the way `agent run` wires it: SIGTERM + * calls requestStop, and the exit path calls onExit. The supervised child + * leads its own group and holds a grandchild (`sleep 300 &`), the shape of a + * CLI holding an MCP server. + * + * - A launchd seat (its plist exists): the groups die a margin before + * ExitTimeOut, while the turn is still running. + * - A daemon-spawned seat (no plist): nothing is armed, the turn runs past the + * 15 s a default deadline would have cut it at, and the groups die on exit. + */ +import { spawn } from 'child_process'; +import { once } from 'events'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { dirname, join } from 'path'; +import { fileURLToPath, pathToFileURL } from 'url'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const SEAT_STOP = pathToFileURL(join(HERE, '..', 'src', 'lib', 'seat-stop.js')).href; +const SEATS = join(HERE, '..', '..', 'packages', 'seats', 'src'); +const seats = (file) => JSON.stringify(pathToFileURL(join(SEATS, file)).href); +const POSIX = process.platform !== 'win32'; + +const isRunning = (pid) => { + try { process.kill(pid, 0); } catch (error) { return error.code !== 'ESRCH'; } + try { return !/^\d+ \(.*\) Z/.test(readFileSync(`/proc/${pid}/stat`, 'utf8')); } catch { return true; } +}; +const sleep = (ms) => new Promise((r) => { setTimeout(r, ms); }); +const waitFor = async (predicate, ms = 5000) => { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (predicate()) return true; + await sleep(25); + } + return predicate(); +}; +const reap = (pid) => { try { process.kill(pid, 'SIGKILL'); } catch { /* gone */ } }; + +const HOST = ({ home, pidFile, goFile }) => ` +import { spawn } from 'node:child_process'; +import { existsSync, readFileSync } from 'node:fs'; +import { createSeatStop } from ${JSON.stringify(SEAT_STOP)}; +import { superviseRun } from ${seats('supervisor.js')}; +import { createTraceStore } from ${seats('trace/store.js')}; +import { observeSpawn } from ${seats('runtime-observation.js')}; +import { spawnInOwnGroup, wantsOwnProcessGroup } from ${seats('process-group.js')}; +let stopping = false; +const seatStop = createSeatStop({ name: 'probe', stop: () => { stopping = true; }, home: ${JSON.stringify(home)} }); +process.on('SIGTERM', (signal) => { process.stdout.write('stopping deadline=' + seatStop.requestStop(signal) + '\\n'); }); +process.once('exit', seatStop.onExit); +const script = "sleep 300 & echo $! > '${pidFile}'; while [ ! -f '${goFile}' ]; do sleep 0.05; done; echo done; wait"; +const adapter = { name: 'sh', spawn: (prompt, ctx) => new Promise((resolve, reject) => { + const child = spawnInOwnGroup(spawn, 'sh', ['-c', script], { stdio: ['ignore', 'pipe', 'pipe'] }, { ownGroup: wantsOwnProcessGroup(ctx) }); + observeSpawn(child, ctx.onSpawn, {}); + child.stdout.on('data', (chunk) => { if (String(chunk).includes('done')) resolve({ text: 'done' }); }); + child.on('close', (code, signal) => reject(new Error('sh ended ' + (code ?? signal)))); + child.once('spawn', () => { + const poll = setInterval(() => { + if (!existsSync('${pidFile}') || !/^\\d+\\s*$/.test(readFileSync('${pidFile}', 'utf8'))) return; + clearInterval(poll); + process.stdout.write('ready ' + child.pid + '\\n'); + }, 20); + }); +}) }; +const out = await superviseRun({ adapter, prompt: 'p', ctx: {}, store: createTraceStore({ root: ${JSON.stringify(join(home, 'trace'))} }), + policy: { stallMs: 0, timeoutMs: 0 } }); +process.stdout.write('turn ' + out.outcome + '\\n'); +if (stopping) process.exit(0); +`; + +let home; +let state; +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'seat-stop-proc-')); + state = null; +}); +afterEach(() => { + if (state) { + if (state.grandchild) reap(state.grandchild); + if (state.leader) reap(state.leader); + state.host.kill('SIGKILL'); + } + rmSync(home, { recursive: true, force: true }); +}); + +const startHost = async () => { + const pidFile = join(home, 'grandchild.pid'); + const goFile = join(home, 'go'); + const script = join(home, 'host.mjs'); + writeFileSync(script, HOST({ home, pidFile, goFile })); + const host = spawn(process.execPath, [script], { stdio: ['ignore', 'pipe', 'inherit'] }); + state = { host, goFile, out: '' }; + host.stdout.on('data', (chunk) => { state.out += chunk; }); + state.exited = once(host, 'exit'); + expect(await waitFor(() => /ready \d+/.test(state.out))).toBe(true); + state.leader = Number(state.out.match(/ready (\d+)/)[1]); + expect(await waitFor(() => existsSync(pidFile) && /^\d+\s*$/.test(readFileSync(pidFile, 'utf8')))).toBe(true); + state.grandchild = Number(readFileSync(pidFile, 'utf8')); + return state; +}; + +const describePosix = POSIX ? describe : describe.skip; + +describePosix('seat-stop — real processes', () => { + test('a seat with a plist: the groups die a margin before ExitTimeOut, mid-turn', async () => { + // ExitTimeOut 2 s → deadline 1.5 s. + mkdirSync(join(home, 'Library', 'LaunchAgents'), { recursive: true }); + writeFileSync(join(home, 'Library', 'LaunchAgents', 'me.commonly.seat-probe.plist'), + 'ExitTimeOut2'); + await startHost(); + const sent = Date.now(); + state.host.kill('SIGTERM'); + expect(await waitFor(() => /stopping deadline=1500/.test(state.out))).toBe(true); + await sleep(750); + expect(isRunning(state.grandchild)).toBe(true); + expect(await waitFor(() => !isRunning(state.grandchild), 2000)).toBe(true); + expect(Date.now() - sent).toBeLessThan(2000); + expect(existsSync(state.goFile)).toBe(false); + }, 30000); + + test('a seat with no plist: nothing is armed, the turn runs past 15 s, and the groups die on exit', async () => { + await startHost(); + state.host.kill('SIGTERM'); + expect(await waitFor(() => /stopping deadline=null/.test(state.out))).toBe(true); + await sleep(16000); + expect(isRunning(state.grandchild)).toBe(true); + expect(isRunning(state.leader)).toBe(true); + writeFileSync(state.goFile, ''); + const [code] = await Promise.race([state.exited, sleep(5000).then(() => ['timeout'])]); + expect(state.out).toMatch(/turn ok/); + expect(code).toBe(0); + expect(await waitFor(() => !isRunning(state.grandchild))).toBe(true); + expect(await waitFor(() => !isRunning(state.leader))).toBe(true); + }, 40000); +}); diff --git a/cli/__tests__/seat-stop.test.mjs b/cli/__tests__/seat-stop.test.mjs index e75fbb39a9..42934793ae 100644 --- a/cli/__tests__/seat-stop.test.mjs +++ b/cli/__tests__/seat-stop.test.mjs @@ -9,7 +9,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs' import { tmpdir } from 'os'; import { join } from 'path'; import { - DEFAULT_EXIT_TIMEOUT_S, createSeatStop, groupKillDeadlineMs, readExitTimeOut, seatPlistPath, + DEFAULT_EXIT_TIMEOUT_S, createSeatStop, groupKillDeadlineMs, readExitTimeOut, readSeatPlist, seatPlistPath, } from '../src/lib/seat-stop.js'; const plist = (body) => ` @@ -92,12 +92,27 @@ describe('seat-stop — requestStop', () => { expect(kill).not.toHaveBeenCalled(); }); - test('SIGTERM on a seat with no plist uses the 20 s default', () => { - const { arm, seatStop } = make('daemon-seat'); - seatStop.requestStop('SIGTERM'); + test('SIGTERM on a seat with no plist (daemon-spawned) arms nothing: the exit path alone kills', () => { + const { stop, arm, seatStop } = make('daemon-seat'); + expect(seatStop.requestStop('SIGTERM')).toBeNull(); + expect(stop).toHaveBeenCalledTimes(1); + expect(arm).not.toHaveBeenCalled(); + }); + + test('SIGTERM on a seat whose plist has no ExitTimeOut uses launchd\'s 20 s default', () => { + writePlist('anvil', ''); + const { arm, seatStop } = make(); + expect(seatStop.requestStop('SIGTERM')).toBe(15000); expect(arm).toHaveBeenCalledWith(15000); }); + test('a plist that exists but cannot be read still counts as present, at the default', () => { + expect(readSeatPlist('/p.plist', { exists: () => true, readFile: () => { throw new Error('EACCES'); } })) + .toEqual({ present: true, exitTimeOut: null }); + expect(readSeatPlist(join(home, 'nope.plist'))).toEqual({ present: false, exitTimeOut: null }); + expect(readSeatPlist(null)).toEqual({ present: false, exitTimeOut: null }); + }); + test('SIGINT stops the loop and arms nothing', () => { const { stop, arm, seatStop } = make(); expect(seatStop.requestStop('SIGINT')).toBeNull(); diff --git a/cli/src/lib/seat-stop.js b/cli/src/lib/seat-stop.js index 78ceb4f452..b650dab298 100644 --- a/cli/src/lib/seat-stop.js +++ b/cli/src/lib/seat-stop.js @@ -5,15 +5,21 @@ * SIGTERM keeps the in-flight turn: the run loop stops polling and lets the * turn finish. A launchd seat is SIGKILLed ExitTimeOut seconds after SIGTERM, * and that SIGKILL cannot be caught, so a supervised group (outside the seat's - * own group, which launchd kills) would outlive the seat. So SIGTERM also arms - * a deadline a margin before ExitTimeOut, and the exit path kills whatever - * groups are left when the turn ends first. + * own group, which launchd kills) would outlive the seat. So on a launchd + * seat, SIGTERM also arms a deadline a margin before ExitTimeOut, and the exit + * path kills whatever groups are left when the turn ends first. * - * ExitTimeOut is read from the seat's plist and never written. Unset, zero or - * unreadable, it is launchd's default of 20 s. + * The deadline is armed only when the seat's plist exists. A seat without one + * (the daemon spawns it) is stopped with SIGTERM and never SIGKILLed, so it + * relies on the exit path alone, and a reassignment or config change does not + * cut the in-flight turn. + * + * ExitTimeOut is read from the seat's plist and never written. In a plist that + * exists, an unset, zero or unreadable value is launchd's default of 20 s, + * because launchd will SIGKILL that seat. */ import { spawnSync } from 'child_process'; -import { readFileSync } from 'fs'; +import { existsSync, readFileSync } from 'fs'; import { homedir } from 'os'; import { join } from 'path'; import { armSupervisedGroupDeadline, killSupervisedGroups } from '@commonlyai/seats/process-group'; @@ -49,6 +55,15 @@ export const readExitTimeOut = (path, { readFile = readFileSync, run = spawnSync return match ? positive(Number(match[2])) : null; }; +/** + * Whether the seat's plist exists, and its ExitTimeOut (null when unset or + * unreadable). Only an existing plist means launchd will SIGKILL this seat. + */ +export const readSeatPlist = (path, { exists = existsSync, ...deps } = {}) => { + if (!path || !exists(path)) return { present: false, exitTimeOut: null }; + return { present: true, exitTimeOut: readExitTimeOut(path, deps) }; +}; + /** Milliseconds after SIGTERM to kill the groups: ExitTimeOut less a margin of 5 s or a quarter, whichever is less. */ export const groupKillDeadlineMs = (exitTimeOutS) => { const totalMs = (positive(exitTimeOutS) ?? DEFAULT_EXIT_TIMEOUT_S) * 1000; @@ -56,20 +71,23 @@ export const groupKillDeadlineMs = (exitTimeOutS) => { }; /** - * `requestStop(signal)` stops the run loop and, on SIGTERM, arms the group - * deadline; `onExit` kills the groups left at exit. `deps` is a test seam. + * `requestStop(signal)` stops the run loop and, on SIGTERM to a seat with a + * plist, arms the group deadline; `onExit` kills the groups left at exit. + * `deps` is a test seam. */ export const createSeatStop = ({ name, stop, home, deps = {} } = {}) => { const { arm = armSupervisedGroupDeadline, kill = killSupervisedGroups, - readTimeout = readExitTimeOut, + readPlist = readSeatPlist, } = deps; return { requestStop(signal) { stop(); if (signal !== 'SIGTERM') return null; - const deadlineMs = groupKillDeadlineMs(readTimeout(seatPlistPath(name, home))); + const plist = readPlist(seatPlistPath(name, home)); + if (!plist.present) return null; + const deadlineMs = groupKillDeadlineMs(plist.exitTimeOut); arm(deadlineMs); return deadlineMs; }, diff --git a/packages/seats/src/process-group.js b/packages/seats/src/process-group.js index b016d0704d..286b856e23 100644 --- a/packages/seats/src/process-group.js +++ b/packages/seats/src/process-group.js @@ -21,6 +21,14 @@ * this host is left alone. Leaves go first so a pid's parent is still alive, * and its ppid still meaningful, when it is checked. * + * What the walk does not catch, by design, because each case would need the + * ppid match loosened: + * (a) a descendant reparented to 1 (its parent died) between the snapshot + * and its re-read is skipped: its ppid no longer matches; + * (b) a process started after the snapshot is not in it, and one that calls + * setsid after the snapshot leaves the group the group kill signals; + * either is missed. + * * A child in its own group is outside the wrapper's group, so launchd's kill * of the seat's group on exit no longer reaches it. `trackSupervisedGroup` * covers that: while any tracked group is live, the wrapper kills them all