From 21394761e5f2627b18c764a5535045c219af205f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 01:26:58 +0200 Subject: [PATCH 1/3] feat(aws/ladder): export the Savings Plan state set and region rule counted as existing commitment Export ExistingSPStates (a fresh copy of the active and payment-pending set) and CountsAsExistingSP (the EC2Instance region rule), so the platform ladder reconciler can drop its copies and the pinning table. DescribeSavingsPlans and appendRegionScopedSPs now use them, so each has a single implementation. Behavior is unchanged. Closes #355 Co-Authored-By: Claude Opus 5.5 --- providers/aws/ladder/adapters.go | 41 ++++++++++++++++++--------- providers/aws/ladder/adapters_test.go | 36 +++++++++++++++++++++++ 2 files changed, 63 insertions(+), 14 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index 75cb84d0..4f9f12b5 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -25,8 +25,15 @@ type activeSPListAPI interface { DescribeSavingsPlans(ctx context.Context, params *sdksp.DescribeSavingsPlansInput, optFns ...func(*sdksp.Options)) (*sdksp.DescribeSavingsPlansOutput, error) } -// spListerStates is the set of Savings Plan states counted as existing -// commitment. +// spListerStates is the single source of the Savings Plan states counted as +// existing commitment; read it only through ExistingSPStates. +var spListerStates = []sptypes.SavingsPlanState{ + sptypes.SavingsPlanStateActive, + sptypes.SavingsPlanStatePaymentPending, +} + +// ExistingSPStates returns the Savings Plan states counted as existing +// commitment. The slice is a fresh copy, so callers cannot mutate the set. // - active: currently billing. // - payment-pending: purchase accepted, first payment in flight. A // just-purchased SP sits in this state briefly; it MUST count as existing @@ -36,14 +43,23 @@ type activeSPListAPI interface { // started, so counting them would suppress purchases the queued SP will // not cover until its start date. Revisit with L14 (expiry alignment), // which can reason about future-dated coverage explicitly. -var spListerStates = []sptypes.SavingsPlanState{ - sptypes.SavingsPlanStateActive, - sptypes.SavingsPlanStatePaymentPending, +func ExistingSPStates() []sptypes.SavingsPlanState { + return append([]sptypes.SavingsPlanState(nil), spListerStates...) +} + +// CountsAsExistingSP applies the region rule to a Savings Plan already in an +// ExistingSPStates state: EC2Instance SPs bound to another region cover that +// region's usage, not the ladder's, and counting them would inflate +// ExistingUSDPerHour and under-purchase. Compute SPs are global and always +// count. Any other plan type is not filtered here (the ladder's own type +// check handles it). The comparison is exact, case-sensitive. +func CountsAsExistingSP(planType sptypes.SavingsPlanType, spRegion, ladderRegion string) bool { + return string(planType) != spPlanTypeEC2Instance || spRegion == ladderRegion } // spListerAdapter implements the spLister interface using the AWS Savings -// Plans SDK directly. It filters to spListerStates (active + payment-pending; -// see that var for rationale) and scopes EC2Instance SPs to the ladder's +// Plans SDK directly. It filters to ExistingSPStates (active + payment-pending; +// see that func for rationale) and scopes EC2Instance SPs to the ladder's // region. Commitment strings are parsed with strconv.ParseFloat; parse errors // fail loud per feedback_strict_int_parse and feedback_no_silent_fallbacks. type spListerAdapter struct { @@ -55,7 +71,7 @@ type spListerAdapter struct { region string } -// ListActiveSPs lists the account's Savings Plans in spListerStates and maps +// ListActiveSPs lists the account's Savings Plans in ExistingSPStates and maps // them to the AWSLadder view, region-scoped: // // - EC2Instance SPs are region-bound; entries whose Region differs from the @@ -87,7 +103,7 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) page++ input := &sdksp.DescribeSavingsPlansInput{ - States: spListerStates, + States: ExistingSPStates(), NextToken: nextToken, MaxResults: aws.Int32(100), } @@ -115,10 +131,7 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) } // appendRegionScopedSPs maps one DescribeSavingsPlans page onto the -// accumulated slice, applying the region-scoping rule: EC2Instance SPs bound -// to another region cover that region's usage, not ours; including them -// would inflate ExistingUSDPerHour and under-purchase. Compute SPs are -// global and always kept (see ListActiveSPs doc). Extracted to keep +// accumulated slice, applying CountsAsExistingSP. Extracted to keep // ListActiveSPs under the cyclomatic complexity limit. func appendRegionScopedSPs(sps []ActiveSP, page []sptypes.SavingsPlan, region string) ([]ActiveSP, error) { for i := range page { @@ -126,7 +139,7 @@ func appendRegionScopedSPs(sps []ActiveSP, page []sptypes.SavingsPlan, region st if err != nil { return nil, err } - if entry.PlanType == spPlanTypeEC2Instance && entry.Region != region { + if !CountsAsExistingSP(sptypes.SavingsPlanType(entry.PlanType), entry.Region, region) { continue } sps = append(sps, entry) diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go index 27de9a40..985cbc88 100644 --- a/providers/aws/ladder/adapters_test.go +++ b/providers/aws/ladder/adapters_test.go @@ -622,3 +622,39 @@ func TestGetLayerStates_RealSPLister_NonZeroExisting(t *testing.T) { assert.InDelta(t, 2.0, *computeSP.ExistingUSDPerHour, 1e-9, "ExistingUSDPerHour must equal the SP $2/hr commitment (was 0.0 pre-fix)") } + +func TestExistingSPStates(t *testing.T) { + got := ExistingSPStates() + assert.Equal(t, []sptypes.SavingsPlanState{ + sptypes.SavingsPlanStateActive, + sptypes.SavingsPlanStatePaymentPending, + }, got) + + got[0] = sptypes.SavingsPlanStateRetired + assert.Equal(t, sptypes.SavingsPlanStateActive, ExistingSPStates()[0], "mutating a returned copy must not affect the package set") +} + +func TestCountsAsExistingSP(t *testing.T) { + tests := []struct { + name string + planType sptypes.SavingsPlanType + spRegion string + want bool + }{ + {"EC2Instance same region", sptypes.SavingsPlanTypeEc2Instance, "us-east-1", true}, + {"EC2Instance other region", sptypes.SavingsPlanTypeEc2Instance, "eu-west-1", false}, + {"EC2Instance empty region", sptypes.SavingsPlanTypeEc2Instance, "", false}, + {"EC2Instance region case differs", sptypes.SavingsPlanTypeEc2Instance, "US-EAST-1", false}, + {"Compute same region", sptypes.SavingsPlanTypeCompute, "us-east-1", true}, + {"Compute other region", sptypes.SavingsPlanTypeCompute, "eu-west-1", true}, + {"Compute empty region", sptypes.SavingsPlanTypeCompute, "", true}, + {"SageMaker other region", sptypes.SavingsPlanTypeSagemaker, "eu-west-1", true}, + {"unknown type other region", sptypes.SavingsPlanType("Other"), "eu-west-1", true}, + {"plan type case differs", sptypes.SavingsPlanType("ec2instance"), "eu-west-1", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, CountsAsExistingSP(tt.planType, tt.spRegion, "us-east-1")) + }) + } +} From 0496d5c2b6e77b1a2edd556af83e05854494087c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 01:28:43 +0200 Subject: [PATCH 2/3] refactor(aws/ladder): compare the SP type against the SDK constant and document the empty-region case Refs #355 Co-Authored-By: Claude Opus 5.5 --- providers/aws/ladder/adapters.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index 4f9f12b5..6c9a2387 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -52,9 +52,12 @@ func ExistingSPStates() []sptypes.SavingsPlanState { // region's usage, not the ladder's, and counting them would inflate // ExistingUSDPerHour and under-purchase. Compute SPs are global and always // count. Any other plan type is not filtered here (the ladder's own type -// check handles it). The comparison is exact, case-sensitive. +// check handles it). The comparison is exact, case-sensitive. An EC2Instance +// plan with an empty spRegion returns false (not counted), so callers must +// validate the region first; the adapter rejects an empty region in +// mapActiveSP. func CountsAsExistingSP(planType sptypes.SavingsPlanType, spRegion, ladderRegion string) bool { - return string(planType) != spPlanTypeEC2Instance || spRegion == ladderRegion + return planType != sptypes.SavingsPlanTypeEc2Instance || spRegion == ladderRegion } // spListerAdapter implements the spLister interface using the AWS Savings From 75fb3ff31d6c829dfc7b042869f8ccfee541fa77 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 02:52:12 +0200 Subject: [PATCH 3/3] docs(aws/ladder): correct CountsAsExistingSP's empty-region contract The doc said an EC2Instance plan with an empty spRegion returns false, but two empty regions compare equal and count. Behaviour is unchanged from before the export, so the doc is fixed and the case is pinned in the truth table. Co-Authored-By: Claude Opus 5.5 --- providers/aws/ladder/adapters.go | 8 ++++---- providers/aws/ladder/adapters_test.go | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index 6c9a2387..0c789b4a 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -52,10 +52,10 @@ func ExistingSPStates() []sptypes.SavingsPlanState { // region's usage, not the ladder's, and counting them would inflate // ExistingUSDPerHour and under-purchase. Compute SPs are global and always // count. Any other plan type is not filtered here (the ladder's own type -// check handles it). The comparison is exact, case-sensitive. An EC2Instance -// plan with an empty spRegion returns false (not counted), so callers must -// validate the region first; the adapter rejects an empty region in -// mapActiveSP. +// check handles it). The comparison is exact, case-sensitive. An empty region +// is not special-cased: an EC2Instance plan counts only when +// spRegion == ladderRegion, so two empty regions count. Callers must validate +// both regions first; the adapter rejects an empty SP region in mapActiveSP. func CountsAsExistingSP(planType sptypes.SavingsPlanType, spRegion, ladderRegion string) bool { return planType != sptypes.SavingsPlanTypeEc2Instance || spRegion == ladderRegion } diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go index 985cbc88..8b98cd5a 100644 --- a/providers/aws/ladder/adapters_test.go +++ b/providers/aws/ladder/adapters_test.go @@ -657,4 +657,7 @@ func TestCountsAsExistingSP(t *testing.T) { assert.Equal(t, tt.want, CountsAsExistingSP(tt.planType, tt.spRegion, "us-east-1")) }) } + t.Run("EC2Instance both regions empty", func(t *testing.T) { + assert.True(t, CountsAsExistingSP(sptypes.SavingsPlanTypeEc2Instance, "", "")) + }) }