From b7c5c7406f6f978bbd84737de34114e9492ae5f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= <ivov.src@gmail.com>
Date: Mon, 20 Nov 2023 16:03:02 +0100
Subject: [PATCH] fix(core): Ensure failed executions are saved in queue mode
 (#7744)

This PR adds `status` to run data so that
`determineFinalExecutionStatus` resolves correctly on execution failure
and removes the cleanup that is being duplicated in a worker hook.

Followup to https://github.com/n8n-io/n8n/pull/7138

Should fix:
- https://github.com/n8n-io/n8n/issues/7705
-
https://linear.app/n8n/issue/PAY-964/no-execution-found-after-execution-fails
-
https://linear.app/n8n/issue/PAY-1010/execution-deletion-in-queue-mode-not-complying-with-settings
---
 packages/cli/src/WorkflowRunner.ts            | 31 ++-----------------
 .../shared/sharedHookFunctions.ts             |  6 +++-
 2 files changed, 7 insertions(+), 30 deletions(-)

diff --git a/packages/cli/src/WorkflowRunner.ts b/packages/cli/src/WorkflowRunner.ts
index 74fab551b7564..995033805eaeb 100644
--- a/packages/cli/src/WorkflowRunner.ts
+++ b/packages/cli/src/WorkflowRunner.ts
@@ -559,11 +559,8 @@ export class WorkflowRunner {
 					};
 				}
 
-				let racingPromisesResult: JobResponse = {
-					success: false,
-				};
 				try {
-					racingPromisesResult = await Promise.race(racingPromises);
+					await Promise.race(racingPromises);
 					if (clearWatchdogInterval !== undefined) {
 						clearWatchdogInterval();
 					}
@@ -617,6 +614,7 @@ export class WorkflowRunner {
 					mode: fullExecutionData.mode,
 					startedAt: fullExecutionData.startedAt,
 					stoppedAt: fullExecutionData.stoppedAt,
+					status: fullExecutionData.status,
 				} as IRun;
 
 				if (executionHasPostExecutionPromises) {
@@ -631,31 +629,6 @@ export class WorkflowRunner {
 				// Normally also static data should be supplied here but as it only used for sending
 				// data to editor-UI is not needed.
 				await hooks.executeHookFunctions('workflowExecuteAfter', [runData]);
-				try {
-					// Check if this execution data has to be removed from database
-					// based on workflow settings.
-					const workflowSettings = data.workflowData.settings ?? {};
-					const saveDataErrorExecution =
-						workflowSettings.saveDataErrorExecution ?? config.getEnv('executions.saveDataOnError');
-					const saveDataSuccessExecution =
-						workflowSettings.saveDataSuccessExecution ??
-						config.getEnv('executions.saveDataOnSuccess');
-
-					const workflowDidSucceed = !racingPromisesResult.error;
-					if (
-						(workflowDidSucceed && saveDataSuccessExecution === 'none') ||
-						(!workflowDidSucceed && saveDataErrorExecution === 'none')
-					) {
-						await Container.get(ExecutionRepository).hardDelete({
-							workflowId: data.workflowData.id as string,
-							executionId,
-						});
-					}
-					// eslint-disable-next-line id-denylist
-				} catch (err) {
-					// We don't want errors here to crash n8n. Just log and proceed.
-					console.log('Error removing saved execution from database. More details: ', err);
-				}
 
 				resolve(runData);
 			},
diff --git a/packages/cli/src/executionLifecycleHooks/shared/sharedHookFunctions.ts b/packages/cli/src/executionLifecycleHooks/shared/sharedHookFunctions.ts
index 38497128a55ef..477b6be0ee2f6 100644
--- a/packages/cli/src/executionLifecycleHooks/shared/sharedHookFunctions.ts
+++ b/packages/cli/src/executionLifecycleHooks/shared/sharedHookFunctions.ts
@@ -10,8 +10,12 @@ import { Logger } from '@/Logger';
 export function determineFinalExecutionStatus(runData: IRun): ExecutionStatus {
 	const workflowHasCrashed = runData.status === 'crashed';
 	const workflowWasCanceled = runData.status === 'canceled';
+	const workflowHasFailed = runData.status === 'failed';
 	const workflowDidSucceed =
-		!runData.data.resultData?.error && !workflowHasCrashed && !workflowWasCanceled;
+		!runData.data.resultData?.error &&
+		!workflowHasCrashed &&
+		!workflowWasCanceled &&
+		!workflowHasFailed;
 	let workflowStatusFinal: ExecutionStatus = workflowDidSucceed ? 'success' : 'failed';
 	if (workflowHasCrashed) workflowStatusFinal = 'crashed';
 	if (workflowWasCanceled) workflowStatusFinal = 'canceled';