Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 19 additions & 24 deletions packages/opencode/src/mcp/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
}),
)
Expand All @@ -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* (
Expand All @@ -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]
})

Expand Down
15 changes: 13 additions & 2 deletions packages/opencode/test/fixture/mcp-lifecycle-stdio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {})
}

Expand Down
94 changes: 94 additions & 0 deletions packages/opencode/test/mcp/lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading