feat(purchases): make Azure reservation purchases idempotent (refs #641)

pkg/common/tokens.go

@@ -5,6 +5,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"strings"
 )
 
 // GenerateApprovalToken returns a 32-byte cryptographically secure random
@@ -58,3 +59,37 @@ func MaskToken(token string) string {
 	}
 	return token[:8] + "..."
 }
+
+// IdempotencyGUID formats an idempotency token as a deterministic canonical GUID
+// (8-4-4-4-12 lowercase hex) for use as an Azure reservationOrderID (issue #641).
+// The Azure Reservations API path is reservationOrders/{guid} and a PUT is
+// idempotent on a stable order ID, so deriving the GUID from the token makes a
+// re-drive re-PUT the same order rather than create a second reservation.
+//
+// It uses the first 32 hex characters (128 bits) of the token, which is itself a
+// SHA-256 hex digest, so the GUID is deterministic and collision-free at any
+// realistic purchase volume. Returns "" when token is shorter than 32 hex chars
+// (e.g. empty) so callers keep their prior non-idempotent ID behaviour.
+func IdempotencyGUID(token string) string {
+	if len(token) < 32 {
+		return ""
+	}
+	h := strings.ToLower(token[:32])
+	if _, err := hex.DecodeString(h); err != nil {
+		return ""
+	}
+	return fmt.Sprintf("%s-%s-%s-%s-%s", h[0:8], h[8:12], h[12:16], h[16:20], h[20:32])
+}
+
+// ReservationOrderID returns the Azure reservationOrderID to PUT for a purchase:
+// the deterministic GUID derived from token when one is supplied (issue #641, so
+// a re-drive re-PUTs the same idempotent order), otherwise fallback (the caller's
+// prior non-idempotent ID, e.g. a random GUID or a timestamp). Centralising the
+// choice keeps each executor's PurchaseCommitment a single statement and avoids
+// repeating the same empty-token guard across every Azure service.
+func ReservationOrderID(token, fallback string) string {
+	if guid := IdempotencyGUID(token); guid != "" {
+		return guid
+	}
+	return fallback
+}

pkg/common/tokens_test.go

@@ -85,3 +85,40 @@ func TestMaskToken_EmptyAndShort(t *testing.T) {
 	assert.Equal(t, "12345678", MaskToken("12345678"), "exactly 8 chars is returned unchanged")
 	assert.Equal(t, "12345678...", MaskToken("123456789"), "9 chars is truncated to 8 + ellipsis")
 }
+
+func TestIdempotencyGUID_DeterministicCanonicalForm(t *testing.T) {
+	token := DeriveIdempotencyToken("exec-1", 0)
+
+	a := IdempotencyGUID(token)
+	b := IdempotencyGUID(token)
+	assert.Equal(t, a, b, "same token must yield the same GUID")
+
+	// Canonical 8-4-4-4-12 lowercase-hex GUID shape.
+	assert.Regexp(t, `^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`, a)
+	assert.Len(t, a, 36)
+}
+
+func TestIdempotencyGUID_DistinctTokensDistinctGUIDs(t *testing.T) {
+	g0 := IdempotencyGUID(DeriveIdempotencyToken("exec-1", 0))
+	g1 := IdempotencyGUID(DeriveIdempotencyToken("exec-1", 1))
+	assert.NotEqual(t, g0, g1, "different recs must get different order IDs")
+}
+
+func TestIdempotencyGUID_ShortTokenReturnsEmpty(t *testing.T) {
+	assert.Equal(t, "", IdempotencyGUID(""), "empty token must yield empty so callers keep their fallback")
+	assert.Equal(t, "", IdempotencyGUID("abc"), "sub-32-char token must yield empty")
+}
+
+func TestReservationOrderID(t *testing.T) {
+	token := DeriveIdempotencyToken("exec-1", 0)
+
+	// With a token: deterministic GUID, never the fallback.
+	got := ReservationOrderID(token, "fallback-id")
+	assert.Equal(t, IdempotencyGUID(token), got, "a supplied token must derive the GUID")
+	assert.Equal(t, got, ReservationOrderID(token, "other-fallback"),
+		"same token must yield the same order ID regardless of fallback")
+
+	// No usable token: caller's fallback is returned verbatim.
+	assert.Equal(t, "fallback-id", ReservationOrderID("", "fallback-id"))
+	assert.Equal(t, "fallback-id", ReservationOrderID("abc", "fallback-id"))
+}

providers/azure/services/cache/client.go

