fix: Make sure errors are transferred correctly from js task runner (no-changelog) (#11214)

packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts

@@ -13,6 +13,7 @@ import {
 import type { Task } from '@/task-runner';
 
 import { newAllCodeTaskData, newTaskWithSettings, withPairedItem, wrapIntoJson } from './test-data';
+import { ExecutionError } from '../errors/execution-error';
 
 jest.mock('ws');
 
@@ -292,7 +293,7 @@ describe('JsTaskRunner', () => {
 				});
 
 				expect(outcome).toEqual({
-					result: [wrapIntoJson({ error: 'Error message' })],
+					result: [wrapIntoJson({ error: 'Error message [line 1]' })],
 					customData: undefined,
 				});
 			});
@@ -406,8 +407,8 @@ describe('JsTaskRunner', () => {
 
 				expect(outcome).toEqual({
 					result: [
-						withPairedItem(0, wrapIntoJson({ error: 'Error message' })),
-						withPairedItem(1, wrapIntoJson({ error: 'Error message' })),
+						withPairedItem(0, wrapIntoJson({ error: 'Error message [line 1]' })),
+						withPairedItem(1, wrapIntoJson({ error: 'Error message [line 1]' })),
 					],
 					customData: undefined,
 				});
@@ -706,4 +707,56 @@ describe('JsTaskRunner', () => {
 			);
 		});
 	});
+
+	describe('errors', () => {
+		test.each<[CodeExecutionMode]>([['runOnceForAllItems'], ['runOnceForEachItem']])(
+			'should throw an ExecutionError if the code is invalid in %s mode',
+			async (nodeMode) => {
+				await expect(
+					execTaskWithParams({
+						task: newTaskWithSettings({
+							code: 'unknown',
+							nodeMode,
+						}),
+						taskData: newAllCodeTaskData([wrapIntoJson({ a: 1 })]),
+					}),
+				).rejects.toThrow(ExecutionError);
+			},
+		);
+
+		it('sends serializes an error correctly', async () => {
+			const runner = createRunnerWithOpts({});
+			const taskId = '1';
+			const task = newTaskWithSettings({
+				code: 'unknown; return []',
+				nodeMode: 'runOnceForAllItems',
+				continueOnFail: false,
+				mode: 'manual',
+				workflowMode: 'manual',
+			});
+			runner.runningTasks.set(taskId, task);
+
+			const sendSpy = jest.spyOn(runner.ws, 'send').mockImplementation(() => {});
+			jest.spyOn(runner, 'sendOffers').mockImplementation(() => {});
+			jest
+				.spyOn(runner, 'requestData')
+				.mockResolvedValue(newAllCodeTaskData([wrapIntoJson({ a: 1 })]));
+
+			await runner.receivedSettings(taskId, task.settings);
+
+			expect(sendSpy).toHaveBeenCalledWith(
+				JSON.stringify({
+					type: 'runner:taskerror',
+					taskId,
+					error: {
+						message: 'unknown is not defined [line 1]',
+						description: 'ReferenceError',
+						lineNumber: 1,
+					},
+				}),
+			);
+
+			console.log('DONE');
+		}, 1000);
+	});
 });

packages/@n8n/task-runner/src/js-task-runner/errors/__tests__/execution-error.test.ts

@@ -0,0 +1,53 @@
+import { ExecutionError } from '../execution-error';
+
+describe('ExecutionError', () => {
+	const defaultStack = `TypeError: a.unknown is not a function
+    at VmCodeWrapper (evalmachine.<anonymous>:2:3)
+    at evalmachine.<anonymous>:7:2
+    at Script.runInContext (node:vm:148:12)
+    at Script.runInNewContext (node:vm:153:17)
+    at runInNewContext (node:vm:309:38)
+    at JsTaskRunner.runForAllItems (/n8n/packages/@n8n/task-runner/dist/js-task-runner/js-task-runner.js:90:65)
+    at JsTaskRunner.executeTask (/n8n/packages/@n8n/task-runner/dist/js-task-runner/js-task-runner.js:71:26)
+    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
+    at async JsTaskRunner.receivedSettings (/n8n/packages/@n8n/task-runner/dist/task-runner.js:190:26)`;
+
+	it('should parse error details from stack trace without itemIndex', () => {
+		const error = new Error('a.unknown is not a function');
+		error.stack = defaultStack;
+
+		const executionError = new ExecutionError(error);
+		expect(executionError.message).toBe('a.unknown is not a function [line 2]');
+		expect(executionError.lineNumber).toBe(2);
+		expect(executionError.description).toBe('TypeError');
+		expect(executionError.context).toBeUndefined();
+	});
+
+	it('should parse error details from stack trace with itemIndex', () => {
+		const error = new Error('a.unknown is not a function');
+		error.stack = defaultStack;
+
+		const executionError = new ExecutionError(error, 1);
+		expect(executionError.message).toBe('a.unknown is not a function [line 2, for item 1]');
+		expect(executionError.lineNumber).toBe(2);
+		expect(executionError.description).toBe('TypeError');
+		expect(executionError.context).toEqual({ itemIndex: 1 });
+	});
+
+	it('should serialize correctly', () => {
+		const error = new Error('a.unknown is not a function');
+		error.stack = defaultStack;
+
+		const executionError = new ExecutionError(error, 1);
+
+		expect(JSON.stringify(executionError)).toBe(
+			JSON.stringify({
+				message: 'a.unknown is not a function [line 2, for item 1]',
+				description: 'TypeError',
+				itemIndex: 1,
+				context: { itemIndex: 1 },
+				lineNumber: 2,
+			}),
+		);
+	});
+});

