-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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(cli): Fix issue with n8n crashing when error in poll method #4008
Conversation
}; | ||
|
||
const execution = ResponseHelper.flattenExecutionData(fullExecutionData); | ||
|
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.
@krynble Should we check the workflow settings to check whether save error execution is enabled
before saving the error execution?
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.
Hum that's a good point. Now thinking about alternatives, we have what we call "Activation errors".
You can see in the packages/cli/src/ActiveWorkflowRunner.ts
file specifically inside add
function, we try to activate a workflow and if it fails, we deactivate it and add an error the user can see when opening the workflows list.
Instead of generating a failed execution, what if we deactivate the workflow and display this error?
On the other hand, this is bad because we'll deactivate the workflow even if the service is temporarily down and a next poll request would work just fine.
Maybe the current approach you implemented is best for debuggability and failed executions are saved by default, so we should check this flag, yes, and only save if it's checked.
We just need to make sure that WorkflowData
that we received was actually populated with this, because I've seen situations where this did not happen.
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.
Yeah, I think this is a much better approach. For example, in the Airtable case, the issue is generated by changing the trigger column in Airtable (Workflow does not have to be touched). In case this happens, with this approach, the user will have an issue in the error tab or will be notified via the error workflow, then look at the error, and finally adjust in Airtable without having to touch the workflow. The other approach will be an extra step having to activate the workflow again, and if we do not save the execution plus they do not have an error workflow setup, they will never notice the error.
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 |
…io#4008) * 🐛 Fix issue with n8n crashing when error in poll method * Remove unnecessary imports and add async property * Remove unnecessary imports * ⚡ Move createErrorExecution to genericHelper * ⚡ Improvements Co-authored-by: Omar Ajoue <krynble@gmail.com>
N8N-4272