refactor(core): Implement soft-deletions for executions #7092
Conversation
|
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Make sure to check off this list before asking for review. |
n8n-assistant
bot
added
core
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #7092 +/- ##
==========================================
+ Coverage 32.65% 32.70% +0.04%
==========================================
Files 3340 3340
Lines 199463 199435 -28
Branches 21845 21844 -1
==========================================
+ Hits 65132 65219 +87
+ Misses 133251 133125 -126
- Partials 1080 1091 +11
☔ View full report in Codecov by Sentry. |
packages/cli/src/databases/repositories/execution.repository.ts
Outdated
Show resolved
Hide resolved
Passing run #2225 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
| @@ -49,7 +50,7 @@ export class ExecutionEntity { | |||
| @Column({ type: datetimeColumnType, nullable: true }) | |||
| stoppedAt: Date; | |||
|
|
|||
| @Column(datetimeColumnType) | |||
| @DeleteDateColumn({ type: datetimeColumnType, nullable: true }) | |||
There was a problem hiding this comment.
are we really benefiting from this decorator?
There was a problem hiding this comment.
@DeleteDateColumn is what enables softDelete to work, which is used in many places:
| @@ -49,7 +50,7 @@ export class ExecutionEntity { | |||
| @Column({ type: datetimeColumnType, nullable: true }) | |||
| stoppedAt: Date; | |||
|
|
|||
| @Column(datetimeColumnType) | |||
| @DeleteDateColumn({ type: datetimeColumnType, nullable: true }) | |||
| deletedAt: Date; | |||
There was a problem hiding this comment.
should this be
| deletedAt: Date; | |
| deletedAt?: Date; |
There was a problem hiding this comment.
Not 100% on this, but I logged on startup webhook entities (which have webhookId and pathLength which are nullable and marked with ?) and they show up as null when missing:
> [email protected] start /Users/ivov/Development/n8n
> run-script-os
> [email protected] start:default
> cd packages/cli/bin && ./n8n
n8n ready on 0.0.0.0, port 5678
Initializing n8n process
Version: 1.7.0
================================
Start Active Workflows:
================================
- My workflow 21 (ID: HDemWLl4i60bkGsm)
allWebhooks [
WebhookEntity {
workflowId: 'HDemWLl4i60bkGsm',
webhookPath: '9347adb3-289c-4508-8705-531575ac3e60',
method: 'GET',
node: 'Webhook',
webhookId: null,
pathLength: null
}
]
So if anything no properties that are nullable columns should have ? right? (Always present, but may be null)
| } | ||
|
|
||
| setPruningInterval() { | ||
| setInterval(async () => this.pruneBySoftDeleting(), 1 * TIME.HOUR); |
There was a problem hiding this comment.
I think we should add a global AbortController/AbortSignal for the shutdown process, so that we can clean up timers like these properly at shutdown.
There was a problem hiding this comment.
Clearing the timers on shutdown at abe6d9d, to keep scope small.
|
✅ All Cypress E2E specs passed |
| this.setHardDeletionInterval(); | ||
| } | ||
|
|
||
| setPruningInterval() { | ||
| setInterval(async () => this.pruneBySoftDeleting(), 1 * TIME.HOUR); |
There was a problem hiding this comment.
These timers are going to run on every container. Shouldn't we add some checks to run these only on the main container?
There was a problem hiding this comment.
Ah this is a great point, we have something kinda similar that was merged by @flipswitchingmonkey yesterday for things that must run only on the main process.
There was a problem hiding this comment.
Thank you, please see 2682b3d
The column `deletedAt` is marked `@DeleteDateColumn()` so all reads from the repository automatically add a `WHERE` clause checking that the column `IS NULL`.
|
✅ All Cypress E2E specs passed |
* master: refactor(core): Implement soft-deletions for executions (#7092) docs(Google Sheets Node): Operations naming update (no-changelog) (#7211) fix(core): Make parsing of content-type and content-disposition headers more flexible (#7217) fix(HTML Node): Add pairedItem support for 'Convert to HTML Table' operation (#7196) fix(HTTP Request Node): Decrease default timeout to 5min (#7177) feat(Linear Node): Add support for OAuth2 (#7201) refactor: Ignore large-scale revisions (no-changelog) (#7210) fix(core): Resolve domains to IPv4 first (#7206) ci: Fix tests failing on MySQL (no-changelog) (#7208) feat(Set Node): Overhaul (#6348) fix: Attempt license renewal when n8n starts (no-changelog) (#7204)
…g) (#7164) Depends on: #7092 | Story: [PAY-768](https://linear.app/n8n/issue/PAY-768) This PR: - Generalizes the `IBinaryDataManager` interface. - Adjusts `Filesystem.ts` to satisfy the interface. - Sets up an S3 client stub to be filled in in the next PR. - Turns `BinaryDataManager` into an injectable service. - Adjusts the config schema and adds new validators. Note that the PR looks large but all the main changes are in `packages/core/src/binaryData`. Out of scope: - `BinaryDataManager` (now `BinaryDataService`) and `Filesystem.ts` (now `fs.client.ts`) were slightly refactored for maintainability, but fully overhauling them is **not** the focus of this PR, which is meant to clear the way for the S3 implementation. Future improvements for these two should include setting up a backwards-compatible dir structure that makes it easier to locate binary data files to delete, removing duplication, simplifying cloning methods, using integers for binary data size instead of `prettyBytes()`, writing tests for existing binary data logic, etc. --------- Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <[email protected]>
|
Got released with |
Based on #7065 | Story: https://linear.app/n8n/issue/PAY-771
n8n on filesystem mode marks binary data to delete on manual execution deletion, on unsaved execution completion, and on every execution pruning cycle. We later prune binary data in a separate cycle via these marker files, based on the configured TTL. In the context of introducing an S3 client to manage binary data, the filesystem mode's mark-and-prune setup is too tightly coupled to the general binary data management client interface.
This PR...
execution.repository.ts. This keeps related logic in a single place.Out of scope: