fix(plans): retrying two failed accounts of one ramp step advances CurrentStep twice, so a later step silently never purchases #1669

Surfaced during the adversarial review of #1655 (fix for #1537). Pre-existing on main — the reviewer confirmed the increment count is identical before and after that PR, so #1655 does not cause it. Filing because #1655 makes per-account retry the canonical recovery flow, which makes this reachable far more often.

What

IncrementPlanCurrentStep (internal/config/store_postgres.go:624) advances the ramp unconditionally:

if !plan.RampSchedule.IsComplete() {
    plan.RampSchedule.CurrentStep++
}

It carries no notion of which step is being completed, so it is a blind ++ rather than a "complete step N" operation. Its caller is purchase.updatePlanProgress (internal/purchase/execution.go), invoked from executeAndFinalize on every fully-successful execution (execErr == nil) that has a PlanID.

Why that double-counts

A multi-account plan produces one execution per account per ramp step, and each per-account row can succeed or fail independently. Consider plan P at ramp step 3 over accounts A, B, C:

1. Step 3 fans out. A succeeds. B and C fail (credentials, throttling, whatever).
2. The root row is partially_completed, so executeAndFinalize skips updatePlanProgress for it — but executeForAccount rows are saved separately, and the run that does reach execErr == nil increments.
3. The operator retries B. It succeeds → updatePlanProgress → CurrentStep++.
4. The operator retries C. It succeeds → updatePlanProgress → CurrentStep++.

Two retries of the same ramp step advance CurrentStep twice. The ramp skips a step: a later step's purchase silently never happens, and GetNextPurchaseDate is computed from the wrong index so NextExecutionDate is wrong too.

This is the direction-(b) failure shape — not a double purchase, but a legitimate purchase that silently never happens, which is equally a money defect and much harder to notice. Nothing errors; the plan just quietly under-buys and reports itself as further along than it is.

Why #1655 raises the exposure

Before #1655, retrying a per-account row re-entered the whole fan-out (that was the #1537 bug). After it, a per-account retry is a clean single-account re-drive that succeeds, which is exactly the path that reaches execErr == nil and increments. Per-account retry becomes the normal way to recover a partially-failed ramp step, so what used to be an unusual sequence becomes routine.

Suggested fix

Make the operation idempotent per step rather than a blind increment. Options, roughly in order of preference:

1. Compare-and-set on the step being completed. Pass the completing execution's StepNumber and advance only when plan.RampSchedule.CurrentStep == stepNumber, inside the existing FOR UPDATE transaction. A retry of an already-counted step is then a no-op. This is the smallest change and reuses the row lock that is already held.
2. Derive progress instead of accumulating it. Compute CurrentStep from the executions table (the highest step with all its per-account rows terminal-and-successful) rather than storing an incrementally-mutated counter.

Either way, the multi-account case needs a decision the current code never makes: a ramp step is only complete when every account's row for that step has succeeded, not when any one of them has. Worth settling that explicitly in this issue before implementing.

Regression test

Assert on CurrentStep, and drive the real flow rather than calling the store directly: fan a plan step out over 3 accounts with 2 failing, retry both failed rows to success, and assert CurrentStep advanced by exactly 1. Confirm it fails pre-fix with an advance of 2. internal/purchase/money_path_regression_test.go has the fan-out harness.

Related

• fix(purchase): retrying a per-account fan-out row re-fans-out and double-buys every account that already succeeded #1537 / fix(purchase): keep account scope on per-account retry successors #1655 — per-account retry scope, the PR this was found under.
• fix(purchases): multi-account run mislabels partial success as failed; finalizeExecution coupling is fragile #1014 — multi-account partial-success classification, which is what routes the root row away from updatePlanProgress in step 2 above.