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 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