fix(core): Abandon trigger teardown that can never succeed instead of retrying it forever (#36843)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

packages/cli/src/workflows/publication/__tests__/workflow-publication-outbox-consumer.test.ts

@@ -3,6 +3,7 @@ import type { WorkflowsConfig } from '@n8n/config';
 import type { WorkflowPublicationOutbox, WorkflowPublicationOutboxRepository } from '@n8n/db';
 import { mock } from 'vitest-mock-extended';
 import type { ErrorReporter, InstanceSettings, Span, Tracing } from 'n8n-core';
+import { UserError } from 'n8n-workflow';
 
 import type { EventService } from '@/events/event.service';
 import type { PublicationResult } from '@/workflows/publication/publication-result';
@@ -454,6 +455,18 @@ describe('WorkflowPublicationOutboxConsumer', () => {
 			);
 		});
 
+		test('reports a UserError from the applier as-is, without the Unexpected wrapper', async () => {
+			const userError = new UserError('Credential with ID "c-1" does not exist');
+			applier.apply.mockRejectedValue(userError);
+
+			await consumer.processRecord(makeRecord(), abortSignal);
+
+			expect(reporter.report).toHaveBeenCalledWith(
+				expect.anything(),
+				expect.objectContaining({ type: 'failed', error: userError }),
+			);
+		});
+
 		test('logs but swallows a reporter failure, leaving the record for retry', async () => {
 			const reportError = new Error('db write failed');
 			reporter.report.mockRejectedValue(reportError);

packages/cli/src/workflows/publication/workflow-publication-applier.ts

@@ -180,8 +180,10 @@ export class WorkflowPublicationApplier {
 		abort.signal.throwIfAborted();
 
 		// Must happen BEFORE advancing the version, using the currently published
-		// version so the right webhooks are deregistered. A teardown failure here
-		// bubbles up so the version is not advanced.
+		// version so the right webhooks are deregistered. A retryable teardown
+		// failure bubbles up so the version is not advanced; one that can never
+		// succeed (a UserError) is abandoned inside `deactivate`, since retrying
+		// it would block this publication forever.
 		if (toRemove.size > 0 && oldVersion) {
 			await this.workflowTriggerActivator.deactivate(workflow, oldVersion, toRemove, abort);
 		}
@@ -244,7 +246,11 @@ export class WorkflowPublicationApplier {
 	 * clears any trigger-status rows left behind by an interrupted unpublish.
 	 *
 	 * A teardown failure bubbles up (the consumer turns it into a `failed` result)
-	 * so the mapping is only removed once teardown has succeeded.
+	 * so the mapping is only removed once teardown has succeeded. A teardown
+	 * failure that can never succeed on retry (e.g. a webhook's delete hook
+	 * needs a credential that was deleted) is abandoned inside `deactivate`
+	 * rather than surfaced — the mapping is what the reconciler treats as drift,
+	 * so such a failure would otherwise re-enqueue this unpublish forever.
 	 */
 	private async unpublish(
 		workflow: WorkflowEntity,

packages/cli/src/workflows/publication/workflow-publication-outbox-consumer.ts

@@ -6,7 +6,7 @@ import { OnLeaderTakeover, OnPubSubEvent, OnShutdown } from '@n8n/decorators';
 import { Service } from '@n8n/di';
 import { ErrorReporter, InstanceSettings, SpanStatus, Tracing } from 'n8n-core';
 import { ensureError } from '@n8n/utils/errors/ensure-error';
-import { OperationalError, UnexpectedError } from 'n8n-workflow';
+import { OperationalError, UnexpectedError, UserError } from 'n8n-workflow';
 
 import { EventService } from '@/events/event.service';
 import type {
@@ -371,10 +371,12 @@ export class WorkflowPublicationOutboxConsumer {
 						const cause = ensureError(error);
 						result = {
 							type: 'failed',
-							// An abort is our own doing, not an unexpected applier failure.
-							error: signal.aborted
-								? cause
-								: new UnexpectedError(`Unexpected: ${cause.message}`, { cause }),
+							// An abort is our own doing and a UserError is a known cause (e.g.
+							// a missing credential), not an unexpected applier failure.
+							error:
+								signal.aborted || cause instanceof UserError
+									? cause
+									: new UnexpectedError(`Unexpected: ${cause.message}`, { cause }),
 						};
 					}
 

packages/cli/src/workflows/triggers/__tests__/workflow-trigger-activator.test.ts

@@ -4,7 +4,13 @@ import type { IWorkflowDb, WorkflowEntity, WorkflowRepository } from '@n8n/db';
 import { createDeferredPromise } from '@n8n/utils/promise/deferred-promise';
 import type { ErrorReporter, Span, Tracing } from 'n8n-core';
 import type { IWebhookData, IWorkflowExecuteAdditionalData } from 'n8n-workflow';
-import { WebhookPathTakenError, WorkflowActivationError, WorkflowExpression } from 'n8n-workflow';
+import {
+	UserError,
+	WebhookPathTakenError,
+	WorkflowActivationError,
+	WorkflowDeactivationError,
+	WorkflowExpression,
+} from 'n8n-workflow';
 import { mock, type MockProxy } from 'vitest-mock-extended';
 
 import type { ActivationErrorsService } from '@/activation-errors.service';
@@ -518,6 +524,96 @@ describe('WorkflowTriggerActivator', () => {
 		expect(callOrder).toContain('deregister-non-webhook-finish');
 	});
 
+	describe('deactivate teardown failures', () => {
+		function buildDeactivationSetup(deregisterError: Error) {
+			vi.spyOn(WorkflowExecuteAdditionalData, 'getBase').mockResolvedValue(
+				mock<IWorkflowExecuteAdditionalData>(),
+			);
+
+			const webhookTriggerRegistrar = mock<WebhookTriggerRegistrar>();
+			const webhookOk = mock<IWebhookData>({ node: 'Webhook OK' });
+			const webhookBroken = mock<IWebhookData>({ node: 'Webhook Broken' });
+			webhookTriggerRegistrar.getWebhookTriggers.mockReturnValue([webhookOk, webhookBroken]);
+			webhookTriggerRegistrar.deregister.mockImplementation(async ({ webhookData }) => {
+				if (webhookData.node === 'Webhook Broken') throw deregisterError;
+				return webhookData.node;
+			});
+			const nonWebhookTriggerRegistrar = mock<NonWebhookTriggerRegistrar>();
+			nonWebhookTriggerRegistrar.getTriggerNodeIds.mockReturnValue([]);
+
+			const activator = buildActivator({ webhookTriggerRegistrar, nonWebhookTriggerRegistrar });
+			const deactivate = async () =>
+				await activator.deactivate(
+					mock<WorkflowEntity>({ id: 'wf-1', name: 'Test workflow', staticData: {}, settings: {} }),
+					{
+						nodes: [
+							node('ok-node', 'webhook', { name: 'Webhook OK' }),
+							node('broken-node', 'webhook', { name: 'Webhook Broken' }),
+						],
+						connections: {},
+					},
+					new Set(['ok-node', 'broken-node']),
+					abort,
+				);
+
+			return { webhookTriggerRegistrar, deactivate };
+		}
+
+		test('counts a webhook whose deregistration fails with a UserError as removed', async () => {
+			// The failure can never succeed on retry (e.g. the delete hook's
+			// credential was deleted): the remote registration is abandoned and the
+			// node's rows are cleared instead of retained.
+			const { webhookTriggerRegistrar, deactivate } = buildDeactivationSetup(
+				new UserError('Credential with ID "c-1" does not exist for type "trelloApi".'),
+			);
+
+			await deactivate();
+
+			expect(webhookTriggerRegistrar.clearWorkflowWebhooksForNodes).toHaveBeenCalledWith('wf-1', [
+				'Webhook OK',
+				'Webhook Broken',
+			]);
+		});
+
+		test('a retryable failure still fails and retains the node rows', async () => {
+			const { webhookTriggerRegistrar, deactivate } = buildDeactivationSetup(
+				new Error('remote unreachable'),
+			);
+
+			await expect(deactivate()).rejects.toThrow('remote unreachable');
+			expect(webhookTriggerRegistrar.clearWorkflowWebhooksForNodes).toHaveBeenCalledWith('wf-1', [
+				'Webhook OK',
+			]);
+		});
+
+		test('skips a non-webhook trigger whose deregistration fails with a UserError, even when wrapped', async () => {
+			vi.spyOn(WorkflowExecuteAdditionalData, 'getBase').mockResolvedValue(
+				mock<IWorkflowExecuteAdditionalData>(),
+			);
+
+			const webhookTriggerRegistrar = mock<WebhookTriggerRegistrar>();
+			webhookTriggerRegistrar.getWebhookTriggers.mockReturnValue([]);
+			const nonWebhookTriggerRegistrar = mock<NonWebhookTriggerRegistrar>();
+			nonWebhookTriggerRegistrar.getTriggerNodeIds.mockReturnValue(['t']);
+			nonWebhookTriggerRegistrar.deregister.mockRejectedValue(
+				new WorkflowDeactivationError('Failed to deactivate trigger of workflow ID "wf-1"', {
+					cause: new UserError('teardown broken'),
+				}),
+			);
+
+			const activator = buildActivator({ webhookTriggerRegistrar, nonWebhookTriggerRegistrar });
+
+			await activator.deactivate(
+				mock<WorkflowEntity>({ id: 'wf-1', name: 'Test workflow', staticData: {}, settings: {} }),
+				{ nodes: [node('t', 'trigger')], connections: {} },
+				new Set(['t']),
+				abort,
+			);
+
+			expect(nonWebhookTriggerRegistrar.deregister).toHaveBeenCalledWith('wf-1', 't');
+		});
+	});
+
 	test('isolates failures across the concurrent webhook and non-webhook phases', async () => {
 		vi.spyOn(WorkflowExecuteAdditionalData, 'getBase').mockResolvedValue(
 			mock<IWorkflowExecuteAdditionalData>(),

packages/cli/src/workflows/triggers/workflow-trigger-activator.ts

@@ -21,6 +21,7 @@ import {
 	ERROR_TRIGGER_NODE_TYPE,
 	EXECUTE_WORKFLOW_TRIGGER_NODE_TYPE,
 	MANUAL_TRIGGER_NODE_TYPE,
+	UserError,
 	Workflow,
 	WorkflowActivationError,
 } from 'n8n-workflow';
@@ -658,8 +659,19 @@ export class WorkflowTriggerActivator {
 			}
 			deregistrationResults.forEach((result, index) => {
 				if (result.status === 'rejected') {
-					firstFailure ??= ensureError(result.reason);
-					return;
+					const error = ensureError(result.reason);
+					if (this.shouldAbandonFailedTeardown(error)) {
+						// The webhook counts as removed, so its row is cleared below
+						// instead of retained.
+						this.logger.warn('Abandoned webhook whose deregistration can never succeed', {
+							workflowId: workflow.id,
+							nodeName: webhooks[index].node,
+							error: error.message,
+						});
+					} else {
+						firstFailure ??= error;
+						return;
+					}
 				}
 				const nodeName = webhooks[index].node;
 				const pending = (pendingWebhooksByNode.get(nodeName) ?? 0) - 1;
@@ -781,13 +793,39 @@ export class WorkflowTriggerActivator {
 		const triggerNodeIds = this.getNonWebhookTriggerNodeIdsForNodeIds(workflow, nodeIds);
 
 		await Promise.all(
-			triggerNodeIds.map(
-				async (nodeId) =>
-					await raceAbort(this.nonWebhookTriggerRegistrar.deregister(workflowId, nodeId), abort),
-			),
+			triggerNodeIds.map(async (nodeId) => {
+				try {
+					await raceAbort(this.nonWebhookTriggerRegistrar.deregister(workflowId, nodeId), abort);
+				} catch (error) {
+					if (!this.shouldAbandonFailedTeardown(ensureError(error))) throw error;
+					this.logger.warn('Abandoned trigger whose deregistration can never succeed', {
+						workflowId,
+						nodeId,
+						error: ensureError(error).message,
+					});
+				}
+			}),
 		);
 	}
 
+	/**
+	 * A `UserError` from teardown (e.g. a delete hook whose credential was
+	 * deleted) can never succeed on retry, so the trigger is abandoned instead
+	 * of failing the deactivation — a retained registration would only make the
+	 * publication outbox retry a teardown that is permanently broken. An abort
+	 * rejects with the abort reason, never a `UserError`, so it is never
+	 * abandoned. Transient remote failures (network errors, API rejections)
+	 * are not `UserError`s and still fail for retry.
+	 *
+	 * The cause is checked too: a non-webhook trigger's close failure
+	 * arrives wrapped in a `WorkflowDeactivationError` with the node's error as
+	 * its `cause`.
+	 */
+	private shouldAbandonFailedTeardown(error: unknown): boolean {
+		if (error instanceof UserError) return true;
+		return error instanceof Error && error.cause instanceof UserError;
+	}
+
 	private getNonWebhookTriggerNodeIdsForNodeIds(workflow: Workflow, nodeIds: Set<INode['id']>) {
 		return this.nonWebhookTriggerRegistrar
 			.getTriggerNodeIds(workflow)