diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index cb2d4eefe..9a690e08e 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1593,6 +1593,9 @@ func resolveOpsHint(failureReason string) string { // // State gate: // - failedExec.Status must be "failed" → 409 otherwise. +// - every rec must be safe to re-drive per purchase.RedriveRefusalReason +// → 409 with ops_hint + redrive_unsafe when any is not, NOT +// overridable by ?force=true (issue #1668). // - failedExec.Error must NOT match the persistent-failure map → // 409 with ops_hint when it does (Q3). // - failedExec.RetryAttemptN < retryThreshold OR ?force=true → soft @@ -1708,19 +1711,66 @@ func (h *Handler) loadAndValidateRetryRequest(ctx context.Context, req *events.L map[string]any{"retry_execution_id": *failedExec.RetryExecutionID}) } - if err := checkRetryRateGates(failedExec, req); err != nil { + if err := checkRetryEligibilityGates(failedExec, req); err != nil { return nil, nil, err } return failedExec, session, nil } -// checkRetryRateGates runs the persistent-failure (Q3) and -// retry-attempt-threshold (Q2) gates and returns the appropriate -// 409 ClientError when either fires. Extracted from -// loadAndValidateRetryRequest to keep that function under the -// cyclomatic-complexity ceiling without flattening the gate sequence. -func checkRetryRateGates(failedExec *config.PurchaseExecution, req *events.LambdaFunctionURLRequest) error { +// checkRetryEligibilityGates runs the provider re-drive-safety (issue +// #1668), persistent-failure (Q3) and retry-attempt-threshold (Q2) +// gates and returns the appropriate 409 ClientError when any fires. +// Extracted from loadAndValidateRetryRequest to keep that function +// under the cyclomatic-complexity ceiling without flattening the gate +// sequence. +func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *events.LambdaFunctionURLRequest) error { + // Provider re-drive safety (issue #1668). A "failed" row may in fact + // have landed its commitment at the provider (a timeout, a lost + // response, a post-purchase write that failed), so every retry is + // potentially a re-drive. purchase.RedriveRefusalReason is the same + // predicate the reaper's automatic re-drive gates on: it returns a + // reason exactly when the provider offers nothing that would collapse + // the second attempt onto the first, which today means Azure + // savings-plans (no server-side idempotency key, timestamp-derived + // order alias) plus any provider the predicate does not recognize. + // + // This gate runs FIRST and, unlike the threshold below, ?force=true + // does NOT override it: a savings plan cannot be canceled, so there + // is no recovery from getting this wrong, and a duplicate is not what + // the operator clicking Retry is asking for. An operator who genuinely + // wants a second commitment can submit a fresh purchase, which is an + // explicit buy rather than a retry of one that may already exist. + // + // A refusal here is permanent: nothing about the row can change to make + // it retryable. So the gate must fire only where the duplicate hazard is + // real. An execution carrying no recommendations buys nothing and cannot + // double-buy, and rec-less executions are legitimately created by + // createPurchaseExecutionsTx (handler_plans.go) and getOrCreateExecution + // (purchase/notifications.go); a failed approval email marks those + // "failed", and retrying is the only recovery. Refusing them would strand + // that whole class forever, so RedriveRefusalReason stays silent on the + // empty case. Empty here always means empty as created, never "we could + // not load them": GetExecutionByID propagates a recommendations unmarshal + // failure as an error (config/store_postgres.go), which this handler has + // already turned into a 500 well before this gate. + if reason := purchase.RedriveRefusalReason(failedExec); reason != "" { + return NewClientErrorWithDetails(409, + "this purchase cannot be retried safely: "+reason, + // ops_hint reuses the key the History UI already renders in + // place of the Retry button, so the reason reaches the operator + // today. redrive_unsafe distinguishes this PERMANENT refusal from + // the operator-fixable hints below, which clear once the + // configuration is fixed. + // + // No frontend reads redrive_unsafe yet, so it looks unused: issue + // #1714 is its intended consumer, where History will render a + // terminal badge instead of offering a Retry button that always + // 409s. That is presentation only. This refusal is enforced here, + // server-side, and does not depend on any client honoring it. + map[string]any{"ops_hint": reason, "redrive_unsafe": true}) + } + // Persistent-failure block (Q3). Surfaces the ops_hint via the // API for stale-cache callers; the History UI already shows it // inline in place of the Retry button. @@ -1823,10 +1873,11 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // 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 + // landed Azure SP order still duplicates it. Scope propagation is therefore // necessary for dedupe everywhere and sufficient everywhere except Azure SP. + // purchase.RedriveRefusalReason encodes exactly that exclusion and now gates + // this handler too (checkRetryEligibilityGates, issue #1668), so an Azure SP + // row never reaches this function in the first place. // // 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/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go new file mode 100644 index 000000000..337c2f6e0 --- /dev/null +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -0,0 +1,337 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/purchase" + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --- Issue #1668 regression harness ------------------------------------ +// +// A purchase_executions row reaching status="failed" does NOT prove the +// commitment never landed: a timeout, a lost response or a failed +// post-purchase write all produce a "failed" row behind an order the provider +// actually accepted. Every retry is therefore potentially a re-drive, and only +// a provider-side duplicate guard makes it safe. +// +// purchase.RedriveRefusalReason encodes which provider/service combinations +// have such a guard. Before this fix it gated only the reaper's automatic +// re-drive; the user-facing Retry button did not consult it at all, so an +// operator retrying a landed Azure savings-plans row bought a SECOND savings +// plan: a multi-year commitment that cannot be canceled. +// +// The tests below drive the REAL chain the operator drives: the retry HTTP +// handler (Handler.retryPurchase) and then, when it allows the retry, the REAL +// purchase.Manager executing the successor it persisted. They assert on the +// number of purchases that reach the cloud, not on statuses. A narrower unit +// test on purchase.RedriveRefusalReason alone stays green either way, because +// the predicate was already correct and simply unconsulted. + +const ( + redriveAzureSPExecID = "22222222-3333-4444-5555-666666666601" + redriveAzureSPAltExecID = "22222222-3333-4444-5555-666666666602" + redriveAzureRIExecID = "22222222-3333-4444-5555-666666666603" + redriveUnknownExecID = "22222222-3333-4444-5555-666666666604" + redriveForceExecID = "22222222-3333-4444-5555-666666666605" + redriveMixedExecID = "22222222-3333-4444-5555-666666666606" + redriveNoRecsExecID = "22222222-3333-4444-5555-666666666607" + redriveNoRecsPlanID = "33333333-4444-5555-6666-777777777701" + + redriveLineageKey = "lineage-1668" +) + +// redriveFailedRow builds the failed row an operator sees in History for a +// purchase whose order may well have landed at the provider: the execution +// timed out waiting for the response, so nothing local records the commitment +// even though the provider may hold it. +func redriveFailedRow(execID, provider, service string) *config.PurchaseExecution { + creator := retryCallerID + return &config.PurchaseExecution{ + ExecutionID: execID, + Status: "failed", + IdempotencyKey: redriveLineageKey + ":" + execID, + Error: "context deadline exceeded while awaiting the order response", + CreatedByUserID: &creator, + CapacityPercent: 100, + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{{ + Provider: provider, + Service: service, + ResourceType: "Standard_D2s_v3", + Region: "westeurope", + Count: 1, + Term: 3, + UpfrontCost: 30000, + Selected: true, + }}, + } +} + +// purchasesFiredByRetry drives the REAL retry handler for failed and, when the +// handler allows the retry, runs the successor it persisted through the REAL +// purchase.Manager exactly as production does. It returns the idempotency +// token of every commitment purchase that reached the "cloud" (one entry per +// real purchase) alongside the handler's error (nil when the retry was +// allowed). +func purchasesFiredByRetry(t *testing.T, failed *config.PurchaseExecution, req *events.LambdaFunctionURLRequest) ([]string, error) { + t.Helper() + + // retry-own authorizes the row's creator (issue #907), so RBAC is out of + // the way and the re-drive-safety gate is what the assertions see. + session := &Session{UserID: retryCallerID, Email: "operator@example.com"} + handler, mockConfig, _ := buildSessionRetryHandler(failed, session, false, true) + + var saved []*config.PurchaseExecution + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + // Copy so later in-place mutations by the handler don't + // retroactively rewrite the captured successor. + snap := *args.Get(1).(*config.PurchaseExecution) + saved = append(saved, &snap) + }). + Return(nil).Maybe() + + if _, err := handler.retryPurchase(context.Background(), req, failed.ExecutionID); err != nil { + assert.Empty(t, saved, + "a refused retry must not persist a successor row; a persisted successor is one approval click away from reaching the provider") + return nil, err + } + + // First save is the successor; the second is the original row stamped with + // the linkage pointer (the retry tx orders them that way for the FK). + require.NotEmpty(t, saved, "an allowed retry must have persisted a successor execution") + return executeRedriveSuccessor(t, saved[0]), nil +} + +// executeRedriveSuccessor runs a retry successor through the real +// purchase.Manager the way production does: the row is picked up from the +// execute_purchase queue message once the operator has approved it, claimed, +// and executed. Returns the idempotency token of every purchase that reached +// the cloud. +func executeRedriveSuccessor(t *testing.T, successor *config.PurchaseExecution) []string { + t.Helper() + + 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) + 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(context.Background(), string(body)), + "the successor must execute cleanly; a non-nil error would make the purchase tally meaningless") + + return svc.purchasedTokens() +} + +// assertRedriveRefused asserts the shape of the refusal the retry endpoint +// must return: a 409 (the same status the ops-hint and threshold gates use, so +// the History UI already renders it in place of the Retry button) carrying a +// specific reason rather than a generic client error. +func assertRedriveRefused(t *testing.T, err error, wantReasonFragment string) { + t.Helper() + require.Error(t, err, "the retry must be refused") + ce, ok := IsClientError(err) + require.True(t, ok, "the refusal must be a structured client error, got: %v", err) + assert.Equal(t, 409, ce.code) + require.NotNil(t, ce.Details(), "the refusal must carry structured details the UI can render") + assert.Equal(t, true, ce.Details()["redrive_unsafe"], + "redrive_unsafe marks this as a permanent refusal, unlike the operator-fixable ops hints") + hint, ok := ce.Details()["ops_hint"].(string) + require.True(t, ok, "ops_hint must be a string so the existing History renderer can show it") + assert.Contains(t, hint, wantReasonFragment, + "the refusal must name why this purchase cannot be re-driven, not fail generically") +} + +// TestRetryOfLandedAzureSavingsPlanPurchasesNothing is the issue #1668 +// regression guard. +// +// Scenario, exactly as it happens in production: an Azure savings-plans +// purchase is submitted, Azure accepts the order alias, and the execution then +// times out waiting for the response. The row lands in History as "failed" +// with a Retry button on it. The operator, seeing a failure, clicks Retry. +// +// Pre-fix the retry path gated only on status, RBAC, already-retried, the +// ops-hint map and the attempt threshold, none of which knows anything about +// provider re-drive safety, so a successor row was created and, once +// approved, purchased a SECOND savings plan. Azure savings plans carry no +// server-side idempotency key and name their order alias from +// time.Now().UnixNano(), so nothing on the provider side collapsed the second +// order onto the first, and a savings plan cannot be canceled: at $30k +// upfront that is $30k of unrecoverable spend from one Retry click. +// +// Post-fix the handler consults purchase.RedriveRefusalReason, the same +// predicate the reaper's automatic re-drive already gated on, and refuses, +// so no successor row exists and nothing can reach Azure. +func TestRetryOfLandedAzureSavingsPlanPurchasesNothing(t *testing.T) { + // Both spellings the recommendation records use in the wild; the reaper's + // exclusion covers both and the retry path must not diverge. + cases := []struct { + name string + execID string + service string + }{ + {name: "savingsplans", execID: redriveAzureSPExecID, service: "savingsplans"}, + {name: "savings-plans", execID: redriveAzureSPAltExecID, service: "savings-plans"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + failed := redriveFailedRow(tc.execID, "azure", tc.service) + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + // The money assertion comes first and deliberately uses assert, + // not require, so the refusal-shape checks below still name the + // cause in the same failing run on the pre-fix code. + assert.Empty(t, tokens, + "retrying a possibly-landed Azure savings-plans row must fire ZERO purchases; "+ + "any token here is a second, non-cancellable savings plan bought by one Retry click (issue #1668)") + assertRedriveRefused(t, err, "second savings plan") + }) + } +} + +// TestRetryOfLandedAzureSavingsPlanIsNotForceOverridable pins the explicit +// decision that ?force=true, which does override the retry-attempt threshold, +// must NOT override the re-drive-safety refusal. A savings plan cannot be +// canceled, so there is no recovery from a wrong override, and a duplicate is +// not what the operator clicking Retry is asking for; buying a second one on +// purpose is a fresh purchase, not a retry. +func TestRetryOfLandedAzureSavingsPlanIsNotForceOverridable(t *testing.T) { + failed := redriveFailedRow(redriveForceExecID, "azure", "savingsplans") + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReqWithForce()) + + assert.Empty(t, tokens, + "?force=true must not buy a second Azure savings plan; force overrides the attempt threshold, not provider safety") + assertRedriveRefused(t, err, "second savings plan") +} + +// TestRetryOfFailedRecommendationlessExecutionIsAllowed is the guard for the +// far side of the gate: it must fire only where the duplicate hazard is real, +// because a refusal here is permanent. +// +// createPurchaseExecutionsTx (handler_plans.go) and getOrCreateExecution +// (purchase/notifications.go) both create executions with NO recommendations, +// and a failed approval email marks those "failed". They buy nothing, so they +// cannot double-buy, and retrying is the only recovery an operator has. An +// earlier revision of this gate refused them along with the genuinely unsafe +// rows, which would have stranded the whole class permanently. +// +// Empty here always means empty as created, never "we could not load them": +// GetExecutionByID propagates a recommendations unmarshal failure as an error, +// which the handler turns into a 500 long before this gate runs. +func TestRetryOfFailedRecommendationlessExecutionIsAllowed(t *testing.T) { + creator := retryCallerID + failed := &config.PurchaseExecution{ + ExecutionID: redriveNoRecsExecID, + PlanID: redriveNoRecsPlanID, + StepNumber: 1, + Status: "failed", + // A transient send failure, deliberately not one of the + // persistent-failure hints, so the ops-hint gate stays out of the way. + Error: "failed to send approval email: SES throttle exceeded", + CreatedByUserID: &creator, + Source: common.PurchaseSourceWeb, + // Recommendations deliberately nil: this is how both creation paths + // above persist the row. + } + session := &Session{UserID: retryCallerID, Email: "operator@example.com"} + + successor, updated := runSessionRetryAllowed(t, failed, session, false, true, sessionRetryReq()) + + assert.Empty(t, successor.Recommendations, + "the successor carries the predecessor's (empty) recommendations, so it buys nothing; that is precisely why refusing it bought no safety") + assert.Equal(t, 1, successor.RetryAttemptN, "the retry chain still advances") + require.NotNil(t, updated.RetryExecutionID, "the original must be linked to its successor") + assert.Equal(t, successor.ExecutionID, *updated.RetryExecutionID) +} + +// TestRetryOfMixedExecutionWithOneAzureSavingsPlanIsRefused guards the whole +// recommendation list, not just its head. An execution can carry several recs, +// and re-driving it re-drives every one of them: a single unsafe rec anywhere +// in the list makes the whole retry unsafe, however many safe recs sit in +// front of it. The AWS rec here is deliberately first, so a gate that checked +// only the leading rec would pass this row straight through to Azure. +func TestRetryOfMixedExecutionWithOneAzureSavingsPlanIsRefused(t *testing.T) { + failed := redriveFailedRow(redriveMixedExecID, "aws", "ec2") + failed.Recommendations = append(failed.Recommendations, config.RecommendationRecord{ + Provider: "azure", + Service: "savingsplans", + ResourceType: "Standard_D2s_v3", + Region: "westeurope", + Count: 1, + Term: 3, + UpfrontCost: 30000, + Selected: true, + }) + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + assert.Empty(t, tokens, + "one unsafe rec must block the whole retry; purchasing the safe recs alone would still re-drive the Azure savings plan alongside them") + assertRedriveRefused(t, err, "second savings plan") +} + +// TestRetryOfUnknownProviderRowIsRefused covers the fail-closed half of the +// same gate: purchase.RedriveRefusalReason refuses a provider it does not +// recognize rather than assuming a duplicate guard exists. Before this fix an +// unknown provider was un-redrivable by the reaper yet freely retryable from +// the API. The two paths now agree. +func TestRetryOfUnknownProviderRowIsRefused(t *testing.T) { + failed := redriveFailedRow(redriveUnknownExecID, "oraclecloud", "compute") + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + assert.Empty(t, tokens, + "a provider with no known duplicate guard must not be re-driven from the retry endpoint") + assertRedriveRefused(t, err, `provider "oraclecloud" is not known to reject a duplicate purchase`) +} + +// TestRetryOfFailedAzureReservationStillPurchasesOnce is the over-blocking +// guard. The refusal must be as narrow as the underlying provider gap: Azure +// reservations go through DoIdempotentPurchaseTwoStep (#729), which looks the +// order up before purchasing, so retrying one is safe and must keep working. +// A gate that blocked every Azure row would strand legitimate retries. +// +// The single purchase must also carry the token derived from the PREDECESSOR's +// lineage key, which is what makes the provider-side dedupe engage if the +// first attempt had in fact landed. +func TestRetryOfFailedAzureReservationStillPurchasesOnce(t *testing.T) { + failed := redriveFailedRow(redriveAzureRIExecID, "azure", "compute") + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + require.NoError(t, err, "an Azure reservation retry is safe and must still be allowed") + assert.Equal(t, []string{common.DeriveIdempotencyToken(failed.IdempotencyKey, 0)}, tokens, + "the retry must fire exactly one purchase, under the predecessor's token so Azure's two-step lookup dedupes it") +} diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 6859c1713..b221b6f97 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -247,12 +247,38 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE return execErr } -// allRecsSafeToRedrive reports whether every recommendation in the execution -// can be safely re-driven without risking a double-purchase. A re-drive is safe -// when the underlying provider purchase API is idempotent under the -// DeriveIdempotencyToken(idempotencyLineageKey(exec), i) scheme used by -// execution.go. An in-place re-drive (this path) keeps the same row, so the -// lineage key is unchanged and the token is reproduced exactly. +// allRecsSafeToRedrive reports whether this sweep's automatic in-place re-drive +// may run for exec. Two conditions: every recommendation must be safe to +// re-drive, which RedriveRefusalReason below owns and documents, AND the +// execution must carry at least one recommendation. +// +// The empty-recommendations condition is this sweep's own, deliberately not part +// of the shared safety policy. It is not a duplicate-risk statement: a re-drive +// that purchases nothing cannot double-buy. It means "nothing here worth +// re-driving, hand it to a human", and the safe-fail path it selects is benign +// (the row is marked failed and surfaces in History, where a human can retry +// it). Folding it into the shared predicate would export it to the user-facing +// retry endpoint, where the consequence is the opposite of benign: a permanent +// refusal of a row that cannot double-buy, with no recovery path (issue #1668). +// +// An in-place re-drive keeps the same row, so the lineage key is unchanged and +// DeriveIdempotencyToken(idempotencyLineageKey(exec), i) reproduces the original +// token exactly. That is what lets the provider-side dedupe engage at all. +func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { + return len(exec.Recommendations) > 0 && RedriveRefusalReason(exec) == "" +} + +// RedriveRefusalReason returns a short operator-facing reason why exec must not +// be re-driven, or "" when every recommendation on it carries a provider-side +// guarantee that a second attempt collapses onto the first. +// +// This is the single source of truth for re-drive safety. Both the reaper's +// automatic in-place re-drive (via allRecsSafeToRedrive) and the user-facing +// Retry endpoint (Handler.checkRetryEligibilityGates in internal/api) gate on +// it, so a provider/service that is unsafe for one is unsafe for the other. +// Before issue #1668 only the reaper consulted it, and clicking Retry on a +// landed Azure savings-plans row bought a second savings plan, which cannot be +// canceled. // // Safe providers / services (issue #639): // - AWS (all services): tag-guard or ClientToken deduplication (#636/#638). @@ -262,47 +288,56 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE // - GCP compute (CUDs): server-side RequestId + deterministic name from // the token (#654). // -// NOT safe - safe-fail path preserved: +// NOT safe: // - Azure savings-plans: the OrderAlias API uses time.Now().UnixNano() as // the alias name; there is no server-side idempotency key and no // tag-based lookup implemented yet. Re-driving would create a duplicate // savings plan. +// - Any provider this function does not recognize, rather than assuming a +// guard exists. // // Empty provider ("") is treated as AWS (pre-multi-cloud legacy rows). -// An execution with no recommendations returns false so it falls through to the -// safe-fail path (nothing to re-drive anyway). -func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { - if len(exec.Recommendations) == 0 { - return false - } - for _rvc := range exec.Recommendations { - rec := exec.Recommendations[_rvc] - if !recIsSafeToRedrive(rec) { - return false +// +// It answers exactly one question: could re-driving these recommendations buy +// something twice. An execution with no recommendations buys nothing, so it has +// no duplicate risk and gets no refusal here. Callers that need "there is +// nothing worth re-driving" must say so themselves, as allRecsSafeToRedrive +// does above. +// +// The reason is rendered verbatim to the operator, so it explains the refusal +// in product terms rather than naming internals. +func RedriveRefusalReason(exec *config.PurchaseExecution) string { + for i := range exec.Recommendations { + if reason := recRedriveRefusalReason(exec.Recommendations[i]); reason != "" { + return reason } } - return true + return "" } -// recIsSafeToRedrive reports whether a single recommendation can be safely -// re-driven. Extracted from allRecsSafeToRedrive to keep that function under -// the gocyclo budget and to make per-rec exclusions explicit. -func recIsSafeToRedrive(rec config.RecommendationRecord) bool { +// recRedriveRefusalReason returns the reason a single recommendation cannot be +// safely re-driven, or "" when it can. Extracted from RedriveRefusalReason to +// keep that function under the gocyclo budget and to make per-rec exclusions +// explicit. +func recRedriveRefusalReason(rec config.RecommendationRecord) string { switch rec.Provider { case "", "aws": // Empty provider is legacy AWS. All AWS services honor IdempotencyToken. - return true + return "" case "azure": // Azure savings-plans uses a timestamp-based alias name and has no // server-side idempotency key, so a re-drive would create a duplicate. // All other Azure services use DoIdempotentPurchaseTwoStep (#729). - return rec.Service != "savingsplans" && rec.Service != "savings-plans" + if rec.Service == "savingsplans" || rec.Service == "savings-plans" { + return "Azure savings plans have no provider-side duplicate guard, so re-driving this purchase would buy a second savings plan that cannot be canceled" + } + return "" case "gcp": // GCP compute CUDs use RequestId + deterministic name from the token (#654). - return true + return "" default: // Unknown provider: refuse to re-drive rather than risk a double-buy. - return false + return fmt.Sprintf("provider %q is not known to reject a duplicate purchase, so re-driving this could buy a second commitment", rec.Provider) } } diff --git a/internal/purchase/redrive_guard_reach_test.go b/internal/purchase/redrive_guard_reach_test.go new file mode 100644 index 000000000..ee1fc719b --- /dev/null +++ b/internal/purchase/redrive_guard_reach_test.go @@ -0,0 +1,122 @@ +package purchase + +import ( + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- Guard reach vs dispatch reach (issue #1668) ------------------------ +// +// A CodeRabbit finding on PR #1713 proposed normalizing rec.Service before the +// Azure savings-plans re-drive guard, on the grounds that exact matching lets +// variants like "SavingsPlans" or "savings_plans" through. +// +// They do get past the guard. They cannot reach a purchase, which is the +// property that matters, and this test pins it: +// +// 1. mapServiceType is the ONLY thing standing between rec.Service and the +// provider's service client (execution.go executeSinglePurchase calls +// mapServiceType then GetServiceClient with the result). +// 2. Azure routes exactly ONE ServiceType to its savings-plans client: +// `case common.ServiceSavingsPlansAll` in newServiceClientForSubscription +// (providers/azure/provider.go). That switch does no normalization of its +// own, and NewSavingsPlansClient is constructed nowhere else in the Azure +// provider. Every other value lands in `default:` and returns +// "unsupported service: " without purchasing anything. +// +// So a value only reaches an Azure savings-plans purchase if +// mapServiceType(value) == ServiceSavingsPlansAll. The assertion below is that +// this is true for EXACTLY the values the guard refuses -- an iff, not a +// one-way implication. "SavingsPlans" is not refused by the guard AND does not +// dispatch, so it spends nothing. +// +// Adding a normalizer would WIDEN what the guard accepts as a savings plan +// while the dispatch axis stayed exact, creating a second normalization axis +// that would have to be kept in lockstep with the first forever. This repo has +// been bitten by exactly that shape. This test is the cheaper guarantee: if +// either axis ever moves, the iff breaks here. +func TestRedriveGuardReachMatchesDispatchReach(t *testing.T) { + m := NewManager(ManagerConfig{}) + + candidates := []string{ + // Every key of mapSavingsPlansSlug (execution.go). Only the first two + // map to ServiceSavingsPlansAll; the rest are AWS plan-type slugs. + "savings-plans", "savingsplans", + "savings-plans-compute", "savingsplans-compute", + "savings-plans-ec2instance", "savingsplans-ec2instance", + "savings-plans-sagemaker", "savingsplans-sagemaker", + "savings-plans-database", "savingsplans-database", + + // Every key of mapServiceSlug (execution.go). + "compute", "relational-db", "cache", "search", "data-warehouse", + "ec2", "rds", "elasticache", "opensearch", "redshift", "memorydb", + + // The literal value of all 20 common.ServiceType constants, so a value + // that bypasses both slug maps and passes through verbatim is covered. + string(common.ServiceCompute), string(common.ServiceRelationalDB), + string(common.ServiceNoSQL), string(common.ServiceCache), + string(common.ServiceSearch), string(common.ServiceDataWarehouse), + string(common.ServiceStorage), string(common.ServiceSavingsPlansAll), + string(common.ServiceSavingsPlansCompute), string(common.ServiceSavingsPlansEC2Instance), + string(common.ServiceSavingsPlansSageMaker), string(common.ServiceSavingsPlansDatabase), + string(common.ServiceCommitments), string(common.ServiceOther), + string(common.ServiceEC2), string(common.ServiceRDS), + string(common.ServiceElastiCache), string(common.ServiceOpenSearch), + string(common.ServiceRedshift), string(common.ServiceMemoryDB), + + // The variants the finding named, plus neighboring mutations: case, + // separator, whitespace, and near-miss spellings. + "SavingsPlans", "SAVINGSPLANS", "SavingsPlansAll", "savingsPlans", + "Savings-Plans", "SAVINGS-PLANS", + "savings_plans", "savings_plans_compute", + " savingsplans", "savingsplans ", "\tsavingsplans", "savings plans", + "savingsplan", "saving-plans", "savingsplans\n", + + // Not a service at all. + "", "unknown", "azure-savings-plans", + } + + for _, service := range candidates { + service := service + t.Run("service="+service, func(t *testing.T) { + rec := config.RecommendationRecord{Provider: "azure", Service: service} + + // Can this value reach Azure's savings-plans client at all? + dispatchesToSavingsPlans := m.mapServiceType(service) == common.ServiceSavingsPlansAll + // Does the money guard refuse it? + guardRefuses := recRedriveRefusalReason(rec) != "" + + assert.Equal(t, dispatchesToSavingsPlans, guardRefuses, + "guard reach and dispatch reach must be identical for %q: dispatchesToSavingsPlans=%v guardRefuses=%v. "+ + "A value that dispatches but is not refused is a double-purchase hole; a value that is refused but "+ + "cannot dispatch is an unretryable row for no reason", + service, dispatchesToSavingsPlans, guardRefuses) + }) + } +} + +// TestRedriveGuardRefusalSetIsExactlyTheDispatchableSpellings states the same +// property as a closed set, so a change that widens EITHER axis is visible as a +// diff to this list rather than only as a failure in the loop above. +func TestRedriveGuardRefusalSetIsExactlyTheDispatchableSpellings(t *testing.T) { + m := NewManager(ManagerConfig{}) + + // The only two spellings that reach Azure's savings-plans client. + for _, service := range []string{"savingsplans", "savings-plans"} { + require.Equal(t, common.ServiceSavingsPlansAll, m.mapServiceType(service), + "%q must still dispatch to the Azure savings-plans client", service) + assert.NotEmpty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: "azure", Service: service}), + "%q dispatches to a savings-plans purchase, so the re-drive guard must refuse it", service) + } + + // Azure reservations must stay retryable: the guard has to be as narrow as + // the provider gap, or legitimate retries are stranded. + for _, service := range []string{"compute", "relational-db", "cache", "nosql", "memorydb", "search", "data-warehouse"} { + assert.Empty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: "azure", Service: service}), + "%q goes through DoIdempotentPurchaseTwoStep (#729) and must remain retryable", service) + } +}