refactor(core): Consolidate execution lifecycle hooks even more + additional tests #12898

netroy · 2025-01-28T14:55:51Z

Summary

This PR

moves all execution related external hooks into lifecycle hooks to make these consistent across all execution modes.
hoists all Container.get calls in execution-lifecycle-hooks.ts to prepare that file for switching to DI
replaces hookFunctionsPreExecute with hookFunctionsSaveProgress and hookFunctionsExternalHooks
calculates saving settings only once for an execution
skips setting up the progress saving hook, when not needed
skips setting up push hooks, when not needed
reduces passing around of pushRef and retryOf
adds additional unit tests for code in and around lifecycle hooks
improves type-safety on retryOf

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

codecov · 2025-01-28T15:10:17Z

Codecov Report

Attention: Patch coverage is 93.54839% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...c/execution-lifecycle/execution-lifecycle-hooks.ts	93.75%	3 Missing ⚠️
packages/cli/src/workflow-runner.ts	75.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…the settings disable progress saving

ivov

A lot going on here, I'll come back later to continue reviewing.

ivov · 2025-02-03T11:20:34Z

packages/cli/src/workflow-runner.ts

 				});
+				this.logger.error('There was an error in the post-execution promise', {


Do we log this here already?

that's only when not using Sentry. I can remove this, but this line is not the same as the one on the default error reporter.

Ah I see. Let's keep for now but I've always found it confusing that the error reporter both reports to Sentry and logs out errors. I'd expect all error logging to happen via Logger.

packages/cli/src/execution-lifecycle/to-save-settings.ts

packages/cli/src/execution-lifecycle/save-execution-progress.ts

ivov

Most hooks log like this Executing hook (hookFunctionsPush) so we the log loses the actual hook being executed. Can we add them? e.g. Executing hook workflowExecuteAfter (hookFunctionsPush) The hook is as important as the bundle it belongs to. Also ideally if we can log this more centrally instead of at every hook.

ivov · 2025-02-03T13:11:36Z

packages/cli/src/active-executions.ts

-			if (executionData.retryOf !== undefined) {
-				fullExecutionData.retryOf = executionData.retryOf.toString();
-			}
+			fullExecutionData.retryOf = executionData.retryOf ?? undefined;


According to the type ?? undefined is not necessary.

Same elsewhere.

retryOf typings are still not 100% reliable, and sometimes this is null because the value came from typeorm.

packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts

ivov · 2025-02-03T13:14:37Z

packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts

-	} as IExecutionResponse);
+	test('should handle DB errors when updating the execution', async () => {
+		const error = new Error('Something went wrong');
+		executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse);


Suggested change

executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse);

executionRepository.findSingleExecution.mockResolvedValue(mock<IExecutionResponse>());

couldn't get it to work because of all the deep proxies.

packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts

ivov · 2025-02-03T13:15:25Z

packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts

 	});

