Repository navigation
fix(plans): retrying two failed accounts of one ramp step advances CurrentStep twice, so a later step silently never purchases #1669
Description
Activity
Reproduced against a real Postgres before writing the fix, driving the real
purchase.ManagerthroughhandleExecutePurchaserather than calling the store directly. The defect is real and the numbers match, but step 2 of the walkthrough above is wrong in a way worth recording, because it attributes one of the two increments to the wrong place.Fixture: 4-step weekly plan at
CurrentStep = 2, three AWS accounts, A resolves credentials and B and C do not.event observed CurrentStepcommitments bought (cumulative) start 2 0 step-3 fan-out: A commits, B and C fail 2 1 operator repairs B and C, retries B: succeeds 3 2 retries C: succeeds 4 (should be 3) 3 The correction. The issue says of the partially-failed fan-out that "
executeForAccountrows are saved separately, and the run that does reachexecErr == nilincrements". It does not.executeMultiAccountreturns*multiAccountPartialErrorwhen at least one account committed and at least one failed, soexecuteAndFinalizenever reachesupdatePlanProgressfor the root row, andexecuteForAccountnever calls it at all. The ramp correctly stays at 2 after the fan-out (measured, third row above).So both extra counts come from the two per-account retries, each of which is a clean single-account re-drive reaching
execErr == nilwith aPlanID. The net effect is exactly as described (two increments for one ramp step) and the fix is unchanged, but the mechanism has one source, not two. That matters for anyone reading this issue to reason about related paths: a partial fan-out is not a place where the ramp advances.One consequence is also weaker than stated. Nothing gates execution on
CurrentStep— the scheduler never createspurchase_executionsrows, it only runs rows that already exist, keyed onscheduled_date. So an already-created row for a later step still purchases. The damage is in the accounting and what it drives, which the same run measured directly: after the double advance the plan reportedRampSchedule.IsComplete() == trueandNextExecutionDate == nilhaving bought 3 of 4 steps. From there the ramp is finished as far as the plan is concerned, andapi.createPurchaseExecutionsTxnumbers any subsequently created rows from the inflatedCurrentStep. "A later step silently never purchases" is real; it arrives through progress accounting rather than through the scheduler skipping an existing row.Two things found while fixing this, both now handled:
purchase.getOrCreateExecutionstampedstep_numberwith the COUNT of completed steps, whileapi.createPurchaseExecutionsTxstamped the 1-based step being executed. Harmless while the advance was a blind++that never read the column, fatal once it keys on it, so the fix corrects the writer and backfills the executable rows already carrying the old convention.- There is no schema gap:
purchase_executions.step_numberhas beenINTEGER NOT NULL DEFAULT 1since000001_initial_schema.up.sql:78, and retry successors already propagate it. The completing execution's step was readable from its own row; nothing had to be inferred from timing or ordering.
The multi-account granularity question raised in the "Suggested fix" section ("a ramp step is only complete when every account's row for that step has succeeded") is not settled by this fix, which deliberately only stops one step being counted twice. Split out as #1861 along with the observability half.
- added a commit that references this issue
on Aug 19, 2026
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:It carries no notion of which step is being completed, so it is a blind
++rather than a "complete step N" operation. Its caller ispurchase.updatePlanProgress(internal/purchase/execution.go), invoked fromexecuteAndFinalizeon every fully-successful execution (execErr == nil) that has aPlanID.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:
partially_completed, soexecuteAndFinalizeskipsupdatePlanProgressfor it — butexecuteForAccountrows are saved separately, and the run that does reachexecErr == nilincrements.updatePlanProgress→CurrentStep++.updatePlanProgress→CurrentStep++.Two retries of the same ramp step advance
CurrentSteptwice. The ramp skips a step: a later step's purchase silently never happens, andGetNextPurchaseDateis computed from the wrong index soNextExecutionDateis 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 == niland 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:
StepNumberand advance only whenplan.RampSchedule.CurrentStep == stepNumber, inside the existingFOR UPDATEtransaction. 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.CurrentStepfrom 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 assertCurrentStepadvanced by exactly 1. Confirm it fails pre-fix with an advance of 2.internal/purchase/money_path_regression_test.gohas the fan-out harness.Related
updatePlanProgressin step 2 above.