fix(recfilter): dedupe each term and payment variant against the full held quantity

CHANGELOG.md

@@ -312,6 +312,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- **Duplicate check:** a recent purchase now reduces every term and payment variant of a recommendation, not only the first one in input order, for EC2 as well as every other service. Each `term|payment` group starts from the full held quantity and consumes it in input order, so one held unit against four variants of 2 yields 1, 1, 1, 1 (was 1, 2, 2, 2). Providers pick this up on their next `pkg` pin bump. Fixes #703.
 - **`common`:** a `Recommendation`, and a `PurchaseResult` carrying one (with
   no `Error`), now decodes from JSON when `details` holds an object. `details`
   is decoded into the typed pointer for the recommendation's service through

pkg/recfilter/dedupe.go

@@ -3,6 +3,7 @@ package recfilter
 import (
 	"context"
 	"fmt"
+	"maps"
 	"strings"
 	"time"
 
@@ -77,7 +78,7 @@ func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context,
 	existingMap := buildExistingCommitmentsMap(recentExisting, d.Logf)
 	d.Logf.printf("    [DuplicateChecker] Existing map has %d unique keys", len(existingMap))
 
-	passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, newEC2Dedupe(recentExisting, d.Logf), d.Logf)
+	passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, recentExisting, d.Logf)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -249,18 +250,20 @@ func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map
 
 // adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments.
 // Returns (passed, filtered) where filtered contains recs whose count was reduced to zero.
-func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, ec2 *ec2Dedupe, logf Logf) (passed, filtered []common.Recommendation, err error) {
+func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, recent []common.Commitment, logf Logf) (passed, filtered []common.Recommendation, err error) {
 	passed = make([]common.Recommendation, 0, len(recs))
 	filtered = make([]common.Recommendation, 0)
+	variants := &variantGroups{held: existingMap, recent: recent, logf: logf, groups: map[string]*variantGroup{}}
 
 	for _rvc := range recs {
 		rec := recs[_rvc]
 		var adjusted common.Recommendation
 		var err error
-		if ec2.handles(rec) {
-			adjusted, err = ec2.adjust(rec)
+		group := variants.of(rec)
+		if isAWSCompute(rec.Provider, rec.Service) {
+			adjusted, err = group.ec2.adjust(rec)
 		} else {
-			adjusted, err = adjustSingleRecommendation(rec, existingMap, logf)
+			adjusted, err = adjustSingleRecommendation(rec, group.held, logf)
 		}
 		if err != nil {
 			return nil, nil, err
@@ -275,6 +278,32 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing
 	return passed, filtered, nil
 }
 
+// variantGroups holds the held budgets of each term and payment variant.
+// Variants are alternative ways to buy the same resource, so each group starts
+// from the full held quantity; recommendations of one variant share it in
+// input order. An empty term or payment is a group of its own (#703).
+type variantGroups struct {
+	groups map[string]*variantGroup
+	held   map[string]int
+	logf   Logf
+	recent []common.Commitment
+}
+
+type variantGroup struct {
+	held map[string]int
+	ec2  *ec2Dedupe
+}
+
+func (v *variantGroups) of(rec common.Recommendation) *variantGroup {
+	variant := strings.ToLower(strings.TrimSpace(rec.Term)) + "|" + strings.ToLower(strings.TrimSpace(rec.PaymentOption))
+	g, ok := v.groups[variant]
+	if !ok {
+		g = &variantGroup{held: maps.Clone(v.held), ec2: newEC2Dedupe(v.recent, v.logf)}
+		v.groups[variant] = g
+	}
+	return g
+}
+
 // adjustSingleRecommendation adjusts a single recommendation based on existing
 // commitments. A reduced count scales the money fields with it.
 func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) (common.Recommendation, error) {

pkg/recfilter/dedupe_ec2.go

@@ -189,10 +189,6 @@ func ec2DimsFromDetails(details common.ServiceDetails) (platform, tenancy, scope
 	return "", "", "", ""
 }
 
-func (d *ec2Dedupe) handles(rec common.Recommendation) bool {
-	return isAWSCompute(rec.Provider, rec.Service)
-}
-
 // adjust consumes matching entries, fewest wildcards first, and resizes the
 // recommendation once to what is left. Entries are shared across
 // recommendations, so which recommendation is suppressed depends on their

pkg/recfilter/dedupe_variants_test.go

@@ -0,0 +1,116 @@
+package recfilter
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func variantRec(term, payment string) common.Recommendation {
+	return common.Recommendation{ResourceType: "Premium_P1", Region: "eastus", Count: 2, Term: term, PaymentOption: payment}
+}
+
+func adjustCounts(t *testing.T, held []common.Commitment, recs []common.Recommendation) []int {
+	t.Helper()
+	d := NewDuplicateChecker(0)
+	passed, _, err := d.AdjustRecommendationsForExisting(context.Background(), recs, &fakeServiceClient{commitments: held})
+	require.NoError(t, err)
+	counts := make([]int, len(passed))
+	for i, r := range passed {
+		counts[i] = r.Count
+	}
+	return counts
+}
+
+func heldOne() []common.Commitment {
+	return []common.Commitment{{ResourceType: "Premium_P1", Region: "eastus", Count: 1, State: "active", StartDate: time.Now().Add(-time.Hour)}}
+}
+
+// #703: term and payment variants are alternatives for the same resource, so
+// each variant sees the full held quantity.
+func TestDedupe_VariantsEachSeeFullHeldQuantity(t *testing.T) {
+	t.Parallel()
+	recs := []common.Recommendation{
+		variantRec("1yr", "upfront"), variantRec("1yr", "monthly"),
+		variantRec("3yr", "upfront"), variantRec("3yr", "monthly"),
+	}
+	assert.Equal(t, []int{1, 1, 1, 1}, adjustCounts(t, heldOne(), recs))
+}
+
+func TestDedupe_SameVariantStillSharesBudgetInOrder(t *testing.T) {
+	t.Parallel()
+	recs := []common.Recommendation{
+		variantRec("1yr", "upfront"), variantRec("1yr", "upfront"),
+		variantRec("3yr", "upfront"),
+	}
+	assert.Equal(t, []int{1, 2, 1}, adjustCounts(t, heldOne(), recs))
+}
+
+func TestDedupe_VariantsWithDifferentKeysUnaffected(t *testing.T) {
+	t.Parallel()
+	other := variantRec("1yr", "monthly")
+	other.ResourceType = "Premium_P2"
+	recs := []common.Recommendation{variantRec("1yr", "upfront"), other}
+	assert.Equal(t, []int{1, 2}, adjustCounts(t, heldOne(), recs))
+}
+
+// An empty term or payment is its own group: it is not folded into a named
+// variant, because an unknown variant cannot be assumed to be the same offer.
+func TestDedupe_EmptyTermOrPaymentIsOwnGroup(t *testing.T) {
+	t.Parallel()
+	recs := []common.Recommendation{
+		variantRec("", ""), variantRec("", ""),
+		variantRec("1yr", ""), variantRec("", "upfront"),
+	}
+	assert.Equal(t, []int{1, 2, 1, 1}, adjustCounts(t, heldOne(), recs))
+}
+
+func TestDedupe_ElastiCacheCoverWithVariants(t *testing.T) {
+	t.Parallel()
+	held := []common.Commitment{{
+		Provider: common.ProviderAWS, Service: common.ServiceElastiCache, ResourceType: "cache.r6g.large",
+		Region: "us-east-1", Engine: "redis", Count: 4, State: "active", StartDate: time.Now().Add(-time.Hour),
+	}}
+	mk := func(term string) common.Recommendation {
+		return common.Recommendation{
+			Provider: common.ProviderAWS, Service: common.ServiceElastiCache, ResourceType: "cache.r6g.large",
+			Region: "us-east-1", Count: 6, Term: term, PaymentOption: "no-upfront",
+			Details: &common.CacheDetails{Engine: "valkey"},
+		}
+	}
+	// 4 Redis nodes cover 5 Valkey nodes, once per variant: 6 -> 1, 1.
+	assert.Equal(t, []int{1, 1}, adjustCounts(t, held, []common.Recommendation{mk("1yr"), mk("3yr")}))
+}
+
+func TestDedupe_EC2VariantsEachSeeFullHeldQuantity(t *testing.T) {
+	t.Parallel()
+	held := []common.Commitment{{
+		Provider: common.ProviderAWS, Service: common.ServiceEC2, ResourceType: "m5.large", Region: "us-east-1",
+		Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region", Count: 1, State: "active", StartDate: time.Now().Add(-time.Hour),
+	}}
+	mk := func(term, payment string) common.Recommendation {
+		return common.Recommendation{
+			Provider: common.ProviderAWS, Service: common.ServiceEC2, ResourceType: "m5.large", Region: "us-east-1",
+			Count: 2, Term: term, PaymentOption: payment,
+			Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "region"},
+		}
+	}
+	got := adjustCounts(t, held, []common.Recommendation{
+		mk("1yr", "all-upfront"), mk("1yr", "no-upfront"), mk("3yr", "all-upfront"), mk("1yr", "all-upfront"),
+	})
+	assert.Equal(t, []int{1, 1, 1, 2}, got)
+}
+
+func TestDedupe_VariantNormalization(t *testing.T) {
+	t.Parallel()
+	recs := []common.Recommendation{variantRec("1yr", "upfront"), variantRec(" 1YR ", " UPFRONT ")}
+	assert.Equal(t, []int{1, 2}, adjustCounts(t, heldOne(), recs), "same variant after trim and case folding shares the budget")
+	recs = []common.Recommendation{variantRec("1yr", "upfront"), variantRec(" 1YR ", "monthly")}
+	assert.Equal(t, []int{1, 1}, adjustCounts(t, heldOne(), recs), "term folds case, payment differs")
+	recs = []common.Recommendation{variantRec("1yr", "monthly"), variantRec("1yr", " MONTHLY ")}
+	assert.Equal(t, []int{1, 2}, adjustCounts(t, heldOne(), recs), "payment folds case")
+}

providers/azure/services/managedredis/held_reservation_dedupe_test.go

@@ -201,33 +201,6 @@ func TestHeldReservation_Table(t *testing.T) {
 	}
 }
 
-// TestHeldReservation_VariantPin pins today's shared-budget behavior of the
-// recent-purchase guard on GetRecommendations-shaped output. The held
-// quantity is consumed by the first matching variant in input order, so the P1Y
-// upfront variant takes it and every other variant is untouched; the P1Y rec
-// comes before the P3Y rec in the input and the pin depends on that order.
-// The key has no term or payment option, so this is a known over-buy that this
-// test pins and does not endorse; tracked in go#703.
-func TestHeldReservation_VariantPin(t *testing.T) {
-	t.Parallel()
-	c := NewClient(nil, "sub", "eastus")
-	recs := variantsOf(t, c, legacyRedisRec("P1Y"), legacyRedisRec("P3Y"))
-	require.Len(t, recs, 4)
-	c.SetInventoryFactories(factoriesRows(nil, redisRow("held", "Premium_P1", "eastus", 1)))
-
-	passed, err := adjust(t, c, recs)
-	require.NoError(t, err)
-	type variant struct{ term, payment string }
-	got := map[variant]int{}
-	for _, r := range passed {
-		got[variant{r.Term, r.PaymentOption}] = r.Count
-	}
-	assert.Equal(t, map[variant]int{
-		{"1yr", "upfront"}: 1, {"1yr", "monthly"}: 2,
-		{"3yr", "upfront"}: 2, {"3yr", "monthly"}: 2,
-	}, got)
-}
-
 func TestNewClient_DefaultInventoryFactories(t *testing.T) {
 	t.Parallel()
 	for name, c := range map[string]*Client{