From 69b2fbd1027691c7aa723d1a31db16a74ec5fb91 Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 09:36:56 +0000 Subject: [PATCH] fix(core): Drain webhook close functions to prevent MCP connection leaks (backport to 1.x) (#31188) Co-authored-by: Garrit Franke <32395585+garritfra@users.noreply.github.com> Co-authored-by: Garrit Franke Co-authored-by: Claude Opus 4.6 (1M context) --- .../mcp/McpClientTool/McpClientTool.node.ts | 36 ++++++++------ .../__test__/McpClientTool.node.test.ts | 48 +++++++++++++++++++ .../__tests__/webhook.service.test.ts | 32 +++++++++++++ packages/cli/src/webhooks/webhook.service.ts | 22 +++++++-- 4 files changed, 119 insertions(+), 19 deletions(-) diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts index 57e86d44a23..79951de3069 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts @@ -367,6 +367,7 @@ export class McpClientTool implements INodeType { this.logger.debug('McpClientTool: Successfully connected to MCP Server'); if (!mcpTools?.length) { + await client.close(); return setError( new NodeOperationError(node, 'MCP Server returned no tools', { itemIndex, @@ -376,25 +377,30 @@ export class McpClientTool implements INodeType { ); } - const tools = mcpTools.map((tool) => - logWrapper( - mcpToolToDynamicTool( - tool, - createCallTool(tool.name, client, config.timeout, (errorMessage) => { - const error = new NodeOperationError(node, errorMessage, { itemIndex }); - void this.addOutputData(NodeConnectionTypes.AiTool, itemIndex, error); - this.logger.error(`McpClientTool: Tool "${tool.name}" failed to execute`, { error }); - }), + try { + const tools = mcpTools.map((tool) => + logWrapper( + mcpToolToDynamicTool( + tool, + createCallTool(tool.name, client, config.timeout, (errorMessage) => { + const error = new NodeOperationError(node, errorMessage, { itemIndex }); + void this.addOutputData(NodeConnectionTypes.AiTool, itemIndex, error); + this.logger.error(`McpClientTool: Tool "${tool.name}" failed to execute`, { error }); + }), + ), + this, ), - this, - ), - ); + ); - this.logger.debug(`McpClientTool: Connected to MCP Server with ${tools.length} tools`); + this.logger.debug(`McpClientTool: Connected to MCP Server with ${tools.length} tools`); - const toolkit = new McpToolkit(tools); + const toolkit = new McpToolkit(tools); - return { response: toolkit, closeFunction: async () => await client.close() }; + return { response: toolkit, closeFunction: async () => await client.close() }; + } catch (e) { + await client.close(); + throw e; + } } async execute(this: IExecuteFunctions): Promise { diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/__test__/McpClientTool.node.test.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/__test__/McpClientTool.node.test.ts index 183893e0558..f99b46043e2 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/__test__/McpClientTool.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/__test__/McpClientTool.node.test.ts @@ -404,6 +404,54 @@ describe('McpClientTool', () => { ); }); + it('should close client when MCP server returns no tools', async () => { + jest.spyOn(Client.prototype, 'connect').mockResolvedValue(); + jest.spyOn(Client.prototype, 'listTools').mockResolvedValue({ tools: [] }); + const closeSpy = jest.spyOn(Client.prototype, 'close').mockResolvedValue(); + + await expect( + new McpClientTool().supplyData.call( + mock({ + getNode: jest.fn(() => mock({ typeVersion: 1, name: 'MCP Client' })), + logger: { debug: jest.fn(), error: jest.fn() }, + addInputData: jest.fn(() => ({ index: 0 })), + addOutputData: jest.fn(), + }), + 0, + ), + ).rejects.toThrow('MCP Server returned no tools'); + + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('should call client.close() when closeFunction is invoked', async () => { + jest.spyOn(Client.prototype, 'connect').mockResolvedValue(); + jest.spyOn(Client.prototype, 'listTools').mockResolvedValue({ + tools: [ + { + name: 'MyTool1', + description: 'MyTool1 does something', + inputSchema: { type: 'object', properties: { input: { type: 'string' } } }, + }, + ], + }); + const closeSpy = jest.spyOn(Client.prototype, 'close').mockResolvedValue(); + + const supplyDataResult = await new McpClientTool().supplyData.call( + mock({ + getNode: jest.fn(() => mock({ typeVersion: 1, name: 'McpClientTool' })), + logger: { debug: jest.fn(), error: jest.fn() }, + addInputData: jest.fn(() => ({ index: 0 })), + }), + 0, + ); + + expect(supplyDataResult.closeFunction).toBeDefined(); + await supplyDataResult.closeFunction?.(); + + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + it('should support setting a timeout', async () => { jest.spyOn(Client.prototype, 'connect').mockResolvedValue(); const callToolSpy = jest diff --git a/packages/cli/src/webhooks/__tests__/webhook.service.test.ts b/packages/cli/src/webhooks/__tests__/webhook.service.test.ts index d06951fb920..b4b06bc6871 100644 --- a/packages/cli/src/webhooks/__tests__/webhook.service.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook.service.test.ts @@ -374,6 +374,38 @@ describe('WebhookService', () => { expect(result).toEqual(responseData); expect(nodeType.webhook).toHaveBeenCalled(); }); + + test('should run close functions after webhook completes', async () => { + const closeFunction = jest.fn().mockResolvedValue(undefined); + const nodeType = mock({ + webhook: jest.fn().mockImplementation(async function (this: any) { + this.closeFunctions.push(closeFunction); + return responseData; + }), + }); + nodeTypes.getByNameAndVersion.mockReturnValue(nodeType); + + await webhookService.runWebhook(workflow, webhookData, node, additionalData, 'trigger', null); + + expect(closeFunction).toHaveBeenCalledTimes(1); + }); + + test('should run close functions even when webhook throws', async () => { + const closeFunction = jest.fn().mockResolvedValue(undefined); + const nodeType = mock({ + webhook: jest.fn().mockImplementation(async function (this: any) { + this.closeFunctions.push(closeFunction); + throw new Error('webhook failed'); + }), + }); + nodeTypes.getByNameAndVersion.mockReturnValue(nodeType); + + await expect( + webhookService.runWebhook(workflow, webhookData, node, additionalData, 'trigger', null), + ).rejects.toThrow('webhook failed'); + + expect(closeFunction).toHaveBeenCalledTimes(1); + }); }); describe('findCached()', () => { diff --git a/packages/cli/src/webhooks/webhook.service.ts b/packages/cli/src/webhooks/webhook.service.ts index 59a8f7f5d34..f6cef0eea4f 100644 --- a/packages/cli/src/webhooks/webhook.service.ts +++ b/packages/cli/src/webhooks/webhook.service.ts @@ -371,18 +371,32 @@ export class WebhookService { }); } + const closeFunctions: Array<() => Promise> = []; const context = new WebhookContext( workflow, node, additionalData, mode, webhookData, - [], + closeFunctions, runExecutionData ?? null, ); - return nodeType instanceof Node - ? await nodeType.webhook(context) - : ((await nodeType.webhook.call(context)) as IWebhookResponseData); + try { + return nodeType instanceof Node + ? await nodeType.webhook(context) + : ((await nodeType.webhook.call(context)) as IWebhookResponseData); + } finally { + const settledResults = await Promise.allSettled(closeFunctions.map(async (fn) => await fn())); + for (const result of settledResults) { + if (result.status === 'rejected') { + this.logger.error('Failed to run webhook close function', { + error: ensureError(result.reason), + nodeName: node.name, + nodeType: node.type, + }); + } + } + } } }