Kill processes left in a destroyed worktree environment - #1696
Conversation
![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.
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>
…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>
… sweep - Add stopProcessGroupLeaderFirst to @bb/process-utils: SIGTERM the group leader alone so the provider bridge can drive its CLI down gracefully; SIGTERM the group only after the leader exits with members alive, poll the group, and SIGKILL the group on timeout. Both provider shutdown paths use it. - Real-process tests for leader-first ordering and group escalation. - Call out that the destroy-time cwd sweep kills any user process rooted in the workspace, in code comments, docs/worktrees.md, and the environments guide chapter. Co-Authored-By: Claude <noreply@anthropic.com>
b3f6d2e to
0aaef37
Compare
|
Applied the two changes from the independent review (https://get-bb.github.io/reports/issues/1647.html, section "PR review"), rebased on
|
1 participant
Fixes #1647
Independent report: https://get-bb.github.io/reports/issues/1647.html (section "PR review").
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,isProcessGroupAlive,stopProcessGroupLeaderFirst,listProcessesWithCwdUnder, andkillProcessesWithCwdUnder(Linux/proc/*/cwd; macOSlsof -d cwd; no-op on Windows).runtime-provider-process.ts) and setup scripts (provisioning.ts) spawn as process-group leaders. The setup script already did this; the helper is now shared. ACP agents (agent-connection.ts) are unchanged: they stay in the ACP bridge's process group.RuntimeManager.destroyEnvironmentsweeps and kills every process still rooted in a managed workspace (SIGTERM, 2 s grace, rescan, SIGKILL) before it removes the directory, and logs the reaped pids.forgetEnvironmentand idle eviction do not sweep because the workspace stays on disk.The cwd sweep is ownership-agnostic
The sweep kills any process of the current user whose cwd is inside the destroyed managed workspace, including processes bb never started: a shell you
cd'd into the worktree, an editor terminal, a debugger. The directory is about to be removed, so those processes lose their cwd anyway. This is deliberate and documented in the code comment at the sweep, indocs/worktrees.md, and in thebb guide environmentschapter. Personal (non-managed) workspaces are never swept.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, rescans for respawned processes, ignores the current process, sibling directories, and symlinked roots; leader-first stop lets the leader shut its own child down (a group-wide SIGTERM would reach the child first); a member that ignores SIGTERM and outlives the leader is reaped by the group escalation.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/agent-runtime,@bb/host-daemon,@bb/templates: all pass.