fix(api): dedupe per-cell variants in by_service savings to stop ~6x over-report

internal/api/handler_dashboard.go

@@ -191,6 +191,10 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S
 // coverageByKey via config.AccountConfigKey(account, provider, service).
 // Pass coverageByKey=nil to disable scaling (every rec counts fully).
 //
+// Variants of the same physical-resource cell are deduped to a single
+// representative before summing (see bestVariantPerCell) so the per-(term,
+// payment) fan-out does not over-report savings; details in the function body.
+//
 // Recs without a CloudAccountID and recs whose triple has no entry in the
 // map all count at full weight — this matches the pre-#196 behaviour for
 // un-configured accounts. Zero-coverage configs are excluded from the map
@@ -218,12 +222,24 @@ func summarizeRecommendationsWithCoverage(
 	recs []config.RecommendationRecord,
 	coverageByKey map[string]float64,
 ) (float64, map[string]ServiceSavings) {
+	// Dedupe to one representative variant per physical-resource cell BEFORE
+	// summing. After PR #195's per-(term, payment) fan-out, a single physical
+	// resource produces up to 6 rec rows (2 terms x 3 payments). Those rows are
+	// mutually-exclusive ALTERNATIVES, not additive: the operator can only buy
+	// one commitment for that resource. Summing every variant inflated both the
+	// headline total and each by_service[svc].PotentialSavings by ~6x. The
+	// headline KPI already avoids this on the frontend by grouping per cell
+	// (frontend/src/recommendations.ts cellKey / groupRecsByCell, and the
+	// dashboard.ts:139-142 comment about "~6x inflation of summing every
+	// variant"); this reducer is the backend equivalent.
+	representatives := bestVariantPerCell(recs, coverageByKey)
+
 	var total float64
 	byService := make(map[string]ServiceSavings)
-	for _, rec := range recs {
-		scaled := scaledSavings(rec, coverageByKey)
+	for _, rep := range representatives {
+		scaled := rep.scaled
 		total += scaled
-		svc := byService[rec.Service]
+		svc := byService[rep.rec.Service]
 		svc.PotentialSavings += scaled
 		// CurrentSavings is the committed/realized monthly savings for the
 		// service: the full 100%-coverage potential (rec.Savings) projected
@@ -236,11 +252,72 @@ func summarizeRecommendationsWithCoverage(
 		// Issue #908: this field was previously never set, so the Home
 		// chart's current-savings underlay always rendered as $0.
 		svc.CurrentSavings += scaled
-		byService[rec.Service] = svc
+		byService[rep.rec.Service] = svc
 	}
 	return total, byService
 }
 
+// cellRepresentative is the chosen variant for one physical-resource cell plus
+// its already-computed coverage-scaled savings (cached so callers don't re-run
+// scaledSavings).
+type cellRepresentative struct {
+	rec    config.RecommendationRecord
+	scaled float64
+}
+
+// recCellKey composes the physical-resource cell key for a recommendation:
+// (provider, cloud_account_id, service, region, resource_type, engine). Recs
+// sharing this key are per-(term, payment) ALTERNATIVES for the same resource,
+// not additive entries.
+//
+// Cross-language duplication note: this MUST stay aligned with the frontend
+// cellKey in frontend/src/recommendations.ts, which joins the same six fields
+// with "|" in this order: provider, cloud_account_id, service, region,
+// resource_type, engine. The two implementations must bucket the exact same
+// variants so the backend by_service rollup and the frontend per-cell grouping
+// agree. A nil CloudAccountID maps to the empty segment, matching the
+// frontend's nullish-coalescing of cloud_account_id to an empty string.
+func recCellKey(rec config.RecommendationRecord) string {
+	account := ""
+	if rec.CloudAccountID != nil {
+		account = *rec.CloudAccountID
+	}
+	return strings.Join([]string{
+		rec.Provider,
+		account,
+		rec.Service,
+		rec.Region,
+		rec.ResourceType,
+		rec.Engine,
+	}, "|")
+}
+
+// bestVariantPerCell collapses recs to one representative per physical-resource
+// cell, keeping the variant with the MAXIMUM coverage-scaled savings. Max (not
+// sum) is correct because the variants are mutually-exclusive alternatives, and
+// the best realistic single purchase is the per-cell potential a user expects.
+// Order of first-seen cells is preserved for deterministic aggregation.
+func bestVariantPerCell(
+	recs []config.RecommendationRecord,
+	coverageByKey map[string]float64,
+) []cellRepresentative {
+	indexByCell := make(map[string]int, len(recs))
+	reps := make([]cellRepresentative, 0, len(recs))
+	for _, rec := range recs {
+		scaled := scaledSavings(rec, coverageByKey)
+		key := recCellKey(rec)
+		if idx, ok := indexByCell[key]; ok {
+			if scaled > reps[idx].scaled {
+				reps[idx] = cellRepresentative{rec: rec, scaled: scaled}
+			}
+			continue
+		}
+		indexByCell[key] = len(reps)
+		reps = append(reps, cellRepresentative{rec: rec, scaled: scaled})
+	}
+	return reps
+}
+
 // scaledSavings returns rec.Savings * min(max(coverage, 0), 100) / 100 when
 // a coverage entry exists for the rec's (account, provider, service) triple.
 // Otherwise returns rec.Savings unchanged.

internal/api/handler_dashboard_test.go

@@ -45,10 +45,16 @@ func TestHandler_getDashboardSummary(t *testing.T) {
 	mockScheduler := new(MockScheduler)
 	mockStore := new(MockConfigStore)
 
+	// Two rds recs for DISTINCT physical resources (different resource_type) so
+	// they aggregate additively, plus one ec2 rec. Distinct cells are required
+	// because summarizeRecommendationsWithCoverage now dedupes per
+	// physical-resource cell: two recs sharing the same
+	// (provider, account, service, region, resource_type, engine) key would be
+	// treated as mutually-exclusive term/payment variants and collapsed to one.
 	recommendations := []config.RecommendationRecord{
-		{Service: "rds", Savings: 100.0},
-		{Service: "ec2", Savings: 200.0},
-		{Service: "rds", Savings: 50.0},
+		{Service: "rds", ResourceType: "db.r5.large", Savings: 100.0},
+		{Service: "ec2", ResourceType: "m5.large", Savings: 200.0},
+		{Service: "rds", ResourceType: "db.r5.xlarge", Savings: 50.0},
 	}
 
 	globalCfg := &config.GlobalConfig{
@@ -354,6 +360,59 @@ func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing
 	require.Positive(t, byService["rds"].CurrentSavings)
 }
 
+// TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell is the
+// regression for the by_service ~6x over-report: after PR #195's per-(term,
+// payment) fan-out, one physical-resource cell yields up to 6 mutually-exclusive
+// variant recs. The reducer used to sum every variant into
+// by_service[svc].PotentialSavings, inflating it ~6x. The fix dedupes to one
+// representative per cell (the MAX scaled savings) before summing.
+//
+// Pre-fix this test FAILS: the old sum-all-variants code returned
+// PotentialSavings = 600 (100+200+300) for cell-1 plus 50 for cell-2 = 650,
+// whereas the correct per-cell MAX is 300 + 50 = 350.
+func TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell(t *testing.T) {
+	acct := "acct-A"
+	// cellVariant builds one term/payment variant of the SAME physical-resource
+	// cell: identical provider/account/service/region/resource_type/engine, only
+	// the savings (and notionally term/payment) differ.
+	cellVariant := func(region, resourceType, engine string, term int, payment string, savings float64) config.RecommendationRecord {
+		a := acct
+		return config.RecommendationRecord{
+			Provider:       "aws",
+			Service:        "ec2",
+			Region:         region,
+			ResourceType:   resourceType,
+			Engine:         engine,
+			Term:           term,
+			Payment:        payment,
+			Savings:        savings,
+			CloudAccountID: &a,
+		}
+	}
+
+	recs := []config.RecommendationRecord{
+		// Cell 1: three variants of the same resource. MAX scaled savings = 300.
+		cellVariant("us-east-1", "m5.large", "", 1, "no_upfront", 100),
+		cellVariant("us-east-1", "m5.large", "", 1, "partial_upfront", 200),
+		cellVariant("us-east-1", "m5.large", "", 3, "all_upfront", 300),
+		// Cell 2: a distinct resource (different resource_type). Only variant: 50.
+		cellVariant("us-east-1", "r5.large", "", 1, "no_upfront", 50),
+	}
+
+	// No coverage override: scaledSavings returns rec.Savings unchanged.
+	total, byService := summarizeRecommendationsWithCoverage(recs, nil)
+
+	// by_service must equal the sum of per-cell MAX savings (300 + 50), NOT the
+	// sum of all variants (100+200+300+50 = 650).
+	assert.InDelta(t, 350.0, byService["ec2"].PotentialSavings, 0.0001,
+		"by_service potential must sum per-cell MAX (300+50), not all variants")
+	assert.InDelta(t, 350.0, byService["ec2"].CurrentSavings, 0.0001,
+		"by_service current must dedupe identically to potential")
+	// The headline total dedupes the same way.
+	assert.InDelta(t, 350.0, total, 0.0001,
+		"total must sum per-cell MAX (300+50), not all variants")
+}
+
 func TestHandler_getUpcomingPurchases(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)