From dc4819ecdacb5c592a3d7d606b6465141eae9977 Mon Sep 17 00:00:00 2001 From: TomasRup Date: Fri, 11 Apr 2025 13:58:45 +0300 Subject: [PATCH 1/6] Ensuring stdio transport closure despite server hanging. Fixing #271 --- src/client/stdio.ts | 8 ++++ src/integration-tests/process-cleanup.test.ts | 38 ++++++++++++++++++- src/integration-tests/server-that-hangs.js | 20 ++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 src/integration-tests/server-that-hangs.js diff --git a/src/client/stdio.ts b/src/client/stdio.ts index b83bf27c..a2ca13d3 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -170,6 +170,13 @@ export class StdioClientTransport implements Transport { return this._process?.stderr ?? null; } + /** + * The process id of the child process, if it has been started. + */ + get pid(): number | undefined { + return this._process?.pid; + } + private processReadBuffer() { while (true) { try { @@ -187,6 +194,7 @@ export class StdioClientTransport implements Transport { async close(): Promise { this._abortController.abort(); + process.kill(this._process!.pid!, 'SIGKILL'); this._process = undefined; this._readBuffer.clear(); } diff --git a/src/integration-tests/process-cleanup.test.ts b/src/integration-tests/process-cleanup.test.ts index 0dd7861a..68450ab5 100644 --- a/src/integration-tests/process-cleanup.test.ts +++ b/src/integration-tests/process-cleanup.test.ts @@ -1,10 +1,14 @@ +import { execSync } from "node:child_process"; import { Server } from "../server/index.js"; import { StdioServerTransport } from "../server/stdio.js"; +import { Client } from "../client/index.js"; +import { StdioClientTransport } from "../client/stdio.js"; +import { resolve } from "node:path"; describe("Process cleanup", () => { jest.setTimeout(5000); // 5 second timeout - it("should exit cleanly after closing transport", async () => { + it("server should exit cleanly after closing transport", async () => { const server = new Server( { name: "test-server", @@ -25,4 +29,34 @@ describe("Process cleanup", () => { // The test runner will fail if the process hangs expect(true).toBe(true); }); -}); \ No newline at end of file + + it("client should exit cleanly after closing transport", async () => { + const isProcessRunning = (pid: number) => { + try { + execSync(`ps -p ${pid}`, { stdio: 'ignore' }); + return true; + } catch (_) { + return false; + } + } + + const client = new Client({ + name: "test-client", + version: "1.0.0", + }); + + const transport = new StdioClientTransport({ + command: "node", + args: ["server-that-hangs.js"], + cwd: __dirname + }); + + await client.connect(transport); + const pid = transport.pid; + + await client.close(); + await new Promise(resolve => setTimeout(resolve, 1000)); + + expect(isProcessRunning(pid!)).toBe(false); + }); +}); diff --git a/src/integration-tests/server-that-hangs.js b/src/integration-tests/server-that-hangs.js new file mode 100644 index 00000000..eaeb6fd4 --- /dev/null +++ b/src/integration-tests/server-that-hangs.js @@ -0,0 +1,20 @@ +import { McpServer } from "../../dist/esm/server/mcp.js"; +import { StdioServerTransport } from "../../dist/esm/server/stdio.js"; +import { spawn } from "node:child_process"; + +const transport = new StdioServerTransport(); + +const server = new McpServer({ + name: "test-stdio-server", + version: "1.0.0" +}); + +await server.connect(transport); + +const doNotExitImmediately = async () => { + setTimeout(() => process.exit(0), 30 * 1000); +}; + +process.stdin.on('close', doNotExitImmediately); +process.on('SIGINT', doNotExitImmediately); +process.on('SIGTERM', doNotExitImmediately); From 0b8651750428901209114ffa03849beed3a1be35 Mon Sep 17 00:00:00 2001 From: TomasRup Date: Fri, 11 Apr 2025 14:05:53 +0300 Subject: [PATCH 2/6] linter issues --- src/integration-tests/process-cleanup.test.ts | 5 +++-- src/integration-tests/server-that-hangs.js | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/integration-tests/process-cleanup.test.ts b/src/integration-tests/process-cleanup.test.ts index 68450ab5..24aeff6e 100644 --- a/src/integration-tests/process-cleanup.test.ts +++ b/src/integration-tests/process-cleanup.test.ts @@ -3,7 +3,6 @@ import { Server } from "../server/index.js"; import { StdioServerTransport } from "../server/stdio.js"; import { Client } from "../client/index.js"; import { StdioClientTransport } from "../client/stdio.js"; -import { resolve } from "node:path"; describe("Process cleanup", () => { jest.setTimeout(5000); // 5 second timeout @@ -35,7 +34,9 @@ describe("Process cleanup", () => { try { execSync(`ps -p ${pid}`, { stdio: 'ignore' }); return true; - } catch (_) { + + /* eslint-disable @typescript-eslint/no-unused-vars */ + } catch (error) { return false; } } diff --git a/src/integration-tests/server-that-hangs.js b/src/integration-tests/server-that-hangs.js index eaeb6fd4..581d9d39 100644 --- a/src/integration-tests/server-that-hangs.js +++ b/src/integration-tests/server-that-hangs.js @@ -1,6 +1,7 @@ +import { setTimeout } from 'node:timers' +import process from 'node:process' import { McpServer } from "../../dist/esm/server/mcp.js"; import { StdioServerTransport } from "../../dist/esm/server/stdio.js"; -import { spawn } from "node:child_process"; const transport = new StdioServerTransport(); From 6077dd06191f4de41916256af9e97c2716fbbd68 Mon Sep 17 00:00:00 2001 From: TomasRup Date: Fri, 11 Apr 2025 16:09:58 +0300 Subject: [PATCH 3/6] adding SIGTERM kill first, only then SIGKILL --- src/client/stdio.ts | 27 ++++++++++++++++++- src/integration-tests/process-cleanup.test.ts | 4 +-- src/integration-tests/server-that-hangs.js | 2 +- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/client/stdio.ts b/src/client/stdio.ts index a2ca13d3..1834aee5 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -192,9 +192,34 @@ export class StdioClientTransport implements Transport { } } + private gracefullyExitProcess(): void { + const pid = this.pid; + + if (!pid) { + return; + } + + // Killing after 3 seconds to ensure the process has time to exit gracefully. + setTimeout(() => { + try { + process.kill(pid, "SIGKILL"); + + } catch (err) { + if (typeof err === "object" && err !== null && "code" in err && err.code === "ESRCH") { + // Ignoring ESRCH error, which means the process is already dead. + return; + } + + console.warn(`Failed to kill process ${pid}: ${err}`); + } + }, 3000); + + process.kill(pid, "SIGINT"); + } + async close(): Promise { this._abortController.abort(); - process.kill(this._process!.pid!, 'SIGKILL'); + this.gracefullyExitProcess(); this._process = undefined; this._readBuffer.clear(); } diff --git a/src/integration-tests/process-cleanup.test.ts b/src/integration-tests/process-cleanup.test.ts index 24aeff6e..f6fd4d59 100644 --- a/src/integration-tests/process-cleanup.test.ts +++ b/src/integration-tests/process-cleanup.test.ts @@ -5,7 +5,7 @@ import { Client } from "../client/index.js"; import { StdioClientTransport } from "../client/stdio.js"; describe("Process cleanup", () => { - jest.setTimeout(5000); // 5 second timeout + jest.setTimeout(10 * 1000); // 10 second timeout it("server should exit cleanly after closing transport", async () => { const server = new Server( @@ -56,7 +56,7 @@ describe("Process cleanup", () => { const pid = transport.pid; await client.close(); - await new Promise(resolve => setTimeout(resolve, 1000)); + await new Promise(resolve => setTimeout(resolve, 5000)); expect(isProcessRunning(pid!)).toBe(false); }); diff --git a/src/integration-tests/server-that-hangs.js b/src/integration-tests/server-that-hangs.js index 581d9d39..72ef77bf 100644 --- a/src/integration-tests/server-that-hangs.js +++ b/src/integration-tests/server-that-hangs.js @@ -6,7 +6,7 @@ import { StdioServerTransport } from "../../dist/esm/server/stdio.js"; const transport = new StdioServerTransport(); const server = new McpServer({ - name: "test-stdio-server", + name: "server-that-hangs", version: "1.0.0" }); From 4b471aac121e39d92c80a57db16def34224abd33 Mon Sep 17 00:00:00 2001 From: TomasRup Date: Fri, 11 Apr 2025 16:23:10 +0300 Subject: [PATCH 4/6] typed error --- src/client/stdio.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/stdio.ts b/src/client/stdio.ts index 1834aee5..728c415a 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -205,7 +205,7 @@ export class StdioClientTransport implements Transport { process.kill(pid, "SIGKILL"); } catch (err) { - if (typeof err === "object" && err !== null && "code" in err && err.code === "ESRCH") { + if ((err as NodeJS.ErrnoException)?.code === "ESRCH") { // Ignoring ESRCH error, which means the process is already dead. return; } From ff67af75064180387e5c2a62d7e5d9f43486082f Mon Sep 17 00:00:00 2001 From: TomasRup Date: Fri, 11 Apr 2025 16:44:50 +0300 Subject: [PATCH 5/6] refactoring for readability --- src/client/stdio.ts | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/client/stdio.ts b/src/client/stdio.ts index 728c415a..b10e9503 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -192,34 +192,32 @@ export class StdioClientTransport implements Transport { } } - private gracefullyExitProcess(): void { - const pid = this.pid; - - if (!pid) { - return; - } - - // Killing after 3 seconds to ensure the process has time to exit gracefully. - setTimeout(() => { - try { - process.kill(pid, "SIGKILL"); - - } catch (err) { - if ((err as NodeJS.ErrnoException)?.code === "ESRCH") { - // Ignoring ESRCH error, which means the process is already dead. - return; - } - - console.warn(`Failed to kill process ${pid}: ${err}`); + private async forceExitProcess(pid: number): Promise { + try { + process.kill(pid, "SIGKILL"); + + } catch (err) { + if ((err as NodeJS.ErrnoException)?.code === "ESRCH") { + // Ignoring ESRCH error, which means the process is already dead. + return; } - }, 3000); + console.warn(`Failed to kill process ${pid}: ${err}`); + } + } + + private async gracefullyExitProcess(pid: number): Promise { process.kill(pid, "SIGINT"); + + await new Promise(resolve => setTimeout(() => { + this.forceExitProcess(pid); + resolve(); + }, 3000)); } async close(): Promise { this._abortController.abort(); - this.gracefullyExitProcess(); + this.gracefullyExitProcess(this.pid!); this._process = undefined; this._readBuffer.clear(); } From 658952f9a8658d3d2eae2b318b623f021f883040 Mon Sep 17 00:00:00 2001 From: TomasRup Date: Fri, 11 Apr 2025 17:12:52 +0300 Subject: [PATCH 6/6] graceful closure through abort contorller --- src/client/stdio.ts | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/client/stdio.ts b/src/client/stdio.ts index b10e9503..f8756a1e 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -192,7 +192,7 @@ export class StdioClientTransport implements Transport { } } - private async forceExitProcess(pid: number): Promise { + private forceExitProcess(pid: number): void { try { process.kill(pid, "SIGKILL"); @@ -206,18 +206,10 @@ export class StdioClientTransport implements Transport { } } - private async gracefullyExitProcess(pid: number): Promise { - process.kill(pid, "SIGINT"); - - await new Promise(resolve => setTimeout(() => { - this.forceExitProcess(pid); - resolve(); - }, 3000)); - } - async close(): Promise { + const pid = this.pid; + this._abortController.signal.onabort = () => setTimeout(() => pid && this.forceExitProcess(pid), 3000); this._abortController.abort(); - this.gracefullyExitProcess(this.pid!); this._process = undefined; this._readBuffer.clear(); }