diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 2ea40a328..cf9f20bcd 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1744,6 +1744,19 @@ func checkRetryRateGates(failedExec *config.PurchaseExecution, req *events.Lambd return nil } +// clonePtr returns a pointer to a copy of *p, or nil when p is nil, so a +// value copied between two records never leaves them aliasing the same +// pointee. nil is preserved rather than collapsed to a zero value: on +// PurchaseExecution.CloudAccountID nil means "no account scope" (a root or +// ambient-credentials execution) and is not interchangeable with "". +func clonePtr[T any](p *T) *T { + if p == nil { + return nil + } + v := *p + return &v +} + // persistRetryExecution builds the successor PurchaseExecution and // writes it + the predecessor linkage + suppressions in a single tx. // Returns the populated newExecution so the caller can send the @@ -1789,11 +1802,42 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // planned execution stays attributed to its plan + ramp step (CR // #168 review). For ad-hoc executions PlanID is "" and StepNumber // is 0, so propagation is a no-op for the non-plan case. + // + // CloudAccountID propagates for the same reason, and it is load-bearing on + // the money path (issue #1537). A multi-account plan fans out into one + // purchase_executions row per cloud account, each stamped with + // CloudAccountID plus a per-account idempotency key ":" + // (purchase.executeForAccount). Those per-account rows reach + // status="failed" on their own and are therefore retryable. Dropping + // CloudAccountID here left the successor with a nil account scope, which + // purchase.executePurchase reads as "root execution" and re-fans-out across + // EVERY account on the plan: the accounts that already committed were + // purchased a SECOND time, under re-suffixed keys + // "::" whose derived tokens miss the AWS + // ClientToken, the EC2 RI tag-guard and the Azure two-step lookup alike. + // Carrying the scope keeps a per-account retry a single-account re-drive + // against the identical lineage key, which is the precondition for the + // provider-side dedupe to engage at all. + // + // Whether it then DOES engage is per-provider and not universal: AWS (all + // services), Azure reservations and GCP CUDs reproduce the token and + // dedupe, but Azure savings-plans has no server-side idempotency key and + // names its order alias from time.Now().UnixNano(), so a re-drive of a + // landed Azure SP order still duplicates it. purchase.recIsSafeToRedrive + // encodes exactly that exclusion, but it currently gates only the reaper's + // re-drive, not this user-facing retry (issue #1668). Scope propagation is + // necessary for dedupe everywhere and sufficient everywhere except Azure SP. + // + // Copied by value rather than by pointer so the successor and the + // historical failed row never share a *string, matching the defensive + // deep copy of Recommendations above. + cloudAccountID := clonePtr(failedExec.CloudAccountID) newExecution := &config.PurchaseExecution{ ExecutionID: newExecutionID, IdempotencyKey: idempotencyKey, PlanID: failedExec.PlanID, StepNumber: failedExec.StepNumber, + CloudAccountID: cloudAccountID, Status: "pending", ScheduledDate: time.Now(), Recommendations: copiedRecs, diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go new file mode 100644 index 000000000..480776be4 --- /dev/null +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -0,0 +1,349 @@ +package api + +import ( + "context" + "encoding/json" + "sort" + "sync" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/purchase" + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/provider" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --- Issue #1537 regression harness ------------------------------------ +// +// A multi-account plan fans out into one purchase_executions row per cloud +// account (purchase.executeForAccount), each stamped with CloudAccountID and a +// per-account idempotency key ":". Those per-account rows +// reach status="failed" on their own and the History UI offers Retry on them. +// +// The tests below drive the REAL chain end to end — the retry HTTP handler +// (Handler.retryPurchase, which builds the successor row) followed by the REAL +// purchase.Manager executing that successor — and count the cloud purchases +// that actually fire. A narrower unit test on either half alone would stay +// green while the double-spend lived in the seam between them. + +const ( + // Valid UUIDs: retryPurchase runs validateUUID on the execution ID. + fanoutFailedAcctCExecID = "11111111-2222-3333-4444-555555555501" + fanoutFailedAcctAExecID = "11111111-2222-3333-4444-555555555502" + fanoutFailedRootExecID = "11111111-2222-3333-4444-555555555503" + + fanoutPlanID = "77777777-7777-7777-7777-777777777777" + fanoutRootKey = "root-lineage-1537" + + fanoutAcctA = "acct-A" + fanoutAcctB = "acct-B" + fanoutAcctC = "acct-C" +) + +// fanoutPlanAccounts is the plan's account set: the fan-out unit whose +// re-entry on retry is what issue #1537 is about. +func fanoutPlanAccounts() []config.CloudAccount { + return []config.CloudAccount{ + {ID: fanoutAcctA, Name: "A", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"}, + {ID: fanoutAcctB, Name: "B", Provider: "aws", ExternalID: "222222222222", AWSAuthMode: "access_keys"}, + {ID: fanoutAcctC, Name: "C", Provider: "aws", ExternalID: "333333333333", AWSAuthMode: "access_keys"}, + } +} + +func fanoutRecs() []config.RecommendationRecord { + return []config.RecommendationRecord{ + { + Provider: "aws", Service: "ec2", ResourceType: "m5.large", + Region: "us-east-1", Count: 1, Term: 1, + UpfrontCost: 300, Selected: true, + }, + } +} + +// fanoutCredStore resolves a static AWS access-key blob so per-account +// credential resolution succeeds without touching the cloud. +type fanoutCredStore struct { + MockCredentialStore +} + +func (*fanoutCredStore) LoadRaw(_ context.Context, _, _ string) ([]byte, error) { + return []byte(`{"access_key_id":"AKIAIOSFODNN7EXAMPLE","secret_access_key":"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"}`), nil +} + +// fanoutServiceClient records the idempotency token of every commitment +// purchase that reaches the "cloud". One recorded token == one real purchase. +type fanoutServiceClient struct { + mu sync.Mutex + tokens []string +} + +func (c *fanoutServiceClient) PurchaseCommitment(_ context.Context, _ common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + c.mu.Lock() + c.tokens = append(c.tokens, opts.IdempotencyToken) + c.mu.Unlock() + return common.PurchaseResult{Success: true, CommitmentID: "ri-" + opts.IdempotencyToken}, nil +} + +// purchasedTokens returns a sorted copy of the recorded tokens. Sorted so +// assertions are stable under the parallel per-account fan-out. +func (c *fanoutServiceClient) purchasedTokens() []string { + c.mu.Lock() + defer c.mu.Unlock() + out := append([]string(nil), c.tokens...) + sort.Strings(out) + return out +} + +func (c *fanoutServiceClient) GetServiceType() common.ServiceType { return common.ServiceEC2 } +func (c *fanoutServiceClient) GetRegion() string { return "us-east-1" } +func (c *fanoutServiceClient) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { + return nil, nil +} +func (c *fanoutServiceClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) { + return nil, nil +} +func (c *fanoutServiceClient) ValidateOffering(_ context.Context, _ common.Recommendation) error { + return nil +} +func (c *fanoutServiceClient) GetOfferingDetails(_ context.Context, _ common.Recommendation) (*common.OfferingDetails, error) { + return &common.OfferingDetails{}, nil +} +func (c *fanoutServiceClient) GetValidResourceTypes(_ context.Context) ([]string, error) { + return []string{"m5.large"}, nil +} + +// fanoutProvider is a provider.Provider whose service client is the shared +// recorder above, so every account's purchases land in one tally. +type fanoutProvider struct{ svc *fanoutServiceClient } + +func (p *fanoutProvider) Name() string { return "aws" } +func (p *fanoutProvider) DisplayName() string { return "Amazon Web Services" } +func (p *fanoutProvider) IsConfigured() bool { return true } +func (p *fanoutProvider) GetCredentials() (provider.Credentials, error) { return nil, nil } +func (p *fanoutProvider) ValidateCredentials(_ context.Context) error { return nil } +func (p *fanoutProvider) GetAccounts(_ context.Context) ([]common.Account, error) { + return nil, nil +} +func (p *fanoutProvider) GetRegions(_ context.Context) ([]common.Region, error) { return nil, nil } +func (p *fanoutProvider) GetDefaultRegion() string { return "us-east-1" } +func (p *fanoutProvider) GetSupportedServices() []common.ServiceType { + return []common.ServiceType{common.ServiceEC2} +} +func (p *fanoutProvider) GetServiceClient(_ context.Context, _ common.ServiceType, _ string) (provider.ServiceClient, error) { + return p.svc, nil +} +func (p *fanoutProvider) GetRecommendationsClient(_ context.Context) (provider.RecommendationsClient, error) { + return nil, nil +} + +type fanoutProviderFactory struct{ prov *fanoutProvider } + +func (f *fanoutProviderFactory) CreateAndValidateProvider(_ context.Context, _ string, _ *provider.ProviderConfig) (provider.Provider, error) { + return f.prov, nil +} + +// executeSuccessorForReal runs the retry successor through the REAL +// purchase.Manager the way production does: the approved row is picked up from +// the execute_purchase queue message, claimed, and executed. It returns the +// sorted idempotency tokens of every commitment purchase that reached the +// cloud — one entry per real purchase. +func executeSuccessorForReal(t *testing.T, successor *config.PurchaseExecution) []string { + t.Helper() + ctx := context.Background() + + // The row as the DB holds it once the operator has clicked the approval + // link: same fields the retry handler persisted, status "approved". + approved := *successor + approved.Status = "approved" + running := approved + running.Status = "running" + + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + + store.On("GetExecutionByID", mock.Anything, approved.ExecutionID).Return(&approved, nil).Once() + store.On("TransitionExecutionStatus", mock.Anything, approved.ExecutionID, + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&running, nil).Once() + store.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) + // Plan progress advances only on a fully clean run; .Maybe() so a run that + // ends partial/failed does not turn into a mock-expectation failure that + // would mask the purchase-count assertion the caller actually makes. + store.On("IncrementPlanCurrentStep", mock.Anything, fanoutPlanID).Return(nil).Maybe() + + store.GetPurchasePlanFn = func(_ context.Context, planID string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: planID, Name: "Plan 1537"}, nil + } + // Fn overrides rather than testify expectations: whether the fan-out + // consults the plan's accounts at all is exactly what is under test, so + // these must not double as assertions. + store.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { + return fanoutPlanAccounts(), nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + for _, a := range fanoutPlanAccounts() { + if a.ID == id { + acct := a + return &acct, nil + } + } + return nil, nil + } + store.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error { return nil } + + svc := &fanoutServiceClient{} + mgr := purchase.NewManager(purchase.ManagerConfig{ + ConfigStore: store, + EmailSender: &stubEmailNotifier{}, + CredentialStore: &fanoutCredStore{}, + ProviderFactory: &fanoutProviderFactory{prov: &fanoutProvider{svc: svc}}, + DashboardURL: "https://dashboard.example.com", + }) + + body, err := json.Marshal(purchase.AsyncMessage{ + Type: purchase.MessageTypeExecutePurchase, + ExecutionID: approved.ExecutionID, + }) + require.NoError(t, err) + require.NoError(t, mgr.ProcessMessage(ctx, string(body)), + "the successor must execute cleanly; a non-nil error would make the purchase tally meaningless") + + return svc.purchasedTokens() +} + +// retryFailedRow drives the real retry handler for a failed row owned by the +// calling session and returns the successor execution it persisted. +func retryFailedRow(t *testing.T, failed *config.PurchaseExecution) *config.PurchaseExecution { + t.Helper() + session := &Session{UserID: retryCallerID, Email: "operator@example.com"} + // retry-own authorizes the row's creator (issue #907). + successor, _ := runSessionRetryAllowed(t, failed, session, false, true, sessionRetryReq()) + return successor +} + +// failedPerAccountRow builds the purchase_executions row that +// purchase.executeForAccount persists when one account of a fan-out fails: +// scoped to that account and keyed ":". +func failedPerAccountRow(execID, accountID string) *config.PurchaseExecution { + creator := retryCallerID + return &config.PurchaseExecution{ + ExecutionID: execID, + Status: "failed", + PlanID: fanoutPlanID, + StepNumber: 1, + CloudAccountID: strPtr(accountID), + IdempotencyKey: fanoutRootKey + ":" + accountID, + Error: "credential resolution failed for account " + accountID + ": RequestLimitExceeded", + CreatedByUserID: &creator, + CapacityPercent: 100, + Source: common.PurchaseSourceWeb, + Recommendations: fanoutRecs(), + } +} + +// TestRetryOfFailedPerAccountRowDoesNotRefanOut is the issue #1537 regression +// guard, direction (a): two attempts at the SAME logical purchase must +// converge on ONE. +// +// Scenario, exactly as it happens in production: plan P covers accounts A, B +// and C. The root execution fans out; A and B commit real reserved instances, +// C fails on credential resolution and its per-account row is saved "failed". +// The operator clicks Retry on that C row — the only row History shows as +// failed. +// +// Pre-fix, persistRetryExecution dropped CloudAccountID, so the successor was +// born account-scopeless and purchase.executePurchase re-entered the +// multi-account fan-out: A and B were purchased a SECOND time, under freshly +// suffixed keys (":C:A") whose derived provider tokens miss the AWS +// ClientToken / EC2 RI tag-guard entirely, so nothing deduped them. At $300 +// upfront per RI that is $600 of unintended spend from one Retry click, and it +// scales with the plan's account count. +// +// Post-fix the successor keeps account C's scope, so exactly one purchase +// fires and it carries the SAME provider token the original C attempt used — +// so even if C's first attempt had actually landed at the provider, the retry +// dedupes instead of buying twice. +func TestRetryOfFailedPerAccountRowDoesNotRefanOut(t *testing.T) { + failedC := failedPerAccountRow(fanoutFailedAcctCExecID, fanoutAcctC) + + successor := retryFailedRow(t, failedC) + tokens := executeSuccessorForReal(t, successor) + + // The money assertion comes first and deliberately uses assert, not + // require: on the pre-fix code the follow-up checks below name the cause + // (a dropped account scope) in the same failing run. + assert.Equal(t, []string{common.DeriveIdempotencyToken(failedC.IdempotencyKey, 0)}, tokens, + "retrying account C's failed row must fire exactly ONE purchase, carrying the ORIGINAL C attempt's provider token. "+ + "Extra tokens are accounts that already committed being bought AGAIN (issue #1537); a different token would miss the provider's dedupe guard") + + if assert.NotNil(t, successor.CloudAccountID, + "the retry successor of a per-account row must inherit that account's scope; a nil scope re-enters the multi-account fan-out (issue #1537)") { + assert.Equal(t, fanoutAcctC, *successor.CloudAccountID, + "the successor must stay scoped to the account whose row was retried") + } + assert.Equal(t, failedC.IdempotencyKey, successor.IdempotencyKey, + "the lineage key must carry over verbatim so the provider token is reproduced (issue #1012)") +} + +// TestRetryOfPerAccountRowsKeepsAccountsDistinct is the issue #1537 guard for +// direction (b): two GENUINELY DIFFERENT purchases must not collapse onto one +// token. Scoping the successor must not over-converge — retrying account A's +// failed row and account C's failed row are separate money events and must +// keep separate provider tokens, or one of the two legitimate purchases would +// be silently deduped away and never happen. +func TestRetryOfPerAccountRowsKeepsAccountsDistinct(t *testing.T) { + failedA := failedPerAccountRow(fanoutFailedAcctAExecID, fanoutAcctA) + failedC := failedPerAccountRow(fanoutFailedAcctCExecID, fanoutAcctC) + + tokensA := executeSuccessorForReal(t, retryFailedRow(t, failedA)) + tokensC := executeSuccessorForReal(t, retryFailedRow(t, failedC)) + + require.Len(t, tokensA, 1, "account A's retry must purchase exactly once") + require.Len(t, tokensC, 1, "account C's retry must purchase exactly once") + assert.NotEqual(t, tokensA[0], tokensC[0], + "retries of DIFFERENT accounts' rows must derive DIFFERENT provider tokens; collapsing them would make one legitimate purchase silently never happen") + assert.Equal(t, common.DeriveIdempotencyToken(fanoutRootKey+":"+fanoutAcctA, 0), tokensA[0]) + assert.Equal(t, common.DeriveIdempotencyToken(fanoutRootKey+":"+fanoutAcctC, 0), tokensC[0]) +} + +// TestRetryOfFailedRootRowStillFansOutToEveryAccount is the other half of +// direction (b): the fix must not suppress a fan-out that SHOULD happen. +// +// A root execution that failed before any account committed (e.g. the plan's +// account list could not be loaded) carries no CloudAccountID. Retrying it +// must still fan out across every plan account, each under the per-account +// lineage key the first attempt would have used — so nothing is skipped and no +// two accounts share a token. +func TestRetryOfFailedRootRowStillFansOutToEveryAccount(t *testing.T) { + creator := retryCallerID + failedRoot := &config.PurchaseExecution{ + ExecutionID: fanoutFailedRootExecID, + Status: "failed", + PlanID: fanoutPlanID, + StepNumber: 1, + // CloudAccountID deliberately nil: the root row never reached the fan-out. + IdempotencyKey: fanoutRootKey, + Error: "failed to load plan accounts for plan " + fanoutPlanID + ": RequestLimitExceeded", + CreatedByUserID: &creator, + CapacityPercent: 100, + Source: common.PurchaseSourceWeb, + Recommendations: fanoutRecs(), + } + + successor := retryFailedRow(t, failedRoot) + assert.Nil(t, successor.CloudAccountID, + "a root row's successor must stay account-scopeless so the fan-out still runs") + + tokens := executeSuccessorForReal(t, successor) + + want := []string{ + common.DeriveIdempotencyToken(fanoutRootKey+":"+fanoutAcctA, 0), + common.DeriveIdempotencyToken(fanoutRootKey+":"+fanoutAcctB, 0), + common.DeriveIdempotencyToken(fanoutRootKey+":"+fanoutAcctC, 0), + } + sort.Strings(want) + assert.Equal(t, want, tokens, + "retrying a root row must still purchase once per plan account, each under its own per-account token") +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 525d4ef6c..b9dcd9b0a 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -61,7 +61,7 @@ func (m *Manager) executePurchase(ctx context.Context, exec *config.PurchaseExec return fmt.Errorf("failed to load plan accounts for plan %s: %w", exec.PlanID, err) } if len(accounts) > 0 { - return m.executeMultiAccount(ctx, exec, plan, accounts) + return m.executeScopeAware(ctx, exec, plan, accounts) } } } @@ -716,6 +716,91 @@ func idempotencyLineageKey(exec *config.PurchaseExecution) string { return exec.ExecutionID } +// reattachAccountScope is the second #1537 gate: it decides whether a plan +// execution that carries no cloud_account_id is genuinely a root row (safe to +// fan out) or a per-account row that lost its scope in transit, and repairs the +// latter in place. +// +// How per-account-ness is detected. executeForAccount is the ONLY writer that +// puts a colon in a lineage key (":", execution.go). Every +// other producer yields a colon-free value: SavePurchaseExecution mints a bare +// uuid.New().String() at first insert, persistRetryExecution copies the +// predecessor's key verbatim or falls back to its ExecutionID (also a UUID), +// and the read path just round-trips the column. (The unrelated +// api.purchaseIdempotencyKey submit-fingerprint is sha256 hex and is never +// written to this field.) So on a scopeless row a colon is by itself proof the +// row is per-account -- which is why the colon, not a match against the plan's +// current account set, is the detector. Matching the current account set would +// fail OPEN exactly when the scoped account has since been removed from the +// plan: no match, no refusal, and the row re-fans-out and double-buys. +// +// What happens then: +// +// - Colon-free key: a real root row. Returns nil leaving CloudAccountID nil, +// and the caller fans out as before. +// - Terminal segment matches a current plan account: repair rather than +// refuse. The lineage key is durable proof of the intended scope, so +// stamping CloudAccountID makes this the single-account re-drive it was +// always meant to be, reproducing the identical per-rec provider token. +// That is strictly safer than erroring -- same purchase count, correct +// dedupe -- and it is the only exit that exists: a refused row lands +// "failed", and retrying it produces another scopeless successor carrying +// the same key (persistRetryExecution copies both verbatim), so it would +// be refused forever, while its predecessor is already 409 "was already +// retried". The repair also persists: executeAndFinalize saves exec, and +// cloud_account_id is in SavePurchaseExecution's ON CONFLICT SET. +// - Colon present but no plan account matches: the scoped account has left +// the plan (removed, or deleted and re-added under a new ID). There is +// nothing to re-attach to and fanning out would re-suffix the key and +// re-buy every account that already committed, so refuse. This is the +// genuinely unrecoverable case and the one that must not fail open. +func reattachAccountScope(exec *config.PurchaseExecution, accounts []config.CloudAccount) error { + key := idempotencyLineageKey(exec) + if !strings.Contains(key, ":") { + return nil // root key: a genuine multi-account fan-out. + } + for i := range accounts { + // An empty ID would degrade the test to HasSuffix(key, ":"), so require + // a real account ID to match on. + if accounts[i].ID == "" { + continue + } + if strings.HasSuffix(key, ":"+accounts[i].ID) { + scoped := accounts[i].ID + exec.CloudAccountID = &scoped + logging.Warnf("purchase[%s]: re-attached account scope %s (%s) from the idempotency lineage key; "+ + "the row reached the executor with no cloud_account_id (issue #1537)", + exec.ExecutionID, accounts[i].ID, accounts[i].Name) + return nil + } + } + // The key contains a colon (checked above), so the terminal segment is the + // account it was scoped to. Naming it is what makes the error actionable: + // that is the account to re-add. + orphanedAccount := key[strings.LastIndex(key, ":")+1:] + return fmt.Errorf("refusing to fan out execution %s across %d plan accounts: "+ + "its idempotency lineage key is scoped to account %s, which is no longer on plan %s, "+ + "and the row carries no cloud_account_id, so the scope cannot be recovered; "+ + "re-deriving the key would double-purchase every account that already committed (issue #1537). "+ + "Re-add account %s to the plan to let the retry complete, or cancel this execution", + exec.ExecutionID, len(accounts), orphanedAccount, exec.PlanID, orphanedAccount) +} + +// executeScopeAware dispatches a plan execution that reached the fan-out +// decision with no cloud_account_id. Split out of executePurchase so the +// #1537 scope repair does not push that function over the gocyclo budget. +func (m *Manager) executeScopeAware(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accounts []config.CloudAccount) error { + if err := reattachAccountScope(exec, accounts); err != nil { + return err + } + if exec.CloudAccountID != nil { + // Scope recovered from the lineage key: this is a single-account + // re-drive, not a fan-out. + return m.executeSingleAccount(ctx, exec, plan) + } + return m.executeMultiAccount(ctx, exec, plan, accounts) +} + // appendErrNote joins note onto an existing error string with "; ", // returning note alone when existing is empty. Shared by the audit-gap and // partial-failure paths so a successful-rec audit gap (#621) and a per-rec diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index a99181b3c..19f3714af 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -3,6 +3,7 @@ package purchase import ( "context" "fmt" + "sort" "sync" "testing" @@ -357,3 +358,251 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { assert.Equal(t, "partially_completed", savedRoot.Status, "root must reflect partial success, never 'failed' (double-spend mislabel #642/#1014)") } + +// scopelessRunResult is what runScopelessRow observed: the tokens of every +// commitment purchase that reached the fake cloud (one entry == one real +// purchase), and the error executePurchase returned. +type scopelessRunResult struct { + err error + tokens []string +} + +// runScopelessRow drives executePurchase for a plan execution that carries NO +// cloud_account_id, against a plan whose accounts are `accounts`, and reports +// what actually got bought. +// +// The whole provider chain is wired and WOULD happily sell, so a missing or +// fail-open guard shows up as extra purchases rather than as an +// unexpected-mock-call panic that a require.Error would misread as "refused". +func runScopelessRow(t *testing.T, exec *config.PurchaseExecution, accounts []config.CloudAccount) scopelessRunResult { + t.Helper() + ctx := context.Background() + + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + t.Cleanup(func() { mockFactory.AssertExpectations(t) }) + t.Cleanup(func() { mockProviderInst.AssertExpectations(t) }) + t.Cleanup(func() { mockServiceClient.AssertExpectations(t) }) + + mockStore.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: id, Name: "Plan X"}, nil + } + // Fn overrides, not testify expectations: whether the plan's accounts are + // consulted at all is part of what is under test. + mockStore.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { + return accounts, nil + } + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + for i := range accounts { + if accounts[i].ID == id { + acct := accounts[i] + return &acct, nil + } + } + return nil, nil + } + mockStore.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error { return nil } + + var mu sync.Mutex + var tokens []string + mockStore.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Maybe() + mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() + mockStore.On("IncrementPlanCurrentStep", mock.Anything, "plan-x").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() + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.AnythingOfType("common.PurchaseOptions")). + Run(func(args mock.Arguments) { + opts := args.Get(2).(common.PurchaseOptions) + mu.Lock() + tokens = append(tokens, opts.IdempotencyToken) + mu.Unlock() + }). + Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil).Maybe() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + credStore: awsAccessKeyCredStore(), + dashboardURL: "https://dashboard.example.com", + } + + err := manager.executePurchase(ctx, exec) + + mu.Lock() + defer mu.Unlock() + out := append([]string(nil), tokens...) + sort.Strings(out) + return scopelessRunResult{err: err, tokens: out} +} + +// scopelessPerAccountRow is the row shape both tests below start from: a +// successor minted by the pre-#1537 retry handler that is still pending or +// approved when the fix deploys. Its cloud_account_id is nil, but its lineage +// key is still the per-account ":" that +// executeForAccount stamped. +func scopelessPerAccountRow(accountID string) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: "successor-of-" + accountID + "-row", + CloudAccountID: nil, + IdempotencyKey: "root-lineage:" + accountID, + PlanID: "plan-x", + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + }, + } +} + +// TestScopelessPerAccountRowSelfHealsToItsAccount is the issue #1537 +// defense-in-depth guard at the purchase layer, recovery half. +// +// The primary fix keeps cloud_account_id on a per-account row's retry successor +// (api.persistRetryExecution). This is the second gate, for a row that reaches +// the executor scopeless while its lineage is already per-account. Fanning such +// a row out would re-suffix its key ("root-lineage:acct-A" -> +// "root-lineage:acct-A:acct-B"), derive tokens matching nothing the first +// attempt used, and re-buy every account that already committed. +// +// When the scoped account is still on the plan the lineage key is durable proof +// of intent, so the executor re-attaches the scope and runs the single-account +// re-drive the row was always meant to be: exactly ONE purchase, under the +// IDENTICAL token the original attempt used, so a commitment that actually +// landed dedupes. Refusing here instead would be a dead end — the successor of +// a refused row carries the same nil scope and the same key, so it would be +// refused forever, and its predecessor already 409s as "already retried". +func TestScopelessPerAccountRowSelfHealsToItsAccount(t *testing.T) { + accounts := []config.CloudAccount{ + {ID: "acct-A", Name: "A", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"}, + {ID: "acct-B", Name: "B", Provider: "aws", ExternalID: "222222222222", AWSAuthMode: "access_keys"}, + } + orphan := scopelessPerAccountRow("acct-A") + + got := runScopelessRow(t, orphan, accounts) + + require.NoError(t, got.err, + "a scopeless row whose account is still on the plan must complete as a single-account re-drive, not dead-end on a refusal it can never recover from") + assert.Equal(t, []string{common.DeriveIdempotencyToken("root-lineage:acct-A", 0)}, got.tokens, + "exactly ONE purchase, under the token the ORIGINAL acct-A attempt derived. A second token is acct-B being bought again; a different single token would miss the provider's dedupe guard (issue #1537)") + + require.NotNil(t, orphan.CloudAccountID, + "the recovered scope must be stamped on the row so the repair persists (executeAndFinalize saves it, and cloud_account_id is in SavePurchaseExecution's ON CONFLICT SET)") + assert.Equal(t, "acct-A", *orphan.CloudAccountID) +} + +// TestScopelessPerAccountRowRefusesWhenAccountLeftThePlan is the fail-closed +// half, and the case a plan-account-set matcher gets wrong. +// +// Detecting per-account-ness by matching the plan's CURRENT accounts fails OPEN +// exactly when the scoped account has since been removed from the plan (or +// deleted and re-added under a new ID): nothing matches, no refusal fires, and +// the row re-fans-out and re-buys every remaining account under freshly +// suffixed keys. Detecting it by the colon that only executeForAccount ever +// writes into a lineage key does not have that hole. +// +// There is nothing to re-attach to here, so the only safe outcome is to refuse +// and buy NOTHING. +func TestScopelessPerAccountRowRefusesWhenAccountLeftThePlan(t *testing.T) { + // acct-C has been removed from the plan since its row failed. + accounts := []config.CloudAccount{ + {ID: "acct-A", Name: "A", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"}, + {ID: "acct-B", Name: "B", Provider: "aws", ExternalID: "222222222222", AWSAuthMode: "access_keys"}, + } + orphan := scopelessPerAccountRow("acct-C") + + got := runScopelessRow(t, orphan, accounts) + + assert.Empty(t, got.tokens, + "a scopeless per-account row whose account left the plan must buy NOTHING; every purchase here is an account that already committed being bought again under a fresh, undeduplicated token (issue #1537)") + assert.Nil(t, orphan.CloudAccountID, + "no plan account matches the lineage key, so nothing may be stamped as the recovered scope") + + require.Error(t, got.err, + "the refusal must surface as an error, not a silent skip, so the row lands 'failed' and an operator sees it") + assert.Contains(t, got.err.Error(), "refusing to fan out", + "the error must be the #1537 refusal itself, not an incidental downstream failure") + assert.Contains(t, got.err.Error(), "no longer on plan", + "the error must say WHY the scope could not be recovered, so the operator knows to re-add the account or cancel") + assert.Contains(t, got.err.Error(), "acct-C", + "the error must name the departed account, since re-adding THAT account is the operator's way out") +} + +// TestRootKeyStillFansOutThroughExecutePurchase is the counterpart to the guard +// above: the refusal must be narrow. A genuine root row (root lineage key, no +// account scope) must still fan out across every plan account, each under its +// own per-account key — a legitimate multi-account purchase silently not +// happening is the same defect with the opposite sign. +func TestRootKeyStillFansOutThroughExecutePurchase(t *testing.T) { + ctx := context.Background() + accounts := []config.CloudAccount{ + {ID: "acct-A", Name: "A", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"}, + {ID: "acct-B", Name: "B", Provider: "aws", ExternalID: "222222222222", AWSAuthMode: "access_keys"}, + } + + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + t.Cleanup(func() { mockFactory.AssertExpectations(t) }) + t.Cleanup(func() { mockProviderInst.AssertExpectations(t) }) + t.Cleanup(func() { mockServiceClient.AssertExpectations(t) }) + + mockStore.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: id, Name: "Plan X"}, nil + } + mockStore.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { + return accounts, nil + } + + var mu sync.Mutex + perAccountKey := map[string]string{} + mockStore.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + mu.Lock() + if e.CloudAccountID != nil { + perAccountKey[*e.CloudAccountID] = e.IdempotencyKey + } + mu.Unlock() + return nil + } + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceEC2, mock.Anything).Return(mockServiceClient, nil) + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything). + Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + credStore: awsAccessKeyCredStore(), + dashboardURL: "https://dashboard.example.com", + } + + root := &config.PurchaseExecution{ + ExecutionID: "root-exec", + IdempotencyKey: "root-lineage", + PlanID: "plan-x", + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + }, + } + require.NoError(t, manager.executePurchase(ctx, root), + "a genuine root row must still fan out; the #1537 refusal must not catch it") + + mu.Lock() + defer mu.Unlock() + assert.Equal(t, map[string]string{ + "acct-A": "root-lineage:acct-A", + "acct-B": "root-lineage:acct-B", + }, perAccountKey, "every plan account must still be purchased under its own per-account lineage key") +}