fix(purchase): pre-#1713 retry successors can still buy a second Azure savings plan on approval

[cristim]

#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 > 0 for an Azure savings-plans recommendation, already sitting in pending, notified or approved, 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:

RetryAttemptN > 0 && RedriveRefusalReason(exec) != ""

The RetryAttemptN > 0 term is what preserves first-purchase behaviour: a fresh execution has no retry lineage, so it is unaffected. Reuse purchase.RedriveRefusalReason introduced 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:

SELECT execution_id, plan_id, status, retry_attempt_n, scheduled_date
FROM purchase_executions
WHERE retry_attempt_n > 0
  AND status IN ('pending','notified','approved')
  AND recommendations::text ILIKE '%savingsplans%';

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 > 0 in approved, driven through ApproveAndExecute, asserting no provider purchase call. Plus the negative control: the same recommendation with RetryAttemptN == 0 must 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.

[cristim]

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_date and recommendations all exist on purchase_executions, and recommendations is JSONB NOT NULL DEFAULT '[]' (migrations/000001_initial_schema.up.sql:84), with provider / service as 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.

1. Misses status = 'scheduled'. approveWithDelay (internal/api/handler_purchases.go) transitions an approved row pending → scheduled and returns; it fires later from the scheduler sweep via fireOneDue. 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).
2. Misses the savings-plans spelling. ILIKE '%savingsplans%' does not match the hyphenated form, which RedriveRefusalReason treats as equally unsafe and which mapSavingsPlansSlug routes to the same Azure savings-plans client.
3. False-positives on AWS savings plans. No provider filter, so aws / savingsplans rows 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_at is in the projection because for a scheduled row 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), aws savings plans and savings-plans-compute (AWS plan types, all ClientToken-deduped), azure / compute reservations (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 failed with 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.

[cristim]

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 of executeAndFinalize:

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          <- MISSED

fireOneDue is not incidental. approveWithDelay is invoked from both approval routes (internal/api/handler_purchases.go:613 and :713); it transitions the row pending -> scheduled and returns, and the purchase fires later from the scheduler sweep. So on any deployment with the pre-fire delay configured, approval never passes through ApproveAndExecute at 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 via fireOneDue
• misses the savings-plans spelling — 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_elements version 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.

[cristim]

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's default: 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 rows result 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-plans spellings, 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.