-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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): Do not execute workflowExecuteBefore
hook when resuming executions from a waiting state
#5727
Conversation
workflowExecuteBefore
hook when resuming executions from a waiting stateworkflowExecuteBefore
hook when resuming executions from a waiting state
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Make sure to check off this list before asking for review. |
packages/core/src/WorkflowExecute.ts
Outdated
@@ -746,7 +746,10 @@ export class WorkflowExecute { | |||
|
|||
const returnPromise = (async () => { | |||
try { | |||
await this.executeHook('workflowExecuteBefore', [workflow]); | |||
const { executionId, restartExecutionId } = this.additionalData; | |||
if (restartExecutionId !== executionId) { |
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.
Isn't it enough to simply check if restartExecutionId
is not undefined?
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.
LGTM! Ideally we would place it in WorkflowExecuteAdditionalData
but there is some context missing there, so this is the place.
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.
I just realized that this also skips the workflowExecuteBefore
hook that sends the executionStarted
push message to the frontend.
But, since when the execution resumes, no sessionId
is getting passed down to that hook, which is also skiping sending the push message.
@krynble Should we move the restartExecutionId
to the WorkflowHooks
class instead?
We'd still need to fix the issue of the missing sessionId
, but that can be a separate PR.
…executions from a waiting state
056f853
to
2c4fc6b
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5727 +/- ##
=======================================
Coverage 17.52% 17.52%
=======================================
Files 2495 2495
Lines 114289 114293 +4
Branches 17852 17853 +1
=======================================
+ Hits 20027 20028 +1
- Misses 93671 93674 +3
Partials 591 591
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
LGTM
Got released with |
…executions from a waiting state (n8n-io#5727)
Fixes CP-344