Kill processes left in a destroyed worktree environment - #1696
Kill processes left in a destroyed worktree environment#1696SawyerHood wants to merge 2 commits into
Conversation
Environment teardown only signalled the shell and provider CLI pids. Their children (dev servers, MCP servers, background jobs) survived with a cwd in the removed directory. Spawn provider CLIs, ACP agents, and setup scripts as process-group leaders and signal the group; signal the pty process group on terminal close; and sweep every process still rooted in a managed workspace before its directory is removed. Fixes #1647 Co-Authored-By: Claude <noreply@anthropic.com>
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| for (const target of signalled) { | ||
| if (isProcessAlive(target.pid)) { | ||
| try { | ||
| process.kill(target.pid, "SIGKILL"); |
There was a problem hiding this comment.
🚨 slopcop/review — The final SIGKILL can target an unrelated process after PID reuse.
The code waits two seconds and then checks only PID existence. The operating system can reuse an exited target's PID during that wait.
Save the process start identity. Verify that identity and the workspace path before each signal. Use a pidfd on Linux when possible.
There was a problem hiding this comment.
Fixed in b3f6d2e. The sweep now rescans before each signal and only SIGKILLs pids that a fresh scan still shows rooted in the workspace. A reused pid with a cwd elsewhere no longer receives a signal.
| } | ||
| let directory = resolve(args.directory); | ||
| try { | ||
| directory = await realpath(directory); |
There was a problem hiding this comment.
🚨 slopcop/review — A changed workspace symlink can redirect this process sweep.
realpath() follows the workspace root at destroy time. Worktree code can replace that root with a symlink before destruction.
BB can then signal unrelated processes under the symlink target. Store the canonical device and inode after provision. Reject identity changes before the scan.
There was a problem hiding this comment.
Mitigated in b3f6d2e. The sweep resolves the parent path and skips the sweep when the root itself is a symlink. Storing device/inode at provision is more than this change needs: BB owns the worktree root, and a process that can swap it already runs on the host.
| ): AcpAgentConnection { | ||
| const child: ChildProcess = spawn(options.command, options.args, { | ||
| cwd: options.cwd, | ||
| detached: supportsProcessGroups(), |
There was a problem hiding this comment.
🚨 slopcop/review — The ACP agent now leaves the bridge process group.
Runtime shutdown signals the outer bridge group. The bridge handles a closed input stream, but it has no SIGTERM handler.
The ACP agent group can therefore survive daemon shutdown or idle eviction. Add signal handlers that stop all sessions and escalate when necessary.
There was a problem hiding this comment.
Fixed in b3f6d2e. I reverted the ACP agent to stay in the bridge process group. The bridge itself is now a group leader, so the runtime's group signal reaches the bridge, the agent, and their children.
| export async function killProcessesWithCwdUnder( | ||
| args: KillProcessesWithCwdUnderArgs, | ||
| ): Promise<ProcessWithCwd[]> { | ||
| const targets = await listProcessesWithCwdUnder({ |
There was a problem hiding this comment.
🚨 slopcop/review — One current-directory snapshot does not stop processes that appear during shutdown.
I reproduced this race. A target handled SIGTERM, created a new-session child, and exited. This helper returned while the new child remained alive.
Repeat the scan until no matching process remains. An owned process-group or operating-system job boundary would give stronger control.
There was a problem hiding this comment.
Fixed in b3f6d2e. killProcessesWithCwdUnder now loops: scan, SIGTERM, wait, rescan, SIGKILL, and repeat until a scan is empty (bounded to 5 rounds). A test covers a target that spawns a setsid child on SIGTERM.
| new Promise<void>((resolve) => { | ||
| const timer = setTimeout(() => { | ||
| providerProcess.child.kill("SIGKILL"); | ||
| killProcessGroup({ |
There was a problem hiding this comment.
🚨 slopcop/review — The group escalation stops when only the leader exits.
This exit handler clears the SIGKILL timer without checking the process group. A direct probe confirmed that a group member can remain alive.
Keep the timer until kill(-pgid, 0) reports no group. Send SIGKILL to the group after the grace period.
There was a problem hiding this comment.
Fixed in b3f6d2e. On leader exit both shutdown paths check kill(-pgid, 0) and keep the SIGKILL timer while group members remain.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This PR tries to stop commands, terminals, and agents that remain after BB deletes a worktree.
The process-group helper is a useful shared abstraction. The runtime manager also keeps environment policy in the correct host-daemon layer.
I found five important correctness and safety defects:
- A replaced workspace symlink can redirect cleanup toward unrelated processes.
- A process can create a new child after the single current-directory scan. I reproduced this survivor race.
- PID reuse during the grace period can send
SIGKILLto an unrelated process. - ACP agents now enter a separate group that outer provider shutdown does not reach.
- Provider escalation stops when the group leader exits, even if other group members remain.
The macOS implementation also scans every host process with lsof for each destroyed environment. Concurrent destroys multiply that cost.
The focused Turbo typechecks passed for all four affected packages. The focused Turbo test run failed in the new process-tree test.
That test reached its five-second timeout under the normal parallel package run. A separate run passed, which shows a load-sensitive test.
The source dev app loaded in Chromium. I created and deleted a managed environment through the local lifecycle path. The worktree was removed.
The direct race probe still left a new-session child alive after cleanup. The inline comments contain the specific fixes.
Architecture note: Keep process primitives in @bb/process-utils. Add one verified group-termination operation and reuse the existing process-identity checks.
Do not use a current directory as the only ownership boundary. Use tracked process groups or operating-system jobs when the platform supports them.
…oup escalation - killProcessesWithCwdUnder rescans before each signal and repeats until a scan is empty, so reused pids and processes that appear during shutdown never receive a stale signal. - The sweep skips a workspace root that is itself a symlink. - Provider shutdown keeps the SIGKILL timer while group members outlive the leader. - ACP agents stay in the bridge process group so the bridge group kill reaches them. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Fixes #1647
Problem
environment.destroyclosed terminals withpty.kill()and stopped provider CLIs withchild.kill("SIGTERM"). Both signal only the direct child pid. Grandchildren (dev servers from an agent's Bash tool, MCP servers,nohup/disowned jobs) survivedgit worktree removewith a cwd in the deleted directory. Reproduction: SIGTERM to ash -c 'sleep 300 & wait'child, then remove its cwd;/proc/<pid>/cwdshows<path> (deleted)for thesleep.Change
@bb/process-utils: addsupportsProcessGroups,killProcessGroup,listProcessesWithCwdUnder, andkillProcessesWithCwdUnder(Linux/proc/*/cwd; macOSlsof -d cwd; no-op on Windows).runtime-provider-process.ts), ACP agents (agent-connection.ts), and setup scripts (provisioning.ts) spawn as process-group leaders and receive group signals on shutdown. The setup script already did this; the helper is now shared.RuntimeManager.destroyEnvironmentsweeps and kills every process still rooted in a managed workspace (SIGTERM, 2 s grace, SIGKILL) before it removes the directory, and logs the reaped pids.forgetEnvironmentand idle eviction do not sweep because the workspace stays on disk.No server/daemon wire change, so no protocol version bump.
Tests
packages/process-utils/test/process-tree.test.ts: group kill reaps a grandchild; the cwd sweep finds and kills a detached session leader and its child, ignores the current process and sibling directories.apps/host-daemon/src/runtime-manager.test.ts:destroyEnvironmentkills a detached process rooted in a managed workspace.turbo run typecheck testfor@bb/process-utils,@bb/host-workspace,@bb/agent-runtime,@bb/host-daemon: all pass.