no-sandbox: reap the agent process tree on teardown (process-group kill)

[mattpocock]

Background

Follow-up split from #590 and documented in docs/adr/0019-completion-timeout-for-hanging-process.md.

The #590 fix introduces a completion timeout: once the agent emits the completion signal, Sandcastle waits one minute and, if the process is still silent (a hanging process — typically a spawned gh/git child or a long-lived MCP server holding the sandbox exec's stdout open), it force-completes the iteration successfully and abandons the process.

For container-based sandbox providers this is fine — container teardown (docker rm -f, src/sandboxes/docker.ts:238) kills the abandoned process. But for the no-sandbox provider, close() is a no-op (src/sandboxes/no-sandbox.ts:150-152) and there is no proc.kill() anywhere in the codebase, so the abandoned agent process and the child holding the fd leak on the host.

This leak already exists on the timeout/abort paths, but the #590 fix promotes it from a rare edge case to the common success path for no-sandbox users.

Why it's non-trivial

proc.kill() only reaps the direct sh -c child, not the grandchild holding the pipe. Reaping the whole tree means either:

• spawning detached (detached: true) and signalling the process group via process.kill(-pid), or
• a tree-kill approach,

plus tracking the active exec proc on the no-sandbox handle (not tracked today). Both carry their own failure modes — signal handling and detached-process semantics differ on Windows, which is a supported host.

Scope

• Track the active exec process(es) on the no-sandbox handle.
• On close() (and ideally on the completion-timeout/abort/idle paths), reap the process tree, not just the direct child.
• Verify behaviour on Windows hosts.

Out of scope

The #590 completion-timeout behaviour itself — that ships independently and benefits all providers.

[github-actions]

Triage — read-only first pass

Claim verification

Issue claim	Status
close() is a no-op	✅ src/sandboxes/no-sandbox.ts:150-152
No proc.kill() anywhere	✅ Repo-wide grep finds only process.kill(pid, 0) as a liveness check (docs/adr/0007-worktree-locking.md:32) — no signalling kills anywhere
Container teardown reaps the tree on docker	✅ src/sandboxes/docker.ts:238 via docker rm -f, registered with the shared shutdown registry at :245
Windows is a supported host	✅ ADR-0006 + src/mountUtils.ts:121-175 + WorktreeManager.windowsPath.test.ts + SessionStore.windowsPath.test.ts
Documented in docs/adr/0019-completion-timeout-for-hanging-process.md	⚠️ The ADR file doesn't exist yet. Latest on disk is 0017-sandbox-owned-sync-base-ref.md. The completion-timeout design lives only in the planning comments on #590; the code for it (completionTimeoutSeconds, signal-aware racing) isn't in main either. #766 is a follow-up to planned work, not shipped work — worth deciding whether this ships before, with, or after #590.

Difficulty: medium

Mechanically small (one provider file, a handful of LoC, a couple of tests). Two real footguns:

1. The leak is wider than the issue states. Issue lists close() + the completion-timeout success path. Exploration surfaced four more paths in the same provider with the same shape:

   • Orchestrator Effect.raceFirst abandonment on idle-timeout / abort win (src/Orchestrator.ts:157-163); cleanup at :143-155, 168 only touches JS timers and event listeners, not the spawned proc.
   • interactiveExec is the same pattern as exec (no-sandbox.ts:128-148) — same Promise around spawn, no kill on rejection.
   • Effect.tryPromise wrapping in SandboxFactory.ts:102-110 has no cancellation hook into the underlying Promise — so even after close() learns to kill, a raced-away exec is not killed until the outer acquireUseRelease release fires.
   • No registerShutdown(...) registration in no-sandbox.ts. Docker registers a sync teardown at docker.ts:245; no-sandbox does not, so SIGINT/SIGTERM of the parent Sandcastle process also strands children.

2. Windows is genuinely tricky. No precedent in the repo. No tree-kill / taskkill dep, no detached: true usage, no process.kill(-pid) anywhere. Both candidate approaches require platform-conditional code that this codebase hasn't needed before.

Relevant files

Primary:

• src/sandboxes/no-sandbox.ts — track active procs, kill in close(), plumb cleanup into exec/interactiveExec failure paths.
• src/sandboxes/no-sandbox.test.ts — currently covers metadata, exec, streaming, cwd, sudo no-op, env, interactiveExec, tail bounding, and the no-op close(). Nothing covers process cleanup or leak detection.

