fix(aws/ec2): refuse a re-drive when several RIs carry the idempotency token

CHANGELOG.md

@@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Notices
 
+- **EC2 re-drives now refuse when several RIs carry the idempotency token.**
+  If more than one active or payment-pending RI has the token (#724), the
+  purchase is refused instead of adopting the first one: no purchase is sent and
+  the error wraps `common.ErrRefusedBeforePurchase`, names the count and the
+  masked token, and needs a human to reconcile.
+
 - **Azure Redis purchases must go through managedredis (`ServiceMemoryDB`).**
   The Azure recommendations adapter still labels Redis recommendations
   `ServiceCache`, and the cache client now refuses every purchase (#538), so

providers/aws/services/ec2/client.go

@@ -160,7 +160,7 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count
 			// would defeat the guard and risk a double-buy on a re-drive. Fail
 			// loudly so the recovery sweep treats it as not-yet-purchased and
 			// retries the whole guarded path, rather than silently buying.
-			result.Error = fmt.Errorf("idempotency lookup failed before EC2 RI purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr)
+			result.Error = idempotencyLookupError(lookupErr)
 			return result, result.Error
 		}
 		if existingRI != nil {
@@ -230,6 +230,15 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count
 	return result, nil
 }
 
+// idempotencyLookupError passes an ambiguity refusal through unchanged and
+// wraps any other lookup failure as a refusal to purchase.
+func idempotencyLookupError(lookupErr error) error {
+	if errors.Is(lookupErr, common.ErrRefusedBeforePurchase) {
+		return lookupErr
+	}
+	return fmt.Errorf("idempotency lookup failed before EC2 RI purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr)
+}
+
 // checkApprovedUpfront refuses an offering whose total upfront exceeds the
 // approved recommendation's upfront by more than offeringprice.UpfrontTolerance,
 // or whose price cannot be compared with it. The API's LimitPrice is not used:
@@ -276,12 +285,17 @@ func adoptExistingRI(result *common.PurchaseResult, ri *types.ReservedInstances,
 }
 
 // findRIByIdempotencyToken looks for an active or payment-pending Reserved
-// Instance tagged with the given idempotency token (issue #636). It returns the
-// first such RI, or nil when none exists, so a re-driven purchase can
+// Instance tagged with the given idempotency token (issue #636). It returns
+// the single such RI, or nil when none exists, so a re-driven purchase can
 // short-circuit instead of buying a second commitment and report the adopted
-// RI's real offering class (issue #211). Retired/canceled RIs are
-// excluded (they carry the same state filter as GetExistingCommitments) so a
-// returned or expired commitment does not suppress a legitimate fresh purchase.
+// RI's real offering class (issue #211). Entries without a ReservedInstancesId
+// are ignored. When more than one RI carries the token the commitment state is
+// ambiguous (issue #724), so it fails closed: the error wraps
+// common.ErrRefusedBeforePurchase (this call sent no purchase; ErrOutcomeUnknown
+// would wrongly claim it may have), names the count and the masked token, and
+// needs a human to reconcile. Retired/canceled RIs are excluded (they carry the
+// same state filter as GetExistingCommitments) so a returned or expired
+// commitment does not suppress a legitimate fresh purchase.
 func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (*types.ReservedInstances, error) {
 	input := &ec2.DescribeReservedInstancesInput{
 		Filters: []types.Filter{
@@ -300,13 +314,21 @@ func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (*t
 	if err != nil {
 		return nil, fmt.Errorf("failed to describe reserved instances for idempotency check: %w", err)
 	}
+	var matches []*types.ReservedInstances
 	for i := range response.ReservedInstances {
-		ri := &response.ReservedInstances[i]
-		if ri.ReservedInstancesId != nil {
-			return ri, nil
+		if ri := &response.ReservedInstances[i]; ri.ReservedInstancesId != nil {
+			matches = append(matches, ri)
 		}
 	}
-	return nil, nil
+	switch len(matches) {
+	case 0:
+		return nil, nil
+	case 1:
+		return matches[0], nil
+	default:
+		return nil, fmt.Errorf("%w: %d active or payment-pending EC2 RIs carry idempotency token %s; commitment state is ambiguous and needs a human to reconcile",
+			common.ErrRefusedBeforePurchase, len(matches), common.MaskToken(token))
+	}
 }
 
 // tagReservedInstance applies the standard CUDly tag set (including the

providers/aws/services/ec2/client_test.go

@@ -2525,3 +2525,79 @@ func TestListTargetOfferings_CancelMidWalk_StopsWithContextError(t *testing.T) {
 	assert.Nil(t, offs)
 	assert.Equal(t, 3, calls)
 }
+
+// TestPurchaseCommitment_IdempotencyTokenMatchCount covers issue #724: a
+// re-drive adopts exactly one tagged RI, buys on zero, ignores ID-less entries
+// and fails closed (no purchase) when several RIs carry the token.
+func TestPurchaseCommitment_IdempotencyTokenMatchCount(t *testing.T) {
+	token := common.DeriveIdempotencyToken("exec-724", 0)
+	ri := func(id string) types.ReservedInstances {
+		return types.ReservedInstances{ReservedInstancesId: aws.String(id)}
+	}
+
+	tests := []struct {
+		name        string
+		ris         []types.ReservedInstances
+		wantAdopt   string
+		wantBuy     bool
+		wantAmbig   bool
+		wantCountIn string
+	}{
+		{name: "zero matches buys", ris: nil, wantBuy: true},
+		{name: "only id-less entries buys", ris: []types.ReservedInstances{{}, {}}, wantBuy: true},
+		{name: "one match adopts", ris: []types.ReservedInstances{ri("ri-a")}, wantAdopt: "ri-a"},
+		{name: "two matches fail closed", ris: []types.ReservedInstances{ri("ri-a"), ri("ri-b")}, wantAmbig: true, wantCountIn: "2 "},
+		{name: "three matches fail closed", ris: []types.ReservedInstances{ri("ri-a"), ri("ri-b"), ri("ri-c")}, wantAmbig: true, wantCountIn: "3 "},
+		{name: "id-less entry plus one RI adopts", ris: []types.ReservedInstances{{}, ri("ri-a")}, wantAdopt: "ri-a"},
+		{name: "two entries one id-less adopts", ris: []types.ReservedInstances{ri("ri-a"), {}}, wantAdopt: "ri-a"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockEC2 := &MockEC2Client{}
+			client := &Client{client: mockEC2, region: "us-east-1"}
+			mockEC2.On("DescribeReservedInstances", mock.Anything, mock.Anything).
+				Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: tt.ris}, nil).Once()
+			if tt.wantBuy {
+				// Reaching the offering lookup proves the guard fell through to a purchase.
+				mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything).
+					Return(nil, errors.New("offering lookup reached")).Once()
+			}
+			rec := common.Recommendation{
+				ResourceType:  "t3.micro",
+				Count:         1,
+				PaymentOption: "all-upfront",
+				Term:          "1yr",
+				Details:       &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"},
+			}
+
+			result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token})
+
+			switch {
+			case tt.wantAdopt != "":
+				require.NoError(t, err)
+				assert.True(t, result.Success)
+				assert.True(t, result.ExistingCommitment)
+				assert.Equal(t, tt.wantAdopt, result.CommitmentID)
+			case tt.wantAmbig:
+				require.Error(t, err)
+				assert.False(t, result.Success)
+				assert.False(t, result.ExistingCommitment)
+				assert.Empty(t, result.CommitmentID)
+				assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase)
+				assert.NotErrorIs(t, err, common.ErrOutcomeUnknown)
+				assert.Contains(t, err.Error(), tt.wantCountIn)
+				assert.Contains(t, err.Error(), common.MaskToken(token))
+				assert.NotContains(t, err.Error(), token)
+				assert.Contains(t, err.Error(), "ambiguous")
+				assert.NotContains(t, err.Error(), "idempotency lookup failed")
+				assert.Equal(t, err, result.Error)
+				mockEC2.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything)
+				mockEC2.AssertNotCalled(t, "DescribeReservedInstancesOfferings", mock.Anything, mock.Anything)
+			case tt.wantBuy:
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), "offering lookup reached")
+			}
+			mockEC2.AssertExpectations(t)
+		})
+	}
+}