Skip to content

Commit

Permalink
refactor(core)!: Remove webhook deregistration at startup and shutdown (
Browse files Browse the repository at this point in the history
  • Loading branch information
ivov authored Oct 26, 2023
1 parent 5477e3f commit ae8c7a6
Show file tree
Hide file tree
Showing 8 changed files with 19 additions and 75 deletions.
12 changes: 12 additions & 0 deletions packages/cli/BREAKING-CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
20 changes: 1 addition & 19 deletions packages/cli/src/ActiveWorkflowRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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:');
Expand Down Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions packages/cli/src/TestWebhooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,12 +284,4 @@ export class TestWebhooks implements IWebhookManager {

return foundWebhook;
}

/**
* Removes all the currently active test webhooks
*/
async removeAll(): Promise<void> {
const workflows = Object.values(this.testWebhookData).map(({ workflow }) => workflow);
return this.activeWebhooks.removeAll(workflows);
}
}
6 changes: 6 additions & 0 deletions packages/cli/src/commands/BaseCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
16 changes: 0 additions & 16 deletions packages/cli/src/commands/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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();
Expand Down
6 changes: 0 additions & 6 deletions packages/cli/src/services/webhook.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
2 changes: 0 additions & 2 deletions packages/cli/test/unit/ActiveWorkflowRunner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand All @@ -171,7 +170,6 @@ describe('ActiveWorkflowRunner', () => {
databaseActiveWorkflowsCount,
);
expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled();
expect(webhookService.deleteInstanceWebhooks).toHaveBeenCalled();
expect(externalHooks.run).toHaveBeenCalled();
});

Expand Down
24 changes: 0 additions & 24 deletions packages/cli/test/unit/services/webhook.service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down

0 comments on commit ae8c7a6

Please sign in to comment.