Skip to content

fix(server): MCP plugin servers die with the Claude CLI's process group - #19045

Closed
akushonkamen wants to merge 1 commit into
pingdotgg:mainfrom
akushonkamen:fix/3089-orphaned-mcp-bun
Closed

akushonkamen wants to merge 1 commit into
pingdotgg:mainfrom
akushonkamen:fix/3089-orphaned-mcp-bun

Conversation

@akushonkamen

Copy link
Copy Markdown

Problem

Closing or deleting a session leaked the MCP plugin servers the Claude CLI spawns under the hood (each plugin's bun server.ts): one orphan per session, each logging into a stdout pipe nobody drains, wedging a CPU core until kill -9 (#3089, two machines, reproduces on 0.0.33).

Reproduced outside the desktop app by driving @anthropic-ai/claude-agent-sdk exactly the way the Claude adapter does (a stdio plugin launched through bun run --cwd … --shell=bun --silent start, Claude CLI 2.1.241, macOS 15 arm64). After query.close() the CLI is gone but the plugin tree survives, still inside the CLI's process group:

===== after query.close(): cliGone=true =====
29045     1 28987 S  0.0 0:00.01 bun run --cwd .../fake-plugin --shell=bun --silent start
29048 29045 28987 S  0.2 0:00.07 .../bun server.ts

(The --plugin-dir path — the CLI reading the plugin's own .mcp.json, the real telegram-plugin shape — reproduces the identical orphan pair.)

Root cause: the SDK's ProcessTransport spawns the CLI non-detached and on close signals only the CLI PID (SIGTERM, then SIGKILL after 5 s). The CLI's own shutdown does not reap the plugin server tree, so nothing owns those processes anymore. The reported "SIGTERM respawn" did not reproduce on CLI 2.1.241 in either the setMcpServers or --plugin-dir path; consistent with @iskyiskyisky's Detail 1, the wedge alone explains it — a server stuck in the dead-pipe write never reaches its SIGTERM handler.

Change

layerQueryRunner now hands the SDK a spawnClaudeCodeProcess implementation (makeClaudeProcessGroupSpawner, wired with HostProcess.Platform) that:

  • spawns the CLI detached, so it leads its own POSIX process group (precondition for the rest; without it a group signal would hit the server's own group);
  • makes every kill signal that group after Node's own PID kill, so the transport's SIGTERM → SIGKILL escalation reaps the CLI together with every plugin server, wedged or not;
  • SIGKILLs whatever remains in the group when the CLI exits for any reason — the transport's graceful close ends stdin and the CLI exits by itself, no kill involved, which is exactly the path that leaked the tree before. (Found by E2E: the kill patch alone still leaked orphans; the exit hook is what closes it.)

Caller-supplied spawnClaudeCodeProcess (remote runtimes, tests) keeps ownership of its process; on Windows the SDK's own spawn is kept.

Scope and approval

Focused fix for the triaged bug issue #3089 (bug, needs-triage). No public behavior changes beyond reclamation of leaked processes.

Verification

Focused tests (ClaudeAdapterV2.process-group.test.ts), real processes, macOS 15 arm64:

  • reaps the whole CLI process group on kill, including plugin-server descendants
  • reaps the whole CLI process group when the CLI exits by itself (the graceful-close leak shape)
  • reaps a wedged descendant that ignores SIGTERM when the CLI exits (trap "" TERM grandchild, the [Bug]: MCP plugin processes (bun server.ts) are never killed when closing conversations #3089 wedge)
  • spawns the CLI detached, so it leads its own process group
  • hands the SDK a process-group spawn for the CLI (wiring through layerQueryRunner, mock-spied query)

Red → green: with the import/wiring missing, the file fails 4/4; with the exit hook removed (kill patch only), the two lifecycle tests fail (2 failed / 3 passed) — that is the E2E-discovered graceful-close gap; with the full change, 5/5 pass. Existing suites still pass: ClaudeAdapterV2.test.ts 252/252, ClaudeAgentSdkQueryRunner.test.ts 1/1 (their fake-CLI spawn hooks keep precedence over the new default). tsc-rs --noEmit clean for the touched files; vp lint 0 errors (1 pre-existing unused-var warning on main).

End-to-end, the same harness as the reproduction with the adapter's spawner attached: after query.close(), cliGone=true and zero matching processes remain (before: the orphan pair above).

Could not check: Windows (group semantics are skipped there by design) and the Linux systemd-scope wrap (agentScope wraps only native-binary launches, and the wrapper execs into the CLI, so the detached PID is the CLI's; not exercised here).

This work was done by GLM (a model) in the ZCode agent harness.

Closing or deleting a session leaked the CLI's MCP plugin servers (each
plugin's bun server.ts): the SDK transport signals only the CLI PID, so the
plugin tree survived as orphans logging into a stdout pipe nobody drains,
pinning a core per wedged server until kill -9 (pingdotgg#3089).

The query runner now hands the SDK a spawner that runs the CLI detached in
its own POSIX process group, turns every kill into a group kill, and SIGKILLs
whatever is left in the group when the CLI exits for any reason, so the
transport's graceful close reaps the whole tree. Caller-supplied spawners
(remote runtimes, tests) keep ownership; Windows keeps the SDK's own spawn.

This work was done by GLM (a model) in the ZCode agent harness.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 13, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This focused fix changes the default Claude CLI process lifecycle on POSIX systems by introducing process-group ownership and cleanup, with real-process tests covering the behavior. Human review is required because the PR adds static-analysis diagnostic suppression directives in both production and test code.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: aa6b798e-4a12-4189-ad5d-2647c199d664
📥 Commits

Reviewing files that changed from the base of the PR and between 6524a8d and 6c540d2.

📒 Files selected for processing (2)
  • apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.process-group.test.ts
  • apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The Claude adapter adds a platform-specific process-group spawner and uses it by default in the query runner when no caller-supplied spawner is present. New tests cover process-group setup, CLI and descendant termination, and query-runner spawner wiring.

Changes: Claude CLI process lifecycle

Layer / File(s) Summary
Process-group spawner
apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts
The spawner starts the CLI in a separate process group on non-Windows platforms. It signals the CLI and group during termination, and sends SIGKILL to remaining group members when the CLI exits. Windows retains native process termination.
Query-runner wiring and lifecycle tests
apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.process-group.test.ts
The query runner supplies the spawner unless the caller provides one. Tests check process-group setup, descendant termination, child pipes, and query-runner wiring.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant QueryRunner
  participant ClaudeSDK
  participant ProcessGroupSpawner
  participant ClaudeCLI
  participant ProcessGroup
  QueryRunner->>ClaudeSDK: Supply default spawner
  ClaudeSDK->>ProcessGroupSpawner: Spawn CLI
  ProcessGroupSpawner->>ClaudeCLI: Start in separate process group
  ClaudeCLI->>ProcessGroup: Exit
  ProcessGroupSpawner->>ProcessGroup: Send SIGKILL to remaining members
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 6c540

The change is mergeable after normal checks. No actionable issue remains in the reviewed CLI lifecycle path.

Security architecture risk: 🔵 Low · up to 6c540

The change improves cleanup of session-owned processes and preserves explicitly supplied launchers. No introduced security vulnerability was established, but platform-specific cancellation and cleanup behavior remains partly unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new termination authority targets the CLI's POSIX process group and is bounded by the server process's OS signaling permissions. Process-group membership is a cleanup boundary, not a security sandbox or a guarantee against descendants deliberately changing groups.

Trust Boundaries and Controls

  • observed — The new callback does not introduce shell interpretation of its command or argument array. Existing native-command wrapping and explicit caller-owned spawn hooks remain in the launch path.

Resilience and Maintainability Implications

  • inferred — Exit-triggered group SIGKILL improves containment of ordinary inherited-group descendants even when they ignore graceful termination. Cleanup remains best-effort: signaling errors are ignored, and escaped groups are outside this mechanism.

Hardening Proposals

  • proposed — Make group cleanup explicitly terminal and idempotent so retained kill callbacks cannot continue signaling a captured process-group identifier after cleanup completes. This would reduce identifier-reuse risk; no cross-group termination was established in this review.
Pre-merge checks · Passed 4
Passed Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main fix: preventing MCP plugin servers from surviving with the Claude CLI process group.
Description check Passed The description is complete and directly addresses the problem, implementation, scope, verification results, limitations, and agent attribution. It includes detailed reproduction and test evidence. Th…
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 unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@t3dotgg

t3dotgg commented Oct 14, 2026

Copy link
Copy Markdown
Member

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Closing because this PR states that GLM wrote code in it:

This work was done by GLM (a model) in the ZCode agent harness.

We do not accept code written by GLM, Kimi, MiniMax, Mistral, Gemini, or alpha models, or by Muse models other than Muse Wonder Full. See Models we do not accept.

To have this considered again, open a new PR with the code written by a different model and link this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants