fix(aws/recommendations): complete NaN/Inf money guards across parser_ri, ondemand_series, RI-utilization (follow-up to #1461)

providers/aws/ladder/adapters.go

@@ -3,6 +3,7 @@ package ladder
 import (
 	"context"
 	"fmt"
+	"math"
 	"strconv"
 	"time"
 
@@ -158,6 +159,14 @@ func mapActiveSP(sp sptypes.SavingsPlan) (ActiveSP, error) {
 		return ActiveSP{}, fmt.Errorf("ListActiveSPs: cannot parse Commitment %q for SP %s: %w",
 			commitment, *sp.SavingsPlanId, err)
 	}
+	// strconv.ParseFloat accepts "NaN"/"Inf" with a nil error; a non-finite or
+	// negative hourly commitment would flow through sumSPHourlyCost /
+	// sumExpiringSPHourlyCost into the ladder layer-state totals the engine
+	// sizes purchases from. Fail loud (a commitment is a non-negative money rate).
+	if math.IsNaN(hourly) || math.IsInf(hourly, 0) || hourly < 0 {
+		return ActiveSP{}, fmt.Errorf("ListActiveSPs: Commitment %q for SP %s is not a finite non-negative number",
+			commitment, *sp.SavingsPlanId)
+	}
 	start, err := parseSPDate("Start", sp.Start, *sp.SavingsPlanId)
 	if err != nil {
 		return ActiveSP{}, err

providers/aws/ladder/adapters_test.go

@@ -268,6 +268,25 @@ func TestSPLister_InvalidCommitmentFails(t *testing.T) {
 	assert.Contains(t, err.Error(), "cannot parse Commitment")
 }
 
+// TestSPLister_NonFiniteOrNegativeCommitmentFails verifies that a "NaN"/"Inf"
+// or negative Commitment (which strconv.ParseFloat accepts / passes through)
+// fails loud rather than flowing into the ladder layer-state SP-cost totals.
+func TestSPLister_NonFiniteOrNegativeCommitmentFails(t *testing.T) {
+	for _, bad := range []string{"NaN", "Inf", "-Inf", "-1.5"} {
+		t.Run(bad, func(t *testing.T) {
+			sp := makeSPEntry("sp-nonfinite", string(sptypes.SavingsPlanTypeCompute), bad, "", sptypes.SavingsPlanStateActive)
+			mock := &mockDescribeSP{
+				pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}},
+			}
+			lister := newSPLister(mock)
+
+			_, err := lister.ListActiveSPs(context.Background())
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "is not a finite non-negative number")
+		})
+	}
+}
+
 // TestSPLister_BadDatesFail verifies fail-loud on missing or unparseable
 // Start/End dates: a silently zero EndDate would drop the SP from
 // sumExpiringSPHourlyCost and understate expiring commitment.

providers/aws/recommendations/ondemand_series.go

@@ -3,6 +3,7 @@ package recommendations
 import (
 	"context"
 	"fmt"
+	"math"
 	"sort"
 	"strconv"
 	"time"
@@ -249,6 +250,15 @@ func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCost
 			return fmt.Errorf("cannot parse CE amount %q for day %s: %w",
 				aws.ToString(mv.Amount), dateStr, err)
 		}
+		// strconv.ParseFloat accepts "NaN"/"Inf" with a nil error; a non-finite
+		// daily amount would slip past the downstream all-zero fail-loud check
+		// (NaN != 0) and poison the ladder baseline. Fail loud here instead. No
+		// negative guard: CE unblended cost is legitimately negative on
+		// credit/refund days.
+		if math.IsNaN(usd) || math.IsInf(usd, 0) {
+			return fmt.Errorf("CE amount %q for day %s is not a finite number",
+				aws.ToString(mv.Amount), dateStr)
+		}
 		// Divide total daily USD by 24 to get USD/hr. Derived from DAILY
 		// granularity: 1 day = 24 hours; not a magic constant.
 		byDate[dateStr] = usd / 24.0

providers/aws/recommendations/ondemand_series_test.go

@@ -248,6 +248,26 @@ func TestGetOnDemandSeries_ParseErrorFails(t *testing.T) {
 	assert.Contains(t, err.Error(), "cannot parse CE amount")
 }
 
