refactor(core): Extract immediate webhook response handling (no-changelog) (#39455)

Co-authored-by: Danny Martini <[email protected]>
Co-authored-by: Claude Fable 5.1 <[email protected]>

packages/cli/src/webhooks/__tests__/webhook-helpers.test.ts

@@ -82,6 +82,7 @@ import {
 	executeWebhook,
 	_privateGetWebhookErrorMessage,
 	invokeWebhook,
+	handleImmediateWebhookResponse,
 } from '../webhook-helpers';
 import { WebhookService } from '../webhook.service';
 import type { IWebhookResponseCallbackData, WebhookRequest } from '../webhook.types';
@@ -1328,6 +1329,92 @@ describe('invokeWebhook', () => {
 	});
 });
 
+describe('handleImmediateWebhookResponse', () => {
+	it.each([
+		{ name: 'continues with workflow data', workflowData: [[{ json: {} }]], shouldContinue: true },
+		{ name: 'stops without workflow data', workflowData: undefined, shouldContinue: false },
+	])('reports a no-response result once and $name', ({ workflowData, shouldContinue }) => {
+		const responseCallback = vi.fn();
+
+		const result = handleImmediateWebhookResponse({
+			webhookResultData: { noWebhookResponse: true, workflowData },
+			didSendResponse: false,
+			responseCode: 200,
+			responseCallback,
+		});
+
+		expect(responseCallback).toHaveBeenCalledOnce();
+		expect(responseCallback).toHaveBeenCalledWith(null, { noWebhookResponse: true });
+		expect(result).toEqual({
+			didSendResponse: true,
+			shouldContinueWorkflowExecution: shouldContinue,
+		});
+	});
+
+	it.each([
+		{ name: 'the default response', webhookResultData: {}, shouldContinue: false },
+		{
+			name: 'a no-response result',
+			webhookResultData: { noWebhookResponse: true, workflowData: [[{ json: {} }]] },
+			shouldContinue: true,
+		},
+	])(
+		'does not call back for $name when a response was already sent',
+		({ webhookResultData, shouldContinue }) => {
+			const responseCallback = vi.fn();
+
+			const result = handleImmediateWebhookResponse({
+				webhookResultData,
+				didSendResponse: true,
+				responseCode: 200,
+				responseCallback,
+			});
+
+			expect(responseCallback).not.toHaveBeenCalled();
+			expect(result).toEqual({
+				didSendResponse: true,
+				shouldContinueWorkflowExecution: shouldContinue,
+			});
+		},
+	);
+
+	it.each([
+		{
+			name: 'a custom response',
+			webhookResultData: { webhookResponse: { ok: true } },
+			expectedData: { ok: true },
+		},
+		{
+			name: 'the default response',
+			webhookResultData: {},
+			expectedData: { message: 'Webhook call received' },
+		},
+		{
+			name: 'an explicit null response',
+			webhookResultData: { webhookResponse: null },
+			expectedData: null,
+		},
+	])('sends $name and stops execution', ({ webhookResultData, expectedData }) => {
+		const responseCallback = vi.fn();
+
+		const result = handleImmediateWebhookResponse({
+			webhookResultData,
+			didSendResponse: false,
+			responseCode: 201,
+			responseCallback,
+		});
+
+		expect(responseCallback).toHaveBeenCalledWith(null, {
+			data: expectedData,
+			responseCode: 201,
+		});
+		expect(result).toEqual({
+			didSendResponse: true,
+			shouldContinueWorkflowExecution: false,
+		});
+	});
+});
+
 describe('executeWebhook form content type', () => {
 	const runRequest = async (startNode: INode, rawBody = '{') => {
 		ownershipService.getWorkflowProjectCached.mockResolvedValue(
@@ -2162,7 +2249,10 @@ describe('executeWebhook response-mode callback contract', () => {
 	 * One case per self-responding mode, so the next mode that forgets is caught
 	 * here rather than in production memory.
 	 */
-	const startWebhook = async (responseMode: string) => {
+	const startWebhook = async (
+		responseMode: string,
+		webhookResult: IWebhookResponseData = { workflowData: [[{ json: {} }]] },
+	) => {
 		vi.spyOn(WorkflowExecuteAdditionalData, 'getBase').mockResolvedValue(
 			mock<IWorkflowExecuteAdditionalData>({
 				// A real string: the formPage branch builds a URL from it.
@@ -2172,7 +2262,7 @@ describe('executeWebhook response-mode callback contract', () => {
 		ownershipService.getWorkflowProjectCached.mockResolvedValue(
 			mock<Project>({ id: 'project-1', name: 'Project 1' }),
 		);
-		webhookService.runWebhook.mockResolvedValue({ workflowData: [[{ json: {} }]] });
+		webhookService.runWebhook.mockResolvedValue(webhookResult);
 		workflowRunner.run.mockResolvedValue(EXECUTION_ID);
 		activeExecutions.getPostExecutePromise.mockReturnValue(
 			createDeferredPromise<IRun | undefined>().promise,
@@ -2233,6 +2323,16 @@ describe('executeWebhook response-mode callback contract', () => {
 		expect(responseCallback).toHaveBeenCalledTimes(1);
 		expect(responseCallback).toHaveBeenCalledWith(null, { noWebhookResponse: true });
 	});
+
+	it('invokes the response callback exactly once when the node answered itself in onReceived mode', async () => {
+		const { responseCallback } = await startWebhook('onReceived', {
+			noWebhookResponse: true,
+			workflowData: [[{ json: {} }]],
+		});
+
+		expect(responseCallback).toHaveBeenCalledTimes(1);
+		expect(responseCallback).toHaveBeenCalledWith(null, { noWebhookResponse: true });
+	});
 });
 
 describe('executeWebhook on engine v2', () => {

packages/cli/src/webhooks/webhook-helpers.ts

@@ -128,6 +128,11 @@ interface WebhookExecutionDataChanges {
 	};
 }
 
+interface WebhookContinuationResult {
+	didSendResponse: boolean;
+	shouldContinueWorkflowExecution: boolean;
+}
+
 const deferCleanupUntilStreamEnds = (
 	stream: Readable,
 	res: express.Response,
@@ -630,6 +635,50 @@ export async function invokeWebhook({
 	}
 }
 
+/**
+ * Sends the immediate webhook response unless a response was already sent. When the
+ * node answered the request itself (`noWebhookResponse`), it only reports that to the
+ * callback. Reports whether the workflow must run.
+ */
+export function handleImmediateWebhookResponse({
+	webhookResultData,
+	didSendResponse,
+	responseCode,
+	responseCallback,
+}: {
+	webhookResultData: IWebhookResponseData;
+	didSendResponse: boolean;
+	responseCode: number;
+	responseCallback: (
+		error: Error | null,
+		data: IWebhookResponseCallbackData | WebhookResponse,
+	) => void;
+}): WebhookContinuationResult {
+	if (webhookResultData.noWebhookResponse === true && !didSendResponse) {
+		responseCallback(null, { noWebhookResponse: true });
+		didSendResponse = true;
+	}
+
+	if (webhookResultData.workflowData !== undefined) {
+		return { didSendResponse, shouldContinueWorkflowExecution: true };
+	}
+
+	if (!didSendResponse) {
+		// Only `undefined` selects the default message. A node can respond with `null`,
+		// which `??` would wrongly replace.
+		responseCallback(null, {
+			data:
+				webhookResultData.webhookResponse !== undefined
+					? (webhookResultData.webhookResponse as IDataObject | IDataObject[])
+					: { message: 'Webhook call received' },
+			responseCode,
+		});
+		didSendResponse = true;
+	}
+
+	return { didSendResponse, shouldContinueWorkflowExecution: false };
+}
+
 /**
  * Reconciles a pre-seeded execution stack (identity/context trigger flows) with the
  * webhook node's real output. No-op unless the start node seeded execution data.
@@ -1045,43 +1094,17 @@ export async function executeWebhook(
 			responseHeaders.applyToResponse(res);
 		}
 
-		if (webhookResultData.noWebhookResponse === true && !didSendResponse) {
-			// The response got already send
-			responseCallback(null, {
-				noWebhookResponse: true,
-			});
-			didSendResponse = true;
-		}
+		const immediateResponse = handleImmediateWebhookResponse({
+			webhookResultData,
+			didSendResponse,
+			responseCode,
+			responseCallback,
+		});
+		didSendResponse = immediateResponse.didSendResponse;
+		if (!immediateResponse.shouldContinueWorkflowExecution) return;
 
-		if (webhookResultData.workflowData === undefined) {
-			// Workflow should not run
-			if (webhookResultData.webhookResponse !== undefined) {
-				// Data to respond with is given
-				if (!didSendResponse) {
-					responseCallback(null, {
-						data: webhookResultData.webhookResponse as IDataObject | IDataObject[],
-						responseCode,
-					});
-					didSendResponse = true;
-				}
-			} else {
-				// Send default response
-
-				if (!didSendResponse) {
-					responseCallback(null, {
-						data: {
-							message: 'Webhook call received',
-						},
-						responseCode,
-					});
-					didSendResponse = true;
-				}
-			}
-			return;
-		}
-
-		// The node's output is the only place a file shows up, so this cannot run with
-		// the checks above.
+		// Engine v2 cannot receive files yet. A file exists only in the node's output,
+		// so this check runs after the node, unlike `engineV2Webhooks.assertSupported()`.
 		if (routesToEngineV2) engineV2Webhooks.assertPayloadSupported(webhookResultData);
 
 		// Reactive credential-status gate. Runs only once we know the workflow will