From dfb5ff2f2317bc4497e7f388bf090b36c44ec93d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 23:23:02 +0200 Subject: [PATCH 1/4] fix(purchase): gate user-facing Retry on provider re-drive safety (#1668) 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 leave 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. The codebase already knew this. purchase.recIsSafeToRedrive encoded that Azure savings-plans is unsafe to re-drive (timestamp-derived order alias, no server-side idempotency key) and that an unrecognized provider must be refused. It gated the reaper's automatic re-drive and the reaper's "safe to retry" failure note, and nothing else. The user-facing Retry endpoint gated on status, RBAC, already-retried, the ops-hint map and the attempt threshold, none of which knows anything about provider re-drive safety, so clicking Retry on a landed Azure savings-plans row created a successor that bought a SECOND savings plan. A savings plan is a multi-year commitment that cannot be canceled. Consult the existing predicate on the money path rather than adding a second notion of re-drive safety: - Rework recIsSafeToRedrive into recRedriveRefusalReason (same decisions, expressed as an explanation) and export RedriveRefusalReason over an execution. allRecsSafeToRedrive becomes RedriveRefusalReason(exec) == "", so the reaper and the API cannot disagree and the reaper's semantics, including empty-recs and unknown-provider, are unchanged. - Gate the retry endpoint on it in checkRetryEligibilityGates (renamed from checkRetryRateGates, one call site). The refusal is a 409 naming the provider reason, in the same shape as the ops-hint 409 the History UI already renders, plus a redrive_unsafe flag marking it permanent. - The gate runs first and ?force=true does not override it. force exists to push past the attempt threshold; there is no recovery from a wrongly overridden savings-plan purchase, and an operator who genuinely wants a second commitment can submit a fresh purchase. Regression coverage drives the real chain the operator drives, the retry handler followed by the real purchase.Manager executing the successor, and asserts on the number of purchases reaching the cloud rather than on statuses: a unit test on the predicate alone stays green either way, because the predicate was already correct and simply unconsulted. Covers both savings-plans spellings, a mixed execution whose unsafe rec is not first, an unknown provider, ?force=true, and, as the over-blocking guard, an Azure reservation row that must still retry into exactly one purchase under the predecessor's token. Closes #1668 --- internal/api/handler_purchases.go | 51 ++- .../handler_purchases_retry_redrive_test.go | 295 ++++++++++++++++++ internal/api/handler_purchases_test.go | 4 + internal/purchase/manager.go | 50 ++- 4 files changed, 376 insertions(+), 24 deletions(-) create mode 100644 internal/api/handler_purchases_retry_redrive_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index cb2d4eefe..dd92146eb 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,46 @@ 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. + 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; redrive_unsafe distinguishes this + // permanent refusal from the operator-fixable hints below, which + // do clear once the configuration is fixed. + 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 +1853,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..7980d0273 --- /dev/null +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -0,0 +1,295 @@ +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" + + 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") +} + +// 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/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 5878c99c2..7cb67eb29 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3249,6 +3249,10 @@ func TestHandler_retryPurchase_PersistentFailure_BlocksWithOpsHint(t *testing.T) Status: "failed", Error: "FROM_EMAIL not configured for this deployment", CreatedByUserID: &creator, + // A safe-to-re-drive rec so the re-drive-safety gate (issue #1668), + // which runs ahead of the ops-hint gate and refuses a row carrying no + // recommendations at all, passes through to the gate under test. + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 6859c1713..ecf446be3 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -272,37 +272,59 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE // 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 { + return RedriveRefusalReason(exec) == "" +} + +// RedriveRefusalReason returns a short operator-facing reason why exec must not +// be re-driven, or "" when every recommendation on it is safe to re-drive. It +// is the bool form above expressed as an explanation, so the two can never +// disagree. +// +// 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. +// +// 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 { if len(exec.Recommendations) == 0 { - return false + return "this execution carries no recommendations, so what it did or did not purchase cannot be established" } - for _rvc := range exec.Recommendations { - rec := exec.Recommendations[_rvc] - if !recIsSafeToRedrive(rec) { - return false + 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) } } From 98f469111d288e2f238ff79cfd8ee917763896bc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 23:56:57 +0200 Subject: [PATCH 2/4] fix(purchase): narrow the re-drive gate to real hazards, render it terminal (#1668) Two CodeRabbit findings on the re-drive-safety gate. Empty recommendations no longer refuse. createPurchaseExecutionsTx (handler_plans.go) and getOrCreateExecution (purchase/notifications.go) both create executions with no recommendations, and a failed approval email marks those "failed". The gate refused them along with the genuinely unsafe rows, and a refusal here is permanent, so that whole class became unretryable with no operator recovery. Such an execution buys nothing, so it cannot double-buy: refusing it was failing closed against a hazard that is absent. The distinction between "empty as created" and "empty because loading failed" is decidable and lands on the safe side by itself, so nothing is being papered over: a recommendations unmarshal failure propagates as an error from GetExecutionByID (config/store_postgres.go), which the handler turns into a 500 long before the gate runs. Empty at the gate is always empty as created. The condition moves back into allRecsSafeToRedrive, which keeps its byte-for-byte semantics, because it was never a statement about provider duplicate risk. It is the reaper sweep's own "nothing to do, hand it to a human" condition, and the safe-fail path it selects there is benign. RedriveRefusalReason now answers exactly one question, could re-driving these recommendations buy something twice, so exporting it to a caller whose refusal is permanent is safe. The frontend renders the refusal as terminal. redrive_unsafe was reaching the retry-error handler and being displayed through the ops_hint branch, which words a refusal as operator-fixable and re-enables the Retry button. That is the one outcome this p0 exists to prevent: the user reads "failed, try again", clicks again, and buys the duplicate the backend just refused. A redrive_unsafe refusal now replaces the button with the same ops-hint badge renderActionCell already uses for non-retryable rows, carrying the provider's reason, and does not re-enable. The badge is built as a DOM node with textContent rather than an HTML string, since the reason interpolates the row's provider. Regression coverage for both, each confirmed failing on the previous head: a failed recommendationless execution retries into a linked successor, and a redrive_unsafe refusal leaves no retry affordance in the document while an ops_hint-only refusal still leaves the button live. The ops-hint test fixture reverts to its original form: it no longer needs a recommendation to reach the gate under test. Refs #1668 --- .../__tests__/history-retry-button.test.ts | 82 +++++++++++++++++++ frontend/src/api/types.ts | 8 +- frontend/src/history.ts | 31 +++++++ internal/api/handler_purchases.go | 13 +++ .../handler_purchases_retry_redrive_test.go | 42 ++++++++++ internal/api/handler_purchases_test.go | 4 - internal/purchase/manager.go | 24 ++++-- 7 files changed, 191 insertions(+), 13 deletions(-) diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index d24d13392..8bde52fd5 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -465,6 +465,88 @@ describe('History inline Retry button (issue #47)', () => { expect(btn?.disabled).toBe(false); }); + // Issue #1668: a 409 carrying redrive_unsafe means the purchase can NEVER + // be retried, because the provider cannot tell a re-drive apart from a fresh + // purchase and a second attempt would buy a second commitment. Every other + // error branch describes something that can be fixed and retried, so those + // correctly leave the button live. Rendering this one the same way is what + // the p0 exists to prevent: the user reads "failed, try again", clicks + // again, and buys the duplicate the backend gate just refused. + test('redrive_unsafe refusal renders as terminal: no retry affordance left on the row (issue #1668)', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + const reason = 'Azure savings plans have no provider-side duplicate guard, so re-driving this purchase would buy a second savings plan that cannot be canceled'; + const refusal = Object.assign(new Error('this purchase cannot be retried safely: ' + reason), { + status: 409, + details: { ops_hint: reason, redrive_unsafe: true }, + }); + (api.retryPurchase as jest.Mock).mockRejectedValue(refusal); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], + }); + console.error = jest.fn(); + + await loadHistory(); + const btn = document.querySelector('.history-retry-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + // The affordance is gone from the document, not merely disabled: a + // disabled button still reads as "temporarily unavailable". + expect(document.querySelector('.history-retry-btn')).toBeNull(); + + // Replaced by the same badge renderActionCell uses for rows that cannot + // be retried, carrying the provider's reason. + const badge = document.querySelector('.history-ops-hint'); + expect(badge).not.toBeNull(); + expect(badge?.textContent).toContain('second savings plan'); + + // The badge is built with textContent, so an untrusted reason cannot + // inject markup. + expect(badge?.querySelector('*')).toBeNull(); + + // And the toast says cannot, not failed. + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ + kind: 'error', + message: expect.stringContaining('Cannot retry:'), + }), + ); + }); + + // The complement of the test above: an ordinary refusal must keep behaving + // as it did. ops_hint alone (operator-fixable) is not terminal -- the + // operator fixes the configuration and retries the same row. + test('ops_hint refusal without redrive_unsafe still leaves the Retry button in place', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + const refusal = Object.assign(new Error('this failure is operator-fixable'), { + status: 409, + details: { ops_hint: 'Set FROM_EMAIL tfvar then retry' }, + }); + (api.retryPurchase as jest.Mock).mockRejectedValue(refusal); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], + }); + console.error = jest.fn(); + + await loadHistory(); + const btn = document.querySelector('.history-retry-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + expect(document.querySelector('.history-retry-btn')).not.toBeNull(); + expect(btn?.disabled).toBe(false); + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ + kind: 'error', + message: expect.stringContaining('Failed to retry:'), + }), + ); + }); + test('admin WITHOUT Purchaser membership does not see Retry on rows they did not create (CR #924 F5)', async () => { // Issue #923 + CR #924 F5: retry-any:purchases is carved out of // admin:*. canRetryFailedRow must gate on diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 459bad021..d8c5d37cf 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -913,8 +913,12 @@ export interface ApiError extends Error { status?: number; // Structured detail fields the backend attaches to a 4xx response // alongside the human `error` message (e.g. `ops_hint`, - // `retry_attempt_n`, `threshold`, `retry_execution_id`). Callers - // can branch on these without substring-matching the message. + // `retry_attempt_n`, `threshold`, `retry_execution_id`, + // `redrive_unsafe`). Callers can branch on these without + // substring-matching the message. `redrive_unsafe: true` (issue #1668) + // marks a refusal as PERMANENT: unlike `ops_hint`, nothing an operator + // does makes the purchase retryable, so callers must not leave a retry + // affordance on screen. // See internal/api/handler.go for the flattening — keys are // promoted to the top level of the JSON body. details?: Record; diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 2615de798..ac353de0f 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -1330,10 +1330,12 @@ function wireRowActionHandlers(container: HTMLElement): void { } catch (retryError) { console.error('Failed to retry purchase:', retryError); // Surface structured retry hints from the backend (issue #47): + // * redrive_unsafe: permanent, this purchase can never be retried // * ops_hint — operator-actionable reason; takes priority // * retry_attempt_n + threshold — soft-block message // * else — fall back to the raw error message const err = retryError as Error & { details?: Record }; + const redriveUnsafe = err.details?.['redrive_unsafe'] === true; const opsHint = typeof err.details?.['ops_hint'] === 'string' ? err.details['ops_hint'] : ''; const retryAttemptN = typeof err.details?.['retry_attempt_n'] === 'number' ? err.details['retry_attempt_n'] : undefined; const threshold = typeof err.details?.['threshold'] === 'number' ? err.details['threshold'] : undefined; @@ -1344,6 +1346,35 @@ function wireRowActionHandlers(container: HTMLElement): void { detailMessage = `already retried ${retryAttemptN} times (threshold ${threshold}) — confirm the override prompt to force`; } const finalMessage = detailMessage || err.message || 'unknown error'; + // redrive_unsafe (issue #1668) is terminal, not a failed attempt: the + // provider offers no way to tell a re-drive apart from a fresh + // purchase, so clicking Retry again would buy a second commitment. + // Every other branch here describes something the user or an operator + // can act on and then retry, so leaving the button live is right for + // them and wrong for this one. Replace the button with the same + // ops-hint badge renderActionCell shows on rows that are not + // retryable, and do NOT re-enable; the whole point of the gate is + // that re-clicking must not be on offer. + // + // This covers the row in place. A later loadHistory() re-render brings + // the button back, because the History row projection does not yet + // carry the re-drive verdict (only the retry response does), so + // renderActionCell has nothing to gate on. Issue #1714 tracks moving + // the verdict onto the row so the button is never offered at all. The + // backend refuses either way; this is about not inviting the click. + if (redriveUnsafe) { + showToast({ message: `Cannot retry: ${finalMessage}`, kind: 'error', timeout: 8_000 }); + // Built as a DOM node rather than an HTML string: the reason is + // server-generated and interpolates the row's provider, so it is + // untrusted input. textContent removes the injection sink entirely + // instead of relying on an escape helper being applied correctly. + const badge = document.createElement('span'); + badge.className = 'history-ops-hint'; + badge.title = 'This purchase cannot be retried - retrying could buy a second commitment'; + badge.textContent = `⚠ ${finalMessage}`; + btn.replaceWith(badge); + return; + } showToast({ message: `Failed to retry: ${finalMessage}`, kind: 'error' }); btn.disabled = false; return; diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index dd92146eb..e2b7bdec0 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1741,6 +1741,19 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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, diff --git a/internal/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go index 7980d0273..337c2f6e0 100644 --- a/internal/api/handler_purchases_retry_redrive_test.go +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -42,6 +42,8 @@ const ( redriveUnknownExecID = "22222222-3333-4444-5555-666666666604" redriveForceExecID = "22222222-3333-4444-5555-666666666605" redriveMixedExecID = "22222222-3333-4444-5555-666666666606" + redriveNoRecsExecID = "22222222-3333-4444-5555-666666666607" + redriveNoRecsPlanID = "33333333-4444-5555-6666-777777777701" redriveLineageKey = "lineage-1668" ) @@ -234,6 +236,46 @@ func TestRetryOfLandedAzureSavingsPlanIsNotForceOverridable(t *testing.T) { 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 diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 7cb67eb29..5878c99c2 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3249,10 +3249,6 @@ func TestHandler_retryPurchase_PersistentFailure_BlocksWithOpsHint(t *testing.T) Status: "failed", Error: "FROM_EMAIL not configured for this deployment", CreatedByUserID: &creator, - // A safe-to-re-drive rec so the re-drive-safety gate (issue #1668), - // which runs ahead of the ops-hint gate and refuses a row carrying no - // recommendations at all, passes through to the gate under test. - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index ecf446be3..8c42c88a8 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -271,14 +271,21 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE // 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). +// +// The empty-recommendations condition lives here rather than in +// RedriveRefusalReason because it is not a statement about provider duplicate +// risk: a re-drive that purchases nothing cannot double-buy. It is this sweep's +// own "nothing to do, hand it to a human" condition, and the safe-fail path it +// selects is benign (the row is marked failed and surfaces in History). Folding +// it into the shared predicate would export it to the retry endpoint, where the +// consequence is the opposite of benign: a permanent refusal (issue #1668 CR). func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { - return RedriveRefusalReason(exec) == "" + 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 is safe to re-drive. It -// is the bool form above expressed as an explanation, so the two can never -// disagree. +// 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 @@ -288,12 +295,15 @@ func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { // landed Azure savings-plans row bought a second savings plan, which cannot be // canceled. // +// 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 { - if len(exec.Recommendations) == 0 { - return "this execution carries no recommendations, so what it did or did not purchase cannot be established" - } for i := range exec.Recommendations { if reason := recRedriveRefusalReason(exec.Recommendations[i]); reason != "" { return reason From c50f72f6b9750a8a37e143a037535a78249ef547 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 4 Aug 2026 00:12:45 +0200 Subject: [PATCH 3/4] refactor(purchase): scope #1713 to the backend gate, dedupe the policy prose Drops the frontend change out of this PR and applies three review notes. The frontend terminal-rendering change is reverted to main. The money risk is closed server-side: the retry endpoint returns a 409 and initiates no purchase, and that does not depend on any client honoring it. What remains is a Retry button that is always offered and always fails, which is a real defect but a presentation one, and fixing it properly needs the re-drive verdict on the History row projection (handler_history.go plus the frontend) rather than the in-place suppression this PR carried. That work is tracked in #1714 and does not belong in a p0 whose scope is stopping the spend. Three notes from review: - The redrive_unsafe detail now says at its write site that #1714 is its intended consumer, so the next reader does not have to wonder whether an unread field is dead. - allRecsSafeToRedrive's doc block no longer restates the per-provider policy that RedriveRefusalReason owns. It documents only what it adds, the sweep's own empty-recommendations condition and why that condition is deliberately not part of the shared predicate. The policy table moves to RedriveRefusalReason, and gains the unknown-provider case it had always implemented but never listed. - loadAndValidateRetryRequest is left at gocyclo 10. It measures 10 on origin/main too, this PR does not add to it, and five other functions in the same file sit at the same ceiling, so splitting only this one is an unrelated refactor. Its gates are a documented ordered sequence whose order is the security boundary, which is worth changing under its own review rather than inside a money-path fix. Behavior is unchanged from the previous head: the empty-recommendations narrowing stands, and the full Go suite, golangci-lint v2.10.1 and gocyclo are clean. Refs #1668 --- .../__tests__/history-retry-button.test.ts | 82 ------------------- frontend/src/api/types.ts | 8 +- frontend/src/history.ts | 31 ------- internal/api/handler_purchases.go | 13 ++- internal/purchase/manager.go | 63 +++++++------- 5 files changed, 45 insertions(+), 152 deletions(-) diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 8bde52fd5..d24d13392 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -465,88 +465,6 @@ describe('History inline Retry button (issue #47)', () => { expect(btn?.disabled).toBe(false); }); - // Issue #1668: a 409 carrying redrive_unsafe means the purchase can NEVER - // be retried, because the provider cannot tell a re-drive apart from a fresh - // purchase and a second attempt would buy a second commitment. Every other - // error branch describes something that can be fixed and retried, so those - // correctly leave the button live. Rendering this one the same way is what - // the p0 exists to prevent: the user reads "failed, try again", clicks - // again, and buys the duplicate the backend gate just refused. - test('redrive_unsafe refusal renders as terminal: no retry affordance left on the row (issue #1668)', async () => { - (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); - (confirmDialog as jest.Mock).mockResolvedValue(true); - const reason = 'Azure savings plans have no provider-side duplicate guard, so re-driving this purchase would buy a second savings plan that cannot be canceled'; - const refusal = Object.assign(new Error('this purchase cannot be retried safely: ' + reason), { - status: 409, - details: { ops_hint: reason, redrive_unsafe: true }, - }); - (api.retryPurchase as jest.Mock).mockRejectedValue(refusal); - (api.getHistory as jest.Mock).mockResolvedValue({ - summary: {}, - purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], - }); - console.error = jest.fn(); - - await loadHistory(); - const btn = document.querySelector('.history-retry-btn'); - btn?.click(); - await new Promise((r) => setTimeout(r, 10)); - - // The affordance is gone from the document, not merely disabled: a - // disabled button still reads as "temporarily unavailable". - expect(document.querySelector('.history-retry-btn')).toBeNull(); - - // Replaced by the same badge renderActionCell uses for rows that cannot - // be retried, carrying the provider's reason. - const badge = document.querySelector('.history-ops-hint'); - expect(badge).not.toBeNull(); - expect(badge?.textContent).toContain('second savings plan'); - - // The badge is built with textContent, so an untrusted reason cannot - // inject markup. - expect(badge?.querySelector('*')).toBeNull(); - - // And the toast says cannot, not failed. - expect(showToast).toHaveBeenCalledWith( - expect.objectContaining({ - kind: 'error', - message: expect.stringContaining('Cannot retry:'), - }), - ); - }); - - // The complement of the test above: an ordinary refusal must keep behaving - // as it did. ops_hint alone (operator-fixable) is not terminal -- the - // operator fixes the configuration and retries the same row. - test('ops_hint refusal without redrive_unsafe still leaves the Retry button in place', async () => { - (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); - (confirmDialog as jest.Mock).mockResolvedValue(true); - const refusal = Object.assign(new Error('this failure is operator-fixable'), { - status: 409, - details: { ops_hint: 'Set FROM_EMAIL tfvar then retry' }, - }); - (api.retryPurchase as jest.Mock).mockRejectedValue(refusal); - (api.getHistory as jest.Mock).mockResolvedValue({ - summary: {}, - purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], - }); - console.error = jest.fn(); - - await loadHistory(); - const btn = document.querySelector('.history-retry-btn'); - btn?.click(); - await new Promise((r) => setTimeout(r, 10)); - - expect(document.querySelector('.history-retry-btn')).not.toBeNull(); - expect(btn?.disabled).toBe(false); - expect(showToast).toHaveBeenCalledWith( - expect.objectContaining({ - kind: 'error', - message: expect.stringContaining('Failed to retry:'), - }), - ); - }); - test('admin WITHOUT Purchaser membership does not see Retry on rows they did not create (CR #924 F5)', async () => { // Issue #923 + CR #924 F5: retry-any:purchases is carved out of // admin:*. canRetryFailedRow must gate on diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index d8c5d37cf..459bad021 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -913,12 +913,8 @@ export interface ApiError extends Error { status?: number; // Structured detail fields the backend attaches to a 4xx response // alongside the human `error` message (e.g. `ops_hint`, - // `retry_attempt_n`, `threshold`, `retry_execution_id`, - // `redrive_unsafe`). Callers can branch on these without - // substring-matching the message. `redrive_unsafe: true` (issue #1668) - // marks a refusal as PERMANENT: unlike `ops_hint`, nothing an operator - // does makes the purchase retryable, so callers must not leave a retry - // affordance on screen. + // `retry_attempt_n`, `threshold`, `retry_execution_id`). Callers + // can branch on these without substring-matching the message. // See internal/api/handler.go for the flattening — keys are // promoted to the top level of the JSON body. details?: Record; diff --git a/frontend/src/history.ts b/frontend/src/history.ts index ac353de0f..2615de798 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -1330,12 +1330,10 @@ function wireRowActionHandlers(container: HTMLElement): void { } catch (retryError) { console.error('Failed to retry purchase:', retryError); // Surface structured retry hints from the backend (issue #47): - // * redrive_unsafe: permanent, this purchase can never be retried // * ops_hint — operator-actionable reason; takes priority // * retry_attempt_n + threshold — soft-block message // * else — fall back to the raw error message const err = retryError as Error & { details?: Record }; - const redriveUnsafe = err.details?.['redrive_unsafe'] === true; const opsHint = typeof err.details?.['ops_hint'] === 'string' ? err.details['ops_hint'] : ''; const retryAttemptN = typeof err.details?.['retry_attempt_n'] === 'number' ? err.details['retry_attempt_n'] : undefined; const threshold = typeof err.details?.['threshold'] === 'number' ? err.details['threshold'] : undefined; @@ -1346,35 +1344,6 @@ function wireRowActionHandlers(container: HTMLElement): void { detailMessage = `already retried ${retryAttemptN} times (threshold ${threshold}) — confirm the override prompt to force`; } const finalMessage = detailMessage || err.message || 'unknown error'; - // redrive_unsafe (issue #1668) is terminal, not a failed attempt: the - // provider offers no way to tell a re-drive apart from a fresh - // purchase, so clicking Retry again would buy a second commitment. - // Every other branch here describes something the user or an operator - // can act on and then retry, so leaving the button live is right for - // them and wrong for this one. Replace the button with the same - // ops-hint badge renderActionCell shows on rows that are not - // retryable, and do NOT re-enable; the whole point of the gate is - // that re-clicking must not be on offer. - // - // This covers the row in place. A later loadHistory() re-render brings - // the button back, because the History row projection does not yet - // carry the re-drive verdict (only the retry response does), so - // renderActionCell has nothing to gate on. Issue #1714 tracks moving - // the verdict onto the row so the button is never offered at all. The - // backend refuses either way; this is about not inviting the click. - if (redriveUnsafe) { - showToast({ message: `Cannot retry: ${finalMessage}`, kind: 'error', timeout: 8_000 }); - // Built as a DOM node rather than an HTML string: the reason is - // server-generated and interpolates the row's provider, so it is - // untrusted input. textContent removes the injection sink entirely - // instead of relying on an escape helper being applied correctly. - const badge = document.createElement('span'); - badge.className = 'history-ops-hint'; - badge.title = 'This purchase cannot be retried - retrying could buy a second commitment'; - badge.textContent = `⚠ ${finalMessage}`; - btn.replaceWith(badge); - return; - } showToast({ message: `Failed to retry: ${finalMessage}`, kind: 'error' }); btn.disabled = false; return; diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index e2b7bdec0..9a690e08e 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1758,9 +1758,16 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event 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; redrive_unsafe distinguishes this - // permanent refusal from the operator-fixable hints below, which - // do clear once the configuration is fixed. + // 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}) } diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 8c42c88a8..b221b6f97 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -247,38 +247,23 @@ 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. // -// Safe providers / services (issue #639): -// - AWS (all services): tag-guard or ClientToken deduplication (#636/#638). -// - Azure reservations (compute, relational-db, cache, nosql, memorydb, -// search, data-warehouse): DoIdempotentPurchaseTwoStep performs a -// tag-based lookup before purchasing (#729 / #721). -// - GCP compute (CUDs): server-side RequestId + deterministic name from -// the token (#654). +// 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). // -// NOT safe - safe-fail path preserved: -// - 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. -// -// 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). -// -// The empty-recommendations condition lives here rather than in -// RedriveRefusalReason because it is not a statement about provider duplicate -// risk: a re-drive that purchases nothing cannot double-buy. It is this sweep's -// own "nothing to do, hand it to a human" condition, and the safe-fail path it -// selects is benign (the row is marked failed and surfaces in History). Folding -// it into the shared predicate would export it to the retry endpoint, where the -// consequence is the opposite of benign: a permanent refusal (issue #1668 CR). +// 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) == "" } @@ -295,6 +280,24 @@ func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { // 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). +// - Azure reservations (compute, relational-db, cache, nosql, memorydb, +// search, data-warehouse): DoIdempotentPurchaseTwoStep performs a +// tag-based lookup before purchasing (#729 / #721). +// - GCP compute (CUDs): server-side RequestId + deterministic name from +// the token (#654). +// +// 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). +// // 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 From f47edb49887fc403a935f305de2cbfe6d49c0c0c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 4 Aug 2026 18:48:39 +0200 Subject: [PATCH 4/4] test(purchase): pin re-drive guard reach to Azure dispatch reach (#1668) A review finding proposed normalizing rec.Service before the Azure savings-plans re-drive guard, since exact matching lets variants such as "SavingsPlans" or "savings_plans" past it. They do get past the guard, and they cannot reach a purchase, which is the property that decides whether money moves. mapServiceType is the only step between rec.Service and the provider's service client, and Azure routes exactly one ServiceType to its savings-plans client: common.ServiceSavingsPlansAll, matched exactly in newServiceClientForSubscription, which normalizes nothing and constructs NewSavingsPlansClient nowhere else. Everything else lands in default and returns "unsupported service" without purchasing. So the set of values that can reach an Azure savings-plans purchase is exactly {"savingsplans", "savings-plans"}, which is exactly the set the guard refuses. Normalizing before the guard would widen what the guard treats as a savings plan while the dispatch axis stayed exact, creating a second normalization axis to keep in lockstep with the first forever. Two axes of one guard drifting apart is a shape this repo has already been bitten by, so the guard stays an exact match against the canonical constant and the coupling gets a test instead. The test asserts the biconditional over every key of both slug maps, the literal value of all 20 ServiceType constants, and case, separator, whitespace and near-miss mutations: a value dispatches to the Azure savings-plans client if and only if the guard refuses it. 61 cases, no gaps. Stated a second time as a closed set, so widening either axis shows up as a diff to an explicit list and not only as a loop failure. Confirmed non-vacuous: dropping the "savings-plans" spelling from the guard fails both tests, naming the resulting hole. Refs #1668 --- internal/purchase/redrive_guard_reach_test.go | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 internal/purchase/redrive_guard_reach_test.go 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) + } +}