diff --git a/packages/client/src/client/stdio.ts b/packages/client/src/client/stdio.ts index 5dcb8ef9a6..656034c273 100644 --- a/packages/client/src/client/stdio.ts +++ b/packages/client/src/client/stdio.ts @@ -1,4 +1,5 @@ import type { ChildProcess, IOType } from 'node:child_process'; +import { execFile } from 'node:child_process'; import process from 'node:process'; import type { Stream } from 'node:stream'; import { PassThrough } from 'node:stream'; @@ -85,6 +86,39 @@ export function getDefaultEnvironment(): Record { return env; } +/** + * Kill a process and all its descendants. + * + * - Unix: kills the process group via negative PID (requires the child to have + * been spawned with `detached: true` so it leads its own process group). + * - Windows: uses `taskkill /T /F` to kill the process tree. + * + * Falls back to direct `ChildProcess.kill()` if the tree-wide signal fails + * (e.g. the process already exited). + */ +function killProcessTree(childProcess: ChildProcess, signal: NodeJS.Signals = 'SIGTERM'): void { + const pid = childProcess.pid; + if (pid === undefined) { + childProcess.kill(signal); + return; + } + + if (process.platform === 'win32') { + try { + execFile('taskkill', ['/T', '/F', '/PID', pid.toString()], { windowsHide: true }); + } catch { + childProcess.kill(signal); + } + } else { + try { + process.kill(-pid, signal); + } catch { + // Process group may already be gone; fall back to direct kill. + childProcess.kill(signal); + } + } +} + /** * Client transport for stdio: this will connect to a server by spawning a process and communicating with it over stdin/stdout. * @@ -126,6 +160,7 @@ export class StdioClientTransport implements Transport { }, stdio: ['pipe', 'pipe', this._serverParams.stderr ?? 'inherit'], shell: false, + detached: process.platform !== 'win32', windowsHide: process.platform === 'win32', cwd: this._serverParams.cwd }); @@ -223,7 +258,7 @@ export class StdioClientTransport implements Transport { if (processToClose.exitCode === null) { try { - processToClose.kill('SIGTERM'); + killProcessTree(processToClose, 'SIGTERM'); } catch { // ignore } @@ -233,7 +268,7 @@ export class StdioClientTransport implements Transport { if (processToClose.exitCode === null) { try { - processToClose.kill('SIGKILL'); + killProcessTree(processToClose, 'SIGKILL'); } catch { // ignore } diff --git a/packages/client/test/client/stdio.test.ts b/packages/client/test/client/stdio.test.ts index 28a7834bcb..237a439ef4 100644 --- a/packages/client/test/client/stdio.test.ts +++ b/packages/client/test/client/stdio.test.ts @@ -1,16 +1,42 @@ import type { JSONRPCMessage } from '@modelcontextprotocol/core'; +import { execSync } from 'node:child_process'; import type { StdioServerParameters } from '../../src/client/stdio.js'; import { StdioClientTransport } from '../../src/client/stdio.js'; -// Configure default server parameters based on OS -// Uses 'more' command for Windows and 'tee' command for Unix/Linux -const getDefaultServerParameters = (): StdioServerParameters => { +const isUnix = process.platform !== 'win32'; + +function getDefaultServerParameters(): StdioServerParameters { if (process.platform === 'win32') { return { command: 'more' }; } return { command: '/usr/bin/tee' }; -}; +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function getChildPids(parentPid: number): number[] { + return execSync(`pgrep -P ${parentPid} 2>/dev/null || true`, { encoding: 'utf-8' }).trim().split('\n').filter(Boolean).map(Number); +} + +function getAllDescendantPids(rootPid: number): number[] { + const result: number[] = []; + const queue = [rootPid]; + while (queue.length > 0) { + const pid = queue.shift()!; + const children = getChildPids(pid); + result.push(...children); + queue.push(...children); + } + return result; +} const serverParameters = getDefaultServerParameters(); @@ -77,3 +103,110 @@ test('should return child process pid', async () => { await client.close(); expect(client.pid).toBeNull(); }); + +test.skipIf(!isUnix)('should kill child process tree on close', async () => { + const client = new StdioClientTransport({ + command: '/bin/sh', + args: ['-c', 'sleep 300 & wait'] + }); + + await client.start(); + const parentPid = client.pid!; + expect(parentPid).not.toBeNull(); + + const grandchildPids = getChildPids(parentPid); + expect(grandchildPids.length).toBeGreaterThan(0); + + await client.close(); + + expect(isProcessAlive(parentPid)).toBe(false); + for (const gPid of grandchildPids) { + expect(isProcessAlive(gPid)).toBe(false); + } +}); + +test.skipIf(!isUnix)('should kill multiple grandchildren on close', async () => { + const client = new StdioClientTransport({ + command: '/bin/sh', + args: ['-c', 'sleep 301 & sleep 302 & sleep 303 & wait'] + }); + + await client.start(); + const parentPid = client.pid!; + + const grandchildPids = getChildPids(parentPid); + expect(grandchildPids.length).toBe(3); + + await client.close(); + + expect(isProcessAlive(parentPid)).toBe(false); + for (const gPid of grandchildPids) { + expect(isProcessAlive(gPid)).toBe(false); + } +}); + +test.skipIf(!isUnix)('should kill a 3-level deep process tree on close', async () => { + const client = new StdioClientTransport({ + command: '/bin/sh', + args: ['-c', '/bin/sh -c "sleep 304 & wait" & wait'] + }); + + await client.start(); + const rootPid = client.pid!; + + // Give the nested shell time to spawn its children + await new Promise(resolve => setTimeout(resolve, 500)); + + const allDescendants = getAllDescendantPids(rootPid); + expect(allDescendants.length).toBeGreaterThanOrEqual(2); + + await client.close(); + + expect(isProcessAlive(rootPid)).toBe(false); + for (const pid of allDescendants) { + expect(isProcessAlive(pid)).toBe(false); + } +}); + +test.skipIf(!isUnix)('should fire onclose callback when killing process tree', async () => { + const client = new StdioClientTransport({ + command: '/bin/sh', + args: ['-c', 'sleep 305 & wait'] + }); + + let didClose = false; + client.onclose = () => { + didClose = true; + }; + + await client.start(); + await client.close(); + + expect(didClose).toBe(true); +}); + +test('should not throw when closing an already-exited process', async () => { + const client = new StdioClientTransport( + isUnix ? { command: '/bin/sh', args: ['-c', 'exit 0'] } : { command: 'cmd.exe', args: ['/c', 'exit 0'] } + ); + + await client.start(); + + // Wait for the process to exit on its own + await new Promise(resolve => setTimeout(resolve, 500)); + + await expect(client.close()).resolves.toBeUndefined(); +}); + +test('should not throw when close is called twice', async () => { + const client = new StdioClientTransport(serverParameters); + + await client.start(); + await client.close(); + await expect(client.close()).resolves.toBeUndefined(); +}); + +test('should not throw when close is called without start', async () => { + const client = new StdioClientTransport(serverParameters); + await expect(client.close()).resolves.toBeUndefined(); +});