fix(providers/azure): restore IdempotencyToken threading in DoPurchaseTwoStep#729
Conversation
…eTwoStep PR #680 (commit 65ecbf8) switched all 7 Azure reservation executors to DoPurchaseTwoStep, which mints a fresh server-side reservationOrderId per calculatePrice call and dropped the deterministic IdempotencyToken threading PR #653 had added. A re-drive of the same (executionID, recIndex) would create a SECOND Azure reservation, regressing #641's invariant. Option A (client-supplied order ID) is not viable: Azure rejects client-supplied IDs in the two-step flow (the very reason PR #680 had to adopt it). The fix implements Option B from the issue, mirroring the AWS EC2 findRIByIdempotencyToken pattern: * ApplyPurchaseTags now stamps two tags on every purchase body: purchase-automation (attribution, existing) and cudly-idempotency-token (new, the deterministic per-rec token). * FindReservationOrderByIdempotencyToken lists reservation orders via the Microsoft.Capacity REST endpoint, filters by tag, skips terminal-failed states (Cancelled/Failed/Expired), and follows nextLink pagination. * DoIdempotentPurchaseTwoStep wraps DoPurchaseTwoStep with the lookup-first/short-circuit guard. Empty token falls straight through (CLI legacy path). Failed lookups refuse to purchase rather than risk a double-buy (mirrors EC2 safety contract). All 7 service executors (cache, compute, cosmosdb, database, managedredis, search, synapse) now call DoIdempotentPurchaseTwoStep with opts.IdempotencyToken and ApplyPurchaseTags with both opts.Source and opts.IdempotencyToken. 20 new tests cover both invariants: same token produces one reservation (short-circuit), different tokens produce distinct reservations. DoPurchaseTwoStep retains its previous behaviour for callers without an owning execution. Per-service existing tests continue to pass unchanged. Closes #721, partial-restore of #641 invariant for Azure, unblocks #639.
cristim
added
triaged
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAzure reservation purchase requests now include idempotency-token tags and undergo tag-based deduplication via a new two-step idempotent orchestrator applied across all seven service clients, with comprehensive test coverage validating the lookup, short-circuit, and fallthrough paths. ChangesAzure Idempotent Reservation Purchases
Sequence DiagramsequenceDiagram
participant ServiceClient
participant PurchaseCommitment
participant DoIdempotentPurchaseTwoStep
participant FindReservationOrderByIdempotencyToken
participant AzureListAPI
participant DoPurchaseTwoStep
participant AzurePurchaseAPI
ServiceClient->>PurchaseCommitment: opts (source, idempotencyToken)
PurchaseCommitment->>PurchaseCommitment: validate source non-empty
PurchaseCommitment->>PurchaseCommitment: ApplyPurchaseTags(body, source, token)
PurchaseCommitment->>DoIdempotentPurchaseTwoStep: calcURL, body, token
alt Token present
DoIdempotentPurchaseTwoStep->>FindReservationOrderByIdempotencyToken: lookup by token
FindReservationOrderByIdempotencyToken->>AzureListAPI: list-reservation-orders
alt Match found (non-terminal)
AzureListAPI-->>FindReservationOrderByIdempotencyToken: order list
FindReservationOrderByIdempotencyToken-->>DoIdempotentPurchaseTwoStep: existing orderID
DoIdempotentPurchaseTwoStep-->>PurchaseCommitment: return orderID (short-circuit)
else Lookup error
AzureListAPI-->>FindReservationOrderByIdempotencyToken: error
FindReservationOrderByIdempotencyToken-->>DoIdempotentPurchaseTwoStep: error
DoIdempotentPurchaseTwoStep-->>PurchaseCommitment: abort with error
else No match
FindReservationOrderByIdempotencyToken-->>DoIdempotentPurchaseTwoStep: not found
DoIdempotentPurchaseTwoStep->>DoPurchaseTwoStep: proceed to purchase
end
else Token absent
DoIdempotentPurchaseTwoStep->>DoPurchaseTwoStep: skip lookup, direct purchase
end
DoPurchaseTwoStep->>AzurePurchaseAPI: calculate + purchase
AzurePurchaseAPI-->>DoPurchaseTwoStep: new orderID
DoPurchaseTwoStep-->>DoIdempotentPurchaseTwoStep: orderID
DoIdempotentPurchaseTwoStep-->>PurchaseCommitment: orderID
PurchaseCommitment-->>ServiceClient: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Closes #721 (regression of #641; unblocks #639).
Why Option B (caller-level guard), not Option A (client-supplied order ID)
The package docstring at
purchase.go:19-26documents that Azure rejects client-suppliedreservationOrderIdin the two-step flow — that's the very reason PR #680 had to switch toDoPurchaseTwoStep. Option A (restoreIdempotencyGUID-derived order IDs from PR #653) is therefore not viable. Option B mirrors the AWS EC2findRIByIdempotencyTokenpattern: tag every purchase with the deterministic token, look up by tag before purchasing, short-circuit on match.Implementation
Centralized in
providers/azure/services/internal/reservations/purchase.go:ApplyPurchaseTags(body, source, token)— stamps bothpurchase-automationandcudly-idempotency-tokentags into the request body.FindReservationOrderByIdempotencyToken(...)— lists reservation orders viaMicrosoft.Capacity/reservationOrders, filters by tag, skips terminal-failed states (Cancelled/Failed/Expired), followsnextLinkpagination. Split intofetchReservationOrdersPage+matchReservationOrderInPageto stay under gocyclo:10.DoIdempotentPurchaseTwoStep(...)wrapsDoPurchaseTwoStep: empty token falls through (CLI path preserved); non-empty token does the lookup first and short-circuits on match; lookup failure REFUSES to purchase rather than risk a double-buy (mirrors EC2 safety contract).All 7 service executors (cache, compute, cosmosdb, database, managedredis, search, synapse) now call
reservations.ApplyPurchaseTags(...)andreservations.DoIdempotentPurchaseTwoStep(...). Duplicated per-serviceapplyPurchaseAutomationTaghelpers deleted.Tests (28 new)
TestApplyPurchaseTags_BothTags / OnlySource / OnlyIdempotency / NoTagsTestFindReservationOrderByIdempotencyToken_Match / NoMatch / SkipsTerminalFailed (×3) / AcceptsInFlightStates (×5) / HTTPError / 403 / EmptyToken / PaginatedFollowsNextLinkTestDoIdempotentPurchaseTwoStep_EmptyToken_NoLookup / NoMatch_FallsThroughToPurchase / **Match_ShortCircuits** (the #721 regression test — zero POST calls on re-drive) / LookupFailure_DoesNotPurchase / DifferentTokens_DistinctReservations / PreservesTwoStepFlowTestBuildReservationBody_IncludesIdempotencyTokenTag(compute-level)Verification
go test ./...fromproviders/azure/: all 12 packages passgo test ./internal/purchase/...from root: passesgo vet+ gofmt + gocyclo (post-refactor) cleanOnce this lands, #639 (full auto-re-drive flip across all providers) becomes safely actionable.
Summary by CodeRabbit
New Features
Bug Fixes
Tests