-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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: Refactoring of useRunWorkflow to fix race condition #11311
base: master
Are you sure you want to change the base?
fix: Refactoring of useRunWorkflow to fix race condition #11311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as before. I'm not 100% aware of how the bug is reproduced so I'll leave testing to someone who's involved. Small change request to help us out in the future.
const execution = await workflowsStore.getExecution((executionId as string) || ''); | ||
if ( | ||
execution && | ||
(execution.finished || ['error', 'canceled', 'crashed', 'success'].includes(execution.status)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition seems to be repeating here and on line 387. Can you please extract it into a isExecutionResolved
helper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the changes!
n8n Run #7432
Run Properties:
|
Project |
n8n
|
Run status |
Passed #7432
|
Run duration | 04m 24s |
Commit |
90fe852e03: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 michael-radency 🗃️ e2e/*
|
Committer | Michael Kret |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
0
|
Skipped |
0
|
Passing |
453
|
✅ All Cypress E2E specs passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should hold off merging this until @ShireenMissi confirms that this fixes the issue.
…custom-approval-operations-in-nodes-that-send-messages-race-fix
Summary
Moved clearing of active execution id after push handler
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-2127/node-keeps-on-loading
https://linear.app/n8n/issue/NODE-1480/custom-approval-operations-in-nodes-that-send-messages