diff --git a/packages/core/src/errors/error-reporter.ts b/packages/core/src/errors/error-reporter.ts index 21097dcb611..cae7d692cdc 100644 --- a/packages/core/src/errors/error-reporter.ts +++ b/packages/core/src/errors/error-reporter.ts @@ -31,6 +31,8 @@ const SIX_WEEKS_IN_MS = 6 * 7 * ONE_DAY_IN_MS; const RELEASE_EXPIRATION_WARNING = 'Error tracking disabled because this release is older than 6 weeks.'; +const SENTRY_MAX_VALUE_LENGTH = 500; + @Service() export class ErrorReporter { private expirationTimer?: NodeJS.Timeout; @@ -147,6 +149,7 @@ export class ErrorReporter { tracesSampleRate: inProduction ? 0.01 : 0, serverName, beforeBreadcrumb: () => null, + maxValueLength: SENTRY_MAX_VALUE_LENGTH, beforeSend: this.beforeSend.bind(this) as NodeOptions['beforeSend'], integrations: (integrations) => [ ...integrations.filter(({ name }) => enabledIntegrations.includes(name)), diff --git a/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts b/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts new file mode 100644 index 00000000000..404fec3826c --- /dev/null +++ b/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts @@ -0,0 +1,204 @@ +/** + * Verifies how errors in WorkflowExecute decides what gets forwarded to ErrorReporter + */ + +jest.mock('@n8n/di', () => ({ + Container: { + get: jest.fn(), + }, + Service: () => (target: unknown) => target, +})); + +import { Container } from '@n8n/di'; +import { mock } from 'jest-mock-extended'; +import { + ApplicationError, + createDeferredPromise, + NodeConnectionTypes, + OperationalError, + UnexpectedError, + UserError, + Workflow, +} from 'n8n-workflow'; +import type { INodeType, INodeTypes, IWorkflowExecuteAdditionalData, IRun } from 'n8n-workflow'; + +import { ExecutionLifecycleHooks } from '@/execution-engine/execution-lifecycle-hooks'; + +import { createNodeData } from '../partial-execution-utils/__tests__/helpers'; +import { WorkflowExecute } from '../workflow-execute'; + +const mockContainer = Container as jest.Mocked; + +function makeAdditionalData( + waitPromise: ReturnType>, +): IWorkflowExecuteAdditionalData { + const hooks = new ExecutionLifecycleHooks('trigger', '1', mock()); + hooks.addHandler('workflowExecuteAfter', (fullRunData) => waitPromise.resolve(fullRunData)); + return mock({ hooks }); +} + +describe('WorkflowExecute node error forwarding to ErrorReporter', () => { + let mockErrorReporter: { error: jest.Mock }; + let mockNodeTypes: jest.Mocked; + let mockLogger: { error: jest.Mock; warn: jest.Mock }; + let mockExecutionContextService: { augmentExecutionContextWithHooks: jest.Mock }; + + beforeEach(() => { + jest.clearAllMocks(); + + mockErrorReporter = { error: jest.fn() }; + mockLogger = { error: jest.fn(), warn: jest.fn() }; + + // establishExecutionContext is called before any node runs and does + // Container.get(ExecutionContextService).augmentExecutionContextWithHooks() + // Without this the call throws and aborts the run + mockExecutionContextService = { + augmentExecutionContextWithHooks: jest.fn().mockResolvedValue({ + context: { version: 1, establishedAt: Date.now(), source: 'manual' }, + triggerItems: undefined, + }), + }; + + mockContainer.get.mockImplementation((token) => { + const name = typeof token === 'function' ? token.name : token; + if (name === 'ErrorReporter') return mockErrorReporter; + if (name === 'Logger') return mockLogger; + if (name === 'ExecutionContextService') return mockExecutionContextService; + return { error: jest.fn(), warn: jest.fn() }; + }); + + mockNodeTypes = mock(); + }); + + /** + * Runs a workflow with a single node rejecting with `error`, then resolves + * once execution has finished. Wires the throwing node into mockNodeTypes and + * suppresses checkForWorkflowIssues + */ + async function runWorkflowThatThrows(error: unknown): Promise { + const nodeType = mock({ + description: { + name: 'manualTrigger', + displayName: 'Manual Trigger', + defaultVersion: 1, + properties: [], + inputs: [], + outputs: [{ type: NodeConnectionTypes.Main }], + }, + execute: jest.fn().mockRejectedValue(error) as unknown as INodeType['execute'], + }); + mockNodeTypes.getByNameAndVersion.mockReturnValue(nodeType); + + const triggerNode = createNodeData({ + name: 'ThrowingNode', + type: 'n8n-nodes-base.manualTrigger', + }); + const workflow = new Workflow({ + id: 'test-error', + nodes: [triggerNode], + connections: {}, + active: false, + nodeTypes: mockNodeTypes, + }); + + const waitPromise = createDeferredPromise(); + const additionalData = makeAdditionalData(waitPromise); + const workflowExecute = new WorkflowExecute(additionalData, 'manual'); + + jest + .spyOn( + workflowExecute as unknown as { checkForWorkflowIssues: () => void }, + 'checkForWorkflowIssues', + ) + .mockImplementation(() => {}); + + await workflowExecute.run(workflow, triggerNode); + await waitPromise.promise; + } + + it('should report a Error instance as class + stack only, without its message', async () => { + const rawError = new Error('raw error message'); + + await runWorkflowThatThrows(rawError); + + expect(mockErrorReporter.error).toHaveBeenCalledTimes(1); + const [reported] = mockErrorReporter.error.mock.calls[0] as [Error]; + expect(reported).toBeInstanceOf(Error); + expect(reported.message).not.toContain('raw error message'); + expect(reported.message).toBe('Error'); + expect(reported.stack).toBeDefined(); + }); + + it('should report an ApplicationError wrapping a raw Error', async () => { + const causeError = new Error('underlying third-party error'); + const wrappedError = new ApplicationError('wrapper', { cause: causeError }); + + await runWorkflowThatThrows(wrappedError); + + expect(mockErrorReporter.error).toHaveBeenCalledWith( + causeError, + expect.objectContaining({ + extra: { + nodeName: 'ThrowingNode', + nodeType: 'n8n-nodes-base.manualTrigger', + nodeVersion: expect.any(Number), + workflowId: 'test-error', + }, + }), + ); + }); + + it.each([ + ['UnexpectedError', new UnexpectedError('unexpected')], + ['OperationalError', new OperationalError('operational')], + ['UserError', new UserError('user')], + ])('should report a %s (BaseError subclass) as-is', async (_name, baseError) => { + await runWorkflowThatThrows(baseError); + + expect(mockErrorReporter.error).toHaveBeenCalledTimes(1); + const [reported] = mockErrorReporter.error.mock.calls[0] as [Error]; + expect(reported).toBe(baseError); + }); + + it('should not report an ApplicationError with no cause', async () => { + const plainAppError = new ApplicationError('plain operational error', { level: 'error' }); + + await runWorkflowThatThrows(plainAppError); + + expect(mockErrorReporter.error).not.toHaveBeenCalled(); + }); + + it('should forward an error to Sentry with no message', async () => { + const payloadError = new Error('Failed to parse: {"key":"value"}'); + + await runWorkflowThatThrows(payloadError); + + expect(mockErrorReporter.error).toHaveBeenCalledTimes(1); + const [reported] = mockErrorReporter.error.mock.calls[0] as [Error]; + expect(reported.message).not.toContain('Failed to parse'); + expect(reported.message).not.toContain('"value"'); + }); + + it('should strip the message from the reported stack', async () => { + const payloadError = new Error('Failed to parse: {"key1":"single-line-value"}'); + + await runWorkflowThatThrows(payloadError); + + const [reported] = mockErrorReporter.error.mock.calls[0] as [Error]; + expect(reported.stack).toBeDefined(); + expect(reported.stack).not.toContain('Failed to parse'); + expect(reported.stack).not.toContain('single-line-value'); + }); + + it('should strip a multi-line message from the reported stack', async () => { + const payloadError = new Error('Failed to parse. Text: first-value\nMore text: second-value'); + + await runWorkflowThatThrows(payloadError); + + const [reported] = mockErrorReporter.error.mock.calls[0] as [Error]; + expect(reported.stack).toBeDefined(); + expect(reported.stack).not.toContain('first-value'); + expect(reported.stack).not.toContain('second-value'); + expect(reported.stack).toContain('at '); // call frame + }); +}); diff --git a/packages/core/src/execution-engine/workflow-execute.ts b/packages/core/src/execution-engine/workflow-execute.ts index b8c28141774..3d802d51b80 100644 --- a/packages/core/src/execution-engine/workflow-execute.ts +++ b/packages/core/src/execution-engine/workflow-execute.ts @@ -47,6 +47,7 @@ import { NodeHelpers, NodeConnectionTypes, ApplicationError, + BaseError, sleep, Node, UnexpectedError, @@ -1779,9 +1780,23 @@ export class WorkflowExecute { if (error instanceof ApplicationError) { // Report any unhandled errors that were wrapped in by one of our error classes if (error.cause instanceof Error) toReport = error.cause; - } else { - // Report any unhandled and non-wrapped errors to Sentry + } else if (error instanceof BaseError) { + // BaseError subclasses specify shouldReport and level + // so always report and let beforeSend decide toReport = error; + } else if (error instanceof Error) { + // Non-BaseError errors only report their class and call frames + // The full error is still stored in the execution resultData + // Stack frames are in the format `: \n` + // they can span multiple lines, so we drop everything except call frame lines + const errorClass = error.name || 'Error'; + const sanitized = new Error(errorClass); + sanitized.name = errorClass; + const frames = (error.stack ?? '') + .split('\n') + .filter((line) => /^\s+at\s/.test(line)); + sanitized.stack = [errorClass, ...frames].join('\n'); + toReport = sanitized; } if (toReport) { Container.get(ErrorReporter).error(toReport, {