fix: kill process tree on StdioClientTransport.close()

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<string, string> {
     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
                 }

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();
+});