Repository navigation
fix(purchase): pre-#1713 retry successors can still buy a second Azure savings plan on approval #1718
Description
Activity
Ops query: column names are fine, the predicates are not
Fix is up as #1721. Before writing it I sanity-checked the query above, since a query that errors or silently under-reports when someone finally runs it is worse than none.
Column names all check out against the current schema:
execution_id,plan_id,status,retry_attempt_n,scheduled_dateandrecommendationsall exist onpurchase_executions, andrecommendationsisJSONB NOT NULL DEFAULT '[]'(migrations/000001_initial_schema.up.sql:84), withprovider/serviceas the per-recommendation JSON keys.The predicates are wrong in three ways, two of them safety-critical. I did not eyeball this: I stood up a throwaway Postgres 16, recreated the table from the migration plus the retry columns, seeded a row for each case, and ran both queries.
- Misses
status = 'scheduled'.approveWithDelay(internal/api/handler_purchases.go) transitions an approved rowpending→scheduledand returns; it fires later from the scheduler sweep viafireOneDue. That is the delayed-approval window, and an armed row waiting it out is invisible to the query above. It is also the executor path this issue did not name (see fix(purchase): refuse to execute armed pre-#1668 retry successors (#1718) #1721). - Misses the
savings-plansspelling.ILIKE '%savingsplans%'does not match the hyphenated form, whichRedriveRefusalReasontreats as equally unsafe and whichmapSavingsPlansSlugroutes to the same Azure savings-plans client. - False-positives on AWS savings plans. No provider filter, so
aws/savingsplansrows match. Those are safe to re-drive (ClientToken), so they are noise on a query whose whole purpose is deciding whether to intervene on live money.
Measured on the seeded set: the original returned 5 rows, missing 2 genuinely armed ones and including 1 that is not armed. The corrected query returned all 6 armed rows and nothing else.
Corrected query
SELECT execution_id, plan_id, status, retry_attempt_n, scheduled_date, scheduled_execution_at FROM purchase_executions WHERE retry_attempt_n > 0 AND status IN ('pending','notified','approved','scheduled') AND EXISTS ( SELECT 1 FROM jsonb_array_elements(recommendations) AS r WHERE r->>'provider' = 'azure' AND r->>'service' IN ('savingsplans','savings-plans') ) ORDER BY scheduled_execution_at NULLS LAST, scheduled_date;
scheduled_execution_atis in the projection because for ascheduledrow it is the deadline: that is when the row fires on its own.Rows this correctly excludes, all verified:
retry_attempt_n = 0(a first buy, nothing to duplicate),awssavings plans andsavings-plans-compute(AWS plan types, allClientToken-deduped),azure/computereservations (DoIdempotentPurchaseTwoStep, #729), and rows already in a terminal status.Any row this returns is armed to buy a second, non-cancelable savings plan on approval. Once #1721 lands they are defused automatically on the next execution attempt: the executor refuses and stamps the row
failedwith the reason. Before then, cancelling them is the direct remedy.I left the original query in the issue body rather than rewriting it, so the reasoning trail stays intact.
- Misses
Correcting this issue: I named two executors; there are four, and the one I missed is the likely path.
Verified against
origin/main— every production caller ofexecuteAndFinalize:internal/purchase/approvals.go:352 ApproveAndExecute (named here) internal/purchase/manager.go:193 claimAndExecute (named here) internal/purchase/manager.go:377 claimAndRedrive (already gated by allRecsSafeToRedrive) internal/purchase/scheduled_fire.go:100 fireOneDue <- MISSEDfireOneDueis not incidental.approveWithDelayis invoked from both approval routes (internal/api/handler_purchases.go:613and:713); it transitions the rowpending->scheduledand returns, and the purchase fires later from the scheduler sweep. So on any deployment with the pre-fire delay configured, approval never passes throughApproveAndExecuteat all. A fix gating only the two executors named above would have left the primary approval route open for precisely the armed rows this issue exists to defuse.Because the call-site count in this issue was wrong, PR #1721 places the single gate at
executeAndFinalize— the funnel all four reach money through — rather than copying the condition to each caller. That is the more durable placement: a fifth executor added later cannot forget it.The ops query in the body above is also wrong; use the corrected one
The column names are right, the predicates are not. Verified empirically against a throwaway Postgres seeded with a row per case, rather than by inspection:
- misses
status = scheduled— the delayed-approval window, i.e. exactly the rows armed viafireOneDue - misses the
savings-plansspelling —ILIKE %savingsplans%does not match the hyphenated form, which is equally unsafe - false-positives on AWS savings plans, which are
ClientToken-deduped and therefore not armed
Measured on the seeded data: the original returns 5 rows — missing 2 genuinely armed ones and including 1 that is not armed. The corrected
jsonb_array_elementsversion returns all 6 armed rows and nothing else.The false negatives are the dangerous half: someone runs the query, sees fewer rows than exist, and concludes the exposure is smaller than it is. The original is left in the body deliberately so the reasoning trail stays intact — but do not run it; use the corrected query in the following comment.
- misses
Scope note on the corrected query above, from the #1721 adversarial review: it is exact for Azure savings plans but narrower than the guard it is meant to survey.
The query filters
provider = 'azure'. The guard does not stop there:recRedriveRefusalReason'sdefault:branch also refuses an unrecognized provider, which that filter excludes by construction.So the query answers "which Azure savings-plan successors are armed", not "which rows will the guard refuse". Those differ for any row carrying a provider string the code does not recognise.
Why this matters when someone actually runs it: a
0 rowsresult reads naturally as "nothing is armed, the guard will never fire". The accurate reading is "no Azure savings-plan rows are armed". If a row exists with an unrecognised provider, the guard still refuses it and the query never showed it.Everything else about the corrected query checks out — the status set, both
savingsplans/savings-plansspellings, and the JSON key paths all match what the guard actually inspects.Whoever runs this: treat the result as a lower bound on refusals, not a complete inventory. If you need the true set, drop the provider filter and post-filter, or query for the complement of the recognised providers as well.
- added a commit that references this issue
on Aug 8, 2026
#1713 gates the user-facing Retry at creation time, which stops new duplicate Azure savings-plan purchases. It does not defuse successors created before that fix landed.
An execution with
RetryAttemptN > 0for an Azure savings-plans recommendation, already sitting inpending,notifiedorapproved, will still purchase when approved. Neither executor consults re-drive safety:claimAndExecute(internal/purchase/manager.go:179)ApproveAndExecute(internal/purchase/approvals.go:332)That is correct as general behaviour — both must allow a first savings-plan purchase, and a blanket re-drive gate there would block legitimate buys. The gap is specifically the successor rows that a pre-fix retry already created.
Why this is not a defect in #1713
#1713 stops the ongoing bleed and is the right scope for #1668. This issue covers the rows already in flight, which no creation-time gate can reach.
Suggested fix
Gate at the executor on the conjunction, not on re-drive safety alone:
The
RetryAttemptN > 0term is what preserves first-purchase behaviour: a fresh execution has no retry lineage, so it is unaffected. Reusepurchase.RedriveRefusalReasonintroduced by #1713 — do not add a second predicate.Apply it at both executors, since either can reach a successor row.
Operational check, worth doing before the code fix
Determine whether any such rows exist right now. Something equivalent to:
Confirm the column names against the current schema before running; the intent is retried, not-yet-executed rows whose recommendations include an Azure savings plan. If any exist, they are armed to buy a second uncancellable commitment on approval, and that is worth handling directly rather than waiting for the code fix.
Regression test
An Azure savings-plans execution with
RetryAttemptN > 0inapproved, driven throughApproveAndExecute, asserting no provider purchase call. Plus the negative control: the same recommendation withRetryAttemptN == 0must still purchase exactly once, so the fix cannot be mistaken for blocking first buys.Found during the independent adversarial review of #1713. Not verified against production data: whether any matching rows actually exist is unknown from here.