Repository navigation
sec(azure): re-check idempotency between in-process purchase retries - #1793
Conversation
cristim
added
triaged
📝 WalkthroughWalkthroughThe Azure reservation purchase flow now uses a shared guarded retry loop. Idempotent purchases recheck existing orders before retries, handle unreadable 400 responses, respect cancellation during delays, and stop duplicate purchases. Tests cover committed purchases, genuine retries, failed rechecks, and independent response bodies. ChangesAzure reservation retry safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
DoIdempotentPurchaseTwoStep looked up the idempotency token exactly once,
before the retry loop was entered. Each retry then minted a NEW
reservationOrderId via doCalculatePrice and purchased again, with nothing
linking attempt N+1 to a purchase attempt N may have committed before
reporting failure. The guard covered a re-drive ACROSS invocations and
nothing within one; the bound was purchaseMaxAttempts, not the guard.
Option 2 from the issue -- reuse one reservationOrderId across attempts so
Azure deduplicates -- is ruled out by the vendor contract, established rather
than assumed:
- Azure's own 400 says "Session timed out - Call CalculatePrice again and
provide the NEW Reservation Order ID for purchase". The order ID is
session-bound and explicitly dead after the only failure that reaches
this retry path, so reusing it is precisely what Azure tells callers not
to do.
- armreservations v1.1.0 documents no idempotency on BeginPurchase, and
ReservationOrderClientBeginPurchaseOptions carries only ResumeToken. No
idempotency-key parameter exists anywhere in the SDK.
- The package already records (#677 Option B) that a client-supplied
dedupe ID is unavailable on this API, which is why the tag-and-search
strategy exists at all.
So option 1: the same lookup runs before every attempt after the first. A
committed-but-failed attempt is found and its order returned instead of
buying again. A lookup that cannot be completed refuses the retry rather
than falling through, matching the pre-loop guard's contract -- treating an
unusable lookup as "no existing order" would reinstate the hole.
Cost: one list call per retry, on the retry path only. The happy path is
unchanged.
Three tests, both directions, because a fix that simply refused to retry
would pass a duplicate-only test while breaking the recovery this loop
exists for:
- a first attempt that committed before failing must yield exactly one
purchase and one calculatePrice (the re-check runs before minting
another order ID)
- a genuinely failed first attempt must still be recovered by the retry
- a re-check whose lookup fails must stop the retry
The first and third fail without the re-check; the third shows 3 purchases
where the fix gives 1. The recovery test passes both before and after, which
is its job as the control.
purchaseTwoStepGuarded reached gocyclo 13 with the re-check inlined, over the
10 the pre-merge check enforces, so the re-check, the retry predicate and the
inter-attempt delay are extracted -- the same pattern fetchReservationOrdersPage
already uses for the pagination loop.
An existing test asserted exactly one idempotency lookup and had to change.
Splitting it into two registrations surfaced the stateful-fixture hazard the
issue warns about: testify returns the same *http.Response for .Times(2), and
the first read drains its bytes.Buffer, so the second lookup decoded an empty
body. Each response is now minted separately.
Three comments that claimed no recheck exists between attempts -- on
errRetryabilityUnknown, on DoIdempotentPurchaseTwoStep and in the package doc
-- are corrected rather than left asserting the opposite of the new behavior.
Closes #1774
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
54fa36a to
c17d5ec
Compare
|
Merging. Closes #1774. Independently reviewed, with every claim re-derived by mutation rather than read. The reviewer is the agent that found this gap during #1767, and did not write the fix, so independence holds. The defect
Pre-existing on the The shape was chosen on evidence, and the evidence contradicted my preferenceI favoured reusing the
Had the attractive option been taken on its attractiveness, the fix would have reused an ID Azure had already retired, inside a change whose purpose is preventing a double purchase. Worth recording that establishing this took about ten minutes and no external documentation: it went unknown because nobody asked, which is how the original class started. The fixLookup before every attempt after the first. A committed-but-failed attempt is found and adopted; a lookup that cannot complete refuses the retry rather than falling through. Placed before Verified by execution, not by readingReviewer reverted
Three purchase calls reaching Azure where one is allowed is the defect in a single number. The control passing in both configurations is what proves safety was not bought by disabling retries, which a duplicate-only suite would never catch. Also verified independently:
The fixture hazard is real, and was reproducedReverting the two Gates20/20 checks pass. Reviewer re-ran everything against the final head rather than carrying it over, using separate clones at exact commits rather than golangci-lint v2.10.1 run sequentially (base then head, never parallel) to avoid the exit-3-with-empty-findings false clean: base Note for anyone reading this after deploying: Azure deploys have been failing on every |
1 participant
Closes #1774
The gap
DoIdempotentPurchaseTwoSteplooked up the idempotency token exactly once, before the retry loop was entered. Each retry then minted a newreservationOrderIdviadoCalculatePriceand purchased again, with nothing linking attempt N+1 to a purchase attempt N may have committed before reporting failure.The guard covered a re-drive across invocations and nothing within one. The bound was
purchaseMaxAttempts, not the guard.Option 2 is ruled out by the vendor contract — established, not assumed
The issue asked to verify Azure's behaviour before choosing a shape, since an unverified vendor assumption created this class. Reusing one
reservationOrderIdacross attempts so Azure deduplicates is not available:armreservations@v1.1.0:BeginPurchaseis "Purchase ReservationOrder and create resource under the specified URI", andReservationOrderClientBeginPurchaseOptionscarries onlyResumeToken. No idempotency-key parameter exists anywhere in the package.So this is option 1, and it is chosen on evidence rather than as a fallback.
The fix
The same lookup runs before every attempt after the first. A committed-but-failed attempt is found and its order returned instead of buying again. A lookup that cannot be completed refuses the retry rather than falling through — matching the pre-loop guard's contract, since treating an unusable lookup as "no existing order" would reinstate the hole.
The check runs before
doCalculatePrice, so an already-committed purchase short-circuits without minting yet another order ID.Cost: one list call per retry, on the retry path only. The happy path is unchanged.
Verification — both directions
The issue is explicit that a fix which simply refused to retry would pass a duplicate-only test while breaking the recovery this loop exists for. Three tests, driven through the real loop with a counting client:
DoesNotDoubleBuyWhenFirstAttemptCommittedStillRecoversWhenFirstAttemptGenuinelyFailedRefusesRetryWhenRecheckLookupFailsThe third is the clearest statement of the defect: three purchase calls reach Azure where the fix allows one. The recovery test passing both before and after is exactly its job — it is what proves the fix did not buy safety by disabling retries.
Two things surfaced while doing this
gocyclocaught the inlined version at 13, over the 10 the pre-merge check enforces. The re-check, the retry predicate and the inter-attempt delay are extracted — the same patternfetchReservationOrdersPagealready uses in this file for the pagination loop.An existing test asserted exactly one idempotency lookup and had to change. Splitting it into two registrations surfaced the stateful-fixture hazard the issue itself warns about: testify hands back the same
*http.Responsefor.Times(2), and the first read drains itsbytes.Buffer, so the second lookup decoded an empty body. Each response is now minted separately.Comments corrected, not left stale
Three claims asserted that no recheck exists between attempts — on
errRetryabilityUnknown, onDoIdempotentPurchaseTwoStep, and in the package doc. All three now describe the new behaviour. The loop's trailing"failed after N attempts (session timeout)"is unreachable and named only one of two triggers; it is now accurate and marked unreachable with the reason.Gates
providers/azure: gofmt clean, build, vet,go test -race -count=1 ./...green. Root: build, vet,gocyclo -over 10 -ignore "_test\.go" .exit 0 empty,golangci-lintv2.10.1 exit 0 with a genuine0 issues.line.providers/azurelint set diff vsorigin/main(a37e14790, after #1792 merged): 589 → 588, zero only-in-HEAD, one only-in-BASE. Rebased onto that merge; the branch is now a single commit as expected.Getting there took three iterations, and the method earned itself again: my first version added three findings (a
bodyclosefrom the extra fixture, ahonouringmisspelling, and agocritic unnamedResulton the new helper). A totals check would have shown 589 → 592 without naming any of them. All three fixed; the one removed line is afakeRespcall site replaced by an inline literal.Lint note: two runs came back
exit 3with an empty findings block —Error: parallel golangci-lint is running, the false-clean trap. Neither was recorded; the numbers above come from runs that produced a real findings block.Summary by CodeRabbit