From 62d7e4c90177768dad508804ea8a1229f616665e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Repe=C4=87?= Date: Mon, 24 Aug 2026 14:23:26 +0000 Subject: [PATCH] fix(core): Abandon trigger teardown that can never succeed instead of retrying it forever (#36843) Co-authored-by: Claude Fable 5 --- ...rkflow-publication-outbox-consumer.test.ts | 13 +++ .../workflow-publication-applier.ts | 12 ++- .../workflow-publication-outbox-consumer.ts | 12 ++- .../workflow-trigger-activator.test.ts | 98 ++++++++++++++++++- .../triggers/workflow-trigger-activator.ts | 50 ++++++++-- 5 files changed, 170 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/workflows/publication/__tests__/workflow-publication-outbox-consumer.test.ts b/packages/cli/src/workflows/publication/__tests__/workflow-publication-outbox-consumer.test.ts index 9f3030c7645..148eef9b9c4 100644 --- a/packages/cli/src/workflows/publication/__tests__/workflow-publication-outbox-consumer.test.ts +++ b/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); diff --git a/packages/cli/src/workflows/publication/workflow-publication-applier.ts b/packages/cli/src/workflows/publication/workflow-publication-applier.ts index ddd821a580b..76d78114f80 100644 --- a/packages/cli/src/workflows/publication/workflow-publication-applier.ts +++ b/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, diff --git a/packages/cli/src/workflows/publication/workflow-publication-outbox-consumer.ts b/packages/cli/src/workflows/publication/workflow-publication-outbox-consumer.ts index 4c799024d7d..a11591631ba 100644 --- a/packages/cli/src/workflows/publication/workflow-publication-outbox-consumer.ts +++ b/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 }), }; } diff --git a/packages/cli/src/workflows/triggers/__tests__/workflow-trigger-activator.test.ts b/packages/cli/src/workflows/triggers/__tests__/workflow-trigger-activator.test.ts index 832541bd69e..95af9d5ab9a 100644 --- a/packages/cli/src/workflows/triggers/__tests__/workflow-trigger-activator.test.ts +++ b/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(), + ); + + const webhookTriggerRegistrar = mock(); + const webhookOk = mock({ node: 'Webhook OK' }); + const webhookBroken = mock({ 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.getTriggerNodeIds.mockReturnValue([]); + + const activator = buildActivator({ webhookTriggerRegistrar, nonWebhookTriggerRegistrar }); + const deactivate = async () => + await activator.deactivate( + mock({ 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(), + ); + + const webhookTriggerRegistrar = mock(); + webhookTriggerRegistrar.getWebhookTriggers.mockReturnValue([]); + const nonWebhookTriggerRegistrar = mock(); + 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({ 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(), diff --git a/packages/cli/src/workflows/triggers/workflow-trigger-activator.ts b/packages/cli/src/workflows/triggers/workflow-trigger-activator.ts index 133a71d4bb8..8dab0225cfe 100644 --- a/packages/cli/src/workflows/triggers/workflow-trigger-activator.ts +++ b/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) { return this.nonWebhookTriggerRegistrar .getTriggerNodeIds(workflow)