@@ -273,7 +273,10 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme
 		Timestamp:      time.Now(),
 	}
 
-	reservationOrderID := uuid.New().String()
+	// Derive a deterministic reservationOrderID from the idempotency token (issue
+	// #641) so a re-drive re-PUTs the same idempotent Azure reservation order
+	// instead of creating a second; fall back to a random GUID otherwise.
+	reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String())
 	apiVersion := "2022-11-01"
 	purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s",
 		reservationOrderID, apiVersion)

providers/azure/services/compute/client.go

@@ -457,7 +457,12 @@ func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recom
 		return result, result.Error
 	}
 
-	reservationOrderID := uuid.New().String()
+	// When an idempotency token is supplied (issue #641) the reservationOrderID is
+	// derived deterministically from it. The Azure Reservations API PUTs to
+	// reservationOrders/{id} and is idempotent on a stable order ID, so a re-drive
+	// re-PUTs the same order and returns the existing reservation rather than
+	// creating a second. Otherwise mint a random GUID (prior behaviour).
+	reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String())
 	purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=2022-11-01",
 		reservationOrderID)
 

providers/azure/services/compute/client_test.go

@@ -566,6 +566,70 @@ func TestComputeClient_PurchaseCommitment_BadStatus(t *testing.T) {
 	assert.Contains(t, err.Error(), "reservation purchase failed with status 400")
 }
 
+// purchaseURLFromMock returns the reservation PUT URL captured by the mock HTTP
+// client for the last successful PurchaseCommitment call. The Azure Reservations
+// API PUTs to .../reservationOrders/{id}; the {id} segment is the order ID that
+// makes the request idempotent.
+func purchaseURLFromMock(t *testing.T, m *mocks.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 {
+			return req.URL.String()
+		}
+	}
+	t.Fatalf("no HTTP request captured by mock")
+	return ""
+}
+
+// TestComputeClient_PurchaseCommitment_IdempotentReDrive is the issue #641
+// regression test: a re-drive with the *same* IdempotencyToken must not create a
+// second reservation. Because the Azure Reservations API PUTs to
+// reservationOrders/{id} and is idempotent on a stable order ID, "no second
+// reservation" is proven by the two re-drives PUTting to the *same*
+// reservationOrders/{id} URL (and yielding the same CommitmentID). A distinct
+// token must target a distinct order so unrelated purchases never collide.
+func TestComputeClient_PurchaseCommitment_IdempotentReDrive(t *testing.T) {
+	ctx := context.Background()
+	rec := common.Recommendation{
+		ResourceType:   "Standard_D2s_v3",
+		Term:           "1yr",
+		Count:          1,
+		CommitmentCost: 2000.0,
+	}
+	token := common.DeriveIdempotencyToken("exec-641", 0)
+
+	purchase := func(tok string) (common.PurchaseResult, string) {
+		mockHTTP := &mocks.MockHTTPClient{}
+		mockCred := &MockTokenCredential{token: "test-token"}
+		client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP)
+		mockHTTP.On("Do", mock.Anything).Return(
+			mocks.CreateMockHTTPResponse(http.StatusOK, `{"id": "reservation-123"}`), nil)
+		result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{IdempotencyToken: tok})
+		require.NoError(t, err)
+		require.True(t, result.Success)
+		return result, purchaseURLFromMock(t, mockHTTP)
+	}
+
+	first, firstURL := purchase(token)
+	second, secondURL := purchase(token)
+
+	// Same token => same idempotent order ID => same PUT URL => Azure re-PUTs the
+	// existing order rather than minting a second reservation.
+	assert.Equal(t, first.CommitmentID, second.CommitmentID,
+		"same idempotency token must reuse the same reservationOrderID")
+	assert.Equal(t, firstURL, secondURL,
+		"re-drive must PUT to the same reservationOrders/{id} URL")
+	assert.Contains(t, firstURL, common.IdempotencyGUID(token),
+		"order ID must be derived deterministically from the token")
+
+	// Distinct token => distinct order so independent purchases don't collide.
+	other, otherURL := purchase(common.DeriveIdempotencyToken("exec-641", 1))
+	assert.NotEqual(t, first.CommitmentID, other.CommitmentID,
+		"different recs must get different reservation orders")
+	assert.NotEqual(t, firstURL, otherURL)
+}
+
 // TestComputeClient_ConvertAzureVMRecommendation_NilGuards pins the new
 // contract: unusable SDK payloads (nil, wrong concrete type, nil Properties)
 // produce a nil *Recommendation so the caller can filter it out. Before

