From 5f431f394ff4a49455a731852ec7ab5253b6492b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:23:56 +0200 Subject: [PATCH 1/2] fix(purchase): keep account scope on per-account retry successors A multi-account plan fans out into one purchase_executions row per cloud account, each stamped with cloud_account_id and a per-account idempotency key ":". Those rows reach status="failed" on their own, so History offers Retry on them. persistRetryExecution copied the lineage key, plan and step onto the successor but not cloud_account_id. The successor was therefore born account-scopeless, and executePurchase reads a nil cloud_account_id on a planned execution as "root execution" and re-enters the fan-out: for a plan covering accounts A, B and C where only C failed, one Retry click on C re-purchased A and B as well, under re-suffixed keys ":C:A" whose derived tokens match neither the AWS ClientToken, the EC2 RI tag-guard nor the Azure two-step lookup, so nothing deduped them. At $300 upfront per reserved instance that is $600 of unintended spend from a single click on a three-account plan, scaling linearly with the plan's account count. Two gates now: - persistRetryExecution carries cloud_account_id onto the successor (by value, so the successor and the historical failed row never alias the same pointee). A per-account retry stays a single-account re-drive against the identical lineage key, so the provider dedupes it. - executePurchase refuses to fan out a row whose lineage key already terminates in one of the plan's own account IDs while the row carries no cloud_account_id. That covers successors minted by the old handler and still pending/approved when this deploys: they fail loud with zero purchases instead of re-buying the accounts that already committed. The refusal is deliberately narrow: a genuine root row still fans out across every plan account under its own per-account key, because a legitimate multi-account purchase silently never happening is the same defect with the opposite sign. Regression coverage reproduces the real scenario end to end (retry handler -> real purchase.Manager, counting the commitments that reach the provider). Against pre-fix code the per-account retry fires three purchases instead of one; both directions are guarded: retries of different accounts' rows keep distinct provider tokens, and a root-row retry still buys once per account. No schema change. Closes #1537 --- internal/api/handler_purchases.go | 34 ++ .../handler_purchases_retry_fanout_test.go | 349 ++++++++++++++++++ internal/purchase/execution.go | 39 ++ .../purchase/money_path_regression_test.go | 164 ++++++++ 4 files changed, 586 insertions(+) create mode 100644 internal/api/handler_purchases_retry_fanout_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 2ea40a328..9d373c320 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,32 @@ 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, so the provider dedupes it. + // + // 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..b4047bf98 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -61,6 +61,20 @@ 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 { + if scoped := accountScopedLineageKeyOwner(exec, accounts); scoped != nil { + // Second gate for issue #1537. cloud_account_id is the + // primary one, but a row can reach here scopeless with a + // key that is already per-account: rows minted by the + // pre-#1537 retry handler and still pending/approved at + // deploy time. Fanning those out re-suffixes the key and + // re-buys every account that already committed, so fail + // loud instead — the operator re-drives the correct + // per-account row rather than the plan paying twice. + return fmt.Errorf("refusing to fan out execution %s across %d plan accounts: "+ + "its idempotency lineage is already scoped to account %s (%s) but the row carries no cloud_account_id; "+ + "re-deriving the key would double-purchase every account that already committed (issue #1537)", + exec.ExecutionID, len(accounts), scoped.ID, scoped.Name) + } return m.executeMultiAccount(ctx, exec, plan, accounts) } } @@ -716,6 +730,31 @@ func idempotencyLineageKey(exec *config.PurchaseExecution) string { return exec.ExecutionID } +// accountScopedLineageKeyOwner returns the plan account whose ID terminates +// exec's idempotency lineage key, or nil when the key is a root key. +// +// executeForAccount keys every fan-out row ":", so a +// terminal ":" match against the plan's OWN account set identifies a +// row that is already scoped to one account regardless of what cloud_account_id +// says. Matching against the plan's accounts (rather than parsing the string +// for a UUID-shaped tail) is what makes this precise: a root key is a UUID or a +// legacy execution ID and cannot end in ":" plus one of this plan's account +// IDs, so there are no false positives to trade off against. +func accountScopedLineageKeyOwner(exec *config.PurchaseExecution, accounts []config.CloudAccount) *config.CloudAccount { + key := idempotencyLineageKey(exec) + for i := range accounts { + // An empty ID would reduce the test to HasSuffix(key, ":") and could + // veto a legitimate fan-out, so require a real account ID to match on. + if accounts[i].ID == "" { + continue + } + if strings.HasSuffix(key, ":"+accounts[i].ID) { + return &accounts[i] + } + } + return nil +} + // 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..219e12235 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -357,3 +357,167 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { assert.Equal(t, "partially_completed", savedRoot.Status, "root must reflect partial success, never 'failed' (double-spend mislabel #642/#1014)") } + +// TestScopelessPerAccountKeyRefusesToFanOut is the issue #1537 defense-in-depth +// guard at the purchase layer. +// +// 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 idempotency lineage is already per-account — +// concretely, a successor minted by the pre-#1537 retry handler that is still +// pending/approved when the fix deploys. Fanning such a row out would re-suffix +// its key ("root:acct-A" -> "root:acct-A:acct-B"), derive tokens matching +// nothing the first attempt used, and re-buy every account that already +// committed. It must fail loud with ZERO purchases instead. +func TestScopelessPerAccountKeyRefusesToFanOut(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 + } + mockStore.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error { return nil } + + // The whole provider chain is wired and WOULD happily sell: without the + // guard this run completes and buys for both accounts. Counting the + // purchases (rather than leaving the chain unwired and reading an + // unexpected-mock-call panic as "refused") is what makes this test fail on + // pre-guard code instead of passing for the wrong reason. + var purchases int + var mu sync.Mutex + 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.Anything). + Run(func(mock.Arguments) { mu.Lock(); purchases++; 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", + } + + orphan := &config.PurchaseExecution{ + ExecutionID: "successor-of-acct-A-row", + // Scope lost in transit; the lineage key still says "account A". + CloudAccountID: nil, + IdempotencyKey: "root-lineage:acct-A", + 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}, + }, + } + + err := manager.executePurchase(ctx, orphan) + + mu.Lock() + got := purchases + mu.Unlock() + assert.Zero(t, got, + "a scopeless row whose lineage key is already per-account must buy NOTHING; each purchase here is an account that already committed being bought again (issue #1537)") + + require.Error(t, 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, err.Error(), "refusing to fan out", + "the error must be the #1537 refusal itself, not an incidental downstream failure") + assert.Contains(t, err.Error(), "acct-A", + "the error must name the account the lineage key is scoped to so an operator can re-drive the right row") +} + +// 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") +} From 85c0d941e06761d238d389925d4127c2d2c08611 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 22:00:48 +0200 Subject: [PATCH 2/2] fix(purchase): repair lost account scope instead of failing open Review follow-up on the #1537 guard. Two defects in the second gate, both in internal/purchase/execution.go. The gate detected a per-account row by matching the lineage key's terminal segment against the plan's CURRENT account set. That fails OPEN precisely 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 exactly as it did before the fix. Demonstrated against the previous commit -- plan {A, B}, scopeless row keyed "root-lineage:acct-C" -> no error and two commitments, under tokens derived from "root-lineage:acct-C:acct-A" and ":acct-B", neither of which matches anything the first attempt used. At $300 upfront per reserved instance that is $600 undeduplicated. Detect per-account-ness by the colon instead. executeForAccount is the only writer that puts a colon in a lineage key; every other producer yields a colon-free value (SavePurchaseExecution mints a bare UUID, persistRetryExecution copies verbatim or falls back to an ExecutionID, which is also a UUID, and the read path round-trips the column). So on a scopeless row a colon is by itself proof the row is per-account, and the detector no longer depends on mutable plan membership. The gate also had no way out. It errored, the row landed failed, and retrying it produced another scopeless successor carrying the same key, which the gate refused again -- through the retry threshold and through force. The predecessor could not be re-driven either: it already has retry_execution_id set, so the retry endpoint 409s with "act on its descendant instead", and the descendant is the row that can never execute. Only DB surgery or cancelling ended it. So when the terminal segment does match a current plan account, repair rather than refuse: stamp CloudAccountID and run the single-account re-drive the row was always meant to be. The lineage key is durable proof of the intended scope, and the derived per-rec token is then reproduced exactly, which makes this strictly safer than erroring -- same purchase count, correct dedupe, plus a way out. The repair persists: executeAndFinalize saves the row and cloud_account_id is in SavePurchaseExecution's ON CONFLICT SET. The fail-loud error is reserved for the unrecoverable case above, and now names the departed account so the operator knows which one to re-add. Also corrects an overstatement in persistRetryExecution's comment: carrying the account scope is the precondition for provider-side dedupe, but it is not sufficient for Azure savings-plans, which has no server-side idempotency key and names its order alias from time.Now().UnixNano(). recIsSafeToRedrive encodes that exclusion but gates only the reaper, not the user-facing retry; filed as #1668. Refs #1537 --- internal/api/handler_purchases.go | 12 +- internal/purchase/execution.go | 104 ++++++++---- .../purchase/money_path_regression_test.go | 159 ++++++++++++++---- 3 files changed, 208 insertions(+), 67 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 9d373c320..cf9f20bcd 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1816,7 +1816,17 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // "::" 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, so the provider dedupes it. + // 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 diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index b4047bf98..b9dcd9b0a 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -61,21 +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 { - if scoped := accountScopedLineageKeyOwner(exec, accounts); scoped != nil { - // Second gate for issue #1537. cloud_account_id is the - // primary one, but a row can reach here scopeless with a - // key that is already per-account: rows minted by the - // pre-#1537 retry handler and still pending/approved at - // deploy time. Fanning those out re-suffixes the key and - // re-buys every account that already committed, so fail - // loud instead — the operator re-drives the correct - // per-account row rather than the plan paying twice. - return fmt.Errorf("refusing to fan out execution %s across %d plan accounts: "+ - "its idempotency lineage is already scoped to account %s (%s) but the row carries no cloud_account_id; "+ - "re-deriving the key would double-purchase every account that already committed (issue #1537)", - exec.ExecutionID, len(accounts), scoped.ID, scoped.Name) - } - return m.executeMultiAccount(ctx, exec, plan, accounts) + return m.executeScopeAware(ctx, exec, plan, accounts) } } } @@ -730,29 +716,89 @@ func idempotencyLineageKey(exec *config.PurchaseExecution) string { return exec.ExecutionID } -// accountScopedLineageKeyOwner returns the plan account whose ID terminates -// exec's idempotency lineage key, or nil when the key is a root key. +// 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. // -// executeForAccount keys every fan-out row ":", so a -// terminal ":" match against the plan's OWN account set identifies a -// row that is already scoped to one account regardless of what cloud_account_id -// says. Matching against the plan's accounts (rather than parsing the string -// for a UUID-shaped tail) is what makes this precise: a root key is a UUID or a -// legacy execution ID and cannot end in ":" plus one of this plan's account -// IDs, so there are no false positives to trade off against. -func accountScopedLineageKeyOwner(exec *config.PurchaseExecution, accounts []config.CloudAccount) *config.CloudAccount { +// 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 reduce the test to HasSuffix(key, ":") and could - // veto a legitimate fan-out, so require a real account ID to match on. + // 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) { - return &accounts[i] + 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 } } - 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 "; ", diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index 219e12235..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" @@ -358,23 +359,24 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { "root must reflect partial success, never 'failed' (double-spend mislabel #642/#1014)") } -// TestScopelessPerAccountKeyRefusesToFanOut is the issue #1537 defense-in-depth -// guard at the purchase layer. +// 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 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 idempotency lineage is already per-account — -// concretely, a successor minted by the pre-#1537 retry handler that is still -// pending/approved when the fix deploys. Fanning such a row out would re-suffix -// its key ("root:acct-A" -> "root:acct-A:acct-B"), derive tokens matching -// nothing the first attempt used, and re-buy every account that already -// committed. It must fail loud with ZERO purchases instead. -func TestScopelessPerAccountKeyRefusesToFanOut(t *testing.T) { +// 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() - 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) @@ -390,25 +392,36 @@ func TestScopelessPerAccountKeyRefusesToFanOut(t *testing.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 } - // The whole provider chain is wired and WOULD happily sell: without the - // guard this run completes and buys for both accounts. Counting the - // purchases (rather than leaving the chain unwired and reading an - // unexpected-mock-call panic as "refused") is what makes this test fail on - // pre-guard code instead of passing for the wrong reason. - var purchases int 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.Anything). - Run(func(mock.Arguments) { mu.Lock(); purchases++; mu.Unlock() }). + 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{ @@ -419,32 +432,104 @@ func TestScopelessPerAccountKeyRefusesToFanOut(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - orphan := &config.PurchaseExecution{ - ExecutionID: "successor-of-acct-A-row", - // Scope lost in transit; the lineage key still says "account A". + 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:acct-A", + 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") - err := manager.executePurchase(ctx, orphan) + got := runScopelessRow(t, orphan, accounts) - mu.Lock() - got := purchases - mu.Unlock() - assert.Zero(t, got, - "a scopeless row whose lineage key is already per-account must buy NOTHING; each purchase here is an account that already committed being bought again (issue #1537)") + 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, err, + 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, err.Error(), "refusing to fan out", + 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, err.Error(), "acct-A", - "the error must name the account the lineage key is scoped to so an operator can re-drive the right row") + 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