TransactionCheckpoint#mergeFlow: unreachable branch in the state-merge conflict check (dead code, not a false-premise report)

[suparikoli]

Package

@medusajs/orchestration

What happened

TransactionCheckpoint.#mergeFlow (packages/core/orchestration/src/transaction/distributed-transaction.ts, around line 215-231 on current develop) reconciles the state property when merging a checkpoint against what's currently stored:

} else if (prop === "state") {
  const currentStateIndex =
    stateFlowOrderMap.get(currentTransactionData.flow.state) ?? -1
  const storedStateIndex =
    stateFlowOrderMap.get(storedData.flow.state) ?? -1

  if (storedStateIndex > currentStateIndex) {
    currentTransactionData.flow.state = storedData.flow.state
  } else if (
    currentStateIndex < storedStateIndex &&
    currentTransactionData.flow.state !== TransactionState.WAITING_TO_COMPENSATE
  ) {
    throw new SkipExecutionError(`Transaction is behind another execution`)
  }
}

currentStateIndex < storedStateIndex is the same condition as storedStateIndex > currentStateIndex, just with the operands swapped - not its complement. The else if can only run when the if was false, i.e. when storedStateIndex <= currentStateIndex, which directly contradicts currentStateIndex < storedStateIndex. So the else if branch is unreachable: SkipExecutionError("Transaction is behind another execution") can never be thrown from this branch, for any pair of TransactionState values.

I want to be upfront about what I'm sure of versus not, since I know this tracker has had reports based on stale or misread code before:

What I verified directly, with high confidence: the branch is dead. This is pure boolean logic (a > b and b < a are the same test), not a runtime/environment-dependent claim, so I don't think there's a scenario where it does execute. I traced the surrounding function to rule out the possibility that currentTransactionData.flow.state or storedData.flow.state get mutated between the two comparisons (they don't - both index lookups happen back-to-back with no intervening writes).

What I'm not sure of: what the branch was supposed to do, and therefore what the correct fix is. Comparing this to the structurally similar (and correct) step-level check a few lines down:

if (storedStepVersion > currentStepVersion) {
  throw new SkipExecutionError(`Transaction is behind another execution`)
}

...that one throws precisely when stored is ahead of current - the opposite of what the flow-level if branch does (which adopts the stored state rather than throwing when stored is ahead). That inconsistency, plus the specific WAITING_TO_COMPENSATE carve-out on the dead branch, made me hesitant to guess at intended semantics for a PR. It looks plausible that the comparison operator was meant to be reversed (currentStateIndex > storedStateIndex, i.e. "current is unexpectedly ahead of what's stored"), but I don't have enough context on the intended concurrent-execution model here to be confident, and getting it wrong in checkpoint-merge logic for the transaction/saga engine seems like the kind of thing that's worse to guess at than to flag.

Expected vs actual behavior

Expected: some (storedStateIndex, currentStateIndex) relationship results in SkipExecutionError being thrown from the else if branch shown above.

Actual: no relationship does. The branch's condition is a logical contradiction of the preceding if's condition, so it is unreachable.

How to reproduce that it's dead code

Not a runtime repro - a static one, reproducible by inspection:

const a = 5, b = 3
console.log(a > b)       // true  (a=stored, b=current -> takes the `if`)
console.log(b < a)       // true  (same pair, "else if" condition) - but this can only be
                          // reached when the `if` was false, and for this pair it's true,
                          // so control never gets to the else-if in the first place.

More directly: for any two numbers x (stored) and y (current), x > y and y < x are the same boolean. There is no (x, y) pair where the first is false and the second is true.

Suggested next step

I don't think this is safe for me to fix blind. Flagging with the exact location and the dead-code proof; whoever wrote the original merge logic (or is familiar with the intended stale-worker-detection semantics here) would know whether the fix is a flipped comparison operator, a different condition entirely, or something else. Happy to open a PR once the intended behavior is confirmed.

[medusa-os-bot]

Thanks for reporting this! We've labeled it as a bug and will investigate.

Confirmed dead code in TransactionCheckpoint.#mergeFlow (packages/core/orchestration/src/transaction/distributed-transaction.ts, lines 223-231). The else if condition currentStateIndex < storedStateIndex is logically identical to the preceding if condition storedStateIndex > currentStateIndex — same comparison, operands swapped — so the branch is unreachable and SkipExecutionError("Transaction is behind another execution") can never be thrown from the flow-state conflict check path. The reporter suspects the comparison operator was meant to be reversed (current ahead of stored, not behind), but correctly deferred to the team given the sensitive concurrent-execution semantics of the checkpoint-merge logic. Tagging requires-team for someone familiar with the intended stale-worker-detection model to confirm the correct fix.

[mvanhorn]

Implemented in #16462.