fix(workflow): keep converging steps when an if/else branch is skipped

[thomtrp]

Fixes #24060

Problem

When both branches of an If/Else lead to the same step, that step is silently skipped and everything after it is skipped too. The run still reports COMPLETED.

If/Else
  ├ (if)   → Step A → Merge
  └ (else) ─────────→ Merge

Trigger the if branch and Merge comes out SKIPPED.

Cause

An If/Else succeeds as a step, but only one of its branches is live. Child evaluation (shouldExecuteChildStep, shouldSkipStepExecution, shouldFailSafely) only ever looked at a parent's status, so it could not tell which branch was taken — an If/Else parent read as SUCCESS no matter what.

To compensate, the If/Else injected that fact out of band: it force-wrote SKIPPED onto the roots of the branches not taken. That write happened before the taken branch ran, and stepHasBeenStarted made it permanent, so Merge could never be revived by the branch that did run.

That works for a step with one parent. It cannot work at a join, because no single parent knows enough: the If/Else says "skip Merge", Step A says "run Merge", and one sticky status field can only hold the first writer's opinion.

Fix

getEffectiveParentStatus makes an If/Else read as SKIPPED when seen from a child on a branch that was not taken:

parent is IF_ELSE with a matchingBranchId
  ? (child ∈ that branch's nextStepIds ? parent.status : SKIPPED)
  : parent.status

The three evaluation utils read a parent's status through this helper. The branch cut then falls out of the existing rules — nothing is forced, so Merge waits for its remaining parent and runs when that parent succeeds. getNextStepIdsForIfElse collapses to "hand back every branch root" (deduped), and the skip list, the reachability walk from earlier revisions, and the steps threading are all gone.

No new state: matchingBranchId is already persisted and was already read here.

Semantics this makes uniform

A step runs when a live path reaches it. That is the fix for the reported case, and it
generalises: a root of the branch that was NOT taken still runs if some other,
unconditional parent succeeded, because that parent is a live path to it. Main
force-skipped such a root, but only one level deep — a depth-2 descendant of the losing
branch with the same unrelated parent already ran on main. The cut is not a veto; it
only means the if/else itself is not a live path.

An if/else also reaches children through its own nextStepIds. That edge is
unconditional and is never cut; only branch edges are.

Two things this surfaced

• Iterator after-loop children. A step reaching a terminal state must hand every child back for evaluation, or a step converging on it never gets the second look that decides it. A skipped or fail-safed iterator handed back only its loop body. Main never hit this because the skip pass was awaited before the execute pass, so the iterator was always terminal before anything converged on it; removing the forced skips makes branches concurrent and exposes it.
• Iterator parents. The iterator's own skip and fail-safely checks read a parent's
  raw status, so an iterator wired directly as a branch root saw the if/else as SUCCESS
  and was neither run nor skipped: it stayed NOT_STARTED, everything after it stayed
  untouched, and the run still reported COMPLETED.
• Duplicate branch roots. If two branches point at the same step, handing back both roots would dispatch it twice through executeFromSteps (check-then-act guard lets both start). Roots are deduped before returning.

Testing

get-effective-parent-status.util.spec.ts covers the helper directly (taken/not-taken
branch, non-if/else parent, no-matching-branch, fail-safely propagation).
get-next-step-ids-for-if-else.util.spec.ts and get-next-step-ids-for-iterator.util.spec.ts
cover the dispatch, including a root shared by two branches and the iterator's terminal
returns. Full server workflow suite passes (558 tests).

Verified end to end on a local instance:

graph	result
convergence, if taken	merge SUCCESS (was SKIPPED)
convergence, else taken	merge SUCCESS, branch A SKIPPED
no convergence, either branch	other branch SKIPPED
filter stops the taken path	merge SKIPPED
nested If/Else, both conditions true	merge SKIPPED
nested If/Else, outer else taken	merge SUCCESS
If/Else inside an iterator loop body	branch cut correct every iteration, after-loop runs
iterator on the branch not taken	iterator + body SKIPPED, merge SUCCESS
iterator on the taken branch	loops, then merge SUCCESS
branch root with a live non-if/else parent	runs (was SKIPPED on main)

Known, unchanged from this PR

A cycle drawn inside the branch that was not taken (x -> y -> x) leaves both steps
NOT_STARTED: each waits on the other, and nothing can decide. Main only resolved it
because it force-wrote the root, which seeded the cascade. Same shape for a step made
unreachable by graph editing that still points at a live step, which already misbehaves
on main. Both need a reachability pass, which is out of scope here.

[twenty-ci-bot-public]

👋 Thanks for contributing to Twenty!

Your PR has been set to draft while you work on it. Once you're done, mark it as Ready for review and our automated checks will run.

Looking forward to your contribution!

[greptile-apps]

Greptile Summary

The PR fixes workflow convergence after an if/else by excluding nodes reachable from the selected branch from the non-selected branch’s skip roots.

• Adds cycle-safe graph traversal across normal, if/else, and iterator edges.
• Passes the complete workflow step graph through normal and resumed execution paths.
• Adds focused unit coverage for reachability, convergence, skipping, and fail-safe behavior.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or non-blocking defects identified in the changed execution paths.

The selected branch’s reachable nodes are excluded from force-skipping while genuinely non-selected roots continue through the existing skip cascade, and resumed execution receives the persisted workflow graph needed for the same decision.

Important Files Changed

Filename	Overview
packages/twenty-server/src/modules/workflow/workflow-executor/utils/get-reachable-step-ids.util.ts	Adds cycle-safe traversal over ordinary successors, if/else branches, and iterator loop entries.
packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/get-next-step-ids-for-if-else.util.ts	Prevents direct non-selected branch roots from being skipped when the selected branch can also reach them.
packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts	Supplies the workflow graph to branch continuation logic across normal, skip, and fail-safe execution.
packages/twenty-server/src/modules/workflow/workflow-runner/jobs/run-workflow.job.ts	Supplies persisted workflow steps when resuming execution in a subsequent job.
packages/twenty-server/src/modules/workflow/workflow-executor/utils/tests/get-reachable-step-ids.util.spec.ts	Covers transitive, branching, iterator, cyclic, and missing-node traversal.
packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/tests/get-next-step-ids-for-if-else.util.spec.ts	Covers converging and non-converging branches plus skipped and fail-safe outcomes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  IF["If/Else executes"] --> MATCH["Selected branch roots"]
  IF --> OTHER["Non-selected branch roots"]
  MATCH --> REACH["Compute structurally reachable steps"]
  OTHER --> FILTER{"Root reachable from selected branch?"}
  FILTER -->|Yes| KEEP["Do not force-skip convergence root"]
  FILTER -->|No| SKIP["Force-skip non-selected root"]
  KEEP --> EXEC["Selected branch reaches and executes node"]
  SKIP --> CASCADE["Existing skip cascade handles descendants"]

Loading

_{Reviews (1): Last reviewed commit: "fix(workflow): keep converging steps whe..." | Re-trigger Greptile}

[twenty-ci-bot-public]

✅ Standard review · no findings

Safe to merge — no findings; all prior review points resolved

High-level — Focused convergence bug fix that centralizes branch-cut logic in one getEffectiveParentStatus util reused by every parent-status check — one source of truth, no migration, public surface, or flag.
Low-level — Line-by-line clean: single object params, isDefined guards, const throughout, the result cast follows the module's established z.any()-narrowing pattern, and no WHAT comments are introduced in the new code.

---

_{Reviewed against the pr-review standard — high-level then low-level. Advisory; human review still required. Run details.}

[twenty-ci-bot-public]

🟡 Nit · Low-level · in-memory performance

steps.find inside the while loop scans the array per visited step, making traversal O(N²)

Each visited step re-scans the full steps array by id, which the in-memory perf rule flags as a per-item scan of a record that could be keyed. Build a Map<id, step> once before the loop and read map.get(stepId); low severity since a flow's step count is small.

[thomtrp]

Same as cubic's note, now a Map built once before the walk.

[cubic-dev-ai]

All reported issues were addressed across 6 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[twenty-ci-bot-public]

🔍 Automated Pre-Review

✅ No issues detected - This PR is ready for human review.

---

View details

Automated pre-review — human approval still required.

[twenty-ci-bot-public]

🔍 Visual Regression Review — twenty-ui

✅ No visual changes to review.

Changed: 0 · Added: 0 · Removed: 0 · Unchanged: 236

---

View run details · advisory mode

[cubic-dev-ai]

1 issue found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/twenty-shared/src/workflow/validation/utils/get-step-outgoing-step-ids.util.ts">

<violation number="1">
P2: When `initialLoopStepIds` contains a non-string value, this loop emits it as an outgoing step ID despite the helper’s `string[]` contract. Filter entries by `typeof nextStepId === 'string'` before adding them to the outgoing set.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[bosiraphael]

The fix works for the case in the issue, but the graph alone can't tell you which branch actually ran.

Two problems:

1. A step can now run when it shouldn't. Anything a nested if/else could lead to is treated as still live, so a shared step is left out of the skip list even when the path to it ends up skipped. It then sees the if/else above it succeeded, has no way to know it went the other way, and runs. That's worse than the bug it replaces: a step that runs by mistake writes data.

2. Nesting one level deeper changes nothing. If the skipped branch starts with another if/else, it goes through the part of the code this PR doesn't touch, which still marks everything below it as skipped.

Which branch was taken is only known while the run happens, so the branch-aware approach you mention under "Considered alternative" is what really fixes this. If that's too much for now, not looking past nested if/elses keeps the issue fixed without making anything new run.

Example for the first one:

If/Else A
  ├ (if)   → If/Else B
  │            ├ (if)   → Step 1
  │            └ (else) → Step 2 ─┐
  │                               ├→ Merge
  └ (else) ──────────────────────-┘

Both conditions true, so A goes if and B goes if. Step 1 runs, Step 2 is skipped, and neither arrow into Merge was taken. Merge still runs, and so does everything after it. On main it's skipped.

[thomtrp]

You're right on both, thanks. I traced your graph: Merge ran because Step 2's skip cascade re-enqueued it and evaluation only saw A's SUCCESS, with no idea the branch went the other way. Went with your interim: the walk now only follows nextStepIds and stops at nested if/elses (and doesn't enter iterator loop bodies, same runtime-unknowable argument), so nothing new can run and the flat case stays fixed. Your graph is now a unit test plus verified end to end on a local instance (merge SKIPPED with both conditions true, SUCCESS when A goes else). Nested convergence stays skipped like on main until evaluation becomes branch-aware, which I'd do as a follow-up. Sound good?

