fix(cli): count zero-count recommendations removed by --max-instances or --min-count as dropped

CHANGELOG.md

@@ -13,6 +13,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- `--max-instances` drop summary: a zero-Count recommendation swept out by the
+  `--min-count` floor no longer drives the tally negative (which panicked in
+  `DropSummary.Add`), and removed zero-Count entries are now counted and named
+  (#1742).
 - `--max-instances` and `--min-count` no longer apply to Savings Plans, whose
   Count of 1 is not an instance count: a Savings Plan neither uses up the
   instance cap nor is dropped by the floor (#1613).

cmd/multi_service.go

@@ -497,26 +497,32 @@ func reportInstanceLimit(before, after []common.Recommendation, belowMinCount, t
 		maxInstances, len(before), totalBefore)
 	AppLogger.Printf("   Keeping the %s. The following are reduced or dropped:\n", rule)
 
+	// after is a prefix of before, so membership is positional: every entry past
+	// len(after) was removed, whatever its Count (a zero-Count entry equals its
+	// own "kept" count of 0 and must still be counted as removed).
 	reduced, dropped := 0, 0
 	for i := range before {
 		rec := before[i]
-		kept := 0
-		if i < len(after) {
-			kept = after[i].Count
-		}
-		switch {
-		case kept == rec.Count:
+		if i >= len(after) {
+			dropped++
+			AppLogger.Printf("   • dropped: %s %s %s (%d instances)\n", rec.Service, rec.Region, rec.ResourceType, rec.Count)
 			continue
-		case kept > 0:
+		}
+		if kept := after[i].Count; kept != rec.Count {
 			reduced++
 			AppLogger.Printf("   • reduced: %s %s %s %d → %d instances\n", rec.Service, rec.Region, rec.ResourceType, rec.Count, kept)
-		default:
-			dropped++
-			AppLogger.Printf("   • dropped: %s %s %s (%d instances)\n", rec.Service, rec.Region, rec.ResourceType, rec.Count)
 		}
 	}
 
-	drops.Add(common.DropMaxInstances, dropped-belowMinCount)
+	// The floor only removes entries from the tail of what the cap kept, so
+	// they are a subset of the removed entries. DropSummary.Add panics on a
+	// negative count; surface the broken invariant with context instead.
+	budgetDropped := dropped - belowMinCount
+	if budgetDropped < 0 {
+		panic(fmt.Sprintf("reportInstanceLimit: %d recommendations attributed to --min-count-after-cap but only %d were removed (before=%d, after=%d)",
+			belowMinCount, dropped, len(before), len(after)))
+	}
+	drops.Add(common.DropMaxInstances, budgetDropped)
 	AppLogger.Printf("   Proceeding with %d instances across %d recommendations (%d reduced, %d dropped).\n",
 		CalculateTotalInstances(after), len(after), reduced, dropped)
 }

cmd/multi_service_drop_attribution_test.go

@@ -0,0 +1,61 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func dropRec(rt string, count int) common.Recommendation {
+	return common.Recommendation{Service: common.ServiceEC2, Region: "us-east-1", ResourceType: rt, Count: count}
+}
+
+// A zero-Count entry swept out by the --min-count floor (behind a truncated
+// tail) used to be uncounted as a budget drop (kept == rec.Count == 0) yet
+// subtracted as a floor drop, driving the --max-instances tally to -1 and
+// panicking in DropSummary.Add (#1742).
+func TestApplyGlobalInstanceLimitZeroCountTailDoesNotPanic(t *testing.T) {
+	drops := common.NewDropSummary()
+	recs := []common.Recommendation{dropRec("m5.large", 4), dropRec("c5.large", 0), dropRec("m5.xlarge", 6)}
+
+	var got []common.Recommendation
+	require.NotPanics(t, func() {
+		got = applyGlobalInstanceLimit(recs, Config{MaxInstances: 7, MinCount: 4}, rankBySavingsPercentage, drops)
+	})
+	require.Len(t, got, 1)
+	assert.Equal(t, "m5.large", got[0].ResourceType)
+	// The truncated m5.xlarge and the zero-Count c5.large are both floor
+	// drops, counted once each and none attributed to the budget.
+	assert.Equal(t, 2, drops.Total())
+	assert.Contains(t, drops.FormatOneLine(), "min-count-after-cap=2")
+	assert.NotContains(t, drops.FormatOneLine(), "--max-instances")
+}
+
+// A zero-Count entry the budget left out is a removed recommendation and is
+// attributed to --max-instances, not skipped by the kept == rec.Count check.
+func TestReportInstanceLimitCountsZeroCountBudgetDrop(t *testing.T) {
+	before := []common.Recommendation{dropRec("m5.large", 6), dropRec("m5.xlarge", 6), dropRec("c5.large", 0)}
+	after := ApplyInstanceLimit(before, 6)
+	require.Len(t, after, 1)
+
+	drops := common.NewDropSummary()
+	out := captureAppOutput(t, func() {
+		reportInstanceLimit(before, after, 0, CalculateTotalInstances(before), 6, rankBySavingsPercentage, drops)
+	})
+	assert.Equal(t, 2, drops.Total())
+	assert.Contains(t, out, "c5.large")
+	assert.Contains(t, out, "2 dropped")
+}
+
+// More floor drops than removed entries is an impossible state and must fail
+// loudly with context rather than clamp or hit DropSummary.Add's bare panic.
+func TestReportInstanceLimitImpossibleAttributionPanicsWithContext(t *testing.T) {
+	before := []common.Recommendation{dropRec("m5.large", 6)}
+	assert.PanicsWithValue(t,
+		"reportInstanceLimit: 2 recommendations attributed to --min-count-after-cap but only 0 were removed (before=1, after=1)",
+		func() {
+			reportInstanceLimit(before, before, 2, 6, 6, rankBySavingsPercentage, common.NewDropSummary())
+		})
+}