-	executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse);
+	test('should populate `.data` when it is missing', async () => {
+		const fullExecutionData = {} as IExecutionResponse;


Suggested change

const fullExecutionData = {} as IExecutionResponse;

const fullExecutionData = mock<IExecutionResponse>();

ivov · 2025-02-03T13:22:56Z

packages/cli/src/workflow-runner.ts

@@ -392,7 +383,7 @@ export class WorkflowRunner {
 						data.executionMode,
 						executionId,
 						data.workflowData,
-						{ retryOf: data.retryOf ? data.retryOf.toString() : undefined },
+						{ retryOf: data.retryOf ?? undefined },


Suggested change

{ retryOf: data.retryOf ?? undefined },

{ retryOf: data.retryOf },

ivov · 2025-02-03T13:23:50Z

packages/workflow/src/Interfaces.ts

@@ -2284,7 +2284,7 @@ export interface IWorkflowExecutionDataProcess {
 	executionData?: IRunExecutionData;
 	runData?: IRunData;
 	pinData?: IPinData;
-	retryOf?: string;
+	retryOf?: string | null;


I thought it was string | undefined? If null is what the DB returns, can we transform it to undefined when we retrieve?

Having to remember retryOf ?? undefined disregarding the current type is an accident waiting to happen.

I want to spend more time on retryOf in a separate PR, to make the types around this more reliable.

ivov · 2025-02-03T13:24:12Z

packages/cli/src/active-executions.ts

@@ -179,7 +177,7 @@ export class ActiveExecutions {
 			data = this.activeExecutions[id];
 			returnData.push({
 				id,
-				retryOf: data.executionData.retryOf,
+				retryOf: data.executionData.retryOf ?? undefined,


Suggested change

retryOf: data.executionData.retryOf ?? undefined,

retryOf: data.executionData.retryOf,

ivov · 2025-02-03T13:29:25Z

packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts

+				await externalHooks.run('workflow.postExecute', [
+					fullRunData,
+					this.workflowData,
+					this.executionId,
+				]);


This used to be wrapped in a try-catch that ignored the error. I take it now that's covered when we run the hook, i.e. we won't crash if the external hook fails.

no other external hook (besides n8n.stop) is wrapped in try-catch, and I also clearly remember Jan telling me that if an external hook throws, then we throw that error. If someone has an external hook that breaks because of this, then they need to catch the error in the hook code.
External hooks aren't fire-and-forget. This is also why we await on them and run them sequentially.

packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts

…ifecycle-hooks

ivov · 2025-02-04T08:39:33Z

packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts

+type HooksSetupParameters = {
+	saveSettings: ExecutionSaveSettings;
+	pushRef?: string;
+	retryOf?: string;
+};


Does it make sense to remove this type and use the prop types directly?

Usage hints that these props might not belong together:

hookFunctionsPush accepts saveSettings and does not use it

hookFunctionsSaveProgress acceptspushRef and retryOf and does not use them

hookFunctionsSave uses all three props

hookFunctionsSaveWorker accepts saveSettings and does not use it

getWorkflowHooksWorkerExecuter inline omits saveSettings so we only pass it the other two

getWorkflowHooksWorkerMain inline omits saveSettings so we only pass it the other two

All this code will be re-written once I convert this file to switch to DI, like I did in #12364

ivov

Shaping up nicely! Nothing to block on

github-actions · 2025-02-04T08:53:52Z

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

cypress · 2025-02-04T08:54:02Z

n8n Run #9094

Run Properties: Passed #9094 • db229a3a6d: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*

Project	`n8n`
Branch Review	`external-hooks-in-lifecycle-hooks`
Run status	`Passed #9094`
Run duration	`04m 23s`
Commit	`db229a3a6d: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*`
Committer	`कारतोफ्फेलस्क्रिप्ट™`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`5`
Skipped	`0`
Passing	`433`
View all changes introduced in this branch ↗︎

github-actions · 2025-02-04T09:09:43Z

✅ All Cypress E2E specs passed

…itional tests (#12898)

janober · 2025-02-06T13:21:56Z

Got released with [email protected]

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jan 28, 2025

Base automatically changed from refactor-externalHooks to master January 29, 2025 09:33

netroy force-pushed the external-hooks-in-lifecycle-hooks branch from 429dc92 to d362673 Compare January 30, 2025 14:22

netroy changed the title ~~refactor(core): Consolidate all external hook calls in execution lifecycle hooks~~ refactor(core): Consolidate execution lifecycle hooks even more + additional tests Jan 30, 2025

netroy added 8 commits January 30, 2025 15:56

move all Container.get out of hook functions

da0499e

consolidate all external hook calls in execution lifecycle hooks

793ef7a

hookFunctionsPreExecute -> hookFunctionsSaveProgress

e4ee65f

calculate saving settings only once for an execution

4471c0f

do not even setup the nodeExecuteAfter hook for progress saving if …

50b45cf

…the settings disable progress saving

stop passing pushRef and retryOf around

e833b1b

additional tests for saveExecutionProgress

877511c

improve handling of retryOf

81a01dd

netroy force-pushed the external-hooks-in-lifecycle-hooks branch from d362673 to 81a01dd Compare January 30, 2025 14:56

netroy requested a review from ivov January 30, 2025 15:19

ivov reviewed Feb 3, 2025

View reviewed changes

netroy added 2 commits February 3, 2025 15:12

Merge remote-tracking branch 'origin/master' into external-hooks-in-l…

9b35264

…ifecycle-hooks

address PR comments

db229a3

ivov reviewed Feb 4, 2025

View reviewed changes

ivov approved these changes Feb 4, 2025

View reviewed changes

netroy merged commit 65ec6ae into master Feb 4, 2025
115 checks passed

netroy deleted the external-hooks-in-lifecycle-hooks branch February 4, 2025 09:17

riascho pushed a commit that referenced this pull request Feb 5, 2025

refactor(core): Consolidate execution lifecycle hooks even more + add…

9220b07

…itional tests (#12898)

janober added the Released label Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Consolidate execution lifecycle hooks even more + additional tests #12898

refactor(core): Consolidate execution lifecycle hooks even more + additional tests #12898

netroy commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

ivov left a comment

ivov Feb 3, 2025

netroy Feb 3, 2025

ivov Feb 4, 2025

ivov left a comment •

edited

Loading

ivov Feb 3, 2025

netroy Feb 3, 2025

ivov Feb 3, 2025

netroy Feb 3, 2025

ivov Feb 3, 2025

netroy Feb 3, 2025

ivov Feb 3, 2025

ivov Feb 3, 2025

netroy Feb 3, 2025

ivov Feb 3, 2025

netroy Feb 3, 2025

ivov Feb 3, 2025 •

edited

Loading

netroy Feb 3, 2025

ivov Feb 4, 2025

netroy Feb 4, 2025

ivov left a comment

github-actions bot commented Feb 4, 2025

cypress bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025

janober commented Feb 6, 2025

		});
		this.logger.error('There was an error in the post-execution promise', {

	executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse);
	executionRepository.findSingleExecution.mockResolvedValue(mock<IExecutionResponse>());

	const fullExecutionData = {} as IExecutionResponse;
	const fullExecutionData = mock<IExecutionResponse>();

	{ retryOf: data.retryOf ?? undefined },
	{ retryOf: data.retryOf },

	retryOf: data.executionData.retryOf ?? undefined,
	retryOf: data.executionData.retryOf,

refactor(core): Consolidate execution lifecycle hooks even more + additional tests #12898

refactor(core): Consolidate execution lifecycle hooks even more + additional tests #12898

Conversation

netroy commented Jan 28, 2025 • edited Loading

Summary

Review / Merge checklist

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2025

cypress bot commented Feb 4, 2025 • edited Loading

github-actions bot commented Feb 4, 2025

janober commented Feb 6, 2025

netroy commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

ivov left a comment •

edited

Loading

ivov Feb 3, 2025 •

edited

Loading

cypress bot commented Feb 4, 2025 •

edited

Loading