diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index b37d2cd8a..4073a73de 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -550,6 +550,32 @@ describe('History inline Retry button (issue #47)', () => { expect(btn?.disabled).toBe(false); }); + test('Redshift settle 409 with retry_after_seconds shows a wait toast, not a generic error', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + const settleError = Object.assign(new Error('this Redshift purchase failed too recently to retry safely; retry after the wait'), { + details: { retry_after_seconds: 90 }, + }); + (api.retryPurchase as jest.Mock).mockRejectedValue(settleError); + (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(); + document.querySelector('.history-retry-btn')?.click(); + await flushPromises(); + + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ + kind: 'warning', + message: expect.stringContaining('Try again in 2 minutes'), + }), + ); + expect(showToast).not.toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); + }); + 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/history.ts b/frontend/src/history.ts index 7479ace85..d56a252d5 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -1647,6 +1647,16 @@ function wireRowActionHandlers(container: HTMLElement): void { 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; + // A Redshift purchase that failed minutes ago may still be settling at AWS (not overridable by force). + const retryAfterSeconds = typeof err.details?.['retry_after_seconds'] === 'number' ? err.details['retry_after_seconds'] : undefined; + if (!opsHint && retryAfterSeconds != null) { + const minutes = Math.max(1, Math.ceil(retryAfterSeconds / 60)); + showToast({ + message: `Retry is not possible yet: AWS may still be settling this purchase. Try again in ${minutes} minute${minutes === 1 ? '' : 's'}.`, + kind: 'warning', + }); + return; + } let detailMessage = ''; if (opsHint) { detailMessage = opsHint; diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 80e046e93..70f9e7085 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -406,7 +406,7 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma // failed rows can carry a hint — the resolver returns "" for empty // inputs, so this is safe to call unconditionally. if exec.Status == "failed" { - row.OpsHint = resolveOpsHint(exec.Error) + row.OpsHint = purchase.ResolveOpsHint(exec.Error) // A re-drive 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 @@ -421,12 +421,31 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma } else { row.OpsHint = reason } + } else { + applyRedshiftRetryHint(&row, &exec) } } annotateHistoryRowByStatus(&row, exec, approver) return row } +// applyRedshiftRetryHint hides Retry for a Redshift row the gate refuses for good; the stamp age +// is an app-clock hint, so near 24h Retry may show and then 409 from the database-clock gate. +func applyRedshiftRetryHint(row *config.PurchaseHistoryRecord, exec *config.PurchaseExecution) { + if !purchase.IsRedshiftExecution(exec) { + return + } + var sinceStamp *time.Duration + if exec.FirstDispatchAt != nil { + age := max(0, time.Since(*exec.FirstDispatchAt)) + sinceStamp = &age + } + if reason := purchase.RedshiftRetryPermanentRefusal(exec, sinceStamp); reason != "" { + row.RedriveUnsafe = true + row.OpsHint = reason + } +} + // definiteRefusalOpsHint replaces the duplicate-purchase warning when AWS // refused the purchase before making it. const definiteRefusalOpsHint = "AWS refused this purchase before it was made; start a new purchase after fixing the cause" diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index f4778c5d4..b0c20664f 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -2192,13 +2192,12 @@ func TestExecutionToHistoryRow_RedriveUnsafe(t *testing.T) { {"azure savings plans", failed("azure", "savingsplans", "boom"), true, ""}, {"unrecognized provider", failed("oracle", "compute", "boom"), true, ""}, {"failed aws ec2", failed("aws", "ec2", "boom"), true, ""}, - {"failed aws redshift", failed("aws", "redshift", "boom"), true, ""}, {"failed mixed ec2+rds basket", config.PurchaseExecution{ExecutionID: "e-1", Status: "failed", Error: "boom", Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds"}, {Provider: "aws", Service: "ec2"}}}, true, ""}, {"pending ec2 gets no hint", withStatus("pending", config.RecommendationRecord{Provider: "aws", Service: "ec2"}), false, ""}, {"canceled ec2 gets no hint", withStatus("canceled", config.RecommendationRecord{Provider: "aws", Service: "ec2"}), false, ""}, {"aws rds is retryable", failed("aws", "rds", "boom"), false, ""}, {"refusal wins over failure-reason hint", failed("azure", "savingsplans", persistent), true, ""}, - {"failure-reason hint kept when re-drive is safe", failed("aws", "rds", persistent), false, resolveOpsHint(persistent)}, + {"failure-reason hint kept when re-drive is safe", failed("aws", "rds", persistent), false, purchase.ResolveOpsHint(persistent)}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2214,6 +2213,46 @@ func TestExecutionToHistoryRow_RedriveUnsafe(t *testing.T) { } } +// The Redshift hint is a presentation hint on the app clock; the Retry gate stays authoritative. +func TestExecutionToHistoryRow_RedshiftHistoryHintUsesAppClock(t *testing.T) { + const legacyArmedRefusal = "refusing to execute retry attempt 1: AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" + stampedAgo := func(d time.Duration) *time.Time { ts := time.Now().Add(-d); return &ts } + row := func(status string, attempt int, noSubmission bool, stamp *time.Time, errMsg string) config.PurchaseExecution { + return config.PurchaseExecution{ + ExecutionID: "rs-1", Status: status, Error: errMsg, RetryAttemptN: attempt, NoSubmission: noSubmission, FirstDispatchAt: stamp, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "redshift", Region: "r", Selected: true}}, + } + } + tests := []struct { + name string + exec config.PurchaseExecution + wantReason string + }{ + {"stampless legacy failed row", row("failed", 1, false, nil, "boom"), "no first dispatch time recorded"}, + {"stampless attempt 0 that sent", row("failed", 0, false, nil, "boom"), "no first dispatch time recorded"}, + {"legacy armed refusal row", row("failed", 1, false, nil, legacyArmedRefusal), "no first dispatch time recorded"}, + {"past the 24h horizon", row("failed", 1, false, stampedAgo(25*time.Hour), "boom"), "at least 24h ago"}, + {"inside the horizon stays retryable", row("failed", 1, false, stampedAgo(time.Hour), "boom"), ""}, + {"never submitted attempt 0 skips the stamp rules", row("failed", 0, true, nil, "boom"), ""}, + {"never submitted attempt 0 past the horizon", row("failed", 0, true, stampedAgo(48*time.Hour), "boom"), ""}, + {"failure-reason hint kept inside the horizon", row("failed", 1, false, stampedAgo(time.Hour), "FROM_EMAIL not configured"), ""}, + {"pending row gets no hint", row("pending", 1, false, nil, ""), ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := executionToHistoryRow(tt.exec, "", "") + if tt.wantReason == "" { + assert.False(t, got.RedriveUnsafe) + assert.Equal(t, purchase.ResolveOpsHint(tt.exec.Error), got.OpsHint) + return + } + assert.True(t, got.RedriveUnsafe) + assert.Contains(t, got.OpsHint, tt.wantReason) + }) + } + assert.Equal(t, legacyArmedRefusal, purchase.SanitizeStoredError(legacyArmedRefusal), "the legacy stored error still renders next to the new hint") +} + // TestHandler_getHistory_ProjectsCreatedBySystem: the History UI needs the #233 marker. func TestHandler_getHistory_ProjectsCreatedBySystem(t *testing.T) { ctx := context.Background() diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 6fe82dd9e..be9073a32 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2358,46 +2358,6 @@ func (h *Handler) requireSession(ctx context.Context, req *events.LambdaFunction // they accumulate dozens of dead retry rows. const retryThreshold = 5 -// persistentFailureHints maps known-persistent failure substrings to -// the operator-actionable hint surfaced on the History row in place of -// the Retry button (issue #47, Q3). The match is a case-INsensitive -// substring contains check (see resolveOpsHint) — wording variations -// like "ses sandbox" / "SES Sandbox" all hit. Keep needles short and -// distinctive so legitimate transient failures don't accidentally -// substring-match a persistent hint and lock the user out. -// -// Membership criteria: only failures that NO retry can possibly fix — -// they require an operator to change configuration or unblock an -// upstream resource. SES sandbox / unverified domain / missing -// FROM_EMAIL all fit; a transient SES throttle does NOT (the next -// retry might succeed). When in doubt, leave it out — false-negatives -// cost a wasted retry; false-positives lock the user out of an -// actionable button entirely. -var persistentFailureHints = map[string]string{ - "FROM_EMAIL not configured": "Set FROM_EMAIL tfvar then retry", - "SES sandbox": "Move SES out of sandbox or verify recipient, then retry", - "SES domain not verified": "Verify SES domain in AWS console, then retry", - "IAM denied": "Grant the deploy role missing IAM permission, then retry", -} - -// resolveOpsHint returns a non-empty hint when the failure reason -// matches a known-persistent pattern, else empty. Match is case- -// insensitive substring contains so backend wording variations don't -// silently slip through ("ses sandbox" / "SES sandbox" / "SES Sandbox" -// all hit). -func resolveOpsHint(failureReason string) string { - if failureReason == "" { - return "" - } - lower := strings.ToLower(failureReason) - for needle, hint := range persistentFailureHints { - if strings.Contains(lower, strings.ToLower(needle)) { - return hint - } - } - return "" -} - // retryPurchase creates a new execution from a *failed* execution's // stored Recommendations slice (issue #47). The original failed row // keeps its `failed` status as a historical record and gains a @@ -2601,12 +2561,12 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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 and - // Redshift (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 or Redshift row therefore cannot be - // retried; the operator checks the account and starts a new purchase. + // 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. // If plan steps ever carry recommendations (platform#609), a failed EC2 // step will be un-retryable too and needs its own recovery path. // @@ -2648,7 +2608,7 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // 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. - if hint := resolveOpsHint(failedExec.Error); hint != "" { + if hint := purchase.ResolveOpsHint(failedExec.Error); hint != "" { return NewClientErrorWithDetails(409, "this failure is operator-fixable; retrying without changing configuration will fail again", map[string]any{"ops_hint": hint, "failure_reason": purchase.SanitizeStoredError(failedExec.Error)}) diff --git a/internal/api/handler_purchases_redshift_retry_test.go b/internal/api/handler_purchases_redshift_retry_test.go index 7083f17e3..19be0f621 100644 --- a/internal/api/handler_purchases_redshift_retry_test.go +++ b/internal/api/handler_purchases_redshift_retry_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -100,17 +101,17 @@ func TestCheckRedshiftRetryGate_ClockErrorFailsClosed(t *testing.T) { assert.False(t, isClient, "a store failure is a 500, not a refusal the UI would render as permanent") } -// Until the old guard is lifted it refuses every Redshift Retry first, so the -// new gate must not even read the clock. -func TestLoadAndValidateRetry_OldRedriveRefusalStillWinsForRedshift(t *testing.T) { +// The provider-level refusal no longer covers Redshift, so a Redshift Retry reaches the new gate. +func TestLoadAndValidateRetry_RedshiftReachesTheNewGate(t *testing.T) { failed := redshiftFailedRow(1, false) session := &Session{UserID: retryCallerID, Email: "operator@example.com"} h, store, _ := buildSessionRetryHandler(t, failed, session, false, true) + store.On("GetRedshiftRetryClock", mock.Anything, failed.ExecutionID).Return(config.RedshiftRetryClock{SinceUpdate: time.Hour}, nil) _, _, err := h.loadAndValidateRetryRequest(context.Background(), sessionRetryReqWithForce(), failed.ExecutionID) - assertRedriveRefused(t, err, "Check EC2 Reserved Instances or Redshift Reserved Nodes") - store.AssertNotCalled(t, "GetRedshiftRetryClock", mock.Anything, mock.Anything) + assertRedriveRefused(t, err, "no first dispatch time recorded") + store.AssertCalled(t, "GetRedshiftRetryClock", mock.Anything, failed.ExecutionID) } func persistedSuccessor(t *testing.T, failed *config.PurchaseExecution) *config.PurchaseExecution { @@ -156,3 +157,61 @@ func TestPersistRetryExecution_NoSubmissionAttempt0LeavesStampNil(t *testing.T) func TestPersistRetryExecution_StamplessPredecessorStaysStampless(t *testing.T) { assert.Nil(t, persistedSuccessor(t, redshiftFailedRow(1, false)).FirstDispatchAt) } + +// retryRedshiftViaHandler drives the whole retryPurchase handler for a failed Redshift row. +func retryRedshiftViaHandler(t *testing.T, failed *config.PurchaseExecution, clock config.RedshiftRetryClock, req *events.LambdaFunctionURLRequest) (saved []*config.PurchaseExecution, err error) { + t.Helper() + session := &Session{UserID: retryCallerID, Email: "operator@example.com"} + h, store, _ := buildSessionRetryHandler(t, failed, session, false, true) + store.On("GetRedshiftRetryClock", mock.Anything, failed.ExecutionID).Return(clock, nil) + store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + snap := *args.Get(1).(*config.PurchaseExecution) + saved = append(saved, &snap) + }). + Return(nil).Maybe() + store.On("LinkRetryExecutionAtomic", mock.Anything, mock.Anything, failed.ExecutionID, mock.AnythingOfType("string")).Return(true, nil).Maybe() + + _, err = h.retryPurchase(context.Background(), req, failed.ExecutionID) + return saved, err +} + +func TestRetryRedshift_InsideSettleRefusedWithRetryAfter(t *testing.T) { + stamp := time.Now().Add(-time.Hour) + failed := redshiftFailedRow(1, false) + failed.FirstDispatchAt = &stamp + + for _, req := range []*events.LambdaFunctionURLRequest{sessionRetryReq(), sessionRetryReqWithForce()} { + saved, err := retryRedshiftViaHandler(t, failed, config.RedshiftRetryClock{SinceUpdate: 4 * time.Minute, SinceStamp: ageOf(time.Hour)}, req) + + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 409, ce.code) + assert.Equal(t, 360, ce.Details()["retry_after_seconds"]) + assert.Equal(t, "360", ce.headers["Retry-After"]) + assert.Empty(t, saved, "a refused retry persists no successor") + } +} + +func TestRetryRedshift_AfterSettleCopiesStamp(t *testing.T) { + stamp := time.Now().Add(-time.Hour).UTC().Truncate(time.Microsecond) + failed := redshiftFailedRow(1, false) + failed.FirstDispatchAt = &stamp + + saved, err := retryRedshiftViaHandler(t, failed, config.RedshiftRetryClock{SinceUpdate: 11 * time.Minute, SinceStamp: ageOf(time.Hour)}, sessionRetryReq()) + + require.NoError(t, err) + require.NotEmpty(t, saved) + require.NotNil(t, saved[0].FirstDispatchAt) + assert.True(t, stamp.Equal(*saved[0].FirstDispatchAt), "the successor keeps the lineage stamp") + assert.Equal(t, 2, saved[0].RetryAttemptN) +} + +func TestRetryRedshift_LegacyStamplessRefused(t *testing.T) { + failed := redshiftFailedRow(1, false) + + saved, err := retryRedshiftViaHandler(t, failed, config.RedshiftRetryClock{SinceUpdate: time.Hour}, sessionRetryReqWithForce()) + + assertRedriveRefused(t, err, "no first dispatch time recorded") + assert.Empty(t, saved) +} diff --git a/internal/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go index dbc3b808f..8b3a36fe9 100644 --- a/internal/api/handler_purchases_retry_redrive_test.go +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -299,16 +299,16 @@ func TestRetryOfFailedAzureReservationStillPurchasesOnce(t *testing.T) { "the retry must fire exactly one purchase, under the predecessor's token so Azure's two-step lookup dedupes it") } -// TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused is the MON-02 platform -// regression guard. EC2 and Redshift purchases take no client token, so the -// provider sends them once and reports a lost response as "purchase outcome -// unknown": the commitment may exist. Retrying such a row must not create a -// successor that buys again, whatever spelling the row carries (the canonical -// compute and data-warehouse slugs dispatch to the same clients as ec2 and -// redshift) and with ?force=true too. -func TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused(t *testing.T) { +// TestRetryOfFailedAWSEC2RowIsRefused is the MON-02 platform regression guard. +// EC2 purchases take no client token, so the provider sends them once and +// reports a lost response as "purchase outcome unknown": the commitment may +// exist. Retrying such a row must not create a successor that buys again, +// whatever spelling the row carries (the canonical compute slug dispatches to +// the same client as ec2) and with ?force=true too. Redshift has its own gate +// (handler_purchases_redshift_retry_test.go). +func TestRetryOfFailedAWSEC2RowIsRefused(t *testing.T) { for _, tc := range []struct{ provider, service string }{ - {"aws", "ec2"}, {"aws", "compute"}, {"aws", "redshift"}, {"aws", "data-warehouse"}, {"", "ec2"}, + {"aws", "ec2"}, {"aws", "compute"}, {"", "ec2"}, } { tc := tc t.Run(tc.provider+"/"+tc.service, func(t *testing.T) { @@ -319,8 +319,8 @@ func TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused(t *testing.T) { tokens, err := purchasesFiredByRetry(t, failed, req) assert.Empty(t, tokens, - "a retry of an EC2/Redshift row after an unknown outcome must buy nothing") - assertRedriveRefused(t, err, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + "a retry of an EC2 row after an unknown outcome must buy nothing") + assertRedriveRefused(t, err, "Check EC2 Reserved Instances in the target account") } }) } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 099816aca..67a39f753 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4556,26 +4556,6 @@ func TestResolveApprovalToken_PostBodyNoToken(t *testing.T) { "POST body without token field must fall back to query-string token") } -func TestResolveOpsHint(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - {"empty input", "", ""}, - {"transient SES throttle (no match)", "send failed: SES throttle exceeded", ""}, - {"FROM_EMAIL exact", "FROM_EMAIL not configured for this deployment", "Set FROM_EMAIL tfvar then retry"}, - {"SES sandbox case-insensitive", "send failed: ses sandbox active", "Move SES out of sandbox or verify recipient, then retry"}, - {"domain not verified", "send failed: SES domain not verified", "Verify SES domain in AWS console, then retry"}, - {"IAM denied", "AssumeRole error: IAM denied", "Grant the deploy role missing IAM permission, then retry"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, resolveOpsHint(tt.input)) - }) - } -} - // TestHandler_approvalResponseRecipient_TrimsWhitespace verifies that a // whitespace-only notification_email does not count as set: the contact_email // fallback must be used instead, and no whitespace must appear in the response. diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 602a538b6..349f1d659 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1471,10 +1471,17 @@ 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), its - failure is operator-fixable (ops_hint plus failure_reason), or it - reached the retry threshold without force=true (retry_attempt_n - plus threshold). + redrive_unsafe, permanent and not overridable by force), 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 + failure_reason), or it reached the retry threshold without + force=true (retry_attempt_n plus threshold). + headers: + Retry-After: + description: Seconds to wait before retrying (Redshift settle wait only) + schema: + type: integer content: application/json: schema: @@ -1489,6 +1496,9 @@ paths: type: string redrive_unsafe: type: boolean + retry_after_seconds: + type: integer + description: Seconds until a Redshift purchase has settled and can be retried. failure_reason: type: string retry_attempt_n: @@ -7706,7 +7716,10 @@ components: description: Execution-only. Operator hint shown instead of Retry on failed rows. redrive_unsafe: type: boolean - description: Execution-only. True when ops_hint is a permanent re-drive refusal. + 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. is_audit_gap: type: boolean description: >- diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index a2f03fc70..0dff7f442 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -828,7 +828,7 @@ type StoreInterface interface { // FindOpenRedshiftLineageConflictsTx returns the other open executions // holding a Redshift rec for one of tuples that a dispatch of family must // not race (see the implementation for the predicate). - FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window time.Duration) ([]RedshiftLineageConflict, error) + FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window, settle time.Duration) ([]RedshiftLineageConflict, error) // GetRedshiftRetryClock returns how long ago an execution was last written // and first dispatched, both measured on the database clock. diff --git a/internal/config/store_postgres_redshift_lineage.go b/internal/config/store_postgres_redshift_lineage.go index 268af6840..5e358ad5f 100644 --- a/internal/config/store_postgres_redshift_lineage.go +++ b/internal/config/store_postgres_redshift_lineage.go @@ -56,6 +56,31 @@ func (s *PostgresStore) LockRedshiftTuplesTx(ctx context.Context, tx pgx.Tx, tup return nil } +// redshiftFailedClause matches other-family failed rows that Retry could still re-drive. +// A settled row is unretryable; without the settlement columns none can be settled. +func redshiftFailedClause(settleReady bool) string { + notSettled := "" + if settleReady { + notSettled = ` + AND e.outcome_settled_at IS NULL` + } + return `(split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) <> $2 + AND e.status = 'failed' + AND e.retry_execution_id IS NULL + AND NOT (e.no_submission AND e.retry_attempt_n = 0) + AND e.first_dispatch_at > now() - make_interval(secs => $3)` + notSettled + `)` +} + +// redshiftSameFamilySettleClause matches a same-family row whose own attempt may have bought and +// was last written within $8 seconds: its node can still be missing from AWS describe results. +// A linked failed row already passed the Retry settle (and Retry bumps its updated_at). +func redshiftSameFamilySettleClause() string { + return `(split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) = $2 + AND ((e.status = 'failed' AND e.retry_execution_id IS NULL AND NOT e.no_submission) + OR e.status IN ('completed', 'partially_completed', 'revocation_requested')) + AND e.updated_at > now() - make_interval(secs => $8))` +} + // RedshiftLineageConflict is an open execution whose Redshift rec matches a // candidate tuple. SameFamily is true when it shares the candidate's lineage // root (idempotency key up to the first colon). @@ -70,10 +95,10 @@ type RedshiftLineageConflict struct { // FindOpenRedshiftLineageConflictsTx returns the other executions whose node this lineage could // buy twice or adopt. Account: row's else rec's vs execution's else tuple's; NULL matches any. -// Running rows conflict at any age; other open rows while their stamp is within horizon. -// Other-family finished rows conflict while updated_at is within window of stamp, since go's -// dedupe adopts their node. Same-family rows conflict only while running. -func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window time.Duration) ([]RedshiftLineageConflict, error) { +// Running rows conflict at any age; other open rows within horizon of their stamp. Other-family +// finished rows within window of stamp; other-family retryable failed rows within horizon. +// Same-family rows: running, or failed (unlinked) or finished after a possible send, within settle. +func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window, settle time.Duration) ([]RedshiftLineageConflict, error) { if family == "" { return nil, fmt.Errorf("find open redshift lineage conflicts: empty family") } @@ -84,6 +109,11 @@ func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, if err != nil { return nil, fmt.Errorf("marshal redshift tuples: %w", err) } + // Before the settlement migration no row can be settled (#999). + settleReady, err := settlementSchemaReady(ctx, tx) + if err != nil { + return nil, err + } rows, err := tx.Query(ctx, ` SELECT e.execution_id, e.status, e.first_dispatch_at, COALESCE(e.error, ''), e.recommendations, split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) = $2 @@ -98,6 +128,8 @@ func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, OR (split_part(COALESCE(NULLIF(e.idempotency_key, ''), e.execution_id::text), ':', 1) <> $2 AND e.status IN ('completed', 'partially_completed', 'revocation_requested') AND e.updated_at >= $6::timestamptz - make_interval(secs => $7)) + OR `+redshiftFailedClause(settleReady)+` + OR `+redshiftSameFamilySettleClause()+` ) AND EXISTS ( SELECT 1 @@ -112,7 +144,7 @@ func (s *PostgresStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, AND COALESCE(r->>'provider', '') IN ('', 'aws') AND r->>'service' IN ('redshift', 'data-warehouse')) ORDER BY e.created_at, e.execution_id`, - selfID, family, horizon.Seconds(), accountID, tuplesJSON, stamp, window.Seconds()) + selfID, family, horizon.Seconds(), accountID, tuplesJSON, stamp, window.Seconds(), settle.Seconds()) if err != nil { return nil, fmt.Errorf("find open redshift lineage conflicts: %w", err) } diff --git a/internal/config/store_postgres_redshift_lineage_integration_test.go b/internal/config/store_postgres_redshift_lineage_integration_test.go index 0e285b71e..478de9300 100644 --- a/internal/config/store_postgres_redshift_lineage_integration_test.go +++ b/internal/config/store_postgres_redshift_lineage_integration_test.go @@ -5,6 +5,7 @@ package config import ( "context" + "strings" "testing" "time" @@ -16,6 +17,7 @@ import ( const ( lineageTestHorizon = 24 * time.Hour lineageTestWindow = 10 * time.Minute + lineageTestSettle = 10 * time.Minute ) var lineageTuple = RedshiftTuple{Region: "us-east-1", NodeType: "ra3.xlplus", Term: 1, Payment: "all-upfront", Count: 2} @@ -46,7 +48,7 @@ func findLineageConflicts(ctx context.Context, t *testing.T, store *PostgresStor var got []RedshiftLineageConflict require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { var err error - got, err = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{lineageTuple}, nil, lineageTestHorizon, time.Now(), lineageTestWindow) + got, err = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{lineageTuple}, nil, lineageTestHorizon, time.Now(), lineageTestWindow, lineageTestSettle) return err })) return got @@ -93,9 +95,18 @@ func TestFindOpenRedshiftLineageConflicts(t *testing.T) { t.Run("same_family_non_running_excluded", func(t *testing.T) { reset(t) self := seedLineageRow(ctx, t, store, "fam-c", "running", recent) - for _, st := range []string{"failed", "pending", "approved"} { + for _, st := range []string{"pending", "approved", "scheduled"} { seedLineageRow(ctx, t, store, "fam-c:"+st, st, recent) } + // Old updated_at via a plain INSERT: the trigger overwrites it on UPDATE. + old := time.Now().Add(-lineageTestSettle - time.Minute) + for _, st := range []string{"failed", "completed"} { + _, err := store.db.Exec(ctx, ` + INSERT INTO purchase_executions (status, step_number, scheduled_date, idempotency_key, first_dispatch_at, updated_at, recommendations) + VALUES ($1, 1, NOW(), $2, $3, $3, $4::jsonb)`, st, "fam-c:"+st, old, + `[{"provider":"aws","service":"redshift","region":"us-east-1","resource_type":"ra3.xlplus","term":1,"payment":"all-upfront","count":2,"selected":true}]`) + require.NoError(t, err) + } assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-c")) }) t.Run("other_family_running_any_stamp_age_conflicts", func(t *testing.T) { @@ -159,7 +170,7 @@ func TestFindOpenRedshiftLineageConflicts(t *testing.T) { var got []RedshiftLineageConflict require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { var e error - got, e = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self.ExecutionID, "fam-k", []RedshiftTuple{tp}, nil, lineageTestHorizon, time.Now(), lineageTestWindow) + got, e = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self.ExecutionID, "fam-k", []RedshiftTuple{tp}, nil, lineageTestHorizon, time.Now(), lineageTestWindow, lineageTestSettle) return e })) return got @@ -180,7 +191,7 @@ func TestFindOpenRedshiftLineageConflicts(t *testing.T) { var got []RedshiftLineageConflict require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { var e error - got, e = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{tp}, arg, lineageTestHorizon, time.Now(), lineageTestWindow) + got, e = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{tp}, arg, lineageTestHorizon, time.Now(), lineageTestWindow, lineageTestSettle) return e })) return got @@ -313,7 +324,7 @@ func TestFindOpenRedshiftLineageConflicts(t *testing.T) { t.Run("empty_family_is_rejected", func(t *testing.T) { reset(t) require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { - _, err := store.FindOpenRedshiftLineageConflictsTx(ctx, tx, "x", "", []RedshiftTuple{lineageTuple}, nil, lineageTestHorizon, time.Now(), lineageTestWindow) + _, err := store.FindOpenRedshiftLineageConflictsTx(ctx, tx, "x", "", []RedshiftTuple{lineageTuple}, nil, lineageTestHorizon, time.Now(), lineageTestWindow, lineageTestSettle) assert.Error(t, err) return nil })) @@ -372,7 +383,7 @@ func TestFindOpenRedshiftLineageConflicts_AdoptionWindow(t *testing.T) { var got []RedshiftLineageConflict require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { var e error - got, e = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{tp}, arg, lineageTestHorizon, stamp, lineageTestWindow) + got, e = store.FindOpenRedshiftLineageConflictsTx(ctx, tx, self, family, []RedshiftTuple{tp}, arg, lineageTestHorizon, stamp, lineageTestWindow, lineageTestSettle) return e })) return got @@ -416,8 +427,19 @@ func TestFindOpenRedshiftLineageConflicts_AdoptionWindow(t *testing.T) { t.Run("same_family_completed_not_conflict", func(t *testing.T) { reset(t) self := seedLineageRow(ctx, t, store, "fam-w5", "running", recent) - other := seedLineageRow(ctx, t, store, "fam-w5:acct", "completed", recent) - assert.Empty(t, find(t, self.ExecutionID, "fam-w5", lineageTuple, nil, updatedAt(t, other.ExecutionID))) + old := time.Now().Add(-lineageTestSettle - time.Minute) + _, err := store.db.Exec(ctx, ` + INSERT INTO purchase_executions (status, step_number, scheduled_date, idempotency_key, first_dispatch_at, updated_at, recommendations) + VALUES ('completed', 1, NOW(), 'fam-w5:acct', $1, $1, $2::jsonb)`, old, + `[{"provider":"aws","service":"redshift","region":"us-east-1","resource_type":"ra3.xlplus","term":1,"payment":"all-upfront","count":2,"selected":true}]`) + require.NoError(t, err) + assert.Empty(t, find(t, self.ExecutionID, "fam-w5", lineageTuple, nil, time.Now())) + }) + t.Run("same_family_completed_inside_settle_conflicts", func(t *testing.T) { + reset(t) + self := seedLineageRow(ctx, t, store, "fam-w5b", "running", recent) + other := seedLineageRow(ctx, t, store, "fam-w5b:acct", "completed", recent) + assert.Equal(t, []string{other.ExecutionID}, conflictIDs(find(t, self.ExecutionID, "fam-w5b", lineageTuple, nil, updatedAt(t, other.ExecutionID)))) }) t.Run("different_count_passes", func(t *testing.T) { reset(t) @@ -477,3 +499,121 @@ func TestFindOpenRedshiftLineageConflicts_AdoptionWindow(t *testing.T) { assert.Equal(t, []string{id}, conflictIDs(find(t, self.ExecutionID, "fam-w10", lineageTuple, nil, stamp))) }) } + +func TestFindOpenRedshiftLineageConflicts_FailedRows(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + recent, old := ptrDur(time.Hour), ptrDur(48*time.Hour) + // seedFailed seeds an other-family failed row and applies a column update to it. + seedFailed := func(t *testing.T, family string, stampAge *time.Duration, set string) (self, failed *PurchaseExecution) { + t.Helper() + _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) + require.NoError(t, err) + self = seedLineageRow(ctx, t, store, "fam-f0", "running", recent) + failed = seedLineageRow(ctx, t, store, family, "failed", stampAge) + if set != "" { + args := []any{failed.ExecutionID} + if strings.Contains(set, "$2") { + args = append(args, self.ExecutionID) + } + _, err = store.db.Exec(ctx, `UPDATE purchase_executions SET `+set+` WHERE execution_id = $1`, args...) + require.NoError(t, err) + } + return self, failed + } + + t.Run("failed_unlinked_conflicts", func(t *testing.T) { + self, failed := seedFailed(t, "fam-f1", recent, "") + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0"))) + }) + t.Run("failed_attempt1_no_submission_conflicts", func(t *testing.T) { + self, failed := seedFailed(t, "fam-f2", recent, "no_submission = true, retry_attempt_n = 1") + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0"))) + }) + t.Run("failed_linked_excluded", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f3", recent, "retry_execution_id = $2") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("failed_attempt0_no_submission_excluded", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f4", recent, "no_submission = true") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("failed_older_than_H_excluded", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f5", old, "") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("failed_settled_excluded", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f6", recent, "outcome_settled_at = now(), outcome_settled_by = 'op', outcome_settlement = 'settled'") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("failed_settled_still_conflicts_before_the_settlement_migration", func(t *testing.T) { + // Prod may run this code before migration 000155: the settled filter must drop out, not error. + self, failed := seedFailed(t, "fam-f8", recent, "outcome_settled_at = now(), outcome_settled_by = 'op', outcome_settlement = 'settled'") + var version int64 + var dirty bool + require.NoError(t, store.db.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty)) + t.Cleanup(func() { + _, err := store.db.Exec(ctx, `UPDATE schema_migrations SET version = $1, dirty = $2`, version, dirty) + require.NoError(t, err) + }) + _, err := store.db.Exec(ctx, `UPDATE schema_migrations SET version = $1, dirty = false`, settlementMigrationVersion-1) + require.NoError(t, err) + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0"))) + }) + t.Run("same_family_failed_inside_settle_conflicts", func(t *testing.T) { + self, failed := seedFailed(t, "fam-f0:acct", recent, "") + got := findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0") + assert.Equal(t, []string{failed.ExecutionID}, conflictIDs(got)) + assert.True(t, got[0].SameFamily) + }) + t.Run("same_family_failed_after_settle_passes", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f9", recent, "") + // A plain INSERT keeps the old updated_at; the trigger would overwrite it on UPDATE. + old := time.Now().Add(-lineageTestSettle - time.Minute) + _, err := store.db.Exec(ctx, ` + INSERT INTO purchase_executions (status, step_number, scheduled_date, idempotency_key, first_dispatch_at, updated_at, recommendations) + VALUES ('failed', 1, NOW(), 'fam-f0:acct', $1, $1, $2::jsonb)`, old, + `[{"provider":"aws","service":"redshift","region":"us-east-1","resource_type":"ra3.xlplus","term":1,"payment":"all-upfront","count":2,"selected":true}]`) + require.NoError(t, err) + _, err = store.db.Exec(ctx, `DELETE FROM purchase_executions WHERE execution_id <> $1 AND idempotency_key = 'fam-f9'`, self.ExecutionID) + require.NoError(t, err) + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("same_family_linked_predecessor_bumped_by_link_passes", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f0:acct", recent, "retry_execution_id = $2") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("same_family_refused_no_submission_attempt1_passes", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f0:acct", recent, "no_submission = true, retry_attempt_n = 1") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("same_family_completed_inside_settle_conflicts", func(t *testing.T) { + _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) + require.NoError(t, err) + self := seedLineageRow(ctx, t, store, "fam-f0", "running", recent) + done := seedLineageRow(ctx, t, store, "fam-f0:acct", "completed", recent) + got := findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0") + assert.Equal(t, []string{done.ExecutionID}, conflictIDs(got)) + assert.True(t, got[0].SameFamily) + }) + t.Run("same_family_completed_after_settle_passes", func(t *testing.T) { + _, err := store.db.Exec(ctx, `DELETE FROM purchase_executions`) + require.NoError(t, err) + self := seedLineageRow(ctx, t, store, "fam-f0", "running", recent) + old := time.Now().Add(-lineageTestSettle - time.Minute) + _, err = store.db.Exec(ctx, ` + INSERT INTO purchase_executions (status, step_number, scheduled_date, idempotency_key, first_dispatch_at, updated_at, recommendations) + VALUES ('completed', 1, NOW(), 'fam-f0:acct', $1, $1, $2::jsonb)`, old, + `[{"provider":"aws","service":"redshift","region":"us-east-1","resource_type":"ra3.xlplus","term":1,"payment":"all-upfront","count":2,"selected":true}]`) + require.NoError(t, err) + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("same_family_failed_attempt0_no_submission_passes", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f0:acct", recent, "no_submission = true") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) + t.Run("failed_without_stamp_excluded", func(t *testing.T) { + self, _ := seedFailed(t, "fam-f7", nil, "") + assert.Empty(t, findLineageConflicts(ctx, t, store, self.ExecutionID, "fam-f0")) + }) +} diff --git a/internal/config/store_postgres_redshift_lineage_test.go b/internal/config/store_postgres_redshift_lineage_test.go new file mode 100644 index 000000000..e00b9a5eb --- /dev/null +++ b/internal/config/store_postgres_redshift_lineage_test.go @@ -0,0 +1,18 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRedshiftFailedClause(t *testing.T) { + ready, notReady := redshiftFailedClause(true), redshiftFailedClause(false) + + assert.Contains(t, ready, "outcome_settled_at IS NULL") + assert.NotContains(t, notReady, "outcome_settled_at", "the column does not exist before the settlement migration") + for _, want := range []string{"e.status = 'failed'", "e.retry_execution_id IS NULL", "NOT (e.no_submission AND e.retry_attempt_n = 0)", "e.first_dispatch_at > now()"} { + assert.Contains(t, ready, want) + assert.Contains(t, notReady, want) + } +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 44a569faa..8457c911e 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -576,9 +576,9 @@ func (m *MockConfigStore) LockRedshiftTuplesTx(ctx context.Context, tx pgx.Tx, t } // FindOpenRedshiftLineageConflictsTx mocks the Redshift lineage conflict query (no default: testify panics if unexpected). -func (m *MockConfigStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []config.RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window time.Duration) ([]config.RedshiftLineageConflict, error) { - m.record("FindOpenRedshiftLineageConflictsTx", ctx, tx, selfID, family, tuples, accountID, horizon, stamp, window) - args := m.Called(ctx, tx, selfID, family, tuples, accountID, horizon, stamp, window) +func (m *MockConfigStore) FindOpenRedshiftLineageConflictsTx(ctx context.Context, tx pgx.Tx, selfID, family string, tuples []config.RedshiftTuple, accountID *string, horizon time.Duration, stamp time.Time, window, settle time.Duration) ([]config.RedshiftLineageConflict, error) { + m.record("FindOpenRedshiftLineageConflictsTx", ctx, tx, selfID, family, tuples, accountID, horizon, stamp, window, settle) + args := m.Called(ctx, tx, selfID, family, tuples, accountID, horizon, stamp, window, settle) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index 6831dc369..0b5a91d93 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -112,7 +112,7 @@ func armedHarness(t *testing.T, serviceType common.ServiceType) (*Manager, *Mock Return(config.StampResult{At: &stampedAt, WithinHorizon: true}, nil).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).Return(nil, nil).Maybe() + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil).Maybe() email.On("SendPurchaseConfirmation", mock.Anything, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() factory.On("CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything).Return(prov, nil).Maybe() diff --git a/internal/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go index 2497716bf..1f71de861 100644 --- a/internal/purchase/aws_no_token_redrive_test.go +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -11,20 +11,21 @@ import ( "github.com/stretchr/testify/require" ) -// --- MON-02: AWS EC2 and Redshift have no duplicate guard -------------- +// --- MON-02: AWS EC2 has no duplicate guard ---------------------------- // -// The EC2 and Redshift purchase APIs take no client token or caller ID, so the -// provider sends them once and returns "purchase outcome unknown" when the -// response is lost. The commitment may exist, so nothing may buy again until -// someone has checked the account. These tests pin where that refusal reaches. +// The EC2 purchase API takes no client token or caller ID, so the provider +// sends it once and returns "purchase outcome unknown" when the response is +// lost. The commitment may exist, so nothing may buy again until someone has +// checked the account. These tests pin where that refusal reaches. Redshift is +// guarded by the lineage gate instead (TestRedshiftRedriveGuardLifted). -// awsNoTokenSlugs are the rec.Service spellings that dispatch to the AWS EC2 or -// Redshift purchase clients (mapServiceSlug: canonical and legacy). -var awsNoTokenSlugs = []string{"ec2", "compute", "redshift", "data-warehouse"} +// awsEC2Slugs are the rec.Service spellings that dispatch to the AWS EC2 +// purchase client (mapServiceSlug: canonical and legacy). +var awsEC2Slugs = []string{"ec2", "compute"} // TestAWSRedriveGuardReachMatchesDispatchReach states the guard as an iff // against dispatch, like the Azure savings-plans test: a value that dispatches -// to the EC2/Redshift client but is not refused is a double-buy hole. +// to the EC2 client but is not refused is a double-buy hole. func TestAWSRedriveGuardReachMatchesDispatchReach(t *testing.T) { m := NewManager(ManagerConfig{}) @@ -42,8 +43,7 @@ func TestAWSRedriveGuardReachMatchesDispatchReach(t *testing.T) { provider, service := provider, service t.Run("provider="+provider+"/service="+service, func(t *testing.T) { svc := m.mapServiceType(service) - dispatchesToNoTokenClient := svc == common.ServiceEC2 || svc == common.ServiceCompute || - svc == common.ServiceRedshift || svc == common.ServiceDataWarehouse + dispatchesToNoTokenClient := svc == common.ServiceEC2 || svc == common.ServiceCompute refused := recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}) != "" assert.Equal(t, dispatchesToNoTokenClient, refused, @@ -58,12 +58,12 @@ func TestAWSRedriveGuardReachMatchesDispatchReach(t *testing.T) { // compute on any provider) fails here. func TestAWSRedriveRefusalSetIsExact(t *testing.T) { for _, provider := range []string{"aws", ""} { - for _, service := range awsNoTokenSlugs { + for _, service := range awsEC2Slugs { reason := recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}) - assert.Contains(t, reason, "Check EC2 Reserved Instances or Redshift Reserved Nodes", + assert.Contains(t, reason, "Check EC2 Reserved Instances in the target account", "provider %q service %q has no duplicate guard and must be refused with the operator action", provider, service) } - for _, service := range []string{"rds", "relational-db", "elasticache", "cache", "memorydb", "opensearch", "search", "savingsplans", "savings-plans"} { + for _, service := range []string{"rds", "relational-db", "elasticache", "cache", "memorydb", "opensearch", "search", "savingsplans", "savings-plans", "redshift", "data-warehouse"} { assert.Empty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}), "provider %q service %q has a provider-side duplicate guard and must stay retryable", provider, service) } @@ -77,19 +77,34 @@ func TestAWSRedriveRefusalSetIsExact(t *testing.T) { } // Any one unsafe rec makes the whole execution unsafe. mixed := &config.PurchaseExecution{Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "rds"}, {Provider: "aws", Service: "redshift"}, + {Provider: "aws", Service: "rds"}, {Provider: "aws", Service: "ec2"}, }} assert.NotEmpty(t, RedriveRefusalReason(mixed)) assert.False(t, allRecsSafeToRedrive(&config.PurchaseExecution{ExecutionID: "x", Recommendations: mixed.Recommendations})) } +// TestRedshiftRedriveGuardLifted pins that Redshift is no longer refused by the provider-level +// re-drive guard: the Retry gate and the dispatch gate decide, so the armed backstop is silent. +func TestRedshiftRedriveGuardLifted(t *testing.T) { + for _, provider := range []string{"aws", ""} { + for _, service := range []string{"redshift", "data-warehouse"} { + rec := config.RecommendationRecord{Provider: provider, Service: service} + assert.Empty(t, recRedriveRefusalReason(rec), "provider %q service %q", provider, service) + + 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") + } + } +} + // TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes drives the real -// executor funnel: a retry successor (RetryAttemptN > 0) of an EC2/Redshift +// executor funnel: a retry successor (RetryAttemptN > 0) of an EC2 // purchase buys nothing and is persisted as failed with the reason, while a // first purchase of the same service still buys exactly once. func TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes(t *testing.T) { m := NewManager(ManagerConfig{}) - for _, service := range awsNoTokenSlugs { + for _, service := range awsEC2Slugs { service := service t.Run(service, func(t *testing.T) { svcType := m.mapServiceType(service) @@ -108,7 +123,7 @@ func TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes(t *testing.T) require.NotEmpty(t, saved) final := saved[len(saved)-1] assert.Equal(t, "failed", final.Status) - assert.Contains(t, final.Error, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + assert.Contains(t, final.Error, "Check EC2 Reserved Instances in the target account") first := armedExecution("aws", service, 0, "notified") mgr, store, rec = armedHarness(t, svcType) @@ -191,7 +206,7 @@ func TestApproveAndExecuteRefusesAWSNoTokenRetrySuccessor(t *testing.T) { m := NewManager(ManagerConfig{}) for _, path := range paths { - for _, service := range append(append([]string{}, awsNoTokenSlugs...), "rds") { + for _, service := range append(append([]string{}, awsEC2Slugs...), "rds") { path, service := path, service refused := service != "rds" t.Run(path.name+"/"+service, func(t *testing.T) { @@ -228,7 +243,7 @@ func TestApproveAndExecuteRefusesAWSNoTokenRetrySuccessor(t *testing.T) { require.NotEmpty(t, saved, "the refused row must be persisted, not left running") final := saved[len(saved)-1] assert.Equal(t, "failed", final.Status) - assert.Contains(t, final.Error, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + assert.Contains(t, final.Error, "Check EC2 Reserved Instances in the target account") store.AssertExpectations(t) }) } diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 561045750..35f1c4168 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -38,7 +38,10 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftConflictRefusal), regexp.QuoteMeta(redshiftHorizonRefusal), regexp.QuoteMeta(redshiftNoStampRefusal), + regexp.QuoteMeta(redshiftFailedConflictRefusal), + regexp.QuoteMeta(redshiftNoJournalRefusal), regexp.QuoteMeta(redshiftRecentBuyRefusal), + regexp.QuoteMeta(redshiftSettleRefusal), `EC2 recommendation for ` + resourceTypePattern + ` has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, `zonal RI offering needs an availability zone; re-fetch the recommendation`, `email notifier not configured for this deployment`, @@ -118,7 +121,7 @@ var exchangeNoteHeads = []string{ `unknown provider`, } -var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + regexp.QuoteMeta(redriveReasonAzureSavingsPlan) + +var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + regexp.QuoteMeta(redriveReasonAWSNoGuardLegacy) + `|` + regexp.QuoteMeta(redriveReasonAzureSavingsPlan) + `|provider "[A-Za-z0-9._+-]{0,32}" is not known to reject a duplicate purchase, so re-driving this could buy a second commitment` // resourceTypePattern is the ": " head the executor's per-rec join diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 562f9cf2e..626e9494a 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -27,6 +27,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { strandedRecoveryError(&config.PurchaseExecution{}), strandedRecoveryError(retry), armedRedriveRefusal(retry).Error(), + "refusing to execute retry attempt 2: " + redriveReasonAWSNoGuardLegacy, armedRedriveRefusal(azSP).Error(), armedRedriveRefusal(gcpRetry).Error(), ErrStaleAzurePricing.Error(), @@ -55,7 +56,10 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, + redshiftFailedConflictRefusal, + redshiftNoJournalRefusal, redshiftRecentBuyRefusal, + redshiftSettleRefusal, "EC2 recommendation for m5.large has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", "zonal RI offering needs an availability zone; re-fetch the recommendation", "email notifier not configured for this deployment", @@ -208,7 +212,7 @@ func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { // The Redshift gate texts are fixed: an appended execution id or raw text must // not pass verbatim. func TestSanitizeStoredError_RedshiftGateRefusalsAreClosed(t *testing.T) { - for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftRecentBuyRefusal} { + for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal} { assert.Equal(t, text, SanitizeStoredError(text)) for _, variant := range []string{ text + " (execution 00000000-0000-0000-0000-000000000000, running)", diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 1965e0f85..23000525f 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -449,7 +449,7 @@ const ( ) // skippedAccountNote words the skip row by whether Retry accepts it: -// RedriveRefusalReason refuses EC2, Redshift and Azure savings plans whatever +// RedriveRefusalReason refuses EC2 and Azure savings plans whatever // NoSubmission says, so those rows point at a new purchase instead. func skippedAccountNote(row *config.PurchaseExecution) string { if RedriveRefusalReason(row) != "" { diff --git a/internal/purchase/execution_skipped_accounts_test.go b/internal/purchase/execution_skipped_accounts_test.go index 1b803c6a1..73c9dc49e 100644 --- a/internal/purchase/execution_skipped_accounts_test.go +++ b/internal/purchase/execution_skipped_accounts_test.go @@ -157,9 +157,9 @@ func TestSkippedAccountNote_MatchesRetryEligibility(t *testing.T) { want string }{ {"aws", "ec2", skippedAccountNoRetryNote}, - {"aws", "redshift", skippedAccountNoRetryNote}, {"azure", "savingsplans", skippedAccountNoRetryNote}, {"aws", "rds", skippedAccountRetryNote}, + {"aws", "redshift", skippedAccountRetryNote}, {"gcp", "compute", skippedAccountRetryNote}, } for _, c := range cases { diff --git a/internal/purchase/idempotency_key_writers_test.go b/internal/purchase/idempotency_key_writers_test.go index 0b26dca75..497fbc55d 100644 --- a/internal/purchase/idempotency_key_writers_test.go +++ b/internal/purchase/idempotency_key_writers_test.go @@ -97,8 +97,9 @@ func idempotencyKeyWriters(f *ast.File, dir string) []string { var ( insertPurchaseExecutions = regexp.MustCompile(`(?is)\bINSERT\s+INTO\s+purchase_executions\b`) - updatePurchaseExecutions = regexp.MustCompile(`(?is)\bUPDATE\s+purchase_executions\b.*?\bSET\b(.*?)(?:\bWHERE\b|\bRETURNING\b|;|$)`) - upsertSet = regexp.MustCompile(`(?is)\bDO\s+UPDATE\s+SET\b(.*?)(?:\bWHERE\b|\bRETURNING\b|;|$)`) + updatePurchaseExecutions = regexp.MustCompile(`(?is)\bUPDATE\s+purchase_executions\b.*?\bSET\b`) + upsertSet = regexp.MustCompile(`(?is)\bDO\s+UPDATE\s+SET\b`) + setTerminator = regexp.MustCompile(`(?i)^(WHERE|RETURNING)\b`) idempotencyKeyColumn = regexp.MustCompile(`(?i)\bidempotency_key\b`) ) @@ -119,6 +120,29 @@ func columnList(rest string) (cols, tail string, ok bool) { return "", "", false } +// setText returns the SET clause text in rest, cut at the first WHERE, RETURNING or ";" +// outside parentheses, so a subselect's WHERE does not hide a later assignment. +func setText(rest string) string { + depth := 0 + for i, r := range rest { + switch { + case r == '(': + depth++ + case r == ')': + depth-- + case depth == 0 && r == ';': + return rest[:i] + case depth == 0 && (i == 0 || !isWordByte(rest[i-1])) && setTerminator.MatchString(rest[i:]): + return rest[:i] + } + } + return rest +} + +func isWordByte(b byte) bool { + return b == '_' || b >= '0' && b <= '9' || b >= 'a' && b <= 'z' || b >= 'A' && b <= 'Z' +} + // sqlIdempotencyKeyWriters counts purchase_executions INSERTs carrying idempotency_key // (no column list counts) and UPDATEs or upserts that SET it; dynamic counts unverifiable INSERTs. func sqlIdempotencyKeyWriters(sql string) (inserts, updates, dynamic int) { @@ -139,14 +163,14 @@ func sqlIdempotencyKeyWriters(sql string) (inserts, updates, dynamic int) { if end := strings.Index(tail, ";"); end >= 0 { tail = tail[:end] } - for _, m := range upsertSet.FindAllStringSubmatch(tail, -1) { - if idempotencyKeyColumn.MatchString(m[1]) { + for _, loc := range upsertSet.FindAllStringIndex(tail, -1) { + if idempotencyKeyColumn.MatchString(setText(tail[loc[1]:])) { updates++ } } } - for _, m := range updatePurchaseExecutions.FindAllStringSubmatch(sql, -1) { - if idempotencyKeyColumn.MatchString(m[1]) { + for _, loc := range updatePurchaseExecutions.FindAllStringIndex(sql, -1) { + if idempotencyKeyColumn.MatchString(setText(sql[loc[1]:])) { updates++ } } @@ -308,6 +332,8 @@ func TestIdempotencyKeySQLScan_Detects(t *testing.T) { {"upsert rewrites the key from a parameter", `INSERT INTO purchase_executions (execution_id) VALUES ($1) ON CONFLICT (execution_id) DO UPDATE SET status = $2, idempotency_key = $3 WHERE true`, 0, 1, 0}, {"update row constructor", `UPDATE purchase_executions SET (idempotency_key, status) = ($2, $3) WHERE execution_id = $1`, 0, 1, 0}, {"update key set after a FROM inside parentheses", `UPDATE purchase_executions SET status = (SELECT s FROM t), idempotency_key = $2 WHERE execution_id = $1`, 0, 1, 0}, + {"update key set after a WHERE inside parentheses", `UPDATE purchase_executions SET x = (SELECT a FROM t WHERE b = 1), idempotency_key = $2 WHERE id = $1`, 0, 1, 0}, + {"upsert key set after a WHERE inside parentheses", `INSERT INTO purchase_executions (execution_id) VALUES ($1) ON CONFLICT (execution_id) DO UPDATE SET x = (SELECT a FROM t WHERE b = 1), idempotency_key = $2`, 0, 1, 0}, {"insert column list built with Sprintf", `INSERT INTO purchase_executions (%s) VALUES (%s)`, 0, 0, 1}, {"insert column list unclosed by concatenation", `INSERT INTO purchase_executions (execution_id, `, 0, 0, 1}, {"insert column list empty before concatenation", `INSERT INTO purchase_executions (`, 0, 0, 1}, diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 41f9a2688..5e0d915b4 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -415,8 +415,8 @@ func (m *Manager) saveSkippedRows(ctx context.Context, root *config.PurchaseExec } // preExecutionRefusal runs every check that must pass before any provider call. -// All refusals except the armed re-drive one are beforeSubmission errors, so -// the row ends failed with no_submission stamped. +// 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 { return err @@ -427,6 +427,10 @@ func (m *Manager) preExecutionRefusal(ctx context.Context, exec *config.Purchase if refusal := m.ladderExecutionRefusal(ctx, exec); refusal != nil { return beforeSubmission(refusal) } + // A Redshift retry reuses the first attempt's request time; only the journal lets go's dedupe tell its node from another's. + if hasRedshiftRec(exec) && exec.RetryAttemptN > 0 && m.attemptJournal == nil { + return errors.New(redshiftNoJournalRefusal) + } return nil } @@ -597,6 +601,9 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { // - AWS RDS, ElastiCache, MemoryDB, OpenSearch and Savings Plans: a // deterministic reservation ID or ClientToken collapses a repeat onto the // first attempt (#636/#638). +// - AWS Redshift: the Retry gate (checkRedshiftRetryGate) and the dispatch gate +// (stampRedshiftDispatch) reuse the first attempt's request time, so go's +// dedupe adopts a node already bought; both refuse when that cannot be proven. // - Azure reservations (compute, relational-db, cache, nosql, memorydb, // search, data-warehouse): DoIdempotentPurchaseTwoStep performs a // tag-based lookup before purchasing (#729 / #721). @@ -604,10 +611,10 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { // the token (#654). // // NOT safe: -// - AWS EC2 and Redshift (slugs ec2, compute, redshift, data-warehouse): the -// purchase APIs take no client token, so a lost response may have bought the -// commitment and a second attempt would buy another (MON-02). The provider -// sends the call once and reports "purchase outcome unknown". +// - AWS EC2 (slugs ec2, compute): the purchase API takes no client token, so a +// lost response may have bought the commitment and a second attempt would buy +// another (MON-02). The provider sends the call once and reports "purchase +// outcome unknown". // - Azure savings-plans: the pinned client names the order alias after the // idempotency token when one is set (a timestamp name only on the // empty-token CLI path), but no test or provider doc here confirms that a @@ -640,24 +647,23 @@ func RedriveRefusalReason(exec *config.PurchaseExecution) string { // Fixed refusal texts, shared with the stored-error allowlist (errnotes.go). const ( - redriveReasonAWSNoGuard = "AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" + redriveReasonAWSNoGuard = "AWS EC2 purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances in the target account and region; if it is missing, start a new purchase instead of retrying" + // Stored by armed-refusal rows written before Redshift re-drive was lifted; errnotes only. + redriveReasonAWSNoGuardLegacy = "AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" redriveReasonAzureSavingsPlan = "Azure savings plans have no provider-side duplicate guard, so re-driving this purchase would buy a second savings plan that cannot be canceled" ) -// 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. +// recRedriveRefusalReason returns why one recommendation cannot be safely re-driven, or "" +// when it can; split out of RedriveRefusalReason for the gocyclo budget. func recRedriveRefusalReason(rec config.RecommendationRecord) string { switch rec.Provider { case "", "aws": // Empty provider is legacy AWS. RDS, ElastiCache, MemoryDB, OpenSearch - // and Savings Plans collapse a repeat onto the first attempt. EC2 and - // Redshift cannot: their purchase APIs take no client token or caller - // ID, and the provider sends them once and reports a lost response as - // "purchase outcome unknown". The commitment may exist, so a second - // attempt must not be made until someone has checked the account. - if awsPurchaseHasNoDuplicateGuard(rec.Service) { + // and Savings Plans collapse a repeat onto the first attempt. Redshift + // is guarded by the journaled lineage gate (redshift_lineage.go). EC2 + // takes no client token or caller ID, so a lost response may hide a + // bought commitment and a second attempt waits for someone to check. + if awsPurchaseIsEC2(rec.Service) { return redriveReasonAWSNoGuard } return "" @@ -679,19 +685,15 @@ func recRedriveRefusalReason(rec config.RecommendationRecord) string { } } -// awsPurchaseHasNoDuplicateGuard reports whether a recommendation's service -// slug dispatches to the AWS EC2 or Redshift purchase path. The canonical -// slugs (compute, data-warehouse) and the legacy ones (ec2, redshift) reach the -// same client, so all four must be covered or the refusal can be bypassed by -// spelling. Matching goes through mapServiceSlug, the same table the executor -// dispatches on, so the two cannot drift. -func awsPurchaseHasNoDuplicateGuard(service string) bool { +// awsPurchaseIsEC2 reports whether a service slug (ec2 or compute) dispatches to the EC2 path, +// matching through mapServiceSlug, the executor's own table, so spellings cannot drift. +func awsPurchaseIsEC2(service string) bool { svc, ok := mapServiceSlug(service) if !ok { return false } switch svc { - case common.ServiceEC2, common.ServiceCompute, common.ServiceRedshift, common.ServiceDataWarehouse: + case common.ServiceEC2, common.ServiceCompute: return true default: return false diff --git a/internal/purchase/ops_hint.go b/internal/purchase/ops_hint.go new file mode 100644 index 000000000..67f8d1c20 --- /dev/null +++ b/internal/purchase/ops_hint.go @@ -0,0 +1,43 @@ +package purchase + +import "strings" + +// persistentFailureHints maps known-persistent failure substrings to +// the operator-actionable hint surfaced on the History row in place of +// the Retry button (issue #47, Q3). The match is a case-INsensitive +// substring contains check (see ResolveOpsHint); wording variations +// like "ses sandbox" / "SES Sandbox" all hit. Keep needles short and +// distinctive so legitimate transient failures don't accidentally +// substring-match a persistent hint and lock the user out. +// +// Membership criteria: only failures that NO retry can possibly fix; +// they require an operator to change configuration or unblock an +// upstream resource. SES sandbox / unverified domain / missing +// FROM_EMAIL all fit; a transient SES throttle does NOT (the next +// retry might succeed). When in doubt, leave it out: false-negatives +// cost a wasted retry; false-positives lock the user out of an +// actionable button entirely. +var persistentFailureHints = map[string]string{ + "FROM_EMAIL not configured": "Set FROM_EMAIL tfvar then retry", + "SES sandbox": "Move SES out of sandbox or verify recipient, then retry", + "SES domain not verified": "Verify SES domain in AWS console, then retry", + "IAM denied": "Grant the deploy role missing IAM permission, then retry", +} + +// ResolveOpsHint returns a non-empty hint when the failure reason +// matches a known-persistent pattern, else empty. Match is case- +// insensitive substring contains so backend wording variations don't +// silently slip through ("ses sandbox" / "SES sandbox" / "SES Sandbox" +// all hit). +func ResolveOpsHint(failureReason string) string { + if failureReason == "" { + return "" + } + lower := strings.ToLower(failureReason) + for needle, hint := range persistentFailureHints { + if strings.Contains(lower, strings.ToLower(needle)) { + return hint + } + } + return "" +} diff --git a/internal/purchase/ops_hint_test.go b/internal/purchase/ops_hint_test.go new file mode 100644 index 000000000..f95a56b09 --- /dev/null +++ b/internal/purchase/ops_hint_test.go @@ -0,0 +1,27 @@ +package purchase + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestResolveOpsHint(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + {"empty input", "", ""}, + {"transient SES throttle (no match)", "send failed: SES throttle exceeded", ""}, + {"FROM_EMAIL exact", "FROM_EMAIL not configured for this deployment", "Set FROM_EMAIL tfvar then retry"}, + {"SES sandbox case-insensitive", "send failed: ses sandbox active", "Move SES out of sandbox or verify recipient, then retry"}, + {"domain not verified", "send failed: SES domain not verified", "Verify SES domain in AWS console, then retry"}, + {"IAM denied", "AssumeRole error: IAM denied", "Grant the deploy role missing IAM permission, then retry"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, ResolveOpsHint(tt.input)) + }) + } +} diff --git a/internal/purchase/plan_step_proofs_test.go b/internal/purchase/plan_step_proofs_test.go index 80479ed7e..580ee8956 100644 --- a/internal/purchase/plan_step_proofs_test.go +++ b/internal/purchase/plan_step_proofs_test.go @@ -150,13 +150,13 @@ func TestRedriveRefusedWhereNoProviderGuardExists(t *testing.T) { for _, rec := range []config.RecommendationRecord{ {Provider: "azure", Service: "savingsplans"}, {Provider: "aws", Service: "ec2"}, - {Provider: "aws", Service: "redshift"}, {Provider: "oracle", Service: "compute"}, } { assert.NotEmpty(t, recRedriveRefusalReason(rec), "%s/%s", rec.Provider, rec.Service) } for _, rec := range []config.RecommendationRecord{ {Provider: "aws", Service: "rds"}, + {Provider: "aws", Service: "redshift"}, {Provider: "azure", Service: "compute"}, {Provider: "gcp", Service: "compute"}, } { diff --git a/internal/purchase/reaper.go b/internal/purchase/reaper.go index 5af98fc79..13980b756 100644 --- a/internal/purchase/reaper.go +++ b/internal/purchase/reaper.go @@ -275,7 +275,8 @@ func (m *Manager) reapOne(ctx context.Context, exec *config.PurchaseExecution, r // confirmed to deduplicate a repeat order-alias PUT (RedriveRefusalReason), // so retrying a reaped Azure SP row risks creating a duplicate commitment. safeMsg := "" - if allRecsSafeToRedrive(exec) { + // A reaped Redshift row may have sent; its Retry gate decides, so the note makes no promise. + if allRecsSafeToRedrive(exec) && !hasRedshiftRec(exec) { safeMsg = "; safe to retry" } transitioned.Error = fmt.Sprintf("reaped after %dm in %s state — executor did not complete%s", diff --git a/internal/purchase/reaper_test.go b/internal/purchase/reaper_test.go index 97da30787..3f9ef7328 100644 --- a/internal/purchase/reaper_test.go +++ b/internal/purchase/reaper_test.go @@ -38,8 +38,8 @@ func stuckExec(id, status string) config.PurchaseExecution { // stuckExecWithAWSRecs builds a stuck execution with a single AWS RDS rec. // RDS honors a deterministic reservation ID, so allRecsSafeToRedrive is true and -// the reaper's canonical error includes "safe to retry" (F4 fix). EC2 and -// Redshift are the AWS services that are not safe; see stuckExecWithAWSEC2Rec. +// the reaper's canonical error includes "safe to retry" (F4 fix). EC2 is the AWS +// service that is not safe; see stuckExecWithAWSEC2Rec. func stuckExecWithAWSRecs(id, status string) config.PurchaseExecution { e := stuckExec(id, status) e.Recommendations = []config.RecommendationRecord{ @@ -119,6 +119,35 @@ func TestReapStuckExecutions_EC2RowIsNotAdvertisedAsSafeToRetry(t *testing.T) { assert.NotContains(t, saved.Error, "safe to retry") } +// A reaped Redshift row may have sent, and its Retry gate can still refuse it, so the note +// must not promise a safe retry even though Redshift is no longer refused as a provider. +func TestReapedRedshiftRowDoesNotSayRetryIsSafe(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + assertMocksOnCleanup(t, store) + reapAfter := 10 * time.Minute + + row := stuckExec("exec-rs", "running") + row.Recommendations = []config.RecommendationRecord{ + {Provider: "aws", Service: "redshift", ResourceType: "ra3.xlplus", Count: 1, Selected: true}, + } + transitioned := row + transitioned.Status = failedStatus + expectStuckLists(ctx, store, reapAfter, row) + store.On("TransitionExecutionStatus", ctx, "exec-rs", []string{"running"}, failedStatus, (*string)(nil)).Return(&transitioned, nil) + var saved *config.PurchaseExecution + store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }).Return(nil) + + result, err := newReaperManager(store).ReapStuckExecutions(ctx, reapAfter) + + require.NoError(t, err) + assert.Equal(t, 1, result.Reaped) + require.NotNil(t, saved) + assert.Contains(t, saved.Error, "reaped after") + assert.NotContains(t, saved.Error, "safe to retry") +} + func TestReapStuckExecutions_StaleRunningFlippedToFailed(t *testing.T) { ctx := context.Background() store := new(MockConfigStore) diff --git a/internal/purchase/redshift_gate_integration_test.go b/internal/purchase/redshift_gate_integration_test.go index 19e1d1ab6..c75ce7ddd 100644 --- a/internal/purchase/redshift_gate_integration_test.go +++ b/internal/purchase/redshift_gate_integration_test.go @@ -227,9 +227,8 @@ func TestRedshiftDispatchGate_SecondLineageAfterWindowDispatches(t *testing.T) { assert.Equal(t, config.StatusCompleted, rowStatus(ctx, t, f.rampStepFixture, later.ExecutionID).Status) } -// A completed row of the same family (a retry's earlier per-account row) is -// not another lineage, so the retry successor is not refused. -func TestRedshiftDispatchGate_SameFamilyCompletedRowNotRefused(t *testing.T) { +// A same-family completed row written inside the settle window may not be visible to AWS yet. +func TestRedshiftDispatchGate_SameFamilyCompletedRowRefusedInsideSettle(t *testing.T) { ctx := context.Background() f := newRedshiftGateFixture(ctx, t) family := uuid.New().String() @@ -239,6 +238,34 @@ func TestRedshiftDispatchGate_SameFamilyCompletedRowNotRefused(t *testing.T) { claimed, _ := f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(successor), 0) require.True(t, claimed) - assert.NotEqual(t, redshiftRecentBuyRefusal, SanitizeStoredError(rowStatus(ctx, t, f.rampStepFixture, successor.ExecutionID).Error)) + assert.Zero(t, f.bought.Load(), "a refused successor never reaches the provider") + assert.Equal(t, redshiftSettleRefusal, SanitizeStoredError(rowStatus(ctx, t, f.rampStepFixture, successor.ExecutionID).Error)) +} + +// Once the same-family completed row is older than the settle, the retry successor is not refused. +func TestRedshiftDispatchGate_SameFamilyCompletedRowNotRefusedAfterSettle(t *testing.T) { + ctx := context.Background() + f := newRedshiftGateFixture(ctx, t) + family := uuid.New().String() + key := family + ":" + f.accounts[0].ID + f.finishedRow(ctx, t, key) + // The updated_at trigger overwrites any UPDATE, so switch user triggers off for this backdating. + tx, err := f.pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, err = tx.Exec(ctx, `ALTER TABLE purchase_executions DISABLE TRIGGER USER`) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE purchase_executions SET updated_at = now() - $2 * interval '1 second' WHERE idempotency_key = $1`, + key, (redshiftRetrySettle + time.Minute).Seconds()) + require.NoError(t, err) + _, err = tx.Exec(ctx, `ALTER TABLE purchase_executions ENABLE TRIGGER USER`) + require.NoError(t, err) + require.NoError(t, tx.Commit(ctx)) + successor := f.redshiftRowKeyed(ctx, t, "notified", family, true) + + claimed, _ := f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(successor), 0) + require.True(t, claimed) + + assert.NotEqual(t, redshiftSettleRefusal, SanitizeStoredError(rowStatus(ctx, t, f.rampStepFixture, successor.ExecutionID).Error)) assert.Positive(t, f.bought.Load(), "the same-family retry reaches the provider") } diff --git a/internal/purchase/redshift_gate_test.go b/internal/purchase/redshift_gate_test.go index 4584e9cd0..5009e42d7 100644 --- a/internal/purchase/redshift_gate_test.go +++ b/internal/purchase/redshift_gate_test.go @@ -38,7 +38,7 @@ func runGate(t *testing.T, exec *config.PurchaseExecution, stamp config.StampRes Run(func(a mock.Arguments) { run.locked = a.Get(2).([]config.RedshiftTuple) }).Return(nil) store.On("StampFirstDispatchTx", mock.Anything, mock.Anything, exec.ExecutionID, redshiftRedriveHorizon).Return(stamp, nil) store.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, exec.ExecutionID, mock.Anything, - mock.Anything, mock.Anything, redshiftRedriveHorizon, mock.Anything, mock.Anything). + mock.Anything, mock.Anything, redshiftRedriveHorizon, mock.Anything, mock.Anything, mock.Anything). Run(func(a mock.Arguments) { run.family = a.String(3) run.finds = a.Get(4).([]config.RedshiftTuple) @@ -103,6 +103,97 @@ func TestRedshiftDispatchGate_FinishedConflictWinsOverOpenOne(t *testing.T) { assert.Equal(t, redshiftRecentBuyRefusal, run.err.Error()) } +func TestRedshiftDispatchGate_SameFamilyRecentFailureRefusesWithSettleText(t *testing.T) { + rs := []config.RecommendationRecord{gateRec("acct-A", 2)} + sameFailed := config.RedshiftLineageConflict{ExecutionID: "sib", Status: config.StatusFailed, SameFamily: true, Recommendations: rs} + + exec := gateExec(1, gateRec("acct-A", 2)) + run := runGate(t, exec, stamped(), []config.RedshiftLineageConflict{sameFailed}) + require.Error(t, run.err) + assert.True(t, isBeforeSubmission(run.err)) + assert.Equal(t, redshiftSettleRefusal, run.err.Error()) + assert.Nil(t, exec.FirstDispatchAt, "a refused dispatch records no stamp") + + run = runGate(t, gateExec(1, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ + sameFailed, {ExecutionID: "done", Status: config.StatusCompleted, Recommendations: rs}}) + require.Error(t, run.err) + assert.Equal(t, redshiftRecentBuyRefusal, run.err.Error(), "a recent finished buy wins over the settle text") +} + +func TestRedshiftDispatchGate_SameFamilyFinishedRefusesWithSettleText(t *testing.T) { + rs := []config.RecommendationRecord{gateRec("acct-A", 2)} + done := config.RedshiftLineageConflict{ExecutionID: "sib", Status: config.StatusCompleted, SameFamily: true, Recommendations: rs} + run := runGate(t, gateExec(1, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{done}) + require.Error(t, run.err) + assert.Equal(t, redshiftSettleRefusal, run.err.Error()) + + run = runGate(t, gateExec(1, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ + done, {ExecutionID: "live", Status: "running", SameFamily: true}}) + require.Error(t, run.err) + assert.Equal(t, redshiftConflictRefusal, run.err.Error(), "an open row wins over the settle text") +} + +func TestRedshiftDispatchGate_SameFamilyFailureWithHintIsNotDropped(t *testing.T) { + run := runGate(t, gateExec(1, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ + {ExecutionID: "sib", Status: config.StatusFailed, SameFamily: true, Error: "send failed: SES sandbox active"}}) + require.Error(t, run.err) + assert.Equal(t, redshiftSettleRefusal, run.err.Error()) +} + +func TestRedshiftDispatchGate_PersistentFailureCandidateDropped(t *testing.T) { + exec := gateExec(0, gateRec("acct-A", 2)) + run := runGate(t, exec, stamped(), []config.RedshiftLineageConflict{ + {ExecutionID: "stuck", Status: config.StatusFailed, Error: "send failed: SES sandbox active"}}) + + require.NoError(t, run.err, "History hides Retry for this row, so it must not hold the tuple") + assert.NotNil(t, exec.FirstDispatchAt) +} + +func TestRedshiftDispatchGate_RunningRowWithHintTextStillConflicts(t *testing.T) { + run := runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ + {ExecutionID: "live", Status: "running", Error: "SES sandbox"}}) + + require.Error(t, run.err) + assert.Equal(t, redshiftConflictRefusal, run.err.Error()) +} + +func TestRedshiftDispatchGate_DefiniteRefusalFailedRowStillConflicts(t *testing.T) { + run := runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ + {ExecutionID: "refused", Status: config.StatusFailed, Error: redshiftConflictRefusal, Recommendations: []config.RecommendationRecord{gateRec("acct-A", 2)}}}) + + require.Error(t, run.err) + assert.Equal(t, redshiftFailedConflictRefusal, run.err.Error()) + assert.True(t, isBeforeSubmission(run.err)) +} + +func TestRedshiftDispatchGate_PicksTheRefusalTextByConflictKind(t *testing.T) { + rs := []config.RecommendationRecord{gateRec("acct-A", 2)} + failed := config.RedshiftLineageConflict{ExecutionID: "f", Status: config.StatusFailed, Recommendations: rs} + running := config.RedshiftLineageConflict{ExecutionID: "r", Status: "running", Recommendations: rs} + done := config.RedshiftLineageConflict{ExecutionID: "d", Status: config.StatusCompleted, Recommendations: rs} + for name, tc := range map[string]struct { + conflicts []config.RedshiftLineageConflict + want string + }{ + "only failed": {[]config.RedshiftLineageConflict{failed}, redshiftFailedConflictRefusal}, + "failed beside running": {[]config.RedshiftLineageConflict{failed, running}, redshiftConflictRefusal}, + "running": {[]config.RedshiftLineageConflict{running}, redshiftConflictRefusal}, + "finished wins": {[]config.RedshiftLineageConflict{failed, running, done}, redshiftRecentBuyRefusal}, + } { + run := runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), tc.conflicts) + require.Error(t, run.err, name) + assert.Equal(t, tc.want, run.err.Error(), name) + } +} + +func TestRedshiftDispatchGate_FailedRowWithRefusedProviderRecDropped(t *testing.T) { + both := []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}, gateRec("acct-A", 2)} + run := runGate(t, gateExec(0, gateRec("acct-A", 2)), stamped(), []config.RedshiftLineageConflict{ + {ExecutionID: "mixed", Status: config.StatusFailed, Recommendations: both}}) + + require.NoError(t, run.err, "Retry refuses the EC2 rec for good, so the row cannot be retried") +} + func TestRedshiftDispatchGate_QueriesOnlySelectedRedshiftTuples(t *testing.T) { unselected := gateRec("acct-A", 7) unselected.Selected = false @@ -173,7 +264,7 @@ func TestRedshiftDispatchGate_StoreErrorsByAttempt(t *testing.T) { store.On("LockRedshiftTuplesTx", mock.Anything, mock.Anything, mock.Anything).Return(lockErr) store.On("StampFirstDispatchTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(stamped(), nil).Maybe() store.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything, - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, findErr).Maybe() + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, findErr).Maybe() exec := gateExec(attempt, gateRec("acct-A", 2)) err := (&Manager{config: store}).stampRedshiftDispatch(context.Background(), exec) @@ -192,7 +283,7 @@ func TestRedshiftDispatchGate_CallOrderIsLockStampFind(t *testing.T) { store.On("LockRedshiftTuplesTx", mock.Anything, mock.Anything, mock.Anything).Run(note("lock")).Return(nil) store.On("StampFirstDispatchTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(note("stamp")).Return(stamped(), nil) store.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything, - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(note("find")).Return(nil, nil) + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(note("find")).Return(nil, nil) require.NoError(t, (&Manager{config: store}).stampRedshiftDispatch(context.Background(), gateExec(0, gateRec("acct-A", 2)))) assert.Equal(t, []string{"lock", "stamp", "find"}, order) diff --git a/internal/purchase/redshift_journal_guard_test.go b/internal/purchase/redshift_journal_guard_test.go new file mode 100644 index 000000000..529defb01 --- /dev/null +++ b/internal/purchase/redshift_journal_guard_test.go @@ -0,0 +1,61 @@ +package purchase + +import ( + "context" + "testing" + "time" + + "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/require" +) + +func journalGuardExec(attempt int, rec config.RecommendationRecord) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: "rs-retry", IdempotencyKey: "rs-lineage", Status: "running", RetryAttemptN: attempt, + Recommendations: []config.RecommendationRecord{rec}, + } +} + +func TestRedshiftRetryDispatch_NilJournalRefused(t *testing.T) { + at := time.Now().UTC().Add(-time.Hour) + exec := journalGuardExec(1, redshiftScopedRec("acct-A")) + run := runRedshiftFinalize(t, exec, nil, common.ServiceRedshift, stampAt(at)) + + require.Error(t, run.err) + assert.Equal(t, redshiftNoJournalRefusal, run.err.Error()) + assert.False(t, isBeforeSubmission(run.err), "an earlier attempt may have sent, so no_submission must stay false") + assert.False(t, exec.NoSubmission) + assert.Equal(t, "failed", exec.Status) + assert.Empty(t, run.opts, "no PurchaseCommitment call") + assert.Empty(t, run.stampedIDs, "no StampFirstDispatchTx call") +} + +func TestRedshiftRetryDispatch_WithJournalDispatches(t *testing.T) { + at := time.Now().UTC().Add(-time.Hour).Truncate(time.Microsecond) + journal := &fakeAttemptJournal{} + run := runRedshiftFinalizeWithJournal(t, journalGuardExec(1, redshiftScopedRec("acct-A")), nil, common.ServiceRedshift, stampAt(at), journal) + + require.NoError(t, run.err) + require.Len(t, run.opts, 1) + assert.Same(t, journal, run.opts[0].AttemptJournal) + assert.True(t, run.opts[0].RequestedAt.Equal(at)) +} + +func TestRedshiftAttempt0Dispatch_NilJournalStillDispatches(t *testing.T) { + at := time.Now().UTC().Add(-time.Minute) + run := runRedshiftFinalize(t, journalGuardExec(0, redshiftScopedRec("acct-A")), nil, common.ServiceRedshift, stampAt(at)) + + require.NoError(t, run.err) + require.Len(t, run.opts, 1) + assert.Nil(t, run.opts[0].AttemptJournal) +} + +func TestNonRedshiftRetryDispatch_NilJournalUnaffected(t *testing.T) { + rds := scopedTestRec("acct-A") + rds.Service = "rds" + m := &Manager{config: new(MockConfigStore)} + + assert.NoError(t, m.preExecutionRefusal(context.Background(), journalGuardExec(1, rds))) +} diff --git a/internal/purchase/redshift_lineage.go b/internal/purchase/redshift_lineage.go index ee9b5c562..0273d2fa5 100644 --- a/internal/purchase/redshift_lineage.go +++ b/internal/purchase/redshift_lineage.go @@ -43,10 +43,13 @@ var afterRedshiftStampHook func() // Fixed refusal texts: the NULL-account wildcard can match another account's row, // so they never name another execution. errnotes.go allowlists each one. const ( - redshiftConflictRefusal = "refusing to dispatch: another open purchase of the same Redshift reservation (region, node type, term, payment, count) may still complete, so wait for it to finish and then retry" - redshiftHorizonRefusal = "refusing to dispatch: this Redshift purchase was first dispatched more than 24h ago, so check the account in AWS and start a new purchase" - redshiftNoStampRefusal = "refusing to dispatch: this Redshift purchase has no first dispatch time recorded, so check the account in AWS and start a new purchase" - redshiftRecentBuyRefusal = "refusing to dispatch: another purchase of the same Redshift reservation (region, node type, term, payment, count) completed too close to this purchase's first dispatch for AWS to tell the two apart, so start a new purchase at least 10 minutes after the other one completed" + redshiftConflictRefusal = "refusing to dispatch: another open purchase of the same Redshift reservation (region, node type, term, payment, count) may still complete, so wait for it to finish and then retry" + redshiftHorizonRefusal = "refusing to dispatch: this Redshift purchase was first dispatched more than 24h ago, so check the account in AWS and start a new purchase" + redshiftNoStampRefusal = "refusing to dispatch: this Redshift purchase has no first dispatch time recorded, so check the account in AWS and start a new purchase" + redshiftFailedConflictRefusal = "refusing to dispatch: another purchase of the same Redshift reservation (region, node type, term, payment, count) failed and may still be retried, so retry or settle that purchase, or start this one at least 24h after its first dispatch" + redshiftNoJournalRefusal = "refusing to dispatch: this Redshift retry needs the purchase attempt journal, which is not configured, so check the account in AWS and start a new purchase" + redshiftRecentBuyRefusal = "refusing to dispatch: another purchase of the same Redshift reservation (region, node type, term, payment, count) completed too close to this purchase's first dispatch for AWS to tell the two apart, so start a new purchase at least 10 minutes after the other one completed" + redshiftSettleRefusal = "refusing to dispatch: another attempt of this Redshift purchase ended less than 10 minutes ago and may have bought, so wait 10 minutes and then retry" ) // redshiftAdoptionWindow mirrors cloud-commitments-go's 5m Redshift dedupe skew @@ -124,21 +127,76 @@ func (m *Manager) redshiftGate(ctx context.Context, tx pgx.Tx, exec *config.Purc afterRedshiftStampHook() } family, _, _ := strings.Cut(idempotencyLineageKey(exec), ":") - conflicts, err := m.config.FindOpenRedshiftLineageConflictsTx(ctx, tx, exec.ExecutionID, family, tuples, exec.CloudAccountID, redshiftRedriveHorizon, *res.At, redshiftAdoptionWindow) + conflicts, err := m.config.FindOpenRedshiftLineageConflictsTx(ctx, tx, exec.ExecutionID, family, tuples, exec.CloudAccountID, redshiftRedriveHorizon, *res.At, redshiftAdoptionWindow, redshiftRetrySettle) if err != nil { return res, err } + return res, conflictRefusal(exec.ExecutionID, dropPersistentFailures(conflicts)) +} + +// conflictRefusal wraps pickConflict's refusal as before-submission; the other id goes to the log only. +func conflictRefusal(executionID string, conflicts []config.RedshiftLineageConflict) error { + refusal, culprit, ok := pickConflict(conflicts) + if !ok { + return nil + } + logging.Warnf("Redshift gate: execution %s refused (%s, execution %s, %s)", executionID, refusal, culprit.ExecutionID, culprit.Status) + return beforeSubmission(errors.New(refusal)) +} + +// conflictRules is the refusal precedence: another family's recent buy, any open lineage, a +// same-family attempt inside the settle. Failed rows only fall through to pickConflict's last case. +var conflictRules = []struct { + match func(config.RedshiftLineageConflict) bool + refusal string +}{ + {func(c config.RedshiftLineageConflict) bool { return !c.SameFamily && redshiftFinishedStatus(c.Status) }, redshiftRecentBuyRefusal}, + {func(c config.RedshiftLineageConflict) bool { + return c.Status != config.StatusFailed && (!c.SameFamily || c.Status == "running") + }, redshiftConflictRefusal}, + {func(c config.RedshiftLineageConflict) bool { return c.SameFamily }, redshiftSettleRefusal}, +} + +// pickConflict returns the refusal for the first rule any conflict matches, then failed rows only. +func pickConflict(conflicts []config.RedshiftLineageConflict) (string, config.RedshiftLineageConflict, bool) { + for _, rule := range conflictRules { + for _, c := range conflicts { + if rule.match(c) { + return rule.refusal, c, true + } + } + } + if allFailed(conflicts) { + return redshiftFailedConflictRefusal, conflicts[0], true + } + return "", config.RedshiftLineageConflict{}, false +} + +// allFailed reports a non-empty conflict list made only of failed rows. +func allFailed(conflicts []config.RedshiftLineageConflict) bool { for _, c := range conflicts { - if redshiftFinishedStatus(c.Status) { - logging.Warnf("Redshift gate: execution %s refused, another purchase of the tuple finished within the adoption window (execution %s, %s)", exec.ExecutionID, c.ExecutionID, c.Status) - return res, beforeSubmission(errors.New(redshiftRecentBuyRefusal)) + if c.Status != config.StatusFailed { + return false } } - if len(conflicts) > 0 { - logging.Warnf("Redshift gate: execution %s refused, conflicts with execution %s (%s)", exec.ExecutionID, conflicts[0].ExecutionID, conflicts[0].Status) - return res, beforeSubmission(errors.New(redshiftConflictRefusal)) + return len(conflicts) > 0 +} + +// dropPersistentFailures removes failed rows Retry can never re-drive (an operator-fix error or a +// refused provider): they would hold the tuple for the whole horizon with no way out. +func dropPersistentFailures(conflicts []config.RedshiftLineageConflict) []config.RedshiftLineageConflict { + kept := make([]config.RedshiftLineageConflict, 0, len(conflicts)) + for _, c := range conflicts { + if !c.SameFamily && c.Status == config.StatusFailed && (ResolveOpsHint(c.Error) != "" || retryRefusedForRecs(c)) { + continue + } + kept = append(kept, c) } - return res, nil + return kept +} + +func retryRefusedForRecs(c config.RedshiftLineageConflict) bool { + return RedriveRefusalReason(&config.PurchaseExecution{Recommendations: c.Recommendations}) != "" } // redshiftFinishedStatus reports the statuses whose node may already sit in AWS. diff --git a/internal/purchase/redshift_redrive_e2e_test.go b/internal/purchase/redshift_redrive_e2e_test.go new file mode 100644 index 000000000..f5fcc0359 --- /dev/null +++ b/internal/purchase/redshift_redrive_e2e_test.go @@ -0,0 +1,255 @@ +package purchase + +// Redshift re-drive end to end: the fake client models go's no-journal window dedupe (the +// journal is present only for the C10b check) and the store is the mock; no cloud API is reached. + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// dedupingRedshiftClient mirrors go's adoption rule: a node started at or after +// RequestedAt minus the 5m skew is adopted instead of bought again. +type dedupingRedshiftClient struct { + *MockServiceClient + mu sync.Mutex + opts []common.PurchaseOptions + nodes []time.Time + nodeStart time.Time + loseResponse bool +} + +func (c *dedupingRedshiftClient) PurchaseCommitment(_ context.Context, _ common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.opts = append(c.opts, opts) + for _, start := range c.nodes { + if !start.Before(opts.RequestedAt.Add(-5 * time.Minute)) { + return common.PurchaseResult{Success: true, CommitmentID: "adopted-node"}, nil + } + } + c.nodes = append(c.nodes, c.nodeStart) + if c.loseResponse { + c.loseResponse = false + return common.PurchaseResult{}, errors.New("purchase outcome unknown: the request may have been sent") + } + return common.PurchaseResult{Success: true, CommitmentID: "new-node"}, nil +} + +type redriveRig struct { + mgr *Manager + store *MockConfigStore + client *dedupingRedshiftClient + journal *fakeAttemptJournal + saves []config.PurchaseExecution + minted map[string]time.Time + rows map[string]*config.PurchaseExecution +} + +// coalescingStore mirrors StampFirstDispatchTx's COALESCE: a row's own stamp wins over a minted one. +type coalescingStore struct { + *MockConfigStore + rig *redriveRig +} + +func (s coalescingStore) StampFirstDispatchTx(_ context.Context, _ pgx.Tx, id string, _ time.Duration) (config.StampResult, error) { + if row := s.rig.rows[id]; row != nil && row.FirstDispatchAt != nil { + at := *row.FirstDispatchAt + return config.StampResult{At: &at, WithinHorizon: true}, nil + } + minted, ok := s.rig.minted[id] + if !ok { + return config.StampResult{}, nil + } + return config.StampResult{At: &minted, WithinHorizon: true}, nil +} + +// track registers the row the store sees under its ID, by pointer so a gate-set stamp is visible. +func (r *redriveRig) track(row *config.PurchaseExecution) *config.PurchaseExecution { + r.rows[row.ExecutionID] = row + return row +} + +// newRedriveRig wires a manager through ManagerConfig. stamps maps an execution ID to the first +// dispatch time the store mints for it when the row has none; conflicts is what the gate query finds. +func newRedriveRig(t *testing.T, stamps map[string]time.Time, conflicts []config.RedshiftLineageConflict) *redriveRig { + t.Helper() + rig := &redriveRig{ + store: new(MockConfigStore), + client: &dedupingRedshiftClient{MockServiceClient: new(MockServiceClient)}, + journal: &fakeAttemptJournal{}, + } + email := new(MockEmailSender) + factory := new(MockProviderFactory) + prov := new(MockProvider) + var mu sync.Mutex + rig.store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + mu.Lock() + rig.saves = append(rig.saves, *e) + mu.Unlock() + return nil + } + rig.store.On("LockRedshiftTuplesTx", mock.Anything, mock.Anything, mock.Anything).Return(nil) + rig.store.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(conflicts, nil) + rig.minted, rig.rows = stamps, map[string]*config.PurchaseExecution{} + rig.store.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Maybe() + email.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil).Maybe() + email.On("SendPurchaseFailedNotification", mock.Anything, mock.Anything).Return(nil).Maybe() + factory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(prov, nil).Maybe() + prov.On("GetServiceClient", mock.Anything, common.ServiceRedshift, mock.Anything).Return(rig.client, nil).Maybe() + rig.mgr = NewManager(ManagerConfig{ + ConfigStore: coalescingStore{rig.store, rig}, EmailSender: email, ProviderFactory: factory, CredentialStore: awsAccessKeyCredStore(), + DashboardURL: "https://dashboard.example.com", AttemptJournal: rig.journal, + }) + return rig +} + +func redriveExec(id string, attempt int, stamp *time.Time) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: id, IdempotencyKey: "rs-lineage", Status: "running", RetryAttemptN: attempt, + FirstDispatchAt: stamp, Recommendations: []config.RecommendationRecord{redshiftScopedRec("acct-A")}, + } +} + +// The first attempt loses its response after AWS accepted the order. The retry successor copies the +// lineage stamp, so go sees the original RequestedAt and adopts the node instead of buying a second one. +func TestRedshiftRedrive_AdoptsOriginalNodeWithOriginalRequestedAt(t *testing.T) { + stamp := time.Now().UTC().Add(-30 * time.Minute).Truncate(time.Microsecond) + rig := newRedriveRig(t, map[string]time.Time{"attempt-0": stamp, "attempt-1": time.Now().UTC()}, nil) + rig.client.nodeStart, rig.client.loseResponse = stamp.Add(time.Minute), true + + first := rig.track(redriveExec("attempt-0", 0, nil)) + require.Error(t, rig.mgr.executeAndFinalize(context.Background(), first)) + require.NotNil(t, first.FirstDispatchAt, "the gate minted the lineage stamp") + + successor := rig.track(redriveExec("attempt-1", 1, first.FirstDispatchAt)) + require.NoError(t, rig.mgr.executeAndFinalize(context.Background(), successor)) + + require.Len(t, rig.client.opts, 2) + assert.Len(t, rig.client.nodes, 1, "the retry adopted the first node instead of buying another") + assert.True(t, rig.client.opts[0].RequestedAt.Equal(stamp)) + assert.True(t, rig.client.opts[1].RequestedAt.Equal(rig.client.opts[0].RequestedAt), "the retry keeps the original RequestedAt") + for _, o := range rig.client.opts { + assert.Same(t, rig.journal, o.AttemptJournal, "the manager's journal reaches every purchase call") + } + assert.Equal(t, "completed", successor.Status) +} + +// While the first lineage's failed row can still be retried, a second lineage for the same +// tuple is refused before any provider call and proven unsent. +func TestRedshiftRedrive_SecondLineageRefusedWhileFirstRetryable(t *testing.T) { + stamp := time.Now().UTC().Add(-time.Hour) + rig := newRedriveRig(t, map[string]time.Time{"lineage-2": stamp}, + []config.RedshiftLineageConflict{{ExecutionID: "lineage-1-failed", Status: config.StatusFailed, Error: "context deadline exceeded"}}) + + second := redriveExec("lineage-2", 0, nil) + err := rig.mgr.executeAndFinalize(context.Background(), second) + + require.Error(t, err) + assert.Equal(t, redshiftFailedConflictRefusal, err.Error()) + assert.True(t, isBeforeSubmission(err)) + assert.Empty(t, rig.client.opts) + assert.True(t, second.NoSubmission) +} + +// Pins the refusal text and no-provider-call precedence for a fixed same-family conflict; the SQL is +// covered by the lineage integration test. +func TestRedshiftRedrive_SameFamilyConflictRefusesWithSettleText(t *testing.T) { + stamp := time.Now().UTC().Add(-30 * time.Minute).Truncate(time.Microsecond) + rig := newRedriveRig(t, nil, nil) + rig.client.nodeStart, rig.client.loseResponse = stamp.Add(time.Minute), true + + xPrime := rig.track(redriveExec("x-prime", 1, &stamp)) + xPrime.IdempotencyKey = "root:acct" + require.Error(t, rig.mgr.executeAndFinalize(context.Background(), xPrime)) + require.Len(t, rig.client.opts, 1) + + kept := rig.store.ExpectedCalls[:0] + for _, c := range rig.store.ExpectedCalls { + if c.Method != "FindOpenRedshiftLineageConflictsTx" { + kept = append(kept, c) + } + } + rig.store.ExpectedCalls = kept + rig.store.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]config.RedshiftLineageConflict{{ExecutionID: "x-prime", Status: config.StatusFailed, SameFamily: true}}, nil) + + x1 := rig.track(redriveExec("x1", 1, &stamp)) + x1.IdempotencyKey = "root:acct" + err := rig.mgr.executeAndFinalize(context.Background(), x1) + + require.Error(t, err) + assert.Equal(t, redshiftSettleRefusal, err.Error()) + assert.True(t, isBeforeSubmission(err)) + assert.Len(t, rig.client.opts, 1, "X1 never reached the provider") +} + +func strandedRedshiftRow(id string, attempt int, stamp *time.Time) config.PurchaseExecution { + row := redriveExec(id, attempt, stamp) + row.Status = "approved" + return *row +} + +func recoverStranded(t *testing.T, rig *redriveRig, row config.PurchaseExecution) { + t.Helper() + ctx := context.Background() + claimed := row + claimed.Status = "running" + rig.track(&claimed) + rig.store.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold).Return([]config.PurchaseExecution{row}, nil) + rig.store.On("TransitionExecutionStatus", ctx, row.ExecutionID, []string{"approved"}, "running", (*string)(nil)).Return(&claimed, nil) + + recovered, err := rig.mgr.RecoverStrandedApprovals(ctx) + + require.NoError(t, err) + require.Equal(t, 1, recovered) +} + +func TestRedshiftRedrive_RecoverStrandedApprovals_Attempt0MintsStamp(t *testing.T) { + minted := time.Now().UTC().Add(-time.Minute).Truncate(time.Microsecond) + rig := newRedriveRig(t, map[string]time.Time{"stranded-0": minted}, nil) + rig.client.nodeStart = minted + + recoverStranded(t, rig, strandedRedshiftRow("stranded-0", 0, nil)) + + require.Len(t, rig.client.opts, 1) + assert.True(t, rig.client.opts[0].RequestedAt.Equal(minted), "an attempt 0 row never sent, so it mints the lineage stamp") + assert.Same(t, rig.journal, rig.client.opts[0].AttemptJournal) +} + +func TestRedshiftRedrive_RecoverStrandedApprovals_SuccessorKeepsCopiedStamp(t *testing.T) { + copied := time.Now().UTC().Add(-2 * time.Hour).Truncate(time.Microsecond) + rig := newRedriveRig(t, map[string]time.Time{"stranded-1": time.Now().UTC()}, nil) + rig.client.nodeStart = copied.Add(time.Minute) + + recoverStranded(t, rig, strandedRedshiftRow("stranded-1", 1, &copied)) + + require.Len(t, rig.client.opts, 1) + assert.True(t, rig.client.opts[0].RequestedAt.Equal(copied), "a successor dispatches under the copied stamp, not a new one") + assert.Same(t, rig.journal, rig.client.opts[0].AttemptJournal) +} + +// A successor created before the stamp was copied has none; the lift must not let it re-dispatch. +func TestRedshiftRedrive_PreChangeSuccessorWithoutStampIsDefusedAfterLift(t *testing.T) { + rig := newRedriveRig(t, map[string]time.Time{}, nil) + legacy := rig.track(redriveExec("legacy-successor", 1, nil)) + err := rig.mgr.executeAndFinalize(context.Background(), legacy) + + require.Error(t, err) + assert.Equal(t, redshiftNoStampRefusal, err.Error()) + assert.False(t, isBeforeSubmission(err), "an earlier attempt may have sent") + assert.Empty(t, rig.client.opts) + assert.Equal(t, "failed", legacy.Status) +} diff --git a/internal/purchase/requested_at_test.go b/internal/purchase/requested_at_test.go index 611577428..b7e7a066a 100644 --- a/internal/purchase/requested_at_test.go +++ b/internal/purchase/requested_at_test.go @@ -151,6 +151,13 @@ type redshiftFinalizeRun struct { func runRedshiftFinalize(t *testing.T, exec *config.PurchaseExecution, accounts []config.CloudAccount, service common.ServiceType, stamp func() (config.StampResult, error)) *redshiftFinalizeRun { t.Helper() + return runRedshiftFinalizeWithJournal(t, exec, accounts, service, stamp, nil) +} + +// runRedshiftFinalizeWithJournal is runRedshiftFinalize on a manager whose attempt journal is journal. +func runRedshiftFinalizeWithJournal(t *testing.T, exec *config.PurchaseExecution, accounts []config.CloudAccount, service common.ServiceType, + stamp func() (config.StampResult, error), journal common.AttemptJournal) *redshiftFinalizeRun { + t.Helper() ctx := context.Background() mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) @@ -173,7 +180,7 @@ func runRedshiftFinalize(t *testing.T, exec *config.PurchaseExecution, accounts if stamp != nil { mockStore.On("LockRedshiftTuplesTx", mock.Anything, mock.Anything, mock.Anything).Return(nil) mockStore.On("FindOpenRedshiftLineageConflictsTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything, - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) res, stampErr := stamp() mockStore.On("StampFirstDispatchTx", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Run(func(args mock.Arguments) { @@ -200,6 +207,7 @@ func runRedshiftFinalize(t *testing.T, exec *config.PurchaseExecution, accounts manager := &Manager{ config: mockStore, email: mockEmail, providerFactory: mockFactory, credStore: awsAccessKeyCredStore(), dashboardURL: "https://dashboard.example.com", + attemptJournal: journal, } run.err = manager.executeAndFinalize(ctx, exec) return run diff --git a/internal/purchase/stranded_redrive.go b/internal/purchase/stranded_redrive.go index e0cb84eee..2c54cdc5a 100644 --- a/internal/purchase/stranded_redrive.go +++ b/internal/purchase/stranded_redrive.go @@ -24,7 +24,8 @@ func strandedRecoveryError(exec *config.PurchaseExecution) string { // place: every recommendation is safe to re-drive and the row has a durable // ExecutionID for the idempotency lineage. The sweep checks it on the listed // snapshot to pick a path, and claimAndRedrive checks it again on the claimed -// row before buying (issue #1037). +// row before buying (issue #1037). Redshift qualifies: only approved rows are +// swept and nothing claims one before it runs, so a stranded row never sent. func redrivable(exec *config.PurchaseExecution) bool { return allRecsSafeToRedrive(exec) && exec.ExecutionID != "" } diff --git a/known_issues/39_redshift_dedupe_window.md b/known_issues/39_redshift_dedupe_window.md index 399088995..eac76917f 100644 --- a/known_issues/39_redshift_dedupe_window.md +++ b/known_issues/39_redshift_dedupe_window.md @@ -1,6 +1,6 @@ # Known Issues: Redshift reserved-node dedupe window -> **Audit status (2026-10-20):** `5 needs triage · 0 resolved` +> **Audit status (2026-11-06):** `5 needs triage · 1 resolved` ## MEDIUM: a repeat same-tuple Redshift purchase within ~5 minutes adopts the earlier node without buying @@ -19,8 +19,9 @@ - A lineage whose purchase record never landed (the child bought but its row save failed, `execution.go:351` and `:383`) leaves no completed row for the gate to see. - The go-side narrowing in LeanerCloud/cloud-commitments-go#700 and the real fix in #515 are open. - The window is measured from the other row's last save, so a later save of that row (for example a revocation request) restarts it. That only refuses longer; it never allows a double buy. -- A `failed` row without the no-submission proof (reaped mid-buy, or an unknown outcome) may hold a real node but is not refused by this window; if go adopts that node, the accounting stays one node and one completed row. PR3 counts such rows as open lineages. -- After the Redshift re-drive guard is lifted, a retry can be refused for up to 24h while another lineage's completed node sits inside its window (liveness only, no double buy). +- A `failed` row without the no-submission proof (reaped mid-buy, or an unknown outcome) may hold a real node but is not refused by this window; if go adopts that node, the accounting stays one node and one completed row. The gate counts such rows as open lineages (below). +- Now that the Redshift re-drive guard is lifted, a retry can be refused for up to 24h while another lineage's completed node sits inside its window (liveness only, no double buy). +- Plan rows that PR2b refused before submission (failed, `no_submission`, attempt 0) used to hold their ramp step until the account was detached. They are now retryable, so that hold clears with a Retry once the other node leaves its window. **Status:** Mitigated for platform-recorded buys. Tracked in #702 and LeanerCloud/cloud-commitments-go#515. @@ -28,7 +29,7 @@ **Files:** `internal/purchase/execution.go`, `internal/purchase/manager.go` -**Description:** If a Redshift purchase ends with an unknown outcome and a NEW purchase (not a re-drive) of the same tuple is dispatched after the match window has passed, the dedupe cannot see the first node and both are bought. Redshift re-drive stays refused (`awsPurchaseHasNoDuplicateGuard`, `recRedriveRefusalReason`, `armedRedriveRefusal` are unchanged), so only a fresh purchase can hit this. +**Description:** If a Redshift purchase ends with an unknown outcome and a NEW purchase (not a re-drive) of the same tuple is dispatched after the match window has passed, the dedupe cannot see the first node and both are bought. A retry of the same purchase is guarded (it reuses the first dispatch time, see below), so only a fresh purchase can hit this. **Why deferred:** Mitigated by purchase suppressions and the operator message shown for an unknown outcome. A complete fix needs a durable per-purchase identity on the Redshift side. @@ -54,12 +55,35 @@ **Status:** Open. Tracked in #702. -## LOW: RequestedAt is stamped per dispatch, not per purchase lineage +## RESOLVED: RequestedAt is stamped per dispatch, not per purchase lineage -**Files:** `internal/purchase/execution.go` +**Files:** `internal/purchase/redshift_lineage.go`, `internal/purchase/manager.go`, `internal/api/handler_purchases_redshift_retry.go` -**Description:** The library documents `RequestedAt` as stable across re-drives of the same token; the platform stamps a fresh time per dispatch. This is only safe because Redshift re-drive is refused. Lifting that refusal requires a persisted first-dispatch time (#702 option B). +**Description:** The library documents `RequestedAt` as stable across re-drives of the same token. The platform now persists the lineage's first dispatch time (`first_dispatch_at`), copies it to every retry successor, and dispatches under it, so a re-drive carries the original `RequestedAt`. The Redshift re-drive refusal is lifted behind the Retry gate (10 minute settle, 24h horizon, stamp required) and the dispatch gate. -**Why deferred:** Re-drive stays refused, so there is no current impact. +**Status:** Resolved in #702. + +## LOW: limits of the Redshift re-drive + +**Files:** `internal/purchase/redshift_lineage.go`, `internal/purchase/redshift_retry.go`, `internal/config/store_postgres_redshift_lineage.go`, `internal/api/handler_history.go` + +**Description:** Fail-closed edges that cost liveness, not money: + +- A failed Redshift row that Retry can still re-drive (no successor, not settled, not a never-submitted attempt 0, first dispatched within 24h) counts as an open lineage. Two failed same-tuple lineages inside the horizon therefore block each other's new purchases and Retry for up to 24h, which includes the two lineages that exist at deploy time. Operator-fixable failures (`ResolveOpsHint`) and rows past the horizon do not count. +- Failed rows that carry a definite AWS refusal still count, because their first dispatch time is set. Retry them or wait out the horizon. +- A failed direct-purchase row cannot be settled (settling covers plan rows only), so it blocks the same tuple until it is retried or 24h pass. +- A retried row whose ladder tranche is already settled (`errRetryLadderTrancheSettled`) keeps its lineage open for up to 24h. +- The conflict query treats a NULL account on either side as matching any account, so it can over-refuse across accounts. It never under-refuses. +- Rows the reaper failed before anything was sent have no first dispatch time and are not `no_submission`, so Retry refuses them for good; start a new purchase. +- A failed attempt 1 row that is `no_submission` is a dead end: Retry treats only a never-submitted attempt 0 as skippable. Widening that to "the predecessor is `no_submission`" is unsafe, because `ladderExecutionRefusal` can mark a legacy attempt 1 successor `no_submission` while attempt 0 may have sent (`manager.go`, `preExecutionRefusal`); only a check that the whole chain is `no_submission` would be safe. +- Without an attempt journal (`ManagerConfig.AttemptJournal` nil) go keeps its window dedupe only, so the offering-ID hole and the listing-lag window stay open. A retry successor (attempt above 0) is then refused with a plain error and no `no_submission` proof, so each refused successor stays an open failed row and blocks same-tuple purchases for up to 24h. +- A stranded approved row is re-driven in place without a settle wait. That is safe because the sweep claims only `approved` rows, nothing writes `approved` after `running`, and so a stranded row was never claimed and never sent. A stranded successor above attempt 0 is covered by the Retry-time settle and the journal check. +- Legacy armed-refusal rows ("refusing to execute retry attempt N: AWS EC2 and Redshift ...") are still allowlisted for display. History now shows the permanent no-first-dispatch-time hint beside that stored text; both say the purchase cannot be retried. +- History computes the stamp age on the application clock as a hint, while the Retry gate reads the database clock. Close to 24h, Retry can show and then answer 409. The gate is authoritative. +- The History hint does not show the 10 minute settle wait; the Retry button answers 409 with `retry_after_seconds` instead. +- The console and `cudly-mcp` buyers were not checked for the stamp and journal wiring; only the platform dispatch path is covered. +- A child that bought but whose row save failed (`execution.go:351`, `:383`) leaves no row for the gate, unless its root is a retryable failed row. +- Same-family successors of one token (for example a root retry's fan-out child and a retried child, same key) now also wait out the 10 minute settle at dispatch (a sibling that failed after a possible send, or finished, within 10 minutes); the Retry-creation check alone did not cover two successors of one token. +- Related go work: LeanerCloud/cloud-commitments-go#700 (claimed reservations) and #515 (identity stronger than the tuple). **Status:** Open. Tracked in #702.