feat(providers): report the adopted RI offering class and a nil GCP adoption cost (#211)

CHANGELOG.md

@@ -309,6 +309,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
   compiles; keyed literals are unaffected. On an adopted re-drive it carries the
   offering class AWS reports for the existing RI (the EC2 provider sets it in a
   follow-up change); empty otherwise (#211).
+- **`aws`/EC2:** an adopted re-drive now reports the existing RI's offering
+  class in `PurchaseResult.OfferingClass` (#211).
+- GCP Compute Engine adoption now returns a nil `Cost` instead of a pointer to 0;
+  a fresh purchase is unchanged (#211).
 
 ### Fixed
 

providers/aws/go.mod

@@ -3,7 +3,7 @@ module github.com/LeanerCloud/cloud-commitments-go/providers/aws
 go 1.26.10
 
 require (
-	github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106235222-20c9dacb335f
+	github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261108091534-a52dfb9143cc
 	github.com/aws/aws-sdk-go-v2 v1.47.5
 	github.com/aws/aws-sdk-go-v2/config v1.33.13
 	github.com/aws/aws-sdk-go-v2/credentials v1.20.13

providers/aws/go.sum

@@ -1,5 +1,5 @@
-github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106235222-20c9dacb335f h1:+GzNJalL4vfbk1Lwcu2jIGvJxc7DKScfR2N94+uGbXk=
-github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106235222-20c9dacb335f/go.mod h1:l9V+ecJCfAigYr6XiK9kpIUemF9bnXOKHJN1s4TPsM0=
+github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261108091534-a52dfb9143cc h1:4Aw4TpF9X1Jw/eCQTvBp3bHmFhS8GIWdl5L48d6v3Zs=
+github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261108091534-a52dfb9143cc/go.mod h1:l9V+ecJCfAigYr6XiK9kpIUemF9bnXOKHJN1s4TPsM0=
 github.com/aws/aws-sdk-go-v2 v1.47.5 h1:5yA+A+BiXbDq1ZLbYwPViwhEGDMdR0MEhL8Jh2LnLS4=
 github.com/aws/aws-sdk-go-v2 v1.47.5/go.mod h1:iNIkogrRvFwm+1OQIUOP2lPBtlXZboOIUWiCa+pHEMw=
 github.com/aws/aws-sdk-go-v2/config v1.33.13 h1:u52x6ISQOx172tpwM0VRxhJzEsTJYpMcEYUGM2A1wrw=

providers/aws/services/ec2/client.go

@@ -154,7 +154,7 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count
 	// already succeeded, so short-circuit and return the existing RI rather
 	// than buying a second one.
 	if opts.IdempotencyToken != "" {
-		existingID, found, lookupErr := c.findRIByIdempotencyToken(ctx, opts.IdempotencyToken)
+		existingRI, lookupErr := c.findRIByIdempotencyToken(ctx, opts.IdempotencyToken)
 		if lookupErr != nil {
 			// 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. Fail
@@ -163,11 +163,8 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count
 			result.Error = fmt.Errorf("idempotency lookup failed before EC2 RI purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr)
 			return result, result.Error
 		}
-		if found {
-			log.Printf("EC2 RI for idempotency token %s already exists (%s); skipping purchase (issue #636 re-drive)", common.MaskToken(opts.IdempotencyToken), existingID)
-			result.Success = true
-			result.ExistingCommitment = true
-			result.CommitmentID = existingID
+		if existingRI != nil {
+			adoptExistingRI(&result, existingRI, opts.IdempotencyToken)
 			return result, nil
 		}
 	}
@@ -262,13 +259,30 @@ func checkApprovedUpfront(rec common.Recommendation, offering *types.ReservedIns
 	return nil
 }
 
+// adoptExistingRI records a re-driven purchase that found the RI an earlier
+// attempt bought (issue #636). The offering class comes from the RI itself, not
+// the request, so a caller never mislabels an adopted RI (issue #211). Cost
+// stays nil: no charge was made by this call.
+func adoptExistingRI(result *common.PurchaseResult, ri *types.ReservedInstances, token string) {
+	id := aws.ToString(ri.ReservedInstancesId)
+	log.Printf("EC2 RI for idempotency token %s already exists (%s); skipping purchase (issue #636 re-drive)", common.MaskToken(token), id)
+	result.Success = true
+	result.ExistingCommitment = true
+	result.CommitmentID = id
+	result.OfferingClass = string(ri.OfferingClass)
+	if result.OfferingClass == "" {
+		log.Printf("EC2 RI %s adopted for idempotency token %s reports no offering class", id, common.MaskToken(token))
+	}
+}
+
 // findRIByIdempotencyToken looks for an active or payment-pending Reserved
 // Instance tagged with the given idempotency token (issue #636). It returns the
-// RI ID and true when exactly such an RI exists, so a re-driven purchase can
-// short-circuit instead of buying a second commitment. Retired/canceled RIs are
+// first 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.
-func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (riID string, found bool, err error) {
+func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (*types.ReservedInstances, error) {
 	input := &ec2.DescribeReservedInstancesInput{
 		Filters: []types.Filter{
 			{
@@ -284,15 +298,15 @@ func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (ri
 
 	response, err := c.client.DescribeReservedInstances(ctx, input)
 	if err != nil {
-		return "", false, fmt.Errorf("failed to describe reserved instances for idempotency check: %w", err)
+		return nil, fmt.Errorf("failed to describe reserved instances for idempotency check: %w", err)
 	}
 	for i := range response.ReservedInstances {
 		ri := &response.ReservedInstances[i]
 		if ri.ReservedInstancesId != nil {
-			return aws.ToString(ri.ReservedInstancesId), true, nil
+			return ri, nil
 		}
 	}
-	return "", false, nil
+	return nil, nil
 }
 
 // tagReservedInstance applies the standard CUDly tag set (including the

providers/aws/services/ec2/client_test.go

@@ -1711,6 +1711,8 @@ func TestPurchaseCommitment_ExistingCommitmentFlag(t *testing.T) {
 		assert.True(t, result.Success)
 		assert.Equal(t, "ri-fresh-211", result.CommitmentID)
 		assert.False(t, result.ExistingCommitment)
+		assert.Empty(t, result.OfferingClass)
+		assert.NotNil(t, result.Cost)
 		m.AssertExpectations(t)
 	})
 
@@ -1731,28 +1733,77 @@ func TestPurchaseCommitment_ExistingCommitmentFlag(t *testing.T) {
 		assert.False(t, result.ExistingCommitment)
 	})
 
-	// The wrong-class incident: the RI an earlier attempt bought is Standard but
-	// the operator has since configured Convertible. The re-drive still adopts it
-	// (no offering lookup, no second purchase), and the flag is what lets the
-	// caller avoid recording the configured class for it.
-	t.Run("re-drive with a different configured class is flagged", func(t *testing.T) {
+	// The wrong-class incident: the RI an earlier attempt bought differs from the
+	// class now requested. The re-drive still adopts it (no offering lookup, no
+	// second purchase) and reports the RI's real class, not the requested one.
+	for _, tc := range []struct {
+		name      string
+		riClass   types.OfferingClassType
+		requested string
+	}{
+		{"standard-ri-convertible-requested", types.OfferingClassTypeStandard, "convertible"},
+		{"convertible-ri-standard-requested", types.OfferingClassTypeConvertible, "standard"},
+	} {
+		t.Run("re-drive adopts the RI's class/"+tc.name, func(t *testing.T) {
+			t.Parallel()
+			m := &MockEC2Client{}
+			client := &Client{client: m, region: "us-east-1"}
+			m.On("DescribeReservedInstances", mock.Anything, tokenLookup).
+				Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{
+					{ReservedInstancesId: aws.String("ri-adopted-211"), OfferingClass: tc.riClass},
+				}}, nil).Once()
+
+			result, err := client.PurchaseCommitment(context.Background(), rec,
+				common.PurchaseOptions{IdempotencyToken: token, OfferingClass: tc.requested})
+			require.NoError(t, err)
+			assert.True(t, result.Success)
+			assert.Equal(t, "ri-adopted-211", result.CommitmentID)
+			assert.True(t, result.ExistingCommitment)
+			assert.Equal(t, string(tc.riClass), result.OfferingClass)
+			assert.Nil(t, result.Cost)
+			m.AssertExpectations(t)
+			m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything)
+		})
+	}
+
+	t.Run("nil-ID entry is skipped", func(t *testing.T) {
 		t.Parallel()
 		m := &MockEC2Client{}
 		client := &Client{client: m, region: "us-east-1"}
 		m.On("DescribeReservedInstances", mock.Anything, tokenLookup).
 			Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{
-				{ReservedInstancesId: aws.String("ri-standard-211"), OfferingClass: types.OfferingClassTypeStandard},
+				{ReservedInstancesId: nil, OfferingClass: types.OfferingClassTypeStandard},
+				{ReservedInstancesId: aws.String("ri-valid"), OfferingClass: types.OfferingClassTypeConvertible},
 			}}, nil).Once()
 
-		result, err := client.PurchaseCommitment(context.Background(), rec,
-			common.PurchaseOptions{IdempotencyToken: token, OfferingClass: "convertible"})
+		result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token})
 		require.NoError(t, err)
-		assert.True(t, result.Success)
-		assert.Equal(t, "ri-standard-211", result.CommitmentID)
 		assert.True(t, result.ExistingCommitment)
-		m.AssertExpectations(t)
+		assert.Equal(t, "ri-valid", result.CommitmentID)
+		assert.Equal(t, "convertible", result.OfferingClass)
 		m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything)
 	})
+
+	t.Run("nil-ID-only lookup takes the fresh path", func(t *testing.T) {
+		t.Parallel()
+		m := &MockEC2Client{}
+		client := &Client{client: m, region: "us-east-1"}
+		m.On("DescribeReservedInstances", mock.Anything, tokenLookup).
+			Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{
+				{ReservedInstancesId: nil, OfferingClass: types.OfferingClassTypeStandard},
+			}}, nil).Once()
+		m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything).Return(offerings, nil)
+		m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.Anything).
+			Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-fresh-211")}, nil)
+		m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil)
+
+		result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token})
+		require.NoError(t, err)
+		assert.False(t, result.ExistingCommitment)
+		assert.Equal(t, "ri-fresh-211", result.CommitmentID)
+		assert.Empty(t, result.OfferingClass)
+		m.AssertExpectations(t)
+	})
 }
 
 func zonalRec(az string) common.Recommendation {

providers/aws/services/ec2/reconcile_test.go

@@ -187,9 +187,9 @@ func TestReconcilePurchase_LostResponseUntaggedRI(t *testing.T) {
 	_, err := c.PurchaseCommitment(context.Background(), req.Recommendation, req.Options)
 	require.ErrorIs(t, err, common.ErrOutcomeUnknown)
 
-	_, found, err := c.findRIByIdempotencyToken(context.Background(), req.Options.IdempotencyToken)
+	found, err := c.findRIByIdempotencyToken(context.Background(), req.Options.IdempotencyToken)
 	require.NoError(t, err)
-	assert.False(t, found, "the token lookup alone misses an untagged RI")
+	assert.Nil(t, found, "the token lookup alone misses an untagged RI")
 
 	got := c.ReconcilePurchase(context.Background(), req)
 	assert.True(t, got.IsFound())

providers/gcp/services/computeengine/client.go

@@ -768,8 +768,13 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati
 
 	result.Success = true
 	result.CommitmentID = commitmentName
-	upfrontCost := 0.0 // Compute Engine CUDs are billed monthly, with no upfront charge.
-	result.Cost = &upfrontCost
+	if !result.ExistingCommitment {
+		// Compute Engine CUDs are billed monthly, with no upfront charge. An
+		// adopted commitment made no charge in this call, so its Cost stays nil
+		// (unknown) rather than a claimed zero (issue #211).
+		upfrontCost := 0.0
+		result.Cost = &upfrontCost
+	}
 
 	return result, nil
 }

providers/gcp/services/computeengine/client_test.go

@@ -926,8 +926,7 @@ func TestComputeEngineClient_PurchaseCommitment_ReDriveAlreadyExists(t *testing.
 				assert.True(t, result.Success)
 				assert.Equal(t, name, result.CommitmentID)
 				assert.True(t, result.ExistingCommitment, "an adopted commitment must be flagged")
-				require.NotNil(t, result.Cost)
-				assert.Zero(t, *result.Cost)
+				assert.Nil(t, result.Cost, "an adopted commitment made no charge in this call")
 				return
 			}
 			require.Error(t, err)
@@ -971,6 +970,7 @@ func TestComputeEngineClient_PurchaseCommitment_ReDriveAlreadyExistsOnWait(t *te
 	assert.True(t, result.Success)
 	assert.Equal(t, name, result.CommitmentID)
 	assert.True(t, result.ExistingCommitment)
+	assert.Nil(t, result.Cost)
 }
 
 // A first purchase that succeeds outright is not an adoption, even with a token.
@@ -987,6 +987,8 @@ func TestComputeEngineClient_PurchaseCommitment_FreshPurchaseNotExisting(t *test
 	require.NoError(t, err)
 	assert.True(t, result.Success)
 	assert.False(t, result.ExistingCommitment)
+	require.NotNil(t, result.Cost)
+	assert.Zero(t, *result.Cost)
 }
 
 func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) {