fix(core): Release test-webhook isolate only after teardown completes (backport to 1.x) (#36633)

Co-authored-by: Mike Repeć <[email protected]>
Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Danny Martini <[email protected]>

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<IWebhookData>({
 	userId,
 });
 
+const flushMicrotasks = async () => await new Promise(jest.requireActual('timers').setImmediate);
+
 describe('TestWebhooks', () => {
+	const logger = mock<Logger>();
 	const registrations = mock<TestWebhookRegistrationsService>();
 	const webhookService = mock<WebhookService>();
 
 	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<Expression>();
+			const workflowStartNode = mock<ReturnType<Workflow['getNode']>>({
+				type: 'n8n-nodes-base.noOp',
+			});
+			const workflow = mock<Workflow>({
+				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<WebhookRequest>({ params: { path }, method: httpMethod }),
+				mock<express.Response>(),
+			);
+			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<Expression>();
+			const workflowStartNode = mock<ReturnType<Workflow['getNode']>>({
+				type: 'n8n-nodes-base.noOp',
+			});
+			const workflow = mock<Workflow>({
+				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<WebhookRequest>({ params: { path }, method: httpMethod }),
+				mock<express.Response>(),
+			);
+			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<Expression>();
+			const workflow = mock<Workflow>({ 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<Expression>();
+			const workflow = mock<Workflow>({ 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<Expression>();
+			const workflow = mock<Workflow>({ 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<Expression>();
+			const workflow = mock<Workflow>({ 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(

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' })

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) {