-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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: Retain execution data between partial executions (new flow) #11828
base: master
Are you sure you want to change the base?
fix: Retain execution data between partial executions (new flow) #11828
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
@@ -259,7 +259,7 @@ export function useRunWorkflow(useRunWorkflowOpts: { router: ReturnType<typeof u | |||
executedNode, | |||
data: { | |||
resultData: { | |||
runData: newRunData ?? {}, | |||
runData: startRunData.runData ?? {}, |
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.
Fixes that previous run data disappears. This should be the same run data that is sent to the BE.
startRunData.runData
is either the run data the FE pruned (if the old flow is used) or the full run data from before (if the new flow is used).
'n8n-nodes-base.manualTrigger': { | ||
type: new ManualTrigger(), | ||
sourcePath: '', | ||
}, |
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 needed an actual trigger to test the runPartialWorkflow2
, especially the call to findTriggerForPartialExecution
.
The start node above is not marked as a trigger, so I added the manual trigger here.
@@ -206,7 +209,7 @@ describe('WorkflowExecute', () => { | |||
} | |||
}); | |||
|
|||
describe('WorkflowExecute, NodeExecutionOutput type test', () => { | |||
test('WorkflowExecute, NodeExecutionOutput type test', () => { |
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'm assuming this was supposed to be test
and not describe
.
Summary
The run data has been behaving weird with the new execution flow since the last update:
Before:
PAY-2255-before.mp4
But even before that it hasn't been working the way you'd expect, e.g. nodes after the destination node that had run data would keep their run data until the execution finished which means they would stay green until the end of the execution and only then turn gray.
All of this is related to moving the run data management for partial executions to the backend without accounting for a way to keep the FE state in sync with it.
This PR does 2 things to fix this:
deleteRunData
message so that the BE can selectively prune run data from the FE during partial executionsAfter:
PAY-2255-after.mp4
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-2255/the-fe-looses-all-run-data-between-runs-if-the-new-partial-execution
Review / Merge checklist
Docs updated or follow-up ticket created.release/backport
(if the PR is an urgent fix that needs to be backported)