packages/@n8n/task-runner/src/js-task-runner/errors/error-like.ts

@@ -0,0 +1,12 @@
+export interface ErrorLike {
+	message: string;
+	stack?: string;
+}
+
+export function isErrorLike(value: unknown): value is ErrorLike {
+	if (typeof value !== 'object' || value === null) return false;
+
+	const errorLike = value as ErrorLike;
+
+	return typeof errorLike.message === 'string';
+}

packages/@n8n/task-runner/src/js-task-runner/errors/execution-error.ts

@@ -1,6 +1,9 @@
-import { ApplicationError } from 'n8n-workflow';
+import type { ErrorLike } from './error-like';
+import { SerializableError } from './serializable-error';
 
-export class ExecutionError extends ApplicationError {
+const VM_WRAPPER_FN_NAME = 'VmCodeWrapper';
+
+export class ExecutionError extends SerializableError {
 	description: string | null = null;
 
 	itemIndex: number | undefined = undefined;
@@ -11,7 +14,7 @@ export class ExecutionError extends ApplicationError {
 
 	lineNumber: number | undefined = undefined;
 
-	constructor(error: Error & { stack?: string }, itemIndex?: number) {
+	constructor(error: ErrorLike, itemIndex?: number) {
 		super(error.message);
 		this.itemIndex = itemIndex;
 
@@ -32,10 +35,11 @@ export class ExecutionError extends ApplicationError {
 
 		if (stackRows.length === 0) {
 			this.message = 'Unknown error';
+			return;
 		}
 
 		const messageRow = stackRows.find((line) => line.includes('Error:'));
-		const lineNumberRow = stackRows.find((line) => line.includes('Code:'));
+		const lineNumberRow = stackRows.find((line) => line.includes(`at ${VM_WRAPPER_FN_NAME} `));
 		const lineNumberDisplay = this.toLineNumberDisplay(lineNumberRow);
 
 		if (!messageRow) {
@@ -56,16 +60,22 @@ export class ExecutionError extends ApplicationError {
 	}
 
 	private toLineNumberDisplay(lineNumberRow?: string) {
-		const errorLineNumberMatch = lineNumberRow?.match(/Code:(?<lineNumber>\d+)/);
+		if (!lineNumberRow) return '';
 
+		// TODO: This doesn't work if there is a function definition in the code
+		// and the error is thrown from that function.
+
+		const regex = new RegExp(
+			`at ${VM_WRAPPER_FN_NAME} \\(evalmachine\\.<anonymous>:(?<lineNumber>\\d+):`,
+		);
+		const errorLineNumberMatch = lineNumberRow.match(regex);
 		if (!errorLineNumberMatch?.groups?.lineNumber) return null;
 
 		const lineNumber = errorLineNumberMatch.groups.lineNumber;
+		if (!lineNumber) return '';
 
 		this.lineNumber = Number(lineNumber);
 
-		if (!lineNumber) return '';
-
 		return this.itemIndex === undefined
 			? `[line ${lineNumber}]`
 			: `[line ${lineNumber}, for item ${this.itemIndex}]`;

packages/@n8n/task-runner/src/js-task-runner/errors/serializable-error.ts

@@ -0,0 +1,21 @@
+/**
+ * Error that has its message property serialized as well. Used to transport
+ * errors over the wire.
+ */
+export abstract class SerializableError extends Error {
+	constructor(message: string) {
+		super(message);
+
+		// So it is serialized as well
+		this.makeMessageEnumerable();
+	}
+
+	private makeMessageEnumerable() {
+		Object.defineProperty(this, 'message', {
+			value: this.message,
+			enumerable: true, // This makes the message property enumerable
+			writable: true,
+			configurable: true,
+		});
+	}
+}

packages/@n8n/task-runner/src/js-task-runner/errors/validation-error.ts

@@ -1,6 +1,6 @@
-import { ApplicationError } from 'n8n-workflow';
+import { SerializableError } from './serializable-error';
 
-export class ValidationError extends ApplicationError {
+export class ValidationError extends SerializableError {
 	description = '';
 
 	itemIndex: number | undefined = undefined;

packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts

@@ -25,6 +25,8 @@ import { runInNewContext, type Context } from 'node:vm';
 import type { TaskResultData } from '@/runner-types';
 import { type Task, TaskRunner } from '@/task-runner';
 
+import { isErrorLike } from './errors/error-like';
+import { ExecutionError } from './errors/execution-error';
 import type { RequireResolver } from './require-resolver';
 import { createRequireResolver } from './require-resolver';
 import { validateRunForAllItemsOutput, validateRunForEachItemOutput } from './result-validation';
@@ -186,7 +188,7 @@ export class JsTaskRunner extends TaskRunner {
 
 		try {
 			const result = (await runInNewContext(
-				`module.exports = async function() {${settings.code}\n}()`,
+				`module.exports = async function VmCodeWrapper() {${settings.code}\n}()`,
 				context,
 			)) as TaskResultData['result'];
 
@@ -195,12 +197,14 @@ export class JsTaskRunner extends TaskRunner {
 			}
 
 			return validateRunForAllItemsOutput(result);
-		} catch (error) {
+		} catch (e) {
+			// Errors thrown by the VM are not instances of Error, so map them to an ExecutionError
+			const error = this.toExecutionErrorIfNeeded(e);
+
 			if (settings.continueOnFail) {
-				return [{ json: { error: this.getErrorMessageFromVmError(error) } }];
+				return [{ json: { error: error.message } }];
 			}
 
-			(error as Record<string, unknown>).node = allData.node;
 			throw error;
 		}
 	}
@@ -233,7 +237,7 @@ export class JsTaskRunner extends TaskRunner {
 
 			try {
 				let result = (await runInNewContext(
-					`module.exports = async function() {${settings.code}\n}()`,
+					`module.exports = async function VmCodeWrapper() {${settings.code}\n}()`,
 					context,
 				)) as INodeExecutionData | undefined;
 
@@ -257,14 +261,16 @@ export class JsTaskRunner extends TaskRunner {
 								},
 					);
 				}
-			} catch (error) {
+			} catch (e) {
+				// Errors thrown by the VM are not instances of Error, so map them to an ExecutionError
+				const error = this.toExecutionErrorIfNeeded(e);
+
 				if (!settings.continueOnFail) {
-					(error as Record<string, unknown>).node = allData.node;
 					throw error;
 				}
 
 				returnData.push({
-					json: { error: this.getErrorMessageFromVmError(error) },
+					json: { error: error.message },
 					pairedItem: {
 						item: index,
 					},
@@ -304,11 +310,15 @@ export class JsTaskRunner extends TaskRunner {
 		).getDataProxy();
 	}
 
-	private getErrorMessageFromVmError(error: unknown): string {
-		if (typeof error === 'object' && !!error && 'message' in error) {
-			return error.message as string;
+	private toExecutionErrorIfNeeded(error: unknown): Error {
+		if (error instanceof Error) {
+			return error;
 		}
 
-		return JSON.stringify(error);
+		if (isErrorLike(error)) {
+			return new ExecutionError(error);
+		}
+
+		return new ExecutionError({ message: JSON.stringify(error) });
 	}
 }

packages/@n8n/task-runner/src/task-runner.ts

@@ -1,4 +1,4 @@
-import { ApplicationError, ensureError } from 'n8n-workflow';
+import { ApplicationError } from 'n8n-workflow';
 import { nanoid } from 'nanoid';
 import { URL } from 'node:url';
 import { type MessageEvent, WebSocket } from 'ws';
@@ -256,8 +256,7 @@ export abstract class TaskRunner {
 		try {
 			const data = await this.executeTask(task);
 			this.taskDone(taskId, data);
-		} catch (e) {
-			const error = ensureError(e);
+		} catch (error) {
 			this.taskErrored(taskId, error);
 		}
 	}