fix(core): Disallow orphan executions #7069

ivov · 2023-09-01T08:24:51Z

Until #7061 we had an edge case where a manual unsaved workflow when run creates an orphan execution, i.e. a saved execution not pointing to any workflow. This execution is only ever visible to the instance owner (even if triggered by a member), and is wrongly stored as unfinished and crashed. This PR enforces that the DB disallows any such executions from making it into the DB.

This is needed also for the S3 client, which will include the workflowId in the path-like filename.

Story: https://linear.app/n8n/issue/PAY-766/disallow-orphan-executions

github-actions · 2023-09-01T08:25:07Z

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.

Files matching packages/cli/src/databases/migrations/**:

Requested review from at least two engineers on migration.
Avoided irreversible data migrations.
Avoided deleting or updating data keys.
Wrote 'down' migration if possible.

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

packages/cli/src/databases/migrations/sqlite/1693554410387-DisallowOrphanExecutions.ts

codecov · 2023-09-01T08:44:23Z

Codecov Report

Patch coverage: 77.41% and no project coverage change.

Comparison is base (58e55ba) 32.11% compared to head (d21401b) 32.11%.
Report is 1 commits behind head on master.

❗ Current head d21401b differs from pull request most recent head e9f3753. Consider uploading reports for the commit e9f3753 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7069   +/-   ##
=======================================
  Coverage   32.11%   32.11%           
=======================================
  Files        3188     3188           
  Lines      195900   195924   +24     
  Branches    21364    21359    -5     
=======================================
+ Hits        62915    62924    +9     
- Misses     131953   131968   +15     
  Partials     1032     1032

Files Changed	Coverage Δ
packages/cli/src/databases/dsl/index.ts	`73.68% <57.14%> (-6.32%)`	⬇️
packages/cli/src/databases/dsl/Table.ts	`87.67% <81.81%> (-2.90%)`	⬇️
packages/cli/src/databases/dsl/Indices.ts	`91.30% <100.00%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This reverts commit a377768.

netroy · 2023-09-01T10:26:42Z

packages/cli/src/databases/migrations/mysqldb/1693554410387-DisallowOrphanExecutions.ts

+	 * Ensure all executions point to a workflow.
+	 */
+	async up({ queryRunner, escape }: MigrationContext) {
+		const executionEntity = escape.tableName('execution_entity');


why not combine the 3 migrations into a single one?

How do you mean? The syntax to modify a column differs between MySQL and Postgres, and sqlite requires recreating the table, and it looks like the DSL does not support this yet.

flipswitchingmonkey · 2023-09-01T11:19:19Z

packages/cli/src/databases/migrations/sqlite/1693554410387-DisallowOrphanExecutions.ts

+				tableName: 'workflow_entity',
+				columnName: 'id',
+				onDelete: 'CASCADE',
+			});


Will require recreating the indexes as well I think

Please see 672d2d7

ivov · 2023-09-04T09:00:42Z

@krynble @flipswitchingmonkey

If we recreate the indices, it all looks good. See before and after. The only difference is the intended non-null workflowId and the change from datetime to datetime(3).

If we recreate the column like below, typeorm creates a new table and fails when populating it. Note that the table name temporary_execution_entity is nowhere in my migration - I assume it comes from typeorm recreating the table when we add a column to sqlite. Unless you see a workaround, this would mean there is no way around recreating the table.

async up({
	escape,
	schemaBuilder: { column, addColumns, dropColumns },
	runQuery,
}: MigrationContext) {
	const executionEntity = escape.tableName('execution_entity');
	const workflowId = escape.columnName('workflowId');

	await runQuery(`DELETE FROM ${executionEntity} WHERE ${workflowId} IS NULL;`);

	await addColumns('execution_entity', [column('temp_column').varchar(36).notNull]);

	await runQuery(`UPDATE ${executionEntity} SET "temp_column" = ${workflowId};`);

	await dropColumns('execution_entity', ['workflowId']);

	await runQuery(`ALTER TABLE ${executionEntity} RENAME COLUMN "temp_column" TO ${workflowId};`);
}

