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