[bosiraphael]

Confirmed your graph and the flat case: merge stays skipped now, and the whole workflow suite passes on your head.

Two shapes still run a step that shouldn't, though. Neither is new to this commit, the previous walk had them too.

The easy one is a filter:

If/Else A
  ├ (if)   → Filter → Send email
  └ (else) ─────────→ Send email

The filter doesn't match, so its path stops there. But the walk counted the filter's next step as guaranteed, so the email stayed out of the skip list, and evaluation then sees a stopped parent plus A's success and sends it. On main it's skipped. Filters are common enough that this is much easier to hit than the nested if/else was.

The other one needs two if/elses: if a step on the taken path is killed by a second if/else, that step's own skip cascade re-enqueues whatever it pointed at, and it runs for the same reason. There's also a smaller one where the dead path runs through an iterator and the merge ends up with no status at all, run still COMPLETED.

The pattern is that "only follow nextStepIds" is right about the start of an edge but not the end: a step reached that way can still be stopped by a filter or skipped by another branch. Making the walk safe means also excluding filters, iterators, and anything another branch points at, which leaves little more than straight chains, and the next step type with an early exit reopens it.

So yes on nested convergence as a follow-up, but I'd rather not ship the filter case. Either the branch-aware evaluation now, or narrow the walk to just the straight-chain case it can actually guarantee.

