Skip to content

Commit d112da0

Browse files
yangshuai0711yangshuai0711
committed
fix: terminate local stdio mcp processes on teardown
1 parent 903d818 commit d112da0

2 files changed

Lines changed: 221 additions & 16 deletions

File tree

‎packages/opencode/src/mcp/index.ts‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,7 @@ export const layer = Layer.effect(
474474

475475
const listed = yield* defs(key, mcpClient, mcp.timeout)
476476
if (!listed) {
477-
yield* Effect.tryPromise(() => mcpClient.close()).pipe(Effect.ignore)
477+
yield* stopClient(mcpClient)
478478
return { status: { status: "failed", error: "Failed to get tools" } } satisfies CreateResult
479479
}
480480

@@ -507,6 +507,20 @@ export const layer = Layer.effect(
507507
Effect.catch(() => Effect.succeed([] as number[])),
508508
)
509509

510+
const stopClient = Effect.fnUntraced(function* (client: MCPClient) {
511+
const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : undefined
512+
513+
if (typeof pid === "number") {
514+
for (const target of [...(yield* descendants(pid)), pid]) {
515+
try {
516+
process.kill(target, "SIGTERM")
517+
} catch {}
518+
}
519+
}
520+
521+
yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore)
522+
})
523+
510524
function watch(s: State, name: string, client: MCPClient, bridge: EffectBridge.Shape, timeout?: number) {
511525
client.setNotificationHandler(ToolListChangedNotificationSchema, async () => {
512526
log.info("tools list changed notification received", { server: name })
@@ -563,19 +577,7 @@ export const layer = Layer.effect(
563577
Effect.gen(function* () {
564578
yield* Effect.forEach(
565579
Object.values(s.clients),
566-
(client) =>
567-
Effect.gen(function* () {
568-
const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null
569-
if (typeof pid === "number") {
570-
const pids = yield* descendants(pid)
571-
for (const dpid of pids) {
572-
try {
573-
process.kill(dpid, "SIGTERM")
574-
} catch {}
575-
}
576-
}
577-
yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore)
578-
}),
580+
(client) => stopClient(client),
579581
{ concurrency: "unbounded" },
580582
)
581583
pendingOAuthTransports.clear()
@@ -590,7 +592,7 @@ export const layer = Layer.effect(
590592
const client = s.clients[name]
591593
delete s.defs[name]
592594
if (!client) return Effect.void
593-
return Effect.tryPromise(() => client.close()).pipe(Effect.ignore)
595+
return stopClient(client)
594596
}
595597

596598
const storeClient = Effect.fnUntraced(function* (

‎packages/opencode/test/mcp/lifecycle.test.ts‎

Lines changed: 204 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { test, expect, mock, beforeEach } from "bun:test"
1+
import { test, expect, mock, beforeEach, spyOn } from "bun:test"
2+
import fs from "fs/promises"
23
import { InstanceRuntime } from "../../src/project/instance-runtime"
34
import { Effect } from "effect"
45
import type { MCP as MCPNS } from "../../src/mcp/index"
@@ -323,6 +324,95 @@ test(
323324
),
324325
)
325326

327+
test(
328+
"disconnect terminates the local stdio root process",
329+
withInstance({}, (mcp) =>
330+
Effect.gen(function* () {
331+
lastCreatedClientName = "disconnect-root-server"
332+
const serverState = getOrCreateClientState("disconnect-root-server")
333+
const killed: Array<{ pid: number; signal: NodeJS.Signals | number | undefined }> = []
334+
const kill = spyOn(process, "kill").mockImplementation((pid: number, signal?: NodeJS.Signals | number) => {
335+
killed.push({ pid, signal })
336+
return true
337+
})
338+
339+
try {
340+
yield* mcp.add("disconnect-root-server", {
341+
type: "local",
342+
command: ["echo", "test"],
343+
})
344+
345+
yield* mcp.disconnect("disconnect-root-server")
346+
347+
expect(killed).toContainEqual({ pid: 12345, signal: "SIGTERM" })
348+
expect(serverState.closed).toBe(true)
349+
} finally {
350+
kill.mockRestore()
351+
}
352+
}),
353+
),
354+
)
355+
356+
test("disconnect terminates local stdio descendant processes discovered by pgrep", async () => {
357+
await using tmp = await tmpdir({
358+
init: async (dir) => {
359+
await Bun.write(
360+
`${dir}/opencode.json`,
361+
JSON.stringify({
362+
$schema: "https://opencode.ai/config.json",
363+
mcp: {},
364+
}),
365+
)
366+
await Bun.write(
367+
`${dir}/pgrep`,
368+
`#!/bin/sh
369+
if [ "$2" = "12345" ]; then
370+
printf '54321\n'
371+
fi
372+
`,
373+
)
374+
await fs.chmod(`${dir}/pgrep`, 0o755)
375+
},
376+
})
377+
378+
const originalPath = process.env.PATH ?? ""
379+
process.env.PATH = `${tmp.path}:${originalPath}`
380+
const killed: Array<{ pid: number; signal: NodeJS.Signals | number | undefined }> = []
381+
const kill = spyOn(process, "kill").mockImplementation((pid: number, signal?: NodeJS.Signals | number) => {
382+
killed.push({ pid, signal })
383+
return true
384+
})
385+
386+
try {
387+
await WithInstance.provide({
388+
directory: tmp.path,
389+
fn: async () => {
390+
lastCreatedClientName = "disconnect-descendant-server"
391+
await Effect.runPromise(
392+
MCP.Service.use((mcp) =>
393+
Effect.gen(function* () {
394+
yield* mcp.add("disconnect-descendant-server", {
395+
type: "local",
396+
command: ["echo", "test"],
397+
})
398+
399+
yield* mcp.disconnect("disconnect-descendant-server")
400+
401+
expect(killed).toContainEqual({ pid: 54321, signal: "SIGTERM" })
402+
expect(killed).toContainEqual({ pid: 12345, signal: "SIGTERM" })
403+
}),
404+
).pipe(Effect.provide(MCP.defaultLayer)),
405+
)
406+
407+
await InstanceRuntime.disposeInstance(Instance.current)
408+
},
409+
})
410+
} finally {
411+
process.env.PATH = originalPath
412+
kill.mockRestore()
413+
}
414+
})
415+
326416
test(
327417
"connect() after disconnect() re-establishes the server",
328418
withInstance(
@@ -394,6 +484,40 @@ test(
394484
),
395485
)
396486

487+
test(
488+
"add() replacing a server terminates the old local stdio root process",
489+
withInstance({}, (mcp) =>
490+
Effect.gen(function* () {
491+
lastCreatedClientName = "replace-root-server"
492+
getOrCreateClientState("replace-root-server")
493+
const killed: Array<{ pid: number; signal: NodeJS.Signals | number | undefined }> = []
494+
const kill = spyOn(process, "kill").mockImplementation((pid: number, signal?: NodeJS.Signals | number) => {
495+
killed.push({ pid, signal })
496+
return true
497+
})
498+
499+
try {
500+
yield* mcp.add("replace-root-server", {
501+
type: "local",
502+
command: ["echo", "test"],
503+
})
504+
505+
clientStates.delete("replace-root-server")
506+
getOrCreateClientState("replace-root-server")
507+
508+
yield* mcp.add("replace-root-server", {
509+
type: "local",
510+
command: ["echo", "test"],
511+
})
512+
513+
expect(killed).toContainEqual({ pid: 12345, signal: "SIGTERM" })
514+
} finally {
515+
kill.mockRestore()
516+
}
517+
}),
518+
),
519+
)
520+
397521
// ========================================================================
398522
// Test: state init with mixed success/failure
399523
// ========================================================================
@@ -446,6 +570,85 @@ test(
446570
),
447571
)
448572

573+
test(
574+
"create rollback after defs failure terminates the local stdio root process",
575+
withInstance({}, (mcp) =>
576+
Effect.gen(function* () {
577+
lastCreatedClientName = "rollback-root-server"
578+
const serverState = getOrCreateClientState("rollback-root-server")
579+
serverState.listToolsShouldFail = true
580+
const killed: Array<{ pid: number; signal: NodeJS.Signals | number | undefined }> = []
581+
const kill = spyOn(process, "kill").mockImplementation((pid: number, signal?: NodeJS.Signals | number) => {
582+
killed.push({ pid, signal })
583+
return true
584+
})
585+
586+
try {
587+
const addResult = yield* mcp.add("rollback-root-server", {
588+
type: "local",
589+
command: ["echo", "test"],
590+
})
591+
592+
const serverStatus = (addResult.status as any)["rollback-root-server"] ?? addResult.status
593+
expect(serverStatus.status).toBe("failed")
594+
expect(killed).toContainEqual({ pid: 12345, signal: "SIGTERM" })
595+
expect(serverState.closed).toBe(true)
596+
} finally {
597+
kill.mockRestore()
598+
}
599+
}),
600+
),
601+
)
602+
603+
test("instance finalizer terminates the local stdio root process", async () => {
604+
await using tmp = await tmpdir({
605+
init: async (dir) => {
606+
await Bun.write(
607+
`${dir}/opencode.json`,
608+
JSON.stringify({
609+
$schema: "https://opencode.ai/config.json",
610+
mcp: {},
611+
}),
612+
)
613+
},
614+
})
615+
616+
const killed: Array<{ pid: number; signal: NodeJS.Signals | number | undefined }> = []
617+
const kill = spyOn(process, "kill").mockImplementation((pid: number, signal?: NodeJS.Signals | number) => {
618+
killed.push({ pid, signal })
619+
return true
620+
})
621+
622+
try {
623+
await WithInstance.provide({
624+
directory: tmp.path,
625+
fn: async () => {
626+
lastCreatedClientName = "finalizer-root-server"
627+
const serverState = getOrCreateClientState("finalizer-root-server")
628+
629+
await Effect.runPromise(
630+
MCP.Service.use((mcp) =>
631+
Effect.gen(function* () {
632+
yield* mcp.add("finalizer-root-server", {
633+
type: "local",
634+
command: ["echo", "test"],
635+
})
636+
expect(serverState.closed).toBe(false)
637+
}),
638+
).pipe(Effect.provide(MCP.defaultLayer)),
639+
)
640+
641+
await InstanceRuntime.disposeInstance(Instance.current)
642+
643+
expect(killed).toContainEqual({ pid: 12345, signal: "SIGTERM" })
644+
expect(serverState.closed).toBe(true)
645+
},
646+
})
647+
} finally {
648+
kill.mockRestore()
649+
}
650+
})
651+
449652
test(
450653
"falls back when MCP output schema refs fail SDK tool discovery",
451654
withInstance({}, (mcp) =>

0 commit comments

Comments
 (0)