From bd044ddbcfdecf15c461a37f9c3d3fbd9521e95a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 16:15:07 +0100 Subject: [PATCH 1/2] fix(purchase): suppress every variant when a legacy EC2 recommendation has no details A failed execution saved before Details existed (#453) is retried from its persisted recs without re-matching. BuildSuppressions read the empty Details as the known variant {az, "", ""}, so the actually-bought platform/tenancy stayed unsuppressed. Treat an EC2 rec with no platform or tenancy as an unknown variant, which writes the existing NULL-variant wildcard row and over-suppresses rather than risk a double buy. Part of #1232 Co-Authored-By: Claude Opus 5.5 --- internal/config/suppressions_build.go | 10 +++++- internal/config/suppressions_build_test.go | 24 ++++++++++++++ .../suppression_variant_integration_test.go | 31 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index c2af92f0e..d0662e9b7 100644 --- a/internal/config/suppressions_build.go +++ b/internal/config/suppressions_build.go @@ -21,7 +21,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 EC2 rec 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 +42,13 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { b.unknownVariant = true return b } + if rec.Service == "ec2" && 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 } diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 1f42b5681..d8003f283 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -152,3 +152,27 @@ 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) +} diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go index 37728557e..8de1f79f4 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/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") +} From b16fbbf967b2c8c5a20fdc3b2efc9b2b7b678058 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 16:39:35 +0100 Subject: [PATCH 2/2] fix(config): wildcard legacy compute-slug EC2 suppressions AWS recs can carry the canonical "compute" service slug as well as "ec2". The legacy wildcard (no platform/tenancy) only matched "ec2", so compute-slug recs kept a narrow empty variant and could be double-bought (#1232). Co-Authored-By: Claude Opus 5.5 --- internal/config/suppressions_build.go | 9 +++++--- internal/config/suppressions_build_test.go | 27 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index d0662e9b7..2bf6e7c94 100644 --- a/internal/config/suppressions_build.go +++ b/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,8 +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. An EC2 rec with no platform or tenancy -// (legacy, no Details) is treated the same way (issue #1232). +// 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 { @@ -42,7 +43,9 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { b.unknownVariant = true return b } - if rec.Service == "ec2" && v.Platform == "" && v.Tenancy == "" { + 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. diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index d8003f283..e8c85f22e 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -176,3 +176,30 @@ func TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard(t *testing.T) { 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) + }) + } +}