From ba559138f6e9f3ec1d7a0e9cb62ad0b74bbbdad4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 18:31:16 +0200 Subject: [PATCH 1/2] fix(purchase): stamp the purchased EC2 offering class on history savePurchaseHistory wrote a literal "convertible" into purchase_history.offering_class for every AWS EC2 purchase, but the EC2 client buys the class from global_config.offering_class, which can be "standard". The marketplace-sell guard reads this column instead of asking AWS, so every Standard RI was recorded as unsellable. The guard also matched only the legacy "ec2" slug, so canonical "compute" recs got no class at all. Resolve the configured class once into an ec2types.OfferingClassType using the same mapping as the pinned EC2 client ("" means convertible), stamp that value for both slugs, and refuse the run with errUnknownOfferingClass before any purchase when the value is unknown. Closes #112 --- internal/purchase/coverage_extra_test.go | 3 +- internal/purchase/execution.go | 40 ++++++--- .../purchase/money_path_regression_test.go | 81 +++++++++++++++++++ 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 4dcb88cd0..8973f057e 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -10,6 +10,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/aws/aws-sdk-go-v2/aws" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -1073,7 +1074,7 @@ func TestManager_SavePurchaseHistory_RevocationWindow(t *testing.T) { } result := common.PurchaseResult{Success: true, CommitmentID: "commit-rev-001"} - err := manager.savePurchaseHistory(ctx, exec, plan, rec, result, "acct-1") + err := manager.savePurchaseHistory(ctx, exec, plan, rec, result, "acct-1", ec2types.OfferingClassTypeConvertible) require.NoError(t, err) require.NotNil(t, captured) diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 5ecbe816f..450bb8e52 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -567,6 +567,10 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf if globalCfg != nil { opts.OfferingClass = globalCfg.OfferingClass } + offeringClass, err := resolveEC2OfferingClass(opts.OfferingClass) + if err != nil { + return 0, 0, nil, fmt.Errorf("purchase[%s]: %w", exec.ExecutionID, err) + } // Build the list of selected indices once so the fan-out closure only // has to look up rec[i] (no second pass over the full slice). @@ -625,7 +629,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // there are no concurrent writes to totals, exec.Recommendations, or // purchaseErrors (05-N2). Do NOT move the aggregation inside the FanOut // closure or run it concurrently with the fan-out. - totalSavings, totalUpfront, purchaseErrors = m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) + totalSavings, totalUpfront, purchaseErrors = m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, offeringClass, results) return totalSavings, totalUpfront, purchaseErrors, nil } @@ -635,7 +639,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // so the aggregation logic is single-threaded — no concurrent writes to // totals, purchaseErrors, or exec.Recommendations[i] regardless of how // many recs ran in parallel. -func (m *Manager) aggregatePurchaseOutcomes(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, results []execution.Result[recPurchaseOutcome]) (float64, float64, []string) { //nolint:gocritic // unnamedResult: return names would conflict with body locals +func (m *Manager) aggregatePurchaseOutcomes(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, offeringClass ec2types.OfferingClassType, results []execution.Result[recPurchaseOutcome]) (float64, float64, []string) { //nolint:gocritic // unnamedResult: return names would conflict with body locals var totalSavings, totalUpfront float64 var purchaseErrors []string for _, r := range results { @@ -670,7 +674,7 @@ func (m *Manager) aggregatePurchaseOutcomes(ctx context.Context, exec *config.Pu exec.Recommendations[i].PurchaseID = v.purchase.CommitmentID totalSavings += rec.Savings totalUpfront += rec.UpfrontCost - if histErr := m.savePurchaseHistory(ctx, exec, plan, rec, v.purchase, accountID); histErr != nil { + if histErr := m.savePurchaseHistory(ctx, exec, plan, rec, v.purchase, accountID, offeringClass); histErr != nil { // The purchase SUCCEEDED but its purchase_history row failed to // persist. Do NOT add this to purchaseErrors — that would flip the // execution to "failed" and tempt the user to re-approve a purchase @@ -925,7 +929,7 @@ func SingleCloudAccountIDFromRecs(recs []config.RecommendationRecord) (*string, // caller can record an audit gap on the execution: a swallowed failure here // used to leave the execution silently "completed" with no purchase_history // row, making the purchase invisible in the History view (issue #621). -func (m *Manager) savePurchaseHistory(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, rec config.RecommendationRecord, result common.PurchaseResult, accountID string) error { +func (m *Manager) savePurchaseHistory(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, rec config.RecommendationRecord, result common.PurchaseResult, accountID string, offeringClass ec2types.OfferingClassType) error { purchasedAt := time.Now() historyRecord := &config.PurchaseHistoryRecord{ AccountID: accountID, @@ -950,13 +954,11 @@ func (m *Manager) savePurchaseHistory(ctx context.Context, exec *config.Purchase // Revoke button (issue #290). Azure-only in Phase 1; nil for AWS/GCP. RevocationWindowClosesAt: config.RevocationWindowClosesAtFor(rec.Provider, purchasedAt), } - // Stamp offering_class for AWS EC2 Reserved Instances. CUDly always - // purchases EC2 RIs with offering-class=Convertible (the EC2 client - // hard-wires OfferingClassTypeConvertible). Stamping at write time - // ensures the marketplace-sell guard can distinguish eligibility without - // an extra AWS round-trip for CUDly-purchased rows (issue #292). - if rec.Provider == string(common.ProviderAWS) && rec.Service == string(common.ServiceEC2) { - historyRecord.OfferingClass = string(ec2types.OfferingClassTypeConvertible) + // The marketplace-sell guard reads this column instead of asking AWS + // (issue #292), so it must be the class the EC2 client was asked to buy. + svc := m.mapServiceType(rec.Service) + if rec.Provider == string(common.ProviderAWS) && (svc == common.ServiceEC2 || svc == common.ServiceCompute) { + historyRecord.OfferingClass = string(offeringClass) } if err := m.config.SavePurchaseHistory(ctx, historyRecord); err != nil { logging.Errorf("Failed to save history: %v", err) @@ -1162,6 +1164,22 @@ func (m *Manager) mapServiceType(service string) common.ServiceType { return common.ServiceType(service) } +var errUnknownOfferingClass = errors.New("unknown EC2 RI offering class") + +// resolveEC2OfferingClass mirrors the pinned EC2 client's resolveOfferingClassType, +// where "" means convertible, so history records the class the client buys. +func resolveEC2OfferingClass(s string) (ec2types.OfferingClassType, error) { + switch s { + case "", string(ec2types.OfferingClassTypeConvertible): + return ec2types.OfferingClassTypeConvertible, nil + case string(ec2types.OfferingClassTypeStandard): + return ec2types.OfferingClassTypeStandard, nil + default: + return "", fmt.Errorf("%w %q: must be %q or %q", errUnknownOfferingClass, s, + ec2types.OfferingClassTypeConvertible, ec2types.OfferingClassTypeStandard) + } +} + // mapServiceSlug returns the common.ServiceType for a canonical or legacy // service slug. // diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index 776ddae2f..043ebcca2 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -10,6 +10,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-sdk-go-v2/aws" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -76,6 +77,86 @@ func captureIdempotencyTokens(t *testing.T, exec *config.PurchaseExecution) map[ return tokens } +// TestOfferingClassStampedOnHistoryMatchesPurchase is the issue #112 regression +// guard: purchase_history.offering_class must be the class the EC2 client was +// asked to buy, for both the legacy "ec2" and canonical "compute" slugs. +func TestOfferingClassStampedOnHistoryMatchesPurchase(t *testing.T) { + cases := []struct { + name string + configured string + service string + svcType common.ServiceType + want ec2types.OfferingClassType + }{ + {"standard legacy slug", "standard", "ec2", common.ServiceEC2, ec2types.OfferingClassTypeStandard}, + {"standard canonical slug", "standard", "compute", common.ServiceCompute, ec2types.OfferingClassTypeStandard}, + {"convertible", "convertible", "ec2", common.ServiceEC2, ec2types.OfferingClassTypeConvertible}, + {"unset means the client default", "", "compute", common.ServiceCompute, ec2types.OfferingClassTypeConvertible}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{OfferingClass: tc.configured}, nil) + var history *config.PurchaseHistoryRecord + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")). + Run(func(args mock.Arguments) { history = args.Get(1).(*config.PurchaseHistoryRecord) }). + Return(nil).Once() + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", mock.Anything, tc.svcType, "us-east-1").Return(mockServiceClient, nil) + var requested string + mockServiceClient.On("PurchaseCommitment", mock.Anything, + mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions"), + ).Run(func(args mock.Arguments) { + requested = args.Get(2).(common.PurchaseOptions).OfferingClass + }).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-112"}, nil) + + manager := &Manager{config: mockStore, email: mockEmail, providerFactory: mockFactory} + exec := &config.PurchaseExecution{ + ExecutionID: "exec-112", + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: tc.service, ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + }, + } + + _, _, errs, procErr := manager.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "Direct purchase"}, "111111111111", nil) + require.NoError(t, procErr) + require.Empty(t, errs) + require.NotNil(t, history) + assert.Equal(t, tc.configured, requested, "the EC2 client must be asked for the configured class") + assert.Equal(t, string(tc.want), history.OfferingClass, + "history must record the class that was bought, or the marketplace-sell guard reads it inverted") + }) + } +} + +// TestUnknownOfferingClassRefusesBeforePurchase: a class the EC2 client cannot +// resolve must stop the run before any commitment is bought. +func TestUnknownOfferingClassRefusesBeforePurchase(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockFactory := new(MockProviderFactory) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{OfferingClass: "Standard"}, nil) + + manager := &Manager{config: mockStore, providerFactory: mockFactory} + exec := &config.PurchaseExecution{ + ExecutionID: "exec-112-unknown", + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + }, + } + + _, _, errs, procErr := manager.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "Direct purchase"}, "111111111111", nil) + require.ErrorIs(t, procErr, errUnknownOfferingClass) + assert.Nil(t, errs) + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) +} + // TestRetryReusesIdempotencyToken is the issue #1012 regression guard: a retry // of a failed-but-landed execution must derive the SAME per-rec provider // idempotency token as the original attempt, so the provider dedupes and the From 8b4a9d5f2feff364034dcf0911cdbf48570eb79a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 18:53:58 +0200 Subject: [PATCH 2/2] docs(frontend): correct the offering_class stamping comment The class is now stamped from the configured offering class, not a hard-coded convertible. Comment only. --- frontend/src/history.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/history.ts b/frontend/src/history.ts index b42ee6acb..3de24240b 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -719,9 +719,9 @@ function retryThresholdReached(p: HistoryPurchase): boolean { // Conditions: // * row must be a completed purchase (status "completed" or absent); // * row must be an AWS EC2 RI (marketplace is EC2-only); -// * offering_class must be "standard" OR still unknown (empty). CUDly stamps -// "convertible" on its own EC2 purchases, but externally-created Standard -// RIs (and pre-migration rows) have an empty offering_class until the +// * offering_class must be "standard" OR still unknown (empty). CUDly stamps the +// configured class on its own EC2 purchases; externally-created RIs +// (and pre-migration rows) have an empty offering_class until the // backend lazily populates it from AWS on the list call. The backend // definitively gates -- it 400s a fetched "convertible" -- so we show the // button for unknown-class EC2 rows and let the backend decide, otherwise