fix(azure/savingsplans)!: refuse a malformed idempotency token before the purchase PUT

pkg/common/tokens.go

@@ -106,21 +106,6 @@ func IdempotencyGUID(token string) string {
 	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). Centralizing the
-// choice keeps each executor's PurchaseCommitment a single statement and avoids
-// repeating the same empty-token guard across every Azure service.
-//
-// Prefer OrderIDFromToken, which rejects a malformed non-empty token; this function is removed once providers/azure switches (#104).
-func ReservationOrderID(token, fallback string) string {
-	if guid := IdempotencyGUID(token); guid != "" {
-		return guid
-	}
-	return fallback
-}
-
 // ErrInvalidIdempotencyToken is returned by OrderIDFromToken when a non-empty
 // token cannot be turned into a reservation order GUID.
 var ErrInvalidIdempotencyToken = errors.New("common: invalid idempotency token")

pkg/common/tokens_test.go

@@ -113,20 +113,6 @@ func TestIdempotencyGUID_ShortTokenReturnsEmpty(t *testing.T) {
 	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"))
-}
-
 func TestOrderIDFromToken(t *testing.T) {
 	hex32 := "0123456789abcdef0123456789abcdef"
 	derived := DeriveIdempotencyToken("exec-1", 0)

providers/azure/go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/search/armsearch v1.4.0
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql v1.2.0
-	github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016163842-4566498f2a08
+	github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017001455-7383c3a7db7e
 	github.com/stretchr/testify v1.11.1
 	golang.org/x/sync v0.23.0
 )

providers/azure/go.sum

@@ -38,8 +38,8 @@ github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1 h1:WJ
 github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1/go.mod h1:tCcJZ0uHAmvjsVYzEFivsRTN00oz5BEsRgQHu5JZ9WE=
 github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0 h1:XRzhVemXdgvJqCH0sFfrBUTnUJSBrBf7++ypk+twtRs=
 github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0/go.mod h1:HKpQxkWaGLJ+D/5H8QRpyQXA1eKjxkFlOMwck5+33Jk=
-github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016163842-4566498f2a08 h1:5gtjJzL+IexMiQ4QvMHYHozLgEqehQUIiiO8rbnu9fs=
-github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016163842-4566498f2a08/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ=
+github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017001455-7383c3a7db7e h1:SeuL9mqUKVNiDa4yXU9IdaKKlwNo/iaaDmDXMHClw+o=
+github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017001455-7383c3a7db7e/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ=
 github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY=
 github.com/aws/aws-sdk-go-v2 v1.41.5/go.mod h1:mwsPRE8ceUUpiTgF7QmQIJ7lgsKUPQOUl3o72QBrE1o=
 github.com/aws/smithy-go v1.24.2 h1:FzA3bu/nt/vDvmnkg+R8Xl46gmzEDam6mZ1hzmwXFng=

providers/azure/services/savingsplans/client.go

