Repository navigation
fix(plans): a ramp step counts as complete when one of N accounts buys, and both silent-under-buy paths are log-only #1861
Description
Split out of #1669 (fix in flight on fix/1669-ramp-step-idempotent), which deliberately kept its scope to "one ramp step must not be counted twice". Two problems remain, and they are the same problem seen from two sides: a plan can quietly buy less commitment than the customer intended, and nothing durable says so.
1. A ramp step counts as complete when ONE account's execution runs clean
CompletePlanStep advances the ramp when any execution for that step finishes successfully. A multi-account plan fans one ramp step out into one execution per cloud account, each of which succeeds or fails independently, so an operator who repairs and retries a single failed account moves the plan to "step N done" while the other accounts have bought nothing for step N.
This predates #1669 and is unchanged by it: the pre-#1669 blind CurrentStep++ had the same granularity, plus the double-count that #1669 removes. It is visible directly in that fix's own regression test (internal/purchase/ramp_step_progress_integration_test.go): a 3-account plan whose step-3 fan-out committed only account A reaches CurrentStep = 3 as soon as account B's retry succeeds, while account C is still failed.
The original issue asked for this decision to be settled explicitly:
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.
It was not settled, and #1669 does not settle it. Settling it is this issue.
Options, roughly in order of preference:
- Derive progress instead of accumulating it (fix(plans): retrying two failed accounts of one ramp step advances CurrentStep twice, so a later step silently never purchases #1669's option 2): compute
CurrentStepfrom the executions table as the highest step whose per-account rows are all terminal-and-successful, rather than storing an incrementally-mutated counter. This also removes the migration/backfill class of problem entirely, because there is no stored convention left to get wrong. - Gate the advance on sibling completeness: keep the counter, but only advance when no per-account row for that step is outstanding. Cheaper, but needs care that a permanently-failed account cannot freeze the ramp forever, which is the failure mode option 1 avoids by construction.
Whichever is chosen, "the plan's target accounts at the time the step ran" has to be pinned down, since a plan's account set can change between steps.
2. Two reachable money-path decisions are recorded only in a log line
Both of these silently under-buy and neither leaves durable evidence. purchase.executeAndFinalize calls updatePlanProgress for side effect only and swallows any error into logging.Errorf, so once the Lambda log ages out there is nothing to find.
- A refused advance.
CompletePlanSteprefuses to advance when the completing step is more than one beyondCurrentStep, because the steps in between never completed and jumping would overstate what the plan bought. Reachable whenever a step fails terminally and a later step then succeeds. The plan silently stops advancing,next_execution_datestops moving, andplan_healthcan only infer "behind schedule" from wall-clock drift, which it reports identically for a plan that is merely late. - A mis-stamped step. A
purchase_executionsrow whosestep_numberpredates fix(plans): retrying two failed accounts of one ramp step advances CurrentStep twice, so a later step silently never purchases #1669's convention correction, or that was written during the deploy overlap, completes a step the plan has already counted. The advance is a correct no-op, and equally invisible.
Wanted:
- Stamp the outcome on the execution row (an audit note alongside
error) so a refused or no-op advance surfaces in History rather than only in CloudWatch. - Give
plan_healtha distinct stalled-ramp factor, keyed on the recorded refusal instead of lettingbehind_scheduleproxy for it.
Why this is P1 rather than P2
The failure direction is the same silent under-buy that #1669 was filed for: commitment the customer intended to buy that never gets bought, with nothing erroring and the plan reporting itself further along than it is. Multi-account plans are the normal shape for the customers who ramp, and per-account retry became the canonical recovery flow in #1655, so item 1 is reachable on the routine path rather than an edge case.
Related
- fix(plans): retrying two failed accounts of one ramp step advances CurrentStep twice, so a later step silently never purchases #1669 — ramp step counted twice; the fix that deliberately left these two open.
- fix(purchase): keep account scope on per-account retry successors #1655 / fix(purchase): retrying a per-account fan-out row re-fans-out and double-buys every account that already succeeded #1537 — per-account retry scope, which makes item 1 routine.
- fix(purchases): multi-account run mislabels partial success as failed; finalizeExecution coupling is fragile #1014 — multi-account partial-success classification.
Activity
cristim commentedon Aug 19, 2026
Adding a third concrete failure to item 2, surfaced by CodeRabbit's review of #1862 and confirmed against the code.
A stale
step_numbertakes the "already counted" branch, which is silent by construction and cannot be flagged at this layer.CompletePlanStepreturns nil whenCurrentStep >= stepNumber, so a row stampedkon a plan already atCurrentStep = kproduces no error and no log line at all. The other two paths in this issue at least log; this one does not.It is indistinguishable from the designed case, which is why no guard in #1862 can catch it. A second per-account retry of a step a sibling already counted also arrives with
CurrentStep == stepNumber, and that no-op is exactly what #1669 was filed to produce. Both look identical to the store, so flagging one flags the other and makes the normal recovery path noisy.Why it matters more than "the plan is one step behind": the stall is not self-correcting.
CompletePlanStepreturns before its write, sonext_execution_datestays stale;shouldNotifyPlancomputesdaysUntil := int(time.Until(*plan.NextExecutionDate).Hours() / config.HoursPerDay)and returns false once that is negative. The plan is then never notified again,getOrCreateExecutionis never reached, and no further executions are created for it. On the notification-driven path the plan goes quiet permanently rather than retrying, so the remaining ramp steps never buy.Reachable in two windows that #1862's migration deliberately does not close: instances still running the old writer during the deploy rollout, and retry successors of rows that were already terminal when the migration ran, since
persistRetryExecutionpropagatesfailedExec.StepNumberand terminal rows are not rewritten.This is the strongest argument for option 1 over option 2 in the original description. Deriving
CurrentStepfrom the executions table separates the two cases naturally, because the derivation looks at what actually completed rather than at a counter plus a stored convention that can go stale. A sibling-completeness gate on the accumulated counter would not: it inherits the same ambiguity, since it still has to trust the row'sstep_numberto know which step is being reported.For the record, #1862 does now stamp the two detectable refusals on the execution row (
recordRampAdvanceRefusal), so those surface in History rather than only in CloudWatch. This third path is the one that remains invisible, and it is invisible by construction rather than by omission.fix(plans): record a refused ramp advance on the execution row
fix(plans): make the ramp advance idempotent per step (#1862)
fix(plans): decide the create-side ramp guard under the plan lock (#1861
fix(plans): count a ramp step only when every account has bought (#1880)