diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 70f9e7085..e5f8c098f 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -407,11 +407,12 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma // inputs, so this is safe to call unconditionally. if exec.Status == "failed" { row.OpsHint = purchase.ResolveOpsHint(exec.Error) - // A re-drive the backend will always refuse is permanent, so it wins + // A retry the backend will always refuse is permanent, so it wins // over the operator-fixable failure-reason hint (which promises that // fixing the config makes Retry work). Same call and input as the - // retry handler's gate so the two cannot disagree (issue #161). - if reason := purchase.RedriveRefusalReason(&exec); reason != "" { + // retry handler's gate so the two cannot disagree (issue #161); a + // proven-unsent first attempt keeps Retry (issue #1803). + if reason := purchase.RetryRefusalReason(&exec); reason != "" { row.RedriveUnsafe = true // Only a proven pre-send AWS refusal on every rec means nothing was // bought; for any other outcome the duplicate-purchase warning stays. diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 6b6bcf753..bf928e21d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2428,8 +2428,9 @@ const retryThreshold = 5 // - failedExec.Status must be "failed" → 409 otherwise. // - a plan-step row (PlanID set) must carry at least one recommendation // → 409 otherwise, nothing to buy (platform#609). -// - every rec must be safe to re-drive per purchase.RedriveRefusalReason -// → 409 with ops_hint + redrive_unsafe when any is not, NOT +// - every rec must be safe to re-drive per purchase.RetryRefusalReason +// (a proven-unsent attempt-0 EC2 or Azure savings-plan row is lifted, +// #1803) → 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). @@ -2603,15 +2604,17 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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 AWS EC2 (no - // client token; a lost response is reported as outcome unknown), Azure - // savings-plans (no server-side idempotency key, timestamp-derived order - // alias) plus any provider the predicate does not recognize. A failed EC2 - // row therefore cannot be retried; the operator checks the account and - // starts a new purchase. Redshift is gated by checkRedshiftRetryGate. + // potentially a re-drive. purchase.RetryRefusalReason is the reaper's + // RedriveRefusalReason minus the EC2 and Azure savings-plans refusals for a + // row proven to have sent nothing (no_submission at attempt 0). The + // remaining reasons exist exactly when the provider offers nothing that + // would collapse the second attempt onto the first: AWS EC2 (no client + // token; a lost response is reported as outcome unknown), Azure + // savings-plans (no server-side idempotency key, no tag-based lookup) plus + // any provider the predicate does not recognize. A failed EC2 or Azure + // savings-plans row that may have sent therefore cannot be retried; the + // operator checks the account and starts a new purchase. Redshift is gated + // by checkRedshiftRetryGate. // If plan steps ever carry recommendations (platform#609), a failed EC2 // step will be un-retryable too and needs its own recovery path. // @@ -2625,7 +2628,7 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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, so RedriveRefusalReason stays silent on the empty case. The + // double-buy, so the predicate stays silent on the empty case. The // rec-less plan steps that createPurchaseExecutionsTx (handler_plans.go) and // getOrCreateExecution (purchase/notifications.go) used to create are // refused earlier in this handler instead (platform#609), because a retry of @@ -2633,7 +2636,7 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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 != "" { + if reason := purchase.RetryRefusalReason(failedExec); reason != "" { return NewClientErrorWithDetails(409, "this purchase cannot be retried safely: "+reason, // ops_hint reuses the key the History UI already renders in @@ -2642,11 +2645,10 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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. + // History also sets redrive_unsafe on the row (handler_history.go) + // to hide Retry. 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}) } @@ -2761,15 +2763,15 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // against the identical lineage key, which is the precondition for the // provider-side dedupe to engage at all. // - // Whether it then DOES engage is per-provider and not universal: AWS (all - // services), Azure reservations and GCP CUDs reproduce the token and - // dedupe, but Azure savings-plans has no server-side idempotency key and - // names its order alias from time.Now().UnixNano(), so a re-drive of a - // landed Azure SP order still duplicates it. 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. + // Whether it then DOES engage is per-provider and not universal: most AWS + // services, Azure reservations and GCP CUDs reproduce the token and + // dedupe, but AWS EC2 takes no client token and Azure savings-plans has no + // tag-based lookup, so a re-drive of a landed order of either may still + // duplicate it. Scope propagation is therefore necessary for dedupe + // everywhere and sufficient nowhere for those two. + // purchase.RetryRefusalReason gates this handler on that exclusion + // (checkRetryEligibilityGates, issue #1668) and lifts it only for a + // proven-unsent first attempt, which cannot have landed. // // Copied by value rather than by pointer so the successor and the // historical failed row never share a *string, matching the defensive @@ -2816,8 +2818,9 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // Order matters: the FK on retry_execution_id requires the successor to // exist before the original can point at it. // -// The linkage write is an atomic CAS (LinkRetryExecutionAtomic, conditional -// on status='failed' AND retry_execution_id IS NULL), not a full-row +// The linkage write is an atomic CAS (LinkUnsentRetryExecutionAtomic when the +// no-guard refusal was lifted by the proven-unsent proof, LinkRetryExecutionAtomic +// otherwise; conditional on status='failed' AND retry_execution_id IS NULL), not a full-row // upsert of a stale in-memory copy of the failed row. This is the // authoritative, race-safe guard: the pre-tx already-retried check in // loadAndValidateRetryRequest is only a fast-path optimization, since two @@ -2880,7 +2883,7 @@ func (h *Handler) writeRetrySuccessorTx(ctx context.Context, tx pgx.Tx, failedEx if err := h.config.SavePurchaseExecutionTx(ctx, tx, newExecution); err != nil { return err } - linked, linkErr := h.config.LinkRetryExecutionAtomic(ctx, tx, failedExec.ExecutionID, newExecution.ExecutionID) + linked, linkErr := h.linkRetrySuccessor(ctx, tx, failedExec, newExecution.ExecutionID) if linkErr != nil { return linkErr } @@ -2898,6 +2901,16 @@ func (h *Handler) writeRetrySuccessorTx(ctx context.Context, tx pgx.Tx, failedEx return nil } +// linkRetrySuccessor links the failed row to its successor. A row retryable +// only on its no-submission proof links through the CAS that re-checks the +// proof, so a row that stopped being one after the gate read it is not linked. +func (h *Handler) linkRetrySuccessor(ctx context.Context, tx pgx.Tx, failedExec *config.PurchaseExecution, successorID string) (bool, error) { + if purchase.RedriveRefusalReason(failedExec) != "" && purchase.RetryRefusalReason(failedExec) == "" { + return h.config.LinkUnsentRetryExecutionAtomic(ctx, tx, failedExec.ExecutionID, successorID) + } + return h.config.LinkRetryExecutionAtomic(ctx, tx, failedExec.ExecutionID, successorID) +} + // checkRetryDuplicateSubmit returns a *retryDuplicateSubmitError when recent // (the creator's live web submits, read under the submit lock) already holds // an execution with the successor's submit fingerprint. The successor's own @@ -2927,7 +2940,7 @@ func (e *retryDuplicateSubmitError) Error() string { // errRetryLinkRaceLost is returned from persistRetrySuccessorTx's tx // closure to force a rollback (via WithTx's error path) when -// LinkRetryExecutionAtomic matches zero rows -- a concurrent retry already +// the link CAS matches zero rows -- a concurrent retry already // claimed the failed row between the pre-tx already-retried check and this // tx's UPDATE. Never returned to a caller directly; persistRetrySuccessorTx // translates it into a 409 ClientError. diff --git a/internal/api/handler_purchases_retry_unsent_test.go b/internal/api/handler_purchases_retry_unsent_test.go new file mode 100644 index 000000000..56ea86592 --- /dev/null +++ b/internal/api/handler_purchases_retry_unsent_test.go @@ -0,0 +1,122 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #1803: a failed EC2 or Azure savings-plans row that is a proven-unsent +// first attempt (no_submission at attempt 0) cannot have bought anything, so +// Retry is allowed and links through the CAS that re-checks that proof. + +const redriveUnsentExecID = "22222222-3333-4444-5555-666666666609" + +func unsentRow(provider, service string) *config.PurchaseExecution { + row := redriveFailedRow(redriveUnsentExecID, provider, service) + row.NoSubmission = true + return row +} + +// retryWithLinkMocks runs the real retry handler with the successor insert +// mocked and returns the store so the caller registers the link outcome first. +func retryWithLinkMocks(t *testing.T, failed *config.PurchaseExecution) (*Handler, *MockConfigStore) { + t.Helper() + session := &Session{UserID: retryCallerID, Email: "operator@example.com"} + handler, mockConfig, _ := buildSessionRetryHandler(t, failed, session, false, true) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil).Maybe() + return handler, mockConfig +} + +func TestRetryOfProvenUnsentRowLinksViaUnsentCAS(t *testing.T) { + for _, tc := range []struct{ name, provider, service string }{ + {"aws ec2", "aws", "ec2"}, + {"azure savings plans", "azure", "savingsplans"}, + } { + t.Run(tc.name, func(t *testing.T) { + failed := unsentRow(tc.provider, tc.service) + handler, store := retryWithLinkMocks(t, failed) + store.On("LinkUnsentRetryExecutionAtomic", mock.Anything, mock.Anything, failed.ExecutionID, mock.AnythingOfType("string")).Return(true, nil).Once() + + result, err := handler.retryPurchase(context.Background(), sessionRetryReq(), failed.ExecutionID) + + require.NoError(t, err, "a proven-unsent first attempt must be retryable") + assert.NotEmpty(t, result.(map[string]any)["execution_id"]) + store.AssertNotCalled(t, "LinkRetryExecutionAtomic") + }) + } +} + +func TestRetryOfProvenUnsentRowLosingUnsentLinkIs409(t *testing.T) { + failed := unsentRow("aws", "ec2") + handler, store := retryWithLinkMocks(t, failed) + store.On("LinkUnsentRetryExecutionAtomic", mock.Anything, mock.Anything, failed.ExecutionID, mock.AnythingOfType("string")).Return(false, nil).Once() + + result, err := handler.retryPurchase(context.Background(), sessionRetryReq(), failed.ExecutionID) + + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + assert.Contains(t, err.Error(), "already retried or settled") + store.AssertNotCalled(t, "CreateSuppressionTx") +} + +// The proof is per row: a submitted EC2 row and a no_submission successor +// (attempt 1) stay refused, whatever the flag says about later attempts. +func TestRetryOfEC2RowWithoutAttemptZeroProofIsRefused(t *testing.T) { + submitted := redriveFailedRow(redriveUnsentExecID, "aws", "ec2") + attempt1 := unsentRow("aws", "ec2") + attempt1.RetryAttemptN = 1 + for name, failed := range map[string]*config.PurchaseExecution{"submitted": submitted, "no_submission attempt 1": attempt1} { + t.Run(name, func(t *testing.T) { + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + assert.Empty(t, tokens) + assertRedriveRefused(t, err, "Check EC2 Reserved Instances in the target account") + }) + } +} + +// Rows that were never refused keep the plain CAS and never touch the new one. +func TestRetryOfRDSRowStillLinksViaPlainCAS(t *testing.T) { + failed := unsentRow("aws", "rds") + handler, store := retryWithLinkMocks(t, failed) + store.On("LinkRetryExecutionAtomic", mock.Anything, mock.Anything, failed.ExecutionID, mock.AnythingOfType("string")).Return(true, nil).Once() + + _, err := handler.retryPurchase(context.Background(), sessionRetryReq(), failed.ExecutionID) + + require.NoError(t, err) + store.AssertNotCalled(t, "LinkUnsentRetryExecutionAtomic") +} + +func TestExecutionToHistoryRow_RetryHintForUnsentRows(t *testing.T) { + for _, tc := range []struct { + name string + provider string + service string + unsent bool + wantUnsafe bool + }{ + {"unsent ec2 shows Retry", "aws", "ec2", true, false}, + {"unsent azure savings plans shows Retry", "azure", "savingsplans", true, false}, + {"submitted ec2 is not retryable", "aws", "ec2", false, true}, + {"submitted azure savings plans is not retryable", "azure", "savingsplans", false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + exec := redriveFailedRow(redriveUnsentExecID, tc.provider, tc.service) + exec.Error = "boom" + exec.NoSubmission = tc.unsent + + row := executionToHistoryRow(*exec, "", "") + + assert.Equal(t, tc.wantUnsafe, row.RedriveUnsafe) + assert.Equal(t, tc.wantUnsafe, row.OpsHint != "", "ops_hint is what hides the Retry button") + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 9dde67325..e754e7c26 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1471,7 +1471,9 @@ paths: known; a concurrent retry also answers this way), it is a plan step with no recommendations, it belongs to a settled ladder tranche, a provider re-drive of it is unsafe (ops_hint plus - redrive_unsafe, permanent and not overridable by force), it is a + redrive_unsafe, permanent and not overridable by force; an AWS + EC2 or Azure savings-plans row proven to have sent nothing, with + no_submission at attempt 0, is retryable), it is a Redshift purchase that failed too recently for AWS to have settled (retry_after_seconds and a Retry-After header, not overridable by force), its failure is operator-fixable (ops_hint plus @@ -7731,7 +7733,9 @@ components: description: >- Execution-only. True when ops_hint is a permanent re-drive refusal, including a Redshift purchase with no first dispatch time or one - dispatched at least 24h ago. + dispatched at least 24h ago. False for an AWS EC2 or Azure + savings-plans row proven to have sent nothing (no_submission at + attempt 0), which stays retryable. is_audit_gap: type: boolean description: >- diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 8325ea424..4570c52f5 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -313,6 +313,10 @@ type StoreInterface interface { // DB error. Must be called inside the same tx that inserts the // successor row, after that insert. LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID string) (linked bool, err error) + // LinkUnsentRetryExecutionAtomic is LinkRetryExecutionAtomic with the extra + // condition no_submission AND retry_attempt_n = 0 (a proven-unsent first + // attempt). The handler uses it for retries it allowed only on that proof. + LinkUnsentRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID string) (linked bool, err error) // LockLadderTrancheForRetryTx row-locks the ladder tranche whose id equals // idempotencyKey (matched as text; found=false for any non-ladder key) so // Retry can refuse a settled tranche before inserting a successor. @@ -821,6 +825,10 @@ type StoreInterface interface { // whether it is within horizon on the database clock. StampFirstDispatchTx(ctx context.Context, tx pgx.Tx, executionID string, horizon time.Duration) (StampResult, error) + // PredecessorProvenUnsent reports whether executionID's retry predecessor + // is a no_submission attempt 0 row (the lineage provably sent nothing). + PredecessorProvenUnsent(ctx context.Context, executionID string) (bool, error) + // LockRedshiftTuplesTx takes a transaction-scoped advisory lock per // distinct tuple, in sorted order, serializing dispatches that share one. LockRedshiftTuplesTx(ctx context.Context, tx pgx.Tx, tuples []RedshiftTuple) error diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 6e13cb0d4..721721c42 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1290,6 +1290,19 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, // successor row, after that insert, so the FK on retry_execution_id is // satisfied. func (s *PostgresStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID string) (linked bool, err error) { + return s.linkRetryExecution(ctx, tx, executionID, retryExecutionID, "") +} + +// LinkUnsentRetryExecutionAtomic is LinkRetryExecutionAtomic for a retry the +// handler allowed only because the row is a proven-unsent first attempt: the +// same conditional UPDATE plus no_submission AND retry_attempt_n = 0, so a row +// that stopped being that proof between the gate and the link is not linked. +func (s *PostgresStore) LinkUnsentRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID string) (linked bool, err error) { + return s.linkRetryExecution(ctx, tx, executionID, retryExecutionID, "AND no_submission AND retry_attempt_n = 0") +} + +// linkRetryExecution is the shared link CAS; extraPredicate narrows it. +func (s *PostgresStore) linkRetryExecution(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID, extraPredicate string) (bool, error) { // Before the settlement migration no row can be settled (#999). notSettled := "" if ready, readyErr := settlementSchemaReady(ctx, tx); readyErr != nil { @@ -1304,7 +1317,8 @@ func (s *PostgresStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, WHERE execution_id = $1 AND status = 'failed' AND retry_execution_id IS NULL - ` + notSettled + ` + notSettled + ` + ` + extraPredicate tag, err := tx.Exec(ctx, q, executionID, retryExecutionID) if err != nil { return false, fmt.Errorf("failed to link retry execution: %w", err) diff --git a/internal/config/store_postgres_first_dispatch.go b/internal/config/store_postgres_first_dispatch.go index ca8de1ca8..a8b29ef07 100644 --- a/internal/config/store_postgres_first_dispatch.go +++ b/internal/config/store_postgres_first_dispatch.go @@ -17,6 +17,26 @@ type StampResult struct { WithinHorizon bool } +// provenUnsentPredecessorSelect selects a no_submission attempt 0 row whose +// retry_execution_id equals the expression appended to it. The status is +// checked explicitly because only the upsert's update path ties NS to failed. +// Shared by the stamp mint rule and PredecessorProvenUnsent so the two cannot +// drift. +const provenUnsentPredecessorSelect = ` + SELECT 1 FROM purchase_executions p + WHERE p.no_submission AND p.retry_attempt_n = 0 AND p.status = 'failed' + AND p.retry_execution_id = ` + +// PredecessorProvenUnsent reports whether executionID is the retry successor +// of a no_submission attempt 0 row, i.e. the whole lineage never submitted. +func (s *PostgresStore) PredecessorProvenUnsent(ctx context.Context, executionID string) (bool, error) { + var ok bool + if err := s.db.QueryRow(ctx, `SELECT EXISTS (`+provenUnsentPredecessorSelect+`$1)`, executionID).Scan(&ok); err != nil { + return false, fmt.Errorf("check predecessor of execution %s was never submitted: %w", executionID, err) + } + return ok, nil +} + // StampFirstDispatchTx stamps a lineage's first dispatch time on the claimed // running row, with the database clock, and returns it. An existing stamp is // kept. A stamp is minted only at attempt 0, or when the predecessor is a @@ -31,11 +51,8 @@ func (s *PostgresStore) StampFirstDispatchTx(ctx context.Context, tx pgx.Tx, exe err := tx.QueryRow(ctx, ` UPDATE purchase_executions SET first_dispatch_at = COALESCE(first_dispatch_at, CASE - WHEN retry_attempt_n = 0 OR EXISTS ( - SELECT 1 FROM purchase_executions p - WHERE p.retry_execution_id = purchase_executions.execution_id - AND p.no_submission AND p.retry_attempt_n = 0 - ) THEN now() END) + WHEN retry_attempt_n = 0 OR EXISTS (`+provenUnsentPredecessorSelect+`purchase_executions.execution_id) + THEN now() END) WHERE execution_id = $1 AND status = 'running' RETURNING first_dispatch_at, first_dispatch_at > now() - make_interval(secs => $2)`, executionID, horizon.Seconds()).Scan(&res.At, &within) diff --git a/internal/config/store_postgres_first_dispatch_integration_test.go b/internal/config/store_postgres_first_dispatch_integration_test.go index 46906de3b..320225ed7 100644 --- a/internal/config/store_postgres_first_dispatch_integration_test.go +++ b/internal/config/store_postgres_first_dispatch_integration_test.go @@ -171,3 +171,39 @@ func TestStampFirstDispatch_ExactHorizonIsOutside(t *testing.T) { require.NotNil(t, res.At) assert.False(t, res.WithinHorizon) } + +func TestPredecessorProvenUnsent(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + cases := []struct { + name string + attemptN int + ns bool + seed bool + status string // overrides the seeded predecessor's failed status + want bool + }{ + {"no_submission attempt 0", 0, true, true, "", true}, + {"no_submission attempt 1", 1, true, true, "", false}, + {"submitted attempt 0", 0, false, true, "", false}, + {"no_submission attempt 0 but running", 0, true, true, "running", false}, + // Each case seeds its own successor, so this one has no predecessor row at all. + {"no predecessor", 0, false, false, "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + succ := seedRunningSuccessor(ctx, t, store, tc.attemptN+1, nil) + if tc.seed { + seedPredecessor(ctx, t, store, succ.ExecutionID, tc.attemptN, tc.ns) + if tc.status != "" { + _, err := store.db.Exec(ctx, `UPDATE purchase_executions SET status = $2 WHERE retry_execution_id = $1`, succ.ExecutionID, tc.status) + require.NoError(t, err) + } + } + got, err := store.PredecessorProvenUnsent(ctx, succ.ExecutionID) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/internal/config/store_postgres_link_unsent_integration_test.go b/internal/config/store_postgres_link_unsent_integration_test.go new file mode 100644 index 000000000..660974c33 --- /dev/null +++ b/internal/config/store_postgres_link_unsent_integration_test.go @@ -0,0 +1,70 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" +) + +// linkUnsent inserts a pending successor and tries the proven-unsent link for +// predecessor in the same tx, as the Retry handler does. +func (f *rampFixture) linkUnsent(ctx context.Context, t *testing.T, predecessor *PurchaseExecution) bool { + t.Helper() + successor := &PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: f.planID, Status: "pending", StepNumber: predecessor.StepNumber} + var linked bool + require.NoError(t, f.store.WithTx(ctx, func(tx pgx.Tx) error { + require.NoError(t, f.store.SavePurchaseExecutionTx(ctx, tx, successor)) + var err error + linked, err = f.store.LinkUnsentRetryExecutionAtomic(ctx, tx, predecessor.ExecutionID, successor.ExecutionID) + return err + })) + return linked +} + +func TestLinkUnsentRetryExecutionAtomic(t *testing.T) { + ctx := context.Background() + + t.Run("links a no_submission attempt-0 failed row", func(t *testing.T) { + f := managedSettleFixture(ctx, t) + row := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, true) + require.True(t, f.linkUnsent(ctx, t, row)) + got, err := f.store.GetExecutionByID(ctx, row.ExecutionID) + require.NoError(t, err) + require.NotNil(t, got.RetryExecutionID) + }) + + t.Run("refuses a row that is not no_submission", func(t *testing.T) { + f := managedSettleFixture(ctx, t) + row := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, false) + require.False(t, f.linkUnsent(ctx, t, row)) + }) + + t.Run("refuses a no_submission row past attempt 0", func(t *testing.T) { + f := managedSettleFixture(ctx, t) + row := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, true) + _, err := f.store.db.Exec(ctx, `UPDATE purchase_executions SET retry_attempt_n = 1 WHERE execution_id = $1`, row.ExecutionID) + require.NoError(t, err) + require.False(t, f.linkUnsent(ctx, t, row)) + }) + + t.Run("refuses an already linked row", func(t *testing.T) { + f := managedSettleFixture(ctx, t) + row := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, true) + require.True(t, f.linkUnsent(ctx, t, row)) + require.False(t, f.linkUnsent(ctx, t, row)) + }) + + t.Run("refuses a settled row", func(t *testing.T) { + f := managedSettleFixture(ctx, t) + row := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, true) + _, err := f.settle(ctx, row.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.False(t, f.linkUnsent(ctx, t, row)) + }) +} diff --git a/internal/config/store_postgres_ramp_step.go b/internal/config/store_postgres_ramp_step.go index a28428187..b6aaf2f9d 100644 --- a/internal/config/store_postgres_ramp_step.go +++ b/internal/config/store_postgres_ramp_step.go @@ -25,8 +25,8 @@ import ( // // That intersection is the gate's release valve, and it has to exist. An // account can become permanently unable to buy (revoked credentials, closed -// account, an Azure savings-plans row that purchase.RedriveRefusalReason -// refuses to retry by design), and a failed row cannot be canceled either -- +// account, an EC2 or Azure savings-plans row that +// purchase.RetryRefusalReason refuses to retry by design), and a failed row cannot be canceled either -- // IsCancelable and CancelExecutionAtomic both admit only pending/notified. // Without the attachment test such a unit would hold its step open // forever and the plan would never buy the rest of its ramp: the exact diff --git a/internal/config/types.go b/internal/config/types.go index 7f964cccd..91d39376a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -1228,7 +1228,7 @@ type PurchaseHistoryRecord struct { // time so updates to the persistent-failure map land instantly). OpsHint string `json:"ops_hint,omitempty" dynamodbav:"-"` // RedriveUnsafe is true when OpsHint is a permanent refusal from - // purchase.RedriveRefusalReason (provider has no duplicate guard), not an + // purchase.RetryRefusalReason (provider has no duplicate guard), not an // operator-fixable misconfiguration. Computed at read time, like OpsHint. RedriveUnsafe bool `json:"redrive_unsafe,omitempty" dynamodbav:"-"` // IsAuditGap marks a synthesized "completed" row whose purchase_history diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 8457c911e..f1c2563f8 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -569,6 +569,13 @@ func (m *MockConfigStore) StampFirstDispatchTx(ctx context.Context, tx pgx.Tx, e return res, args.Error(1) } +// PredecessorProvenUnsent mocks the predecessor proof lookup (no default: testify panics if unexpected). +func (m *MockConfigStore) PredecessorProvenUnsent(ctx context.Context, executionID string) (bool, error) { + m.record("PredecessorProvenUnsent", ctx, executionID) + args := m.Called(ctx, executionID) + return args.Bool(0), args.Error(1) +} + // LockRedshiftTuplesTx mocks the Redshift tuple advisory locks (no default: testify panics if unexpected). func (m *MockConfigStore) LockRedshiftTuplesTx(ctx context.Context, tx pgx.Tx, tuples []config.RedshiftTuple) error { m.record("LockRedshiftTuplesTx", ctx, tx, tuples) @@ -724,6 +731,13 @@ func (m *MockConfigStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.T return args.Bool(0), args.Error(1) } +// LinkUnsentRetryExecutionAtomic mocks the proven-unsent link CAS (no default: testify panics if unexpected). +func (m *MockConfigStore) LinkUnsentRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID string) (bool, error) { + m.record("LinkUnsentRetryExecutionAtomic", ctx, tx, executionID, retryExecutionID) + args := m.Called(ctx, tx, executionID, retryExecutionID) + return args.Bool(0), args.Error(1) +} + // LockLadderTrancheForRetryTx mocks the operation. Defaults to (not a ladder // execution) when no expectation is registered. func (m *MockConfigStore) LockLadderTrancheForRetryTx(ctx context.Context, tx pgx.Tx, idempotencyKey string) (ladder.TrancheStatus, bool, error) { diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index 0b5a91d93..df2d12aeb 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -86,6 +86,18 @@ func armedExecution(provider, service string, retryAttemptN int, status string) // the provider's GetServiceClient is asked for. func armedHarness(t *testing.T, serviceType common.ServiceType) (*Manager, *MockConfigStore, *armedPurchaseRecorder) { t.Helper() + return armedHarnessWithProof(t, serviceType, predecessorProof{}) +} + +// predecessorProof is the answer the harness gives PredecessorProvenUnsent; +// the zero value is "not proven, no error". +type predecessorProof struct { + proven bool + err error +} + +func armedHarnessWithProof(t *testing.T, serviceType common.ServiceType, proof predecessorProof) (*Manager, *MockConfigStore, *armedPurchaseRecorder) { + t.Helper() rec := &armedPurchaseRecorder{} store := new(MockConfigStore) @@ -110,6 +122,7 @@ func armedHarness(t *testing.T, serviceType common.ServiceType) (*Manager, *Mock stampedAt := time.Now().UTC() store.On("StampFirstDispatchTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(config.StampResult{At: &stampedAt, WithinHorizon: true}, nil).Maybe() + store.On("PredecessorProvenUnsent", mock.Anything, mock.Anything).Return(proof.proven, proof.err).Maybe() store.On("LockRedshiftTuplesTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() store.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil).Maybe() diff --git a/internal/purchase/armed_redrive_unsent_test.go b/internal/purchase/armed_redrive_unsent_test.go new file mode 100644 index 000000000..9bb11de53 --- /dev/null +++ b/internal/purchase/armed_redrive_unsent_test.go @@ -0,0 +1,122 @@ +package purchase + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// unprovenPredecessorStore is a strict store whose only answer is "predecessor +// not proven unsent"; any other call panics. +func unprovenPredecessorStore(t *testing.T) *MockConfigStore { + t.Helper() + store := new(MockConfigStore) + store.On("PredecessorProvenUnsent", mock.Anything, mock.Anything).Return(false, nil).Maybe() + return store +} + +func armedRefusalText(t *testing.T, m *Manager, exec *config.PurchaseExecution) string { + t.Helper() + err := m.armedRedriveRefusal(context.Background(), exec) + require.Error(t, err) + return err.Error() +} + +// runArmedRetry claims and executes a pending retry successor and returns the +// provider call count and the last saved row. +func runArmedRetry(t *testing.T, exec *config.PurchaseExecution, svcType common.ServiceType, proof predecessorProof) (int, config.PurchaseExecution, *MockConfigStore) { + t.Helper() + mgr, store, rec := armedHarnessWithProof(t, svcType, proof) + var last config.PurchaseExecution + store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + last = *e + return nil + } + if exec.Recommendations[0].Provider == "azure" { + // The stale-pricing check runs before every Azure dispatch; serve the row's own price. + exec.Recommendations[0].ID = "current-azure" + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}). + Return(exec.Recommendations, nil).Maybe() + } + expectClaim(store, exec, []string{"approved", "pending", "notified"}) + _, _ = mgr.claimAndExecuteWithin(context.Background(), exec, 0) + return rec.count(), last, store +} + +func TestArmedRetryOfProvenUnsentFirstAttemptExecutes(t *testing.T) { + cases := []struct { + name, provider, service string + svcType common.ServiceType + }{ + {"aws ec2", "aws", "ec2", common.ServiceEC2}, + {"azure savingsplans", "azure", "savingsplans", common.ServiceSavingsPlansAll}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exec := armedExecution(tc.provider, tc.service, 1, "notified") + calls, last, _ := runArmedRetry(t, exec, tc.svcType, predecessorProof{proven: true}) + assert.Equal(t, 1, calls, "a retry of a proven-unsent first attempt buys exactly once") + assert.NotContains(t, last.Error, "refusing to execute") + }) + } +} + +func TestArmedRetryRefusedWithoutProof(t *testing.T) { + cases := []struct { + name, provider, service string + svcType common.ServiceType + attempt int + proven bool + lookupErr error + wantText string + wantLookups int + }{ + {"ec2 unproven", "aws", "ec2", common.ServiceEC2, 1, false, nil, "Check EC2 Reserved Instances", 1}, + {"azure sp unproven", "azure", "savingsplans", common.ServiceSavingsPlansAll, 1, false, nil, "second savings plan", 1}, + {"ec2 attempt 2 even if proven", "aws", "ec2", common.ServiceEC2, 2, true, nil, "Check EC2 Reserved Instances", 0}, + {"azure sp attempt 2 even if proven", "azure", "savingsplans", common.ServiceSavingsPlansAll, 2, true, nil, "second savings plan", 0}, + {"ec2 lookup error", "aws", "ec2", common.ServiceEC2, 1, true, errors.New("db down"), "start a new purchase", 1}, + {"azure sp lookup error", "azure", "savingsplans", common.ServiceSavingsPlansAll, 1, true, errors.New("db down"), "start a new purchase", 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exec := armedExecution(tc.provider, tc.service, tc.attempt, "notified") + calls, last, store := runArmedRetry(t, exec, tc.svcType, predecessorProof{proven: tc.proven, err: tc.lookupErr}) + assert.Equal(t, 0, calls, "a refused retry must make zero provider calls") + assert.Equal(t, "failed", last.Status) + assert.False(t, last.NoSubmission, "an armed refusal must not stamp no_submission") + assert.Contains(t, last.Error, tc.wantText) + assert.NotContains(t, last.Error, "db down", "the stored error must not carry the DB error") + if tc.wantLookups == 0 { + store.AssertNotCalled(t, "PredecessorProvenUnsent", mock.Anything, mock.Anything) + } + }) + } +} + +// Attempt-0 rows and retries with no refusal never consult the store. +func TestArmedRedriveRefusalDoesNotLookUpUnlessNeeded(t *testing.T) { + cases := []struct { + name, provider, service string + svcType common.ServiceType + attempt int + }{ + {"ec2 attempt 0", "aws", "ec2", common.ServiceEC2, 0}, + {"azure sp attempt 0", "azure", "savingsplans", common.ServiceSavingsPlansAll, 0}, + {"rds attempt 1", "aws", "rds", common.ServiceRDS, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exec := armedExecution(tc.provider, tc.service, tc.attempt, "notified") + calls, _, store := runArmedRetry(t, exec, tc.svcType, predecessorProof{}) + assert.Equal(t, 1, calls) + store.AssertNotCalled(t, "PredecessorProvenUnsent", mock.Anything, mock.Anything) + }) + } +} diff --git a/internal/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go index 1f71de861..57aadd4cf 100644 --- a/internal/purchase/aws_no_token_redrive_test.go +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -93,7 +93,7 @@ func TestRedshiftRedriveGuardLifted(t *testing.T) { exec := &config.PurchaseExecution{RetryAttemptN: 1, Recommendations: []config.RecommendationRecord{rec}} assert.Empty(t, RedriveRefusalReason(exec)) - assert.NoError(t, armedRedriveRefusal(exec), "a Redshift retry successor is not refused by the armed backstop") + assert.NoError(t, (&Manager{}).armedRedriveRefusal(context.Background(), exec), "a Redshift retry successor is not refused by the armed backstop") } } } diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 4ff7e75c6..770ae7bbd 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -22,6 +22,7 @@ var serverNotePatterns = []string{ `reaped after \d+m in [a-z_]+ state \x{2014} executor did not complete(?:; safe to retry)?`, `execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck \(issue #632\)\. (?:Verify on the cloud provider that no commitment was created, then Retry\.|Retry is refused for this purchase because a repeat could buy a second commitment\. Verify on the cloud provider whether the commitment was created; if it was not, start a new purchase\.)`, `refusing to execute retry attempt \d+: (?:` + redriveReasonPattern + `)`, + regexp.QuoteMeta(armedRedriveLookupRefusal), `stale Azure recommendation pricing; refresh recommendations and submit a new purchase(?:: the latest collection state is missing, so Azure prices cannot be confirmed current)?`, `(?:execution ` + uuidPattern + `: )?plan step has no recommendations attached, so it cannot buy anything(?: \(platform#631: plan steps cannot yet carry recommendations\))?`, `plan step refused by commitment cap(?:: plan has no max_step_commitment set \(auto-purchase of steps with recommendations is refused until one is set\)|: step total commitment \d+\.\d{2} exceeds max_step_commitment \d+\.\d{2})?`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 7734efac9..a27c3c8d2 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -18,6 +18,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { retry := &config.PurchaseExecution{RetryAttemptN: 2, Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}}} azSP := &config.PurchaseExecution{RetryAttemptN: 1, Recommendations: []config.RecommendationRecord{{Provider: "azure", Service: "savingsplans"}}} gcpRetry := &config.PurchaseExecution{RetryAttemptN: 1, Recommendations: []config.RecommendationRecord{{Provider: "weird"}}} + armedMgr := &Manager{config: unprovenPredecessorStore(t)} notes := []string{ fresh.Error, recoveredAfterReapNote, @@ -26,10 +27,11 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { "reaped after 30m in approved state — executor did not complete; safe to retry", strandedRecoveryError(&config.PurchaseExecution{}), strandedRecoveryError(retry), - armedRedriveRefusal(retry).Error(), + armedRefusalText(t, armedMgr, retry), "refusing to execute retry attempt 2: " + redriveReasonAWSNoGuardLegacy, - armedRedriveRefusal(azSP).Error(), - armedRedriveRefusal(gcpRetry).Error(), + armedRefusalText(t, armedMgr, azSP), + armedRefusalText(t, armedMgr, gcpRetry), + armedRedriveLookupRefusal, ErrStaleAzurePricing.Error(), ErrPlanStepNoRecommendations.Error(), legacyPlanStepNoRecommendationsNote, diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 7fc82acf8..120856a54 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -449,11 +449,12 @@ const ( skippedAccountNoRetryNote = "the run ended before this account's purchase started, so nothing was submitted; this purchase type cannot be retried automatically, so start a new purchase for this account if it is still wanted" ) -// skippedAccountNote words the skip row by whether Retry accepts it: -// RedriveRefusalReason refuses EC2 and Azure savings plans whatever -// NoSubmission says, so those rows point at a new purchase instead. +// skippedAccountNote words the skip row by whether Retry accepts it. The row +// must already be failed and NoSubmission; it inherits the root's +// RetryAttemptN, so a retried root's skip rows stay refused and point at a +// new purchase instead. func skippedAccountNote(row *config.PurchaseExecution) string { - if RedriveRefusalReason(row) != "" { + if RetryRefusalReason(row) != "" { return skippedAccountNoRetryNote } return skippedAccountRetryNote diff --git a/internal/purchase/execution_skipped_accounts_test.go b/internal/purchase/execution_skipped_accounts_test.go index 73c9dc49e..c708d2196 100644 --- a/internal/purchase/execution_skipped_accounts_test.go +++ b/internal/purchase/execution_skipped_accounts_test.go @@ -30,6 +30,8 @@ type skipFixture struct { // cancelAtB moves the cancel from A's purchase to B's credential load, so B // starts and fails on its own (a started account, not a skip). cancelAtB bool + // rootAttempt is the root's RetryAttemptN, copied onto its skip rows. + rootAttempt int } const ( @@ -56,6 +58,7 @@ func (f *skipFixture) run(t *testing.T, ctx context.Context, cancel context.Canc Count: 1, Savings: 75, UpfrontCost: 300, Selected: true} exec := &config.PurchaseExecution{ ExecutionID: "exec-skip", PlanID: "plan-skip", Status: "pending", IdempotencyKey: "lineage-skip", + RetryAttemptN: f.rootAttempt, Recommendations: []config.RecommendationRecord{withAccount(rec, skipAcctA), withAccount(rec, skipAcctB), withAccount(rec, skipAcctC)}, } plan := &config.PurchasePlan{ID: "plan-skip", Name: "Skip", RampSchedule: config.RampSchedule{TotalSteps: 1}} @@ -140,7 +143,8 @@ func TestExecuteMultiAccount_RecordsAccountsSkippedAfterACommit(t *testing.T) { r := rows[0] assert.Equal(t, "failed", r.Status, id) assert.True(t, r.NoSubmission, "%s: the skip proves nothing was submitted", id) - assert.Equal(t, skippedAccountNoRetryNote, r.Error, "%s: Retry refuses EC2, so the row must not promise one", id) + assert.Equal(t, skippedAccountRetryNote, r.Error, "%s: an attempt-0 NoSubmission EC2 row is retryable", id) + assert.Empty(t, RetryRefusalReason(&r), id) assert.Equal(t, "lineage-skip:"+id, r.IdempotencyKey, "%s: the stable per-account key", id) assert.NotEqual(t, "exec-skip", r.ExecutionID, id) require.Len(t, r.Recommendations, 1, "%s: the row keeps only its own account's recs, so a retry buys exactly them", id) @@ -150,22 +154,45 @@ func TestExecuteMultiAccount_RecordsAccountsSkippedAfterACommit(t *testing.T) { } } +// A retried root's skip rows copy its RetryAttemptN, so Retry still refuses +// them and the note points at a new purchase. +func TestExecuteMultiAccount_SkipRowsOfRetriedRootStayRefused(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + f := &skipFixture{rootAttempt: 1} + require.Error(t, f.run(t, ctx, cancel, true)) + for _, id := range []string{skipAcctB, skipAcctC} { + rows := f.rowsFor(id) + require.Len(t, rows, 1, id) + assert.Equal(t, skippedAccountNoRetryNote, rows[0].Error, id) + assert.NotEmpty(t, RetryRefusalReason(&rows[0]), id) + } +} + // The skip row promises a retry only where Retry accepts the row. func TestSkippedAccountNote_MatchesRetryEligibility(t *testing.T) { cases := []struct { provider, service string + attempt int want string }{ - {"aws", "ec2", skippedAccountNoRetryNote}, - {"azure", "savingsplans", skippedAccountNoRetryNote}, - {"aws", "rds", skippedAccountRetryNote}, - {"aws", "redshift", skippedAccountRetryNote}, - {"gcp", "compute", skippedAccountRetryNote}, + {"aws", "ec2", 0, skippedAccountRetryNote}, + {"azure", "savingsplans", 0, skippedAccountRetryNote}, + {"aws", "ec2", 1, skippedAccountNoRetryNote}, + {"azure", "savingsplans", 1, skippedAccountNoRetryNote}, + {"aws", "rds", 0, skippedAccountRetryNote}, + {"aws", "rds", 1, skippedAccountRetryNote}, + {"aws", "redshift", 0, skippedAccountRetryNote}, + {"gcp", "compute", 0, skippedAccountRetryNote}, } for _, c := range cases { - row := &config.PurchaseExecution{Recommendations: []config.RecommendationRecord{{Provider: c.provider, Service: c.service}}} - assert.Equal(t, c.want, skippedAccountNote(row), c.provider+"/"+c.service) - assert.Equal(t, RedriveRefusalReason(row) == "", c.want == skippedAccountRetryNote, c.provider+"/"+c.service) + row := &config.PurchaseExecution{ + Status: config.StatusFailed, NoSubmission: true, RetryAttemptN: c.attempt, + Recommendations: []config.RecommendationRecord{{Provider: c.provider, Service: c.service}}, + } + name := fmt.Sprintf("%s/%s/attempt%d", c.provider, c.service, c.attempt) + assert.Equal(t, c.want, skippedAccountNote(row), name) + assert.Equal(t, RetryRefusalReason(row) == "", c.want == skippedAccountRetryNote, name) } } diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 0765ee84d..5c3162cb9 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -426,7 +426,7 @@ func (m *Manager) saveSkippedRows(ctx context.Context, root *config.PurchaseExec // All refusals except the armed re-drive and missing-journal ones are // beforeSubmission errors, so the row ends failed with no_submission stamped. func (m *Manager) preExecutionRefusal(ctx context.Context, exec *config.PurchaseExecution) error { - if err := armedRedriveRefusal(exec); err != nil { + if err := m.armedRedriveRefusal(ctx, exec); err != nil { return err } if refusal := m.staleAzurePricingRefusal(ctx, exec); refusal != nil { @@ -567,22 +567,22 @@ func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { // the way three copies at three call sites can. executePurchase has exactly one // production caller, immediately below this check. // -// The condition is a conjunction and BOTH halves are load-bearing: +// The checks run in this order, so a lookup happens only when it can matter: // -// - RetryAttemptN > 0 restricts it to retry successors. A fresh execution -// (n == 0) is a FIRST purchase with nothing to duplicate, and Azure savings -// plans must remain buyable, so re-drive safety alone here would block -// every legitimate first buy. -// - RedriveRefusalReason != "" restricts it to providers with no duplicate -// guard, reusing the predicate from #1668 rather than introducing a second -// notion of re-drive safety. +// - (a) RetryAttemptN <= 0 returns nil. A fresh execution is a FIRST purchase +// with nothing to duplicate, and Azure savings plans must remain buyable. +// - (b) An empty RedriveRefusalReason returns nil: the provider has a +// duplicate guard (#1668 predicate, not a second notion of re-drive safety). +// - (c) Otherwise, for a first retry only, the predecessor is read. When it is +// a failed no_submission attempt 0, the lineage provably sent nothing and +// the no-guard refusal is lifted (retryRefusalReason). A lookup error +// refuses with fixed text, never a beforeSubmission error. // // Returning an error rather than skipping silently means finalizeExecution // stamps the row "failed" with this reason and executeAndFinalize persists it, -// so an armed row is DEFUSED and visible in History instead of remaining armed -// for the next sweep. Retrying it from the UI then hits the #1668 gate, which -// refuses, so it cannot be re-armed either. -func armedRedriveRefusal(exec *config.PurchaseExecution) error { +// so a refused row is visible in History instead of remaining armed for the +// next sweep. +func (m *Manager) armedRedriveRefusal(ctx context.Context, exec *config.PurchaseExecution) error { if exec.RetryAttemptN <= 0 { return nil } @@ -590,9 +590,27 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { if reason == "" { return nil } + // Only a first retry can follow a proven-unsent first attempt (D1: no chain + // walk), so deeper attempts skip the lookup and keep the refusal. + proven := false + if exec.RetryAttemptN == 1 { + var err error + proven, err = m.config.PredecessorProvenUnsent(ctx, exec.ExecutionID) + if err != nil { + logging.Errorf("predecessor proof lookup failed for execution %s: %v", exec.ExecutionID, err) + return errors.New(armedRedriveLookupRefusal) + } + } + if reason = retryRefusalReason(exec, proven); reason == "" { + return nil + } return fmt.Errorf("refusing to execute retry attempt %d: %s", exec.RetryAttemptN, reason) } +// armedRedriveLookupRefusal is fixed text (no wrapped DB error) so the stored +// error stays on the errnotes allowlist. +const armedRedriveLookupRefusal = "refusing to execute retry: could not confirm that the earlier attempt sent nothing; start a new purchase instead of retrying" + // 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. diff --git a/internal/purchase/retry_unsent.go b/internal/purchase/retry_unsent.go new file mode 100644 index 000000000..f81ceb6e8 --- /dev/null +++ b/internal/purchase/retry_unsent.go @@ -0,0 +1,27 @@ +package purchase + +import "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + +// RetryRefusalReason is RedriveRefusalReason for the user-facing Retry of a +// failed row. A row proven to have sent nothing (no_submission at attempt 0) +// cannot have bought anything, so the no-duplicate-guard refusals do not apply. +func RetryRefusalReason(exec *config.PurchaseExecution) string { + return retryRefusalReason(exec, NeverSubmitted(exec) && exec.Status == config.StatusFailed) +} + +// retryRefusalReason lifts only the AWS EC2 and Azure savings-plans +// reasons when lineageUnsent. Unknown providers and any other reason still +// refuse: no proof of an unsent lineage says anything about them. +func retryRefusalReason(exec *config.PurchaseExecution, lineageUnsent bool) string { + for i := range exec.Recommendations { + reason := recRedriveRefusalReason(exec.Recommendations[i]) + if reason == "" { + continue + } + if lineageUnsent && (reason == redriveReasonAWSNoGuard || reason == redriveReasonAzureSavingsPlan) { + continue + } + return reason + } + return "" +} diff --git a/internal/purchase/retry_unsent_test.go b/internal/purchase/retry_unsent_test.go new file mode 100644 index 000000000..2bb90955b --- /dev/null +++ b/internal/purchase/retry_unsent_test.go @@ -0,0 +1,56 @@ +package purchase + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +func retryExec(status string, ns bool, attempt int, recs ...config.RecommendationRecord) *config.PurchaseExecution { + return &config.PurchaseExecution{Status: status, NoSubmission: ns, RetryAttemptN: attempt, Recommendations: recs} +} + +func TestRetryRefusalReason(t *testing.T) { + ec2 := config.RecommendationRecord{Provider: "aws", Service: "ec2"} + redshift := config.RecommendationRecord{Provider: "aws", Service: "redshift"} + azSP := config.RecommendationRecord{Provider: "azure", Service: "savingsplans"} + rds := config.RecommendationRecord{Provider: "aws", Service: "rds"} + unknown := config.RecommendationRecord{Provider: "oracle", Service: "compute"} + + tests := []struct { + name string + exec *config.PurchaseExecution + refused bool + }{ + {"NS attempt 0 failed EC2", retryExec(config.StatusFailed, true, 0, ec2), false}, + {"Redshift has no no-guard refusal to lift", retryExec(config.StatusFailed, false, 0, redshift), false}, + {"NS attempt 0 failed Azure SP", retryExec(config.StatusFailed, true, 0, azSP), false}, + {"not NS EC2", retryExec(config.StatusFailed, false, 0, ec2), true}, + {"NS attempt 1 EC2", retryExec(config.StatusFailed, true, 1, ec2), true}, + {"NS attempt 0 not failed EC2", retryExec("running", true, 0, ec2), true}, + {"NS unknown provider", retryExec(config.StatusFailed, true, 0, unknown), true}, + {"NS mixed EC2 and unknown", retryExec(config.StatusFailed, true, 0, ec2, unknown), true}, + {"RDS not NS", retryExec(config.StatusFailed, false, 0, rds), false}, + {"RDS NS", retryExec(config.StatusFailed, true, 0, rds), false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := RetryRefusalReason(tc.exec) + if tc.refused { + assert.NotEmpty(t, got) + } else { + assert.Empty(t, got) + } + }) + } +} + +func TestRetryRefusalReasonLineageUnsentOverride(t *testing.T) { + ec2 := config.RecommendationRecord{Provider: "aws", Service: "ec2"} + unknown := config.RecommendationRecord{Provider: "oracle", Service: "compute"} + e := retryExec(config.StatusFailed, false, 1, ec2) + assert.NotEmpty(t, retryRefusalReason(e, false)) + assert.Empty(t, retryRefusalReason(e, true)) + assert.NotEmpty(t, retryRefusalReason(retryExec(config.StatusFailed, false, 1, unknown), true)) +}