fix(purchase): suppress every variant when a legacy EC2 recommendation has no details

internal/config/suppressions_build.go

@@ -3,6 +3,7 @@ package config
 import (
 	"time"
 
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
 )
 
@@ -21,7 +22,8 @@ type suppressionBucket struct {
 // behavior, which over-suppresses (under-buys) rather than leaving the
 // purchased capacity on show. The purchase paths reject such recs before
 // they get here (RecIdentityKey reads the same Details), so this is a
-// fallback, not a path in normal use.
+// fallback, not a path in normal use. An AWS EC2 rec (service ec2 or compute) with no
+// platform or tenancy (legacy, no Details) is treated the same way (issue #1232).
 func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket {
 	accountID := ""
 	if rec.CloudAccountID != nil {
@@ -41,6 +43,15 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket {
 		b.unknownVariant = true
 		return b
 	}
+	isAWSEC2 := rec.Provider == string(common.ProviderAWS) &&
+		(rec.Service == string(common.ServiceEC2) || rec.Service == string(common.ServiceCompute))
+	if isAWSEC2 && v.Platform == "" && v.Tenancy == "" {
+		// Saved before Details existed (#453) and retried without re-matching:
+		// the bought platform/tenancy is unknown, so a known {az,"",""} would
+		// leave the real variant unsuppressed (double-buy). Wildcard instead.
+		b.unknownVariant = true
+		return b
+	}
 	b.variant = v
 	return b
 }

internal/config/suppressions_build_test.go

@@ -152,3 +152,54 @@ func TestPurchaseSuppression_AppliesTo(t *testing.T) {
 	assert.True(t, legacy.AppliesTo(&SuppressionVariant{"us-east-1b", "Windows", "dedicated"}), "a legacy row is a wildcard")
 	assert.True(t, legacy.AppliesTo(nil))
 }
+
+// TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard: an EC2 rec saved
+// before Details existed has an unknown platform and tenancy, so it must
+// suppress every variant (issue #1232). Non-EC2 and fully detailed recs keep
+// their known variant.
+func TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard(t *testing.T) {
+	base := RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, AvailabilityZone: "us-east-1a"}
+	for name, details := range map[string][]byte{"nil": nil, "empty": {}, "null": []byte(`null`), "no keys": []byte(`{}`)} {
+		t.Run(name, func(t *testing.T) {
+			rec := base
+			rec.Details = details
+			sups := BuildSuppressions([]RecommendationRecord{rec}, "exec-1", &GlobalConfig{}, time.Now())
+			require.Len(t, sups, 1)
+			assert.Nil(t, sups[0].Variant)
+			assert.True(t, sups[0].AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}))
+		})
+	}
+
+	full := base
+	full.Details = []byte(`{"platform":"Linux/UNIX","tenancy":"default"}`)
+	sups := BuildSuppressions([]RecommendationRecord{full}, "exec-1", &GlobalConfig{}, time.Now())
+	require.Len(t, sups, 1)
+	assert.Equal(t, &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}, sups[0].Variant)
+}
+
+// TestBuildSuppressions_ComputeSlugEC2: AWS EC2 recs under the canonical
+// "compute" slug follow the same wildcard rule as "ec2"; a rec with only a
+// platform or only a tenancy keeps its narrow variant.
+func TestBuildSuppressions_ComputeSlugEC2(t *testing.T) {
+	base := RecommendationRecord{Provider: "aws", Service: "compute", Region: "us-east-1", ResourceType: "m5.large", Count: 1, AvailabilityZone: "us-east-1a"}
+	sups := BuildSuppressions([]RecommendationRecord{base}, "exec-1", &GlobalConfig{}, time.Now())
+	require.Len(t, sups, 1)
+	assert.Nil(t, sups[0].Variant)
+	assert.True(t, sups[0].AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}))
+
+	for name, tc := range map[string]struct {
+		details []byte
+		want    SuppressionVariant
+	}{
+		"platform only": {[]byte(`{"platform":"Linux/UNIX"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", ""}},
+		"tenancy only":  {[]byte(`{"tenancy":"default"}`), SuppressionVariant{"us-east-1a", "", "default"}},
+	} {
+		t.Run(name, func(t *testing.T) {
+			rec := base
+			rec.Details = tc.details
+			sups := BuildSuppressions([]RecommendationRecord{rec}, "exec-1", &GlobalConfig{}, time.Now())
+			require.Len(t, sups, 1)
+			assert.Equal(t, &tc.want, sups[0].Variant)
+		})
+	}
+}

internal/scheduler/suppression_variant_integration_test.go

@@ -71,3 +71,34 @@ func TestSuppressionsScopedToPurchasedVariant_RealPostgres(t *testing.T) {
 		"legacy-a": 4, "legacy-b": 4,
 	}, counts)
 }
+
+// TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres is the issue #1232
+// regression: a retried execution persisted before Details existed carries an
+// EC2 rec with no platform or tenancy. Its suppression must wildcard the
+// 6-tuple, so the later Linux/UNIX recommendation that was actually bought is
+// filtered instead of re-offered.
+func TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres(t *testing.T) {
+	ctx := t.Context()
+	pg := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations")
+	store := config.NewPostgresStore(pg.DB)
+
+	later := config.RecommendationRecord{
+		ID: "later", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large",
+		Count: 3, Term: 1, Payment: "all-upfront", AvailabilityZone: "us-east-1a",
+		Details: []byte(`{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`),
+	}
+	require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), []config.RecommendationRecord{later}, nil))
+
+	legacy := later
+	legacy.ID = "legacy"
+	legacy.Count = 3
+	legacy.Details = nil
+	sups := config.BuildSuppressions([]config.RecommendationRecord{legacy}, uuid.NewString(), &config.GlobalConfig{}, time.Now())
+	require.Len(t, sups, 1)
+	require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { return store.CreateSuppressionTx(ctx, tx, &sups[0]) }))
+
+	s := &Scheduler{config: store, isLambda: true}
+	got, err := s.ListRecommendations(ctx, config.RecommendationFilter{})
+	require.NoError(t, err)
+	assert.Empty(t, got, "the bought variant must be fully suppressed")
+}