fix(mcp): reap MCP child processes on process exit

packages/opencode/src/mcp/index.ts

@@ -32,10 +32,53 @@ import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"
 import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner"
 import { McpSampling } from "./sampling"
 import { SessionID } from "@/session/schema"
+import { spawnSync } from "node:child_process"
 
 const log = Log.create({ service: "mcp" })
 const DEFAULT_TIMEOUT = 30_000
 
+// Global registry so MCP children can be reaped on process exit even when the
+// Effect teardown path doesn't run (crash, SIGTERM, abrupt quit).
+const mcpChildPids = new Set<number>()
+
+/** Synchronous recursive kill (pgrep -P) — usable from process.on("exit"). */
+export function reapMcpChildren() {
+  for (const pid of mcpChildPids) {
+    const stack = [pid]
+    const seen = new Set<number>()
+    while (stack.length > 0) {
+      const current = stack.pop()!
+      if (seen.has(current)) continue
+      seen.add(current)
+      try { process.kill(current, "SIGTERM") } catch {}
+      try {
+        const r = spawnSync("pgrep", ["-P", String(current)], { encoding: "utf-8" })
+        for (const tok of (r.stdout ?? "").split("\n")) {
+          const c = parseInt(tok, 10)
+          if (!isNaN(c)) stack.push(c)
+        }
+      } catch {}
+    }
+  }
+}
+
+let exitCleanupRegistered = false
+function registerExitCleanup() {
+  if (exitCleanupRegistered) return
+  exitCleanupRegistered = true
+  process.on("exit", reapMcpChildren)
+  // Deliberate tradeoff: reap synchronously before exiting so a child spawned on
+  // the crash path can never survive the parent, and exit(0) masks the signal's
+  // non-zero code for supervisors. SIGINT is intentionally NOT hooked so the TUI
+  // Ctrl+C path still uses graceful Effect teardown (which also runs the finalizer).
+  process.on("SIGTERM", () => { reapMcpChildren(); process.exit(0) })
+}
+
+/** Test hook: register a pid for reap (used by tests; production uses connectLocal). */
+export function registerMcpChildForTest(pid: number) {
+  mcpChildPids.add(pid)
+}
+
 export const Resource = z
   .object({
     name: z.string(),
@@ -596,6 +639,13 @@ export const layer = Layer.effect(
 
       const connectTimeout = mcp.timeout ?? DEFAULT_TIMEOUT
       return yield* connectTransport(transport, connectTimeout).pipe(
+        Effect.tap(
+          Effect.sync(() => {
+            const pid = transport.pid
+            if (typeof pid === "number") mcpChildPids.add(pid)
+            registerExitCleanup()
+          }),
+        ),
         Effect.map((client): { client: MCPClient | undefined; status: Status } => ({
           client,
           status: { status: "connected" },
@@ -727,6 +777,7 @@ export const layer = Layer.effect(
                         process.kill(dpid, "SIGTERM")
                       } catch {}
                     }
+                    mcpChildPids.delete(pid)
                   }
                   yield* McpSampling.cancelAll(client)
                   yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore)
@@ -745,11 +796,26 @@ export const layer = Layer.effect(
       const client = s.clients[name]
       delete s.defs[name]
       if (!client) return Effect.void
+      // Capture the pid before close: StdioClientTransport.close() clears its
+      // process handle, so transport.pid would be undefined afterwards.
+      const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null
       // Interrupt sampling still running for this client first: once the
       // transport is gone its response can never be delivered, so the fiber
       // would otherwise keep a model call alive with nowhere to send the result.
       return McpSampling.cancelAll(client).pipe(
-        Effect.andThen(Effect.tryPromise(() => client.close()).pipe(Effect.ignore)),
+        Effect.andThen(
+          Effect.tryPromise(() => client.close()).pipe(
+            // StdioClientTransport.close() terminates the child, so it's safe to
+            // drop the pid here — this prunes stale pids on disconnect/reconnect
+            // so the exit handler can never SIGTERM a recycled pid.
+            Effect.tap(
+              Effect.sync(() => {
+                if (typeof pid === "number") mcpChildPids.delete(pid)
+              }),
+            ),
+            Effect.ignore,
+          ),
+        ),
       )
     }
 

packages/opencode/test/mcp/reap.test.ts

@@ -0,0 +1,64 @@
+import { describe, expect, test } from "bun:test"
+import { spawnSync } from "node:child_process"
+import { reapMcpChildren, registerMcpChildForTest } from "../../src/mcp/index"
+import { Log } from "../../src/util"
+
+void Log.init({ print: false })
+
+function alive(pid: number): boolean {
+  try {
+    process.kill(pid, 0)
+    return true
+  } catch {
+    return false
+  }
+}
+
+function kill(pid: number) {
+  try {
+    process.kill(pid, "SIGKILL")
+  } catch {}
+}
+
+describe("reapMcpChildren", () => {
+  test.skipIf(process.platform === "win32")("kills a registered process", () => {
+    // `sh -c 'sleep 300 & echo $!'` spawns a child `sleep` whose PID is printed.
+    // Register the sleep PID via the test hook; reap must terminate it.
+    const out = spawnSync("sh", ["-c", "sleep 300 & echo $!"], { encoding: "utf-8" })
+    const pid = parseInt((out.stdout ?? "").trim(), 10)
+    expect(Number.isNaN(pid)).toBe(false)
+    expect(alive(pid)).toBe(true)
+    try {
+      registerMcpChildForTest(pid)
+      reapMcpChildren()
+      expect(alive(pid)).toBe(false)
+    } finally {
+      // Cleanup on assertion failure so the long-lived sleep never leaks.
+      kill(pid)
+    }
+  })
+
+  test.skipIf(process.platform === "win32")("kills a registered non-leaf process", () => {
+    // `sh -c 'sh -c "sleep 300" & echo $!'` spawns an inner shell running a
+    // foreground `sleep 300`; the printed PID is the inner shell. Register it so
+    // reap has to SIGTERM a process that still owns a child (exercises the tree
+    // path rather than a leaf). Only the registered pid is asserted dead — the
+    // grandchild may reparent before pgrep runs, so its liveness is not a
+    // reliable assertion here.
+    const out = spawnSync("sh", ["-c", 'sh -c "sleep 300" & echo $!'], { encoding: "utf-8" })
+    const pid = parseInt((out.stdout ?? "").trim(), 10)
+    expect(Number.isNaN(pid)).toBe(false)
+    expect(alive(pid)).toBe(true)
+    try {
+      registerMcpChildForTest(pid)
+      reapMcpChildren()
+      expect(alive(pid)).toBe(false)
+    } finally {
+      kill(pid)
+    }
+  })
+
+  test("does not throw on empty registry", () => {
+    expect(() => reapMcpChildren()).not.toThrow()
+  })
+})