From 627b488556fd3085c1006a17fbc02fb254b5c70d Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Mon, 1 Jun 2026 17:03:50 +0200 Subject: [PATCH] Fix else branches not properly skipped in nested if/else workflows (#20938) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Extract `findParentSteps` utility that recognizes IF-ELSE steps as parents of their branch children (via `settings.input.branches[].nextStepIds`), used in all parent detection sites (`shouldSkipStepExecution`, `shouldExecuteStep`, `shouldFailSafely`, and their iterator variants) - Centralize next-step resolution in `getNextStepIdsToExecute` via extracted `getNextStepIdsForIterator` and `getNextStepIdsForIfElse` utils — Iterator now properly returns loop children as `nextStepIdsToSkip`/`nextStepIdsToFailSafely` when skipped - Refactor `skipAndFailSafelyStepsThenContinue` to delegate to `getNextStepIdsToExecute` instead of duplicating type-specific propagation logic Fixes #20934 ## Test plan - [x] New unit tests for `findParentSteps` (7 tests covering IF-ELSE branch parent detection) - [x] New IF-ELSE-specific tests added to `shouldSkipStepExecution`, `shouldExecuteStep`, `shouldFailSafely` test suites - [x] Updated Iterator skip/fail-safely tests in `workflow-executor.workspace-service.spec.ts` - [x] All 300 workflow executor tests pass - [x] `lint:ci` passes --- .../__tests__/find-parent-steps.util.spec.ts | 93 +++++++++++++ .../should-execute-step.util.spec.ts | 46 +++++++ .../__tests__/should-fail-safely.util.spec.ts | 39 ++++++ .../should-skip-step-execution.util.spec.ts | 47 ++++++- .../utils/find-parent-steps.util.ts | 30 ++++ .../utils/should-execute-step.util.ts | 7 +- .../utils/should-fail-safely.util.ts | 7 +- .../utils/should-skip-step-execution.util.ts | 7 +- .../get-next-step-ids-for-if-else.util.ts | 48 +++++++ .../get-next-step-ids-for-iterator.util.ts | 45 ++++++ .../should-execute-iterator-step.util.ts | 12 +- .../should-fail-safely-iterator-step.util.ts | 12 +- ...hould-skip-iterator-step-execution.util.ts | 10 +- ...orkflow-executor.workspace-service.spec.ts | 8 +- .../workflow-executor.workspace-service.ts | 130 ++++++++++-------- 15 files changed, 441 insertions(+), 100 deletions(-) create mode 100644 packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/find-parent-steps.util.spec.ts create mode 100644 packages/twenty-server/src/modules/workflow/workflow-executor/utils/find-parent-steps.util.ts create mode 100644 packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/get-next-step-ids-for-if-else.util.ts create mode 100644 packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-next-step-ids-for-iterator.util.ts diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/find-parent-steps.util.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/find-parent-steps.util.spec.ts new file mode 100644 index 0000000000..d97395806b --- /dev/null +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/find-parent-steps.util.spec.ts @@ -0,0 +1,93 @@ +import { + createMockCodeStep, + createMockIfElseStep, +} from 'src/modules/workflow/workflow-executor/utils/create-mock-workflow-steps.util'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; +import { type WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; + +describe('findParentSteps', () => { + it('should find parent via nextStepIds', () => { + const parent = createMockCodeStep('parent', ['child']); + const child = createMockCodeStep('child'); + const steps = [parent, child]; + + const result = findParentSteps({ step: child, steps }); + + expect(result).toEqual([parent]); + }); + + it('should return empty array when no parent exists', () => { + const stepA = createMockCodeStep('a'); + const stepB = createMockCodeStep('b'); + const steps = [stepA, stepB]; + + const result = findParentSteps({ step: stepB, steps }); + + expect(result).toEqual([]); + }); + + it('should find IF-ELSE parent via branch nextStepIds', () => { + const branchChild = createMockCodeStep('branch-child'); + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['branch-child'] }, + { id: 'false-branch', nextStepIds: ['other-child'] }, + ]); + const steps: WorkflowAction[] = [ifElseStep, branchChild]; + + const result = findParentSteps({ step: branchChild, steps }); + + expect(result).toEqual([ifElseStep]); + }); + + it('should find IF-ELSE parent for else branch child', () => { + const elseChild = createMockCodeStep('else-child'); + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['true-child'] }, + { id: 'false-branch', nextStepIds: ['else-child'] }, + ]); + const steps: WorkflowAction[] = [ifElseStep, elseChild]; + + const result = findParentSteps({ step: elseChild, steps }); + + expect(result).toEqual([ifElseStep]); + }); + + it('should find nested IF-ELSE parent via branch', () => { + const nestedIfElse = createMockIfElseStep('nested-if-else', [ + { id: 'nested-true', nextStepIds: ['step-y'] }, + { id: 'nested-false', nextStepIds: ['step-z'] }, + ]); + const outerIfElse = createMockIfElseStep('outer-if-else', [ + { id: 'outer-true', nextStepIds: ['step-x'] }, + { id: 'outer-false', nextStepIds: ['nested-if-else'] }, + ]); + const steps: WorkflowAction[] = [outerIfElse, nestedIfElse]; + + const result = findParentSteps({ step: nestedIfElse, steps }); + + expect(result).toEqual([outerIfElse]); + }); + + it('should handle undefined steps gracefully', () => { + const parent = createMockCodeStep('parent', ['child']); + const child = createMockCodeStep('child'); + const steps = [parent, undefined as unknown as WorkflowAction, child]; + + const result = findParentSteps({ step: child, steps }); + + expect(result).toEqual([parent]); + }); + + it('should find multiple parents from different sources', () => { + const child = createMockCodeStep('child'); + const standardParent = createMockCodeStep('standard', ['child']); + const ifElseParent = createMockIfElseStep('if-else', [ + { id: 'branch', nextStepIds: ['child'] }, + ]); + const steps: WorkflowAction[] = [standardParent, ifElseParent, child]; + + const result = findParentSteps({ step: child, steps }); + + expect(result).toEqual([standardParent, ifElseParent]); + }); +}); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-execute-step.util.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-execute-step.util.spec.ts index 417bcc146a..713caf3b27 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-execute-step.util.spec.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-execute-step.util.spec.ts @@ -1,6 +1,10 @@ import { StepStatus } from 'twenty-shared/workflow'; import { WorkflowRunStatus } from 'src/modules/workflow/common/standard-objects/workflow-run.workspace-entity'; +import { + createMockCodeStep, + createMockIfElseStep, +} from 'src/modules/workflow/workflow-executor/utils/create-mock-workflow-steps.util'; import { shouldExecuteStep } from 'src/modules/workflow/workflow-executor/utils/should-execute-step.util'; import { type WorkflowAction, @@ -466,4 +470,46 @@ describe('shouldExecuteStep', () => { expect(result).toBe(false); }); + + it('should return true when IF-ELSE parent succeeded and step is a branch child', () => { + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['step-a'] }, + { id: 'false-branch', nextStepIds: ['step-b'] }, + ]); + const stepA = createMockCodeStep('step-a'); + const allSteps: WorkflowAction[] = [ifElseStep, stepA]; + + const result = shouldExecuteStep({ + step: stepA, + steps: allSteps, + stepInfos: { + 'if-else': { status: StepStatus.SUCCESS }, + 'step-a': { status: StepStatus.NOT_STARTED }, + }, + workflowRunStatus: WorkflowRunStatus.RUNNING, + }); + + expect(result).toBe(true); + }); + + it('should return false when IF-ELSE parent is skipped and step is a branch child', () => { + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['step-a'] }, + { id: 'false-branch', nextStepIds: ['step-b'] }, + ]); + const stepA = createMockCodeStep('step-a'); + const allSteps: WorkflowAction[] = [ifElseStep, stepA]; + + const result = shouldExecuteStep({ + step: stepA, + steps: allSteps, + stepInfos: { + 'if-else': { status: StepStatus.SKIPPED }, + 'step-a': { status: StepStatus.NOT_STARTED }, + }, + workflowRunStatus: WorkflowRunStatus.RUNNING, + }); + + expect(result).toBe(false); + }); }); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-fail-safely.util.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-fail-safely.util.spec.ts index 7b0a46b9de..0b9dcd9d60 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-fail-safely.util.spec.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-fail-safely.util.spec.ts @@ -2,9 +2,11 @@ import { StepStatus } from 'twenty-shared/workflow'; import { createMockCodeStep, + createMockIfElseStep, createMockIteratorStep, } from 'src/modules/workflow/workflow-executor/utils/create-mock-workflow-steps.util'; import { shouldFailSafely } from 'src/modules/workflow/workflow-executor/utils/should-fail-safely.util'; +import { type WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; describe('shouldFailSafely', () => { it('should return false when step has no parents', () => { @@ -107,6 +109,43 @@ describe('shouldFailSafely', () => { expect(result).toBe(true); }); + + it('should return true when IF-ELSE parent is FAILED_SAFELY and step is a branch child', () => { + const childStep = createMockCodeStep('child'); + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['child'] }, + { id: 'false-branch', nextStepIds: ['other'] }, + ]); + const steps: WorkflowAction[] = [ifElseStep, childStep]; + + const result = shouldFailSafely({ + step: childStep, + steps, + stepInfos: { + 'if-else': { status: StepStatus.FAILED_SAFELY, error: 'err' }, + }, + }); + + expect(result).toBe(true); + }); + + it('should return false when IF-ELSE parent succeeded and step is a branch child', () => { + const childStep = createMockCodeStep('child'); + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'branch', nextStepIds: ['child'] }, + ]); + const steps: WorkflowAction[] = [ifElseStep, childStep]; + + const result = shouldFailSafely({ + step: childStep, + steps, + stepInfos: { + 'if-else': { status: StepStatus.SUCCESS }, + }, + }); + + expect(result).toBe(false); + }); }); describe('shouldFailSafely for iterator steps', () => { diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-skip-step-execution.util.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-skip-step-execution.util.spec.ts index f21b0a4f90..fe25b007ad 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-skip-step-execution.util.spec.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/__tests__/should-skip-step-execution.util.spec.ts @@ -1,6 +1,9 @@ import { StepStatus } from 'twenty-shared/workflow'; -import { createMockCodeStep } from 'src/modules/workflow/workflow-executor/utils/create-mock-workflow-steps.util'; +import { + createMockCodeStep, + createMockIfElseStep, +} from 'src/modules/workflow/workflow-executor/utils/create-mock-workflow-steps.util'; import { shouldSkipStepExecution } from 'src/modules/workflow/workflow-executor/utils/should-skip-step-execution.util'; import { type WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; @@ -295,4 +298,46 @@ describe('shouldSkipStepExecution', () => { }), ).toBe(false); }); + + it('should return true when IF-ELSE parent is skipped and step is a branch child', () => { + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['step-a'] }, + { id: 'false-branch', nextStepIds: ['step-b'] }, + ]); + const stepA = createMockCodeStep('step-a'); + const stepB = createMockCodeStep('step-b'); + const steps: WorkflowAction[] = [ifElseStep, stepA, stepB]; + + const result = shouldSkipStepExecution({ + step: stepA, + steps, + stepInfos: { + 'if-else': { status: StepStatus.SKIPPED }, + 'step-a': { status: StepStatus.NOT_STARTED }, + 'step-b': { status: StepStatus.NOT_STARTED }, + }, + }); + + expect(result).toBe(true); + }); + + it('should return false when IF-ELSE parent succeeded and step is a branch child', () => { + const ifElseStep = createMockIfElseStep('if-else', [ + { id: 'true-branch', nextStepIds: ['step-a'] }, + { id: 'false-branch', nextStepIds: ['step-b'] }, + ]); + const stepA = createMockCodeStep('step-a'); + const steps: WorkflowAction[] = [ifElseStep, stepA]; + + const result = shouldSkipStepExecution({ + step: stepA, + steps, + stepInfos: { + 'if-else': { status: StepStatus.SUCCESS }, + 'step-a': { status: StepStatus.NOT_STARTED }, + }, + }); + + expect(result).toBe(false); + }); }); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/find-parent-steps.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/find-parent-steps.util.ts new file mode 100644 index 0000000000..aefe6473a1 --- /dev/null +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/find-parent-steps.util.ts @@ -0,0 +1,30 @@ +import { isDefined } from 'twenty-shared/utils'; + +import { isWorkflowIfElseAction } from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/guards/is-workflow-if-else-action.guard'; +import { type WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; + +export const findParentSteps = ({ + step, + steps, +}: { + step: WorkflowAction; + steps: WorkflowAction[]; +}): WorkflowAction[] => { + return steps.filter((candidateParent) => { + if (!isDefined(candidateParent)) { + return false; + } + + if (candidateParent.nextStepIds?.includes(step.id)) { + return true; + } + + if (isWorkflowIfElseAction(candidateParent)) { + return candidateParent.settings.input.branches.some((branch) => + branch.nextStepIds?.includes(step.id), + ); + } + + return false; + }); +}; diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-execute-step.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-execute-step.util.ts index a5ddd90d25..1bd6ab557f 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-execute-step.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-execute-step.util.ts @@ -1,7 +1,7 @@ -import { isDefined } from 'twenty-shared/utils'; import { type WorkflowRunStepInfos } from 'twenty-shared/workflow'; import { WorkflowRunStatus } from 'src/modules/workflow/common/standard-objects/workflow-run.workspace-entity'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; import { shouldExecuteChildStep } from 'src/modules/workflow/workflow-executor/utils/should-execute-child-step.util'; import { stepHasBeenStarted } from 'src/modules/workflow/workflow-executor/utils/step-has-been-started.util'; import { isWorkflowIteratorAction } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/guards/is-workflow-iterator-action.guard'; @@ -35,10 +35,7 @@ export const shouldExecuteStep = ({ return false; } - const parentSteps = steps.filter( - (parentStep) => - isDefined(parentStep) && parentStep.nextStepIds?.includes(step.id), - ); + const parentSteps = findParentSteps({ step, steps }); return shouldExecuteChildStep({ parentSteps, diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-fail-safely.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-fail-safely.util.ts index 65a54d2a5f..88d95a8dce 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-fail-safely.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-fail-safely.util.ts @@ -1,7 +1,7 @@ -import { isDefined } from 'twenty-shared/utils'; import { StepStatus, type WorkflowRunStepInfos } from 'twenty-shared/workflow'; import { TERMINAL_STEP_STATUSES } from 'src/modules/workflow/workflow-executor/constants/terminal-step-statuses.constant'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; import { isWorkflowIteratorAction } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/guards/is-workflow-iterator-action.guard'; import { shouldFailSafelyIteratorStep } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-fail-safely-iterator-step.util'; import { type WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; @@ -23,10 +23,7 @@ export const shouldFailSafely = ({ }); } - const parentSteps = steps.filter( - (parentStep) => - isDefined(parentStep) && parentStep.nextStepIds?.includes(step.id), - ); + const parentSteps = findParentSteps({ step, steps }); if (parentSteps.length === 0) { return false; diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-skip-step-execution.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-skip-step-execution.util.ts index 741b050012..895fce0080 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-skip-step-execution.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/utils/should-skip-step-execution.util.ts @@ -1,6 +1,6 @@ -import { isDefined } from 'twenty-shared/utils'; import { StepStatus, type WorkflowRunStepInfos } from 'twenty-shared/workflow'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; import { isWorkflowIteratorAction } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/guards/is-workflow-iterator-action.guard'; import { shouldSkipIteratorStepExecution } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-skip-iterator-step-execution.util'; import { type WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; @@ -22,10 +22,7 @@ export const shouldSkipStepExecution = ({ }); } - const parentSteps = steps.filter( - (parentStep) => - isDefined(parentStep) && parentStep.nextStepIds?.includes(step.id), - ); + const parentSteps = findParentSteps({ step, steps }); if (parentSteps.length === 0) { return false; diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/get-next-step-ids-for-if-else.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/get-next-step-ids-for-if-else.util.ts new file mode 100644 index 0000000000..cf8cec3a98 --- /dev/null +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/get-next-step-ids-for-if-else.util.ts @@ -0,0 +1,48 @@ +import { type WorkflowActionOutput } from 'src/modules/workflow/workflow-executor/types/workflow-action-output.type'; +import { type WorkflowIfElseResult } from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/types/workflow-if-else-result.type'; +import { type WorkflowIfElseAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; + +export const getNextStepIdsForIfElse = ({ + executedStep, + executedStepOutput, +}: { + executedStep: WorkflowIfElseAction; + executedStepOutput: WorkflowActionOutput; +}): { + nextStepIdsToExecute?: string[]; + nextStepIdsToSkip?: string[]; + nextStepIdsToFailSafely?: string[]; +} => { + const ifElseResult = executedStepOutput.result as + | WorkflowIfElseResult + | undefined; + + const branches = executedStep.settings.input.branches; + + if (ifElseResult?.matchingBranchId) { + const matchingBranch = branches.find( + (branch) => branch.id === ifElseResult.matchingBranchId, + ); + + const nonMatchingBranches = branches.filter( + (branch) => branch.id !== ifElseResult.matchingBranchId, + ); + + return { + nextStepIdsToExecute: matchingBranch?.nextStepIds, + nextStepIdsToSkip: nonMatchingBranches.flatMap( + (branch) => branch.nextStepIds, + ), + }; + } + + if (executedStepOutput.shouldFailSafely) { + return { + nextStepIdsToFailSafely: branches.flatMap((branch) => branch.nextStepIds), + }; + } + + return { + nextStepIdsToSkip: branches.flatMap((branch) => branch.nextStepIds), + }; +}; diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-next-step-ids-for-iterator.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-next-step-ids-for-iterator.util.ts new file mode 100644 index 0000000000..6af288e5ff --- /dev/null +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-next-step-ids-for-iterator.util.ts @@ -0,0 +1,45 @@ +import { isString } from '@sniptt/guards'; + +import { type WorkflowActionOutput } from 'src/modules/workflow/workflow-executor/types/workflow-action-output.type'; +import { type WorkflowIteratorResult } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/types/workflow-iterator-result.type'; +import { type WorkflowIteratorAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; + +type NextStepIds = { + nextStepIdsToExecute?: string[]; + nextStepIdsToSkip?: string[]; + nextStepIdsToFailSafely?: string[]; +}; + +// Returns next step IDs for the Iterator's children, or undefined if the +// Iterator has processed all items (caller should fall through to nextStepIds). +export const getNextStepIdsForIterator = ({ + executedStep, + executedStepOutput, +}: { + executedStep: WorkflowIteratorAction; + executedStepOutput: WorkflowActionOutput; +}): NextStepIds | undefined => { + const initialLoopStepIds = isString( + executedStep.settings.input.initialLoopStepIds, + ) + ? JSON.parse(executedStep.settings.input.initialLoopStepIds) + : (executedStep.settings.input.initialLoopStepIds ?? []); + + if (executedStepOutput.shouldSkipStepExecution) { + return { nextStepIdsToSkip: initialLoopStepIds }; + } + + if (executedStepOutput.shouldFailSafely) { + return { nextStepIdsToFailSafely: initialLoopStepIds }; + } + + const iteratorStepResult = executedStepOutput.result as + | WorkflowIteratorResult + | undefined; + + if (!iteratorStepResult?.hasProcessedAllItems) { + return { nextStepIdsToExecute: initialLoopStepIds }; + } + + return undefined; +}; diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-execute-iterator-step.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-execute-iterator-step.util.ts index b57748aca2..e841b6a6db 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-execute-iterator-step.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-execute-iterator-step.util.ts @@ -2,6 +2,7 @@ import { isDefined } from 'twenty-shared/utils'; import { StepStatus, type WorkflowRunStepInfos } from 'twenty-shared/workflow'; import { TERMINAL_STEP_STATUSES } from 'src/modules/workflow/workflow-executor/constants/terminal-step-statuses.constant'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; import { shouldExecuteChildStep } from 'src/modules/workflow/workflow-executor/utils/should-execute-child-step.util'; import { stepHasBeenStarted } from 'src/modules/workflow/workflow-executor/utils/step-has-been-started.util'; import { getAllStepIdsInLoop } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-all-step-ids-in-loop.util'; @@ -19,10 +20,7 @@ export const shouldExecuteIteratorStep = ({ steps: WorkflowAction[]; stepInfos: WorkflowRunStepInfos; }) => { - const stepsTargetingIterator = steps.filter( - (parentStep) => - isDefined(parentStep) && parentStep.nextStepIds?.includes(step.id), - ); + const allParentSteps = findParentSteps({ step, steps }); const initialLoopStepIds = step.settings.input.initialLoopStepIds; @@ -34,13 +32,13 @@ export const shouldExecuteIteratorStep = ({ }) : []; - const parentSteps = stepsTargetingIterator.filter( + const externalParentSteps = allParentSteps.filter( (parentStep) => !stepIdsInLoop.includes(parentStep.id), ); const stepsToCheck = stepHasBeenStarted(step.id, stepInfos) - ? stepsTargetingIterator - : parentSteps; + ? allParentSteps + : externalParentSteps; // When iterator has been started and has the continue-on-failure flag, // allow re-execution even if all loop-back parents are FAILED_SAFELY/SKIPPED diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-fail-safely-iterator-step.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-fail-safely-iterator-step.util.ts index 5f36a0a68e..f871b62a89 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-fail-safely-iterator-step.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-fail-safely-iterator-step.util.ts @@ -2,6 +2,7 @@ import { isDefined } from 'twenty-shared/utils'; import { StepStatus, type WorkflowRunStepInfos } from 'twenty-shared/workflow'; import { TERMINAL_STEP_STATUSES } from 'src/modules/workflow/workflow-executor/constants/terminal-step-statuses.constant'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; import { stepHasBeenStarted } from 'src/modules/workflow/workflow-executor/utils/step-has-been-started.util'; import { getAllStepIdsInLoop } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-all-step-ids-in-loop.util'; import { @@ -18,10 +19,7 @@ export const shouldFailSafelyIteratorStep = ({ steps: WorkflowAction[]; stepInfos: WorkflowRunStepInfos; }): boolean => { - const stepsTargetingIterator = steps.filter( - (parentStep) => - isDefined(parentStep) && parentStep.nextStepIds?.includes(step.id), - ); + const allParentSteps = findParentSteps({ step, steps }); const initialLoopStepIds = step.settings.input.initialLoopStepIds; @@ -33,7 +31,7 @@ export const shouldFailSafelyIteratorStep = ({ }) : []; - const externalParentSteps = stepsTargetingIterator.filter( + const externalParentSteps = allParentSteps.filter( (parentStep) => !stepIdsInLoop.includes(parentStep.id), ); @@ -55,7 +53,7 @@ export const shouldFailSafelyIteratorStep = ({ return areAllExternalParentsTerminal && hasFailedSafelyExternalParent; } - const areAllParentsTerminal = stepsTargetingIterator.every((parentStep) => + const areAllParentsTerminal = allParentSteps.every((parentStep) => TERMINAL_STEP_STATUSES.includes(stepInfos[parentStep.id]?.status), ); @@ -63,7 +61,7 @@ export const shouldFailSafelyIteratorStep = ({ return false; } - const hasFailedSafelyParent = stepsTargetingIterator.some( + const hasFailedSafelyParent = allParentSteps.some( (parentStep) => stepInfos[parentStep.id]?.status === StepStatus.FAILED_SAFELY, ); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-skip-iterator-step-execution.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-skip-iterator-step-execution.util.ts index 63115928f8..5ef0345476 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-skip-iterator-step-execution.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/should-skip-iterator-step-execution.util.ts @@ -1,6 +1,7 @@ import { isDefined } from 'twenty-shared/utils'; import { StepStatus, type WorkflowRunStepInfos } from 'twenty-shared/workflow'; +import { findParentSteps } from 'src/modules/workflow/workflow-executor/utils/find-parent-steps.util'; import { stepHasBeenStarted } from 'src/modules/workflow/workflow-executor/utils/step-has-been-started.util'; import { getAllStepIdsInLoop } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-all-step-ids-in-loop.util'; import { @@ -17,10 +18,7 @@ export const shouldSkipIteratorStepExecution = ({ steps: WorkflowAction[]; stepInfos: WorkflowRunStepInfos; }) => { - const stepsTargetingIterator = steps.filter( - (parentStep) => - isDefined(parentStep) && parentStep.nextStepIds?.includes(step.id), - ); + const allParentSteps = findParentSteps({ step, steps }); const initialLoopStepIds = step.settings.input.initialLoopStepIds; @@ -32,8 +30,8 @@ export const shouldSkipIteratorStepExecution = ({ }) : []; - const parentSteps = stepsTargetingIterator.filter( - (step) => !stepIdsInLoop.includes(step.id), + const parentSteps = allParentSteps.filter( + (parentStep) => !stepIdsInLoop.includes(parentStep.id), ); if (stepHasBeenStarted(step.id, stepInfos) || parentSteps.length === 0) { diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/__tests__/workflow-executor.workspace-service.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/__tests__/workflow-executor.workspace-service.spec.ts index a1a4bd58af..64f8947e50 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/__tests__/workflow-executor.workspace-service.spec.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/__tests__/workflow-executor.workspace-service.spec.ts @@ -542,7 +542,7 @@ describe('WorkflowExecutorWorkspaceService', () => { }); }); - it('should return nextStepIds for a fail-safe iterator instead of entering the loop', async () => { + it('should return loop children as nextStepIdsToFailSafely for a fail-safe iterator', async () => { const step = { id: 'iterator-1', type: WorkflowActionType.ITERATOR, @@ -562,11 +562,11 @@ describe('WorkflowExecutorWorkspaceService', () => { }); expect(result).toEqual({ - nextStepIdsToExecute: ['after-loop'], + nextStepIdsToFailSafely: ['loop-step-1'], }); }); - it('should return nextStepIds for a skipped iterator instead of entering the loop', async () => { + it('should return loop children as nextStepIdsToSkip for a skipped iterator', async () => { const step = { id: 'iterator-1', type: WorkflowActionType.ITERATOR, @@ -586,7 +586,7 @@ describe('WorkflowExecutorWorkspaceService', () => { }); expect(result).toEqual({ - nextStepIdsToExecute: ['after-loop'], + nextStepIdsToSkip: ['loop-step-1'], }); }); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts index 0b9ef297df..cc3fa7b152 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts @@ -1,6 +1,5 @@ import { Injectable } from '@nestjs/common'; -import { isString } from '@sniptt/guards'; import { isDefined } from 'twenty-shared/utils'; import { getWorkflowRunContext, @@ -42,10 +41,10 @@ import { shouldSkipStepExecution } from 'src/modules/workflow/workflow-executor/ import { workflowShouldFail } from 'src/modules/workflow/workflow-executor/utils/workflow-should-fail.util'; import { workflowShouldKeepRunning } from 'src/modules/workflow/workflow-executor/utils/workflow-should-keep-running.util'; import { isWorkflowIfElseAction } from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/guards/is-workflow-if-else-action.guard'; -import { type WorkflowIfElseResult } from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/types/workflow-if-else-result.type'; +import { getNextStepIdsForIfElse } from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/get-next-step-ids-for-if-else.util'; import { isWorkflowIteratorAction } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/guards/is-workflow-iterator-action.guard'; -import { WorkflowIteratorResult } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/types/workflow-iterator-result.type'; import { findEnclosingIteratorWithContinueOnFailure } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/find-enclosing-iterator-with-continue-on-failure.util'; +import { getNextStepIdsForIterator } from 'src/modules/workflow/workflow-executor/workflow-actions/iterator/utils/get-next-step-ids-for-iterator.util'; import { WorkflowAction } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type'; import { RUN_WORKFLOW_JOB_NAME } from 'src/modules/workflow/workflow-runner/constants/run-workflow-job-name'; import { type RunWorkflowJobData } from 'src/modules/workflow/workflow-runner/types/run-workflow-job-data.type'; @@ -250,61 +249,22 @@ export class WorkflowExecutorWorkspaceService { nextStepIdsToSkip?: string[]; nextStepIdsToFailSafely?: string[]; }> { - const isIteratorStep = isWorkflowIteratorAction(executedStep); + if (isWorkflowIteratorAction(executedStep)) { + const result = getNextStepIdsForIterator({ + executedStep, + executedStepOutput, + }); - if (isIteratorStep) { - const iteratorStepResult = executedStepOutput.result as - | WorkflowIteratorResult - | undefined; - - if ( - !iteratorStepResult?.hasProcessedAllItems && - !executedStepOutput.shouldFailSafely && - !executedStepOutput.shouldSkipStepExecution - ) { - const nextStepIdsToExecute = isString( - executedStep.settings.input.initialLoopStepIds, - ) - ? JSON.parse(executedStep.settings.input.initialLoopStepIds) - : executedStep.settings.input.initialLoopStepIds; - - return { nextStepIdsToExecute }; + if (result) { + return result; } } if (isWorkflowIfElseAction(executedStep)) { - const ifElseResult = executedStepOutput.result as - | WorkflowIfElseResult - | undefined; - - const branches = executedStep.settings.input.branches; - - if (ifElseResult?.matchingBranchId) { - const matchingBranch = branches.find( - (branch) => branch.id === ifElseResult.matchingBranchId, - ); - - const nonMatchingBranches = branches.filter( - (branch) => branch.id !== ifElseResult.matchingBranchId, - ); - - return { - nextStepIdsToExecute: matchingBranch?.nextStepIds, - nextStepIdsToSkip: nonMatchingBranches.flatMap( - (branch) => branch.nextStepIds, - ), - }; - } else if (executedStepOutput.shouldFailSafely) { - return { - nextStepIdsToFailSafely: branches.flatMap( - (branch) => branch.nextStepIds, - ), - }; - } else { - return { - nextStepIdsToSkip: branches.flatMap((branch) => branch.nextStepIds), - }; - } + return getNextStepIdsForIfElse({ + executedStep, + executedStepOutput, + }); } return { nextStepIdsToExecute: executedStep.nextStepIds }; @@ -563,19 +523,69 @@ export class WorkflowExecutorWorkspaceService { workspaceId, }); - const nextStepIds = new Set(); + const nextStepIdsToExecute = new Set(); + const cascadedStepIdsToSkip: string[] = []; + const cascadedStepIdsToFailSafely: string[] = []; - for (const stepId of [...stepIdsToSkip, ...stepIdsToFailSafely]) { - const step = steps.find((step) => step.id === stepId); + for (const stepId of stepIdsToSkip) { + const step = steps.find((candidate) => candidate.id === stepId); - for (const nextStepId of step?.nextStepIds ?? []) { - nextStepIds.add(nextStepId); + if (!step) { + continue; } + + const result = await this.getNextStepIdsToExecute({ + executedStep: step, + executedStepOutput: { shouldSkipStepExecution: true }, + }); + + for (const id of result.nextStepIdsToExecute ?? []) { + nextStepIdsToExecute.add(id); + } + cascadedStepIdsToSkip.push(...(result.nextStepIdsToSkip ?? [])); + cascadedStepIdsToFailSafely.push( + ...(result.nextStepIdsToFailSafely ?? []), + ); } - if (nextStepIds.size > 0) { + for (const stepId of stepIdsToFailSafely) { + const step = steps.find((candidate) => candidate.id === stepId); + + if (!step) { + continue; + } + + const result = await this.getNextStepIdsToExecute({ + executedStep: step, + executedStepOutput: { shouldFailSafely: true }, + }); + + for (const id of result.nextStepIdsToExecute ?? []) { + nextStepIdsToExecute.add(id); + } + cascadedStepIdsToSkip.push(...(result.nextStepIdsToSkip ?? [])); + cascadedStepIdsToFailSafely.push( + ...(result.nextStepIdsToFailSafely ?? []), + ); + } + + if ( + cascadedStepIdsToSkip.length > 0 || + cascadedStepIdsToFailSafely.length > 0 + ) { + await this.skipAndFailSafelyStepsThenContinue({ + stepIdsToSkip: cascadedStepIdsToSkip, + stepIdsToFailSafely: cascadedStepIdsToFailSafely, + steps, + workflowRunId, + workspaceId, + executedStepsCount, + }); + } + + if (nextStepIdsToExecute.size > 0) { await this.executeFromSteps({ - stepIds: Array.from(nextStepIds), + stepIds: Array.from(nextStepIdsToExecute), workflowRunId, workspaceId, shouldComputeWorkflowRunStatus: false,