+// TestGetOnDemandSeries_NonFiniteAmountFails verifies that a "NaN"/"Inf" CE
+// amount (which strconv.ParseFloat accepts with a nil error) fails loud rather
+// than poisoning the daily series that seeds the ladder baseline.
+func TestGetOnDemandSeries_NonFiniteAmountFails(t *testing.T) {
+	for _, bad := range []string{"NaN", "Inf", "-Inf"} {
+		t.Run(bad, func(t *testing.T) {
+			pages := []*costexplorer.GetCostAndUsageOutput{{
+				ResultsByTime: []types.ResultByTime{{
+					TimePeriod: &types.DateInterval{Start: aws.String("2026-01-01")},
+					Total:      map[string]types.MetricValue{onDemandMetric: {Amount: aws.String(bad)}},
+				}},
+			}}
+			client := newOnDemandClient(&mockOnDemandCE{pages: pages})
+			_, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "is not a finite number")
+		})
+	}
+}
+
 // TestGetOnDemandSeries_BadDateFails verifies that a malformed CE period-start
 // date fails loud instead of being skipped or misdated
 // (feedback_no_silent_fallbacks).

providers/aws/recommendations/parser_ri.go

@@ -89,20 +89,19 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R
 // SDK; nil or unparseable values leave the destination at zero, which the
 // --target-coverage sizing path treats as "no signal" and skips.
 func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) {
-	if details.AverageNumberOfInstancesUsedPerHour != nil {
-		if v, err := strconv.ParseFloat(*details.AverageNumberOfInstancesUsedPerHour, 64); err == nil {
-			rec.AverageInstancesUsedPerHour = v
-		} else {
-			log.Printf("WARNING: failed to parse AverageNumberOfInstancesUsedPerHour %q for RI recommendation (service=%s, account=%s): %v", *details.AverageNumberOfInstancesUsedPerHour, rec.Service, rec.Account, err)
-		}
-	}
-	if details.AverageUtilization != nil {
-		if v, err := strconv.ParseFloat(*details.AverageUtilization, 64); err == nil {
-			rec.RecommendedUtilization = v
-		} else {
-			log.Printf("WARNING: failed to parse AverageUtilization %q for RI recommendation (service=%s, account=%s): %v", *details.AverageUtilization, rec.Service, rec.Account, err)
-		}
-	}
+	// Route through parseOptionalFloatOrWarn so a non-finite/negative value
+	// (which strconv.ParseFloat accepts / passes through) degrades to 0 rather
+	// than being stored as a live signal. The downstream --target-coverage
+	// guards are all `<= 0`, and NaN <= 0 is false, so a stored NaN would be
+	// treated as a real signal and produce NaN purchase counts.
+	//
+	// The field label carries service/account context so a warning still
+	// identifies which row was corrupt (the pre-refactor inline logs did).
+	ctx := fmt.Sprintf("service=%s account=%s", rec.Service, rec.Account)
+	rec.AverageInstancesUsedPerHour = parseOptionalFloatOrWarn(
+		"AverageNumberOfInstancesUsedPerHour ("+ctx+")", details.AverageNumberOfInstancesUsedPerHour)
+	rec.RecommendedUtilization = parseOptionalFloatOrWarn(
+		"AverageUtilization ("+ctx+")", details.AverageUtilization)
 }
 
 // parseRecommendedQuantity extracts the recommended quantity from details
@@ -117,10 +116,19 @@ func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseReco
 	_, err := fmt.Sscanf(qty, "%f", &count)
 	if err != nil {
 		if intCount, atoiErr := strconv.Atoi(qty); atoiErr == nil {
+			if intCount < 0 {
+				return 0, fmt.Errorf("recommended quantity %q is negative", qty)
+			}
 			return intCount, nil
 		}
 		return 0, fmt.Errorf("failed to parse quantity '%s' as float or int", qty)
 	}
+	// Sscanf %f accepts "NaN"/"Inf"; a non-finite quantity would corrupt the
+	// purchase count (int(math.Round(NaN)) is undefined). A negative count is
+	// likewise invalid for a purchase quantity. Fail loud on both.
+	if math.IsNaN(count) || math.IsInf(count, 0) || count < 0 {
+		return 0, fmt.Errorf("recommended quantity %q is not a finite non-negative number", qty)
+	}
 
 	return int(math.Round(count)), nil
 }
