Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Implement inter-main communication for test webhooks in multi-main setup #8267

Merged

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Jan 9, 2024

Summary

In single-main setup, the main process keeps the frontend informed of the real-time state of the execution via hooks that push events using websockets or server-sent events. At the end of a test webhook execution, the main process also cleans up test webhooks, i.e. clears the 2-minute timeout, deregisters all test webhooks from the cache hash, and deletes them from the third-party service.

In multi-main setup, it may happen that the user's browser is connected to the main process that created the webhook, but the webhook call then arrives at a different main process. In this case events are not being informed to the frontend that the creator process is connected to, and cleanup is not happening in the creator process either.

To address this, we implement inter-main communication. The webhook-handling process does not push to its frontend - rather, the webhook-handling process sends a series of events into the command channel that all main instances are listening to. On receiving an event via this channel, the main process, if it has the relevant session ID, reacts by pushing the event to the FE.

Additionally, we set up a new event for a successful test webhook execution, and following the same procedure we instruct the original process to clear the webhooks after execution. And as a safeguard if the creator process crashes, we set a TTL on the test webhook registration hash in the cache - this TTL is extended on each new registration.

Diagram

Capture 2024-01-10 at 12 24 36@2x

How to test

  1. Run Postgres and Redis containers and set up config file:
docker network create n8n

docker run --name n8n-postgres --network n8n -p 5433:5432 -d postgres

docker exec n8n-postgres psql -U postgres -c "CREATE DATABASE n8n;"

docker run --name n8n-redis --network n8n -p 6379:6379 -d redis
  1. Start ngrok:
ngrok http --domain=<subdomain>.ngrok-free.app 5678
  1. Start first main:
EXECUTIONS_MODE=queue N8N_CONFIG_FILES=<path> N8N_MULTI_MAIN_SETUP_ENABLED=true N8N_LICENSE_TENANT_ID=<id> N8N_LICENSE_ACTIVATION_KEY=<key> N8N_LOG_LEVEL=debug WEBHOOK_URL=https://<subdomain>.ngrok-free.app pnpm start
  1. Start second main:
EXECUTIONS_MODE=queue N8N_MULTI_MAIN_SETUP_ENABLED=true N8N_LICENSE_TENANT_ID=<id> N8N_LICENSE_ACTIVATION_KEY=<key> N8N_LOG_LEVEL=debug N8N_PORT=5679 WEBHOOK_URL=https://<subdomain>.hooks.n8n.cloud/ pnpm start
  1. Start worker:
EXECUTIONS_MODE=queue N8N_CONFIG_FILES=<path> N8N_LICENSE_TENANT_ID=<id> N8N_LICENSE_ACTIVATION_KEY=<key> N8N_ENCRYPTION_KEY=<key> N8N_LOG_LEVEL=debug pnpm worker

Expectations:

  • Log into the second main at http://localhost:5679, set up a test webhook, run it, and call it at http://localhost:5679/webhook-test/<path>. The webhook call should land via ngrok on the first main, which should handle it successfully, relaying execution lifecycle events to the second main, and clearing test webhooks on success.
  • Log into the first main at http://localhost:5678, set up a test webhook, run it, and call it at http://localhost:5678/webhook-test/<path>. The webhook call should land on the first main, which should handle it successfully and clear test webhooks, without going through inter-main communication.
  • Test webhook cancellation and expiration should continue to work as before, without going through inter-main communication.

Story

https://linear.app/n8n/issue/PAY-1189

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jan 9, 2024
@ivov ivov changed the title feat(core): Implement IPC for test webhooks in multi-main setup feat(core): Implement inter-main communication for test webhooks in multi-main setup Jan 9, 2024
@ivov ivov marked this pull request as ready for review January 10, 2024 10:59
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this file needs refactoring and cleanup, hopefully soon. Only added to it to avoid increasing the scope.

Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

cypress bot commented Jan 12, 2024

2 flaky tests on run #3734 ↗︎

0 330 5 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n Commit: 302103eeb2
Status: Passed Duration: 04:01 💡
Started: Jan 12, 2024 10:43 AM Ended: Jan 12, 2024 10:47 AM
Flakiness  24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items Screenshots Video
Flakiness  29-templates.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Templates > should save template id with the workflow Screenshots Video

Review all test suite changes for PR #8267 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit 1a0e285 into master Jan 12, 2024
32 checks passed
@ivov ivov deleted the pay-1189-implement-ipc-for-test-webhooks-in-multi-main-setup branch January 12, 2024 10:48
@github-actions github-actions bot mentioned this pull request Jan 17, 2024
@janober
Copy link
Member

janober commented Jan 17, 2024

Got released with [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants