From 8cd4db0ab7893097825261c1bdbb0e0198150572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 31 Aug 2023 16:02:20 +0200 Subject: [PATCH] refactor(core): Simplify marking logic in binary data manager (no-changelog) (#7046) - For a saved execution, we write to disk binary data and metadata. These two are only ever deleted via `POST /executions/delete`. No marker file, so untouched by pruning. - For an unsaved execution, we write to disk binary data, binary data metadata, and a marker file at `/meta`. We later delete all three during pruning. - The third flow is legacy. Currently, if the execution is unsaved, we actually store it in the DB while running the workflow and immediately after the workflow is finished during the `onWorkflowPostExecute()` hook we delete that execution, so the second flow applies. But formerly, we did not store unsaved executions in the DB ("ephemeral executions") and so we needed to write a marker file at `/persistMeta` so that, if the ephemeral execution crashed after the step where binary data was stored, we had a way to later delete its associated dangling binary data via a second pruning cycle, and if the ephemeral execution succeeded, then we immediately cleaned up the marker file at `/persistMeta` during the `onWorkflowPostExecute()` hook. This creation and cleanup at `/persistMeta` is still happening, but this third flow no longer has a purpose, as we now store unsaved executions in the DB and delete them immediately after. Hence the third flow can be removed. --- packages/cli/src/InternalHooks.ts | 3 -- packages/cli/src/config/schema.ts | 6 --- .../core/src/BinaryDataManager/FileSystem.ts | 53 ------------------- packages/core/src/BinaryDataManager/index.ts | 6 --- packages/core/src/Interfaces.ts | 2 - .../core/test/NodeExecuteFunctions.test.ts | 2 - packages/nodes-base/test/nodes/Helpers.ts | 1 - 7 files changed, 73 deletions(-) diff --git a/packages/cli/src/InternalHooks.ts b/packages/cli/src/InternalHooks.ts index a099d6fd7..de41f61fe 100644 --- a/packages/cli/src/InternalHooks.ts +++ b/packages/cli/src/InternalHooks.ts @@ -1,6 +1,5 @@ import { Service } from 'typedi'; import { snakeCase } from 'change-case'; -import { BinaryDataManager } from 'n8n-core'; import type { AuthenticationMethod, ExecutionStatus, @@ -461,8 +460,6 @@ export class InternalHooks implements IInternalHooksClass { }), ); - await BinaryDataManager.getInstance().persistBinaryDataForExecutionId(executionId); - void Promise.all([...promises, this.telemetry.trackWorkflowExecution(properties)]); } diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 742407853..157e9c2a9 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -913,12 +913,6 @@ export const schema = { env: 'N8N_BINARY_DATA_TTL', doc: 'TTL for binary data of unsaved executions in minutes', }, - persistedBinaryDataTTL: { - format: Number, - default: 1440, - env: 'N8N_PERSISTED_BINARY_DATA_TTL', - doc: 'TTL for persisted binary data in minutes (binary data gets deleted if not persisted before TTL expires)', - }, }, deployment: { diff --git a/packages/core/src/BinaryDataManager/FileSystem.ts b/packages/core/src/BinaryDataManager/FileSystem.ts index e84e47947..cf9f1d94d 100644 --- a/packages/core/src/BinaryDataManager/FileSystem.ts +++ b/packages/core/src/BinaryDataManager/FileSystem.ts @@ -11,7 +11,6 @@ import type { IBinaryDataConfig, IBinaryDataManager } from '../Interfaces'; import { FileNotFoundError } from '../errors'; const PREFIX_METAFILE = 'binarymeta'; -const PREFIX_PERSISTED_METAFILE = 'persistedmeta'; const executionExtractionRegexp = /^(\w+)(?:[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/; @@ -21,12 +20,9 @@ export class BinaryDataFileSystem implements IBinaryDataManager { private binaryDataTTL: number; - private persistedBinaryDataTTL: number; - constructor(config: IBinaryDataConfig) { this.storagePath = config.localStoragePath; this.binaryDataTTL = config.binaryDataTTL; - this.persistedBinaryDataTTL = config.persistedBinaryDataTTL; } async init(startPurger = false): Promise { @@ -34,18 +30,12 @@ export class BinaryDataFileSystem implements IBinaryDataManager { setInterval(async () => { await this.deleteMarkedFiles(); }, this.binaryDataTTL * 30000); - - setInterval(async () => { - await this.deleteMarkedPersistedFiles(); - }, this.persistedBinaryDataTTL * 30000); } await this.assertFolder(this.storagePath); await this.assertFolder(this.getBinaryDataMetaPath()); - await this.assertFolder(this.getBinaryDataPersistMetaPath()); await this.deleteMarkedFiles(); - await this.deleteMarkedPersistedFiles(); } async getFileSize(identifier: string): Promise { @@ -55,7 +45,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager { async copyBinaryFile(filePath: string, executionId: string): Promise { const binaryDataId = this.generateFileName(executionId); - await this.addBinaryIdToPersistMeta(executionId, binaryDataId); await this.copyFileToLocalStorage(filePath, binaryDataId); return binaryDataId; } @@ -72,7 +61,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager { async storeBinaryData(binaryData: Buffer | Readable, executionId: string): Promise { const binaryDataId = this.generateFileName(executionId); - await this.addBinaryIdToPersistMeta(executionId, binaryDataId); await this.saveToLocalStorage(binaryData, binaryDataId); return binaryDataId; } @@ -105,30 +93,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager { return this.deleteMarkedFilesByMeta(this.getBinaryDataMetaPath(), PREFIX_METAFILE); } - async deleteMarkedPersistedFiles(): Promise { - return this.deleteMarkedFilesByMeta( - this.getBinaryDataPersistMetaPath(), - PREFIX_PERSISTED_METAFILE, - ); - } - - private async addBinaryIdToPersistMeta(executionId: string, identifier: string): Promise { - const currentTime = new Date().getTime(); - const timeAtNextHour = currentTime + 3600000 - (currentTime % 3600000); - const timeoutTime = timeAtNextHour + this.persistedBinaryDataTTL * 60000; - - const filePath = this.resolveStoragePath( - 'persistMeta', - `${PREFIX_PERSISTED_METAFILE}_${executionId}_${timeoutTime}`, - ); - - try { - await fs.access(filePath); - } catch { - await fs.writeFile(filePath, identifier); - } - } - private async deleteMarkedFilesByMeta(metaPath: string, filePrefix: string): Promise { const currentTimeValue = new Date().valueOf(); const metaFileNames = await glob(`${filePrefix}_*`, { cwd: metaPath }); @@ -184,19 +148,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager { return this.deleteFromLocalStorage(identifier); } - async persistBinaryDataForExecutionId(executionId: string): Promise { - const metaFiles = await fs.readdir(this.getBinaryDataPersistMetaPath()); - const promises = metaFiles.reduce>>((prev, curr) => { - if (curr.startsWith(`${PREFIX_PERSISTED_METAFILE}_${executionId}_`)) { - prev.push(fs.rm(path.join(this.getBinaryDataPersistMetaPath(), curr))); - return prev; - } - - return prev; - }, []); - await Promise.all(promises); - } - private async assertFolder(folder: string): Promise { try { await fs.access(folder); @@ -213,10 +164,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager { return path.join(this.storagePath, 'meta'); } - private getBinaryDataPersistMetaPath() { - return path.join(this.storagePath, 'persistMeta'); - } - private async deleteFromLocalStorage(identifier: string) { return fs.rm(this.getBinaryPath(identifier)); } diff --git a/packages/core/src/BinaryDataManager/index.ts b/packages/core/src/BinaryDataManager/index.ts index 46d0f446f..7bfbf614c 100644 --- a/packages/core/src/BinaryDataManager/index.ts +++ b/packages/core/src/BinaryDataManager/index.ts @@ -172,12 +172,6 @@ export class BinaryDataManager { } } - async persistBinaryDataForExecutionId(executionId: string): Promise { - if (this.managers[this.binaryDataMode]) { - await this.managers[this.binaryDataMode].persistBinaryDataForExecutionId(executionId); - } - } - async deleteBinaryDataByExecutionIds(executionIds: string[]): Promise { if (this.managers[this.binaryDataMode]) { await this.managers[this.binaryDataMode].deleteBinaryDataByExecutionIds(executionIds); diff --git a/packages/core/src/Interfaces.ts b/packages/core/src/Interfaces.ts index 538a1127c..d01b655bf 100644 --- a/packages/core/src/Interfaces.ts +++ b/packages/core/src/Interfaces.ts @@ -39,7 +39,6 @@ export interface IBinaryDataConfig { availableModes: string; localStoragePath: string; binaryDataTTL: number; - persistedBinaryDataTTL: number; } export interface IBinaryDataManager { @@ -57,7 +56,6 @@ export interface IBinaryDataManager { deleteBinaryDataByIdentifier(identifier: string): Promise; duplicateBinaryDataByIdentifier(binaryDataId: string, prefix: string): Promise; deleteBinaryDataByExecutionIds(executionIds: string[]): Promise; - persistBinaryDataForExecutionId(executionId: string): Promise; } export namespace n8n { diff --git a/packages/core/test/NodeExecuteFunctions.test.ts b/packages/core/test/NodeExecuteFunctions.test.ts index d16bcf1a6..d981c1d24 100644 --- a/packages/core/test/NodeExecuteFunctions.test.ts +++ b/packages/core/test/NodeExecuteFunctions.test.ts @@ -35,7 +35,6 @@ describe('NodeExecuteFunctions', () => { availableModes: 'default', localStoragePath: temporaryDir, binaryDataTTL: 1, - persistedBinaryDataTTL: 1, }); // Set our binary data buffer @@ -86,7 +85,6 @@ describe('NodeExecuteFunctions', () => { availableModes: 'filesystem', localStoragePath: temporaryDir, binaryDataTTL: 1, - persistedBinaryDataTTL: 1, }); // Set our binary data buffer diff --git a/packages/nodes-base/test/nodes/Helpers.ts b/packages/nodes-base/test/nodes/Helpers.ts index 5e3e4a0c7..ecc87dbb5 100644 --- a/packages/nodes-base/test/nodes/Helpers.ts +++ b/packages/nodes-base/test/nodes/Helpers.ts @@ -223,7 +223,6 @@ export async function initBinaryDataManager(mode: 'default' | 'filesystem' = 'de availableModes: mode, localStoragePath: temporaryDir, binaryDataTTL: 1, - persistedBinaryDataTTL: 1, }); return temporaryDir; }