fix(cli): re-derive count-linear projections after Count changes

cmd/helpers.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"math"
 	"os"
 	"strings"
 	"sync"
@@ -137,20 +138,20 @@ func CalculateTotalInstances(recs []common.Recommendation) int {
 // ApplyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the
 // logging sink. Substantive documentation lives on recfilter.ApplyCoverage.
 func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation {
-	return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, nil)
+	return mapRecs(recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, nil), syncDataWarehouseNodes)
 }
 
 // applyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the
 // logging sink. Substantive documentation lives on recfilter.ApplyCoverage.
 func applyCoverage(recs []common.Recommendation, coverage float64, drops *common.DropSummary) []common.Recommendation {
-	return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, drops)
+	return mapRecs(recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, drops), syncDataWarehouseNodes)
 }
 
 // ApplyTargetCoverage delegates to recfilter.ApplyTargetCoverage, wiring
 // AppLogger as the logging sink. Substantive documentation (the RI/SP sizing
 // formulas and the #338 flag-name history) lives on recfilter.ApplyTargetCoverage.
 func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops *common.DropSummary) []common.Recommendation {
-	return recfilter.ApplyTargetCoverage(recs, targetPct, AppLogger.Printf, drops)
+	return mapRecs(recfilter.ApplyTargetCoverage(recs, targetPct, AppLogger.Printf, drops), syncDataWarehouseNodes)
 }
 
 // applySizing chooses target-coverage or coverage sizing.
@@ -187,7 +188,9 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro
 // alone leaves the row claiming the savings of instances the run will not
 // buy. The run summary and the purchase report then overstate the benefit
 // of a capped run, which is the wrong direction to be wrong in on a money
-// path (#1830).
+// path (#1830). The count-linear fields that are not money (ProjectedCoverage,
+// ProjectedUtilization, Redshift NumberOfNodes) are re-derived from the new
+// Count by refreshCountLinear (#1845).
 func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation {
 	if maxInstances <= 0 {
 		return recs
@@ -210,6 +213,7 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm
 		if rec.Count > remaining {
 			adjusted = common.ScaleRecommendationCosts(rec, float64(remaining)/float64(rec.Count))
 			adjusted.Count = remaining
+			adjusted = refreshCountLinear(adjusted)
 		}
 		result = append(result, adjusted)
 		// Only a positive Count consumes budget. Subtracting a non-positive
@@ -222,6 +226,91 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm
 	return result
 }
 
+// refreshCountLinear re-derives the count-linear, non-money fields after a
+// caller outside the shared sizing code changed rec.Count (#1845): the
+// projections the target-coverage sizer wrote, and the Redshift node count
+// mirrored from Count at parse. It is idempotent on a row whose Count did not
+// change, so callers may apply it to every row of a slice.
+func refreshCountLinear(rec common.Recommendation) common.Recommendation {
+	return syncDataWarehouseNodes(rederiveProjections(rec))
+}
+
+// mapRecs applies fn to every rec into a new slice, keeping a nil input nil.
+func mapRecs(recs []common.Recommendation, fn func(common.Recommendation) common.Recommendation) []common.Recommendation {
+	if recs == nil {
+		return nil
+	}
+	out := make([]common.Recommendation, len(recs))
+	for i := range recs {
+		out[i] = fn(recs[i])
+	}
+	return out
+}
+
+// rederiveProjections mirrors the projection formulas of the shared sizer
+// (recfilter applyTargetCoverageRI) for the rec's current Count. A projection
+// that is zero ("not computed") stays zero. Savings Plans, rows without a
+// usage signal (zero, negative, NaN or infinite avg) and rows without a positive
+// Count are returned unchanged.
+// RDS ProjectedCoverage is a family aggregate set by family-NU sizing, which a
+// per-row formula would corrupt, so only RDS ProjectedUtilization is refreshed.
+func rederiveProjections(rec common.Recommendation) common.Recommendation {
+	avg := rec.AverageInstancesUsedPerHour
+	if common.IsSavingsPlan(rec.Service) || rec.Count <= 0 || !(avg > 0) || math.IsInf(avg, 1) {
+		return rec
+	}
+	n := float64(rec.Count)
+	if rec.ProjectedUtilization != 0 {
+		rec.ProjectedUtilization = math.Min(100, avg/n*100)
+	}
+	if rec.ProjectedCoverage != 0 && rec.ExistingCoverageKnown && rec.Service != common.ServiceRDS {
+		rec.ProjectedCoverage = math.Min(100, rec.ExistingCoveragePct+n/avg*100)
+	}
+	return rec
+}
+
+// syncDataWarehouseNodes re-syncs NumberOfNodes (and the single-node/multi-node
+// ClusterType derived from it) with rec.Count for AWS Redshift recs, the only
+// parser that mirrors Count into NumberOfNodes; other users of
+// DataWarehouseDetails keep their value. The details are copied, never written
+// through the shared pointer.
+func syncDataWarehouseNodes(rec common.Recommendation) common.Recommendation {
+	if rec.Service != common.ServiceRedshift || rec.Count <= 0 {
+		return rec
+	}
+	d, isPtr, ok := dataWarehouseDetails(rec.Details)
+	if !ok || d.NumberOfNodes == rec.Count {
+		return rec
+	}
+	d.NumberOfNodes = rec.Count
+	if d.ClusterType == "single-node" || d.ClusterType == "multi-node" {
+		d.ClusterType = "multi-node"
+		if rec.Count == 1 {
+			d.ClusterType = "single-node"
+		}
+	}
+	if isPtr {
+		rec.Details = &d
+	} else {
+		rec.Details = d
+	}
+	return rec
+}
+
+// dataWarehouseDetails returns a copy of a pointer or value DataWarehouseDetails.
+func dataWarehouseDetails(d common.ServiceDetails) (cp common.DataWarehouseDetails, isPtr, ok bool) {
+	switch v := d.(type) {
+	case *common.DataWarehouseDetails:
+		if v == nil {
+			return cp, false, false
+		}
+		return *v, true, true
+	case common.DataWarehouseDetails:
+		return v, false, true
+	}
+	return cp, false, false
+}
+
 // ConfirmPurchase requires terminal input; totalSavings is monthly savings, not purchase cost.
 func ConfirmPurchase(totalInstances int, totalSavings float64) bool {
 	if !term.IsTerminal(int(os.Stdin.Fd())) { //nolint:gosec // G115: uintptr->int for file descriptor; FD values are always small positive integers

cmd/helpers_count_linear_test.go

@@ -0,0 +1,237 @@
+package main
+
+import (
+	"context"
+	"math"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func riRec(count int, avg, existing, cov, util float64) common.Recommendation {
+	return common.Recommendation{
+		Service: common.ServiceEC2, CommitmentType: common.CommitmentReservedInstance, Region: "us-east-1", ResourceType: "m5.large",
+		Count: count, AverageInstancesUsedPerHour: avg, ExistingCoveragePct: existing,
+		ExistingCoverageKnown: true, ProjectedCoverage: cov, ProjectedUtilization: util,
+		EstimatedSavings: 1000, CommitmentCost: 2000, OnDemandCost: 3000,
+	}
+}
+
+func TestRederiveProjections(t *testing.T) {
+	rds := riRec(10, 125, 10, 55, 100)
+	rds.Service = common.ServiceRDS
+	noKnown := riRec(10, 125, 0, 80, 100)
+	noKnown.ExistingCoverageKnown = false
+	sp := riRec(10, 125, 0, 0, 90)
+	sp.Service = common.ServiceSavingsPlansCompute
+
+	tests := []struct {
+		name      string
+		rec       common.Recommendation
+		cov, util float64
+	}{
+		{"issue scenario: cap 100 -> 10", riRec(10, 125, 0, 80, 100), 8.0, 100},
+		{"unclamped util, clamped cov (hand-computed)", riRec(10, 4, 0, 50, 100), 100, 40},
+		{"unclamped cov with existing (hand-computed)", riRec(10, 125, 10, 80, 100), 18.0, 100},
+		{"zero projections stay zero", riRec(10, 125, 10, 0, 0), 0, 0},
+		{"only util computed stays so", riRec(10, 4, 0, 0, 100), 0, 40},
+		{"avg zero untouched", riRec(10, 0, 0, 80, 100), 80, 100},
+		{"count zero untouched", riRec(0, 125, 0, 80, 100), 80, 100},
+		{"existing coverage unknown: cov untouched", noKnown, 80, 100},
+		{"RDS: cov untouched (family aggregate), util refreshed", rds, 55, 100},
+		{"SP untouched", sp, 0, 90},
+		{"NaN avg untouched", riRec(10, math.NaN(), 0, 80, 100), 80, 100},
+		{"+Inf avg untouched", riRec(10, math.Inf(1), 0, 80, 100), 80, 100},
+		{"negative avg untouched", riRec(10, -3, 0, 80, 100), 80, 100},
+		{"negative count untouched", riRec(-5, 125, 0, 80, 100), 80, 100},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := rederiveProjections(tt.rec)
+			assert.InDelta(t, tt.cov, got.ProjectedCoverage, 1e-9)
+			assert.InDelta(t, tt.util, got.ProjectedUtilization, 1e-9)
+		})
+	}
+}
+
+func TestSyncDataWarehouseNodes(t *testing.T) {
+	ptr := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"}
+	rec := common.Recommendation{Service: common.ServiceRedshift, Count: 3, Details: ptr}
+
+	got := syncDataWarehouseNodes(rec)
+	assert.Equal(t, 3, got.Details.(*common.DataWarehouseDetails).NumberOfNodes)
+	assert.Equal(t, 10, ptr.NumberOfNodes, "the shared pointer must not be written through")
+
+	rec.Count = 1
+	assert.Equal(t, "single-node", syncDataWarehouseNodes(rec).Details.(*common.DataWarehouseDetails).ClusterType)
+
+	val := common.Recommendation{Service: common.ServiceRedshift, Count: 2, Details: common.DataWarehouseDetails{NumberOfNodes: 1, ClusterType: "single-node"}}
+	d := syncDataWarehouseNodes(val).Details.(common.DataWarehouseDetails)
+	assert.Equal(t, 2, d.NumberOfNodes)
+	assert.Equal(t, "multi-node", d.ClusterType)
+
+	azure := common.Recommendation{Service: common.ServiceDataWarehouse, Count: 4, Details: common.DataWarehouseDetails{ClusterType: "dedicated-sql-pool"}}
+	assert.Equal(t, azure, syncDataWarehouseNodes(azure), "non-Redshift recs are never touched")
+	custom := common.Recommendation{Service: common.ServiceDataWarehouse, Count: 4, Details: common.DataWarehouseDetails{NumberOfNodes: 9, ClusterType: "single-node"}}
+	assert.Equal(t, custom, syncDataWarehouseNodes(custom), "non-Redshift NumberOfNodes stays 9")
+
+	same := common.Recommendation{Count: 10, Details: ptr}
+	assert.Same(t, ptr, syncDataWarehouseNodes(same).Details, "equal values keep pointer identity")
+	var nilPtr *common.DataWarehouseDetails
+	assert.NotPanics(t, func() { syncDataWarehouseNodes(common.Recommendation{Count: 2, Details: nilPtr}) })
+	zero := common.Recommendation{Count: 0, Details: ptr}
+	assert.Same(t, ptr, syncDataWarehouseNodes(zero).Details)
+	other := common.Recommendation{Count: 2, Details: &common.DatabaseDetails{Engine: "mysql"}}
+	assert.Equal(t, other, syncDataWarehouseNodes(other))
+}
+
+// The coverage mirror must match what the shared sizer writes.
+func TestRefreshCountLinearMatchesSizerOutput(t *testing.T) {
+	in := make([]common.Recommendation, 0, 4)
+	for _, p := range []struct{ avg, existing float64 }{{125, 0}, {40.5, 30}, {400, 79.5}, {33.3, 10}} {
+		in = append(in, riRec(50, p.avg, p.existing, 0, 0))
+	}
+	out := ApplyTargetCoverage(in, 80, nil)
+	require.Len(t, out, len(in))
+	for i := range out {
+		require.NotZero(t, out[i].ProjectedCoverage)
+		assert.Equal(t, out[i], refreshCountLinear(out[i]), "pool %d drifted", i)
+	}
+}
+
+// Issue #1845: the cap changes Count, so the projections must follow.
+func TestApplyInstanceLimitRederivesProjections(t *testing.T) {
+	capped := riRec(100, 125, 0, 80, 100)
+	untouched := riRec(5, 10, 0, 40, 100)
+	untouched.ResourceType = "c5.large"
+	zero := riRec(100, 125, 0, 0, 0)
+	zero.ResourceType = "r5.large"
+	got := ApplyInstanceLimit([]common.Recommendation{untouched, capped, zero}, 10)
+	require.Len(t, got, 2)
+	assert.Equal(t, untouched, got[0], "a row the cap does not truncate is byte-identical")
+	assert.Equal(t, 5, got[1].Count)
+	// remaining budget 5 after the first row: cov = 5/125*100 = 4.0
+	assert.InDelta(t, 4.0, got[1].ProjectedCoverage, 1e-9)
+
+	got = ApplyInstanceLimit([]common.Recommendation{capped}, 10)
+	assert.InDelta(t, 8.0, got[0].ProjectedCoverage, 1e-9)
+	assert.InDelta(t, 100, got[0].ProjectedUtilization, 1e-9)
+
+	got = ApplyInstanceLimit([]common.Recommendation{zero}, 10)
+	assert.Zero(t, got[0].ProjectedCoverage)
+	assert.Zero(t, got[0].ProjectedUtilization)
+
+	noSignal := riRec(100, 0, 0, 80, 100)
+	assert.Equal(t, 80.0, ApplyInstanceLimit([]common.Recommendation{noSignal}, 10)[0].ProjectedCoverage)
+}
+
+func TestApplyInstanceLimitSyncsRedshiftNodes(t *testing.T) {
+	src := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"}
+	rec := common.Recommendation{Service: common.ServiceRedshift, Count: 10, Details: src}
+	got := ApplyInstanceLimit([]common.Recommendation{rec}, 3)
+	assert.Equal(t, 3, got[0].Details.(*common.DataWarehouseDetails).NumberOfNodes)
+	assert.Equal(t, 10, src.NumberOfNodes)
+}
+
+func TestApplyCountOverrideRederivesProjections(t *testing.T) {
+	got := ApplyCountOverride([]common.Recommendation{riRec(100, 125, 0, 80, 100)}, 200)
+	assert.InDelta(t, 100, got[0].ProjectedCoverage, 1e-9) // 200/125*100 clamps
+	assert.InDelta(t, 62.5, got[0].ProjectedUtilization, 1e-9)
+
+	rds := riRec(100, 125, 0, 55, 100)
+	rds.Service = common.ServiceRDS
+	got = ApplyCountOverride([]common.Recommendation{rds}, 10)
+	assert.Equal(t, 55.0, got[0].ProjectedCoverage, "RDS family coverage survives")
+}
+
+func TestAdjustRecsForDuplicatesRederivesProjections(t *testing.T) {
+	ctx := context.Background()
+	client := &MockServiceClient{}
+	client.On("GetExistingCommitments", ctx).Return([]common.Commitment{
+		{ResourceType: "m5.large", Region: "us-east-1", Count: 60, State: "active", StartDate: time.Now().Add(-time.Hour)},
+	}, nil)
+	got, err := adjustRecsForDuplicates(ctx, []common.Recommendation{riRec(100, 125, 0, 80, 100)}, client)
+	require.NoError(t, err)
+	require.Len(t, got, 1)
+	require.Equal(t, 40, got[0].Count)
+	assert.InDelta(t, 32.0, got[0].ProjectedCoverage, 1e-9) // 40/125*100
+}
+
+// Fixture-based: sizer output flows through the run-wide cap unchanged in shape.
+func TestTargetCoverageThenGlobalCapAgree(t *testing.T) {
+	sized := ApplyTargetCoverage([]common.Recommendation{riRec(300, 125, 0, 0, 0)}, 80, nil)
+	require.Len(t, sized, 1)
+	require.Equal(t, 100, sized[0].Count)
+	require.InDelta(t, 80.0, sized[0].ProjectedCoverage, 1e-9)
+	got := applyGlobalInstanceLimit(sized, Config{MaxInstances: 10}, rankBySavingsPercentage, common.NewDropSummary())
+	require.Len(t, got, 1)
+	assert.Equal(t, 10, got[0].Count)
+	assert.InDelta(t, 8.0, got[0].ProjectedCoverage, 1e-9)
+}
+
+func TestTargetCoverageSyncsRedshiftNodes(t *testing.T) {
+	src := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"}
+	rec := riRec(10, 5, 0, 0, 0)
+	rec.Service = common.ServiceRedshift
+	rec.Details = src
+	got := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil)
+	require.Len(t, got, 1)
+	assert.Equal(t, 4, got[0].Count)
+	assert.Equal(t, 4, got[0].Details.(*common.DataWarehouseDetails).NumberOfNodes)
+	assert.Equal(t, 10, src.NumberOfNodes)
+}
+
+func rdsFamilyRec(count int, avg, cov, util float64) common.Recommendation {
+	r := riRec(count, avg, 0, cov, util)
+	r.Service = common.ServiceRDS
+	return r
+}
+
+func TestRDSCoverageSurvivesCapAndDedupe(t *testing.T) {
+	got := ApplyInstanceLimit([]common.Recommendation{rdsFamilyRec(100, 40, 55, 40)}, 10)
+	assert.Equal(t, 55.0, got[0].ProjectedCoverage, "family coverage untouched by the cap")
+	assert.InDelta(t, 100, got[0].ProjectedUtilization, 1e-9) // 40/10 clamps
+
+	ctx := context.Background()
+	client := &MockServiceClient{}
+	client.On("GetExistingCommitments", ctx).Return([]common.Commitment{
+		{ResourceType: "m5.large", Region: "us-east-1", Count: 60, State: "active", StartDate: time.Now().Add(-time.Hour)},
+	}, nil)
+	dd := checkDuplicates(ctx, []common.Recommendation{rdsFamilyRec(100, 20, 55, 20)}, client, false, common.NewDropSummary())
+	require.Len(t, dd, 1)
+	require.Equal(t, 40, dd[0].Count)
+	assert.Equal(t, 55.0, dd[0].ProjectedCoverage)
+	assert.InDelta(t, 50, dd[0].ProjectedUtilization, 1e-9) // 20/40
+}
+
+func TestCheckDuplicatesRederivesProjections(t *testing.T) {
+	ctx := context.Background()
+	client := &MockServiceClient{}
+	client.On("GetExistingCommitments", ctx).Return([]common.Commitment{
+		{ResourceType: "m5.large", Region: "us-east-1", Count: 60, State: "active", StartDate: time.Now().Add(-time.Hour)},
+	}, nil)
+	got := checkDuplicates(ctx, []common.Recommendation{riRec(100, 125, 0, 80, 100)}, client, false, common.NewDropSummary())
+	require.Len(t, got, 1)
+	require.Equal(t, 40, got[0].Count)
+	assert.InDelta(t, 32.0, got[0].ProjectedCoverage, 1e-9)
+}
+
+func TestApplyCoverageSyncsRedshiftNodes(t *testing.T) {
+	src := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"}
+	rec := riRec(10, 5, 0, 0, 0)
+	rec.Service = common.ServiceRedshift
+	rec.Details = src
+	for name, got := range map[string][]common.Recommendation{
+		"ApplyCoverage": ApplyCoverage([]common.Recommendation{rec}, 40),
+		"applyCoverage": applyCoverage([]common.Recommendation{rec}, 40, common.NewDropSummary()),
+	} {
+		require.Len(t, got, 1, name)
+		assert.Equal(t, 4, got[0].Count, name)
+		assert.Equal(t, 4, got[0].Details.(*common.DataWarehouseDetails).NumberOfNodes, name)
+	}
+	assert.Equal(t, 10, src.NumberOfNodes)
+	assert.Nil(t, mapRecs(nil, syncDataWarehouseNodes), "nil stays nil")
+}