query failed: INSERT INTO "temporary_execution_entity"("id", "workflowId", "finished", "mode", "retryOf", "retrySuccessId", "startedAt", "stoppedAt", "waitTill", "status") SELECT "id", "workflowId", "finished", "mode", "retryOf", "retrySuccessId", "startedAt", "stoppedAt", "waitTill", "status" FROM "execution_entity"
error: Error: SQLITE_CONSTRAINT: NOT NULL constraint failed: temporary_execution_entity.temp_column
QueryFailedError: SQLITE_CONSTRAINT: NOT NULL constraint failed: temporary_execution_entity.temp_column

…s (no-changelog)

This reverts commit a377768.

flipswitchingmonkey

see comment

flipswitchingmonkey · 2023-09-04T11:09:09Z

packages/cli/src/databases/dsl/Table.ts

+	}
+}
+
+export class DropNotNull extends TableOperation {


tiny comment: Drop in the name suggests that the column is actually gone after this, I'd probably use RemoveNotNullAttribute or something clearer here?

I'll leave this one to @netroy. I personally find it clear.

cypress · 2023-09-04T11:23:01Z

2 flaky tests on run #2082 ↗︎

0	238	0	0	2

Details:

🌳 disallow-orphan-executions 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e...
Project: n8n	Commit: `e9f3753a1c`
Status: Passed	Duration: 07:32 💡
Started: Sep 4, 2023 1:42 PM	Ended: Sep 4, 2023 1:49 PM

24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items		`Output` `Screenshots` `Video`

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		`Output` `Screenshots` `Video`

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

github-actions · 2023-09-04T11:23:02Z

✅ All Cypress E2E specs passed

github-actions · 2023-09-04T13:18:26Z

✅ All Cypress E2E specs passed

netroy

Let's merge after the DB Tests pass.

github-actions · 2023-09-04T13:50:04Z

✅ All Cypress E2E specs passed

# [1.6.0](https://github.com/n8n-io/n8n/compare/n8n@1.5.1...n8n@1.6.0) (2023-09-06) ### Bug Fixes * **core:** Add support for in-transit encryption (TLS) on Redis connections ([#7047](#7047)) ([a910757](a910757)) * **core:** Disallow orphan executions ([#7069](#7069)) ([8a28e98](8a28e98)) * **core:** Split event bus controller into community and ee ([#7107](#7107)) ([011ee2e](011ee2e)) * **editor:** Standardize save text ([#7093](#7093)) ([58b3492](58b3492)) * Ensure all new executions are saved ([#7061](#7061)) ([b8e06d2](b8e06d2)) * Load remote resources even if expressions in non requried parameters resolve ([#6987](#6987)) ([8a8d4e8](8a8d4e8)) * **Postgres Node:** Connection pool of the database object has been destroyed ([#7074](#7074)) ([9dd5f0e](9dd5f0e)) * **Postgres Node:** Tunnel doesn't always close ([#7087](#7087)) ([58e55ba](58e55ba)) ### Features * **core:** Add list query middleware to credentials ([#7041](#7041)) ([fd78021](fd78021)) * **core:** Add support for floating licenses ([#7090](#7090)) ([e26553f](e26553f)) * **core:** Migration for soft deletions for executions ([#7088](#7088)) ([413e0bc](413e0bc)) * **HTTP Request Node:** Determine binary file name from content-disposition headers ([#7032](#7032)) ([273d091](273d091)) * **TheHive Node:** Overhaul ([#6457](#6457)) ([73e782e](73e782e)) Co-authored-by: netroy <netroy@users.noreply.github.com>

janober · 2023-09-06T12:25:29Z

Got released with n8n@1.6.0

refactor: Disallow orphan executions

9adc4cc

ivov requested a review from a team as a code owner September 1, 2023 08:24

ivov commented Sep 1, 2023

View reviewed changes

packages/cli/src/databases/migrations/sqlite/1693554410387-DisallowOrphanExecutions.ts Outdated Show resolved Hide resolved

ivov commented Sep 1, 2023

View reviewed changes

packages/cli/src/databases/migrations/sqlite/1693554410387-DisallowOrphanExecutions.ts Outdated Show resolved Hide resolved

ivov added 2 commits September 1, 2023 11:33

Add backticks for MySQL

a377768

Revert "Add backticks for MySQL"

022bee2

This reverts commit a377768.

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

Fix silly mistake

b41180c

netroy reviewed Sep 1, 2023

View reviewed changes

flipswitchingmonkey reviewed Sep 1, 2023

View reviewed changes

ivov added 2 commits September 1, 2023 15:20

Merge branch 'master' into disallow-orphan-executions

c621ec7

Add indices

672d2d7

Merge branch 'master' into disallow-orphan-executions

3ed4afe

ivov requested a review from flipswitchingmonkey September 4, 2023 09:21

netroy and others added 12 commits September 4, 2023 12:47

feat(core): Add new DSL for adding and dropping NOT NULL constraint…

af7d7d2

…s (no-changelog)

feat(core): Add new DSL for adding and dropping NOT NULL constraint…

50fb9ea

…s (no-changelog)

Merge branch 'DSL-dropNotNull' into disallow-orphan-executions

ea5569b

refactor: Disallow orphan executions

7cc3edd

Add backticks for MySQL

7164645

Revert "Add backticks for MySQL"

a797bcf

This reverts commit a377768.

Fix silly mistake

9052696

Add indices

5db2111

Simplify using DSL updates

a07ecd5

use consistent order of fields in DSL methods

5735388

DSL exceptions should reject the lazy promise

a7eca82

Update 1693554410387-DisallowOrphanExecutions.ts

ed5ca4b

flipswitchingmonkey previously approved these changes Sep 4, 2023

View reviewed changes

ivov added 2 commits September 4, 2023 13:51

Merge branch 'DSL-dropNotNull' into disallow-orphan-executions

c8c3510

Cleanup

d21401b

ivov dismissed flipswitchingmonkey’s stale review via d21401b September 4, 2023 12:01

Merge branch 'master' into disallow-orphan-executions

e9f3753

flipswitchingmonkey approved these changes Sep 4, 2023

View reviewed changes

netroy approved these changes Sep 4, 2023

View reviewed changes

ivov merged commit 8a28e98 into master Sep 4, 2023
64 of 65 checks passed

ivov deleted the disallow-orphan-executions branch September 4, 2023 14:57

github-actions bot mentioned this pull request Sep 6, 2023

🚀 Release 1.6.0 #7120

Merged

janober added the Released label Sep 6, 2023

ivov mentioned this pull request Sep 15, 2023

refactor(core): Implement soft-deletions for executions #7092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Disallow orphan executions #7069

fix(core): Disallow orphan executions #7069

ivov commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023

codecov bot commented Sep 1, 2023 •

edited

Loading

netroy Sep 1, 2023

ivov Sep 1, 2023

flipswitchingmonkey Sep 1, 2023

ivov Sep 4, 2023

ivov commented Sep 4, 2023 •

edited

Loading

flipswitchingmonkey left a comment

flipswitchingmonkey Sep 4, 2023

ivov Sep 4, 2023

cypress bot commented Sep 4, 2023 •

edited

Loading

github-actions bot commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

netroy left a comment

github-actions bot commented Sep 4, 2023

janober commented Sep 6, 2023

fix(core): Disallow orphan executions #7069

fix(core): Disallow orphan executions #7069

Conversation

ivov commented Sep 1, 2023 • edited Loading

github-actions bot commented Sep 1, 2023

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

netroy Sep 1, 2023

Choose a reason for hiding this comment

ivov Sep 1, 2023

Choose a reason for hiding this comment

flipswitchingmonkey Sep 1, 2023

Choose a reason for hiding this comment

ivov Sep 4, 2023

Choose a reason for hiding this comment

ivov commented Sep 4, 2023 • edited Loading

flipswitchingmonkey left a comment

Choose a reason for hiding this comment

flipswitchingmonkey Sep 4, 2023

Choose a reason for hiding this comment

ivov Sep 4, 2023

Choose a reason for hiding this comment

cypress bot commented Sep 4, 2023 • edited Loading

2 flaky tests on run #2082 ↗︎

github-actions bot commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

netroy left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 4, 2023

janober commented Sep 6, 2023

ivov commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading

ivov commented Sep 4, 2023 •

edited

Loading

cypress bot commented Sep 4, 2023 •

edited

Loading