diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 05f12fa2ee45..07dc000c8b54 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -398,11 +398,7 @@ const layer = Layer.effect( defs: listed, instructions: mcpClient.getInstructions()?.trim(), } satisfies CreateResult - }).pipe( - Effect.catchCause((cause) => - Effect.tryPromise(() => mcpClient.close()).pipe(Effect.ignore, Effect.andThen(Effect.failCause(cause))), - ), - ) + }).pipe(Effect.catchCause((cause) => terminate(mcpClient).pipe(Effect.andThen(Effect.failCause(cause))))) }, Effect.map((result): CreateResult => result), Effect.catchCause((cause) => { @@ -439,6 +435,21 @@ const layer = Layer.effect( Effect.catch(() => Effect.succeed([] as number[])), ) + function terminate(client: MCPClient) { + return Effect.gen(function* () { + const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null + if (typeof pid === "number") { + const pids = yield* descendants(pid) + for (const dpid of pids) { + try { + process.kill(dpid, "SIGTERM") + } catch {} + } + } + yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + }) + } + function watch(s: State, name: string, client: MCPClient, bridge: EffectBridge.Shape, timeout?: number) { client.onclose = () => { if (s.clients[name] !== client) return @@ -534,23 +545,7 @@ const layer = Layer.effect( s.clients = {} s.defs = {} s.instructions = {} - yield* Effect.forEach( - clients, - (client) => - Effect.gen(function* () { - const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null - if (typeof pid === "number") { - const pids = yield* descendants(pid) - for (const dpid of pids) { - try { - process.kill(dpid, "SIGTERM") - } catch {} - } - } - yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) - }), - { concurrency: "unbounded" }, - ) + yield* Effect.forEach(clients, terminate, { concurrency: "unbounded" }) pendingOAuthTransports.clear() }), ) @@ -565,7 +560,7 @@ const layer = Layer.effect( delete s.defs[name] delete s.instructions[name] if (!client) return Effect.void - return Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + return terminate(client) } const storeClient = Effect.fnUntraced(function* ( @@ -584,7 +579,7 @@ const layer = Layer.effect( if (instructions) s.instructions[name] = instructions else delete s.instructions[name] watch(s, name, client, bridge, timeout) - if (previous) yield* Effect.tryPromise(() => previous.close()).pipe(Effect.ignore) + if (previous) yield* terminate(previous) return s.status[name] }) diff --git a/packages/opencode/test/fixture/mcp-lifecycle-stdio.ts b/packages/opencode/test/fixture/mcp-lifecycle-stdio.ts index b01ed921cfd4..7a2eb2e13106 100644 --- a/packages/opencode/test/fixture/mcp-lifecycle-stdio.ts +++ b/packages/opencode/test/fixture/mcp-lifecycle-stdio.ts @@ -2,10 +2,21 @@ import { Server } from "@modelcontextprotocol/sdk/server/index.js" import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js" +const pidFile = process.env.MCP_LIFECYCLE_PID_FILE +if (pidFile) await Bun.write(pidFile, String(process.pid)) + +const childPidFile = process.env.MCP_LIFECYCLE_CHILD_PID_FILE +if (childPidFile) { + const child = Bun.spawn([process.execPath, "-e", "await new Promise(() => {})"], { + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + }) + await Bun.write(childPidFile, String(child.pid)) +} + if (process.argv.includes("--hang")) { - const pidFile = process.env.MCP_LIFECYCLE_PID_FILE if (!pidFile) throw new Error("MCP_LIFECYCLE_PID_FILE is required") - await Bun.write(pidFile, String(process.pid)) await new Promise(() => {}) } diff --git a/packages/opencode/test/mcp/lifecycle.test.ts b/packages/opencode/test/mcp/lifecycle.test.ts index 80c8fd22f886..18a23761d9a3 100644 --- a/packages/opencode/test/mcp/lifecycle.test.ts +++ b/packages/opencode/test/mcp/lifecycle.test.ts @@ -528,6 +528,100 @@ it.instance("local stdio timeout terminates the real server process", () => }), ) +it.instance("disconnect and replacement terminate real local MCP process trees", () => + Effect.gen(function* () { + const test = yield* TestInstance + const files = { + previous: { + parent: path.join(test.directory, "previous-parent.pid"), + child: path.join(test.directory, "previous-child.pid"), + }, + current: { + parent: path.join(test.directory, "current-parent.pid"), + child: path.join(test.directory, "current-child.pid"), + }, + } + yield* Effect.addFinalizer(() => + Effect.promise(async () => { + for (const file of Object.values(files).flatMap((item) => Object.values(item))) { + if (!(await Bun.file(file).exists())) continue + try { + process.kill(Number(await Bun.file(file).text()), "SIGKILL") + } catch {} + } + }), + ) + const stopped = (label: string, pids: readonly [number, number]) => + Effect.forEach( + [ + ["parent", pids[0]], + ["child", pids[1]], + ] as const, + ([name, pid]) => + pollWithTimeout( + Effect.sync(() => { + try { + process.kill(pid, 0) + return undefined + } catch { + return true + } + }), + `${label} ${name} stdio fixture process was not terminated`, + "1 second", + ), + { discard: true }, + ) + + const mcp = yield* MCP.Service + yield* mcp.add("process-tree", { + type: "local", + command: [process.execPath, stdioFixture], + environment: { + MCP_LIFECYCLE_PID_FILE: files.previous.parent, + MCP_LIFECYCLE_CHILD_PID_FILE: files.previous.child, + }, + }) + const previous = yield* pollWithTimeout( + Effect.promise(async () => { + if (!(await Bun.file(files.previous.parent).exists()) || !(await Bun.file(files.previous.child).exists())) { + return undefined + } + return [ + Number(await Bun.file(files.previous.parent).text()), + Number(await Bun.file(files.previous.child).text()), + ] as const + }), + "previous stdio fixture did not publish its process tree", + ) + + yield* mcp.add("process-tree", { + type: "local", + command: [process.execPath, stdioFixture], + environment: { + MCP_LIFECYCLE_PID_FILE: files.current.parent, + MCP_LIFECYCLE_CHILD_PID_FILE: files.current.child, + }, + }) + const current = yield* pollWithTimeout( + Effect.promise(async () => { + if (!(await Bun.file(files.current.parent).exists()) || !(await Bun.file(files.current.child).exists())) { + return undefined + } + return [ + Number(await Bun.file(files.current.parent).text()), + Number(await Bun.file(files.current.child).text()), + ] as const + }), + "current stdio fixture did not publish its process tree", + ) + yield* stopped("previous", previous) + + yield* mcp.disconnect("process-tree") + yield* stopped("current", current) + }), +) + it.instance("remote timeout aborts both real HTTP transport attempts", () => Effect.gen(function* () { const server = yield* hangingLifecycleServer()