providers/azure/services/cosmosdb/client.go

@@ -266,8 +266,11 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco
 		Timestamp:      time.Now(),
 	}
 
-	// Build reservation purchase request
-	reservationOrderID := uuid.New().String()
+	// Build reservation purchase request. Derive a deterministic
+	// reservationOrderID from the idempotency token (issue #641) so a re-drive
+	// re-PUTs the same idempotent Azure reservation order instead of creating a
+	// second; fall back to a random GUID otherwise.
+	reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String())
 
 	// Construct the Azure Reservations API request
 	apiVersion := "2022-11-01"

providers/azure/services/database/client.go

@@ -274,8 +274,11 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco
 		Timestamp:      time.Now(),
 	}
 
-	// Build reservation purchase request
-	reservationOrderID := uuid.New().String()
+	// Build reservation purchase request. Derive a deterministic
+	// reservationOrderID from the idempotency token (issue #641) so a re-drive
+	// re-PUTs the same idempotent Azure reservation order instead of creating a
+	// second; fall back to a random GUID otherwise.
+	reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String())
 
 	// Construct the Azure Reservations API request
 	apiVersion := "2022-11-01"

providers/azure/services/search/client.go

@@ -16,6 +16,7 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/search/armsearch"
+	"github.com/google/uuid"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
 	"github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient"
@@ -244,7 +245,11 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm
 		Timestamp:      time.Now(),
 	}
 
-	reservationOrderID := fmt.Sprintf("search-reservation-%d", time.Now().Unix())
+	// Derive a deterministic reservationOrderID from the idempotency token (issue
+	// #641) so a re-drive re-PUTs the same idempotent Azure reservation order
+	// instead of creating a second; fall back to the prior timestamped ID
+	// otherwise.
+	reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String())
 	apiVersion := "2022-11-01"
 	purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s",
 		reservationOrderID, apiVersion)

providers/azure/services/search/client_test.go

@@ -804,3 +804,60 @@ func TestSearchClient_ValidateOffering_Invalid(t *testing.T) {
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "invalid Azure Search SKU")
 }
+
+// searchPurchaseURLFromMock returns the reservation PUT URL captured by the mock
+// HTTP client for the last PurchaseCommitment call. The {id} segment of
+// .../reservationOrders/{id} is the order ID that makes the request idempotent.
+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 {
+			return req.URL.String()
+		}
+	}
+	t.Fatalf("no HTTP request captured by mock")
+	return ""
+}
+
+// TestSearchClient_PurchaseCommitment_IdempotentReDrive is the issue #641
+// regression test for the search executor, whose prior reservationOrderID was a
+// non-idempotent timestamp ("search-reservation-<unix>"). A re-drive with the
+// same IdempotencyToken must now PUT to the same reservationOrders/{id} URL so
+// Azure re-PUTs the existing order rather than creating a second reservation.
+func TestSearchClient_PurchaseCommitment_IdempotentReDrive(t *testing.T) {
+	ctx := context.Background()
+	rec := common.Recommendation{
+		ResourceType:   "standard",
+		Term:           "1yr",
+		Count:          1,
+		CommitmentCost: 3000.0,
+	}
+	token := common.DeriveIdempotencyToken("exec-641", 0)
+
+	purchase := func(tok string) (common.PurchaseResult, string) {
+		mockHTTP := &MockHTTPClient{}
+		mockCred := &MockTokenCredential{token: "test-token"}
+		client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP)
+		mockHTTP.On("Do", mock.Anything).Return(
+			createMockHTTPResponse(http.StatusOK, `{"id": "reservation-123"}`), nil)
+		result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{IdempotencyToken: tok})
+		require.NoError(t, err)
+		require.True(t, result.Success)
+		return result, searchPurchaseURLFromMock(t, mockHTTP)
+	}
+
+	first, firstURL := purchase(token)
+	second, secondURL := purchase(token)
+
+	assert.Equal(t, first.CommitmentID, second.CommitmentID,
+		"same idempotency token must reuse the same reservationOrderID")
+	assert.Equal(t, firstURL, secondURL,
+		"re-drive must PUT to the same reservationOrders/{id} URL")
+	assert.Contains(t, firstURL, common.IdempotencyGUID(token),
+		"order ID must be derived deterministically from the token, not a timestamp")
+
+	other, otherURL := purchase(common.DeriveIdempotencyToken("exec-641", 1))
+	assert.NotEqual(t, first.CommitmentID, other.CommitmentID)
+	assert.NotEqual(t, firstURL, otherURL)
+}