fix(core): Handle cancellation of waiting executions correctly #13051
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
n8n-assistant
bot
added
core
netroy
force-pushed
the
shutdown-with-waiting-executions
branch
from
da9f95c to
ed009e1
Compare
| let count = 0; | ||
| let executionIds = Object.keys(this.activeExecutions); |
There was a problem hiding this comment.
What if new executions end up in the active executions between when we cancel them and we start waiting for them? I guess those never get cancelled?
There was a problem hiding this comment.
we already have some mechanisms to prevent that from happening, but we still might need more checks. however, that's not a problem specific to this PR, right?
There was a problem hiding this comment.
No. I'm just pointing out potential issues we have in the current implementation
There was a problem hiding this comment.
should we add a isShutingDown flag here, like we have on a couple of other services?
|
✅ All Cypress E2E specs passed |
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
shutdown-with-waiting-executions
|
| Run status |
|
| Run duration | 04m 33s |
| Commit |
|
| Committer | कारतोफ्फेलस्क्रिप्ट™ |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
5
|
Skipped
|
0
|
Passing
|
434
|
| View all changes introduced in this branch ↗︎ | |
Summary
In #12806 we stopped removing waiting executions from
ActiveExecutionsto be able to handle the response promises correctly. This has however led to a new issue where cancellation of waiting executions doesn't remove the execution fromActiveExecutions, which also prevents n8n from shutting down cleanly.Related Linear tickets, Github issues, and Community forum posts
CAT-545
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)