fix(core): Keep breaking change detection running when a rule throws (#38738)

Co-authored-by: Claude Fable 5.1 <[email protected]>

packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.service.test.ts

@@ -24,6 +24,7 @@ describe('BreakingChangeService', () => {
 	let workflowStatisticsRepository: Mocked<WorkflowStatisticsRepository>;
 	let ruleRegistry: RuleRegistry;
 	let cacheService: Mocked<CacheService>;
+	let errorReporter: Mocked<ErrorReporter>;
 	let service: BreakingChangeService;
 
 	beforeEach(() => {
@@ -33,6 +34,7 @@ describe('BreakingChangeService', () => {
 		workflowStatisticsRepository = mock<WorkflowStatisticsRepository>();
 		ruleRegistry = new RuleRegistry(logger);
 		cacheService = mock<CacheService>();
+		errorReporter = mock<ErrorReporter>();
 
 		// Mock getHashValue to call refreshFn directly (bypass caching for tests)
 		cacheService.getHashValue.mockImplementation(async (_key, _hashKey, options) => {
@@ -52,7 +54,7 @@ describe('BreakingChangeService', () => {
 			workflowStatisticsRepository,
 			cacheService,
 			logger,
-			mock<ErrorReporter>(),
+			errorReporter,
 		);
 
 		// Manually register only the rules we want to test with
@@ -185,6 +187,39 @@ describe('BreakingChangeService', () => {
 			expect(result2).toEqual(result3);
 		});
 
+		it('should skip a rule that throws for a workflow and keep the other results', async () => {
+			const { workflow } = createWorkflow('wf-1', 'Test Workflow', [
+				createNode('Spontit Node', 'n8n-nodes-base.spontit'),
+			]);
+			workflowRepository.find.mockResolvedValue([workflow as never]);
+			workflowRepository.count.mockResolvedValue(1);
+
+			const throwingRule = ruleRegistry.getRule('file-access-restriction-v2') as FileAccessRule;
+			vi.spyOn(throwingRule, 'detectWorkflow').mockRejectedValue(new Error('boom'));
+
+			const result = await service.getDetectionResults('v2');
+
+			const ruleIds = result.report.workflowResults.map((r) => r.ruleId);
+			expect(ruleIds).toContain('removed-nodes-v2');
+			expect(ruleIds).not.toContain(throwingRule.id);
+			expect(errorReporter.error).toHaveBeenCalledWith(
+				expect.any(Error),
+				expect.objectContaining({ extra: { ruleId: throwingRule.id, workflowId: 'wf-1' } }),
+			);
+		});
+
+		it('should reject when detection fails and allow a later detection to run', async () => {
+			workflowRepository.count.mockRejectedValueOnce(new Error('db down'));
+			workflowRepository.count.mockResolvedValue(0);
+			workflowRepository.find.mockResolvedValue([]);
+
+			await expect(service.getDetectionResults('v2')).rejects.toThrow('db down');
+
+			const detectSpy = vi.spyOn(service, 'detect');
+			await expect(service.getDetectionResults('v2')).resolves.toBeDefined();
+			expect(detectSpy).toHaveBeenCalledTimes(1);
+		});
+
 		it('should clean up ongoing detection promise after completion', async () => {
 			workflowRepository.find.mockResolvedValue([]);
 			workflowRepository.count.mockResolvedValue(0);

packages/cli/src/modules/breaking-changes/breaking-changes.service.ts

@@ -23,6 +23,7 @@ import type {
 	IBreakingChangeInstanceRule,
 	IBreakingChangeRule,
 	IBreakingChangeWorkflowRule,
+	WorkflowDetectionReport,
 } from './types';
 import { N8N_VERSION } from '../../constants';
 
@@ -84,7 +85,6 @@ export class BreakingChangeService {
 					});
 				}
 			} catch (error) {
-				console.log('error', error);
 				this.errorReporter.error(error, { shouldBeLogged: true });
 			}
 		}
@@ -227,7 +227,13 @@ export class BreakingChangeService {
 				workflowMetadataMap.set(workflow.id, workflowMetadata);
 
 				for (const rule of workflowLevelRules) {
-					const result = await rule.detectWorkflow(workflow, nodesGroupedByType);
+					let result: WorkflowDetectionReport;
+					try {
+						result = await rule.detectWorkflow(workflow, nodesGroupedByType);
+					} catch (error) {
+						this.reportRuleError(error, rule.id, workflow.id);
+						continue;
+					}
 					if (result.isAffected) {
 						const affectedWorkflow: BreakingChangeAffectedWorkflow = {
 							id: workflow.id,
@@ -244,7 +250,11 @@ export class BreakingChangeService {
 				}
 
 				for (const rule of batchRules) {
-					await rule.collectWorkflowData(workflow, nodesGroupedByType);
+					try {
+						await rule.collectWorkflowData(workflow, nodesGroupedByType);
+					} catch (error) {
+						this.reportRuleError(error, rule.id, workflow.id);
+					}
 				}
 			}
 		}
@@ -275,40 +285,39 @@ export class BreakingChangeService {
 			return await existingDetection;
 		}
 
-		const cacheKey = `${BreakingChangeService.CACHE_KEY_PREFIX}_${targetVersion}`;
-
-		// Start a new detection and store the promise
-		const detectionPromise: Promise<BreakingChangeReportResult> = new Promise((resolve) => {
-			void (async () => {
-				// Check cache first
-				const cachedResult = await this.cacheService.get<BreakingChangeReportResult>(cacheKey);
-				if (cachedResult) {
-					this.logger.debug('Using cached breaking change detection results', {
-						targetVersion,
-					});
-					return resolve(cachedResult);
-				}
-
-				// Perform detection
-				const detectionResult = await this.detect(targetVersion);
-				return resolve(detectionResult);
-			})();
-		});
+		const detectionPromise = this.detectWithCache(targetVersion);
 		this.ongoingDetections.set(targetVersion, detectionPromise);
 
 		try {
-			const result = await detectionPromise;
-			// Store in cache if detection took significant time
-			if (result.shouldCache) {
-				await this.cacheService.set(cacheKey, result);
-			}
-			return result;
+			return await detectionPromise;
 		} finally {
-			// Clean up the promise after completion (success or failure)
 			this.ongoingDetections.delete(targetVersion);
 		}
 	}
 
+	private async detectWithCache(
+		targetVersion: BreakingChangeVersion,
+	): Promise<BreakingChangeReportResult> {
+		const cacheKey = `${BreakingChangeService.CACHE_KEY_PREFIX}_${targetVersion}`;
+
+		const cachedResult = await this.cacheService.get<BreakingChangeReportResult>(cacheKey);
+		if (cachedResult) {
+			this.logger.debug('Using cached breaking change detection results', { targetVersion });
+			return cachedResult;
+		}
+
+		const result = await this.detect(targetVersion);
+		if (result.shouldCache) {
+			await this.cacheService.set(cacheKey, result);
+		}
+		return result;
+	}
+
+	private reportRuleError(error: unknown, ruleId: string, workflowId: string) {
+		this.logger.warn('Breaking change rule failed for workflow, skipping', { ruleId, workflowId });
+		this.errorReporter.error(error, { extra: { ruleId, workflowId } });
+	}
+
 	private shouldCacheDetection(durationMs: number): boolean {
 		return durationMs > BreakingChangeService.REPORT_DURATION_CACHE_THRESHOLD;
 	}

packages/cli/src/modules/breaking-changes/rules/v2/__tests__/process-env-access.rule.test.ts

@@ -1,3 +1,5 @@
+import type { INode } from 'n8n-workflow';
+
 import { createNode, createWorkflow } from '../../../__tests__/test-helpers';
 import { BreakingChangeCategory } from '../../../types';
 import { ProcessEnvAccessRule } from '../process-env-access.rule';
@@ -10,6 +12,17 @@ describe('ProcessEnvAccessRule', () => {
 		rule = new ProcessEnvAccessRule();
 	});
 
+	it('should not be affected by a node that has no parameters', async () => {
+		const node = { ...createNode('Start', 'n8n-nodes-base.manualTrigger'), parameters: undefined };
+		const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [
+			node as unknown as INode,
+		]);
+
+		const result = await rule.detectWorkflow(workflow, nodesGroupedByType);
+
+		expect(result.isAffected).toBe(false);
+	});
+
 	describe('getMetadata()', () => {
 		it('should return correct metadata', () => {
 			const metadata = rule.getMetadata();

packages/cli/src/modules/breaking-changes/rules/v2/process-env-access.rule.ts

@@ -54,7 +54,7 @@ export class ProcessEnvAccessRule implements IBreakingChangeWorkflowRule {
 				}
 			} else {
 				// Check in expressions
-				const nodeJson = JSON.stringify(node.parameters);
+				const nodeJson = JSON.stringify(node.parameters ?? {});
 				if (processEnvPattern.test(nodeJson) && !affectedNodes.some((n) => n.nodeId === node.id)) {
 					affectedNodes.push({ nodeId: node.id, nodeName: node.name });
 				}

packages/cli/src/modules/breaking-changes/rules/v3/__tests__/get-paired-item.rule.test.ts

@@ -1,3 +1,5 @@
+import type { INode } from 'n8n-workflow';
+
 import { createNode, createWorkflow } from '../../../__tests__/test-helpers';
 import { GetPairedItemRule } from '../get-paired-item.rule';
 
@@ -9,6 +11,20 @@ describe('GetPairedItemRule', () => {
 	});
 
 	describe('detectWorkflow()', () => {
+		it('should not be affected by a node that has no parameters', async () => {
+			const node = {
+				...createNode('Start', 'n8n-nodes-base.manualTrigger'),
+				parameters: undefined,
+			};
+			const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [
+				node as unknown as INode,
+			]);
+
+			const result = await rule.detectWorkflow(workflow, nodesGroupedByType);
+
+			expect(result.isAffected).toBe(false);
+		});
+
 		it('should not be affected when no node uses $getPairedItem', async () => {
 			const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [
 				createNode('Set', 'n8n-nodes-base.set', { value: '={{ $json.name }}' }),

packages/cli/src/modules/breaking-changes/rules/v3/get-paired-item.rule.ts

@@ -47,7 +47,7 @@ export class GetPairedItemRule implements IBreakingChangeWorkflowRule {
 		_nodesGroupedByType: Map<string, INode[]>,
 	): Promise<WorkflowDetectionReport> {
 		const affectedNodes = workflow.nodes.filter((node) =>
-			JSON.stringify(node.parameters).includes(HELPER_NAME),
+			JSON.stringify(node.parameters ?? {}).includes(HELPER_NAME),
 		);
 
 		return reportAffectedNodes(affectedNodes, (node) => ({