fix(core): Reduce memory consumption on `BinaryDataManager.init` #6633

netroy · 2023-07-10T14:27:04Z

When there are a few thousand binary data file to delete, the deleteMarkedFiles and deleteMarkedPersistedFiles methods need a lot of memory to process these files, irrespective of if these files have any data or not.

Fixes https://linear.app/n8n/issue/HELP-219

github-actions · 2023-07-10T14:27:40Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

krynble · 2023-07-10T16:06:00Z

packages/core/src/BinaryDataManager/FileSystem.ts

+		const fileNames = await fs.readdir(this.storagePath);
+		const deletedIds = [];
+		for (const fileName of fileNames) {
+			const executionId = fileName.match(executionExtractionRegexp)?.[1];


Is it possible that the regexp also matches the .metadata that is the extension as part of this? If so, it'd cause the deletion below to fail as it'd try to delete something.metadata.metadata

the regexp has a $ after the uuid matching, so it should not be matching the metadata file names.

krynble

LGTM - it looks tome that the main change is simply removing deleteMetaFileByPath to using glob to find all files that needed to be removed. Is that right?

codecov · 2023-07-10T16:24:58Z

Codecov Report

Patch coverage: 54.90% and project coverage change: -0.07 ⚠️

Comparison is base (d6b2ae0) 28.93% compared to head (751ded4) 28.87%.

❗ Current head 751ded4 differs from pull request most recent head 35dd6e3. Consider uploading reports for the commit 35dd6e3 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6633      +/-   ##
==========================================
- Coverage   28.93%   28.87%   -0.07%     
==========================================
  Files        3071     3069       -2     
  Lines      188445   188422      -23     
  Branches    20893    20893              
==========================================
- Hits        54532    54409     -123     
- Misses     133033   133131      +98     
- Partials      880      882       +2

Impacted Files	Coverage Δ
packages/core/src/BinaryDataManager/index.ts	`36.06% <0.00%> (ø)`
...src/databases/repositories/execution.repository.ts	`25.56% <25.00%> (+0.19%)`	⬆️
packages/core/src/BinaryDataManager/FileSystem.ts	`62.12% <59.09%> (+8.73%)`	⬆️
...icApi/v1/handlers/executions/executions.handler.ts	`78.26% <100.00%> (ø)`

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-07-10T16:28:42Z

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

cypress · 2023-07-10T16:28:49Z

Passing run #1425 ↗︎

0	219	0	0	0

Details:

🌳 HELP-219-fix-binary-data-pruning 🖥️ browsers:node18.12.0-chrome107 🤖 netroy...
Project: n8n	Commit: `35dd6e39da`
Status: Passed	Duration: 07:13 💡
Started: Jul 12, 2023 7:46 AM	Ended: Jul 12, 2023 7:53 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2023-07-10T17:29:03Z

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

When there are a few thousand binary data file to delete, the `deleteMarkedFiles` and `deleteMarkedPersistedFiles` methods need a lot of memory to process these files, irrespective of if these files have any data or not.

krynble

LGTM

github-actions · 2023-07-12T07:53:47Z

✅ All Cypress E2E specs passed

fix(core): Reduce memory consumption on BinaryDataManager.init When there are a few thousand binary data file to delete, the `deleteMarkedFiles` and `deleteMarkedPersistedFiles` methods need a lot of memory to process these files, irrespective of if these files have any data or not.

janober · 2023-07-12T12:40:38Z

Got released with n8n@0.236.1

fix(core): Reduce memory consumption on BinaryDataManager.init When there are a few thousand binary data file to delete, the `deleteMarkedFiles` and `deleteMarkedPersistedFiles` methods need a lot of memory to process these files, irrespective of if these files have any data or not.

* master: ci!: Drop support for Node.js 16. Add initial support for Node.js 20 (#6649) feat(API): Implement users account quota guards (#6434) feat(OpenAI Node): Update max token limit to support newer model limits (#6644) ci: Run checklist job as a separate workflow (no-changelog) (#6645) fix(HTTP Request Node): Cleanup circular references in response (#6590) fix(Postgres Node): Upsert does not fetch columns when schema other then public (#6643) fix(core): Update docker compose setup for V1 images (#6642) refactor: Delete unnecessary interface re-exports from core, and delete unused code in nodes-base (no-changelog) (#6631) fix(core): Reduce memory consumption on `BinaryDataManager.init` (#6633) fix(editor): Implement simple view for templates experiment (no-changelog) (#6637) # Conflicts: # packages/editor-ui/src/stores/settings.store.ts

* ADO-814-trial-banner: ✔️ Updated callout component snapshots 👌 Moving DOM logic from the store 👌 Addressing more points from code review 👌 Refactoring trial banner component and ui store 👌 Updating permanent banner dismiss endpoint and back-end logic ⚡ Updating the migrations syntax ci!: Drop support for Node.js 16. Add initial support for Node.js 20 (#6649) feat(API): Implement users account quota guards (#6434) feat(OpenAI Node): Update max token limit to support newer model limits (#6644) ci: Run checklist job as a separate workflow (no-changelog) (#6645) fix(HTTP Request Node): Cleanup circular references in response (#6590) fix(Postgres Node): Upsert does not fetch columns when schema other then public (#6643) fix(core): Update docker compose setup for V1 images (#6642) refactor: Delete unnecessary interface re-exports from core, and delete unused code in nodes-base (no-changelog) (#6631) fix(core): Reduce memory consumption on `BinaryDataManager.init` (#6633) ⚡ Updating upgrade plan button click 👌 Addressing design review feedback ⚡ Using query parameters in migrations fix(editor): Implement simple view for templates experiment (no-changelog) (#6637) ⚡ Updating migration files

netroy requested a review from ahsanv July 10, 2023 14:27

netroy changed the title ~~fix(core): Reduce memory consumption on BinaryDataManager.init~~ fix(core): Reduce memory consumption on BinaryDataManager.init Jul 10, 2023

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

netroy force-pushed the HELP-219-fix-binary-data-pruning branch from f642abc to 751ded4 Compare July 10, 2023 14:36

krynble reviewed Jul 10, 2023

View reviewed changes

krynble approved these changes Jul 10, 2023

View reviewed changes

fix(core): Reduce memory consumption on BinaryDataManager.init

35dd6e3

When there are a few thousand binary data file to delete, the `deleteMarkedFiles` and `deleteMarkedPersistedFiles` methods need a lot of memory to process these files, irrespective of if these files have any data or not.

netroy force-pushed the HELP-219-fix-binary-data-pruning branch from 751ded4 to 35dd6e3 Compare July 11, 2023 09:05

netroy requested a review from krynble July 11, 2023 17:41

krynble approved these changes Jul 12, 2023

View reviewed changes

netroy merged commit 329d22f into master Jul 12, 2023
18 of 19 checks passed

netroy deleted the HELP-219-fix-binary-data-pruning branch July 12, 2023 08:08

github-actions bot mentioned this pull request Jul 12, 2023

🚀 Release 0.236.1 #6647

Merged

janober added the Released label Jul 12, 2023

github-actions bot mentioned this pull request Jul 12, 2023

🚀 Release 1.0.2 #6651

Merged

github-actions bot mentioned this pull request Jul 26, 2023

🚀 Release 1.1.0 #6746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Reduce memory consumption on `BinaryDataManager.init` #6633

fix(core): Reduce memory consumption on `BinaryDataManager.init` #6633

netroy commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

krynble Jul 10, 2023 •

edited

Loading

netroy Jul 10, 2023

krynble left a comment

codecov bot commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023

cypress bot commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023

krynble left a comment

github-actions bot commented Jul 12, 2023

janober commented Jul 12, 2023

fix(core): Reduce memory consumption on BinaryDataManager.init #6633

fix(core): Reduce memory consumption on BinaryDataManager.init #6633

Conversation

netroy commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

krynble Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

netroy Jul 10, 2023

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2023 • edited Loading

Codecov Report

github-actions bot commented Jul 10, 2023

cypress bot commented Jul 10, 2023 • edited Loading

Passing run #1425 ↗︎

github-actions bot commented Jul 10, 2023

krynble left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 12, 2023

janober commented Jul 12, 2023

fix(core): Reduce memory consumption on `BinaryDataManager.init` #6633

fix(core): Reduce memory consumption on `BinaryDataManager.init` #6633

krynble Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 10, 2023 •

edited

Loading

cypress bot commented Jul 10, 2023 •

edited

Loading