@@ -314,10 +314,11 @@ func commitmentCurrency(d *common.SavingsPlanDetails) (string, error) {
 // PUT with a previously-used name returns the existing order alias rather than
 // creating a second savings plan. When opts.IdempotencyToken is set (the
 // purchase-execution path), the alias name is derived deterministically from it
-// via common.ReservationOrderID so a re-drive of a stranded execution (issue
+// via common.OrderIDFromToken so a re-drive of a stranded execution (issue
 // #636) re-PUTs the same alias instead of double-buying. When the token is empty
 // (the CLI path, which has no owning execution), the prior timestamp-based name
-// is retained.
+// is retained. A non-empty malformed token returns an error wrapping
+// common.ErrInvalidIdempotencyToken before any PUT.
 func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) {
 	result := common.PurchaseResult{
 		Recommendation: rec,
@@ -348,6 +349,16 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati
 		return result, result.Error
 	}
 	displayName := fmt.Sprintf("cudly-%s-%s", spDetails.PlanType, rec.Term)
+	// Derive a deterministic alias name from the idempotency token when present
+	// so a re-drive PUTs the same alias (idempotent), falling back to the prior
+	// timestamp-based name on the CLI path (empty token). A malformed non-empty
+	// token fails here, before the body or alias client exists, so it can never
+	// downgrade to a non-idempotent purchase.
+	aliasName, err := common.OrderIDFromToken(opts.IdempotencyToken, fmt.Sprintf("cudly-%d", time.Now().UnixNano()))
+	if err != nil {
+		result.Error = err
+		return result, result.Error
+	}
 
 	body := armbillingbenefits.SavingsPlanOrderAliasModel{
 		SKU: &armbillingbenefits.SKU{Name: toPtr(spDetails.PlanType)},
@@ -376,10 +387,6 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati
 		aliasClient = &realOrderAliasClient{client: client}
 	}
 
-	// Derive a deterministic alias name from the idempotency token when present
-	// so a re-drive PUTs the same alias (idempotent), falling back to the prior
-	// timestamp-based name on the CLI path (empty token).
-	aliasName := common.ReservationOrderID(opts.IdempotencyToken, fmt.Sprintf("cudly-%d", time.Now().UnixNano()))
 	poller, err := aliasClient.BeginCreate(ctx, aliasName, body, nil)
 	if err != nil {
 		result.Error = fmt.Errorf("failed to begin savings plan purchase: %w", err)

providers/azure/services/savingsplans/client_test.go

@@ -356,14 +356,15 @@ func TestPurchaseCommitment_DeterministicAliasNameFromToken(t *testing.T) {
 	}
 
 	token := common.DeriveIdempotencyToken("exec-1", 0)
-	wantName := common.ReservationOrderID(token, "")
+	wantName, err := common.OrderIDFromToken(token, "")
+	require.NoError(t, err)
 	require.NotEmpty(t, wantName, "token must yield a deterministic GUID")
 
 	// First drive.
 	mock1 := &mockOrderAliasClient{createPoller: &mockPoller{resp: resp}}
 	c1 := NewClient(nil, "sub", "eastus")
 	c1.SetOrderAliasClient(mock1)
-	_, err := c1.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token})
+	_, err = c1.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token})
 	require.NoError(t, err)
 	assert.Equal(t, wantName, mock1.capturedName)
 
@@ -377,6 +378,35 @@ func TestPurchaseCommitment_DeterministicAliasNameFromToken(t *testing.T) {
 		"re-drive with the same idempotency token must reuse the alias name (PUT is idempotent)")
 }
 
+// TestPurchaseCommitment_MalformedTokenFailsBeforePUT verifies a non-empty token
+// that yields no GUID is refused before BeginCreate. The case without an
+// injected alias client checks the error is the token error, not "failed to
+// create order alias client". Pre-fix the malformed token silently fell back to
+// a timestamp alias name and the purchase went ahead.
+func TestPurchaseCommitment_MalformedTokenFailsBeforePUT(t *testing.T) {
+	for _, token := range []string{"abc", "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"} {
+		id := "/subscriptions/sub/providers/Microsoft.BillingBenefits/savingsPlanOrders/o"
+		mock := &mockOrderAliasClient{createPoller: &mockPoller{resp: armbillingbenefits.SavingsPlanOrderAliasClientCreateResponse{
+			SavingsPlanOrderAliasModel: armbillingbenefits.SavingsPlanOrderAliasModel{ID: &id},
+		}}}
+		c := NewClient(nil, "sub", "eastus")
+		c.SetOrderAliasClient(mock)
+		result, err := c.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token})
+		require.Error(t, err)
+		assert.ErrorIs(t, err, common.ErrInvalidIdempotencyToken)
+		assert.False(t, result.Success)
+		assert.Zero(t, mock.calls, "no BeginCreate for a malformed token")
+		assert.NotContains(t, err.Error(), token)
+	}
+
+	// No injected alias client and a nil credential: the token error must win
+	// over "failed to create order alias client".
+	c := NewClient(nil, "sub", "eastus")
+	_, err := c.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: "abc"})
+	require.Error(t, err)
+	assert.ErrorIs(t, err, common.ErrInvalidIdempotencyToken)
+}
+
 // TestPurchaseCommitment_EmptyTokenUsesTimestampName verifies the CLI path (no
 // idempotency token) retains a non-deterministic timestamp-based alias name.
 func TestPurchaseCommitment_EmptyTokenUsesTimestampName(t *testing.T) {