fix(ri-exchange): exclude convertible RIs with no pricing from cost math

internal/api/handler_ri_exchange.go

@@ -1605,18 +1605,47 @@ func parseThresholdParam(params map[string]string) (float64, error) {
 //	monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × 730
 //
 // 730 is AWS's canonical hours-per-month constant. Returns an error when
-// Duration is non-positive: a zero cost would read as a free RI and skip
-// reshape's dollar-units pricing gate, so callers must exclude the RI.
+// Duration is non-positive or when the computed cost is not positive (for
+// example FixedPrice, UsagePrice and RecurringHourlyAmount all zero, which
+// means AWS returned no pricing, not a free RI): a zero cost would read as a
+// free RI and skip reshape's dollar-units pricing gate, so callers must
+// exclude the RI. The error wraps errRINonPositiveDuration or errRINoPricing.
 //
 // Used to populate exchange.RIInfo.MonthlyCost so the cross-family
 // dollar-units pre-filter can compare against per-target offering
 // costs computed with the same formula.
+//
+// Keep in sync with the copy in internal/server/handler_ri_exchange.go
+// (auto-exchange and ladder path), which applies the same checks but does
+// not wrap the sentinels.
 func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) {
 	if ri.Duration <= 0 {
-		return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration)
+		return 0, fmt.Errorf("convertible RI %s has %w %d: monthly cost is not computable", ri.ReservedInstanceID, errRINonPositiveDuration, ri.Duration)
 	}
 	hoursPerTerm := float64(ri.Duration) / 3600
-	return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil
+	cost := ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730
+	// !(cost > 0) also rejects NaN, which is no usable price either.
+	if !(cost > 0) {
+		return 0, fmt.Errorf("convertible RI %s has %w (fixed %v, usage %v, recurring hourly %v): monthly cost is not computable",
+			ri.ReservedInstanceID, errRINoPricing, ri.FixedPrice, ri.UsagePrice, ri.RecurringHourlyAmount)
+	}
+	return cost, nil
+}
+
+// Sentinel causes for monthlyCostFromConvertibleRI, used to pick the reason
+// shown for an RI excluded from reshape analysis.
+var (
+	errRINonPositiveDuration = errors.New("non-positive duration")
+	errRINoPricing           = errors.New("no positive price")
+)
+
+// excludedRIReason is the user-facing reason for an RI left out of reshape
+// analysis because monthlyCostFromConvertibleRI returned err.
+func excludedRIReason(err error) string {
+	if errors.Is(err, errRINoPricing) {
+		return "AWS returned no pricing (fixed, usage and recurring charges total zero or less), so its monthly cost cannot be computed"
+	}
+	return "AWS returned no term length (duration), so its monthly cost cannot be computed"
 }
 
 // convertToExchangeTypes converts provider-specific types to the exchange package types.
