sec(azure): re-check idempotency between in-process purchase retries

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.

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")
+}