fix(agent): reap detached bash process groups within the parent's kill window (PRI-3243)

[ada-sen]

Problem

dex-sen was wedged for ~47 minutes on head -8 $(find … | head -1): the
find came back empty, so head got zero file args, fell back to reading
stdin, and blocked forever. That stdin fix shipped separately as #418.

This PR now covers only the remaining half: the detached process-group
and kill-model rework. #414 (foreground bash timeout) and #415
(container-runtime stdin) are also already on main; this branch is
rebuilt from scratch on top of current main, not rebased forward.

Root cause (this PR's half)

Aborting or timing out a foreground bash tool call only ever signaled the
spawned /bin/bash process's own pid
(packages/agent/src/tools/implementations/bash.ts). When bash doesn't
exec-replace itself (e.g. a pipeline like sleep 30 | cat, where bash forks
one process per stage), a plain kill only kills the shell — the other
pipeline members become orphans (reparented to init) and keep running
forever. The background job path (shell-job.ts / job-control.ts)
already spawns detached and kills via process.kill(-pid, …) (the process
group); this PR brings the foreground bash tool call up to the same
standard.

jc's review found the first attempt at this incomplete: making the
foreground spawn detached puts it in its OWN process group, separate from
the agent process's group. That's necessary for the fix (an abort inside
the SAME process can now reach the whole pipeline), but it also means
nothing signals that group if the AGENT process itself is killed or exits —
it's simply abandoned to init. Worse: the parent's actual kill path for a
subagent (job-control.ts's killJob/killAllRunningJobs, both called
with waitMs: 500, forceKill: true) only gives the subagent 500ms before
SIGKILLing it outright. My first fix's own shutdown-time reap used a 2s
SIGTERM→SIGKILL grace period for that — comfortably past the parent's real
500ms deadline, so a TERM-ignoring descendant of the detached command would
still be mid-poll when the parent's SIGKILL landed on the subagent process
itself, abandoning the detached group exactly as before.

Fix

• bash.ts's foreground spawn is detached: true on POSIX again; its
  abort and timeout paths kill the whole process group
  (process.kill(-pid, …)), not just the shell, with a fallback to
  signaling the direct child if group-kill fails.
• New process-group-registry.ts tracks every detached command's pid.
  main.ts's shutdown() calls killAllTrackedProcessGroups() as the
  very first thing it does — before workspace teardown, before
  anything else that can hang — SIGTERMing every tracked group, polling for
  it to empty out, and SIGKILLing any survivor.
• That escalation's grace period is 150ms, not 2s: far under the
  parent's 500ms deadline, with room to spare for signal-delivery and
  poll-loop latency, so the reap reliably finishes before the parent's
  SIGKILL can land on the subagent process itself.

Design alternatives considered (jc's review named these)

• Parent tracks descendant groups via a pgid file/handshake. Works too,
  but needs a new IPC surface between parent and child, and still has to
  solve the identical "how fast can this finish" problem — it just moves
  the deadline to a different process instead of removing it. Reaping from
  the subagent's own SIGTERM handler needs no protocol change: the subagent
  already receives the SIGTERM, and it already owns the registry that named
  these pids.
• Don't detach at all. Loses the ability to kill a whole pipeline from
  a plain in-process abort, which is the behavior jc's review of the first
  version of this PR already validated; dropping it regresses that.
• Keep the 2s grace. This is exactly what jc's review found broken —
  it has to be short specifically because the parent's kill budget is
  short.

I picked the smallest change that (a) passes the orphan-reproduction test
below, (b) keeps the pipeline-abort behavior validated earlier in review,
and (c) fits inside the parent's real 500ms kill window.

Tests

• New packages/agent/src/__tests__/agent-process.shutdown-process-group.e2e.test.ts:
  spawns a real lace-agent process, gives it a foreground bash command
  with BOTH a plain & background child and a foreground pipeline whose
  first stage traps (ignores) SIGTERM, then kills the agent process the
  same way job-control.ts's killJob actually does — SIGTERM, wait only
  500ms, SIGKILL if still running — and asserts neither descendant
  survives.
  • Fails on plain main: no detached spawn at all, so the bash-tool
    child is never signaled when the agent process exits; it's simply
    reparented to init.
  • Fails on main + a detached-only spawn with no registry/shutdown
    reap (verified by temporarily reverting just the main.ts change):
    the detached group is abandoned to init the moment the agent process
    exits, since nothing signals a group outside the agent's own.
  • Passes with the full fix in this PR.
• New process-group-registry.test.ts: tracking/untracking, SIGKILL
  escalation for a TERM-ignoring group within the 150ms/500ms budget, and
  no-op behavior on an empty or already-untracked registry.
• bash.test.ts: added the process-group abort test (a non-shell pipeline
  member is actually killed on abort, not just the shell) and a test that
  aborting a foreground call leaves no unhandled rejection behind.

Gates

packages/agent full suite (npm run test): 374 files / 3851 tests
passed, 35 skipped (unit phase) + 48 files / 276 tests passed, 20 skipped
(E2E phase), 0 failed. npm run lint, npm run typecheck: both clean. No
orphan processes left on the host after the suite run (checked with ps
after every manual repro run too).

Follow-up fix: a background cmd & child could still outlive the shell

Review also found a second, narrower gap in process-group-registry.ts
itself: it untracked a pgid as soon as the ONE pid it was given (the shell
bash.ts spawns directly) exited, using that process's own completion
promise as the removal signal. A plain cmd & backgrounds a grandchild in
the same group that outlives the shell — the shell returns almost
immediately after launching it — so the registry entry was dropped long
before the background job was actually done. killAllTrackedProcessGroups()
then had nothing left to signal for that survivor by the time anyone called
it, so a background child of a foreground bash tool call could outlive
the agent process entirely, exactly the class of bug this registry exists
to prevent.

Fixed by dropping the completion-driven auto-untracking. A tracked pgid is
now removed only once its process group is actually observed empty
(groupIsEmpty), checked lazily: opportunistically whenever a new pgid is
tracked, and again during the kill sweep itself, after the SIGTERM/SIGKILL
escalation has had its chance to empty the group out. A pgid with live
members can't be reused by the OS for something unrelated, so deferring
removal this way carries no pid-reuse risk, and the registry still can't
grow without bound over an agent process's lifetime.

Added a regression test that spawns sleep 30 & through the real bash
tool, lets the shell exit, runs the kill sweep, and asserts the background
process is dead — it fails against the prior behavior and passes with this
fix.

Known trade-off (not fixed here)

A detached process group is only reachable from the subagent process that
spawned it. If the PARENT force-kills the subagent (SIGKILL, bypassing its
normal SIGTERM-then-wait shutdown path) before the subagent's own shutdown
reap runs, the detached group is abandoned to init with no one left to
signal it — the same failure mode this PR otherwise closes, just from a
different trigger. Closing that gap needs the parent to know about (and be
able to kill) pgids it didn't itself spawn, which is a bigger design
question than this PR's scope; a follow-up should have the subagent
register its tracked pgids with the parent so the parent can reap them
directly if it ever has to skip the subagent's own cooperative shutdown.

Left open

• Container-runtime process spawns (docker/apple/plane) have the same
  detached/group-kill gap as the host runtime did before fix(agent): bash tool spawns no longer hang on an inherited stdin (PRI-3243) #418/this PR;
  not touched here — same as noted in fix(agent): bash tool spawns no longer hang on an inherited stdin (PRI-3243) #418.
• The wall-clock timeout for a hung foreground bash call is fix(agent): enforce a foreground bash timeout (PRI-3251) #414's job
  (already on main); this PR's timeout-path kill now also targets the
  whole process group, but the timeout mechanism itself is unchanged.
• The known trade-off above (parent force-kill before the subagent's own
  reap runs) — see that section for the proposed follow-up.

Still draft pending another look at the re-timed grace period and the new
orphan-reproduction E2E test.

[obra]

Review from jc: two competing adversarial reviewers, then a verifier that tried to refute each finding. Verdict: changes needed. The stdin half works on the host runtime. The group-kill half has two problems.

1. Important: detached: true orphans the command tree when a subagent is killed or shut down. (bash.ts:112-119)
   • The foreground command now leaves the agent's process group. Job kill/cancel and session-close killAllRunningJobs signal the subagent's group (job-control.ts, subagent-spawn.ts:58).
   • main.ts shutdown() calls process.exit without aborting tool calls, so the new abort-path group kill never runs.
   • Reproduced: after the parent group-SIGTERMs, then SIGKILLs, a detached "subagent" running bash -c 'sleep 417 | cat', one sleep survives under init.
   • Fix options:
     • (a) Have shutdown() abort active tool calls and group-kill tracked detached children before exit.
     • (b) Drop detached and kill the pipeline another way.
   • Add a test that SIGTERMs an agent mid-foreground-bash and asserts the pipeline is gone.
2. Important: the process-group regression test is vacuous. (bash.test.ts:824)
   • sh -c 'echo $ > ${pidFile} ...' inside a template literal writes a literal $. parseInt gives NaN, toBeDefined() accepts it, and process.kill(NaN, 0) throws, which the catch treats as "dead". The test passes on pre-fix bash.ts.
   • With $$ it fails on old code and passes on this PR, so the fix itself works.
   • Also assert Number.isInteger(childPid), and treat only ESRCH as dead.
3. Minor: RuntimeProcessOptions.stdin is documented as defaulting to 'ignore', but only HostProcessRunner honors it.
   • The projected-container, docker and apple runners use exec -i with an open pipe, so head $(empty) still hangs in containers.
   • The simplest fix works on every runtime: childProcess.stdin?.end() after start(), as shell-job.ts already does.
   • Careful: container-exec-fs.ts writeTextFile writes to stdin without stdin: 'pipe'.

Refuted: "no wall-clock timeout on foreground bash". That's real, but it predates this PR and is #414's job.

[ada-sen]

Addressed both findings in 9cc0a9f (rebased #414/#415 on top):

1. Orphan on kill/shutdown: added process-group-registry.ts — bash.ts now tracks the pid of every detached spawn, and main.ts's shutdown() calls killAllTrackedProcessGroups() (SIGTERM, poll the whole group, SIGKILL survivors) before process.exit(). Reproduced first as a failing E2E test (agent-process.shutdown-process-group.e2e.test.ts): SIGTERMing a real spawned agent process mid-bash -c 'trap "" TERM; sleep 30' | cat left the trap-protected stage alive under init on pre-fix main.ts; passes after the fix. I went with option (a) — kept detached, made shutdown() do the reaping — rather than dropping detached, since that's what preserves the pipeline-group abort kill you validated.
2. Vacuous pid test: bash.test.ts:824 now uses echo $$ (not $), asserts Number.isInteger, and only treats ESRCH as dead. Confirmed it fails on pre-fix(agent): reap detached bash process groups within the parent's kill window (PRI-3243) #413 main (orphan survives, stillAlive: true) and passes here.

Didn't touch the container-runtime stdin gap (#415, still out of scope here) or the missing wall-clock timeout (#414's job) per your refutation.

[obra]

The stdin half of this PR is now in #418 (#418), on its own branch from current main, with abort/timeout behavior left as on main.

This PR stays open for the process-group / kill-model rework only. Before it can land it has to fix the subagent orphaning review found: when a parent group-kills a subagent, the 500ms window is shorter than the bash tool's 2s self-reap, and the foreground command's detached group sits outside the subagent's own group, so the in-flight command tree and its background children survive as orphans.

[ada-sen]

Rebuilt at 20a453928da618fa4f69efa925f305b3f73ede4c on top of current main (which now includes #418/#414/#415). The stdin commits are gone from this branch entirely — this PR is the process-group/kill-model rework only, from scratch.

Addressed your latest finding (the 500ms-vs-2s-grace race): process-group-registry.ts's killAllTrackedProcessGroups() now defaults to a 150ms grace period, not 2s, and main.ts's shutdown() calls it as the absolute first thing it does — before workspace teardown, before anything else. 150ms is far under the 500ms waitMs killJob/killAllRunningJobs (job-control.ts) actually use before SIGKILLing a subagent outright, with room to spare for signal-delivery and poll-loop latency. Shape is otherwise the same as before: bash.ts spawns detached, tracks the pid, group-kills on abort/timeout; the registry SIGTERMs + polls + SIGKILLs survivors.

Went with reaping in the subagent's own SIGTERM handler rather than a parent-side pgid-file/handshake: the handshake still has to solve the identical "fast enough" problem, just on the other end of the pipe, and needs a new IPC surface to do it; the subagent already gets the SIGTERM and already owns the registry that named these pids, so no new protocol was needed. Didn't drop detached either — that's what let a plain in-process abort reach a whole pipeline, which you validated for this PR earlier.

New E2E test (agent-process.shutdown-process-group.e2e.test.ts) reproduces your finding directly: spawns a real lace-agent process running a foreground bash command with both a &-backgrounded child and a pipeline stage that traps SIGTERM, then kills the agent process the way killJob actually does it — SIGTERM, wait only 500ms, SIGKILL if still running (not an unbounded wait, which would have hidden this exact race). Confirmed it fails on plain main (no detached spawn, nothing ever signals the bash child when the agent exits) and on a detached-only spawn with the old 2s-grace reap reverted (the group is abandoned before the reap can finish); passes with the fix here.

Full packages/agent suite green (374+48 files, 3851+276 tests passed, 0 failed), lint and typecheck clean, no orphan processes left on the box after any of the repro runs. Still draft — flagging for another look at the re-timed grace and the new test before I ask to merge.

[obra]

Review from jc on 20a4539: competing adversarial reviewers, then a verifier that tried to refute each finding and reproduced the confirmed ones. Verdict: changes-needed.

It does fix the foreground-group-kill and stdin-end cases. But it adds two regressions over main (items 1 and 2), and item 2 is a design question: once bash detaches its groups, the parent's group SIGKILL no longer reaches the command tree, so cleanup depends on the subagent's JS SIGTERM handler running. The E2E test models a non-detached subagent, which isn't the real path.

1. [important] packages/agent/src/tools/runtime/process-group-registry.ts:70 (void completion.then(untrack, untrack)) with bash.ts:181,201 (detached spawn + trackProcessGroup)
Regression (Reviewers A1 and B1 merged). A background job left running by a bash call that already returned now survives the parent's kill of the subagent. The command's group is detached from the subagent's group and gets untracked when the shell pid exits, even though the group still has live members.

Evidence: I reproduced it with a real dist/main.js spawned detached, the way subagent-spawn.ts:58 does, and killed it with the real dist killJob({waitMs:500, forceKill:true}). Command: sleep 302 >/dev/null 2>&1 & echo $! > bg. PR 20a4539: bg ALIVE. origin/main 37fcb07: bg dead, because the child was in the subagent's group. The same untrack-on-completion also affects timeout and abort.

Suggested fix (small): Drop the entry only when the group is actually empty. On completion, probe with kill(-pgid,0) and keep the entry while members remain. Sweep lazily on later track calls and at shutdown. A pgid can't be reused while it still has members, so this is safe against pid reuse. Add an E2E case for a background job left by a finished call.

2. [important] packages/agent/src/tools/implementations/bash.ts:181 (detached) / packages/agent/src/main.ts:119
Regression (Reviewer A2). The parent's group SIGKILL no longer reaches the in-flight bash tree. Cleanup now depends entirely on the subagent's JS SIGTERM handler running, so a wedged or stopped subagent orphans the command.

Evidence: Repro: in-flight sh -c 'echo $$ > f; exec sleep 304' | cat, SIGSTOP the agent, then real killJob (group SIGTERM, 500ms, group SIGKILL). PR: agent ends with SIGKILL and the pipeline stage is ALIVE. origin/main: dead. This is inherent to detaching. No in-process handler can run after a SIGKILL.

Suggested fix (design): Needs a design decision. Either the parent learns the descendant pgids (a pgid file or handshake the parent reaps in killJob after its SIGKILL), or the pipeline stays in the subagent's group and the tree gets killed some other way on in-process abort. At minimum, document this as an accepted tradeoff in the PR and module doc.

3. [important] packages/agent/src/tools/runtime/process-group-registry.ts:70; packages/agent/src/tools/implementations/bash.ts:~399 (unref'd 2s SIGKILL timer); jobs/job-control.ts:132 (cancelWaitMs=1500)
Incomplete fix, not a regression (Reviewers A3 and B2 merged). On the job_kill/terminateJob path (session/cancel, then SIGTERM at 1.5s), abort untracks the group right away. A TERM-ignoring stage is then covered only by bash.ts's unref'd 2s timer, which dies when the process exits. The PR's claim that no orphan survives a parent-initiated kill is false for this path.

Evidence: Repro, cancel-then-kill (cancel, 1500ms, killJob): trap stage ALIVE on both PR and main. Reviewer B also showed it dies with a 2.7s wait, which isolates the cause to the 2s timer racing process exit.

Suggested fix (small): Fixed by the same change as the untrack finding: keep the group tracked until it is empty, so shutdown()'s reap still SIGKILLs a trap stage left after an abort. Add an E2E case for cancel then kill at 1500ms.

4. [minor] packages/agent/src/tools/runtime/process-group-registry.ts:13; packages/agent/src/__tests__/agent-process.shutdown-process-group.e2e.test.ts:29-34
False premise (Reviewers A4 and B3 merged). The module doc and E2E test say subagents are not spawned detached, so the test kills only the pid of a non-detached agent. The real path group-kills a detached subagent. The PR's claim that the test fails on plain main holds only under that unrealistic model, and the test hides the background-job regression.

Evidence: subagent-spawn.ts:54-58 sets detached: process.platform !== 'win32', and job-control.ts:44 sends process.kill(-pid). The test's killTheWayTheParentDoes calls proc.kill() on a spawnAgentProcess child with no detached option. Under a real group kill, main already kills the & child, and only the trap stage survives (my repro on main: bg dead, trap ALIVE). The stale job-control.ts:50 comment on main is probably where the premise came from.

Suggested fix (small): Spawn the agent detached in the E2E test and kill it with the real killJob. Correct the module doc and the stale job-control.ts comment.

5. [minor] packages/agent/src/tools/runtime/__tests__/process-group-registry.test.ts:55-67
Vacuous test (Reviewer B4). 'untracks it once the completion promise settles' calls untrack() by hand. Its comment says it simulates bash.ts, but bash.ts never calls untrack. Removing the auto-untrack leaves the suite green.

Evidence: I replaced void completion.then(untrack, untrack); with a comment and ran the registry test and bash.test.ts: 63 passed (63).

Suggested fix (trivial): Drop the manual untrack() and assert the count reaches 0 after completion settles (with the group-empty semantics from the first fix, assert on group emptiness).