fix(core): Stop binary data restoration from preventing execution from finishing #8082

krynble · 2023-12-19T08:32:26Z

In the case of a filesystem failure to rename the binary files as part of the execution's cleanup process, the execution would fail to be saved and would never finish. This catch prevents it.

Summary

Whenever an execution is wrapping u to save the data, if it uses binary data n8n will try to find possibly misallocated files and place them in the right folder. If this process fails, the execution fails to finish.

Given the execution has already finished at this point, and we cannot handle the binary data errors more gracefully, all we can do at this point is log the message as it's a filesystem issue. The rest of the execution saving process should remain as normal.

Related tickets and issues

https://linear.app/n8n/issue/HELP-430

Review / Merge checklist

PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
Docs updated or follow-up ticket created.
Tests included.

A bug is not considered fixed, unless a test is added to prevent it from happening again.
A feature is not complete without tests.

…inishing

packages/cli/src/WorkflowExecuteAdditionalData.ts

tomi · 2023-12-19T11:58:49Z

packages/cli/src/WorkflowExecuteAdditionalData.ts

+				try {
+					await restoreBinaryDataId(fullRunData, this.executionId, this.mode);
+				} catch (e) {
+					logger.debug('Failed to restore binary data ID', {


Would be nice to handle only the specific error here instead of everything.

Also, since this isn't really something that is expected, wouldn't we want to log this either as an error or warning?

Good idea, Tomi - changed the level to errror

Changed - looks good? Would you mind approving?

tomi · 2023-12-20T08:37:55Z

packages/cli/src/WorkflowExecuteAdditionalData.ts

+					logger.error('Failed to restore binary data ID', {
+						executionId: this.executionId,
+						mode: this.mode,
+					});


We are still ignoring the thrown error. Could we at least include it in the logging so we know what the underlying reason for the failure was?

@ivov added your suggestions and I rebased my branch to contain it

packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

tomi · 2023-12-20T09:46:46Z

packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts

-	const promises = Object.keys(runData).map(async (nodeName) => {
-		const binaryDataId = runData[nodeName]?.[0]?.data?.main?.[0]?.[0]?.binary?.data?.id;
+		const promises = Object.keys(runData).map(async (nodeName) => {
+			const binaryDataId = runData[nodeName]?.[0]?.data?.main?.[0]?.[0]?.binary?.data?.id;


I think it would make more sense to have the try catch inside the map function, as the tasks are independent of each others and any one or multiple of them could in theory throw. Otherwise if one of them throws the function will return even if the other promises are still going. Might as well extract it to its own function while at it.

Other option would be to use Promise.allSettled and check the result afterwards and log possible errors

TBH @tomi I did implement the change but given we have to then update the binary data id after the renaming process happens, it turned out so ugly by having to use then as well as allSettled in a .forEach pushing things to a promise array that I was weighing the pros and cons.

If we fail to rename some files even though this is independent, the impact is that you might end up with some files not being deleted when the execution is removed.

This is not great, but this also only happens if there's something else went wrong with the execution, where the binary file was not written to disk in the first place, so this is an edge case and the impact is minimal, so I went for code legibility.

tomi

👍

github-actions · 2023-12-21T08:31:57Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2023-12-21T08:31:59Z

1 flaky test on run #3438 ↗︎

0	306	5	0	1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 krynble 🗃️ e2e/*
Project: n8n	Commit: `ddc2051c6c`
Status: Passed	Duration: 46:45 💡
Started: Dec 21, 2023 8:51 AM	Ended: Dec 21, 2023 9:38 AM

cypress/e2e/27-two-factor-authentication.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Two-factor authentication > Should be able to login with MFA token		`Screenshots` `Video`

Review all test suite changes for PR #8082 ↗︎

github-actions · 2023-12-21T09:38:10Z

✅ All Cypress E2E specs passed

# [1.22.0](https://github.com/n8n-io/n8n/compare/n8n@1.21.0...n8n@1.22.0) (2023-12-21) ### Bug Fixes * **core:** Close db connection gracefully when exiting ([#8045](#8045)) ([e69707e](e69707e)) * **core:** Consider timeout in shutdown an error ([#8050](#8050)) ([4cae976](4cae976)) * **core:** Do not display error when stopping jobless execution in queue mode ([#8007](#8007)) ([8e6b951](8e6b951)) * **core:** Fix shutdown if terminating before hooks are initialized ([#8047](#8047)) ([6ae2f5e](6ae2f5e)) * **core:** Handle multiple termination signals correctly ([#8046](#8046)) ([67bd8ad](67bd8ad)) * **core:** Initialize queue once in queue mode ([#8025](#8025)) ([53c0b49](53c0b49)) * **core:** Prevent axios from force setting a form-urlencoded content-type ([#8117](#8117)) ([bba9576](bba9576)) * **core:** Remove circular references before serializing executions in public API ([#8043](#8043)) ([989888d](989888d)) * **core:** Restore workflow ID during execution creation ([#8031](#8031)) ([c5e6ba8](c5e6ba8)) * **core:** Use relative imports for dynamic imports in SecurityAuditService ([#8086](#8086)) ([785bf99](785bf99)) * **core:** Stop binary data restoration from preventing execution from finishing ([#8082](#8082)) ([5ffff1b](5ffff1b)) * **editor:** Add back credential `use` permission ([#8023](#8023)) ([329e5bf](329e5bf)) * **editor:** Cleanup Executions page component ([#8053](#8053)) ([2689c37](2689c37)) * **editor:** Disable auto scroll and list size check when clicking on executions ([#7983](#7983)) ([fcb8b91](fcb8b91)) * **editor:** Ensure execution data overrides pinned data when copying in executions view ([#8009](#8009)) ([1d1cb0d](1d1cb0d)) * **editor:** Fix copy/paste issue when switch node is in workflow ([#8103](#8103)) ([4b86926](4b86926)) * **editor:** Make keyboard shortcuts more strict; don't accept extra Ctrl/Alt/Shift keys ([#8024](#8024)) ([8df49e1](8df49e1)) * **editor:** Show credential share info only to appropriate users ([#8020](#8020)) ([b29b4d4](b29b4d4)) * **editor:** Turn off executions list auto-refresh after leaving the page ([#8005](#8005)) ([e3c363d](e3c363d)) * **editor:** Update image sizes in template description not to be full width always ([#8037](#8037)) ([63a6e7e](63a6e7e)) * **ActiveCampaign Node:** Fix pagination issue when loading tags ([#8017](#8017)) ([1943857](1943857)) * **HTTP Request Node:** Do not create circular references in HTTP request node output ([#8030](#8030)) ([5b7ea16](5b7ea16)) * Upgrade axios to address CVE-2023-45857 ([#7713](#7713)) ([64eb9bb](64eb9bb)) ### Features * Add option to `returnIntermediateSteps` for AI agents ([#8113](#8113)) ([7806a65](7806a65)) * **core:** Add config option to prefer GET request over LIST when using Hashicorp Vault ([#8049](#8049)) ([439a22d](439a22d)) * **core:** Add N8N_GRACEFUL_SHUTDOWN_TIMEOUT env var ([#8068](#8068)) ([614f488](614f488)) * **editor:** Add lead enrichment suggestions to workflow list ([#8042](#8042)) ([36a923c](36a923c)) * **editor:** Finalize workers view ([#8052](#8052)) ([edfa784](edfa784)) * **editor:** Gracefully ignore invalid payloads in postMessage handler ([#8096](#8096)) ([9d22c7a](9d22c7a)) * **editor:** Upgrade frontend tooling to address a few vulnerabilities ([#8100](#8100)) ([19b7f1f](19b7f1f)) * **Filter Node:** Overhaul UI by adding the new filter component ([#8016](#8016)) ([3d53052](3d53052)) * **Respond to Webhook Node:** Overhaul with improvements like returning all items ([#8093](#8093)) ([32d397e](32d397e)) ### Performance Improvements * **editor:** Improve canvas rendering performance ([#8022](#8022)) ([b780436](b780436)) Co-authored-by: ivov <ivov@users.noreply.github.com>

janober · 2023-12-21T13:25:49Z

Got released with n8n@1.22.0

fix: Prevent binary data restoration from preventing execution from f…

5e632ca

…inishing

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 19, 2023

ivov reviewed Dec 19, 2023

View reviewed changes

packages/cli/src/WorkflowExecuteAdditionalData.ts Outdated Show resolved Hide resolved

tomi reviewed Dec 19, 2023

View reviewed changes

ivov added a commit that referenced this pull request Dec 19, 2023

refactor(core): Suggestions for #8082

bf2bd22

refactor: Change logging level to error

a7898e6

krynble changed the title ~~fix: Prevent binary data restoration from preventing execution from finishing~~ fix: Stop binary data restoration from preventing execution from finishing Dec 19, 2023

tomi reviewed Dec 20, 2023

View reviewed changes

refactor(core): Suggestions for #8082

944f6c6

ivov reviewed Dec 20, 2023

View reviewed changes

packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts Outdated Show resolved Hide resolved

Update packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts

66b8ef2

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

tomi reviewed Dec 20, 2023

View reviewed changes

fix: Lint issue

ddc2051

tomi approved these changes Dec 21, 2023

View reviewed changes

krynble merged commit 5ffff1b into master Dec 21, 2023
19 checks passed

krynble deleted the help-430 branch December 21, 2023 09:40

github-actions bot mentioned this pull request Dec 21, 2023

🚀 Release 1.22.0 #8115

Merged

netroy changed the title ~~fix: Stop binary data restoration from preventing execution from finishing~~ fix(core): Stop binary data restoration from preventing execution from finishing Dec 21, 2023

janober added the Released label Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Stop binary data restoration from preventing execution from finishing #8082

fix(core): Stop binary data restoration from preventing execution from finishing #8082

krynble commented Dec 19, 2023

tomi Dec 19, 2023

krynble Dec 19, 2023

krynble Dec 19, 2023

tomi Dec 20, 2023

krynble Dec 20, 2023

tomi Dec 20, 2023

krynble Dec 20, 2023

tomi left a comment

github-actions bot commented Dec 21, 2023

cypress bot commented Dec 21, 2023 •

edited

Loading

github-actions bot commented Dec 21, 2023

janober commented Dec 21, 2023

fix(core): Stop binary data restoration from preventing execution from finishing #8082

fix(core): Stop binary data restoration from preventing execution from finishing #8082

Conversation

krynble commented Dec 19, 2023

Summary

Related tickets and issues

Review / Merge checklist

tomi Dec 19, 2023

Choose a reason for hiding this comment

krynble Dec 19, 2023

Choose a reason for hiding this comment

krynble Dec 19, 2023

Choose a reason for hiding this comment

tomi Dec 20, 2023

Choose a reason for hiding this comment

krynble Dec 20, 2023

Choose a reason for hiding this comment

tomi Dec 20, 2023

Choose a reason for hiding this comment

krynble Dec 20, 2023

Choose a reason for hiding this comment

tomi left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2023

cypress bot commented Dec 21, 2023 • edited Loading

1 flaky test on run #3438 ↗︎

Review all test suite changes for PR #8082 ↗︎

github-actions bot commented Dec 21, 2023

janober commented Dec 21, 2023

cypress bot commented Dec 21, 2023 •

edited

Loading