From 2baca965ac50684a10a750f60d8a976fc29b17e4 Mon Sep 17 00:00:00 2001 From: Hamza Shah Date: Mon, 3 Aug 2026 13:12:00 +0500 Subject: [PATCH] fix(server): install createMcpHandler's onclose hook once per instance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createMcpHandler wrapped server.onclose on every request to keep its in-flight set current. A factory that returns the same McpServer for every request therefore stacked one wrapper per request: the chain retained every closure, and running it recursed one frame per layer and threw RangeError: Maximum call stack size exceeded. Because the throw happened in the async close path it landed after handler.close() had already resolved, so callers could not catch it. Guard the wrap with a WeakSet so it is installed at most once per instance. The intended per-request-instance path is unchanged; a reused instance now costs O(1) per request instead of crashing the process. Reusing an instance still isn't a supported pattern — the era write, handler installation and identity seeding all re-run against the shared server — but it should degrade, not take the process down. --- .changeset/mcp-handler-onclose-hook-once.md | 5 ++ .../server/src/server/createMcpHandler.ts | 25 +++++++--- .../test/server/createMcpHandler.test.ts | 49 +++++++++++++++++++ 3 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 .changeset/mcp-handler-onclose-hook-once.md diff --git a/.changeset/mcp-handler-onclose-hook-once.md b/.changeset/mcp-handler-onclose-hook-once.md new file mode 100644 index 0000000000..70fac7e3bb --- /dev/null +++ b/.changeset/mcp-handler-onclose-hook-once.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/server': patch +--- + +Install `createMcpHandler`'s in-flight `onclose` hook at most once per server instance. The factory contract is one fresh instance per request, but a factory that returns the same instance every time (`createMcpHandler(() => sharedServer)`) previously stacked one `onclose` wrapper per request. The chain retained every closure for the life of the process and, once it ran, recursed one frame per accumulated layer — dying with `RangeError: Maximum call stack size exceeded` after roughly 20k requests, as an uncaught async error that surfaced _after_ `handler.close()` had already resolved. Instance reuse still is not the intended pattern, but it now costs O(1) instead of crashing the process. diff --git a/packages/server/src/server/createMcpHandler.ts b/packages/server/src/server/createMcpHandler.ts index a484869944..a15c58a0fe 100644 --- a/packages/server/src/server/createMcpHandler.ts +++ b/packages/server/src/server/createMcpHandler.ts @@ -616,6 +616,11 @@ export function createMcpHandler(factory: McpServerFactory, options: CreateMcpHa /** Modern per-request instances with an exchange still in flight (close() tears these down). */ const inflight = new Set(); + /** + * Instances whose `onclose` already carries the in-flight bookkeeping, so a + * factory returning the same instance twice does not wrap it twice. + */ + const inflightHookInstalled = new WeakSet(); let closed = false; const reportError = (error: Error) => { @@ -778,13 +783,21 @@ export function createMcpHandler(factory: McpServerFactory, options: CreateMcpHa }); } - // Track the instance until its exchange tears down so close() can abort it. - const previousOnClose = server.onclose; + // Track the instance until its exchange tears down so close() can abort + // it. The `onclose` hook is installed at most once per instance: a + // factory that returns the same instance for every request (not the + // intended contract, but an easy mistake to make) would otherwise stack + // one wrapper per request, retaining every closure and eventually + // overflowing the stack when the chain finally runs. inflight.add(server); - server.onclose = () => { - inflight.delete(server); - previousOnClose?.(); - }; + if (!inflightHookInstalled.has(server)) { + inflightHookInstalled.add(server); + const previousOnClose = server.onclose; + server.onclose = () => { + inflight.delete(server); + previousOnClose?.(); + }; + } try { const response = await invoke(product, route.message, { diff --git a/packages/server/test/server/createMcpHandler.test.ts b/packages/server/test/server/createMcpHandler.test.ts index ded506e57c..6921d70be6 100644 --- a/packages/server/test/server/createMcpHandler.test.ts +++ b/packages/server/test/server/createMcpHandler.test.ts @@ -793,6 +793,55 @@ describe('createMcpHandler — handler faces', () => { // pseudo-headers, write backpressure) are pinned at unit level there. }); +describe('createMcpHandler — in-flight bookkeeping', () => { + /** + * The factory contract is one fresh instance per request, but a factory that + * returns the same instance every time must still cost O(1): the in-flight + * `onclose` hook is installed once per instance, not stacked per request. + * Stacking it grew one closure layer per request and eventually died with + * `RangeError: Maximum call stack size exceeded` when the chain ran. + */ + function sharedInstanceFactory(): McpServer { + const mcpServer = new McpServer({ name: 'shared-instance', version: '1.0.0' }); + mcpServer.registerTool('echo', { inputSchema: z.object({ text: z.string() }) }, async ({ text }) => ({ + content: [{ type: 'text', text }] + })); + return mcpServer; + } + + it('installs the in-flight onclose hook at most once per instance', async () => { + const shared = sharedInstanceFactory(); + const handler = createMcpHandler(() => shared); + + await handler.fetch(postRequest(modernToolsCall('echo', { text: 'first' }))); + const hookAfterFirst = shared.server.onclose; + expect(hookAfterFirst).toBeTypeOf('function'); + + for (let i = 0; i < 20; i++) { + const response = await handler.fetch(postRequest(modernToolsCall('echo', { text: `reuse-${i}` }))); + expect(response.status).toBe(200); + } + + // Same function object, so the chain is one layer deep rather than 21. + expect(shared.server.onclose).toBe(hookAfterFirst); + }); + + it("preserves the instance's own onclose behind the hook", async () => { + const shared = sharedInstanceFactory(); + let ownOnCloseCalls = 0; + shared.server.onclose = () => { + ownOnCloseCalls += 1; + }; + const handler = createMcpHandler(() => shared); + + const response = await handler.fetch(postRequest(modernToolsCall('echo', { text: 'wrapped' }))); + expect(response.status).toBe(200); + await shared.close(); + + expect(ownOnCloseCalls).toBe(1); + }); +}); + describe('createMcpHandler — close()', () => { it('aborts in-flight modern exchanges and refuses further requests', async () => { const { factory } = testFactory();