@@ -1630,7 +1659,7 @@ func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recomme
 		monthlyCost, err := monthlyCostFromConvertibleRI(inst)
 		if err != nil {
 			logging.Warnf("convertToExchangeTypes: excluding RI from reshape: %v", err)
-			excluded = append(excluded, ExcludedRI{RIID: inst.ReservedInstanceID, Reason: "AWS returned no term length (duration), so its monthly cost cannot be computed"})
+			excluded = append(excluded, ExcludedRI{RIID: inst.ReservedInstanceID, Reason: excludedRIReason(err)})
 			continue
 		}
 		riInfos = append(riInfos, exchange.RIInfo{

internal/api/handler_ri_exchange_integration_test.go

@@ -109,6 +109,12 @@ func seedRecsForRegion(ctx context.Context, t *testing.T, store *config.Postgres
 		"seeding recommendations into the test container failed")
 }
 
+// reshapeIntegrationFixedPrice gives fixture RIs a real price (an unpriced
+// RI is excluded, issue #1188) small enough that the dollar-units gate keeps
+// the seeded cross-family recs: 100 over 1y is ~8.33/month, and NF 8 x 8.33
+// stays under the c5.large / r5.large recs' NF 4 x 50 and NF 4 x 60.
+const reshapeIntegrationFixedPrice = 100
+
 // TestReshapeRecommendations_Integration_EndToEnd exercises the full
 // path: auth → cache cold-fetch → AnalyzeReshapingWithRecs (recs-driven
 // alternatives) → JSON response. Asserts the response carries
@@ -135,8 +141,9 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) {
 
 	ec2Fake := &fakeReshapeEC2{
 		instances: []ec2svc.ConvertibleRI{
-			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000},
+			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice},
 			{ReservedInstanceID: "ri-noterm", InstanceType: "m5.xlarge", InstanceCount: 1},
+			{ReservedInstanceID: "ri-unpriced", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000},
 		},
 	}
 	recsFake := &fakeReshapeRecs{
@@ -154,8 +161,11 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) {
 	rec := body.Recommendations[0]
 	require.Equal(t, "ri-1", rec.SourceRIID)
 	require.Equal(t, "m5.large", rec.TargetInstanceType)
-	require.Len(t, body.ExcludedRIs, 1, "an RI with no term length must be reported, not silently skipped")
+	require.Len(t, body.ExcludedRIs, 2, "RIs with no term length or no pricing must be reported, not silently skipped")
 	require.Equal(t, "ri-noterm", body.ExcludedRIs[0].RIID)
+	require.Contains(t, body.ExcludedRIs[0].Reason, "term length")
+	require.Equal(t, "ri-unpriced", body.ExcludedRIs[1].RIID)
+	require.Contains(t, body.ExcludedRIs[1].Reason, "no pricing")
 
 	// Same-family m5 stripped; c5 + r5 surface ascending by cost.
 	require.Len(t, rec.AlternativeTargets, 2,
@@ -200,7 +210,7 @@ func TestReshapeRecommendations_Integration_SecondCallHitsCache(t *testing.T) {
 
 	ec2Fake := &fakeReshapeEC2{
 		instances: []ec2svc.ConvertibleRI{
-			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000},
+			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice},
 		},
 	}
 	recsFake := &fakeReshapeRecs{
@@ -236,7 +246,7 @@ func TestReshapeRecommendations_Integration_NoCachedRecsReturnsPrimaryOnly(t *te
 
 	ec2Fake := &fakeReshapeEC2{
 		instances: []ec2svc.ConvertibleRI{
-			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000},
+			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice},
 		},
 	}
 	recsFake := &fakeReshapeRecs{
@@ -337,7 +347,7 @@ func TestReshapeRecommendations_Integration_ScopedAccount_FiltersToAccount(t *te
 
 	ec2Fake := &fakeReshapeEC2{
 		instances: []ec2svc.ConvertibleRI{
-			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000},
+			{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice},
 		},
 	}
 	recsFake := &fakeReshapeRecs{

internal/api/handler_ri_exchange_test.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"math"
 	"math/big"
 	"strings"
 	"testing"
@@ -974,7 +975,7 @@ func TestGetReshapeRecommendations_EmptyRegionUsesConfigRegion(t *testing.T) {
 		reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client {
 			return &fakeReshapeEC2Stub{
 				instances: []ec2svc.ConvertibleRI{
-					{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD", Duration: 31536000},
+					{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD", Duration: 31536000, FixedPrice: 876},
 				},
 			}
 		},
@@ -1059,8 +1060,11 @@ func TestGetReshapeRecommendations_AlternativeCurrencyFromRIs(t *testing.T) {
 				config: mockStore,
 				auth:   &mockAuthForExchange{},
 				reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client {
+					// FixedPrice 100 over 1y is ~8.33/month; NF 8 x 8.33 stays
+					// under the c6i.large rec's NF 4 x 40, so the dollar-units
+					// gate keeps the alternative this test is about.
 					return &fakeReshapeEC2Stub{instances: []ec2svc.ConvertibleRI{
-						{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: tc.riCurrency, Duration: 31536000},
+						{ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: tc.riCurrency, Duration: 31536000, FixedPrice: 100},
 					}}
 				},
 				reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient {
@@ -2803,3 +2807,101 @@ func TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors(t *testing.T) {
 	require.NoError(t, err)
 	assert.InDelta(t, 730.0, cost, 1e-6)
 }
+
+// A convertible RI whose FixedPrice, UsagePrice and RecurringHourlyAmount are
+// all zero has no pricing data; it is not free. MonthlyCost=0 would make
+// reshape's pricing gate pass every alternative and score the RI as neutral,
+// so it must be excluded (issue #1188). A No Upfront RI (FixedPrice 0 with an
+// hourly charge) is real pricing and must be kept.
+func TestConvertToExchangeTypes_ExcludesRIWithoutPricing(t *testing.T) {
+	const oneYear = 31536000
+	instances := []ec2svc.ConvertibleRI{
+		{ReservedInstanceID: "ri-unpriced", InstanceType: "m5.large", InstanceCount: 1, Duration: oneYear},
+		{ReservedInstanceID: "ri-no-upfront", InstanceType: "m5.large", InstanceCount: 1, RecurringHourlyAmount: 0.05, Duration: oneYear},
+		{ReservedInstanceID: "ri-usage-only", InstanceType: "m5.large", InstanceCount: 1, UsagePrice: 0.04, Duration: oneYear},
+		{ReservedInstanceID: "ri-all-upfront", InstanceType: "m5.large", InstanceCount: 1, FixedPrice: 876, Duration: oneYear},
+		{ReservedInstanceID: "ri-negative", InstanceType: "c5.large", InstanceCount: 1, FixedPrice: -876, Duration: oneYear},
+	}
+
+	riInfos, _, excluded := convertToExchangeTypes(instances, nil)
+
+	require.Len(t, riInfos, 3)
+	want := map[string]float64{"ri-no-upfront": 0.05 * 730, "ri-usage-only": 0.04 * 730, "ri-all-upfront": 876.0 / 8760 * 730}
+	for _, ri := range riInfos {
+		cost, ok := want[ri.ID]
+		require.True(t, ok, "unexpected RI %s kept", ri.ID)
+		assert.InDelta(t, cost, ri.MonthlyCost, 1e-6, ri.ID)
+		assert.Greater(t, ri.MonthlyCost, 0.0, ri.ID)
+	}
+	require.Len(t, excluded, 2)
+	assert.Equal(t, "ri-unpriced", excluded[0].RIID)
+	assert.Equal(t, "ri-negative", excluded[1].RIID)
+	for _, e := range excluded {
+		assert.Contains(t, e.Reason, "no pricing")
+		assert.NotContains(t, e.Reason, "term length")
+	}
+}
+
+// End to end through the reshape handler: an unpriced RI is reported in
+// excluded_ris with the pricing reason and gets no recommendation, while a
+// priced RI in the same account still does.
+func TestGetReshapeRecommendations_ReportsUnpricedRIs(t *testing.T) {
+	mockStore := &MockConfigStore{}
+	mockStore.On("ListStoredRecommendations", mock.Anything, mock.Anything).
+		Return([]config.RecommendationRecord(nil), nil)
+	h := &Handler{
+		config: mockStore,
+		auth:   &mockAuthForExchange{},
+		reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client {
+			return &fakeReshapeEC2Stub{instances: []ec2svc.ConvertibleRI{
+				{ReservedInstanceID: "ri-unpriced", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, CurrencyCode: "USD"},
+				{ReservedInstanceID: "ri-ok", InstanceType: "m5.xlarge", InstanceCount: 1, FixedPrice: 8760, Duration: 31536000, CurrencyCode: "USD"},
+			}}
+		},
+		reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient {
+			return &fakeReshapeRecsStub{utilization: []recommendations.RIUtilization{
+				{ReservedInstanceID: "ri-unpriced", UtilizationPercent: 50.0},
+				{ReservedInstanceID: "ri-ok", UtilizationPercent: 50.0},
+			}}
+		},
+		reshapeAccountResolver: func(_ context.Context) (string, error) { return "", nil },
+	}
+	h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} })
+
+	req := &events.LambdaFunctionURLRequest{
+		Headers:               map[string]string{"authorization": "Bearer test-token"},
+		QueryStringParameters: map[string]string{"lookback_days": "30", "threshold": "95.0"},
+	}
+	out, err := h.getReshapeRecommendations(context.Background(), req)
+	require.NoError(t, err)
+	resp, ok := out.(*ReshapeRecommendationsResponse)
+	require.True(t, ok)
+
+	require.Len(t, resp.ExcludedRIs, 1)
+	assert.Equal(t, "ri-unpriced", resp.ExcludedRIs[0].RIID)
+	assert.Contains(t, resp.ExcludedRIs[0].Reason, "no pricing")
+	require.NotEmpty(t, resp.Recommendations)
+	for _, r := range resp.Recommendations {
+		assert.NotEqual(t, "ri-unpriced", r.SourceRIID)
+	}
+}
+
+func TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors(t *testing.T) {
+	for name, ri := range map[string]ec2svc.ConvertibleRI{
+		"all zero":     {ReservedInstanceID: "ri-x", Duration: 31536000},
+		"negative sum": {ReservedInstanceID: "ri-x", FixedPrice: -876, Duration: 31536000},
+		"NaN":          {ReservedInstanceID: "ri-x", UsagePrice: math.NaN(), Duration: 31536000},
+	} {
+		cost, err := monthlyCostFromConvertibleRI(ri)
+		require.Error(t, err, name)
+		assert.Zero(t, cost, name)
+		assert.Contains(t, err.Error(), "ri-x", name)
+		assert.ErrorIs(t, err, errRINoPricing, name)
+		assert.NotErrorIs(t, err, errRINonPositiveDuration, name)
+	}
+	_, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{ReservedInstanceID: "ri-y"})
+	assert.ErrorIs(t, err, errRINonPositiveDuration, "duration is checked before pricing")
+	cost, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{RecurringHourlyAmount: 0.05, Duration: 31536000})
+	require.NoError(t, err)
+	assert.InDelta(t, 36.5, cost, 1e-6)
+}

internal/api/openapi.yaml

@@ -1341,9 +1341,12 @@ paths:
                   excluded_ris:
                     type: array
                     description: >
-                      Convertible RIs left out of cost-based analysis (for
-                      example AWS returned no term length), so they have no
-                      recommendation. Omitted when empty.
+                      Convertible RIs left out of cost-based analysis
+                      because their monthly cost cannot be computed (AWS
+                      returned no term length, or no pricing: fixed, usage
+                      and recurring charges total zero or less), so they have
+                      no recommendation. reason says which. Omitted when
+                      empty.
                     items:
                       type: object
                       properties:

internal/server/handler_ri_exchange.go

@@ -244,22 +244,38 @@ func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboar
 //
 //	monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × 730
 //
-// Returns an error when Duration is non-positive: a zero cost would read
-// as a free RI and skip the pricing gate, so callers must exclude the RI.
+// Returns an error when Duration is non-positive or when the computed cost
+// is not positive (for example FixedPrice, UsagePrice and
+// RecurringHourlyAmount all zero, which means AWS returned no pricing, not a
+// free RI): a zero cost would read as a free RI and skip the pricing gate,
+// so callers must exclude the RI.
 //
 // Used to populate exchange.RIInfo.MonthlyCost so the cross-family
 // dollar-units pre-filter compares apples-to-apples against per-target
 // offering costs.
+//
+// Keep in sync with the copy in internal/api/handler_ri_exchange.go. That
+// copy also wraps errRINonPositiveDuration / errRINoPricing so the API can
+// report a per-RI reason; this one does not, because the callers here only
+// log and drop the RI.
 func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) {
 	if ri.Duration <= 0 {
 		return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration)
 	}
 	hoursPerTerm := float64(ri.Duration) / 3600
-	return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil
+	cost := ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730
+	// !(cost > 0) also rejects NaN, which is no usable price either.
+	if !(cost > 0) {
+		return 0, fmt.Errorf("convertible RI %s has no positive price (fixed %v, usage %v, recurring hourly %v): monthly cost is not computable",
+			ri.ReservedInstanceID, ri.FixedPrice, ri.UsagePrice, ri.RecurringHourlyAmount)
+	}
+	return cost, nil
 }
 
 // convertForAutoExchange converts provider-specific types to exchange package types,
-// including RI metadata needed for offering lookup.
+// including RI metadata needed for offering lookup. It serves both the
+// auto-exchange run and the ladder write path (ladder_write.go); an RI whose
+// monthly cost is not computable is logged and dropped on both.
 func convertForAutoExchange(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo, map[string]exchange.RIMetadataInfo) {
 	riInfos := make([]exchange.RIInfo, 0, len(instances))
 	riMetadata := make(map[string]exchange.RIMetadataInfo, len(instances))
@@ -268,7 +284,7 @@ func convertForAutoExchange(instances []ec2svc.ConvertibleRI, utilData []recomme
 		inst := instances[i]
 		monthlyCost, err := monthlyCostFromConvertibleRI(inst)
 		if err != nil {
-			logging.Warnf("convertForAutoExchange: excluding RI from auto-exchange: %v", err)
+			logging.Warnf("convertForAutoExchange: excluding RI from auto-exchange and ladder reshaping: %v", err)
 			continue
 		}
 		riInfos = append(riInfos, exchange.RIInfo{

internal/server/handler_ri_exchange_reservation_integration_test.go

@@ -63,7 +63,7 @@ func TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClient(t *testing.T) {
 	}
 	ri := func(id string) []ec2svc.ConvertibleRI {
 		return []ec2svc.ConvertibleRI{{ReservedInstanceID: id, InstanceType: "c5.xlarge", InstanceCount: 1, NormalizationFactor: 8,
-			ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", Duration: 31536000}}
+			ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", Duration: 31536000, FixedPrice: 876}}
 	}
 	util := func(id string) []recommendations.RIUtilization {
 		return []recommendations.RIUtilization{{ReservedInstanceID: id, UtilizationPercent: 30}}