From 05de34323ce2ec71fed2fbb963d617307efe6389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Repe=C4=87?= Date: Wed, 19 Aug 2026 14:16:25 +0000 Subject: [PATCH] fix(core): Release test-webhook isolate only after teardown completes (#36573) Co-authored-by: Claude Sonnet 5 Co-authored-by: Danny Martini --- .../webhooks/__tests__/test-webhooks.test.ts | 97 +++++++++++++++++-- packages/cli/src/webhooks/test-webhooks.ts | 23 +++-- 2 files changed, 107 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts index eda6b2289cb..1cebc5d7344 100644 --- a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts +++ b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts @@ -44,6 +44,12 @@ const webhook = mock({ userId, }); +const flushMicrotasks = async () => { + const { setImmediate: realSetImmediate } = + await vi.importActual('timers'); + await new Promise((resolve) => realSetImmediate(resolve)); +}; + describe('TestWebhooks', () => { const logger = mock(); const registrations = mock(); @@ -426,6 +432,91 @@ describe('TestWebhooks', () => { await expect(promise).rejects.toThrowError(WebhookNotFoundError); }); + + 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: vi.fn().mockReturnValue(workflowStartNode), + }); + + vi.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + vi.spyOn(testWebhooks, 'getActiveWebhook').mockResolvedValue(webhook); + registrations.get.mockResolvedValueOnce({ + version: 1, + workflowEntity, + webhook, + } as TestWebhookRegistration); + const deactivateSpy = vi + .spyOn(testWebhooks, 'deactivateWebhooks') + .mockResolvedValue(undefined); + + vi.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 Mock).mock.invocationCallOrder; + const [deactivateOrder] = deactivateSpy.mock.invocationCallOrder; + const [releaseOrder] = (expression.releaseIsolate as 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: vi.fn().mockReturnValue(workflowStartNode), + }); + + vi.spyOn(testWebhooks, 'toWorkflow').mockReturnValue(workflow); + vi.spyOn(testWebhooks, 'getActiveWebhook').mockResolvedValue(webhook); + registrations.get.mockResolvedValueOnce({ + version: 1, + workflowEntity, + webhook, + } as TestWebhookRegistration); + vi.spyOn(testWebhooks, 'deactivateWebhooks').mockResolvedValue(undefined); + + vi.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()', () => { @@ -446,12 +537,6 @@ describe('TestWebhooks', () => { }); describe('cancelWebhook()', () => { - const flushMicrotasks = async () => { - const { setImmediate: realSetImmediate } = - await vi.importActual('timers'); - await new Promise((resolve) => realSetImmediate(resolve)); - }; - test('acquires and releases isolate around deactivateWebhooks', async () => { const expression = mock(); const workflow = mock({ id: workflowEntity.id, expression }); diff --git a/packages/cli/src/webhooks/test-webhooks.ts b/packages/cli/src/webhooks/test-webhooks.ts index d17bb519c57..d55fe1d2284 100644 --- a/packages/cli/src/webhooks/test-webhooks.ts +++ b/packages/cli/src/webhooks/test-webhooks.ts @@ -146,8 +146,10 @@ 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. + return await new Promise(async (resolve, reject) => { + try { try { const executionMode = 'manual'; const executionId = await WebhookHelpers.executeWebhook( @@ -172,7 +174,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; } @@ -212,10 +213,18 @@ export class TestWebhooks implements IWebhookManager { this.clearTimeout(key); await this.deactivateWebhooks(workflow); - }); - } finally { - await workflow.expression.releaseIsolate(); - } + } 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' })