fix(runner): destroy ACP session on run completion — orphaned claude-agent-acp process leak

[mmabrouk]

Context

The OSS team's shared dev runner leaked one orphaned claude-agent-acp Node process per completed agent run. Under steady automated traffic (~1 run/min), this accumulated to 304 orphaned processes and 5.2 GB RSS in about six hours, and contributed to a machine-wide memory incident on the box (62 GB total, 60 GB used, swap full at 31 GB).

Root cause: each run boots a local sandbox-agent daemon, which spawns a claude-agent-acp child process to drive the harness. Teardown only sends SIGTERM/SIGKILL to the daemon. The daemon does not cascade that kill to its ACP child, so the child reparents to PID 1 inside the container and never exits. The graceful shutdown path, an ACP session/cancel via destroySession, was only ever sent on the HITL-pause path. Normal completions and error paths went straight from a live session to destroySandbox(), which hard-kills the daemon but leaves the ACP child behind.

Changes

services/runner/src/engines/sandbox_agent.ts: the run's finally block now sends destroySession(session.id) (ACP session/cancel) before destroySandbox(), so the harness gets a chance to exit its ACP child cleanly on every completion path, not just the pause path. A new sessionDestroyRequested flag guards against a redundant double-send when the HITL pause controller already cancelled the session.

Before: finally block called mcpAbort.abort() → closeToolMcp() → destroySandbox() → dispose(), with no session cancel on normal/error completion.

After: finally block calls mcpAbort.abort() → closeToolMcp() → destroySession(session.id) (skipped if already sent) → destroySandbox() → dispose().

Scope / risk

One file, teardown path only. No change to run semantics, streaming, or tracing. The only behavior change is an extra best-effort ACP cancel call on teardown; it's wrapped in .catch(() => {}) so a harness that already exited is a no-op.

This is a mitigation, not a guarantee. The robust fix belongs upstream in rivet-dev/sandbox-agent: the daemon should either forward its own kill signal to children or run them in a process group it can kill together, so the ACP child cannot outlive the daemon even under a hard SIGKILL. An upstream issue draft exists but hasn't been filed yet.

Tests / notes

• pnpm test in services/runner (556 tests) passes.

How to QA

Prerequisites: any stack with the runner and the claude harness running (local dev stack or the shared dev box).

Steps:

1. Run an agent turn end to end (normal completion).
2. On the host: docker exec <runner-container> ps -o pid,ppid,args | grep claude-agent-acp.

Expected result: no claude-agent-acp processes with ppid=1 accumulate after runs complete. Before this fix, every completed run left one such orphan behind (confirmed during the incident: 304 orphans / 5.2 GB RSS after ~6 hours of automated traffic).

Automated tests:

cd services/runner && pnpm test

Edge cases: the HITL-pause path already called destroySession; confirm a paused-then-resumed-then-completed run doesn't double-cancel (guarded by sessionDestroyRequested, exercised in the test suite).

https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw

[mmabrouk]

@coderabbitai review

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agenta-documentation	Ready	Preview, Comment	Jul 7, 2026 12:16am

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 02f4d968-74ec-4da2-bb0a-54a741f3c5f4

📥 Commits

Reviewing files that changed from the base of the PR and between 27823ae and 832a1dc.

📒 Files selected for processing (1)

• services/runner/src/engines/sandbox_agent.ts

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved session cleanup after runs complete or fail, reducing the chance of leftover background processes.
  • Made shutdown behavior more reliable when approvals are paused or resumed, helping prevent stuck or duplicate session endings.

Walkthrough

The change modifies runSandboxAgent in the sandbox engine to track a session handle and a cancellation flag at the outer function scope. This ensures a graceful session/cancel (destroySession) is attempted before destroySandbox() in the finally block, avoiding a leaked ACP adapter subprocess when cancellation wasn't already triggered by the pause path.

Changes

Sandbox agent session cleanup

Layer / File(s)	Summary
Session state tracking and creation wiring services/runner/src/engines/sandbox_agent.ts	Introduces outer-scoped session and sessionDestroyRequested variables and assigns the created session result into the outer handle instead of a local const.
Pause teardown flag and finally cleanup services/runner/src/engines/sandbox_agent.ts	Pause controller teardown marks sessionDestroyRequested = true before cancelling; finally conditionally performs a graceful destroySession before destroySandbox() when cancellation wasn't already requested.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runSandboxAgent
  participant PendingApprovalPauseController
  participant Sandbox
  participant ACPSession

  runSandboxAgent->>Sandbox: createSession()
  Sandbox->>ACPSession: spawn ACP adapter subprocess
  alt HITL pause triggered
    PendingApprovalPauseController->>runSandboxAgent: set sessionDestroyRequested=true
    PendingApprovalPauseController->>ACPSession: session/cancel
  end
  runSandboxAgent->>runSandboxAgent: finally block
  alt session exists and not sessionDestroyRequested
    runSandboxAgent->>Sandbox: destroySession(session.id)
    Sandbox->>ACPSession: session/cancel
  end
  runSandboxAgent->>Sandbox: destroySandbox()

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: cancelling ACP sessions on teardown to prevent orphaned processes.
Description check	✅ Passed	The description accurately explains the teardown leak, the fix, and validation, and stays on-topic.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/runner-acp-orphan-leak

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[mmabrouk]

lgtm