feat(purchases): make Azure reservation purchases idempotent (refs #641) - #653
Merged
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements deterministic idempotency for Azure reservation order purchases by introducing two helper functions that convert idempotency tokens into canonical GUIDs, then applies these helpers across five Azure service clients to derive reservation order IDs instead of using random or timestamp-based values, preventing duplicate orders on re-drives. ChangesAzure Reservation Purchase Idempotency
Sequence DiagramsequenceDiagram
participant Request as Purchase Request
participant PurchaseCommitment as PurchaseCommitment()
participant ReservationOrderID as ReservationOrderID()
participant IdempotencyGUID as IdempotencyGUID()
participant Azure as Azure Reservations API
Request->>PurchaseCommitment: opts.IdempotencyToken
PurchaseCommitment->>ReservationOrderID: token, fallback UUID
ReservationOrderID->>IdempotencyGUID: extract first 32 hex chars
IdempotencyGUID-->>ReservationOrderID: canonical GUID or empty
ReservationOrderID-->>PurchaseCommitment: deterministic or random ID
PurchaseCommitment->>Azure: PUT /reservationOrders/{id}
Azure-->>PurchaseCommitment: order created/updated
PurchaseCommitment-->>Request: CommitmentID
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
providers/azure/services/search/client_test.go (1)
813-820: ⚡ Quick winHarden URL capture to only match reservation purchase PUT requests.
Line 814-Line 817 currently returns the last captured request regardless of endpoint/method. Filtering to
PUT+reservationOrdersmakes this regression test resilient to future extra HTTP calls inPurchaseCommitment.♻️ Proposed hardening
import ( "bytes" "context" "errors" "io" "net/http" + "strings" "testing" "time" @@ func searchPurchaseURLFromMock(t *testing.T, m *MockHTTPClient) string { t.Helper() for i := len(m.Calls) - 1; i >= 0; i-- { req, ok := m.Calls[i].Arguments.Get(0).(*http.Request) - if ok && req != nil { + if ok && req != nil && + req.Method == http.MethodPut && + strings.Contains(req.URL.Path, "/reservationOrders/") { return req.URL.String() } } - t.Fatalf("no HTTP request captured by mock") + t.Fatalf("no reservation purchase PUT request captured by mock") return "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/search/client_test.go` around lines 813 - 820, The current test helper iterates m.Calls and returns the last HTTP request regardless of endpoint; update the loop in the test (the block using m.Calls and returning req.URL.String()) to only return the URL for requests that match req.Method == "PUT" and whose URL path or string contains "reservationOrders" (so it selects the reservation purchase PUT request), and keep the existing t.Fatalf("no HTTP request captured by mock") fallback if none match; this change localizes the check to the reservation purchase call used by PurchaseCommitment and prevents unrelated mock calls from being returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/common/tokens.go`:
- Around line 55-61: IdempotencyGUID is formatting any 32+ character string into
a GUID without validating or normalizing it; update IdempotencyGUID to first
extract the first 32 characters, verify all characters are valid hex (0-9,
a-f/A-F), convert them to canonical lowercase, and only then format into the
dash-separated GUID; if validation fails return an empty string. Ensure you
modify the IdempotencyGUID function and use the validated/normalized 32-char hex
slice when building the final formatted string.
In `@providers/azure/services/search/client.go`:
- Around line 251-252: The timestamp-based fallback used when building
reservationOrderID (calling common.ReservationOrderID with opts.IdempotencyToken
and fmt.Sprintf("search-reservation-%d", time.Now().Unix())) is prone to
collisions; replace the fmt.Sprintf timestamp fallback with a
collision-resistant identifier (e.g., a UUID via uuid.NewString() or a
cryptographically-random hex string) so concurrent non-token purchases produce
unique fallback IDs before calling common.ReservationOrderID.
---
Nitpick comments:
In `@providers/azure/services/search/client_test.go`:
- Around line 813-820: The current test helper iterates m.Calls and returns the
last HTTP request regardless of endpoint; update the loop in the test (the block
using m.Calls and returning req.URL.String()) to only return the URL for
requests that match req.Method == "PUT" and whose URL path or string contains
"reservationOrders" (so it selects the reservation purchase PUT request), and
keep the existing t.Fatalf("no HTTP request captured by mock") fallback if none
match; this change localizes the check to the reservation purchase call used by
PurchaseCommitment and prevents unrelated mock calls from being returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 463be8d0-5f78-41c6-97e4-9d9bc2d2923d
📒 Files selected for processing (9)
pkg/common/tokens.gopkg/common/tokens_test.goproviders/azure/services/cache/client.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/database/client.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.go
cristim
added a commit
that referenced
this pull request
May 22, 2026
… fallback IdempotencyGUID now validates the first 32 characters are valid lowercase hex before formatting them as a GUID, returning "" for any non-hex input rather than silently producing a malformed GUID. Azure search PurchaseCommitment replaces the Unix-second timestamp fallback with uuid.New().String() so the fallback is collision-resistant at sub-second granularity rather than guaranteed to collide on burst retries. Co-authored-by: CodeRabbit CR #653
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Extends the #636 IdempotencyToken mechanism to all five Azure reservation executors (cache, compute, cosmosdb, database, search) so a re-drive (and Each executor previously minted a fresh reservationOrderID per call (uuid.New(), or a time.Now() timestamp in search), so a retry created a second reservation. The Azure Reservations API PUTs to reservationOrders/{id} and is idempotent on a stable order ID, so the order ID is now derived deterministically from the IdempotencyToken via common.IdempotencyGUID: the same token re-PUTs the same order and returns the existing reservation rather than creating a second. An empty/short token falls back to the prior non-idempotent ID, preserving behaviour for non-execution callers. Adds common.IdempotencyGUID (canonical 8-4-4-4-12 GUID from the first 128 bits of the SHA-256 token) and common.ReservationOrderID, a single helper that returns the derived GUID or the caller's fallback so each executor stays a one-statement assignment (keeps gocyclo under threshold and avoids repeating the empty-token guard across services). Per-provider regression tests on compute and search assert that a same-token re-drive PUTs to the identical reservationOrders/{id} URL (no second reservation) while a distinct token targets a distinct order. Refs #641.
… fallback IdempotencyGUID now validates the first 32 characters are valid lowercase hex before formatting them as a GUID, returning "" for any non-hex input rather than silently producing a malformed GUID. Azure search PurchaseCommitment replaces the Unix-second timestamp fallback with uuid.New().String() so the fallback is collision-resistant at sub-second granularity rather than guaranteed to collide on burst retries. Co-authored-by: CodeRabbit CR #653
cristim
force-pushed
the
fix/641-azure-idem
branch
from
May 22, 2026 11:07
519f2a1 to
ab06a17
Compare
cristim
added a commit
that referenced
this pull request
May 22, 2026
GCP Compute CUD creation named commitments cud-<unix-second> and ignored opts.IdempotencyToken, so a re-drive of the same execution (issue #639's recovery sweep) would create a SECOND committed-use discount: a financial double-purchase. Thread opts.IdempotencyToken into RegionCommitments.Insert via two deterministic mechanisms so the same token can never buy twice: - RequestId: GCP's native server-side idempotency key on Insert, which the API documents as preventing duplicate commitments. It must be a valid non-zero UUID, so we format the SHA-256 token into a canonical UUID with the existing common.IdempotencyGUID helper (the same derivation PR #653 used for the Azure reservationOrderID). The same token always yields the same RequestId, so the second Insert is a server-side no-op. - Name: also derived from the token (cud-<first-32-hex>, RFC1035-valid) as defense in depth. Commitment names are unique per project+region, so a re-drive that somehow reached Insert collides on the name and GCP rejects it with ALREADY_EXISTS rather than creating a duplicate. An empty token preserves the prior non-idempotent timestamp-based name (the CLI path, which has no owning execution). The token is masked in logs via common.MaskToken (never logged verbatim), matching PR #652. Adds a re-drive regression test mirroring the AWS/Azure ones: the same token on a second PurchaseCommitment yields an identical RequestId and commitment name, plus an empty-token test confirming the CLI path stays non-idempotent. Scope is computeengine only; CloudSQL/Storage/Memorystore were made advisory-only in #649 and are untouched. Closes #654 refs #641
5 tasks
This was referenced May 25, 2026
cristim
added a commit
that referenced
this pull request
May 26, 2026
…eTwoStep (#729) 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes Azure reservation purchases idempotent so a re-drive / scheduler retry cannot create a duplicate reservation order. Partially addresses #641 (Azure slice). Stacks on #638 (
feat/idempotent-commitment-creation) — merge #638 first.Mechanism + double-buy argument
Previously every Azure purchase minted a fresh random
reservationOrderID(uuid.New()), so a re-drive created a second order. NowreservationOrderIDis derived deterministically fromopts.IdempotencyTokenvia a sharedcommon.ReservationOrderID(token, fallback)helper (used across all 5 Azure clients). The Reservations API PUTs toreservationOrders/{id}and is idempotent on a stable ID, so a re-drive re-PUTs the same order rather than creating a new one.A shared helper de-dupes the empty-token guard across the 5 clients (also drops cyclomatic complexity under the gocyclo threshold).
Test plan
IdempotencyGUID/ReservationOrderIDunit tests + per-service re-drive regression (same token -> identical reservationOrder URL; distinct tokens -> distinct orders)Partially addresses #641 (AWS-other slice + GCP Compute tracked separately).
Summary by CodeRabbit
Bug Fixes
Tests