refactor(core): Consolidate binary file not found errors (no-changelog) (#7585)

Logging was originally to see if there was a binary data file failing to
be written for [this
user](https://linear.app/n8n/issue/PAY-844/filesystem-binary-data-mode-causing-alerts-in-cloud)
but the cause was not a file failed to be written but a missing `fileId`
in a binary data item in an execution. The error should no longer be
thrown as of 1.12. See story for more info.

This PR is for cleanup and to consolidate any file not found errors in
the context of binary data, to track if this happens again.

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

@@ -1,14 +1,13 @@
 import { readFile, stat } from 'node:fs/promises';
 import prettyBytes from 'pretty-bytes';
 import Container, { Service } from 'typedi';
-import { BINARY_ENCODING, LoggerProxy as Logger, IBinaryData } from 'n8n-workflow';
+import { BINARY_ENCODING } from 'n8n-workflow';
 import { UnknownManagerError, InvalidModeError } from './errors';
 import { areConfigModes, toBuffer } from './utils';
-import { LogCatch } from '../decorators/LogCatch.decorator';
 
 import type { Readable } from 'stream';
 import type { BinaryData } from './types';
-import type { INodeExecutionData } from 'n8n-workflow';
+import type { INodeExecutionData, IBinaryData } from 'n8n-workflow';
 
 @Service()
 export class BinaryDataService {
@@ -40,7 +39,6 @@ export class BinaryDataService {
 		}
 	}
 
-	@LogCatch((error) => Logger.error('Failed to copy binary data file', { error }))
 	async copyBinaryFile(
 		workflowId: string,
 		executionId: string,
@@ -76,7 +74,6 @@ export class BinaryDataService {
 		return binaryData;
 	}
 
-	@LogCatch((error) => Logger.error('Failed to write binary data file', { error }))
 	async store(
 		workflowId: string,
 		executionId: string,
@@ -152,9 +149,6 @@ export class BinaryDataService {
 		if (manager.deleteMany) await manager.deleteMany(ids);
 	}
 
-	@LogCatch((error) =>
-		Logger.error('Failed to copy all binary data files for execution', { error }),
-	)
 	async duplicateBinaryData(
 		workflowId: string,
 		executionId: string,

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

@@ -3,8 +3,8 @@ import fs from 'node:fs/promises';
 import path from 'node:path';
 import { v4 as uuid } from 'uuid';
 import { jsonParse } from 'n8n-workflow';
-import { assertDir } from './utils';
-import { FileNotFoundError } from '../errors';
+import { assertDir, doesNotExist } from './utils';
+import { BinaryFileNotFoundError, InvalidPathError } from '../errors';
 
 import type { Readable } from 'stream';
 import type { BinaryData } from './types';
@@ -46,17 +46,21 @@ export class FileSystemManager implements BinaryData.Manager {
 	async getAsStream(fileId: string, chunkSize?: number) {
 		const filePath = this.resolvePath(fileId);
 
+		if (await doesNotExist(filePath)) {
+			throw new BinaryFileNotFoundError(filePath);
+		}
+
 		return createReadStream(filePath, { highWaterMark: chunkSize });
 	}
 
 	async getAsBuffer(fileId: string) {
 		const filePath = this.resolvePath(fileId);
 
-		try {
-			return await fs.readFile(filePath);
-		} catch {
-			throw new Error(`Error finding file: ${filePath}`);
+		if (await doesNotExist(filePath)) {
+			throw new BinaryFileNotFoundError(filePath);
 		}
+
+		return fs.readFile(filePath);
 	}
 
 	async getMetadata(fileId: string): Promise<BinaryData.Metadata> {
@@ -167,7 +171,7 @@ export class FileSystemManager implements BinaryData.Manager {
 		const returnPath = path.join(this.storagePath, ...args);
 
 		if (path.relative(this.storagePath, returnPath).startsWith('..')) {
-			throw new FileNotFoundError('Invalid path detected');
+			throw new InvalidPathError(returnPath);
 		}
 
 		return returnPath;
@@ -186,7 +190,7 @@ export class FileSystemManager implements BinaryData.Manager {
 			const stats = await fs.stat(filePath);
 			return stats.size;
 		} catch (error) {
-			throw new Error('Failed to find binary data file in filesystem', { cause: error });
+			throw new BinaryFileNotFoundError(filePath);
 		}
 	}
 }

packages/core/src/BinaryData/utils.ts

@@ -23,6 +23,15 @@ export async function assertDir(dir: string) {
 	}
 }
 
+export async function doesNotExist(dir: string) {
+	try {
+		await fs.access(dir);
+		return false;
+	} catch {
+		return true;
+	}
+}
+
 export async function toBuffer(body: Buffer | Readable) {
 	return new Promise<Buffer>((resolve) => {
 		if (Buffer.isBuffer(body)) resolve(body);

packages/core/src/decorators/LogCatch.decorator.ts

@@ -1,29 +0,0 @@
-/* eslint-disable @typescript-eslint/no-unsafe-call */
-/* eslint-disable @typescript-eslint/no-unsafe-member-access */
-/* eslint-disable @typescript-eslint/no-unsafe-assignment */
-
-export const LogCatch = (logFn: (error: unknown) => void) => {
-	return (target: unknown, propertyKey: string, descriptor: PropertyDescriptor) => {
-		const originalMethod = descriptor.value;
-
-		descriptor.value = function (...args: unknown[]) {
-			try {
-				const result: unknown = originalMethod.apply(this, args);
-
-				if (result && result instanceof Promise) {
-					return result.catch((error: unknown) => {
-						logFn(error);
-						throw error;
-					});
-				}
-
-				return result;
-			} catch (error) {
-				logFn(error);
-				throw error;
-			}
-		};
-
-		return descriptor;
-	};
-};

packages/core/src/errors.ts

@@ -3,3 +3,11 @@ export class FileNotFoundError extends Error {
 		super(`File not found: ${filePath}`);
 	}
 }
+
+export class BinaryFileNotFoundError extends FileNotFoundError {}
+
+export class InvalidPathError extends Error {
+	constructor(readonly filePath: string) {
+		super(`Invalid path detected: ${filePath}`);
+	}
+}

packages/core/test/FileSystem.manager.test.ts

@@ -53,6 +53,7 @@ describe('getPath()', () => {
 describe('getAsBuffer()', () => {
 	it('should return a buffer', async () => {
 		fsp.readFile = jest.fn().mockResolvedValue(mockBuffer);
+		fsp.access = jest.fn().mockImplementation(async () => {});
 
 		const result = await fsManager.getAsBuffer(fileId);
 
@@ -64,6 +65,7 @@ describe('getAsBuffer()', () => {
 describe('getAsStream()', () => {
 	it('should return a stream', async () => {
 		fs.createReadStream = jest.fn().mockReturnValue(mockStream);
+		fsp.access = jest.fn().mockImplementation(async () => {});
 
 		const stream = await fsManager.getAsStream(fileId);
 
@@ -123,6 +125,7 @@ describe('copyByFilePath()', () => {
 		const targetPath = toFullFilePath(otherFileId);
 
 		fsp.cp = jest.fn().mockResolvedValue(undefined);
+		fsp.writeFile = jest.fn().mockResolvedValue(undefined);
 
 		const result = await fsManager.copyByFilePath(
 			workflowId,
@@ -132,6 +135,11 @@ describe('copyByFilePath()', () => {
 		);
 
 		expect(fsp.cp).toHaveBeenCalledWith(sourceFilePath, targetPath);
+		expect(fsp.writeFile).toHaveBeenCalledWith(
+			`${toFullFilePath(otherFileId)}.metadata`,
+			JSON.stringify({ ...metadata, fileSize: mockBuffer.length }),
+			{ encoding: 'utf-8' },
+		);
 		expect(result.fileSize).toBe(mockBuffer.length);
 	});
 });