From 5df7a173a67a6fc42d16e2e0e91d2dd43e0a261d Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" <283985454+n8n-cat-bot[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 08:56:17 +0000 Subject: [PATCH] fix: Stop replacing error messages with "Unknown error" (#36820) Co-authored-by: n8n-cat-bot[bot] Co-authored-by: Claude Opus 5 --- .../__tests__/js-task-runner.test.ts | 22 +++ .../errors/__tests__/execution-error.test.ts | 178 ++++++++++++++++++ .../js-task-runner/errors/execution-error.ts | 86 +++++---- 3 files changed, 250 insertions(+), 36 deletions(-) diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts index 79c1b9edbb0..6c31a61c19d 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts @@ -1343,6 +1343,28 @@ describe('JsTaskRunner', () => { }, ); + // A database driver error, e.g. `pg`'s `DatabaseError`, sets `name` to + // `'error'`, so V8 renders the stack header lowercased as `error: `. + // The message the database gave must survive to the user. + test.each<[CodeExecutionMode]>([['runOnceForAllItems'], ['runOnceForEachItem']])( + 'should surface the message of a rethrown database error in %s mode', + async (nodeMode) => { + const code = ` + const dbError = new Error('duplicate key value violates unique constraint "users_email_key"'); + dbError.name = 'error'; + dbError.code = '23505'; + throw dbError; + `; + + await expect( + execTaskWithParams({ + task: newTaskParamsWithSettings({ code, nodeMode }), + taskData: newDataRequestResponse([wrapIntoJson({ a: 1 })]), + }), + ).rejects.toThrow('duplicate key value violates unique constraint "users_email_key"'); + }, + ); + it('sends serializes an error correctly', async () => { const runner = createRunnerWithOpts({}); const taskId = '1'; diff --git a/packages/@n8n/task-runner/src/js-task-runner/errors/__tests__/execution-error.test.ts b/packages/@n8n/task-runner/src/js-task-runner/errors/__tests__/execution-error.test.ts index 53270c25993..21030799cf2 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/errors/__tests__/execution-error.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/errors/__tests__/execution-error.test.ts @@ -34,6 +34,184 @@ describe('ExecutionError', () => { expect(executionError.context).toEqual({ itemIndex: 1 }); }); + it('should parse a stack with CRLF line endings', () => { + const error = new Error('a.unknown is not a function'); + Object.defineProperty(error, 'stack', { value: defaultStack.replace(/\n/g, '\r\n') }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe('a.unknown is not a function [line 2]'); + expect(executionError.description).toBe('TypeError'); + }); + + // A database driver error, e.g. `pg`'s `DatabaseError`, sets `name` to + // `'error'`, so V8 renders the stack header lowercased as `error: `. + describe('errors whose stack header is not "Error:"', () => { + it('should keep the message of a driver error thrown outside the sandbox', () => { + const driverStack = `error: duplicate key value violates unique constraint "users_email_key" + at Parser.parseErrorMessage (/n8n/node_modules/pg-protocol/dist/parser.js:283:98) + at Parser.handlePacket (/n8n/node_modules/pg-protocol/dist/parser.js:122:29) + at Parser.parse (/n8n/node_modules/pg-protocol/dist/parser.js:35:38) + at Socket. (/n8n/node_modules/pg-protocol/dist/index.js:11:42) + at Socket.emit (node:events:518:28) + at addChunk (node:internal/streams/readable:561:12) + at TCP.onStreamRead (node:internal/stream_base_commons:191:23)`; + + const error = new Error('duplicate key value violates unique constraint "users_email_key"'); + error.name = 'error'; + Object.defineProperty(error, 'stack', { value: driverStack }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe( + 'duplicate key value violates unique constraint "users_email_key"', + ); + }); + + it('should keep the message of a driver error rethrown from user code', () => { + const driverStack = `error: null value in column "email" of relation "users" violates not-null constraint + at VmCodeWrapper (evalmachine.:4:9) + at evalmachine.:7:2 + at Script.runInContext (node:vm:149:12) + at runInContext (node:vm:301:6) + 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)`; + + const error = new Error( + 'null value in column "email" of relation "users" violates not-null constraint', + ); + error.name = 'error'; + Object.defineProperty(error, 'stack', { value: driverStack }); + + const executionError = new ExecutionError(error, 0); + + expect(executionError.message).toBe( + 'null value in column "email" of relation "users" violates not-null constraint [line 4, for item 0]', + ); + expect(executionError.lineNumber).toBe(4); + }); + + it.each([ + ['a name containing a space', 'Database Error'], + ['a name containing a dot', 'my.CustomError'], + ['a name containing a dash', 'Custom-Error'], + ['a name carrying a node error code', 'TypeError [ERR_INVALID_ARG_TYPE]'], + ])('should keep the message and surface the type for %s', (_, errorType) => { + const error = new Error('oops'); + error.name = errorType; + Object.defineProperty(error, 'stack', { + value: `${errorType}: oops\n at VmCodeWrapper (evalmachine.:2:9)`, + }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe('oops [line 2]'); + expect(executionError.description).toBe(errorType); + }); + }); + + describe('messages containing ": "', () => { + // Previously handled by two hard-coded special cases that stitched a message + // split on every colon back together. + it.each([ + "Cannot find module 'node:fs'", + "Module 'node:fs' is disallowed", + "Module 'node:child_process' is disallowed", + ])('should keep "%s" whole', (message) => { + const error = new Error(message); + Object.defineProperty(error, 'stack', { + value: `Error: ${message}\n at VmCodeWrapper (evalmachine.:2:9)`, + }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe(`${message} [line 2]`); + expect(executionError.description).toBeNull(); + }); + + // Deliberate change: the message used to be split on every colon, so this + // surfaced as message "Not Found" with description "Request failed with + // status code 404" - the leading half of the message became the type. + it('should not split a message on a colon it happens to contain', () => { + const message = 'Request failed with status code 404: Not Found'; + const error = new Error(message); + Object.defineProperty(error, 'stack', { + value: `Error: ${message}\n at VmCodeWrapper (evalmachine.:2:9)`, + }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe(`${message} [line 2]`); + expect(executionError.description).toBeNull(); + }); + + it('should not mistake a continuation row of a multi-line message for the header', () => { + const error = new Error('request failed\ncause: ETIMEDOUT'); + Object.defineProperty(error, 'stack', { + value: `Error: request failed +cause: ETIMEDOUT + at VmCodeWrapper (evalmachine.:2:9)`, + }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe('request failed [line 2]'); + expect(executionError.description).toBeNull(); + }); + }); + + // V8 prefixes a syntax error's stack with an excerpt of the offending source, + // closed by a caret row. The excerpt holds user code, so a row of it can look + // like a stack header. + it('should skip the source excerpt prefixing a syntax error stack', () => { + const syntaxErrorStack = `evalmachine.:2 +data: { foo: 1 + ^ + +SyntaxError: Unexpected end of input + at new Script (node:vm:117:7) + at createScript (node:vm:269:10) + at Object.runInNewContext (node:vm:310:10)`; + + const error = new SyntaxError('Unexpected end of input'); + Object.defineProperty(error, 'stack', { value: syntaxErrorStack }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe('Unexpected end of input'); + expect(executionError.description).toBe('SyntaxError'); + }); + + describe('stacks without a parseable header', () => { + it("should keep the error's own message", () => { + const error = new Error('connect ETIMEDOUT 10.0.0.1:5432'); + Object.defineProperty(error, 'stack', { + value: ` at VmCodeWrapper (evalmachine.:3:9) + at evalmachine.:7:2`, + }); + + const executionError = new ExecutionError(error); + + expect(executionError.message).toBe('connect ETIMEDOUT 10.0.0.1:5432 [line 3]'); + expect(executionError.description).toBeNull(); + expect(executionError.lineNumber).toBe(3); + }); + + it("should keep the error's own message when there is no stack at all", () => { + const executionError = new ExecutionError({ message: '{"code":"ECONNRESET"}' }); + + expect(executionError.message).toBe('{"code":"ECONNRESET"}'); + expect(executionError.description).toBeNull(); + }); + + it('should fall back to "Unknown error" when there is no message either', () => { + const executionError = new ExecutionError({ message: '' }); + + expect(executionError.message).toBe('Unknown error'); + expect(executionError.description).toBeNull(); + }); + }); + it('should serialize correctly', () => { const error = new Error('a.unknown is not a function'); Object.defineProperty(error, 'stack', { diff --git a/packages/@n8n/task-runner/src/js-task-runner/errors/execution-error.ts b/packages/@n8n/task-runner/src/js-task-runner/errors/execution-error.ts index ace4951960a..f9a4f337dc8 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/errors/execution-error.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/errors/execution-error.ts @@ -1,6 +1,25 @@ import type { ErrorLike } from './error-like'; import { SerializableError } from './serializable-error'; +/** + * Matches the header row of a stack, i.e. `: `. The + * name is whatever the thrown error's `name` is, which is not always + * `Error`-suffixed - `pg`'s `DatabaseError` sets it to `error`, and a custom + * error may contain spaces, dots or dashes - so accept any name up to the first + * `': '`, instead of looking for an `Error:` prefix. + */ +const STACK_HEADER_REGEX = /^(?\S.*?): (?.+)$/; + +/** Matches a stack frame row, e.g. ` at Object.foo (/a/b.js:1:2)`. */ +const STACK_FRAME_REGEX = /^\s+at\s/; + +/** + * Matches the caret row that closes the source excerpt V8 prefixes a syntax + * error's stack with. The excerpt holds user code, which can look like a header + * row, so the header is only searched for past the caret. + */ +const SOURCE_EXCERPT_CARET_REGEX = /^\s*\^+\s*$/; + export class ExecutionError extends SerializableError { description: string | null = null; @@ -29,34 +48,40 @@ export class ExecutionError extends SerializableError { } /** - * Populate error `message` and `description` from error `stack`. + * Populate error `message` and `description` from error `stack`. The stack is + * the richer source - it carries the error name and the line number - but the + * error's own `message` is always present, so keep it as the fallback for a + * stack we cannot parse, rather than discarding it. */ private populateFromStack() { - const stackRows = (this.stack ?? '').split('\n'); + const fallbackMessage = this.message || 'Unknown error'; + const stackRows = (this.stack ?? '').split(/\r?\n/); - if (stackRows.length === 0) { - this.message = 'Unknown error'; - return; - } - - const messageRow = stackRows.find((line) => line.includes('Error:')); const lineNumberDisplay = this.toLineNumberDisplay(stackRows); - - if (!messageRow) { - this.message = `Unknown error ${lineNumberDisplay}`; - return; - } - - const [errorDetails, errorType] = this.toErrorDetailsAndType(messageRow); + const [errorDetails, errorType] = this.toErrorDetailsAndType(this.toMessageRow(stackRows)); if (errorType) this.description = errorType; - if (!errorDetails) { - this.message = `Unknown error ${lineNumberDisplay}`; - return; - } + this.message = `${errorDetails ?? fallbackMessage} ${lineNumberDisplay}`.trim(); + } - this.message = `${errorDetails} ${lineNumberDisplay}`; + /** + * Find the row holding `: `, i.e. the first row + * past any source excerpt and before the first stack frame. Rows after it can + * be continuation rows of a multi-line message, which must not be mistaken + * for the header. + */ + private toMessageRow(stackRows: string[]) { + const firstFrameIndex = stackRows.findIndex((row) => STACK_FRAME_REGEX.test(row)); + const headerRows = firstFrameIndex === -1 ? stackRows : stackRows.slice(0, firstFrameIndex); + + const excerptEndIndex = headerRows.reduce( + (lastCaretIndex, row, index) => + SOURCE_EXCERPT_CARET_REGEX.test(row) ? index + 1 : lastCaretIndex, + 0, + ); + + return headerRows.slice(excerptEndIndex).find((row) => STACK_HEADER_REGEX.test(row)); } private toLineNumberDisplay(stackRows: string[]) { @@ -90,24 +115,13 @@ export class ExecutionError extends SerializableError { } private toErrorDetailsAndType(messageRow?: string) { - if (!messageRow) return [null, null]; + const groups = messageRow?.match(STACK_HEADER_REGEX)?.groups; - const segments = messageRow.split(':').map((i) => i.trim()); - if (segments[1] === "Cannot find module 'node") { - segments[1] = `${segments[1]}:${segments[2]}`; - segments.splice(2, 1); - } + if (!groups) return [null, null]; - if ( - segments.length >= 3 && - segments[1]?.startsWith("Module 'node") && - segments[2]?.includes("' is disallowed") - ) { - segments[1] = `${segments[1]}:${segments[2]}`; - segments.splice(2, 1); - } + const { errorType, errorDetails } = groups; - const [errorDetails, errorType] = segments.reverse(); - return [errorDetails, errorType === 'Error' ? null : errorType]; + // `Error`, and driver names like `error`, tell the user nothing + return [errorDetails.trim() || null, errorType.toLowerCase() === 'error' ? null : errorType]; } }