From ae8c7a635e8cb353e4ead90bad36d2c1e50bc0ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 26 Oct 2023 14:37:54 +0200 Subject: [PATCH] refactor(core)!: Remove webhook deregistration at startup and shutdown (#7515) https://linear.app/n8n/issue/PAY-932/deprecate-flag-to-skip-webhook-deregistration-on-shutdown --- packages/cli/BREAKING-CHANGES.md | 12 ++++++++++ packages/cli/src/ActiveWorkflowRunner.ts | 20 +--------------- packages/cli/src/TestWebhooks.ts | 8 ------- packages/cli/src/commands/BaseCommand.ts | 6 +++++ packages/cli/src/commands/start.ts | 16 ------------- packages/cli/src/services/webhook.service.ts | 6 ----- .../test/unit/ActiveWorkflowRunner.test.ts | 2 -- .../unit/services/webhook.service.test.ts | 24 ------------------- 8 files changed, 19 insertions(+), 75 deletions(-) diff --git a/packages/cli/BREAKING-CHANGES.md b/packages/cli/BREAKING-CHANGES.md index e150b782072ea..bc2316f631709 100644 --- a/packages/cli/BREAKING-CHANGES.md +++ b/packages/cli/BREAKING-CHANGES.md @@ -2,6 +2,18 @@ This list shows all the versions which include breaking changes and how to upgrade. +## 1.15.0 + +### What changed? + +Until now, in main mode, n8n used to deregister webhooks at shutdown and reregister them at startup. Queue mode and the flag `N8N_SKIP_WEBHOOK_DEREGISTRATION_SHUTDOWN` skipped webhook deregistration. + +As from now, in both main and queue modes, n8n no longer deregisters webhooks at startup and shutdown, and the flag `N8N_SKIP_WEBHOOK_DEREGISTRATION_SHUTDOWN` is removed. n8n assumes that third-party services will retry unhandled webhook requests. + +### When is action necessary? + +If using the flag `N8N_SKIP_WEBHOOK_DEREGISTRATION_SHUTDOWN`, note that it no longer has effect and can be removed from your settings. + ## 1.9.0 ### What changed? diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index fbaf850bf2f2a..a91e510065d32 100644 --- a/packages/cli/src/ActiveWorkflowRunner.ts +++ b/packages/cli/src/ActiveWorkflowRunner.ts @@ -47,7 +47,6 @@ import * as ResponseHelper from '@/ResponseHelper'; import * as WebhookHelpers from '@/WebhookHelpers'; import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData'; -import config from '@/config'; import type { User } from '@db/entities/User'; import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { ActiveExecutions } from '@/ActiveExecutions'; @@ -101,18 +100,6 @@ export class ActiveWorkflowRunner implements IWebhookManager { relations: ['shared', 'shared.user', 'shared.user.globalRole', 'shared.role'], })) as IWorkflowDb[]; - if (!config.getEnv('endpoints.skipWebhooksDeregistrationOnShutdown')) { - // Do not clean up database when skip registration is done. - // This flag is set when n8n is running in scaled mode. - // Impact is minimal, but for a short while, n8n will stop accepting requests. - // Also, users had issues when running multiple "main process" - // instances if many of them start at the same time - // This is not officially supported but there is no reason - // it should not work. - // Clear up active workflow table - await this.webhookService.deleteInstanceWebhooks(); - } - if (workflowsData.length !== 0) { this.logger.info(' ================================'); this.logger.info(' Start Active Workflows:'); @@ -404,12 +391,7 @@ export class ActiveWorkflowRunner implements IWebhookManager { false, ); } catch (error) { - if ( - activation === 'init' && - config.getEnv('endpoints.skipWebhooksDeregistrationOnShutdown') && - error.name === 'QueryFailedError' - ) { - // When skipWebhooksDeregistrationOnShutdown is enabled, + if (activation === 'init' && error.name === 'QueryFailedError') { // n8n does not remove the registered webhooks on exit. // This means that further initializations will always fail // when inserting to database. This is why we ignore this error diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts index 8ffa9ad7ad863..4cd5d99809e18 100644 --- a/packages/cli/src/TestWebhooks.ts +++ b/packages/cli/src/TestWebhooks.ts @@ -284,12 +284,4 @@ export class TestWebhooks implements IWebhookManager { return foundWebhook; } - - /** - * Removes all the currently active test webhooks - */ - async removeAll(): Promise { - const workflows = Object.values(this.testWebhookData).map(({ workflow }) => workflow); - return this.activeWebhooks.removeAll(workflows); - } } diff --git a/packages/cli/src/commands/BaseCommand.ts b/packages/cli/src/commands/BaseCommand.ts index a7a6ae0bcd620..868f26d8252c6 100644 --- a/packages/cli/src/commands/BaseCommand.ts +++ b/packages/cli/src/commands/BaseCommand.ts @@ -74,6 +74,12 @@ export abstract class BaseCommand extends Command { ); } + if (process.env.N8N_SKIP_WEBHOOK_DEREGISTRATION_SHUTDOWN) { + this.logger.warn( + 'The flag to skip webhook deregistration N8N_SKIP_WEBHOOK_DEREGISTRATION_SHUTDOWN has been removed. n8n no longer deregisters webhooks at startup and shutdown, in main and queue mode.', + ); + } + await Container.get(PostHogClient).init(); await Container.get(InternalHooks).init(); } diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index b2e4431c2e6c3..0546e6687c72d 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -21,7 +21,6 @@ import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; import * as Db from '@/Db'; import * as GenericHelpers from '@/GenericHelpers'; import { Server } from '@/Server'; -import { TestWebhooks } from '@/TestWebhooks'; import { EDITOR_UI_DIST_DIR, GENERATED_STATIC_DIR } from '@/constants'; import { eventBus } from '@/eventbus'; import { BaseCommand } from './BaseCommand'; @@ -115,21 +114,6 @@ export class Start extends BaseCommand { await Container.get(InternalHooks).onN8nStop(); - const skipWebhookDeregistration = config.getEnv( - 'endpoints.skipWebhooksDeregistrationOnShutdown', - ); - - const removePromises = []; - if (!skipWebhookDeregistration) { - removePromises.push(this.activeWorkflowRunner.removeAll()); - } - - // Remove all test webhooks - const testWebhooks = Container.get(TestWebhooks); - removePromises.push(testWebhooks.removeAll()); - - await Promise.all(removePromises); - // Wait for active workflow executions to finish const activeExecutionsInstance = Container.get(ActiveExecutions); let executingWorkflows = activeExecutionsInstance.getActiveExecutions(); diff --git a/packages/cli/src/services/webhook.service.ts b/packages/cli/src/services/webhook.service.ts index 91cac46b0258e..30f0de294f99d 100644 --- a/packages/cli/src/services/webhook.service.ts +++ b/packages/cli/src/services/webhook.service.ts @@ -107,12 +107,6 @@ export class WebhookService { return this.deleteWebhooks(webhooks); } - async deleteInstanceWebhooks() { - const webhooks = await this.webhookRepository.find(); - - return this.deleteWebhooks(webhooks); - } - private async deleteWebhooks(webhooks: WebhookEntity[]) { void this.cacheService.deleteMany(webhooks.map((w) => w.cacheKey)); diff --git a/packages/cli/test/unit/ActiveWorkflowRunner.test.ts b/packages/cli/test/unit/ActiveWorkflowRunner.test.ts index 8b4a599f9bfe0..06527984fa9e2 100644 --- a/packages/cli/test/unit/ActiveWorkflowRunner.test.ts +++ b/packages/cli/test/unit/ActiveWorkflowRunner.test.ts @@ -160,7 +160,6 @@ describe('ActiveWorkflowRunner', () => { await activeWorkflowRunner.init(); expect(await activeWorkflowRunner.getActiveWorkflows()).toHaveLength(0); expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled(); - expect(webhookService.deleteInstanceWebhooks).toHaveBeenCalled(); expect(externalHooks.run).toHaveBeenCalledTimes(1); }); @@ -171,7 +170,6 @@ describe('ActiveWorkflowRunner', () => { databaseActiveWorkflowsCount, ); expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled(); - expect(webhookService.deleteInstanceWebhooks).toHaveBeenCalled(); expect(externalHooks.run).toHaveBeenCalled(); }); diff --git a/packages/cli/test/unit/services/webhook.service.test.ts b/packages/cli/test/unit/services/webhook.service.test.ts index 7c8f62212d275..e9df60d9dfe6e 100644 --- a/packages/cli/test/unit/services/webhook.service.test.ts +++ b/packages/cli/test/unit/services/webhook.service.test.ts @@ -150,30 +150,6 @@ describe('WebhookService', () => { }); }); - describe('deleteInstanceWebhooks()', () => { - test('should delete all webhooks of the instance', async () => { - const mockInstanceWebhooks = [ - createWebhook('PUT', 'users'), - createWebhook('GET', 'user/:id'), - createWebhook('POST', ':var'), - ]; - - webhookRepository.find.mockResolvedValue(mockInstanceWebhooks); - - await webhookService.deleteInstanceWebhooks(); - - expect(webhookRepository.remove).toHaveBeenCalledWith(mockInstanceWebhooks); - }); - - test('should not delete any webhooks if none found', async () => { - webhookRepository.find.mockResolvedValue([]); - - await webhookService.deleteInstanceWebhooks(); - - expect(webhookRepository.remove).toHaveBeenCalledWith([]); - }); - }); - describe('deleteWorkflowWebhooks()', () => { test('should delete all webhooks of the workflow', async () => { const mockWorkflowWebhooks = [