diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index c2e95b148..bbf94887f 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/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() + +/** 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() + 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, + ), + ), ) } diff --git a/packages/opencode/test/mcp/reap.test.ts b/packages/opencode/test/mcp/reap.test.ts new file mode 100644 index 000000000..5d7d48e2f --- /dev/null +++ b/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() + }) +})