Skip to content

Commit

Permalink
fix: Stop binary data restoration from preventing execution from fini…
Browse files Browse the repository at this point in the history
…shing (#8082)

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](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md))
- [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) 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.

---------

Co-authored-by: Iván Ovejero <[email protected]>
  • Loading branch information
krynble and ivov authored Dec 21, 2023
1 parent 7806a65 commit 5ffff1b
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 14 deletions.
44 changes: 30 additions & 14 deletions packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { BinaryDataService } from 'n8n-core';
import type { IRun, WorkflowExecuteMode } from 'n8n-workflow';
import type { BinaryData } from 'n8n-core';
import config from '@/config';
import { Logger } from '@/Logger';

/**
* Whenever the execution ID is not available to the binary data service at the
Expand Down Expand Up @@ -32,28 +33,43 @@ export async function restoreBinaryDataId(
return;
}

const { runData } = run.data.resultData;
try {
const { runData } = run.data.resultData;

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;

if (!binaryDataId) return;
if (!binaryDataId) return;

const [mode, fileId] = binaryDataId.split(':') as [BinaryData.StoredMode, string];
const [mode, fileId] = binaryDataId.split(':') as [BinaryData.StoredMode, string];

const isMissingExecutionId = fileId.includes('/temp/');
const isMissingExecutionId = fileId.includes('/temp/');

if (!isMissingExecutionId) return;
if (!isMissingExecutionId) return;

const correctFileId = fileId.replace('temp', executionId);
const correctFileId = fileId.replace('temp', executionId);

await Container.get(BinaryDataService).rename(fileId, correctFileId);
await Container.get(BinaryDataService).rename(fileId, correctFileId);

const correctBinaryDataId = `${mode}:${correctFileId}`;
const correctBinaryDataId = `${mode}:${correctFileId}`;

// @ts-expect-error Validated at the top
run.data.resultData.runData[nodeName][0].data.main[0][0].binary.data.id = correctBinaryDataId;
});
// @ts-expect-error Validated at the top
run.data.resultData.runData[nodeName][0].data.main[0][0].binary.data.id = correctBinaryDataId;
});

await Promise.all(promises);
await Promise.all(promises);
} catch (e) {
const error = e instanceof Error ? e : new Error(`${e}`);
const logger = Container.get(Logger);

if (error.message.includes('ENOENT')) {
logger.warn('Failed to restore binary data ID - No such file or dir', {
executionId,
error,
});
return;
}

logger.error('Failed to restore binary data ID - Unknown error', { executionId, error });
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,28 @@ for (const mode of ['filesystem-v2', 's3'] as const) {

expect(binaryDataService.rename).not.toHaveBeenCalled();
});

it('should ignore error thrown on renaming', async () => {
const workflowId = '6HYhhKmJch2cYxGj';
const executionId = 'temp';
const binaryDataFileUuid = 'a5c3f1ed-9d59-4155-bc68-9a370b3c51f6';

const incorrectFileId = `workflows/${workflowId}/executions/temp/binary_data/${binaryDataFileUuid}`;

const run = toIRun({
binary: {
data: { id: `s3:${incorrectFileId}` },
},
});

binaryDataService.rename.mockRejectedValueOnce(new Error('ENOENT'));

const promise = restoreBinaryDataId(run, executionId, 'webhook');

await expect(promise).resolves.not.toThrow();

expect(binaryDataService.rename).toHaveBeenCalled();
});
});
}

Expand Down

0 comments on commit 5ffff1b

Please sign in to comment.