Repository navigation
fix(purchase): user-facing Retry ignores recIsSafeToRedrive, so retrying a landed Azure savings-plan buys a second one #1668
Description
Activity
- addedtriagedItem has been triagedItem has been triagedpriority/p1Next up; this sprintNext up; this sprintseverity/criticalMajor harm when it happensMajor harm when it happensurgency/this-sprintWithin the current sprintWithin the current sprintimpact/fewLimited audienceLimited audienceeffort/sHoursHourstype/bugDefectDefect
on Jul 28, 2026 - addedpriority/p0Drop everything; same-day fixDrop everything; same-day fixurgency/nowDrop other thingsDrop other thingsand removedpriority/p1Next up; this sprintNext up; this sprinturgency/this-sprintWithin the current sprintWithin the current sprint
on Aug 3, 2026 Re-triaged p1 -> p0, urgency/this-sprint -> urgency/now. The
severity/criticallabel was already correct; the derived priority was not consistent with it.Reasoning, so the call is auditable rather than just asserted:
- The loss is irreversible. An Azure savings plan cannot be cancelled. Every other money-path defect triaged p1 this session had a recoverable end state; this one does not. Irreversibility should dominate the derived priority even when
impact/fewwould otherwise temper it. - The trigger is a user clicking Retry, not an internal code path reachable only under an unusual sequence. No operator error and no malice is required — the button is doing what it appears to do.
impact/fewis accurate but misleading here. It counts affected users, not per-user magnitude. A small number of customers each buying a second uncancellable commitment is a worse outcome than a large number seeing a cosmetic defect, and the priority should reflect the product of the two rather than the count alone.
Flagging one limit honestly: I have not re-verified reachability against current
mainin this pass, so if the Retry path has changed since the issue was filed, the priority should be revisited rather than taken as settled.Found during a proportionality sweep of the backlog, which also surfaced a cluster of
p3/severity/lowroll-ups carrying security and money-path mediums inside them (#1607, #1613, LeanerCloud/cloud-commitments-platform#132, LeanerCloud/cloud-commitments-platform#134, LeanerCloud/cloud-commitments-platform#136, LeanerCloud/cloud-commitments-platform#111, LeanerCloud/cloud-commitments-go#53) and issues with missing or absent labels (#1528, #1478). Those are being handled separately.- The loss is irreversible. An Azure savings plan cannot be cancelled. Every other money-path defect triaged p1 this session had a recoverable end state; this one does not. Irreversibility should dominate the derived priority even when
- added 8 commits that reference this issue
on Aug 3, 2026 - added a commit that references this issue
on Aug 8, 2026
Surfaced during the adversarial review of #1655 (fix for #1537). Pre-existing on
main; #1655 does not cause it and does not fix it.What
purchase.recIsSafeToRedrive(internal/purchase/manager.go:290-307) already encodes the fact that an Azure savings-plans purchase is not safe to re-drive:Its wrapper
allRecsSafeToRedriveis called from exactly two places, both in the reaper:internal/purchase/reaper.go:224— deciding whether to append "; safe to retry" to the failure noteinternal/purchase/manager.go:469—RecoverStrandedApprovals, gating an in-place re-driveIt is never called from the user-facing retry path.
loadAndValidateRetryRequest/checkRetryRateGates/persistRetryExecution(internal/api/handler_purchases.go) gate on status, RBAC, already-retried,resolveOpsHint, and the retry-attempt threshold — nothing consults provider re-drive safety.Why it costs money
Every other provider path reproduces a deterministic provider token from the execution's lineage key, so a retry of an execution whose commitment actually landed short-circuits at the provider:
ClientToken(Savings Plans) or the EC2 RI tag-guardDoIdempotentPurchaseTwoSteplookup (#729)RequestId+ deterministic name derived from the token (#654)time.Now().UnixNano()So for an Azure SP row that failed after the order landed (a timeout, a post-purchase history write failure, a lost response), the operator sees a
failedrow with a Retry button, clicks it, and gets a second savings plan. Unlike every other path there is no server-side guard behind it, and a savings plan cannot be cancelled — it is a multi-year commitment.Nothing warns the operator either: the History UI hides Retry only when
resolveOpsHintmatches a known-persistent failure string, and none of those strings relate to provider re-drive safety.Suggested fix
Consult the existing helper on the user-facing path, rather than adding a second notion of re-drive safety:
loadAndValidateRetryRequest(orcheckRetryRateGates, next to the ops-hint gate), reject with a409+ a specific reason when any selected rec failsrecIsSafeToRedrive— the same shape as the existingops_hint409, so the frontend already renders it in place of the Retry button.?force=truemay override it. Given a savings plan cannot be cancelled, defaulting to not overridable seems right; if it is overridable it should require a distinct confirm, not the existing generic threshold confirm.recIsSafeToRedrive'sdefault: return falsealso means an unknown provider is currently retryable from the API while being un-redrivable by the reaper — worth aligning in the same change.Regression test
Assert on purchase counts, not statuses: an
azure/savingsplansfailed row must be refused by the retry endpoint with zero successor rows created, while anazure/computerow with the same shape is still retryable.internal/api/handler_purchases_retry_fanout_test.go(added in #1655) has a harness that drives the real retry handler into a realpurchase.Managerand counts commitments reaching the provider; the same shape fits here.Related
persistRetryExecutioncomment was corrected to state that scope propagation is necessary for dedupe everywhere and sufficient everywhere except Azure SP, and to point at this issue.DoIdempotentPurchaseTwoStep, which covers the other Azure services.