fix(core): Fix missing execution ID in webhook-based workflow producing binary data (#7244)

Story: https://linear.app/n8n/issue/PAY-839

This is a longstanding bug, fixed now so that the S3 backend for binary
data can use execution IDs as part of the filename.

To reproduce:

1. Set up a workflow with a POST Webhook node that accepts binary data.
2. Activate the workflow and call it sending a binary file, e.g. `curl
-X POST -F "file=@/path/to/binary/file/test.jpg"
http://localhost:5678/webhook/uuid`
3. Check `~/.n8n/binaryData`. The binary data and metadata files will be
missing the execution ID, e.g. `11869055-83c4-4493-876a-9092c4708b9b`
instead of `39011869055-83c4-4493-876a-9092c4708b9b`.

packages/cli/src/WorkflowExecuteAdditionalData.ts

@@ -65,6 +65,7 @@ import {
 	prepareExecutionDataForDbUpdate,
 	updateExistingExecution,
 } from './executionLifecycleHooks/shared/sharedHookFunctions';
+import { restoreBinaryDataId } from './executionLifecycleHooks/restoreBinaryDataId';
 
 const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType');
 
@@ -446,6 +447,10 @@ function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks {
 					workflowId: this.workflowData.id,
 				});
 
+				if (this.mode === 'webhook' && config.getEnv('binaryDataManager.mode') === 'filesystem') {
+					await restoreBinaryDataId(fullRunData, this.executionId);
+				}
+
 				const isManualMode = [this.mode, parentProcessMode].includes('manual');
 
 				try {

packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts

@@ -0,0 +1,43 @@
+import Container from 'typedi';
+import { BinaryDataService } from 'n8n-core';
+import type { IRun } from 'n8n-workflow';
+
+export function isMissingExecutionId(binaryDataId: string) {
+	const UUID_CHAR_LENGTH = 36;
+
+	return [UUID_CHAR_LENGTH + 'filesystem:'.length, UUID_CHAR_LENGTH + 's3:'.length].some(
+		(incorrectLength) => binaryDataId.length === incorrectLength,
+	);
+}
+
+/**
+ * Whenever the execution ID is not available to the binary data service at the
+ * time of writing a binary data file, its name is missing the execution ID.
+ *
+ * This function restores the ID in the file name and run data reference.
+ *
+ * ```txt
+ * filesystem:11869055-83c4-4493-876a-9092c4708b9b ->
+ * filesystem:39011869055-83c4-4493-876a-9092c4708b9b
+ * ```
+ */
+export async function restoreBinaryDataId(run: IRun, executionId: string) {
+	const { runData } = run.data.resultData;
+
+	const promises = Object.keys(runData).map(async (nodeName) => {
+		const binaryDataId = runData[nodeName]?.[0]?.data?.main?.[0]?.[0].binary?.data.id;
+
+		if (!binaryDataId || !isMissingExecutionId(binaryDataId)) return;
+
+		const [mode, incorrectFileId] = binaryDataId.split(':');
+		const correctFileId = `${executionId}${incorrectFileId}`;
+		const correctBinaryDataId = `${mode}:${correctFileId}`;
+
+		await Container.get(BinaryDataService).rename(incorrectFileId, correctFileId);
+
+		// @ts-expect-error Validated at the top
+		run.data.resultData.runData[nodeName][0].data.main[0][0].binary.data.id = correctBinaryDataId;
+	});
+
+	await Promise.all(promises);
+}

packages/cli/test/unit/execution.lifecycle.test.ts

@@ -0,0 +1,87 @@
+import { restoreBinaryDataId } from '@/executionLifecycleHooks/restoreBinaryDataId';
+import { BinaryDataService } from 'n8n-core';
+import { mockInstance } from '../integration/shared/utils/mocking';
+import type { IRun } from 'n8n-workflow';
+
+function toIRun(item: object) {
+	return {
+		data: {
+			resultData: {
+				runData: {
+					myNode: [
+						{
+							data: {
+								main: [[item]],
+							},
+						},
+					],
+				},
+			},
+		},
+	} as unknown as IRun;
+}
+
+function getDataId(run: IRun, kind: 'binary' | 'json') {
+	// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+	// @ts-ignore
+	return run.data.resultData.runData.myNode[0].data.main[0][0][kind].data.id;
+}
+
+describe('restoreBinaryDataId()', () => {
+	const binaryDataService = mockInstance(BinaryDataService);
+
+	beforeEach(() => {
+		jest.clearAllMocks();
+	});
+
+	it('should restore if binary data ID is missing execution ID', async () => {
+		const executionId = '999';
+		const incorrectFileId = 'a5c3f1ed-9d59-4155-bc68-9a370b3c51f6';
+		const run = toIRun({
+			binary: {
+				data: { id: `filesystem:${incorrectFileId}` },
+			},
+		});
+
+		await restoreBinaryDataId(run, executionId);
+
+		const correctFileId = `${executionId}${incorrectFileId}`;
+		const correctBinaryDataId = `filesystem:${correctFileId}`;
+
+		expect(binaryDataService.rename).toHaveBeenCalledWith(incorrectFileId, correctFileId);
+		expect(getDataId(run, 'binary')).toBe(correctBinaryDataId);
+	});
+
+	it('should do nothing if binary data ID is not missing execution ID', async () => {
+		const executionId = '999';
+		const fileId = `${executionId}a5c3f1ed-9d59-4155-bc68-9a370b3c51f6`;
+		const binaryDataId = `filesystem:${fileId}`;
+		const run = toIRun({
+			binary: {
+				data: {
+					id: binaryDataId,
+				},
+			},
+		});
+
+		await restoreBinaryDataId(run, executionId);
+
+		expect(binaryDataService.rename).not.toHaveBeenCalled();
+		expect(getDataId(run, 'binary')).toBe(binaryDataId);
+	});
+
+	it('should do nothing if no binary data ID', async () => {
+		const executionId = '999';
+		const dataId = '123';
+		const run = toIRun({
+			json: {
+				data: { id: dataId },
+			},
+		});
+
+		await restoreBinaryDataId(run, executionId);
+
+		expect(binaryDataService.rename).not.toHaveBeenCalled();
+		expect(getDataId(run, 'json')).toBe(dataId);
+	});
+});

packages/core/src/BinaryData/BinaryData.service.ts

@@ -154,6 +154,14 @@ export class BinaryDataService {
 		return inputData as INodeExecutionData[][];
 	}
 
+	async rename(oldFileId: string, newFileId: string) {
+		const manager = this.getManager(this.mode);
+
+		if (!manager) return;
+
+		await manager.rename(oldFileId, newFileId);
+	}
+
 	// ----------------------------------
 	//         private methods
 	// ----------------------------------

packages/core/src/BinaryData/FileSystem.manager.ts

@@ -3,6 +3,7 @@ import fs from 'fs/promises';
 import path from 'path';
 import { v4 as uuid } from 'uuid';
 import { jsonParse } from 'n8n-workflow';
+import { rename } from 'node:fs/promises';
 
 import { FileNotFoundError } from '../errors';
 import { ensureDirExists } from './utils';
@@ -124,6 +125,16 @@ export class FileSystemManager implements BinaryData.Manager {
 		return newFileId;
 	}
 
+	async rename(oldFileId: string, newFileId: string) {
+		const oldPath = this.getPath(oldFileId);
+		const newPath = this.getPath(newFileId);
+
+		await Promise.all([
+			rename(oldPath, newPath),
+			rename(`${oldPath}.metadata`, `${newPath}.metadata`),
+		]);
+	}
+
 	// ----------------------------------
 	//         private methods
 	// ----------------------------------

packages/core/src/BinaryData/types.ts

@@ -47,5 +47,7 @@ export namespace BinaryData {
 		// @TODO: Refactor to also receive `workflowId` to support full path-like identifier:
 		// `workflows/{workflowId}/executions/{executionId}/binary_data/{fileId}`
 		deleteManyByExecutionIds(executionIds: string[]): Promise<string[]>;
+
+		rename(oldFileId: string, newFileId: string): Promise<void>;
 	}
 }