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

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.