Secondary (depends on chosen scope):

• src/SandboxProvider.ts:195-226 — NoSandboxHandle is public API; if observable semantics of close() change (e.g. now awaits child exit), it should be documented.
• src/Orchestrator.ts:22-172 — race construction is the trigger for orphaning; cleanup at :143-155, 168 is where any per-exec abort hook would land if scope expands.
• src/shutdownRegistry.ts — the existing pattern for SIGINT/SIGTERM cleanup that no-sandbox should plug into the same way docker does at docker.ts:245.

Open questions

1. Scope. Issue says close() "and ideally" timeout/abort. The leak is wider (above). Is this PR fixing only close(), or all five paths? My recommendation: do the active-procs Set + close() sweep + registerShutdown sync sweep in one PR — that single mechanism covers close(), SIGINT/SIGTERM, and the orchestrator-race-orphans path on the next iteration's close, with no Orchestrator surgery. Per-exec abort hooks for the raced-away path are a follow-up.
2. Detached process group vs tree-kill? No dep exists for either. detached: true + process.kill(-pid, 'SIGTERM') is zero-dep and the documented Node idiom, but Windows-incompatible. tree-kill (npm) hides the platform branch (taskkill /F /T /PID on win32) in ~20 LoC of vendorable code. The repo's preference for zero-dep solutions (shutdown registry is hand-rolled) leans toward vendoring the branch rather than adding a dep.
3. What does Windows actually need? No taskkill precedent in the repo. Options: (a) call taskkill /F /T /PID directly, (b) vendor tree-kill, (c) add the dep. Worth noting: ADR-0006 establishes Windows hosts mostly run docker containers (which handle their own children), so no-sandbox-on-Windows is a lightly-exercised configuration in practice — but still supported.
4. Sync vs async kill on shutdown. shutdownRegistry.ts runs sync callbacks before process.exit(1). proc.kill() returns immediately and the process dies asynchronously — probably fine; mirrors docker's fire-and-forget execFileSync("docker", ["rm", "-f", ...]) semantics.
5. Grace period. Completion signal not detected when emitted as agent's final line; loop runs to idle timeout #590's completion-timeout already gives the agent 60s of silence before force-completion. If close() then kills hard immediately, you may truncate any in-flight trailing output. Consider SIGTERM-then-SIGKILL with a small grace, mirroring docker's stop-then-rm.
6. Sequencing vs Completion signal not detected when emitted as agent's final line; loop runs to idle timeout #590. The leak exists on rare abort/idle paths today, so no-sandbox: reap the agent process tree on teardown (process-group kill) #766 is shippable independently. But the regression test that demonstrates the "common success-path leak" can't be written until Completion signal not detected when emitted as agent's final line; loop runs to idle timeout #590 lands.

Possible approach

Sketch — single-provider scope, no Orchestrator changes:

1. In the noSandbox() factory closure, hold an activeProcs: Set<ChildProcess>. exec and interactiveExec add the proc on spawn, remove on 'close'.
2. Spawn with detached: true on POSIX so children get a fresh pgid (load-bearing — this is what makes process.kill(-pid) reap the whole tree). Leave detached off on Windows; use taskkill /T /F instead.
3. In close(), iterate activeProcs:
   • POSIX: process.kill(-proc.pid!, 'SIGTERM'), then SIGKILL after a short grace.
   • Windows: spawn('taskkill', ['/pid', String(proc.pid), '/T', '/F']) (or vendored tree-kill).
4. registerShutdown(killAllSync) at provider create-time, mirroring docker.ts:245, so SIGINT/SIGTERM of the parent also reaps.
5. For the raced-away orphan path: rely on the next close() to sweep (matches the "container teardown reaps abandoned procs" mental model that already works for docker — the active-procs Set is the analogue of the container). Per-exec abort hooks are a follow-up if needed.
6. Tests: spawn sleep 30, call close(), assert the PID is gone (process.kill(pid, 0) throws ESRCH). Gate the POSIX branch via process.platform and skip on Windows; the Windows branch is the one thing this PR can't fully validate from CI without changing the CI matrix (currently ubuntu-latest only).