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

fix(core): Start WaitTracker only in the main container #9600

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

netroy
Copy link
Member

@netroy netroy commented Jun 3, 2024

WaitTracker initialization in the constructor means that as soon as this class enters the DI dependency tree, we start tracking for waiting executions, causing getWaitingExecutions queries to run even before the execution_entity table might be available.
This can be verified by using any of the import commands on a new DB on any of the last releases.

Review / Merge checklist

  • PR title and summary are descriptive
  • Tests included

@netroy netroy requested a review from ivov June 3, 2024 13:07
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jun 3, 2024
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

WaitTracker initialization in the constructor means that as soon as this class enters the DI dependency tree, we start tracking for waiting executions

Thanks for this! Will keep an eye out for timers in constructors.

Tested with single main and with multi-main leader and follower and it works as expected.

Copy link
Contributor

github-actions bot commented Jun 3, 2024

✅ All Cypress E2E specs passed

Copy link

cypress bot commented Jun 3, 2024

1 flaky test on run #5291 ↗︎

0 354 0 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project: n8n Commit: dd44b3502e
Status: Passed Duration: 04:53 💡
Started: Jun 3, 2024 3:28 PM Ended: Jun 3, 2024 3:33 PM
Flakiness  cypress/e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > Stop listening for trigger event from NDV Screenshots Video

Review all test suite changes for PR #9600 ↗︎

@netroy netroy merged commit 08d9c9a into master Jun 3, 2024
29 checks passed
@netroy netroy deleted the explit-WaitTracker branch June 3, 2024 15:43
@github-actions github-actions bot mentioned this pull request Jun 5, 2024
@janober
Copy link
Member

janober commented Jun 5, 2024

Got released with n8n@1.45.0

@ivov ivov added the release/backport Changes that need to be backported to older releases. label Jun 7, 2024
@github-actions github-actions bot mentioned this pull request Jun 7, 2024
netroy added a commit that referenced this pull request Jun 11, 2024
We changed WaitTracker to not start the timer in the constructor in #9600. So we don't need to mock it anymore.
@github-actions github-actions bot mentioned this pull request Jun 12, 2024
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 release/backport Changes that need to be backported to older releases. Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants