From a75e5ed20a5e59be33df846a68842e89455bb012 Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:10:31 +0200 Subject: [PATCH] fix(core): Release test-webhook isolate only after teardown completes (backport to 1.x) (#36633) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mike Repeć Co-authored-by: Claude Sonnet 5 Co-authored-by: Danny Martini --- .../webhooks/__tests__/test-webhooks.test.ts | 193 +++++++++++++++++- packages/cli/src/webhooks/test-webhooks.ts | 29 ++- .../src/execution-engine/workflow-execute.ts | 5 +- 3 files changed, 217 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts index d774d425b98..c44f9ad9e7c 100644 --- a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts +++ b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts @@ -1,3 +1,4 @@ +import type { Logger } from '@n8n/backend-common'; import type { WorkflowEntity } from '@n8n/db'; import { generateNanoId } from '@n8n/db'; import type * as express from 'express'; @@ -42,12 +43,15 @@ const webhook = mock({ userId, }); +const flushMicrotasks = async () => await new Promise(jest.requireActual('timers').setImmediate); + describe('TestWebhooks', () => { + const logger = mock(); const registrations = mock(); const webhookService = mock(); const testWebhooks = new TestWebhooks( - mock(), + logger, mock(), mock(), registrations, @@ -184,6 +188,95 @@ describe('TestWebhooks', () => { await expect(promise).rejects.toThrowError(NotFoundError); }); + + test('releases isolate only after deactivateWebhooks completes on successful execution', async () => { + const expression = mock(); + const workflowStartNode = mock>({ + type: 'n8n-nodes-base.noOp', + }); + const workflow = mock({ + id: workflowEntity.id, + expression, + getNode: jest.fn().mockReturnValue(workflowStartNode), + }); + + jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + jest.spyOn(testWebhooks, 'getActiveWebhook').mockResolvedValue(webhook); + registrations.get.mockResolvedValueOnce({ + version: 1, + workflowEntity, + webhook, + } as TestWebhookRegistration); + const deactivateSpy = jest + .spyOn(testWebhooks, 'deactivateWebhooks') + .mockResolvedValue(undefined); + + jest + .spyOn(WebhookHelpers, 'executeWebhook') + .mockImplementation(async (...args: unknown[]) => { + const onDone = args[10] as (error: Error | null, data: unknown) => void; + onDone(null, { noWebhookResponse: true }); + return 'execution-id'; + }); + + await testWebhooks.executeWebhook( + mock({ params: { path }, method: httpMethod }), + mock(), + ); + await flushMicrotasks(); + + expect(deactivateSpy).toHaveBeenCalledWith(workflow); + expect(expression.acquireIsolate).toHaveBeenCalledTimes(1); + expect(expression.releaseIsolate).toHaveBeenCalledTimes(1); + const [acquireOrder] = (expression.acquireIsolate as jest.Mock).mock.invocationCallOrder; + const [deactivateOrder] = deactivateSpy.mock.invocationCallOrder; + const [releaseOrder] = (expression.releaseIsolate as jest.Mock).mock.invocationCallOrder; + expect(acquireOrder).toBeLessThan(deactivateOrder); + expect(deactivateOrder).toBeLessThan(releaseOrder); + }); + + test('logs when isolate release fails after teardown', async () => { + const expression = mock(); + const workflowStartNode = mock>({ + type: 'n8n-nodes-base.noOp', + }); + const workflow = mock({ + id: workflowEntity.id, + expression, + getNode: jest.fn().mockReturnValue(workflowStartNode), + }); + + jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + jest.spyOn(testWebhooks, 'getActiveWebhook').mockResolvedValue(webhook); + registrations.get.mockResolvedValueOnce({ + version: 1, + workflowEntity, + webhook, + } as TestWebhookRegistration); + jest.spyOn(testWebhooks, 'deactivateWebhooks').mockResolvedValue(undefined); + + jest + .spyOn(WebhookHelpers, 'executeWebhook') + .mockImplementation(async (...args: unknown[]) => { + const onDone = args[10] as (error: Error | null, data: unknown) => void; + onDone(null, { noWebhookResponse: true }); + return 'execution-id'; + }); + + const error = new Error('release failed'); + expression.releaseIsolate.mockRejectedValueOnce(error); + + await testWebhooks.executeWebhook( + mock({ params: { path }, method: httpMethod }), + mock(), + ); + await flushMicrotasks(); + + expect(logger.error).toHaveBeenCalledWith( + 'Failed to release expression isolate for test webhook', + expect.objectContaining({ error, workflowId: workflowEntity.id }), + ); + }); }); describe('deactivateWebhooks()', () => { @@ -201,6 +294,104 @@ describe('TestWebhooks', () => { }); }); + describe('cancelWebhook()', () => { + test('acquires and releases isolate around deactivateWebhooks', async () => { + const expression = mock(); + const workflow = mock({ id: workflowEntity.id, expression }); + + jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + registrations.getAllKeys.mockResolvedValue(['key1']); + registrations.get.mockResolvedValue({ + version: 1, + workflowEntity, + webhook, + } as TestWebhookRegistration); + const deactivateSpy = jest + .spyOn(testWebhooks, 'deactivateWebhooks') + .mockResolvedValue(undefined); + + await testWebhooks.cancelWebhook(workflowEntity.id); + await flushMicrotasks(); + + expect(expression.acquireIsolate).toHaveBeenCalledTimes(1); + expect(deactivateSpy).toHaveBeenCalledWith(workflow); + expect(expression.releaseIsolate).toHaveBeenCalledTimes(1); + const [acquireOrder] = (expression.acquireIsolate as jest.Mock).mock.invocationCallOrder; + const [deactivateOrder] = deactivateSpy.mock.invocationCallOrder; + const [releaseOrder] = (expression.releaseIsolate as jest.Mock).mock.invocationCallOrder; + expect(acquireOrder).toBeLessThan(deactivateOrder); + expect(deactivateOrder).toBeLessThan(releaseOrder); + }); + + test('releases isolate and logs when deactivateWebhooks throws', async () => { + const expression = mock(); + const workflow = mock({ id: workflowEntity.id, expression }); + + jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + registrations.getAllKeys.mockResolvedValue(['key1']); + registrations.get.mockResolvedValue({ + version: 1, + workflowEntity, + webhook, + } as TestWebhookRegistration); + const error = new Error('boom'); + jest.spyOn(testWebhooks, 'deactivateWebhooks').mockRejectedValue(error); + + await testWebhooks.cancelWebhook(workflowEntity.id); + await flushMicrotasks(); + + expect(expression.acquireIsolate).toHaveBeenCalledTimes(1); + expect(expression.releaseIsolate).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + 'Failed to deactivate test webhooks on cancel', + expect.objectContaining({ error, workflowId: workflowEntity.id }), + ); + }); + }); + + describe('handleClearTestWebhooks()', () => { + test('acquires and releases isolate around deactivateWebhooks', async () => { + const expression = mock(); + const workflow = mock({ id: workflowEntity.id, expression }); + + jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + ((testWebhooks as any).push.hasPushRef as jest.Mock).mockReturnValue(true); + const deactivateSpy = jest + .spyOn(testWebhooks, 'deactivateWebhooks') + .mockResolvedValue(undefined); + + await testWebhooks.handleClearTestWebhooks({ + webhookKey: 'key1', + workflowEntity, + pushRef: 'push-ref', + }); + + expect(expression.acquireIsolate).toHaveBeenCalledTimes(1); + expect(deactivateSpy).toHaveBeenCalledWith(workflow); + expect(expression.releaseIsolate).toHaveBeenCalledTimes(1); + }); + + test('releases isolate when deactivateWebhooks throws', async () => { + const expression = mock(); + const workflow = mock({ id: workflowEntity.id, expression }); + + jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + ((testWebhooks as any).push.hasPushRef as jest.Mock).mockReturnValue(true); + jest.spyOn(testWebhooks, 'deactivateWebhooks').mockRejectedValue(new Error('boom')); + + await expect( + testWebhooks.handleClearTestWebhooks({ + webhookKey: 'key1', + workflowEntity, + pushRef: 'push-ref', + }), + ).rejects.toThrow('boom'); + + expect(expression.acquireIsolate).toHaveBeenCalledTimes(1); + expect(expression.releaseIsolate).toHaveBeenCalledTimes(1); + }); + }); + describe('getWebhookMethods()', () => { beforeEach(() => { registrations.toKey.mockImplementation( diff --git a/packages/cli/src/webhooks/test-webhooks.ts b/packages/cli/src/webhooks/test-webhooks.ts index 7245e8d1c22..cd6bf3b9a3f 100644 --- a/packages/cli/src/webhooks/test-webhooks.ts +++ b/packages/cli/src/webhooks/test-webhooks.ts @@ -4,7 +4,7 @@ import { OnPubSubEvent } from '@n8n/decorators'; import { Service } from '@n8n/di'; import type express from 'express'; import { InstanceSettings } from 'n8n-core'; -import { WebhookPathTakenError, Workflow } from 'n8n-workflow'; +import { createDeferredPromise, WebhookPathTakenError, Workflow } from 'n8n-workflow'; import type { IWebhookData, IWorkflowExecuteAdditionalData, @@ -131,8 +131,11 @@ export class TestWebhooks implements IWebhookManager { } await workflow.expression.acquireIsolate(); - try { - return await new Promise(async (resolve, reject) => { + // Release only after teardown below runs, not when resolve() settles the + // promise early — teardown still needs the isolate held. + const responded = createDeferredPromise(); + return await new Promise(async (resolve, reject) => { + try { try { const executionMode = 'manual'; const executionId = await WebhookHelpers.executeWebhook( @@ -149,6 +152,7 @@ export class TestWebhooks implements IWebhookManager { (error: Error | null, data: IWebhookResponseCallbackData) => { if (error !== null) reject(error); else resolve(data); + responded.resolve(); }, destinationNode, ); @@ -157,7 +161,6 @@ export class TestWebhooks implements IWebhookManager { // or a ping so do not resolve the promise and wait for the real webhook // request instead. if (executionId === undefined) { - await workflow.expression.releaseIsolate(); return; } @@ -175,6 +178,7 @@ export class TestWebhooks implements IWebhookManager { // Settle the Promise to prevent hanging the request. // No return to ensure test-webhook cleanup. reject(error as Error); + responded.resolve(); } /** @@ -197,10 +201,19 @@ export class TestWebhooks implements IWebhookManager { this.clearTimeout(key); await this.deactivateWebhooks(workflow); - }); - } finally { - await workflow.expression.releaseIsolate(); - } + await responded.promise; + } finally { + // Response (if any) was already sent, so a release failure here can only be logged. + try { + await workflow.expression.releaseIsolate(); + } catch (error) { + this.logger.error('Failed to release expression isolate for test webhook', { + error, + workflowId: workflow.id, + }); + } + } + }); } @OnPubSubEvent('clear-test-webhooks', { instanceType: 'main' }) diff --git a/packages/core/src/execution-engine/workflow-execute.ts b/packages/core/src/execution-engine/workflow-execute.ts index b7014c48f66..18814458fc6 100644 --- a/packages/core/src/execution-engine/workflow-execute.ts +++ b/packages/core/src/execution-engine/workflow-execute.ts @@ -1449,10 +1449,12 @@ export class WorkflowExecute { void hooks.runHook('workflowExecuteAfter', [fullRunData]); }); + let acquiredIsolate = false; + // eslint-disable-next-line complexity const returnPromise = (async () => { try { - await workflow.expression.acquireIsolate(); + acquiredIsolate = await workflow.expression.acquireIsolate(); // Establish the execution context await establishExecutionContext( @@ -2306,6 +2308,7 @@ export class WorkflowExecute { return fullRunData; }) .finally(async () => { + if (!acquiredIsolate) return; try { await workflow.expression.releaseIsolate(); } catch (error) {