[thomtrp]

Went with the branch-aware evaluation. getEffectiveParentStatus makes an if/else read as SKIPPED from a child on a branch that wasn't taken, so the existing parent-status rules cut the branch themselves and nothing is force-written. The static walk is gone.

Your filter graph now skips the merge, and the nested one skips it too, both verified on a local instance rather than just in unit tests.

Building it turned up one more thing worth flagging: a skipped or fail-safed iterator only handed its loop body back for evaluation, never its after-loop children. So a step converging past the iterator stayed NOT_STARTED and the run hung on RUNNING. Main never hit it because the skip pass was awaited before the execute pass, so the iterator was always terminal before anything converged on it — dropping the forced skips makes the branches concurrent and exposes it. Fixed by having the iterator hand back nextStepIds on both terminal paths.

Net diff is smaller than the previous approach. Does this look right to you?

[cubic-dev-ai]

All reported issues were addressed across 13 files (changes from recent commits).

_{You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[bosiraphael]

Now an iterator wired straight onto a branch never gets a status.

If/Else
  ├ (if)   → Send email
  └ (else) → Iterator → Update record

Take the if branch and the iterator is never run, never skipped, never marked at all, so everything after it stays untouched and the run still says COMPLETED. A later step joining back from the other branch silently doesn't run.

The three checks a step goes through only got half converted: the "should this run" one asks which branch was taken, but skip and fail-safely hand off to their iterator versions, which still read the parent's raw status. The if/else succeeded, so none of the three fires.

Passing the child step id into those two and reading the parent through getEffectiveParentStatus fixes it, same as you did for the execute one.

Also get-effective-parent-status.util.ts:35 guards matchingBranch but not its nextStepIds.

[thomtrp]

Fixed, and you were right about the mechanism: the skip and fail-safely checks still read the parent's raw status, so the iterator saw the if/else as SUCCESS and none of the three fired. All three iterator predicates now go through getEffectiveParentStatus.

Verified both directions on a local instance with your exact graph. Before: iterator, loop body and the step after it all NOT_STARTED, run COMPLETED. After: all three SKIPPED. Taking the else branch still loops and runs the merge.

One correction to my earlier repro, in case you tried it too: my first attempt built the graph by creating the iterator under the else branch's EMPTY node and then repointing the branch at it, which leaves that EMPTY node orphaned but still listing the iterator in nextStepIds. That orphan is a permanently NOT_STARTED parent, so the iterator could not conclude either way and the failure looked identical for a different reason. On a clean graph the fix is both necessary and sufficient.

Two more things came out of it. getEffectiveParentStatus was also cutting the if/else's own nextStepIds edge, which is unconditional and should never be cut, so a step reached that way was wrongly skipped. And if both branches name the same root, the deduped dispatch stops it being executed twice.

Two shapes are left out of scope, both noted in the PR body. A cycle drawn inside the branch that was not taken leaves both steps NOT_STARTED, since each waits on the other; main only resolved it because force-writing the root seeded the cascade. And a step made unreachable by graph editing that still points at a live step freezes that step, which already happens on main. Neither is caused by the branch cut and both need a user to draw an unusual graph, so I have left them for a follow-up.