From c17d5ec870126674a86355335b0bc66e9664dc86 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 9 Aug 2026 04:24:31 +0200 Subject: [PATCH] sec(azure): re-check idempotency between in-process purchase retries 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) --- .../internal/reservations/purchase.go | 136 ++++++++++++---- .../internal/reservations/purchase_test.go | 146 +++++++++++++++++- 2 files changed, 252 insertions(+), 30 deletions(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index fdb4da3c8..81462181a 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -30,6 +30,9 @@ // idempotency token, so a re-driven purchase of a stranded execution (issue // #636) reuses the prior reservation rather than buying a second one. Mirrors // the AWS EC2 findRIByIdempotencyToken pattern (providers/aws/services/ec2). +// The same lookup repeats inside the retry loop before each attempt after the +// first, because every retry mints a new order ID and would otherwise be blind +// to a first attempt that committed before reporting failure (issue #1774). // // This package exposes IsSessionTimeout to let callers classify errors, // DoPurchaseTwoStep (the raw two-step flow without the dedupe guard, retained @@ -228,12 +231,12 @@ const sessionTimeoutFragment = "Session timed out" // commit-ambiguous status is a double-buy risk that buys nothing -- every such // case keeps its pre-existing behavior, which nobody has reported as broken. // -// Note what does NOT bound this: DoIdempotentPurchaseTwoStep performs its -// idempotency lookup exactly once, before DoPurchaseTwoStep is entered, and -// there is no recheck between attempts. Each retry mints a fresh -// reservationOrderId via doCalculatePrice, so the guard covers a re-drive -// ACROSS invocations, not the loop within one. The bound here is -// purchaseMaxAttempts. +// What bounds a retry here: when an idempotency token is present, the loop +// re-checks before every attempt after the first whether the previous one +// committed despite reporting failure, and returns the existing order rather +// than buying again (issue #1774). Without a token -- the bare +// DoPurchaseTwoStep entry point -- no such check is possible and the only +// bound is purchaseMaxAttempts. var errRetryabilityUnknown = errors.New("purchase retryability unknown: response body did not read completely") // IsSessionTimeout reports whether err looks like the "Session timed out" @@ -276,7 +279,85 @@ const ( // Returns the Azure-minted reservationOrderId on success, which the caller // should store as the CommitmentID. func DoPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL string, bodyBytes []byte, bearerToken string) (string, error) { + // No idempotency token, so no re-check is possible between attempts. This + // entry point is retained for callers that have none; every service executor + // goes through DoIdempotentPurchaseTwoStep. + return purchaseTwoStepGuarded(ctx, httpClient, calcURL, bodyBytes, bearerToken, "") +} + +// purchaseIsRetryable reports whether a failed purchase attempt should be +// retried. Two triggers: a session timeout Azure names in the response body, +// and a 400 whose body did not read completely, where the first cannot be ruled +// out because the fragment it matches on may never have arrived +// (errRetryabilityUnknown). Attempts beyond purchaseMaxAttempts are never +// retryable regardless of cause. +func purchaseIsRetryable(purchaseErr error, attempt int) bool { + if attempt >= purchaseMaxAttempts { + return false + } + return IsSessionTimeout(purchaseErr) || errors.Is(purchaseErr, errRetryabilityUnknown) +} + +// waitBeforeRetry sleeps the inter-attempt delay, honoring cancellation so a +// canceled context does not sit out the full wait before failing. +func waitBeforeRetry(ctx context.Context) error { + select { + case <-time.After(purchaseRetryDelay): + return nil + case <-ctx.Done(): + return fmt.Errorf("reservation purchase canceled during retry delay: %w", ctx.Err()) + } +} + +// recheckAlreadyPurchased asks, before a retry, whether the attempt that just +// failed nevertheless committed on Azure's side. It is a no-op on the first +// attempt and when no idempotency token is available. +// +// Extracted rather than inlined so purchaseTwoStepGuarded stays under the +// gocyclo:10 threshold the pre-merge check enforces, matching what +// fetchReservationOrdersPage does for the pagination loop. +// +// A lookup that cannot be completed returns an error rather than a not-found, +// so the caller refuses the retry. Treating an unusable lookup as "no existing +// order" would reinstate exactly the fall-through this guard exists to prevent. +func recheckAlreadyPurchased(ctx context.Context, httpClient HTTPClient, bearerToken, idempotencyToken string, attempt int) (orderID string, found bool, err error) { + if attempt == 1 || idempotencyToken == "" { + return "", false, nil + } + existingID, found, err := FindReservationOrderByIdempotencyToken(ctx, httpClient, bearerToken, idempotencyToken) + if err != nil { + return "", false, fmt.Errorf( + "idempotency re-check before purchase retry %d/%d failed (refusing to retry to avoid a possible double-buy): %w", + attempt, purchaseMaxAttempts, err) + } + if found { + log.Printf("reservation order for idempotency token %s already exists (%s) after attempt %d; the previous attempt committed despite reporting failure, so not purchasing again (issue #1774)", + common.MaskToken(idempotencyToken), existingID, attempt-1) + } + return existingID, found, nil +} + +// purchaseTwoStepGuarded is the retry loop shared by both entry points. When +// idempotencyToken is non-empty it re-checks, before every attempt after the +// first, whether the previous attempt committed despite reporting failure. +// +// This closes the gap in issue #1774. The pre-loop lookup in +// DoIdempotentPurchaseTwoStep catches a re-drive ACROSS invocations, but each +// retry inside this loop mints a NEW reservationOrderId via doCalculatePrice, so +// nothing linked attempt N+1 to a purchase that attempt N may have committed +// before failing. The re-check is what makes the loop safe to enter at all. +// +// Ordering matters: the check runs before doCalculatePrice, so a purchase that +// already exists short-circuits without minting yet another order ID. +func purchaseTwoStepGuarded(ctx context.Context, httpClient HTTPClient, calcURL string, bodyBytes []byte, bearerToken, idempotencyToken string) (string, error) { for attempt := 1; attempt <= purchaseMaxAttempts; attempt++ { + existingID, found, err := recheckAlreadyPurchased(ctx, httpClient, bearerToken, idempotencyToken, attempt) + if err != nil { + return "", err + } + if found { + return existingID, nil + } // Step 1: calculatePrice -- mint a session-bound reservationOrderId. orderID, err := doCalculatePrice(ctx, httpClient, calcURL, bodyBytes, bearerToken) if err != nil { @@ -289,22 +370,20 @@ func DoPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL strin return orderID, nil } - // An unreadable body is retried alongside a recognized session timeout: - // without this the truncated case was abandoned on its first attempt, - // because IsSessionTimeout can only classify what it can read. - if (IsSessionTimeout(purchaseErr) || errors.Is(purchaseErr, errRetryabilityUnknown)) && attempt < purchaseMaxAttempts { - log.Printf("reservation purchase session timed out (attempt %d/%d), re-running calculatePrice in %s", - attempt, purchaseMaxAttempts, purchaseRetryDelay) - select { - case <-time.After(purchaseRetryDelay): - case <-ctx.Done(): - return "", fmt.Errorf("reservation purchase canceled during retry delay: %w", ctx.Err()) - } - continue + if !purchaseIsRetryable(purchaseErr, attempt) { + return "", purchaseErr + } + log.Printf("reservation purchase retryable (attempt %d/%d), re-running calculatePrice in %s", + attempt, purchaseMaxAttempts, purchaseRetryDelay) + if err := waitBeforeRetry(ctx); err != nil { + return "", err } - return "", purchaseErr } - return "", fmt.Errorf("reservation purchase failed after %d attempts (session timeout)", purchaseMaxAttempts) + // Unreachable: purchaseIsRetryable is false once attempt == purchaseMaxAttempts, + // so the final iteration always returns purchaseErr from inside the loop. Kept + // because the compiler cannot prove that, and worded generally rather than + // naming session timeout, which is now only one of two retry triggers. + return "", fmt.Errorf("reservation purchase exhausted %d attempts", purchaseMaxAttempts) } // doCalculatePrice calls the calculatePrice endpoint and returns the @@ -469,15 +548,20 @@ func matchReservationOrderInPage(page *reservationOrdersListResponse, idempotenc // the token. If found, short-circuit and return its order ID -- this is a // re-drive of an execution that already created the reservation; buying // again would double-charge the customer (issues #641, #721). -// 3. Otherwise, call DoPurchaseTwoStep. The caller is responsible for having +// 3. Otherwise, run the two-step flow. The caller is responsible for having // stamped the idempotency tag into bodyBytes via ApplyPurchaseTags so the // resulting order is tagged and the NEXT re-drive will short-circuit at // step 2. // -// A failed lookup must NOT fall through to a purchase: doing so would defeat -// the guard and risk a double-buy on a re-drive. The lookup error is returned -// verbatim, and the recovery sweep treats the recommendation as not-yet- -// purchased and retries the whole guarded path (mirroring the EC2 EC2 +// The same lookup runs again inside the retry loop before every attempt after +// the first, so a first attempt that committed before reporting failure is +// found rather than duplicated (issue #1774). Step 2 covers a re-drive ACROSS +// invocations; that in-loop check covers the loop within one. +// +// A failed lookup must NOT fall through to a purchase, in either position: +// doing so would defeat the guard and risk a double-buy. The lookup error is +// returned verbatim, and the recovery sweep treats the recommendation as +// not-yet-purchased and retries the whole guarded path (mirroring the EC2 // findRIByIdempotencyToken safety contract in providers/aws/services/ec2). func DoIdempotentPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL string, bodyBytes []byte, bearerToken, idempotencyToken string) (string, error) { if idempotencyToken != "" { @@ -490,7 +574,7 @@ func DoIdempotentPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, cal return existingID, nil } } - return DoPurchaseTwoStep(ctx, httpClient, calcURL, bodyBytes, bearerToken) + return purchaseTwoStepGuarded(ctx, httpClient, calcURL, bodyBytes, bearerToken, idempotencyToken) } // doPurchase calls the purchase endpoint with the given body. diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go index d35418995..9ade0273f 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -650,10 +650,23 @@ func TestDoIdempotentPurchaseTwoStep_PreservesTwoStepFlow(t *testing.T) { sessionTimeoutBody := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` - // Lookup: no match. - m.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.Method == http.MethodGet - })).Return(fakeResp(http.StatusOK, `{"value":[]}`), nil).Once() + // Lookup: no match. Registered TWICE, each with its own response, because + // the flow now performs two lookups: once before the flow, and again before + // the retry as the in-loop re-check added for issue #1774. Both report no + // existing order, so the retry proceeds and the two-step flow is preserved. + // + // Two .Once() registrations rather than one .Times(2): fakeResp wraps a + // bytes.Buffer, so testify handing back the same *http.Response twice would + // serve an already-drained body to the second caller and fail the decode. + for i := 0; i < 2; i++ { + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet + })).Return(&http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(`{"value":[]}`)), + Header: make(http.Header), + }, nil).Once() + } // calculatePrice #1. m.On("Do", mock.MatchedBy(func(r *http.Request) bool { return r.Method == http.MethodPost && r.URL.String() == calcURL @@ -1011,3 +1024,128 @@ func TestDoPurchaseTwoStep_TruncatedNon400DoesNotRetry(t *testing.T) { "a truncated 500 must be attempted exactly once; retrying it risks re-purchasing after "+ "a commit that the once-only idempotency lookup cannot see (issue #1774)") } + +// idempotencyRetryClient drives the real retry loop while counting each kind of +// call, so tests can assert on what actually reached Azure rather than on an +// error message. listResp is re-evaluated per call because the list result must +// be able to change between attempts -- that is the whole scenario. +type idempotencyRetryClient struct { + purchases int + calcs int + lists int + listResp func(callNo int) (*http.Response, error) + purchase func(callNo int) *http.Response +} + +func (c *idempotencyRetryClient) Do(req *http.Request) (*http.Response, error) { + switch { + case req.URL.String() == ReservationOrdersListURL(): + c.lists++ + return c.listResp(c.lists) + case req.URL.String() == calcURL: + c.calcs++ + return fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"order-`+itoa(c.calcs)+`"}}`), nil + default: + c.purchases++ + return c.purchase(c.purchases), nil + } +} + +func itoa(i int) string { return string(rune('0' + i)) } + +// ordersListWith renders a reservation-orders page containing one order tagged +// with the given idempotency token, in the shape matchReservationOrderInPage +// expects. +func ordersListWith(orderID, token string) string { + return `{"value":[{"name":"` + orderID + `","properties":{"provisioningState":"Succeeded",` + + `"reservationsProperties":{"userFriendlyAppliedScopeType":"Shared"}},` + + `"tags":{"cudly-idempotency-token":"` + token + `"}}]}` +} + +const emptyOrdersList = `{"value":[]}` + +// TestPurchaseRetry_DoesNotDoubleBuyWhenFirstAttemptCommitted is the regression +// test for issue #1774. +// +// The first purchase attempt reports a retryable session timeout, but it had in +// fact committed on Azure's side. Before the in-loop re-check, the loop went +// straight to a second calculatePrice (minting a NEW order ID) and purchased +// again, because the only idempotency lookup happened before the loop was ever +// entered. The bound was purchaseMaxAttempts, not the guard. +func TestPurchaseRetry_DoesNotDoubleBuyWhenFirstAttemptCommitted(t *testing.T) { + const token = "tok-1774" + sessionTimeout := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` + + c := &idempotencyRetryClient{ + // Pre-loop lookup: nothing yet. After attempt 1, the order exists. + listResp: func(n int) (*http.Response, error) { + if n == 1 { + return fakeResp(http.StatusOK, emptyOrdersList), nil + } + return fakeResp(http.StatusOK, ordersListWith("committed-order", token)), nil + }, + purchase: func(int) *http.Response { return fakeResp(http.StatusBadRequest, sessionTimeout) }, + } + + orderID, err := DoIdempotentPurchaseTwoStep(context.Background(), c, calcURL, []byte(testBody), "tok", token) + require.NoError(t, err, "the committed order must be adopted, not treated as a failure") + assert.Equal(t, "committed-order", orderID) + assert.Equal(t, 1, c.purchases, + "exactly one purchase must reach Azure; a second would be a duplicate reservation the "+ + "customer pays for (issue #1774)") + assert.Equal(t, 1, c.calcs, + "the re-check must run BEFORE calculatePrice, so no further order ID is minted once the "+ + "purchase is known to exist") +} + +// TestPurchaseRetry_StillRecoversWhenFirstAttemptGenuinelyFailed is the other +// direction, and it is the one a duplicate-only test would miss: a fix that +// simply refused to retry would pass the test above while silently breaking the +// recovery this retry loop exists for. +func TestPurchaseRetry_StillRecoversWhenFirstAttemptGenuinelyFailed(t *testing.T) { + const token = "tok-1774-recover" + sessionTimeout := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` + + c := &idempotencyRetryClient{ + // The order never appears: attempt 1 genuinely did not commit. + listResp: func(int) (*http.Response, error) { return fakeResp(http.StatusOK, emptyOrdersList), nil }, + purchase: func(n int) *http.Response { + if n == 1 { + return fakeResp(http.StatusBadRequest, sessionTimeout) + } + return fakeResp(http.StatusOK, `{}`) + }, + } + + orderID, err := DoIdempotentPurchaseTwoStep(context.Background(), c, calcURL, []byte(testBody), "tok", token) + require.NoError(t, err, "a genuine session timeout must still be recovered by the retry") + assert.Equal(t, "order-2", orderID, "the second attempt's freshly minted order ID must be returned") + assert.Equal(t, 2, c.purchases, "the retry must actually happen") + assert.Equal(t, 2, c.calcs, "each attempt mints its own session-bound order ID") +} + +// TestPurchaseRetry_RefusesRetryWhenRecheckLookupFails pins the fail-closed +// property in its new position. The pre-loop guard already refuses to purchase +// when its lookup errors; the in-loop check must behave identically, or it +// would reintroduce the fall-through it exists to prevent. +func TestPurchaseRetry_RefusesRetryWhenRecheckLookupFails(t *testing.T) { + const token = "tok-1774-lookup-fail" + sessionTimeout := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` + + c := &idempotencyRetryClient{ + listResp: func(n int) (*http.Response, error) { + if n == 1 { + return fakeResp(http.StatusOK, emptyOrdersList), nil + } + return fakeResp(http.StatusInternalServerError, `{"error":"list unavailable"}`), nil + }, + purchase: func(int) *http.Response { return fakeResp(http.StatusBadRequest, sessionTimeout) }, + } + + _, err := DoIdempotentPurchaseTwoStep(context.Background(), c, calcURL, []byte(testBody), "tok", token) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to retry", + "an unusable re-check must stop the retry rather than proceed blind") + assert.Equal(t, 1, c.purchases, + "no second purchase may be attempted when the re-check could not be completed") +}