From 3a15d5aea29503859c2e05e1406899d48d811b6d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 14:45:26 +0200 Subject: [PATCH 1/4] fix(purchase): refuse to re-drive AWS EC2 and Redshift purchases after an unknown outcome EC2 and Redshift purchase APIs take no client token, so the provider now sends them once and reports a lost response as "purchase outcome unknown": the commitment may exist. The platform still treated every AWS purchase as safe to re-drive, so the stranded-approval sweep, the reaper's "safe to retry" note, the executor backstop for retry successors and the Retry endpoint could all buy a second commitment. recRedriveRefusalReason now refuses AWS recs whose service dispatches to the EC2 or Redshift client (ec2, compute, redshift, data-warehouse, matched through mapServiceSlug so the guard and the executor cannot drift). A first purchase still executes; only re-drive and retry are refused. The refusal text tells the operator to check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region and to start a new purchase if the commitment is missing. The stranded-row message no longer points at the refused Retry. Bumps providers/aws to bcc0dfc, which sends these purchases once. Refs LeanerCloud/cloud-commitments-go#296 Co-Authored-By: Claude Sonnet 5.5 --- go.mod | 2 +- go.sum | 4 +- internal/api/execution_scope_test.go | 2 +- internal/api/handler_purchases.go | 11 +- .../handler_purchases_retry_fanout_test.go | 2 +- .../handler_purchases_retry_redrive_test.go | 43 ++++- internal/api/handler_purchases_test.go | 26 +-- internal/api/retry_carveout_test.go | 2 +- internal/purchase/armed_redrive_test.go | 13 +- .../purchase/aws_no_token_redrive_test.go | 161 ++++++++++++++++++ internal/purchase/manager.go | 43 ++++- internal/purchase/manager_test.go | 12 +- internal/purchase/reaper_test.go | 44 ++++- 13 files changed, 324 insertions(+), 41 deletions(-) create mode 100644 internal/purchase/aws_no_token_redrive_test.go diff --git a/go.mod b/go.mod index 1a4d8d62..7c5dbf81 100644 --- a/go.mod +++ b/go.mod @@ -85,7 +85,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261008100513-a9a426224031 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261009122502-bcc0dfc49227 github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8 github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260929223111-a32fd1a178e9 github.com/aws/aws-lambda-go v1.47.0 diff --git a/go.sum b/go.sum index 93810e40..b7096b22 100644 --- a/go.sum +++ b/go.sum @@ -94,8 +94,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261008100513-a9a426224031 h1:e3DVjk0VIGfDTW3flfD7j6aDiae9T8JINQciL3IXPjo= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261008100513-a9a426224031/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 h1:DfNEBzFS7/MaBZljLGRRRUaarfnXXZyBE/6iozzp9/k= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695/go.mod h1:d4nsy61/Ptib0SxqMg0ble3yksVm3+QbtUtmGja82PA= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261009122502-bcc0dfc49227 h1:S9pAwtTnwt02+Fo1uVWYgtmGyB1koQAHmBSficHOZ2E= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261009122502-bcc0dfc49227/go.mod h1:yJoe/zCikjjlGsy47BZTTuKUcLkLVDwqgLEYklWjlm4= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8 h1:4qx5bagtW25H5X+whaZ/3tWXP7VGyG/qcypTh8yK7Hc= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8/go.mod h1:iIvPuWLWd+SXqLHAQNdodYMUAAcq7PnwfScXWgARt5A= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260929223111-a32fd1a178e9 h1:8OuEGhDypUxV0zv+RisoPKW+i1OGst1DBcNj0gkeA98= diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 57687027..82132a4a 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -46,7 +46,7 @@ type scopeTestKind struct { // the post-scope constraint checks and reaches the mutation. func scopeTestRec(id string, account *string) config.RecommendationRecord { return config.RecommendationRecord{ - ID: id, Provider: "aws", Service: "ec2", Region: "us-east-1", + ID: id, Provider: "aws", Service: "rds", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, CloudAccountID: account, } } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 64977f02..1c522f6f 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2010,9 +2010,14 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // potentially a re-drive. purchase.RedriveRefusalReason is the same // predicate the reaper's automatic re-drive gates on: it returns a // reason exactly when the provider offers nothing that would collapse - // the second attempt onto the first, which today means Azure - // savings-plans (no server-side idempotency key, timestamp-derived - // order alias) plus any provider the predicate does not recognize. + // the second attempt onto the first, which today means AWS EC2 and + // Redshift (no client token; a lost response is reported as outcome + // unknown), Azure savings-plans (no server-side idempotency key, + // timestamp-derived order alias) plus any provider the predicate does + // not recognize. A failed EC2 or Redshift row therefore cannot be + // retried; the operator checks the account and starts a new purchase. + // If plan steps ever carry recommendations (platform#609), a failed EC2 + // step will be un-retryable too and needs its own recovery path. // // This gate runs FIRST and, unlike the threshold below, ?force=true // does NOT override it: a savings plan cannot be canceled, so there diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go index bd584086..5449c500 100644 --- a/internal/api/handler_purchases_retry_fanout_test.go +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -56,7 +56,7 @@ func fanoutPlanAccounts() []config.CloudAccount { func fanoutRecs() []config.RecommendationRecord { return []config.RecommendationRecord{ { - Provider: "aws", Service: "ec2", ResourceType: "m5.large", + Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, Term: 1, UpfrontCost: 300, Selected: true, }, diff --git a/internal/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go index d7b00137..a14f33cf 100644 --- a/internal/api/handler_purchases_retry_redrive_test.go +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -42,6 +42,8 @@ const ( redriveUnknownExecID = "22222222-3333-4444-5555-666666666604" redriveForceExecID = "22222222-3333-4444-5555-666666666605" redriveMixedExecID = "22222222-3333-4444-5555-666666666606" + redriveAWSNoTokenExecID = "22222222-3333-4444-5555-666666666607" + redriveAWSRDSExecID = "22222222-3333-4444-5555-666666666608" redriveLineageKey = "lineage-1668" ) @@ -248,7 +250,7 @@ func TestRetryOfLandedAzureSavingsPlanIsNotForceOverridable(t *testing.T) { // front of it. The AWS rec here is deliberately first, so a gate that checked // only the leading rec would pass this row straight through to Azure. func TestRetryOfMixedExecutionWithOneAzureSavingsPlanIsRefused(t *testing.T) { - failed := redriveFailedRow(redriveMixedExecID, "aws", "ec2") + failed := redriveFailedRow(redriveMixedExecID, "aws", "rds") failed.Recommendations = append(failed.Recommendations, config.RecommendationRecord{ Provider: "azure", Service: "savingsplans", @@ -301,3 +303,42 @@ func TestRetryOfFailedAzureReservationStillPurchasesOnce(t *testing.T) { assert.Equal(t, []string{common.DeriveIdempotencyToken(failed.IdempotencyKey, 0)}, tokens, "the retry must fire exactly one purchase, under the predecessor's token so Azure's two-step lookup dedupes it") } + +// TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused is the MON-02 platform +// regression guard. EC2 and Redshift purchases take no client token, so the +// provider sends them once and reports a lost response as "purchase outcome +// unknown": the commitment may exist. Retrying such a row must not create a +// successor that buys again, whatever spelling the row carries (the canonical +// compute and data-warehouse slugs dispatch to the same clients as ec2 and +// redshift) and with ?force=true too. +func TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused(t *testing.T) { + for _, tc := range []struct{ provider, service string }{ + {"aws", "ec2"}, {"aws", "compute"}, {"aws", "redshift"}, {"aws", "data-warehouse"}, {"", "ec2"}, + } { + tc := tc + t.Run(tc.provider+"/"+tc.service, func(t *testing.T) { + failed := redriveFailedRow(redriveAWSNoTokenExecID, tc.provider, tc.service) + failed.Error = "failed to purchase EC2 RI: purchase outcome unknown: the request may have been sent and the commitment may exist; reconcile before retrying" + + for _, req := range []*events.LambdaFunctionURLRequest{sessionRetryReq(), sessionRetryReqWithForce()} { + tokens, err := purchasesFiredByRetry(t, failed, req) + + assert.Empty(t, tokens, + "a retry of an EC2/Redshift row after an unknown outcome must buy nothing") + assertRedriveRefused(t, err, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + } + }) + } +} + +// TestRetryOfFailedAWSRDSRowStillPurchasesOnce is the over-blocking control: +// RDS uses a deterministic reservation ID, so its retry is safe and must keep +// working. A predicate that refused every AWS row would pass the test above. +func TestRetryOfFailedAWSRDSRowStillPurchasesOnce(t *testing.T) { + failed := redriveFailedRow(redriveAWSRDSExecID, "aws", "rds") + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + require.NoError(t, err, "an AWS RDS retry is idempotent and must still be allowed") + assert.Len(t, tokens, 1, "the RDS retry must purchase exactly once") +} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index e1eb8797..a13bd1e4 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3767,7 +3767,7 @@ func TestHandler_retryPurchase_Admin_AllowsAny(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} // Admin (Administrators-group member) modeled as a retry-any holder; the @@ -3790,7 +3790,7 @@ func TestHandler_retryPurchase_StoresHashOfEmailedToken(t *testing.T) { ExecutionID: retryExecID, Status: "failed", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100, CloudAccountID: &accountID}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100, CloudAccountID: &accountID}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} handler, mockConfig, _ := buildSessionRetryHandler(failed, session, true, false) @@ -3822,7 +3822,7 @@ func TestHandler_retryPurchase_RetryAny_AllowsAny(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "ops@example.com"} runSessionRetryAllowed(t, failed, session, true, false, sessionRetryReq()) @@ -3836,7 +3836,7 @@ func TestHandler_retryPurchase_RetryOwn_AllowsCreator(t *testing.T) { Error: "send failed: SES recipient mailbox full", CreatedByUserID: &creator, RetryAttemptN: 2, // already retried twice - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "u1@example.com"} newExec, updated := runSessionRetryAllowed(t, failed, session, false, true, sessionRetryReq()) @@ -3946,7 +3946,7 @@ func TestHandler_retryPurchase_PersistentFailure_NoMatch_AllowsRetry(t *testing. Status: "failed", Error: "send failed: SES throttle exceeded, please retry", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3960,7 +3960,7 @@ func TestHandler_retryPurchase_Threshold_BlocksAtFive_NoForce(t *testing.T) { Status: "failed", RetryAttemptN: 5, // already at the threshold CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3983,7 +3983,7 @@ func TestHandler_retryPurchase_Threshold_AllowsWithForce(t *testing.T) { Status: "failed", RetryAttemptN: 5, CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3998,7 +3998,7 @@ func TestHandler_retryPurchase_JustUnderThreshold_AllowsNoForce(t *testing.T) { Status: "failed", RetryAttemptN: 4, // n=4 < threshold=5 → allowed CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -4060,7 +4060,7 @@ func TestHandler_retryPurchase_ConcurrentRetryRace_SecondCASLoses(t *testing.T) ExecutionID: retryExecID, Status: "failed", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). Both @@ -4114,7 +4114,7 @@ func TestHandler_retryPurchase_PreservesPlanMetadata(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -4181,7 +4181,7 @@ func TestHandler_retryPurchase_PermissionConstraintsDenied(t *testing.T) { Error: "send failed: transient SES throttle", CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Region: "us-east-1", Term: 1, UpfrontCost: 5000}, + {Provider: "aws", Service: "rds", Region: "us-east-1", Term: 1, UpfrontCost: 5000}, }, } session := &Session{UserID: retryCallerID, Email: "capped@example.com"} @@ -4232,7 +4232,7 @@ func TestPersistRetryExecution_ApprovalTokenNotUUID(t *testing.T) { Status: "failed", Error: "ses throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} // Caller owns the row; retry-own authorizes it (issue #907). @@ -4261,7 +4261,7 @@ func TestPersistRetryExecution_ApprovalTokenExpiresAtSet(t *testing.T) { Status: "failed", Error: "ses throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} diff --git a/internal/api/retry_carveout_test.go b/internal/api/retry_carveout_test.go index bd6e5db9..d89e0381 100644 --- a/internal/api/retry_carveout_test.go +++ b/internal/api/retry_carveout_test.go @@ -42,7 +42,7 @@ func retryCarveoutFailedExec() *config.PurchaseExecution { Error: "send failed: transient SES throttle", CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}, + {Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}, }, } } diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index 7c6fb635..79f61edf 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -217,12 +217,13 @@ func TestFirstAzureSavingsPlanPurchaseStillExecutes(t *testing.T) { } // TestSafeProviderRetrySuccessorStillExecutes is the second negative control: -// the guard must be as narrow as the provider gap. An AWS retry successor -// reproduces its ClientToken, so the provider collapses a re-drive onto the -// original and the retry must still execute. +// the guard must be as narrow as the provider gap. An AWS RDS retry successor +// reproduces its deterministic reservation ID, so the provider collapses a +// re-drive onto the original and the retry must still execute. EC2 and +// Redshift are the AWS services that lack such a guard (see the Refuses tests). func TestSafeProviderRetrySuccessorStillExecutes(t *testing.T) { - exec := armedExecution("aws", "ec2", 3, "approved") - mgr, store, rec := armedHarness(t, common.ServiceEC2) + exec := armedExecution("aws", "rds", 3, "approved") + mgr, store, rec := armedHarness(t, common.ServiceRDS) store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}, "running") @@ -231,7 +232,7 @@ func TestSafeProviderRetrySuccessorStillExecutes(t *testing.T) { require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) assert.Equal(t, 1, rec.count(), - "an AWS retry successor dedupes at the provider via ClientToken and must still execute; the guard must not widen beyond the providers that lack a duplicate guard") + "an AWS RDS retry successor dedupes at the provider via its reservation ID and must still execute; the guard must not widen beyond the providers that lack a duplicate guard") } // TestArmedRowIsDefusedNotLeftArmed pins the disposition of a refused row. A diff --git a/internal/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go new file mode 100644 index 00000000..d187a832 --- /dev/null +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -0,0 +1,161 @@ +package purchase + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --- MON-02: AWS EC2 and Redshift have no duplicate guard -------------- +// +// The EC2 and Redshift purchase APIs take no client token or caller ID, so the +// provider sends them once and returns "purchase outcome unknown" when the +// response is lost. The commitment may exist, so nothing may buy again until +// someone has checked the account. These tests pin where that refusal reaches. + +// awsNoTokenSlugs are the rec.Service spellings that dispatch to the AWS EC2 or +// Redshift purchase clients (mapServiceSlug: canonical and legacy). +var awsNoTokenSlugs = []string{"ec2", "compute", "redshift", "data-warehouse"} + +// TestAWSRedriveGuardReachMatchesDispatchReach states the guard as an iff +// against dispatch, like the Azure savings-plans test: a value that dispatches +// to the EC2/Redshift client but is not refused is a double-buy hole. +func TestAWSRedriveGuardReachMatchesDispatchReach(t *testing.T) { + m := NewManager(ManagerConfig{}) + + candidates := []string{ + "ec2", "compute", "redshift", "data-warehouse", + "rds", "relational-db", "elasticache", "cache", "opensearch", "search", "memorydb", + "savings-plans", "savingsplans", "savings-plans-compute", "savingsplans-ec2instance", + string(common.ServiceEC2), string(common.ServiceCompute), string(common.ServiceRedshift), + string(common.ServiceDataWarehouse), string(common.ServiceRDS), string(common.ServiceNoSQL), + "EC2", "Compute", " ec2", "ec2 ", "ec2\n", "Redshift", "data_warehouse", "", "unknown", + } + + for _, provider := range []string{"aws", ""} { + for _, service := range candidates { + provider, service := provider, service + t.Run("provider="+provider+"/service="+service, func(t *testing.T) { + svc := m.mapServiceType(service) + dispatchesToNoTokenClient := svc == common.ServiceEC2 || svc == common.ServiceCompute || + svc == common.ServiceRedshift || svc == common.ServiceDataWarehouse + refused := recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}) != "" + + assert.Equal(t, dispatchesToNoTokenClient, refused, + "guard reach and dispatch reach must match for %q: dispatches=%v refused=%v", service, dispatchesToNoTokenClient, refused) + }) + } + } +} + +// TestAWSRedriveRefusalSetIsExact is the closed-set form, with negative +// controls so a predicate that refuses every AWS row (or every service named +// compute on any provider) fails here. +func TestAWSRedriveRefusalSetIsExact(t *testing.T) { + for _, provider := range []string{"aws", ""} { + for _, service := range awsNoTokenSlugs { + reason := recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}) + assert.Contains(t, reason, "Check EC2 Reserved Instances or Redshift Reserved Nodes", + "provider %q service %q has no duplicate guard and must be refused with the operator action", provider, service) + } + for _, service := range []string{"rds", "relational-db", "elasticache", "cache", "memorydb", "opensearch", "search", "savingsplans", "savings-plans"} { + assert.Empty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}), + "provider %q service %q has a provider-side duplicate guard and must stay retryable", provider, service) + } + } + // Same slugs on other providers keep their own rules. + for _, provider := range []string{"azure", "gcp"} { + for _, service := range []string{"compute", "data-warehouse"} { + assert.Empty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}), + "%s %s is not an AWS no-token path and must stay retryable", provider, service) + } + } + // Any one unsafe rec makes the whole execution unsafe. + mixed := &config.PurchaseExecution{Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "rds"}, {Provider: "aws", Service: "redshift"}, + }} + assert.NotEmpty(t, RedriveRefusalReason(mixed)) + assert.False(t, allRecsSafeToRedrive(&config.PurchaseExecution{ExecutionID: "x", Recommendations: mixed.Recommendations})) +} + +// TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes drives the real +// executor funnel: a retry successor (RetryAttemptN > 0) of an EC2/Redshift +// purchase buys nothing and is persisted as failed with the reason, while a +// first purchase of the same service still buys exactly once. +func TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes(t *testing.T) { + m := NewManager(ManagerConfig{}) + for _, service := range awsNoTokenSlugs { + service := service + t.Run(service, func(t *testing.T) { + svcType := m.mapServiceType(service) + + retry := armedExecution("aws", service, 2, "approved") + mgr, store, rec := armedHarness(t, svcType) + var saved []config.PurchaseExecution + store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + saved = append(saved, *e) + return nil + } + store.On("GetExecutionByID", mock.Anything, retry.ExecutionID).Return(retry, nil).Maybe() + expectClaim(store, retry, []string{"approved", "pending", "notified"}, "running") + body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: retry.ExecutionID}) + require.NoError(t, err) + _ = mgr.ProcessMessage(context.Background(), string(body)) + + assert.Equal(t, 0, rec.count(), "a retry after a possibly-landed %s purchase must not buy again", service) + require.NotEmpty(t, saved) + final := saved[len(saved)-1] + assert.Equal(t, "failed", final.Status) + assert.Contains(t, final.Error, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + + first := armedExecution("aws", service, 0, "approved") + mgr, store, rec = armedHarness(t, svcType) + store.On("GetExecutionByID", mock.Anything, first.ExecutionID).Return(first, nil).Maybe() + expectClaim(store, first, []string{"approved", "pending", "notified"}, "running") + require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) + assert.Equal(t, 1, rec.count(), "a first %s purchase must still go through; the guard is for retries only", service) + }) + } +} + +// TestRecoverStrandedApprovals_EC2RowIsFailedNotRedriven pins the sweep: a +// stranded approved EC2 row may have bought before the run died, so the sweep +// must make zero provider calls and tell the operator to check the account. +func TestRecoverStrandedApprovals_EC2RowIsFailedNotRedriven(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockFactory := new(MockProviderFactory) // any CreateAndValidateProvider call would panic: no expectation set + + stranded := config.PurchaseExecution{ + ExecutionID: "exec-ec2-stranded", + PlanID: "plan-ec2", + Status: "approved", + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200, Selected: true}, + }, + } + failedRow := stranded + failedRow.Status = "failed" + mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold).Return([]config.PurchaseExecution{stranded}, nil) + mockStore.On("TransitionExecutionStatus", ctx, "exec-ec2-stranded", []string{"approved"}, "failed", (*string)(nil)).Return(&failedRow, nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }).Return(nil) + + mgr := &Manager{config: mockStore, providerFactory: mockFactory, dashboardURL: "https://dashboard.example.com"} + recovered, err := mgr.RecoverStrandedApprovals(ctx) + + require.NoError(t, err) + assert.Equal(t, 1, recovered) + require.NotNil(t, saved) + assert.Equal(t, "failed", saved.Status) + assert.Contains(t, saved.Error, "start a new purchase") + assert.NotContains(t, saved.Error, "then Retry", "Retry is refused for this row, so the message must not point at it") + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) +} diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 3d1630e4..9d48f57a 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -374,7 +374,9 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { // canceled. // // Safe providers / services (issue #639): -// - AWS (all services): tag-guard or ClientToken deduplication (#636/#638). +// - AWS RDS, ElastiCache, MemoryDB, OpenSearch and Savings Plans: a +// deterministic reservation ID or ClientToken collapses a repeat onto the +// first attempt (#636/#638). // - Azure reservations (compute, relational-db, cache, nosql, memorydb, // search, data-warehouse): DoIdempotentPurchaseTwoStep performs a // tag-based lookup before purchasing (#729 / #721). @@ -382,6 +384,10 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { // the token (#654). // // NOT safe: +// - AWS EC2 and Redshift (slugs ec2, compute, redshift, data-warehouse): the +// purchase APIs take no client token, so a lost response may have bought the +// commitment and a second attempt would buy another (MON-02). The provider +// sends the call once and reports "purchase outcome unknown". // - Azure savings-plans: the OrderAlias API uses time.Now().UnixNano() as // the alias name; there is no server-side idempotency key and no // tag-based lookup implemented yet. Re-driving would create a duplicate @@ -415,7 +421,15 @@ func RedriveRefusalReason(exec *config.PurchaseExecution) string { func recRedriveRefusalReason(rec config.RecommendationRecord) string { switch rec.Provider { case "", "aws": - // Empty provider is legacy AWS. All AWS services honor IdempotencyToken. + // Empty provider is legacy AWS. RDS, ElastiCache, MemoryDB, OpenSearch + // and Savings Plans collapse a repeat onto the first attempt. EC2 and + // Redshift cannot: their purchase APIs take no client token or caller + // ID, and the provider sends them once and reports a lost response as + // "purchase outcome unknown". The commitment may exist, so a second + // attempt must not be made until someone has checked the account. + if awsPurchaseHasNoDuplicateGuard(rec.Service) { + return "AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" + } return "" case "azure": // Azure savings-plans uses a timestamp-based alias name and has no @@ -434,6 +448,25 @@ func recRedriveRefusalReason(rec config.RecommendationRecord) string { } } +// awsPurchaseHasNoDuplicateGuard reports whether a recommendation's service +// slug dispatches to the AWS EC2 or Redshift purchase path. The canonical +// slugs (compute, data-warehouse) and the legacy ones (ec2, redshift) reach the +// same client, so all four must be covered or the refusal can be bypassed by +// spelling. Matching goes through mapServiceSlug, the same table the executor +// dispatches on, so the two cannot drift. +func awsPurchaseHasNoDuplicateGuard(service string) bool { + svc, ok := mapServiceSlug(service) + if !ok { + return false + } + switch svc { + case common.ServiceEC2, common.ServiceCompute, common.ServiceRedshift, common.ServiceDataWarehouse: + return true + default: + return false + } +} + // claimAndRedrive atomically claims a stranded execution (by transitioning its // status from "approved" to "running") and then re-drives it via // executeAndFinalize. It is extracted from RecoverStrandedApprovals to keep that @@ -537,7 +570,11 @@ func (m *Manager) safeFail(ctx context.Context, exec *config.PurchaseExecution) return false, fmt.Errorf("failed to transition stranded execution %s to failed: %w", exec.ExecutionID, txErr) } - updated.Error = "execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck (issue #632). Verify on the cloud provider that no commitment was created, then Retry." + next := "Verify on the cloud provider that no commitment was created, then Retry." + if RedriveRefusalReason(exec) != "" { + next = "Retry is refused for this purchase because a repeat could buy a second commitment. Verify on the cloud provider whether the commitment was created; if it was not, start a new purchase." + } + updated.Error = "execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck (issue #632). " + next if saveErr := m.config.SavePurchaseExecution(ctx, updated); saveErr != nil { // The atomic flip to "failed" already landed via TransitionExecutionStatus; // only the explanatory error string failed to persist. Log loudly but diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index 9ccd7b76..1920bdb6 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -599,7 +599,7 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { Status: "approved", StepNumber: 1, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, }, } runningRow := stranded @@ -634,7 +634,7 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { // executeSinglePurchase wraps ctx in a per-rec WithTimeout before calling // CreateAndValidateProvider, so we must match any context, not ctx itself. mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) - mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + mockProvider.On("GetServiceClient", mock.Anything, common.ServiceRDS, "us-east-1").Return(mockServiceClient, nil) mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ Success: true, CommitmentID: "ri-idempotent-12345", @@ -1097,7 +1097,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_PersistenceFailurePropagate PlanID: "plan-aws-persist-456", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, }, } runningRow := stranded @@ -1130,7 +1130,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_PersistenceFailurePropagate Account: aws.String("123456789012"), }, nil) mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) - mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + mockProvider.On("GetServiceClient", mock.Anything, common.ServiceRDS, "us-east-1").Return(mockServiceClient, nil) mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ Success: true, CommitmentID: "ri-persist-fail-12345", @@ -1178,7 +1178,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_ExecAndPersistBothFail(t *t PlanID: "plan-aws-both-fail", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, }, } runningRow := stranded @@ -1240,7 +1240,7 @@ func TestManager_RecoverStrandedApprovals_AWSClaimLost_NoRedrive(t *testing.T) { PlanID: "plan-aws-claimed", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300.0, Selected: true}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300.0, Selected: true}, }, } diff --git a/internal/purchase/reaper_test.go b/internal/purchase/reaper_test.go index 3969904e..50606420 100644 --- a/internal/purchase/reaper_test.go +++ b/internal/purchase/reaper_test.go @@ -36,10 +36,21 @@ func stuckExec(id, status string) config.PurchaseExecution { } } -// stuckExecWithAWSRecs builds a stuck execution with a single AWS EC2 rec. -// allRecsSafeToRedrive returns true for pure-AWS recs, so the reaper's -// canonical error includes "safe to retry" (F4 fix). +// stuckExecWithAWSRecs builds a stuck execution with a single AWS RDS rec. +// RDS honors a deterministic reservation ID, so allRecsSafeToRedrive is true and +// the reaper's canonical error includes "safe to retry" (F4 fix). EC2 and +// Redshift are the AWS services that are not safe; see stuckExecWithAWSEC2Rec. func stuckExecWithAWSRecs(id, status string) config.PurchaseExecution { + e := stuckExec(id, status) + e.Recommendations = []config.RecommendationRecord{ + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Count: 1}, + } + return e +} + +// stuckExecWithAWSEC2Rec is the unsafe AWS case: EC2 purchases take no client +// token, so a stuck row may already have bought the commitment. +func stuckExecWithAWSEC2Rec(id, status string) config.PurchaseExecution { e := stuckExec(id, status) e.Recommendations = []config.RecommendationRecord{ {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1}, @@ -81,6 +92,33 @@ func TestReapStuckExecutions_StaleApprovedFlippedToFailed(t *testing.T) { store.AssertExpectations(t) } +// TestReapStuckExecutions_EC2RowIsNotAdvertisedAsSafeToRetry: a reaped EC2 row +// may have bought before the run died, and Retry is refused for it, so the +// canonical error must not say "safe to retry". The RDS test above is the +// positive control that keeps an always-refusing predicate from passing. +func TestReapStuckExecutions_EC2RowIsNotAdvertisedAsSafeToRetry(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + reapAfter := 10 * time.Minute + + row := stuckExecWithAWSEC2Rec("exec-ec2", "running") + transitioned := row + transitioned.Status = failedStatus + store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter).Return([]config.PurchaseExecution{row}, nil) + store.On("TransitionExecutionStatus", ctx, "exec-ec2", stuckStatuses, failedStatus, (*string)(nil)).Return(&transitioned, nil) + var saved *config.PurchaseExecution + store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }).Return(nil) + + result, err := newReaperManager(store).ReapStuckExecutions(ctx, reapAfter) + + require.NoError(t, err) + assert.Equal(t, 1, result.Reaped) + require.NotNil(t, saved) + assert.Contains(t, saved.Error, "reaped after") + assert.NotContains(t, saved.Error, "safe to retry") +} + func TestReapStuckExecutions_StaleRunningFlippedToFailed(t *testing.T) { ctx := context.Background() store := new(MockConfigStore) From e832349500c1d4292881e9fc978d831e0014b767 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 15:02:46 +0200 Subject: [PATCH 2/4] docs(purchase): correct comments on EC2 Tenancy and Scope defaults At the pinned providers/aws the EC2 client rejects an empty or unknown Tenancy or Scope before sending the request instead of substituting a default. Two comments still described the old defaulting behaviour. Comment-only change. Refs LeanerCloud/cloud-commitments-go#296 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/purchase_pricing.go | 11 +++++++---- internal/purchase/execution.go | 7 +++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 333e7652..53d2e4fe 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -264,10 +264,13 @@ func checkRequestDetailDiscriminators(req, match *config.RecommendationRecord, i // Platform/Scope/AZConfig/MemoryGB) precisely because its true // configuration was never recorded, so there is nothing known to // contradict. But once origin IS known, an empty match is ALSO a mismatch, -// not a pass: buildOfferingFilters substitutes the provider default for an -// empty match field, which can silently buy a different configuration than -// the one origin recorded (e.g. origin=dedicated tenancy, match=legacy- -// empty -> the purchase would resolve to the default-tenancy substitute). +// not a pass: for most services buildOfferingFilters substitutes the +// provider default for an empty match field, which can silently buy a +// different configuration than the one origin recorded (e.g. origin=dedicated +// tenancy, match=legacy-empty -> the purchase would resolve to the +// default-tenancy substitute). The AWS EC2 client rejects an empty Tenancy or +// Scope outright, but this check still has to refuse it here, before the +// purchase is attempted. // "Unknown" is therefore refused right alongside "different", not treated // as compatible. func purchaseDetailMismatch(origin, match common.ServiceDetails) string { diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 20d6f6b9..c46a3266 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1104,8 +1104,11 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // // Legacy rows persisted before #453 carry an empty rec.Details — // DecodeServiceDetailsFor returns a zero-valued typed pointer for - // those, and the cloud client's buildOfferingFilters substitutes - // defaults (Platform=Linux/UNIX, Tenancy=default, etc.). Engine is + // those. Most cloud clients' buildOfferingFilters substitute defaults + // (Platform=Linux/UNIX, etc.), but the AWS EC2 client no longer does for + // Tenancy and Scope: an empty or unknown value is rejected before the + // request is sent, so the purchase fails loudly instead of buying a + // default configuration. Engine is // preserved on the record column too, so we use it as a last-resort // fallback for DB/Cache services to avoid silently mis-purchasing a // legacy non-default-engine rec as the default engine. From 8d00c5fcd2965b470d33a815b7899ff19f1312b8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 15:05:16 +0200 Subject: [PATCH 3/4] docs(purchase): state exactly what the EC2 client rejects before sending The AWS EC2 client rejects an empty Platform and an empty or unknown Tenancy or Scope before sending the request. Drop the Linux/UNIX default example, which applies to exchange only, and stop naming a buildOfferingFilters function that does not exist. Comment-only change. Refs LeanerCloud/cloud-commitments-go#296 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/purchase_pricing.go | 13 ++++++------- internal/purchase/execution.go | 11 +++++------ 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 53d2e4fe..02f18e8d 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -264,13 +264,12 @@ func checkRequestDetailDiscriminators(req, match *config.RecommendationRecord, i // Platform/Scope/AZConfig/MemoryGB) precisely because its true // configuration was never recorded, so there is nothing known to // contradict. But once origin IS known, an empty match is ALSO a mismatch, -// not a pass: for most services buildOfferingFilters substitutes the -// provider default for an empty match field, which can silently buy a -// different configuration than the one origin recorded (e.g. origin=dedicated -// tenancy, match=legacy-empty -> the purchase would resolve to the -// default-tenancy substitute). The AWS EC2 client rejects an empty Tenancy or -// Scope outright, but this check still has to refuse it here, before the -// purchase is attempted. +// not a pass: a client may substitute a default for an empty match field, +// which can silently buy a different configuration than the one origin +// recorded (e.g. origin=dedicated tenancy, match=legacy-empty -> the purchase +// would resolve to the default-tenancy substitute). The AWS EC2 client +// rejects an empty Platform, Tenancy or Scope outright, but this check still +// has to refuse it here, before the purchase is attempted. // "Unknown" is therefore refused right alongside "different", not treated // as compatible. func purchaseDetailMismatch(origin, match common.ServiceDetails) string { diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index c46a3266..7bbb2d3f 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1104,12 +1104,11 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // // Legacy rows persisted before #453 carry an empty rec.Details — // DecodeServiceDetailsFor returns a zero-valued typed pointer for - // those. Most cloud clients' buildOfferingFilters substitute defaults - // (Platform=Linux/UNIX, etc.), but the AWS EC2 client no longer does for - // Tenancy and Scope: an empty or unknown value is rejected before the - // request is sent, so the purchase fails loudly instead of buying a - // default configuration. Engine is - // preserved on the record column too, so we use it as a last-resort + // those. The AWS EC2 client does not fill the gaps: an empty Platform + // or an empty or unknown Tenancy or Scope is rejected before the request + // is sent (providers/aws ec2/client.go buildEC2OfferingQuery), so the + // purchase fails loudly instead of buying a default configuration. Engine + // is preserved on the record column too, so we use it as a last-resort // fallback for DB/Cache services to avoid silently mis-purchasing a // legacy non-default-engine rec as the default engine. details, detailsErr := common.DecodeServiceDetailsFor(rec.Service, rec.Details) From c5e1f007c633a1ce17d8125301868b386bdcf083 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 15:37:50 +0200 Subject: [PATCH 4/4] test(purchase): drive ramp retry tests with a retry-safe service Co-Authored-By: Claude Opus 5.5 --- internal/purchase/ramp_step_progress_integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index 742f48f6..f2b4b929 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -140,7 +140,7 @@ func newRampStepFixture(ctx context.Context, t *testing.T) *rampStepFixture { mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Maybe() - mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceEC2, mock.Anything).Return(mockServiceClient, nil).Maybe() + mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceRDS, mock.Anything).Return(mockServiceClient, nil).Maybe() mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.AnythingOfType("common.PurchaseOptions")). Run(func(args mock.Arguments) { opts := args.Get(2).(common.PurchaseOptions) @@ -195,7 +195,7 @@ func (f *rampStepFixture) purchaseCount() int { // rampStepRecommendation is the single rec every execution in these tests buys. func rampStepRecommendation() []config.RecommendationRecord { return []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, } }