@@ -132,22 +140,17 @@ func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseReco
 // historical on-demand demand. This is the 100%-coverage baseline the dashboard
 // scaling in summarizeRecommendationsWithCoverage depends on (issue #215 audit).
 func (c *Client) parseCostInformation(details *types.ReservationPurchaseRecommendationDetail) (float64, float64, error) {
-	var estimatedSavings, savingsPercent float64
-
-	if details.EstimatedMonthlySavingsAmount != nil {
-		val, err := strconv.ParseFloat(*details.EstimatedMonthlySavingsAmount, 64)
-		if err != nil {
-			return 0, 0, fmt.Errorf("failed to parse estimated savings %q: %w", *details.EstimatedMonthlySavingsAmount, err)
-		}
-		estimatedSavings = val
+	// Route through parseOptionalFloat so a present-but-non-finite CE money value
+	// (NaN/Inf parse to a nil error under strconv.ParseFloat) is rejected the same
+	// way as on the SP path, keeping this parser and the SP parser at genuine
+	// parity. A nil pointer yields (0, nil).
+	estimatedSavings, err := parseOptionalFloat("EstimatedMonthlySavingsAmount", details.EstimatedMonthlySavingsAmount)
+	if err != nil {
+		return 0, 0, err
 	}
-
-	if details.EstimatedMonthlySavingsPercentage != nil {
-		val, err := strconv.ParseFloat(*details.EstimatedMonthlySavingsPercentage, 64)
-		if err != nil {
-			return 0, 0, fmt.Errorf("failed to parse savings percentage %q: %w", *details.EstimatedMonthlySavingsPercentage, err)
-		}
-		savingsPercent = val
+	savingsPercent, err := parseOptionalFloat("EstimatedMonthlySavingsPercentage", details.EstimatedMonthlySavingsPercentage)
+	if err != nil {
+		return 0, 0, err
 	}
 
 	return estimatedSavings, savingsPercent, nil
@@ -160,17 +163,18 @@ func (c *Client) parseCostInformation(details *types.ReservationPurchaseRecommen
 // wrong money figure (e.g. a $0 upfront on an all-upfront RI) into the
 // effective-savings math and purchase decisions with no signal.
 func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error {
+	// All money fields route through parseOptionalFloat for the non-finite guard.
 	if details.UpfrontCost != nil {
-		upfront, err := strconv.ParseFloat(*details.UpfrontCost, 64)
+		upfront, err := parseOptionalFloat("UpfrontCost", details.UpfrontCost)
 		if err != nil {
-			return fmt.Errorf("failed to parse upfront cost %q: %w", *details.UpfrontCost, err)
+			return err
 		}
 		rec.CommitmentCost = upfront
 	}
 	if details.EstimatedMonthlyOnDemandCost != nil {
-		onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64)
+		onDemand, err := parseOptionalFloat("EstimatedMonthlyOnDemandCost", details.EstimatedMonthlyOnDemandCost)
 		if err != nil {
-			return fmt.Errorf("failed to parse estimated monthly on-demand cost %q: %w", *details.EstimatedMonthlyOnDemandCost, err)
+			return err
 		}
 		rec.OnDemandCost = onDemand
 	} else {
@@ -183,9 +187,9 @@ func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types.
 	// RecurringStandardMonthlyCost is the recurring charge per month for this RI.
 	// It is distinct from CommitmentCost (upfront) and EstimatedMonthlySavingsAmount.
 	if details.RecurringStandardMonthlyCost != nil {
-		monthly, err := strconv.ParseFloat(*details.RecurringStandardMonthlyCost, 64)
+		monthly, err := parseOptionalFloat("RecurringStandardMonthlyCost", details.RecurringStandardMonthlyCost)
 		if err != nil {
-			return fmt.Errorf("failed to parse recurring standard monthly cost %q: %w", *details.RecurringStandardMonthlyCost, err)
+			return err
 		}
 		rec.RecurringMonthlyCost = &monthly
 	}

providers/aws/recommendations/parser_ri_test.go

@@ -77,6 +77,38 @@ func TestParseRecommendedQuantity(t *testing.T) {
 			expected:    0,
 			expectError: true,
 		},
+		{
+			name: "NaN quantity rejected",
+			details: &types.ReservationPurchaseRecommendationDetail{
+				RecommendedNumberOfInstancesToPurchase: aws.String("NaN"),
+			},
+			expected:    0,
+			expectError: true,
+		},
+		{
+			name: "Inf quantity rejected",
+			details: &types.ReservationPurchaseRecommendationDetail{
+				RecommendedNumberOfInstancesToPurchase: aws.String("+Inf"),
+			},
+			expected:    0,
+			expectError: true,
+		},
+		{
+			name: "negative float quantity rejected",
+			details: &types.ReservationPurchaseRecommendationDetail{
+				RecommendedNumberOfInstancesToPurchase: aws.String("-3.0"),
+			},
+			expected:    0,
+			expectError: true,
+		},
+		{
+			name: "negative int quantity rejected",
+			details: &types.ReservationPurchaseRecommendationDetail{
+				RecommendedNumberOfInstancesToPurchase: aws.String("-3"),
+			},
+			expected:    0,
+			expectError: true,
+		},
 	}
 
 	for _, tt := range tests {
@@ -305,21 +337,21 @@ func TestParseRecommendationDetail_MalformedCostFields(t *testing.T) {
 			mutate: func(d *types.ReservationPurchaseRecommendationDetail) {
 				d.UpfrontCost = aws.String("not-a-number")
 			},
-			errContains: `failed to parse upfront cost "not-a-number"`,
+			errContains: `failed to parse UpfrontCost "not-a-number"`,
 		},
 		{
 			name: "malformed on-demand cost",
 			mutate: func(d *types.ReservationPurchaseRecommendationDetail) {
 				d.EstimatedMonthlyOnDemandCost = aws.String("$650.00")
 			},
-			errContains: `failed to parse estimated monthly on-demand cost "$650.00"`,
+			errContains: `failed to parse EstimatedMonthlyOnDemandCost "$650.00"`,
 		},
 		{
 			name: "malformed recurring monthly cost",
 			mutate: func(d *types.ReservationPurchaseRecommendationDetail) {
 				d.RecurringStandardMonthlyCost = aws.String("")
 			},
-			errContains: `failed to parse recurring standard monthly cost ""`,
+			errContains: `failed to parse RecurringStandardMonthlyCost ""`,
 		},
 	}
 
@@ -541,6 +573,18 @@ func TestParseRIUtilizationSignals(t *testing.T) {
 			wantAvgInstances: 0,
 			wantUtilization:  0,
 		},
+		{
+			// NaN/Inf parse to a nil error under strconv.ParseFloat; they must
+			// degrade to 0, not be stored as a live signal (NaN <= 0 is false,
+			// so a stored NaN would drive NaN purchase counts in sizing).
+			name: "non-finite values degrade to zero",
+			details: &types.ReservationPurchaseRecommendationDetail{
+				AverageNumberOfInstancesUsedPerHour: aws.String("NaN"),
+				AverageUtilization:                  aws.String("+Inf"),
+			},
+			wantAvgInstances: 0,
+			wantUtilization:  0,
+		},
 	}
 
 	for _, tt := range tests {

providers/aws/recommendations/parser_sp.go

@@ -198,6 +198,10 @@ func (c *Client) parseSavingsPlansRecommendations(
 //     variants) as valid with a nil error, so without this guard a corrupt
 //     non-finite money value would flow straight through to the scheduler and
 //     frontend. Reject non-finite values the same way as unparseable ones.
+//   - non-nil pointer that parses to a negative value: returns (0, error). Every
+//     field this parses (SP/RI cost, commitment, savings, savings %, utilization)
+//     is non-negative by construction; a negative is corrupt in the same way a
+//     non-finite value is. Mirrors the repo's stricter parseSPFloat helper.
 func parseOptionalFloat(field string, s *string) (float64, error) {
 	if s == nil {
 		return 0, nil
@@ -209,6 +213,9 @@ func parseOptionalFloat(field string, s *string) (float64, error) {
 	if math.IsNaN(val) || math.IsInf(val, 0) {
 		return 0, fmt.Errorf("%s %q is not a finite number", field, *s)
 	}
+	if val < 0 {
+		return 0, fmt.Errorf("%s %q is negative, which is invalid for a financial metric", field, *s)
+	}
 	return val, nil
 }