From 83c3d04587033febd6338eaa76d08ededeb81c23 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 20 Aug 2026 11:46:40 +0200 Subject: [PATCH 1/3] fix(plans): count a ramp step only when every account has bought (#1861) CompletePlanStep advanced the ramp as soon as ONE execution for a step ran clean. A multi-account plan fans one ramp step into one execution per cloud account, so an operator repairing a partially-failed step one account at a time moved the plan to "step N done" while the remaining accounts had bought nothing for step N. The plan then reported itself further along than the commitment it held, and step N's tranche for those accounts was never bought by anything. Reachable on the routine path, since per-account retry is the canonical recovery flow (#1655). The advance is now gated on the whole fan-out. CompletePlanStep counts, inside the transaction that already holds the plan row locked, how many of the step's units bought and how many still hold it open, and reports ErrRampStepIncomplete instead of advancing until none do. A step that nothing bought is refused too: an empty result set must not satisfy a guard whose job is to prove a purchase happened. "The plan's target accounts at the time the step ran" is pinned to the execution rows the fan-out wrote, not the plan's account list today, so changing a plan's accounts between steps cannot retroactively change what an earlier step needed. Within a step, one row per account decides: the attempt no retry has superseded, most recently written. That covers both a retry successor (which shares its predecessor's transaction timestamp) and a re-drive of a failed root row (which supersedes nothing), so a dead attempt cannot speak for an account that has since bought. Option 2 of the issue, not the stated preference. Deriving CurrentStep from the executions table is unsafe here: CleanupOldExecutions deletes completed executions past the retention horizon, so a derived position falls back toward zero as rows age out and the plan re-buys its ramp. Deriving also does not avoid the freeze it was meant to avoid, since it either stalls on the same incomplete step or jumps over it. Both silent-under-buy decisions are made durable. A step waiting on its siblings is transient and self-healing, so it is logged at warn and surfaced live through a new ramp_blocked plan-health factor derived from the same rows the gate reads; a stamped note would outlive the fact and describe a ramp that has since advanced. The permanent outcomes -- a refused advance over a gap, and a step the plan has already counted, which previously returned nil and left no trace at all -- keep their audit note on the execution row. ramp_blocked pre-empts behind_schedule so a stopped plan no longer reads as merely late. Verified: the #1669 regression test's own scenario reproduced the bug first (3-account step 3, only B retried: CurrentStep 3, want 2) and passes with the gate; re-confirmed by disabling only the gate call against the final test code. Full unit suite, integration suites for purchase/config/api against a real migrated Postgres, go vet, golangci-lint at the CI-pinned v2.10.1, and gocyclo -over 10 all clean. Deferred: handler_history describes ANY completed execution carrying a non-empty error as an audit gap ("its history record could not be saved"), which mis-describes a ramp-advance note. Pre-existing since #1669 and unchanged here; filed separately rather than widened into a purchase_executions column in this change. --- internal/analytics/collector_test.go | 4 + internal/api/handler_plans.go | 12 +- internal/api/handler_plans_test.go | 85 ++++++++ internal/api/openapi.yaml | 13 +- internal/api/plan_health.go | 48 ++++- internal/api/plan_health_test.go | 117 +++++++++-- internal/config/errors.go | 21 ++ internal/config/interfaces.go | 21 +- internal/config/store_postgres.go | 95 +++++---- .../store_postgres_complete_step_test.go | 81 +++++++- internal/config/store_postgres_ramp_step.go | 165 +++++++++++++++ internal/config/types.go | 61 ++++++ internal/mocks/stores.go | 20 ++ internal/purchase/execution_test.go | 67 ++++++ internal/purchase/manager.go | 30 ++- .../ramp_step_progress_integration_test.go | 195 +++++++++++++++--- internal/server/test_helpers_test.go | 4 + 17 files changed, 929 insertions(+), 110 deletions(-) create mode 100644 internal/config/store_postgres_ramp_step.go diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 30baf7544..456bea744 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -164,6 +164,10 @@ func (m *mockConfigStore) CompletePlanStep(ctx context.Context, planID string, s return nil } +func (m *mockConfigStore) GetStuckRampSteps(_ context.Context) (map[string]config.RampStepBlock, error) { + return nil, nil +} + func (m *mockConfigStore) UpdatePurchasePlanTx(ctx context.Context, _ pgx.Tx, plan *config.PurchasePlan) error { return nil } diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 77e954d5e..2fa84b33f 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -79,8 +79,18 @@ func (h *Handler) attachPlanHealth(ctx context.Context, plans []config.PurchaseP return result } + // A blocked ramp is worth up to 25 points, so a score computed without it + // is not a partially-informed score, it is a wrong one -- and it errs + // healthy, on plans that are stopped. Withhold every score rather than + // publish that, exactly as the counts fetch above does. + stuckByPlan, err := h.config.GetStuckRampSteps(ctx) + if err != nil { + logging.Warnf("listPlans: GetStuckRampSteps failed, plan health reported as unknown: %v", err) + return result + } + for i := range plans { - score, factors := computePlanHealth(plans[i], now, countsByPlan[plans[i].ID]) + score, factors := computePlanHealth(plans[i], now, countsByPlan[plans[i].ID], stuckByPlan[plans[i].ID]) result[i].HealthScore = &score result[i].HealthFactors = factors } diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 6bff9b0b2..38ab1f6e3 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -1581,3 +1581,88 @@ func TestHandler_createPlan_DBErrorOnCreateReturns500WithLog(t *testing.T) { // Raw DB error must NOT be exposed to the caller. assert.NotContains(t, ce.Error(), "connection refused", "internal DB error must not leak to caller") } + +// TestHandler_listPlans_StuckRampFetchFailureLeavesHealthUnknown pins the +// fail-closed half of the ramp_blocked wiring (issue #1861). The factor is +// worth 25 points and only ever subtracts, so a score computed without it is +// not "slightly optimistic", it is a healthy badge on a plan whose ramp is +// stopped -- the exact reading the issue was filed about. Withhold the score +// instead, the same way an unreadable execution-count fetch does. +func TestHandler_listPlans_StuckRampFetchFailureLeavesHealthUnknown(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + planID := "11111111-1111-1111-1111-111111111111" + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + Return([]config.PurchasePlan{{ID: planID, Name: "Ramping Plan", Enabled: true}}, nil) + mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). + Return(map[string]config.ExecutionStatusCounts{}, nil) + mockStore.On("GetStuckRampSteps", ctx).Return(nil, errors.New("connection reset")) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + + result, err := handler.listPlans(ctx, req, map[string]string{}) + require.NoError(t, err, "an unreadable health input must not fail the Plans page") + require.Len(t, result.Plans, 1) + assert.Nil(t, result.Plans[0].HealthScore, + "a score that cannot see stuck ramps must be absent, not optimistic") + assert.Empty(t, result.Plans[0].HealthFactors) +} + +// TestHandler_listPlans_StuckRampProducesTheBlockedFactor is the end-to-end +// wiring check: a plan the store reports as stuck must come back carrying the +// ramp_blocked factor rather than the wall-clock behind_schedule proxy. +func TestHandler_listPlans_StuckRampProducesTheBlockedFactor(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + planID := "11111111-1111-1111-1111-111111111111" + quietPlanID := "22222222-2222-2222-2222-222222222222" + ramp := config.RampSchedule{ + Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, + CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -28), + } + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + Return([]config.PurchasePlan{ + {ID: planID, Name: "Stuck Plan", Enabled: true, RampSchedule: ramp}, + {ID: quietPlanID, Name: "Late Plan", Enabled: true, RampSchedule: ramp}, + }, nil) + mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). + Return(map[string]config.ExecutionStatusCounts{}, nil) + mockStore.On("GetStuckRampSteps", ctx). + Return(map[string]config.RampStepBlock{planID: {StepNumber: 3, StuckExecutions: 1}}, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + + result, err := handler.listPlans(ctx, req, map[string]string{}) + require.NoError(t, err) + byID := make(map[string]PlanWithHealth, len(result.Plans)) + for _, p := range result.Plans { + byID[p.ID] = p + } + + stuck := byID[planID] + require.Len(t, stuck.HealthFactors, 1) + assert.Equal(t, HealthFactorRampBlocked, stuck.HealthFactors[0].Code) + + // Same ramp, same clock, not reported stuck: it is merely late. The two + // plans differing only in the store's report is what proves the factor is + // keyed on the report and not on the schedule. + late := byID[quietPlanID] + require.Len(t, late.HealthFactors, 1) + assert.Equal(t, HealthFactorBehindSchedule, late.HealthFactors[0].Code) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ab1b5aa40..8159f08a5 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2780,10 +2780,15 @@ components: (failed_executions, canceled_executions) count only executions that entered that state within the trailing execution-retention window (30 days), not over all history, so they describe the - plan's recent behaviour rather than its lifetime record. The - remaining codes are derived from the plan's current attributes - and ramp schedule and are not time-windowed. - enum: [overdue, failed_executions, canceled_executions, stalled, behind_schedule, disabled_midway] + plan's recent behaviour rather than its lifetime record. + ramp_blocked is derived from the execution rows of the plan's next + ramp step, not time-windowed, and pre-empts stalled / + behind_schedule: it means the step cannot complete because one of + its executions (one per cloud account, once the step has fanned + out) failed with no retry in flight, as opposed to the plan merely + running late. The remaining codes are derived from the plan's + current attributes and ramp schedule. + enum: [overdue, failed_executions, canceled_executions, ramp_blocked, stalled, behind_schedule, disabled_midway] penalty: type: integer description: Points subtracted from 100 by this factor. diff --git a/internal/api/plan_health.go b/internal/api/plan_health.go index f08bb1683..07b96f881 100644 --- a/internal/api/plan_health.go +++ b/internal/api/plan_health.go @@ -19,6 +19,7 @@ const ( HealthFactorOverdue PlanHealthFactorCode = "overdue" HealthFactorFailedExecutions PlanHealthFactorCode = "failed_executions" HealthFactorCanceledExecutions PlanHealthFactorCode = "canceled_executions" + HealthFactorRampBlocked PlanHealthFactorCode = "ramp_blocked" HealthFactorStalled PlanHealthFactorCode = "stalled" HealthFactorBehindSchedule PlanHealthFactorCode = "behind_schedule" HealthFactorDisabledMidway PlanHealthFactorCode = "disabled_midway" @@ -47,6 +48,11 @@ const ( penaltyBehindSchedule = 20 penaltyDisabledMidway = 25 + // A blocked ramp outranks behind_schedule because it is a diagnosis + // rather than an observation: the plan is not late, it is stopped, and it + // stays stopped until an operator acts on the execution that failed. + penaltyRampBlocked = 25 + // maxCountedFailedExecs / maxCountedCanceledExecs cap how many rows // count toward their respective penalty, so a plan with a long failure // history can't score below the documented worst case for that factor @@ -113,12 +119,12 @@ var planHealthExecutionStatuses = config.HealthScoredExecutionStatuses // historical failure/cancellation rows and a disabled toggle-off after // completion are expected end-of-life noise, not signals an operator // should chase. -func computePlanHealth(plan config.PurchasePlan, now time.Time, counts config.ExecutionStatusCounts) (int, []PlanHealthFactor) { +func computePlanHealth(plan config.PurchasePlan, now time.Time, counts config.ExecutionStatusCounts, block config.RampStepBlock) (int, []PlanHealthFactor) { if plan.RampSchedule.TotalSteps > 0 && plan.RampSchedule.IsComplete() { return planHealthScoreMax, nil } - factors := collectPlanHealthFactors(plan, now, counts) + factors := collectPlanHealthFactors(plan, now, counts, block) score := planHealthScoreMax for _, f := range factors { @@ -132,10 +138,18 @@ func computePlanHealth(plan config.PurchasePlan, now time.Time, counts config.Ex // collectPlanHealthFactors runs every per-factor check and returns the // subset that applies to plan, in table order (overdue, failed_executions, -// canceled_executions, then the mutually-exclusive stalled/behind_schedule -// pair, then disabled_midway). Split out from computePlanHealth so each -// factor stays an independent, individually testable function. -func collectPlanHealthFactors(plan config.PurchasePlan, now time.Time, counts config.ExecutionStatusCounts) []PlanHealthFactor { +// canceled_executions, then the mutually-exclusive +// ramp_blocked/stalled/behind_schedule group, then disabled_midway). Split out +// from computePlanHealth so each factor stays an independent, individually +// testable function. +// +// ramp_blocked pre-empts the schedule factors rather than adding to them. +// Wall-clock drift is what stalled/behind_schedule measure, and a blocked ramp +// always produces that drift, so reporting both would name the symptom +// alongside the cause and read as two independent problems. Which one an +// operator sees is exactly the distinction issue #1861 asked for: a plan that +// is merely late looks nothing like a plan whose next step cannot complete. +func collectPlanHealthFactors(plan config.PurchasePlan, now time.Time, counts config.ExecutionStatusCounts, block config.RampStepBlock) []PlanHealthFactor { var factors []PlanHealthFactor if f, ok := overdueFactor(plan, now); ok { factors = append(factors, f) @@ -146,7 +160,9 @@ func collectPlanHealthFactors(plan config.PurchasePlan, now time.Time, counts co if f, ok := canceledExecutionsFactor(counts); ok { factors = append(factors, f) } - if f, ok := scheduleFactor(plan, now); ok { + if f, ok := rampBlockedFactor(block); ok { + factors = append(factors, f) + } else if f, ok := scheduleFactor(plan, now); ok { factors = append(factors, f) } if f, ok := disabledMidwayFactor(plan); ok { @@ -155,6 +171,24 @@ func collectPlanHealthFactors(plan config.PurchasePlan, now time.Time, counts co return factors } +// rampBlockedFactor: the plan's next ramp step has executions that failed with +// no retry in flight, one per cloud account once the step has fanned out, so +// CompletePlanStep will keep refusing to count that step (issue #1861). Derived +// per request from the execution rows rather than from a stored marker, so it +// clears itself as soon as the account retries successfully or its row is +// canceled. +func rampBlockedFactor(block config.RampStepBlock) (PlanHealthFactor, bool) { + if block.StuckExecutions <= 0 { + return PlanHealthFactor{}, false + } + return PlanHealthFactor{ + Code: HealthFactorRampBlocked, + Penalty: penaltyRampBlocked, + Note: fmt.Sprintf("ramp step %d cannot complete: %d execution(s) failed it with no retry in flight", + block.StepNumber, block.StuckExecutions), + }, true +} + // overdueFactor: enabled AND next_execution_date < now. func overdueFactor(plan config.PurchasePlan, now time.Time) (PlanHealthFactor, bool) { if !plan.Enabled || plan.NextExecutionDate == nil || !plan.NextExecutionDate.Before(now) { diff --git a/internal/api/plan_health_test.go b/internal/api/plan_health_test.go index 461ab96ed..df62b898a 100644 --- a/internal/api/plan_health_test.go +++ b/internal/api/plan_health_test.go @@ -53,7 +53,7 @@ func TestComputePlanHealth_HealthyPlanScoresPerfect(t *testing.T) { }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) assert.Equal(t, 100, score) assert.Empty(t, factors) @@ -74,7 +74,7 @@ func TestComputePlanHealth_CompletedPlanShortCircuitsRegardlessOfOtherIssues(t * } execCounts := counts(map[string]int{"failed": 2, config.StatusCanceled: 1}) - score, factors := computePlanHealth(plan, now, execCounts) + score, factors := computePlanHealth(plan, now, execCounts, config.RampStepBlock{}) assert.Equal(t, 100, score) assert.Empty(t, factors) @@ -94,7 +94,7 @@ func TestComputePlanHealth_Overdue(t *testing.T) { }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorOverdue, factors[0].Code) @@ -116,7 +116,7 @@ func TestComputePlanHealth_OverdueRequiresEnabled(t *testing.T) { }, } - _, factors := computePlanHealth(plan, now, nil) + _, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) assert.NotContains(t, factorCodes(factors), HealthFactorOverdue) } @@ -136,7 +136,7 @@ func TestComputePlanHealth_FailedExecutionsPenaltyCapsAtFour(t *testing.T) { // must still report the true count of 6. execCounts := counts(map[string]int{"failed": 6}) - score, factors := computePlanHealth(plan, now, execCounts) + score, factors := computePlanHealth(plan, now, execCounts, config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorFailedExecutions, factors[0].Code) @@ -167,7 +167,7 @@ func TestComputePlanHealth_CanceledExecutionsPenaltyCapsAtFourAndCountsBothSpell config.LegacyStatusCanceled: 2, }) - score, factors := computePlanHealth(plan, now, execCounts) + score, factors := computePlanHealth(plan, now, execCounts, config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorCanceledExecutions, factors[0].Code) @@ -189,7 +189,7 @@ func TestComputePlanHealth_Stalled(t *testing.T) { }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorStalled, factors[0].Code) @@ -209,7 +209,7 @@ func TestComputePlanHealth_BehindSchedule(t *testing.T) { }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorBehindSchedule, factors[0].Code) @@ -243,7 +243,7 @@ func TestComputePlanHealth_BehindScheduleNoteClampsExpectedStepToTotalSteps(t *t }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) f := factorByCode(t, factors, HealthFactorBehindSchedule) assert.Equal(t, "on step 2, expected step 4 by now", f.Note) @@ -263,7 +263,7 @@ func TestComputePlanHealth_StalledAndBehindScheduleAreMutuallyExclusive(t *testi }, } - _, factors := computePlanHealth(plan, now, nil) + _, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) codes := factorCodes(factors) assert.Contains(t, codes, HealthFactorStalled) @@ -283,7 +283,7 @@ func TestComputePlanHealth_ImmediatePlanSkipsScheduleFactors(t *testing.T) { }, } - _, factors := computePlanHealth(plan, now, nil) + _, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) codes := factorCodes(factors) assert.NotContains(t, codes, HealthFactorStalled) @@ -310,7 +310,7 @@ func TestComputePlanHealth_ZeroStartDateSkipsScheduleFactors(t *testing.T) { }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) codes := factorCodes(factors) assert.NotContains(t, codes, HealthFactorBehindSchedule) @@ -333,7 +333,7 @@ func TestComputePlanHealth_ZeroStartDateStillCountsExecutionFactors(t *testing.T }, } - score, factors := computePlanHealth(plan, now, counts(map[string]int{"failed": 2})) + score, factors := computePlanHealth(plan, now, counts(map[string]int{"failed": 2}), config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorFailedExecutions, factors[0].Code) @@ -352,7 +352,7 @@ func TestComputePlanHealth_DisabledMidway(t *testing.T) { }, } - score, factors := computePlanHealth(plan, now, nil) + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) require.Len(t, factors, 1) assert.Equal(t, HealthFactorDisabledMidway, factors[0].Code) @@ -372,7 +372,7 @@ func TestComputePlanHealth_DisabledButNeverStartedIsBehindScheduleNotDisabledMid }, } - _, factors := computePlanHealth(plan, now, nil) + _, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) codes := factorCodes(factors) assert.Contains(t, codes, HealthFactorBehindSchedule) @@ -395,7 +395,7 @@ func TestComputePlanHealth_ScoreClampsAtZeroWhenPenaltiesStack(t *testing.T) { } execCounts := counts(map[string]int{"failed": 6, config.StatusCanceled: 6}) - score, factors := computePlanHealth(plan, now, execCounts) + score, factors := computePlanHealth(plan, now, execCounts, config.RampStepBlock{}) assert.Equal(t, 0, score) // disabled_midway note: overdue only requires Enabled, which is false @@ -429,8 +429,91 @@ func TestComputePlanHealth_UnrelatedExecutionStatusesAreNotCounted(t *testing.T) // into any penalty. execCounts := counts(map[string]int{"pending": 3, "notified": 1, "completed": 9, "approved": 2}) - score, factors := computePlanHealth(plan, now, execCounts) + score, factors := computePlanHealth(plan, now, execCounts, config.RampStepBlock{}) assert.Equal(t, 100, score) assert.Empty(t, factors) } + +// --- ramp_blocked (issue #1861) --- + +// blockedRampPlan is a plan whose wall clock says "behind schedule": far enough +// past its start that scheduleFactor would fire, so the tests below measure +// which factor wins rather than whether one exists at all. +func blockedRampPlan(now time.Time) config.PurchasePlan { + return config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 2, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -28), + }, + } +} + +func TestComputePlanHealth_RampBlockedReplacesBehindSchedule(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := blockedRampPlan(now) + + // Same plan, no stuck accounts: the wall-clock factor is what fires. + _, lateFactors := computePlanHealth(plan, now, nil, config.RampStepBlock{}) + require.Contains(t, factorCodes(lateFactors), HealthFactorBehindSchedule, + "the fixture must be behind schedule, or this test proves nothing") + + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{StepNumber: 3, StuckExecutions: 2}) + + codes := factorCodes(factors) + assert.Contains(t, codes, HealthFactorRampBlocked) + assert.NotContains(t, codes, HealthFactorBehindSchedule, + "a stopped ramp must not also be reported as merely late") + assert.NotContains(t, codes, HealthFactorStalled) + assert.Equal(t, 100-penaltyRampBlocked, score) + assert.Equal(t, + "ramp step 3 cannot complete: 2 execution(s) failed it with no retry in flight", + factorByCode(t, factors, HealthFactorRampBlocked).Note) +} + +func TestComputePlanHealth_RampBlockedNeedsANonZeroCount(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := blockedRampPlan(now) + + // A step number with a zero count is what a plan absent from + // GetStuckRampSteps looks like after a map lookup miss, and must not be + // read as "step 0 is blocked". + _, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{StepNumber: 3}) + + assert.NotContains(t, factorCodes(factors), HealthFactorRampBlocked) + assert.Contains(t, factorCodes(factors), HealthFactorBehindSchedule) +} + +func TestComputePlanHealth_RampBlockedFiresBeforeTheFirstInterval(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := blockedRampPlan(now) + // Nothing about the wall clock is wrong yet, so scheduleFactor declines. + plan.RampSchedule.StartDate = now.AddDate(0, 0, -1) + + score, factors := computePlanHealth(plan, now, nil, config.RampStepBlock{StepNumber: 3, StuckExecutions: 1}) + + assert.Equal(t, []PlanHealthFactorCode{HealthFactorRampBlocked}, factorCodes(factors)) + assert.Equal(t, 100-penaltyRampBlocked, score, + "a ramp can be blocked the day it breaks, long before drift is measurable") +} + +func TestComputePlanHealth_RampBlockedStacksWithExecutionFactors(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + past := now.AddDate(0, 0, -1) + plan := blockedRampPlan(now) + plan.NextExecutionDate = &past + + score, factors := computePlanHealth(plan, now, counts(map[string]int{"failed": 2}), + config.RampStepBlock{StepNumber: 3, StuckExecutions: 1}) + + // The accounts that failed the blocked step are also failed executions, + // and the plan really is overdue: these describe different observations + // and are not mutually exclusive with each other, unlike the schedule pair. + assert.Equal(t, []PlanHealthFactorCode{ + HealthFactorOverdue, HealthFactorFailedExecutions, HealthFactorRampBlocked, + }, factorCodes(factors)) + assert.Equal(t, 100-penaltyOverdue-2*penaltyPerFailedExec-penaltyRampBlocked, score) +} diff --git a/internal/config/errors.go b/internal/config/errors.go index cc11eed8d..a20f046e0 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -13,6 +13,27 @@ var ErrNotFound = errors.New("not found") // distinguish this legitimate race-loss from a hard DB error. var ErrExecutionNotInExpectedStatus = errors.New("execution not in expected status") +// ErrRampStepIncomplete is returned (wrapped) by CompletePlanStep when the +// ramp step the completing execution belongs to has not been bought in full: +// another cloud account on that step is still outstanding, or nothing on the +// step bought at all. Counting it would report commitment the plan has not +// made (issue #1861). Transient by construction -- it clears when the +// outstanding account buys, when its row is canceled, or when the step is +// abandoned -- so callers must treat it as "not yet", not as a failure of the +// purchase that just completed. +// +// The sentinel text stays generic because both cases wrap it and each supplies +// its own specifics; naming one of them here would mis-describe the other. +var ErrRampStepIncomplete = errors.New("ramp step incomplete") + +// ErrRampStepAlreadyCounted is returned (wrapped) by CompletePlanStep when the +// plan has already counted the completing step. Unlike ErrRampStepIncomplete +// this is permanent and worth an audit note on the execution row: the purchase +// bought commitment for a step the ramp will never count again, which is how a +// step_number stamped by pre-#1669 code (or during that deploy overlap) +// surfaces. +var ErrRampStepAlreadyCounted = errors.New("ramp step already counted") + // ErrAuditLoss is returned (wrapped) by executeAndFinalize when the purchase // run itself completed but the subsequent SavePurchaseExecution call failed. // The execution is already "running" (per the CAS in claimAndRedrive) but its diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 8f5adea63..130c5985e 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -35,11 +35,24 @@ type StoreInterface interface { // prevents the concurrent-write lost-update race of issue #1071. // // It is idempotent in stepNumber: completing a step the plan has already - // counted is a no-op. That is what keeps a multi-account ramp step from - // advancing twice when an operator retries two separately-failed accounts - // of the same step (issue #1669). A step still counts as completed when one - // execution for it ran clean, not when every account has bought. + // counted reports ErrRampStepAlreadyCounted and writes nothing. That is + // what keeps a multi-account ramp step from advancing twice when an + // operator retries two separately-failed accounts of the same step (issue + // #1669). + // + // A step counts as complete only when EVERY cloud account the step fanned + // out to has bought, not when any one of them has (issue #1861). Until + // then it reports ErrRampStepIncomplete, which is a "not yet" rather than a + // failure of the purchase that just completed -- the purchase is real and + // already recorded; only the plan's position is withheld. CompletePlanStep(ctx context.Context, planID string, stepNumber int) error + // GetStuckRampSteps returns, keyed by plan ID, the plan's next ramp step + // and how many of that step's executions have a terminal, unsuccessful + // latest attempt with no retry in flight -- a ramp CompletePlanStep will + // keep refusing to advance until an operator intervenes. Plans with + // nothing stuck are absent from the map. Feeds the plan-health + // ramp_blocked factor (issue #1861). + GetStuckRampSteps(ctx context.Context) (map[string]RampStepBlock, error) // UpdatePurchasePlanTx is the tx-accepting variant of UpdatePurchasePlan. // Used from createPlannedPurchases' WithTx block so the per-row // SavePurchaseExecutionTx writes and the plan's next_execution_date diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 3fce71f18..a973b4b59 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -615,16 +615,17 @@ func (s *PostgresStore) GetPurchasePlan(ctx context.Context, planID string) (*Pu // retrying two separately-failed accounts of the same step used to advance the // ramp twice, so the plan reported itself a step further along than the // commitment it had actually bought (issue #1669). Completing a step at or -// below CurrentStep is therefore a no-op. +// below CurrentStep therefore writes nothing and reports +// ErrRampStepAlreadyCounted. // -// Completing a step more than one beyond CurrentStep is refused with an error -// instead of jumping: the steps in between never completed, and advancing over -// them would silently overstate how much the plan has bought. +// Completing a step more than one beyond CurrentStep is refused for the same +// reason in the other direction: the steps in between never completed, and +// advancing over them would silently overstate how much the plan has bought. // -// Granularity: "the step completed" still means at least one execution for that -// step ran clean, not that every account of a multi-account plan bought. That -// was true before #1669 too and is unchanged here; #1669 only stops one step -// being counted more than once. +// Granularity: a step is complete when EVERY cloud account it fanned out to has +// bought, not when any one of them has (issue #1861) -- see +// requireRampStepBought. Until then the advance reports ErrRampStepIncomplete +// and the plan row is left untouched. // // Returns nil when the plan no longer exists (deleted between execution and // progress update) so the caller is not penalized for a race it cannot control. @@ -642,41 +643,57 @@ func (s *PostgresStore) CompletePlanStep(ctx context.Context, planID string, ste return fmt.Errorf("failed to lock purchase plan %s: %w", planID, err) } - switch { - case plan.RampSchedule.IsComplete(): - // The ramp finished; a trailing execution cannot extend it. Fall - // through to the write anyway so a completed ramp always ends up - // with next_execution_date cleared (the invariant issue #1071 - // established). - case plan.RampSchedule.CurrentStep >= stepNumber: - // Already counted: a second successful execution of a step the ramp - // has passed, which is what a per-account retry of a - // partially-failed fan-out produces. Nothing to advance. - return nil - case plan.RampSchedule.CurrentStep != stepNumber-1: - return fmt.Errorf("refusing to advance plan %s from ramp step %d to %d: step(s) %d-%d never completed", - planID, plan.RampSchedule.CurrentStep, stepNumber, - plan.RampSchedule.CurrentStep+1, stepNumber-1) - default: - plan.RampSchedule.CurrentStep = stepNumber - } - + // A finished ramp cannot be extended by a trailing execution, but it + // still falls through to the write so next_execution_date always ends + // up cleared (the invariant issue #1071 established). if !plan.RampSchedule.IsComplete() { - nextDate := plan.RampSchedule.GetNextPurchaseDate() - plan.NextExecutionDate = &nextDate - } else { - plan.NextExecutionDate = nil + if err := advanceRampStep(ctx, tx, plan, stepNumber); err != nil { + return err + } } + return s.persistRampPosition(ctx, tx, plan) + }) +} + +// advanceRampStep moves plan's in-memory ramp position to stepNumber, or +// returns the sentinel naming why it may not. The plan row is already locked by +// the caller's transaction, so the read-modify-write cannot lose an update. +func advanceRampStep(ctx context.Context, tx pgx.Tx, plan *PurchasePlan, stepNumber int) error { + switch { + case plan.RampSchedule.CurrentStep >= stepNumber: + return fmt.Errorf("%w: plan %s is on ramp step %d and cannot count step %d again", + ErrRampStepAlreadyCounted, plan.ID, plan.RampSchedule.CurrentStep, stepNumber) + case plan.RampSchedule.CurrentStep != stepNumber-1: + return fmt.Errorf("refusing to advance plan %s from ramp step %d to %d: step(s) %d-%d never completed", + plan.ID, plan.RampSchedule.CurrentStep, stepNumber, + plan.RampSchedule.CurrentStep+1, stepNumber-1) + } + if err := requireRampStepBought(ctx, tx, plan.ID, stepNumber); err != nil { + return err + } + plan.RampSchedule.CurrentStep = stepNumber + return nil +} - now := time.Now() - plan.LastExecutionDate = &now - // Refresh updated_at on every advance. The plan was read from the DB - // with its previous UpdatedAt, so UpdatePurchasePlanTx's zero-value - // guard would otherwise persist a stale updated_at timestamp. - plan.UpdatedAt = now +// persistRampPosition writes plan's ramp position back, repointing +// next_execution_date at the step after it (or clearing it on a finished ramp) +// and stamping the execution timestamps. +func (s *PostgresStore) persistRampPosition(ctx context.Context, tx pgx.Tx, plan *PurchasePlan) error { + if plan.RampSchedule.IsComplete() { + plan.NextExecutionDate = nil + } else { + nextDate := plan.RampSchedule.GetNextPurchaseDate() + plan.NextExecutionDate = &nextDate + } - return s.UpdatePurchasePlanTx(ctx, tx, plan) - }) + now := time.Now() + plan.LastExecutionDate = &now + // Refresh updated_at on every advance. The plan was read from the DB + // with its previous UpdatedAt, so UpdatePurchasePlanTx's zero-value + // guard would otherwise persist a stale updated_at timestamp. + plan.UpdatedAt = now + + return s.UpdatePurchasePlanTx(ctx, tx, plan) } // UpdatePurchasePlan updates an existing purchase plan. Delegates to diff --git a/internal/config/store_postgres_complete_step_test.go b/internal/config/store_postgres_complete_step_test.go index 50cd3029c..dc753ddb0 100644 --- a/internal/config/store_postgres_complete_step_test.go +++ b/internal/config/store_postgres_complete_step_test.go @@ -15,10 +15,18 @@ package config // accounts of one step advanced the ramp twice and the plan reported itself a // step further along than the commitment it had bought. // +// The #1861 bug: even counted once, a step counted as complete as soon as ONE +// of its accounts bought, so an operator repairing a multi-account step one +// account at a time moved the plan to "step N done" while the others had +// bought nothing for step N. +// // These tests pin the fix's contract: // - the read happens inside a transaction (Begin) and carries FOR UPDATE, // - completing step N from CurrentStep N-1 persists CurrentStep = N, -// - completing a step the ramp already passed writes nothing at all, +// - completing a step the ramp already passed writes nothing and reports +// ErrRampStepAlreadyCounted, +// - completing a step whose fan-out still has an outstanding account, or +// which nothing bought, writes nothing and reports ErrRampStepIncomplete, // - completing a step more than one beyond CurrentStep is refused, // - a non-positive step is refused before the transaction is opened, // - a plan deleted mid-race is tolerated (returns nil, no spurious error), @@ -104,6 +112,17 @@ func completeStepUpdateArgs(wantStep int, staleUpdatedAt time.Time, wantNextNil return args } +// expectFanOut registers the ramp step's fan-out tally that CompletePlanStep +// consults before advancing (issue #1861): how many of the step's units bought +// and how many are still holding it open. The pattern requires the DISTINCT ON +// reduction, so a store that stopped taking the latest attempt per account -- +// and therefore let a dead attempt block or a stale one pass -- fails to match. +func expectFanOut(mock pgxmock.PgxPoolIface, planID string, stepNumber, bought, outstanding int) { + mock.ExpectQuery(`WITH[\s\S]*DISTINCT ON \(e\.plan_id, e\.cloud_account_id\)[\s\S]*FROM latest_attempt`). + WithArgs(planID, stepNumber, RampStepSucceededStatuses, RampStepSettledStatuses). + WillReturnRows(pgxmock.NewRows([]string{"bought", "outstanding"}).AddRow(bought, outstanding)) +} + // rampPlanRows builds a single purchase_plans row carrying ramp, with // updated_at seeded to stale so a test can assert the store refreshes it. func rampPlanRows(t *testing.T, planID string, ramp RampSchedule, now, stale time.Time, nextExec sql.NullTime) *pgxmock.Rows { @@ -143,6 +162,7 @@ func TestPGXMock_CompletePlanStep_LocksAndAdvances(t *testing.T) { mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). WithArgs("plan-123"). WillReturnRows(rampPlanRows(t, "plan-123", ramp, now, stale, sql.NullTime{Valid: false})) + expectFanOut(mock, "plan-123", 2, 3, 0) mock.ExpectExec(`UPDATE purchase_plans`). WithArgs(completeStepUpdateArgs(2, stale, false)...). WillReturnResult(pgxmock.NewResult("UPDATE", 1)) @@ -152,6 +172,58 @@ func TestPGXMock_CompletePlanStep_LocksAndAdvances(t *testing.T) { require.NoError(t, mock.ExpectationsWereMet()) } +// TestPGXMock_CompletePlanStep_OutstandingAccountBlocksAdvance is the issue +// #1861 guard at the store boundary: the fan-out tally reports an account that +// has not bought, so the plan row must not be written at all. pgxmock fails the +// test if an UPDATE is issued, because no ExpectExec is registered. +func TestPGXMock_CompletePlanStep_OutstandingAccountBlocksAdvance(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + stale := now.AddDate(0, 0, -30) + ramp := RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 1, TotalSteps: 4, StartDate: now} + + mock.ExpectBegin() + mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). + WithArgs("plan-multi"). + WillReturnRows(rampPlanRows(t, "plan-multi", ramp, now, stale, sql.NullTime{Valid: false})) + expectFanOut(mock, "plan-multi", 2, 2, 1) + mock.ExpectRollback() + + err := store.CompletePlanStep(ctx, "plan-multi", 2) + require.ErrorIs(t, err, ErrRampStepIncomplete, + "one account of a 3-account step has not bought, so the step is not complete") + require.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CompletePlanStep_StepNothingBoughtIsRefused pins the fail-closed +// half of the tally. An empty or all-unsuccessful fan-out satisfies "nothing is +// outstanding" vacuously, so a guard that only checked outstanding would wave +// through a step no purchase ever landed for. +func TestPGXMock_CompletePlanStep_StepNothingBoughtIsRefused(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + stale := now.AddDate(0, 0, -30) + ramp := RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 1, TotalSteps: 4, StartDate: now} + + mock.ExpectBegin() + mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). + WithArgs("plan-empty"). + WillReturnRows(rampPlanRows(t, "plan-empty", ramp, now, stale, sql.NullTime{Valid: false})) + expectFanOut(mock, "plan-empty", 2, 0, 0) + mock.ExpectRollback() + + err := store.CompletePlanStep(ctx, "plan-empty", 2) + require.ErrorIs(t, err, ErrRampStepIncomplete) + assert.Contains(t, err.Error(), "bought anything") + require.NoError(t, mock.ExpectationsWereMet()) +} + // TestPGXMock_CompletePlanStep_AlreadyCountedStepWritesNothing is the issue // #1669 guard at the store boundary: the second per-account retry of one ramp // step completes a step the plan already counted, and must leave the row @@ -171,10 +243,11 @@ func TestPGXMock_CompletePlanStep_AlreadyCountedStepWritesNothing(t *testing.T) mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). WithArgs("plan-123"). WillReturnRows(rampPlanRows(t, "plan-123", ramp, now, stale, sql.NullTime{Valid: true, Time: now})) - mock.ExpectCommit() + mock.ExpectRollback() - require.NoError(t, store.CompletePlanStep(ctx, "plan-123", 3), - "re-completing a counted step is a no-op, not an error") + err := store.CompletePlanStep(ctx, "plan-123", 3) + require.ErrorIs(t, err, ErrRampStepAlreadyCounted, + "re-completing a counted step is reported, not silently dropped") require.NoError(t, mock.ExpectationsWereMet()) } diff --git a/internal/config/store_postgres_ramp_step.go b/internal/config/store_postgres_ramp_step.go new file mode 100644 index 000000000..213e7940f --- /dev/null +++ b/internal/config/store_postgres_ramp_step.go @@ -0,0 +1,165 @@ +package config + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" +) + +// Ramp-step accounting for issue #1861. +// +// A ramp step of a multi-account plan is not one purchase, it is one purchase +// per cloud account, and each of those succeeds or fails on its own. The plan +// has bought the step only when every one of them has. Both the advance gate +// (CompletePlanStep) and the plan-health stuck-ramp report read that fact out +// of the same CTE below so they cannot disagree about which plans are frozen. +// +// WHICH ACCOUNTS COUNT +// The accounts the step actually targeted, taken from the execution rows the +// fan-out wrote at the time, not from the plan's account list today. A plan's +// account set changes between steps; those rows do not. Detaching an account +// from a plan therefore does not release a step that account is holding open -- +// deleting the account itself does, because the cloud_account_id FK is ON +// DELETE SET NULL (migration 000011) and its rows drop out of the fan-out +// scope. Short of that, the operator's exits are the ordinary ones: retry the +// account until it buys, or cancel its row. +// +// WHY NOT DERIVE current_step INSTEAD +// The alternative in the issue was to stop storing CurrentStep and compute it +// as the highest fully-bought step. CleanupOldExecutions deletes completed +// executions past the retention horizon, so a derived position would fall back +// toward zero as the rows aged out and the plan would buy its whole ramp a +// second time. Deriving it also does not avoid the freeze it was meant to +// avoid: contiguously it stalls on the same incomplete step, and +// non-contiguously it jumps over it, which is the overstatement the +// skipped-predecessor refusal exists to prevent. + +// The ramp_step CTE names the (plan_id, step_number) pairs a query is about. +// rampStepLatestAttemptCTE joins against it by that name, so every query below +// opens with exactly one of these two definitions. +const ( + // rampStepScopeOne is the single (plan, step) a completion is about. + rampStepScopeOne = ` + ramp_step AS (SELECT $1::uuid AS plan_id, $2::int AS step_number)` + + // rampStepScopeNextPerPlan is every plan's next uncounted ramp step. + rampStepScopeNextPerPlan = ` + ramp_step AS ( + SELECT p.id AS plan_id, + COALESCE((p.ramp_schedule ->> 'current_step')::int, 0) + 1 AS step_number + FROM purchase_plans p)` +) + +// rampStepLatestAttemptCTE reduces a ramp step's execution rows to one row per +// fan-out unit: the attempt that currently speaks for that unit. +// +// Scope. A fanned-out step writes one row per cloud account plus a root row +// carrying no account. The root row's status is an aggregate of its children +// (partially_completed when some bought, failed when none did), so counting it +// alongside them would let an all-accounts-failed step stay blocked by its own +// container forever. When any row for the step names an account, only the +// account rows count; otherwise the single root row is the unit. +// +// Latest attempt. Within a unit the ordering picks, in order: a row no retry +// has superseded (retry_execution_id IS NULL) over one that has, then the most +// recently written row, then execution_id so the pick is total and stable. +// Both keys are load-bearing and neither subsumes the other. A retry successor +// and the predecessor it supersedes are written in one transaction and so +// share an updated_at, which only the supersession key separates; a re-drive +// of a failed ROOT row re-fans-out and writes fresh account rows that supersede +// nothing, which only updated_at separates. Missing either lets a dead attempt +// speak for an account that has since bought, and freeze the ramp on it. +const rampStepLatestAttemptCTE = ` + latest_attempt AS ( + SELECT DISTINCT ON (e.plan_id, e.cloud_account_id) + e.plan_id, e.step_number, e.status + FROM purchase_executions e + JOIN ramp_step s + ON s.plan_id = e.plan_id AND s.step_number = e.step_number + WHERE e.cloud_account_id IS NOT NULL + OR NOT EXISTS ( + SELECT 1 + FROM purchase_executions f + WHERE f.plan_id = e.plan_id + AND f.step_number = e.step_number + AND f.cloud_account_id IS NOT NULL) + ORDER BY e.plan_id, e.cloud_account_id, + (e.retry_execution_id IS NULL) DESC, + e.updated_at DESC, + e.execution_id + )` + +// rampStepFanOutQuery counts, over one step's fan-out units, how many bought +// and how many are still holding the step open. +const rampStepFanOutQuery = `WITH` + rampStepScopeOne + `,` + rampStepLatestAttemptCTE + ` + SELECT count(*) FILTER (WHERE status = ANY($3)), + count(*) FILTER (WHERE status <> ALL($4)) + FROM latest_attempt` + +// stuckRampStepQuery reports every plan whose next ramp step has a fan-out unit +// sitting in a terminal-but-unsuccessful status with no retry in flight. +const stuckRampStepQuery = `WITH` + rampStepScopeNextPerPlan + `,` + rampStepLatestAttemptCTE + ` + SELECT plan_id, step_number, count(*) + FROM latest_attempt + WHERE status = ANY($1) + GROUP BY plan_id, step_number` + +// requireRampStepBought reports whether ramp step stepNumber of planID may be +// counted, using the transaction the caller already holds the plan row locked +// in. It returns a wrapped ErrRampStepIncomplete when the step is not fully +// bought, and that includes the case where no unit bought at all: a step with +// no successful execution has bought nothing, and an empty result set must not +// pass a guard whose whole job is to prove a purchase happened. +func requireRampStepBought(ctx context.Context, tx pgx.Tx, planID string, stepNumber int) error { + var bought, outstanding int + if err := tx.QueryRow(ctx, rampStepFanOutQuery, + planID, stepNumber, RampStepSucceededStatuses, RampStepSettledStatuses, + ).Scan(&bought, &outstanding); err != nil { + return fmt.Errorf("failed to read plan %s ramp step %d fan-out: %w", planID, stepNumber, err) + } + if bought == 0 { + return fmt.Errorf("%w: no execution of plan %s ramp step %d bought anything", + ErrRampStepIncomplete, planID, stepNumber) + } + if outstanding > 0 { + return fmt.Errorf("%w: %d account(s) of plan %s ramp step %d have not bought", + ErrRampStepIncomplete, outstanding, planID, stepNumber) + } + return nil +} + +// GetStuckRampSteps returns, keyed by plan ID, the plan's next ramp step and +// how many of that step's executions are stuck on it -- their latest +// attempt reached a terminal status without buying and no retry is in flight. +// +// Derived on every read rather than stamped on a row when the advance was +// refused, which is what makes it safe to score a plan's health on: a stamped +// refusal is true only at the instant it is written and nothing clears it, so a +// plan that recovered would stay marked unhealthy until an operator noticed. +// Recomputing from the same rows the advance gate reads means the report +// disappears exactly when the ramp unfreezes, and cannot drift from the gate. +// +// Plans with nothing stuck are absent from the map rather than present with a +// zero, so a caller cannot confuse "healthy" with "not reported". +func (s *PostgresStore) GetStuckRampSteps(ctx context.Context) (map[string]RampStepBlock, error) { + rows, err := s.db.Query(ctx, stuckRampStepQuery, RampStepStuckStatuses) + if err != nil { + return nil, fmt.Errorf("failed to query stuck ramp steps: %w", err) + } + defer rows.Close() + + blocks := make(map[string]RampStepBlock) + for rows.Next() { + var planID string + var block RampStepBlock + if err := rows.Scan(&planID, &block.StepNumber, &block.StuckExecutions); err != nil { + return nil, fmt.Errorf("failed to scan stuck ramp step: %w", err) + } + blocks[planID] = block + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to read stuck ramp steps: %w", err) + } + return blocks, nil +} diff --git a/internal/config/types.go b/internal/config/types.go index e972b4bac..69d40c34a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -403,6 +403,67 @@ const LegacyStatusCanceled = "cancel" + "led" // reaper.go) for rows left in approved/running past its threshold. const StatusFailed = "failed" +// StatusCompleted / StatusPartiallyCompleted are the two terminal statuses +// that mean commitment was actually bought. A partially-completed row bought +// some of its recommendations and failed others (issue #642); it is a real +// purchase, which is why re-approving one would double-buy. +const ( + StatusCompleted = "completed" + StatusPartiallyCompleted = "partially_completed" +) + +// StatusExpired is written when an approval lapsed before anyone acted on it. +const StatusExpired = "expired" + +// The three ramp-step status classes below drive the advance gate in +// CompletePlanStep and the stuck-ramp report GetStuckRampSteps (issue #1861). +// They are separate exported lists rather than one, because the gate asks two +// different questions of the same rows and the health report asks a third: +// +// - RampStepSucceededStatuses answers "did this fan-out unit buy?". A step +// no unit bought must not advance the ramp at all. +// - RampStepSettledStatuses answers "is this fan-out unit done holding the +// step open?". It is deliberately an ALLOWLIST: a status added to the +// schema later holds the step open until someone classifies it, which is +// the fail-closed direction on a money path. +// - RampStepStuckStatuses answers "is this unit stuck rather than in +// flight?". Only these produce the plan-health ramp_blocked factor, so a +// retry still working its way through pending/running reads as recovery +// rather than as a frozen ramp. +// +// Canceled counts as settled but neither succeeded nor stuck: to cancel a +// step's row is an operator deciding that unit will not buy, and that decision +// is what releases a ramp an unrecoverable account would otherwise freeze. +// Both spellings are listed for the duration of the expand-contract rename +// (migration 000089, contract in #1278), same as HealthScoredExecutionStatuses. +// +// RampStepSucceededStatuses is the same predicate migration 000098 spells out +// inline as status IN ('completed','partially_completed') when it decides which +// rows a sibling has already bought for. The migration is frozen SQL and cannot +// import this, so the two are kept identical by hand: changing one without the +// other would make the backfill and the advance gate disagree about what +// "bought" means. +var ( + RampStepSucceededStatuses = []string{StatusCompleted, StatusPartiallyCompleted} + RampStepSettledStatuses = []string{StatusCompleted, StatusPartiallyCompleted, StatusCanceled, LegacyStatusCanceled} + RampStepStuckStatuses = []string{StatusFailed, StatusExpired} +) + +// RampStepBlock reports a plan's next ramp step and how many of that step's +// executions are stuck on it. Returned by GetStuckRampSteps and rendered by the +// plan-health ramp_blocked factor (issue #1861). +// +// StuckExecutions counts executions rather than accounts because that is what +// the query can prove: a fanned-out step writes one row per cloud account, so +// the two coincide for the multi-account plans the factor exists for, but a +// step that never fanned out (an approval that expired before it ran) has one +// row standing for the whole step. Naming the count for the rows keeps the +// tooltip from asserting an account total it did not measure. +type RampStepBlock struct { + StepNumber int + StuckExecutions int +} + // HealthScoredExecutionStatuses are the execution statuses the plan health // score counts (internal/api/plan_health.go). It lives here, in the package // that owns both the status constants and the retention sweep, because two diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 6df67fc12..9353602ac 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -198,6 +198,26 @@ func (m *MockConfigStore) CompletePlanStep(ctx context.Context, planID string, s return args.Error(0) } +// GetStuckRampSteps mocks the stuck-ramp report backing plan health. +// Unregistered by default so the many tests that only care about the health +// score's execution counts stay green: no stuck steps is the shape of a plan +// with nothing wrong with it. +func (m *MockConfigStore) GetStuckRampSteps(ctx context.Context) (map[string]config.RampStepBlock, error) { + m.record("GetStuckRampSteps", ctx) + if !isExpected(&m.Mock, "GetStuckRampSteps") { + return map[string]config.RampStepBlock{}, nil + } + args := m.Called(ctx) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(map[string]config.RampStepBlock) + if !ok { + panic(fmt.Sprintf("mock: expected map[string]config.RampStepBlock, got %T", args.Get(0))) + } + return v, args.Error(1) +} + // UpdatePurchasePlanTx mocks the UpdatePurchasePlanTx operation. Falls // back to UpdatePurchasePlan when no expectation is registered so tests // that don't care about the Tx variant stay green -- same pattern as diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index e4f71b544..4e599c676 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -3,6 +3,7 @@ package purchase import ( "context" "errors" + "fmt" "sync" "testing" "time" @@ -521,6 +522,72 @@ func TestExecuteAndFinalize_RefusedRampAdvanceIsRecordedOnTheRow(t *testing.T) { mockStore.AssertNotCalled(t, "CompletePlanStep", mock.Anything, mock.Anything, mock.Anything) } +// TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow is the +// counterpart guard for issue #1861: a step still waiting on its other accounts +// is the ordinary shape of a multi-account fan-out being repaired one account +// at a time, and it stops being true the moment the last account buys. Stamping +// it would leave a permanent note describing a ramp that has since advanced, +// and would flip a cleanly-completed row into History's audit-gap rendering, +// which keys on a non-empty Error. Plan health derives that state live instead +// (config.GetStuckRampSteps). +func TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + exec := &config.PurchaseExecution{ + ExecutionID: "exec-incomplete-step", + PlanID: "plan-multi", + Status: "running", + StepNumber: 3, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + }, + } + + var saves []config.PurchaseExecution + mockStore.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + saves = append(saves, *e) + return nil + } + mockStore.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: id, Name: "Multi Plan"}, nil + } + mockStore.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { + return nil, nil + } + mockStore.On("CompletePlanStep", mock.Anything, "plan-multi", 3). + Return(fmt.Errorf("%w: 1 account(s) of plan plan-multi ramp step 3 have not bought", config.ErrRampStepIncomplete)) + mockStore.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Maybe() + mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Maybe() + mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceEC2, mock.Anything).Return(mockServiceClient, nil).Maybe() + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything). + Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil).Maybe() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + credStore: awsAccessKeyCredStore(), + dashboardURL: "https://dashboard.example.com", + } + + require.NoError(t, manager.executeAndFinalize(ctx, exec), + "a step waiting on a sibling account must not fail the purchase that just completed") + + require.NotEmpty(t, saves, "the terminal save must reach the store") + final := saves[len(saves)-1] + assert.Equal(t, "completed", final.Status) + assert.NotContains(t, final.Error, "ramp not advanced", + "a transient wait must leave no note that will outlive it") + mockStore.AssertCalled(t, "CompletePlanStep", mock.Anything, "plan-multi", 3) +} + func TestManager_GetAWSAccountID_Success(t *testing.T) { ctx := context.Background() mockSTS := new(MockSTSClient) diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index a30f76b48..ac12a63b1 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -246,29 +246,39 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE } if execErr == nil { if err := m.updatePlanProgress(ctx, exec); err != nil { - logging.Errorf("Failed to update plan progress: %v", err) m.recordRampAdvanceRefusal(ctx, exec, err) } } return execErr } -// recordRampAdvanceRefusal stamps a refused ramp advance onto the execution row -// that just completed, so the decision outlives the log retention window. +// recordRampAdvanceRefusal reports a ramp advance that did not happen, stamping +// it onto the execution row that just completed when the outcome is permanent +// so the decision outlives the log retention window. // // The refusal paths are new in issue #1669: the previous blind CurrentStep++ // could not decline, so a purchase always moved the ramp. Now money can be spent -// on a step the plan then declines to count (an unknown step_number, or a step -// more than one beyond CurrentStep), and CompletePlanStep returning before its -// write leaves next_execution_date stale, which shouldNotifyPlan reads as -// daysUntil < 0 and stops notifying that plan. A stall is therefore not +// on a step the plan then declines to count, and CompletePlanStep returning +// before its write leaves next_execution_date stale, which shouldNotifyPlan +// reads as daysUntil < 0 and stops notifying that plan. Such a stall is not // self-correcting, and a logging.Errorf was its only trace. // +// A step still waiting on its other accounts (issue #1861) is the exception and +// is deliberately NOT stamped. It is the ordinary shape of a partially-failed +// multi-account step being repaired one account at a time, and it stops being +// true the moment the last account buys -- a note written here would outlive the +// fact and describe a ramp that has since advanced. That state is derived live +// instead, by config.GetStuckRampSteps for the plan-health ramp_blocked factor. +// // The status stays "completed": the purchase did complete, and only the plan's -// progress accounting did not. Recovery is deliberately NOT scheduled here (see -// issue #1861); this records the fact so History shows it and an operator can -// act on it. +// progress accounting did not. Recovery is deliberately not scheduled here; this +// records the fact so History shows it and an operator can act on it. func (m *Manager) recordRampAdvanceRefusal(ctx context.Context, exec *config.PurchaseExecution, cause error) { + if errors.Is(cause, config.ErrRampStepIncomplete) { + logging.Warnf("Plan %s ramp step %d not advanced yet: %v", exec.PlanID, exec.StepNumber, cause) + return + } + logging.Errorf("Failed to update plan progress: %v", cause) exec.Error = appendErrNote(exec.Error, fmt.Sprintf("ramp not advanced: %v", cause)) if saveErr := m.config.SavePurchaseExecution(ctx, exec); saveErr != nil { logging.Errorf("AUDIT LOSS: failed to persist ramp-advance refusal for execution %s: %v", diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index 64f2e5e09..b4299d16a 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -159,14 +159,20 @@ func newRampStepFixture(ctx context.Context, t *testing.T) *rampStepFixture { return f } -// repairAccounts flips the two broken accounts to a resolvable auth mode, the -// operator-side precondition of a successful retry. +// repairAccount flips one broken account to a resolvable auth mode, the +// operator-side precondition of a successful retry for that account alone. +func (f *rampStepFixture) repairAccount(ctx context.Context, t *testing.T, i int) { + t.Helper() + acct := f.accounts[i] + acct.AWSAuthMode = "access_keys" + require.NoError(t, f.store.UpdateCloudAccount(ctx, &acct)) +} + +// repairAccounts repairs every broken account at once. func (f *rampStepFixture) repairAccounts(ctx context.Context, t *testing.T) { t.Helper() for i := 1; i < len(f.accounts); i++ { - acct := f.accounts[i] - acct.AWSAuthMode = "access_keys" - require.NoError(t, f.store.UpdateCloudAccount(ctx, &acct)) + f.repairAccount(ctx, t, i) } } @@ -209,27 +215,53 @@ func (f *rampStepFixture) saveRootExecution(ctx context.Context, t *testing.T, s return exec } +// failedAccountExecution returns the failed per-account row the fan-out wrote +// for accountID. Retry acts on a concrete predecessor row, so a faithful retry +// fixture has to find it rather than invent one. These tests drive one ramp +// step at a time, so an account has at most one failed row outstanding; more +// than one would make "which row is being retried" ambiguous and is rejected +// rather than resolved by picking arbitrarily. +func (f *rampStepFixture) failedAccountExecution(ctx context.Context, t *testing.T, accountID string) *config.PurchaseExecution { + t.Helper() + execs, err := f.store.GetExecutionsByStatuses(ctx, []string{"failed"}, config.DefaultListLimit) + require.NoError(t, err) + var found []config.PurchaseExecution + for _, e := range execs { + if e.PlanID == f.plan.ID && e.CloudAccountID != nil && *e.CloudAccountID == accountID { + found = append(found, e) + } + } + require.Len(t, found, 1, "expected exactly one outstanding failed row for account %s", accountID) + return &found[0] +} + // saveRetryExecution persists the successor row an operator's Retry produces // for one failed per-account row, mirroring api.persistRetryExecution: the // PlanID, StepNumber, CloudAccountID and idempotency lineage all propagate from -// the predecessor, and the row arrives already approved by the human who -// clicked Retry. -func (f *rampStepFixture) saveRetryExecution(ctx context.Context, t *testing.T, stepNumber int, accountID string) *config.PurchaseExecution { +// the predecessor, the row arrives already approved by the human who clicked +// Retry, and the predecessor is stamped with retry_execution_id pointing at the +// successor. That last write is what marks the failed row as superseded, so +// omitting it would model a retry no production path can produce. +func (f *rampStepFixture) saveRetryExecution(ctx context.Context, t *testing.T, accountID string) *config.PurchaseExecution { t.Helper() + predecessor := f.failedAccountExecution(ctx, t, accountID) acctID := accountID exec := &config.PurchaseExecution{ ExecutionID: uuid.New().String(), - IdempotencyKey: fmt.Sprintf("lineage-step-%d:%s", stepNumber, accountID), + IdempotencyKey: predecessor.IdempotencyKey, PlanID: f.plan.ID, CloudAccountID: &acctID, Status: "approved", - StepNumber: stepNumber, + StepNumber: predecessor.StepNumber, ScheduledDate: time.Now(), Recommendations: rampStepRecommendation(), Source: common.PurchaseSourceWeb, - RetryAttemptN: 1, + RetryAttemptN: predecessor.RetryAttemptN + 1, } require.NoError(t, f.store.SavePurchaseExecution(ctx, exec)) + + predecessor.RetryExecutionID = &exec.ExecutionID + require.NoError(t, f.store.SavePurchaseExecution(ctx, predecessor)) return exec } @@ -266,13 +298,13 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { // The operator fixes the two broken accounts and retries each failed row. f.repairAccounts(ctx, t) - retryB := f.saveRetryExecution(ctx, t, 3, f.accounts[1].ID) + retryB := f.saveRetryExecution(ctx, t, f.accounts[1].ID) require.NoError(t, f.execute(ctx, retryB)) require.Equal(t, 2, f.purchaseCount(), "B's retry must commit") - assert.Equal(t, 3, f.currentStep(ctx, t), - "the first successful completion of step 3 advances the ramp to 3") + assert.Equal(t, 2, f.currentStep(ctx, t), + "B bought but C has not, so step 3 is not complete (issue #1861)") - retryC := f.saveRetryExecution(ctx, t, 3, f.accounts[2].ID) + retryC := f.saveRetryExecution(ctx, t, f.accounts[2].ID) require.NoError(t, f.execute(ctx, retryC)) require.Equal(t, 3, f.purchaseCount(), "C's retry must commit") @@ -281,12 +313,14 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { "the plan has bought 3 of 4 ramp steps and must still say so") // Completing step 3 once more against this exact post-scenario state must - // take the no-op branch. Nothing else observable distinguishes it from the - // refusal branch (both return before the write and both leave CurrentStep - // at 3), and the caller only logs the error, so assert on the return value + // take the already-counted branch, which is distinguishable from the + // skipped-predecessor refusal only by its sentinel: both return before the + // write and both leave CurrentStep at 3. Assert on the return value // directly against real DB state rather than against pgxmock. - require.NoError(t, f.store.CompletePlanStep(ctx, f.plan.ID, 3), - "a counted step must no-op, not be refused as a skipped predecessor") + err := f.store.CompletePlanStep(ctx, f.plan.ID, 3) + require.ErrorIs(t, err, config.ErrRampStepAlreadyCounted, + "a counted step must report already-counted, not a skipped predecessor") + require.NotErrorIs(t, err, config.ErrRampStepIncomplete) // The ramp is not complete, so the plan still points at a next execution. plan, err := f.store.GetPurchasePlan(ctx, f.plan.ID) @@ -303,6 +337,101 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { assert.Equal(t, 6, f.purchaseCount(), "step 4 commits once per account") } +// TestRampStepWaitsForEveryAccountBeforeAdvancing is the issue #1861 +// regression guard, and the scenario the issue names directly. +// +// A 3-account plan fans ramp step 3 out: A buys, B and C fail. The operator +// repairs and retries ONLY B. Pre-fix, B's clean retry advanced the ramp to +// step 3 on its own, so the plan reported a step's worth of commitment it had +// bought for two accounts out of three, and step 3's tranche for C was never +// bought by anything. The ramp must stay on step 2 until C has bought too. +// +// The distinguishing measurement against #1669's guard is the intermediate +// state: that test drove BOTH retries and only checked the end state, which is +// step 3 either way. +func TestRampStepWaitsForEveryAccountBeforeAdvancing(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + + require.Equal(t, 2, f.currentStep(ctx, t), "fixture must start on step 2 of 4") + + root := f.saveRootExecution(ctx, t, 3) + require.NoError(t, f.execute(ctx, root)) + require.Equal(t, 1, f.purchaseCount(), "exactly one account (A) may commit on the first pass") + require.Equal(t, 2, f.currentStep(ctx, t), "a partially-failed step must not advance the ramp") + + // The operator repairs B only, and retries B only. C stays broken. + f.repairAccount(ctx, t, 1) + retryB := f.saveRetryExecution(ctx, t, f.accounts[1].ID) + require.NoError(t, f.execute(ctx, retryB)) + require.Equal(t, 2, f.purchaseCount(), "B's retry must commit") + + assert.Equal(t, 2, f.currentStep(ctx, t), + "2 of 3 accounts bought step 3, so the ramp must NOT report step 3 as done (issue #1861)") + + plan, err := f.store.GetPurchasePlan(ctx, f.plan.ID) + require.NoError(t, err) + assert.Nil(t, plan.LastExecutionDate, + "a blocked advance must not write the plan row at all") + + // Completing step 3 again while C is still outstanding must keep saying so + // rather than degrading into a silent no-op. + require.ErrorIs(t, f.store.CompletePlanStep(ctx, f.plan.ID, 3), config.ErrRampStepIncomplete) + + // C is repaired and retried: now every account of step 3 has bought, and + // exactly now the ramp advances. + f.repairAccount(ctx, t, 2) + retryC := f.saveRetryExecution(ctx, t, f.accounts[2].ID) + require.NoError(t, f.execute(ctx, retryC)) + require.Equal(t, 3, f.purchaseCount(), "C's retry must commit") + + assert.Equal(t, 3, f.currentStep(ctx, t), + "the last account of step 3 completing is what advances the ramp") +} + +// TestStuckRampStepIsReportedForPlanHealth pins the durable, self-healing +// signal issue #1861 asks plan_health to key on: a ramp step the plan cannot +// count because an account's latest attempt failed with no retry in flight. +// +// It is deliberately measured against the SAME fixture state the advance gate +// refuses on, because the two predicates have to agree: a plan whose ramp is +// frozen must be reported frozen, and a plan that recovers must stop being +// reported without anyone clearing a stored flag. +func TestStuckRampStepIsReportedForPlanHealth(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.plan.ID, "a plan with no executions yet is not stuck") + + root := f.saveRootExecution(ctx, t, 3) + require.NoError(t, f.execute(ctx, root)) + + stuck, err = f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + require.Contains(t, stuck, f.plan.ID, "B and C failed step 3 with no retry in flight") + assert.Equal(t, config.RampStepBlock{StepNumber: 3, StuckExecutions: 2}, stuck[f.plan.ID]) + + // A retry in flight is recovery, not a stuck ramp: B's failed row is + // superseded and its successor has not failed. + f.repairAccounts(ctx, t) + retryB := f.saveRetryExecution(ctx, t, f.accounts[1].ID) + stuck, err = f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.Equal(t, config.RampStepBlock{StepNumber: 3, StuckExecutions: 1}, stuck[f.plan.ID], + "only C is still stuck once B has a retry pending") + + require.NoError(t, f.execute(ctx, retryB)) + retryC := f.saveRetryExecution(ctx, t, f.accounts[2].ID) + require.NoError(t, f.execute(ctx, retryC)) + + stuck, err = f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.plan.ID, + "the report must clear itself once the step completes, with nothing to reset") +} + // TestCompletePlanStepIsIdempotentUnderConcurrency answers the question the // row lock is supposed to settle: can two concurrent completions of the SAME // ramp step both pass the guard and both advance? @@ -331,12 +460,18 @@ func TestCompletePlanStepIsIdempotentUnderConcurrency(t *testing.T) { wg.Wait() close(errCh) - seen := 0 + seen, advanced := 0, 0 for err := range errCh { seen++ - require.NoError(t, err, "every concurrent completion of step 3 must succeed or no-op, never error") + if err == nil { + advanced++ + continue + } + require.ErrorIs(t, err, config.ErrRampStepAlreadyCounted, + "a concurrent completion of step 3 either advances or reports already-counted") } require.Equal(t, racers, seen, "all racers must have reported") + require.Equal(t, 1, advanced, "exactly one racer may advance the ramp") reloaded, err := store.GetPurchasePlan(ctx, plan.ID) require.NoError(t, err) @@ -344,7 +479,11 @@ func TestCompletePlanStepIsIdempotentUnderConcurrency(t *testing.T) { "%d concurrent completions of step 3 must leave the ramp on step 3", racers) } -// saveConcurrencyRampPlan persists a plan sitting on step 2 of 4. +// saveConcurrencyRampPlan persists a plan sitting on step 2 of 4 whose step-3 +// execution has already completed. The execution row is not decoration: the +// advance gate refuses a step no execution bought (issue #1861), so a plan with +// an empty executions table can never advance and these tests would measure the +// refusal instead of the race. func saveConcurrencyRampPlan(ctx context.Context, t *testing.T, store *config.PostgresStore, name string) *config.PurchasePlan { t.Helper() plan := &config.PurchasePlan{ @@ -356,6 +495,13 @@ func saveConcurrencyRampPlan(ctx context.Context, t *testing.T, store *config.Po }, } require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + require.NoError(t, store.SavePurchaseExecution(ctx, &config.PurchaseExecution{ + ExecutionID: uuid.New().String(), + PlanID: plan.ID, + Status: "completed", + StepNumber: 3, + ScheduledDate: time.Now(), + })) return plan } @@ -424,7 +570,8 @@ func TestCompletePlanStepBlocksOnTheRowLockAndSeesTheWinner(t *testing.T) { select { case loserErr := <-done: - require.NoError(t, loserErr, "the loser of the race must no-op cleanly, not error") + require.ErrorIs(t, loserErr, config.ErrRampStepAlreadyCounted, + "the loser of the race must report already-counted, not re-advance") case <-time.After(15 * time.Second): t.Fatal("CompletePlanStep never returned after the row lock was released") } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index ce0afc499..c43865579 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -53,6 +53,10 @@ func (m *mockConfigStoreForHealth) CompletePlanStep(_ context.Context, _ string, return nil } +func (m *mockConfigStoreForHealth) GetStuckRampSteps(_ context.Context) (map[string]config.RampStepBlock, error) { + return nil, nil +} + func (m *mockConfigStoreForHealth) UpdatePurchasePlanTx(_ context.Context, _ pgx.Tx, _ *config.PurchasePlan) error { return nil } From 733859ce1f980a00450450c855fdaf9413db4999 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 20 Aug 2026 14:24:20 +0200 Subject: [PATCH 2/3] fix(plans): give the ramp gate an exit, and stop a frozen ramp double-buying (#1861) Adversarial review of the completeness gate found that it closed the under-buy the issue was filed about and opened two new money-path exposures, plus one non-monotone reduction, one false health signal and a set of load-bearing SQL clauses no test pinned. An account can become permanently unable to buy: an Azure savings-plans row is refused re-drive by design, and a failed row cannot be canceled at all, since IsCancelable and CancelExecutionAtomic both admit only pending/notified/scheduled. Such a unit held its step open forever and the plan never bought the rest of its ramp, which is the caveat the issue attached to this design. The gate now counts only units whose cloud account the plan still targets, so detaching the account is a non-destructive, reversible exit that releases the step. The rows still decide which units exist, so an account added later is not retroactively required to have bought an earlier step. While a ramp is held, CurrentStep stops moving and the plan-scoped create endpoint keeps stamping CurrentStep+1: the very step some accounts already bought. That row is a root row, so approving it re-fans-out across every account under a fresh idempotency lineage whose derived tokens miss the provider dedupe entirely, buying the same commitment twice on the routine action an operator takes when a plan looks stuck. Creating executions for a step a unit has already bought is now refused, and fails closed when the check itself cannot run. Whether a unit has bought is now read from any succeeded row rather than from its latest attempt. A purchase cannot be un-made, and the previous reduction let a later failed attempt re-open a bought step, whereupon the stuck report named that account and following it bought twice. Latest-attempt semantics stay where they belong, on the in-flight and stuck questions. A step counted by a sibling execution of the SAME step is separated from one the ramp has moved past. Only the latter is an anomaly worth an audit note; stamping the former put "ramp not advanced" on a cleanly completed purchase and flipped it into History's audit-gap rendering, which is the consequence the incomplete case was already exempted for. The stuck report is restricted to plans with a ramp still running. Without it, an ordinary failed purchase on an "immediate" plan came back as a blocked ramp step, quoting a step on a plan that has no ramp and penalizing it a second time for a row failed_executions already counts. Self-review then found the unit reduction keyed only on account and not on step, which is invisible for the single-step queries but collapses an account across every step of the new range query. Fixed with the step in both keys. Verified with a mutation harness: removing each clause individually (attachment test, ever_bought window, ramp scope filter, root-row exclusion, updated_at ordering, supersession ordering, step key) and confirming a named test fails, then restoring and confirming the tree is byte-identical. All seven were killed; two of the ordering tests had to be rewritten first because they survived, since the ever_bought change had made the representative row irrelevant to the answer they asserted. The create guard and the sibling discrimination were each confirmed failing before the fix and passing after. Full unit suite, integration suites for config/api/purchase against a real migrated Postgres, go vet with and without the integration tag, golangci-lint at the CI-pinned v2.10.1 and gocyclo -over 10 all clean. Declined, with the reasoning recorded next to the code: a fan-out that buys but then fails to persist an account's row leaves that account with no unit, so the step can still be counted without it; closing that needs the fan-out width recorded durably, which needs a migration. Legacy scopeless per-account rows are indistinguishable from root rows here. partially_completed continues to count as bought, because treating it otherwise turns every partial recommendation failure into a ramp freeze. overdue continues to stack with ramp_blocked, because suppressing it would score a stopped plan higher than a merely late one. --- internal/analytics/collector_test.go | 4 + internal/api/handler_plans.go | 47 ++ internal/api/handler_plans_test.go | 125 ++++ internal/config/errors.go | 19 +- internal/config/interfaces.go | 6 + internal/config/store_postgres.go | 10 +- .../store_postgres_complete_step_test.go | 79 ++- internal/config/store_postgres_ramp_step.go | 229 ++++++-- ...ore_postgres_ramp_step_integration_test.go | 547 ++++++++++++++++++ internal/mocks/stores.go | 20 + internal/purchase/execution_test.go | 75 ++- internal/purchase/manager.go | 22 +- .../ramp_step_progress_integration_test.go | 14 +- internal/server/test_helpers_test.go | 4 + 14 files changed, 1099 insertions(+), 102 deletions(-) create mode 100644 internal/config/store_postgres_ramp_step_integration_test.go diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 456bea744..f784943c7 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -168,6 +168,10 @@ func (m *mockConfigStore) GetStuckRampSteps(_ context.Context) (map[string]confi return nil, nil } +func (m *mockConfigStore) BoughtRampStepsInRange(_ context.Context, _ string, _, _ int) ([]int, error) { + return nil, nil +} + func (m *mockConfigStore) UpdatePurchasePlanTx(ctx context.Context, _ pgx.Tx, plan *config.PurchasePlan) error { return nil } diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 2fa84b33f..0f4f510cb 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -354,6 +354,10 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La return nil, err } + if err := h.refusePartlyBoughtRampSteps(ctx, plan, planID, req.Count); err != nil { + return nil, err + } + // Atomic write: per-row execution inserts and the plan's // next_execution_date bump commit together, or roll back together. // The previous implementation called SavePurchaseExecution outside @@ -392,6 +396,49 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La return &CreatePlannedPurchasesResponse{Created: created}, nil } +// refusePartlyBoughtRampSteps blocks a create whose steps overlap one an +// account has already bought (issue #1861). +// +// createPurchaseExecutionsTx stamps CurrentStep+1 .. CurrentStep+count. The +// completeness gate holds CurrentStep still while any account of a step is +// outstanding, so on a plan an operator is trying to unstick, CurrentStep+1 is +// the very step that is partly bought. The row minted for it is a ROOT row: +// approving it re-fans-out across every account on the plan, including the ones +// that already bought, under a fresh idempotency lineage (the new root's key is +// a new UUID, and each account's token derives from it), so the provider-side +// dedupe never engages and the commitment is genuinely bought twice. Per-account +// retry of the outstanding rows is the operation that actually finishes the step. +// +// Fails closed: an unreadable check refuses the create rather than minting rows +// that might double-buy. +func (h *Handler) refusePartlyBoughtRampSteps(ctx context.Context, plan *config.PurchasePlan, planID string, count int) error { + from := plan.RampSchedule.CurrentStep + 1 + bought, err := h.config.BoughtRampStepsInRange(ctx, planID, from, from+count-1) + if err != nil { + logging.Errorf("createPlannedPurchases: BoughtRampStepsInRange failed (plan=%s): %v", planID, err) + return NewClientError(503, "could not verify which ramp steps are already bought; try again") + } + if len(bought) == 0 { + return nil + } + return NewClientError(409, fmt.Sprintf( + "ramp step(s) %s of this plan have already been bought by at least one cloud account; "+ + "retry the outstanding per-account purchases to finish the step instead of scheduling it again", + formatRampSteps(bought))) +} + +// formatRampSteps renders step numbers for an operator-facing message. +func formatRampSteps(steps []int) string { + out := "" + for i, s := range steps { + if i > 0 { + out += ", " + } + out += fmt.Sprintf("%d", s) + } + return out +} + // parseCreatePurchasesRequest parses and validates the create purchases request. func (h *Handler) parseCreatePurchasesRequest(body string) (*CreatePlannedPurchasesRequest, time.Time, error) { var req CreatePlannedPurchasesRequest diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 38ab1f6e3..90dae2741 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -1666,3 +1666,128 @@ func TestHandler_listPlans_StuckRampProducesTheBlockedFactor(t *testing.T) { require.Len(t, late.HealthFactors, 1) assert.Equal(t, HealthFactorBehindSchedule, late.HealthFactors[0].Code) } + +// TestHandler_createPlannedPurchases_RefusesAPartlyBoughtStep is the issue +// #1861 double-buy guard. +// +// The completeness gate holds CurrentStep still while any account of a step is +// outstanding. An operator looking at a plan that has stopped advancing reaches +// for "create planned purchases", and createPurchaseExecutionsTx stamps +// CurrentStep+1 -- the very step some accounts already bought. The row it mints +// is a ROOT row, so approving it re-fans-out across every account on the plan +// under a fresh idempotency lineage: the accounts that already committed buy the +// same commitment a second time, with no provider-side dedupe to catch it. +func TestHandler_createPlannedPurchases_RefusesAPartlyBoughtStep(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + planID := "11111111-1111-1111-1111-111111111111" + mockStore.On("GetPurchasePlan", ctx, planID).Return(&config.PurchasePlan{ + ID: planID, + Name: "Frozen Ramp", + RampSchedule: config.RampSchedule{ + Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, + CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -21), + }, + }, nil) + // Step 3 is the frozen step: one account bought it, another has not. + mockStore.On("BoughtRampStepsInRange", ctx, planID, 3, 4).Return([]int{3}, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"count":2,"start_date":"2026-09-01"}`, + } + + _, err := handler.createPlannedPurchases(ctx, req, planID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "a partly-bought step must map to an HTTP status, not a 500") + assert.Equal(t, 409, ce.code) + assert.Contains(t, err.Error(), "3") + assert.Contains(t, err.Error(), "already been bought") + + // Nothing may be minted, and the plan pointer must not move. + mockStore.AssertNotCalled(t, "WithTx", mock.Anything, mock.Anything) +} + +// TestHandler_createPlannedPurchases_AllowsAnUnstartedStep is the positive +// control: the guard must not block the ordinary case, or it would break +// scheduling entirely while looking like it works. +func TestHandler_createPlannedPurchases_AllowsAnUnstartedStep(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + planID := "11111111-1111-1111-1111-111111111111" + plan := &config.PurchasePlan{ + ID: planID, + Name: "Healthy Ramp", + RampSchedule: config.RampSchedule{ + Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, + CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -21), + }, + } + mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) + mockStore.On("BoughtRampStepsInRange", ctx, planID, 3, 4).Return([]int{}, nil) + mockStore.On("SavePurchaseExecutionTx", ctx, mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("UpdatePurchasePlanTx", ctx, mock.Anything, mock.AnythingOfType("*config.PurchasePlan")).Return(nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"count":2,"start_date":"2026-09-01"}`, + } + + resp, err := handler.createPlannedPurchases(ctx, req, planID) + require.NoError(t, err) + assert.Equal(t, 2, resp.Created) +} + +// TestHandler_createPlannedPurchases_FailsClosedWhenTheProbeFails: the guard +// protects a money path, so an unreadable answer must refuse rather than mint +// rows that might re-fan-out over accounts that already bought. +func TestHandler_createPlannedPurchases_FailsClosedWhenTheProbeFails(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + planID := "11111111-1111-1111-1111-111111111111" + mockStore.On("GetPurchasePlan", ctx, planID).Return(&config.PurchasePlan{ + ID: planID, Name: "Frozen Ramp", + RampSchedule: config.RampSchedule{ + Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, + CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -21), + }, + }, nil) + mockStore.On("BoughtRampStepsInRange", ctx, planID, 3, 3).Return(nil, errors.New("connection reset")) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"count":1,"start_date":"2026-09-01"}`, + } + + _, err := handler.createPlannedPurchases(ctx, req, planID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 503, ce.code) + mockStore.AssertNotCalled(t, "WithTx", mock.Anything, mock.Anything) +} diff --git a/internal/config/errors.go b/internal/config/errors.go index a20f046e0..f87ae2143 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -26,12 +26,21 @@ var ErrExecutionNotInExpectedStatus = errors.New("execution not in expected stat // its own specifics; naming one of them here would mis-describe the other. var ErrRampStepIncomplete = errors.New("ramp step incomplete") +// ErrRampStepCountedBySibling is returned (wrapped) by CompletePlanStep when +// the plan is sitting exactly on the completing step: a sibling execution of +// the same step won the race to advance it, moments earlier. Routine on a +// multi-account step whose last two accounts finish together, and benign -- +// the step WAS counted, just not by this caller -- so it must not be stamped on +// the execution row. A note there would flip a cleanly-completed purchase into +// History's audit-gap rendering, which keys on a non-empty error. +var ErrRampStepCountedBySibling = errors.New("ramp step already counted by a sibling execution") + // ErrRampStepAlreadyCounted is returned (wrapped) by CompletePlanStep when the -// plan has already counted the completing step. Unlike ErrRampStepIncomplete -// this is permanent and worth an audit note on the execution row: the purchase -// bought commitment for a step the ramp will never count again, which is how a -// step_number stamped by pre-#1669 code (or during that deploy overlap) -// surfaces. +// plan has moved PAST the completing step. Unlike ErrRampStepCountedBySibling +// this is an anomaly worth an audit note on the execution row: the purchase +// bought commitment for a step the ramp counted earlier and will never count +// again, which is how a step_number stamped by pre-#1669 code (or during that +// deploy overlap) surfaces. var ErrRampStepAlreadyCounted = errors.New("ramp step already counted") // ErrAuditLoss is returned (wrapped) by executeAndFinalize when the purchase diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 130c5985e..0c8b185f7 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -53,6 +53,12 @@ type StoreInterface interface { // nothing stuck are absent from the map. Feeds the plan-health // ramp_blocked factor (issue #1861). GetStuckRampSteps(ctx context.Context) (map[string]RampStepBlock, error) + // BoughtRampStepsInRange returns the steps in [from, to] of planID that + // already have a fan-out unit which bought, ascending. Callers about to + // mint executions for a step range use it to refuse a step that is partly + // bought, which would otherwise re-fan-out across accounts that already + // committed under a fresh idempotency lineage (issue #1861). + BoughtRampStepsInRange(ctx context.Context, planID string, from, to int) ([]int, error) // UpdatePurchasePlanTx is the tx-accepting variant of UpdatePurchasePlan. // Used from createPlannedPurchases' WithTx block so the per-row // SavePurchaseExecutionTx writes and the plan's next_execution_date diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index a973b4b59..382ecbae3 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -660,8 +660,14 @@ func (s *PostgresStore) CompletePlanStep(ctx context.Context, planID string, ste // the caller's transaction, so the read-modify-write cannot lose an update. func advanceRampStep(ctx context.Context, tx pgx.Tx, plan *PurchasePlan, stepNumber int) error { switch { - case plan.RampSchedule.CurrentStep >= stepNumber: - return fmt.Errorf("%w: plan %s is on ramp step %d and cannot count step %d again", + case plan.RampSchedule.CurrentStep == stepNumber: + // A sibling of this very step advanced the ramp first. The step is + // counted and nothing is wrong, so this stays distinguishable from the + // case below: only that one is worth recording against the execution. + return fmt.Errorf("%w: plan %s is already on ramp step %d", + ErrRampStepCountedBySibling, plan.ID, stepNumber) + case plan.RampSchedule.CurrentStep > stepNumber: + return fmt.Errorf("%w: plan %s is on ramp step %d, past the completing step %d", ErrRampStepAlreadyCounted, plan.ID, plan.RampSchedule.CurrentStep, stepNumber) case plan.RampSchedule.CurrentStep != stepNumber-1: return fmt.Errorf("refusing to advance plan %s from ramp step %d to %d: step(s) %d-%d never completed", diff --git a/internal/config/store_postgres_complete_step_test.go b/internal/config/store_postgres_complete_step_test.go index dc753ddb0..ad87c3188 100644 --- a/internal/config/store_postgres_complete_step_test.go +++ b/internal/config/store_postgres_complete_step_test.go @@ -23,8 +23,9 @@ package config // These tests pin the fix's contract: // - the read happens inside a transaction (Begin) and carries FOR UPDATE, // - completing step N from CurrentStep N-1 persists CurrentStep = N, -// - completing a step the ramp already passed writes nothing and reports -// ErrRampStepAlreadyCounted, +// - completing the step the ramp is sitting on writes nothing and reports +// ErrRampStepCountedBySibling, while completing a step it has moved past +// reports ErrRampStepAlreadyCounted -- only the latter is an anomaly, // - completing a step whose fan-out still has an outstanding account, or // which nothing bought, writes nothing and reports ErrRampStepIncomplete, // - completing a step more than one beyond CurrentStep is refused, @@ -113,13 +114,26 @@ func completeStepUpdateArgs(wantStep int, staleUpdatedAt time.Time, wantNextNil } // expectFanOut registers the ramp step's fan-out tally that CompletePlanStep -// consults before advancing (issue #1861): how many of the step's units bought -// and how many are still holding it open. The pattern requires the DISTINCT ON -// reduction, so a store that stopped taking the latest attempt per account -- -// and therefore let a dead attempt block or a stale one pass -- fails to match. +// consults before advancing (issue #1861): how many of the step's units have +// bought and how many are still holding it open. +// +// The pattern names each clause of that query which, if dropped, still yields a +// query that runs and returns two numbers -- silently wrong ones. Deleting the +// plan_accounts test removes the gate's only release valve; deleting the +// root-row exclusion lets an all-accounts-failed step stay blocked by its own +// container; deleting DISTINCT ON or the updated_at ordering lets a dead +// attempt speak for a unit; dropping step_number from the reduction keys +// collapses an account across steps; deleting ever_bought re-opens a step an +// account has already bought. The behavior of each is measured against a real database in +// store_postgres_ramp_step_integration_test.go; these are the cheap tripwires +// that fire in the default test run. func expectFanOut(mock pgxmock.PgxPoolIface, planID string, stepNumber, bought, outstanding int) { - mock.ExpectQuery(`WITH[\s\S]*DISTINCT ON \(e\.plan_id, e\.cloud_account_id\)[\s\S]*FROM latest_attempt`). - WithArgs(planID, stepNumber, RampStepSucceededStatuses, RampStepSettledStatuses). + mock.ExpectQuery(`WITH[\s\S]*FROM plan_accounts pa[\s\S]*`+ + `DISTINCT ON \(e\.plan_id, e\.step_number, e\.cloud_account_id\)[\s\S]*`+ + `bool_or\(e\.status = ANY\(\$1\)\)[\s\S]*`+ + `NOT EXISTS[\s\S]*FROM eligible f[\s\S]*`+ + `e\.updated_at DESC[\s\S]*FROM unit`). + WithArgs(RampStepSucceededStatuses, RampStepSettledStatuses, planID, stepNumber). WillReturnRows(pgxmock.NewRows([]string{"bought", "outstanding"}).AddRow(bought, outstanding)) } @@ -224,19 +238,24 @@ func TestPGXMock_CompletePlanStep_StepNothingBoughtIsRefused(t *testing.T) { require.NoError(t, mock.ExpectationsWereMet()) } -// TestPGXMock_CompletePlanStep_AlreadyCountedStepWritesNothing is the issue +// TestPGXMock_CompletePlanStep_SiblingCountedStepWritesNothing is the issue // #1669 guard at the store boundary: the second per-account retry of one ramp -// step completes a step the plan already counted, and must leave the row -// exactly as it found it. pgxmock fails the test if any UPDATE is issued, -// because no ExpectExec is registered. -func TestPGXMock_CompletePlanStep_AlreadyCountedStepWritesNothing(t *testing.T) { +// step completes a step a sibling just counted, and must leave the row exactly +// as it found it. pgxmock fails the test if any UPDATE is issued, because no +// ExpectExec is registered. +// +// The sentinel is the sibling one, not the already-counted one, and the +// distinction is load-bearing (issue #1861): only the latter is stamped onto +// the execution row, and stamping this routine case would flip a cleanly +// completed purchase into History's audit-gap rendering. +func TestPGXMock_CompletePlanStep_SiblingCountedStepWritesNothing(t *testing.T) { mock := newMock(t) store := storeWith(mock) ctx := context.Background() now := time.Now().Truncate(time.Second) stale := now.AddDate(0, 0, -30) - // The ramp already advanced to 3 when the first retry of step 3 landed. + // The ramp advanced to 3 when the first retry of step 3 landed moments ago. ramp := RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 3, TotalSteps: 4, StartDate: now} mock.ExpectBegin() @@ -246,8 +265,36 @@ func TestPGXMock_CompletePlanStep_AlreadyCountedStepWritesNothing(t *testing.T) mock.ExpectRollback() err := store.CompletePlanStep(ctx, "plan-123", 3) - require.ErrorIs(t, err, ErrRampStepAlreadyCounted, - "re-completing a counted step is reported, not silently dropped") + require.ErrorIs(t, err, ErrRampStepCountedBySibling, + "a step the plan is sitting on was counted by a sibling, which is routine") + require.NotErrorIs(t, err, ErrRampStepAlreadyCounted, + "the two must stay distinguishable: only the passed-step case is stamped on the row") + require.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CompletePlanStep_PassedStepIsReportedAsAnomalous covers the other +// half of that split: a row completing a step the ramp moved PAST bought +// commitment the plan counted earlier and will never count again. That is the +// mis-stamped-row case issue #1861 asks to be recorded, so it must reach the +// caller under its own sentinel. +func TestPGXMock_CompletePlanStep_PassedStepIsReportedAsAnomalous(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + stale := now.AddDate(0, 0, -30) + ramp := RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 3, TotalSteps: 6, StartDate: now} + + mock.ExpectBegin() + mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). + WithArgs("plan-past"). + WillReturnRows(rampPlanRows(t, "plan-past", ramp, now, stale, sql.NullTime{Valid: true, Time: now})) + mock.ExpectRollback() + + err := store.CompletePlanStep(ctx, "plan-past", 2) + require.ErrorIs(t, err, ErrRampStepAlreadyCounted) + require.NotErrorIs(t, err, ErrRampStepCountedBySibling) require.NoError(t, mock.ExpectationsWereMet()) } diff --git a/internal/config/store_postgres_ramp_step.go b/internal/config/store_postgres_ramp_step.go index 213e7940f..57ab5ae3c 100644 --- a/internal/config/store_postgres_ramp_step.go +++ b/internal/config/store_postgres_ramp_step.go @@ -13,17 +13,56 @@ import ( // per cloud account, and each of those succeeds or fails on its own. The plan // has bought the step only when every one of them has. Both the advance gate // (CompletePlanStep) and the plan-health stuck-ramp report read that fact out -// of the same CTE below so they cannot disagree about which plans are frozen. +// of the same CTEs below so they cannot disagree about which plans are frozen. // // WHICH ACCOUNTS COUNT -// The accounts the step actually targeted, taken from the execution rows the -// fan-out wrote at the time, not from the plan's account list today. A plan's -// account set changes between steps; those rows do not. Detaching an account -// from a plan therefore does not release a step that account is holding open -- -// deleting the account itself does, because the cloud_account_id FK is ON -// DELETE SET NULL (migration 000011) and its rows drop out of the fan-out -// scope. Short of that, the operator's exits are the ordinary ones: retry the -// account until it buys, or cancel its row. +// The execution rows the fan-out wrote at the time, intersected with the +// accounts the plan still targets. The rows decide WHICH units exist, so an +// account added later is not retroactively required to have bought an earlier +// step; the plan's current attachments decide whether a unit still MATTERS, so +// an account the plan no longer targets stops holding a step open. +// +// That intersection is the gate's release valve, and it has to exist. An +// account can become permanently unable to buy (revoked credentials, closed +// account, an Azure savings-plans row that purchase.RedriveRefusalReason +// refuses to retry by design), and a failed row cannot be canceled either -- +// IsCancelable and CancelExecutionAtomic both admit only pending/notified/ +// scheduled. Without the attachment test such a unit would hold its step open +// forever and the plan would never buy the rest of its ramp: the exact +// "permanently-failed account freezes the ramp" caveat issue #1861 attached to +// this design. Detaching the account from the plan (SetPlanAccounts, reachable +// from the plan-accounts endpoint) is a non-destructive, reversible operator +// action that says "this plan no longer buys for that account", and it releases +// the step. Deleting the cloud account works too, because the cloud_account_id +// FK is ON DELETE SET NULL (migration 000011), but that is plan-wide and +// destructive. +// +// ONCE BOUGHT, ALWAYS BOUGHT +// A purchase cannot be un-made, so "has this unit bought?" is answered by +// whether ANY row for it succeeded, never by its latest attempt. Reading it off +// the latest attempt would let a later failed attempt for an account that +// already bought re-open the step, and the stuck report would then tell an +// operator to retry an account that has already bought -- turning a reporting +// artifact into a duplicate commitment. Latest-attempt semantics apply only to +// the in-flight / stuck question, which is genuinely about the present. +// +// KNOWN GAP 1: the unit set is derived from rows that exist. If +// executeForAccount buys for an account and then fails to persist its row (the +// AUDIT LOSS path), that account has no unit and the step can be counted +// complete without it. Closing it needs the fan-out width recorded durably at +// fan-out time, which needs a migration; tracked separately. Note the failure +// needs an audit-loss event, and the same event equally hides a purchase that +// DID land. +// +// KNOWN GAP 2: root and per-account rows are told apart by cloud_account_id, +// which issue #1537 showed is not reliable for rows written before it landed -- +// purchase.reattachAccountScope exists because per-account rows with no scope +// are real, and it recognizes them by a colon in the idempotency lineage key, +// which this has no equivalent of. A step whose only rows are legacy scopeless +// per-account rows collapses them into one unit and the latest speaks for all +// of them, which is #1861's own bug for those rows. Such plans have almost +// always advanced past the affected steps already, so this is documented rather +// than fixed. // // WHY NOT DERIVE current_step INSTEAD // The alternative in the issue was to stop storing CurrentStep and compute it @@ -35,76 +74,133 @@ import ( // non-contiguously it jumps over it, which is the overstatement the // skipped-predecessor refusal exists to prevent. +// PARAMETER CONVENTION for the composed queries below: $1 is always +// RampStepSucceededStatuses, because the shared rampStepUnitCTE reads it. Each +// query binds its own parameters from $2 on. Changing this means changing every +// query in this file together. + // The ramp_step CTE names the (plan_id, step_number) pairs a query is about. -// rampStepLatestAttemptCTE joins against it by that name, so every query below -// opens with exactly one of these two definitions. +// rampStepUnitCTE joins against it by that name, so every query below opens +// with exactly one of these two definitions. const ( // rampStepScopeOne is the single (plan, step) a completion is about. rampStepScopeOne = ` - ramp_step AS (SELECT $1::uuid AS plan_id, $2::int AS step_number)` + ramp_step AS (SELECT $3::uuid AS plan_id, $4::int AS step_number)` - // rampStepScopeNextPerPlan is every plan's next uncounted ramp step. + // rampStepScopeNextPerPlan is the next uncounted step of every plan that + // has a ramp still running. + // + // The total_steps > 1 test is not an optimization. Without it every plan + // is in scope, including "immediate" ones (total_steps 1) whose single + // execution is stamped step 1 by default; one ordinary failed purchase on + // such a plan would be reported as a blocked ramp step on a plan that has + // no ramp, penalizing it for a row the failed_executions factor is already + // counting. The current_step < total_steps test drops finished ramps, + // which have no next step to block. rampStepScopeNextPerPlan = ` ramp_step AS ( SELECT p.id AS plan_id, COALESCE((p.ramp_schedule ->> 'current_step')::int, 0) + 1 AS step_number - FROM purchase_plans p)` + FROM purchase_plans p + WHERE COALESCE((p.ramp_schedule ->> 'total_steps')::int, 1) > 1 + AND COALESCE((p.ramp_schedule ->> 'current_step')::int, 0) + < COALESCE((p.ramp_schedule ->> 'total_steps')::int, 1))` ) -// rampStepLatestAttemptCTE reduces a ramp step's execution rows to one row per -// fan-out unit: the attempt that currently speaks for that unit. +// rampStepUnitCTE reduces a ramp step's execution rows to one row per fan-out +// unit, carrying that unit's current status and whether it has ever bought. +// +// `eligible` drops rows whose cloud account the plan no longer targets (see the +// release-valve note above). Rows with no account survive: they are root rows, +// judged below. // -// Scope. A fanned-out step writes one row per cloud account plus a root row +// `unit` picks the scope and the representative row, once per (step, account). +// Keying on the step as well as the account is only visibly load-bearing for a +// caller whose ramp_step spans a RANGE of steps -- BoughtRampStepsInRange does +// -- but it is the correct key regardless: without it one account collapses to +// a single row across every step in scope, so a step it bought and a step it +// has merely scheduled become indistinguishable. +// +// Scope: a fanned-out step writes one row per cloud account plus a root row // carrying no account. The root row's status is an aggregate of its children // (partially_completed when some bought, failed when none did), so counting it // alongside them would let an all-accounts-failed step stay blocked by its own -// container forever. When any row for the step names an account, only the -// account rows count; otherwise the single root row is the unit. +// container forever, even after every account was retried to success. When any +// eligible row for the step names an account, only account rows count; +// otherwise the single root row is the unit. Deriving that test from `eligible` +// rather than from the raw table matters: a step whose account rows are all +// detached must fall back to its root row, not end up with no units at all. +// +// Representative row: the ordering prefers a row no retry has superseded +// (retry_execution_id IS NULL), then the most recently written, then +// execution_id so the pick is total and stable (execution_id is unique, so no +// tie survives). Both leading keys are load-bearing and neither subsumes the +// other. A retry successor and the predecessor it supersedes are written in one +// transaction and so share an updated_at, which only the supersession key +// separates; a re-drive of a failed ROOT row re-fans-out and writes fresh rows +// that supersede nothing, which only updated_at separates. Missing either lets +// a dead attempt speak for a unit that has moved on. // -// Latest attempt. Within a unit the ordering picks, in order: a row no retry -// has superseded (retry_execution_id IS NULL) over one that has, then the most -// recently written row, then execution_id so the pick is total and stable. -// Both keys are load-bearing and neither subsumes the other. A retry successor -// and the predecessor it supersedes are written in one transaction and so -// share an updated_at, which only the supersession key separates; a re-drive -// of a failed ROOT row re-fans-out and writes fresh account rows that supersede -// nothing, which only updated_at separates. Missing either lets a dead attempt -// speak for an account that has since bought, and freeze the ramp on it. -const rampStepLatestAttemptCTE = ` - latest_attempt AS ( - SELECT DISTINCT ON (e.plan_id, e.cloud_account_id) - e.plan_id, e.step_number, e.status +// ever_bought is a window aggregate over the WHOLE unit, deliberately not the +// representative row: see the "once bought, always bought" note above. +const rampStepUnitCTE = ` + eligible AS ( + SELECT e.plan_id, e.step_number, e.status, e.cloud_account_id, + e.retry_execution_id, e.updated_at, e.execution_id FROM purchase_executions e JOIN ramp_step s ON s.plan_id = e.plan_id AND s.step_number = e.step_number + WHERE e.cloud_account_id IS NULL + OR EXISTS ( + SELECT 1 + FROM plan_accounts pa + WHERE pa.plan_id = e.plan_id + AND pa.account_id = e.cloud_account_id) + ), + unit AS ( + SELECT DISTINCT ON (e.plan_id, e.step_number, e.cloud_account_id) + e.plan_id, e.step_number, e.status, + bool_or(e.status = ANY($1)) + OVER (PARTITION BY e.plan_id, e.step_number, e.cloud_account_id) AS ever_bought + FROM eligible e WHERE e.cloud_account_id IS NOT NULL OR NOT EXISTS ( SELECT 1 - FROM purchase_executions f + FROM eligible f WHERE f.plan_id = e.plan_id AND f.step_number = e.step_number AND f.cloud_account_id IS NOT NULL) - ORDER BY e.plan_id, e.cloud_account_id, + ORDER BY e.plan_id, e.step_number, e.cloud_account_id, (e.retry_execution_id IS NULL) DESC, e.updated_at DESC, e.execution_id )` -// rampStepFanOutQuery counts, over one step's fan-out units, how many bought -// and how many are still holding the step open. -const rampStepFanOutQuery = `WITH` + rampStepScopeOne + `,` + rampStepLatestAttemptCTE + ` - SELECT count(*) FILTER (WHERE status = ANY($3)), - count(*) FILTER (WHERE status <> ALL($4)) - FROM latest_attempt` +// rampStepFanOutQuery counts, over one step's fan-out units, how many have +// bought and how many are still holding the step open. A unit that has bought +// can never be outstanding, however its latest attempt ended. +const rampStepFanOutQuery = `WITH` + rampStepScopeOne + `,` + rampStepUnitCTE + ` + SELECT count(*) FILTER (WHERE ever_bought), + count(*) FILTER (WHERE NOT ever_bought AND status <> ALL($2)) + FROM unit` -// stuckRampStepQuery reports every plan whose next ramp step has a fan-out unit -// sitting in a terminal-but-unsuccessful status with no retry in flight. -const stuckRampStepQuery = `WITH` + rampStepScopeNextPerPlan + `,` + rampStepLatestAttemptCTE + ` +// stuckRampStepQuery reports every running ramp whose next step has a unit that +// has never bought and whose latest attempt ended terminally unsuccessful. +const stuckRampStepQuery = `WITH` + rampStepScopeNextPerPlan + `,` + rampStepUnitCTE + ` SELECT plan_id, step_number, count(*) - FROM latest_attempt - WHERE status = ANY($1) + FROM unit + WHERE NOT ever_bought AND status = ANY($2) GROUP BY plan_id, step_number` +// boughtRampStepsQuery lists the steps in [$3, $4] of plan $2 that already have +// a unit which bought. Used to keep a new fan-out off a step whose commitment +// is partly made (issue #1861); see BoughtRampStepsInRange. +const boughtRampStepsQuery = `WITH + ramp_step AS ( + SELECT $2::uuid AS plan_id, generate_series($3::int, $4::int) AS step_number),` + + rampStepUnitCTE + ` + SELECT DISTINCT step_number FROM unit WHERE ever_bought ORDER BY step_number` + // requireRampStepBought reports whether ramp step stepNumber of planID may be // counted, using the transaction the caller already holds the plan row locked // in. It returns a wrapped ErrRampStepIncomplete when the step is not fully @@ -114,7 +210,7 @@ const stuckRampStepQuery = `WITH` + rampStepScopeNextPerPlan + `,` + rampStepLat func requireRampStepBought(ctx context.Context, tx pgx.Tx, planID string, stepNumber int) error { var bought, outstanding int if err := tx.QueryRow(ctx, rampStepFanOutQuery, - planID, stepNumber, RampStepSucceededStatuses, RampStepSettledStatuses, + RampStepSucceededStatuses, RampStepSettledStatuses, planID, stepNumber, ).Scan(&bought, &outstanding); err != nil { return fmt.Errorf("failed to read plan %s ramp step %d fan-out: %w", planID, stepNumber, err) } @@ -129,9 +225,44 @@ func requireRampStepBought(ctx context.Context, tx pgx.Tx, planID string, stepNu return nil } +// BoughtRampStepsInRange returns the steps between from and to (inclusive) of +// planID that already have a fan-out unit which bought, ascending. +// +// It exists so a caller about to mint executions for a range of steps can +// refuse to target one that is partly bought. That became reachable with the +// completeness gate: a step stays uncounted while an account is outstanding, so +// CurrentStep stops moving, and the plan-scoped create endpoint keeps stamping +// CurrentStep+1 -- the same step. Approving the result re-fans-out across every +// account including the ones that already bought, under a fresh idempotency +// lineage whose derived tokens miss the provider-side dedupe entirely, so it is +// a genuine duplicate commitment rather than a no-op. +func (s *PostgresStore) BoughtRampStepsInRange(ctx context.Context, planID string, from, to int) ([]int, error) { + if from > to { + return nil, fmt.Errorf("bought ramp steps of plan %s: range %d-%d is empty", planID, from, to) + } + rows, err := s.db.Query(ctx, boughtRampStepsQuery, RampStepSucceededStatuses, planID, from, to) + if err != nil { + return nil, fmt.Errorf("failed to query bought ramp steps of plan %s: %w", planID, err) + } + defer rows.Close() + + var steps []int + for rows.Next() { + var step int + if err := rows.Scan(&step); err != nil { + return nil, fmt.Errorf("failed to scan bought ramp step: %w", err) + } + steps = append(steps, step) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to read bought ramp steps of plan %s: %w", planID, err) + } + return steps, nil +} + // GetStuckRampSteps returns, keyed by plan ID, the plan's next ramp step and -// how many of that step's executions are stuck on it -- their latest -// attempt reached a terminal status without buying and no retry is in flight. +// how many of that step's units are stuck on it -- never bought, latest attempt +// terminal and unsuccessful, no retry in flight. // // Derived on every read rather than stamped on a row when the advance was // refused, which is what makes it safe to score a plan's health on: a stamped @@ -143,7 +274,7 @@ func requireRampStepBought(ctx context.Context, tx pgx.Tx, planID string, stepNu // Plans with nothing stuck are absent from the map rather than present with a // zero, so a caller cannot confuse "healthy" with "not reported". func (s *PostgresStore) GetStuckRampSteps(ctx context.Context) (map[string]RampStepBlock, error) { - rows, err := s.db.Query(ctx, stuckRampStepQuery, RampStepStuckStatuses) + rows, err := s.db.Query(ctx, stuckRampStepQuery, RampStepSucceededStatuses, RampStepStuckStatuses) if err != nil { return nil, fmt.Errorf("failed to query stuck ramp steps: %w", err) } diff --git a/internal/config/store_postgres_ramp_step_integration_test.go b/internal/config/store_postgres_ramp_step_integration_test.go new file mode 100644 index 000000000..d46ed2db2 --- /dev/null +++ b/internal/config/store_postgres_ramp_step_integration_test.go @@ -0,0 +1,547 @@ +//go:build integration +// +build integration + +package config + +// Real-DB tests for the ramp-step completeness gate and the stuck-ramp report +// (issue #1861). They build exact execution-row shapes and ask the SQL what it +// decides, because every clause the gate depends on is a predicate over rows +// that a mock cannot evaluate: whether an account is still attached to the +// plan, whether a unit ever bought as opposed to how its latest attempt ended, +// which row speaks for a unit, and whether the root row is a unit at all. +// +// The end-to-end fixtures in internal/purchase drive the executor and so can +// only produce the row shapes that flow naturally out of it. Several of the +// shapes below are reachable in production but not from that fixture (an +// all-accounts-failed root, a superseded root, a bought account with a later +// failed attempt), which is exactly why they had no coverage. + +import ( + "context" + "fmt" + "slices" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func setupRampStepStore(ctx context.Context, t *testing.T) *PostgresStore { + t.Helper() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { container.Cleanup(context.Background()) }) + + require.NoError(t, + migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", ""), + "migrations failed to apply to a fresh database") + return NewPostgresStore(container.DB) +} + +// rampFixture is a plan on step 2 of 4 with a set of attached cloud accounts. +type rampFixture struct { + store *PostgresStore + planID string + accounts []string +} + +// newRampFixture creates the plan and attaches accountCount cloud accounts. +// totalSteps drives the ramp shape so a test can build a non-ramp plan too. +func newRampFixture(ctx context.Context, t *testing.T, accountCount, currentStep, totalSteps int) *rampFixture { + t.Helper() + store := setupRampStepStore(ctx, t) + + plan := &PurchasePlan{ + Name: fmt.Sprintf("Ramp Fixture %s", uuid.New().String()[:8]), + Services: map[string]ServiceConfig{}, + RampSchedule: RampSchedule{ + Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, + CurrentStep: currentStep, TotalSteps: totalSteps, + StartDate: time.Now().AddDate(0, 0, -14), + }, + } + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + + f := &rampFixture{store: store, planID: plan.ID} + for i := 0; i < accountCount; i++ { + acct := &CloudAccount{ + Name: fmt.Sprintf("acct-%c", 'a'+i), + Provider: "aws", + ExternalID: fmt.Sprintf("22222222222%d", i), + Enabled: true, + } + require.NoError(t, store.CreateCloudAccount(ctx, acct)) + f.accounts = append(f.accounts, acct.ID) + } + require.NoError(t, store.SetPlanAccounts(ctx, f.planID, f.accounts)) + return f +} + +// row persists one execution for the fixture's plan. account may be nil for a +// root row. Rows are written oldest-first by the caller, and each write bumps +// updated_at, so call order is what the latest-attempt ordering sees. +func (f *rampFixture) row(ctx context.Context, t *testing.T, step int, account *string, status string) *PurchaseExecution { + t.Helper() + return f.rowWithID(ctx, t, uuid.New().String(), step, account, status) +} + +// rowWithID is row with the execution_id chosen by the caller, so a test can +// put the ordering keys in conflict on purpose: execution_id is the ordering's +// last resort, and a test that lets it agree with the keys above it cannot tell +// whether those keys are doing anything. +func (f *rampFixture) rowWithID(ctx context.Context, t *testing.T, execID string, step int, account *string, status string) *PurchaseExecution { + t.Helper() + exec := &PurchaseExecution{ + ExecutionID: execID, + IdempotencyKey: uuid.New().String(), + PlanID: f.planID, + CloudAccountID: account, + Status: status, + StepNumber: step, + ScheduledDate: time.Now(), + } + require.NoError(t, f.store.SavePurchaseExecution(ctx, exec)) + return exec +} + +// updatedAt reads a row's updated_at, so a test can prove the precondition its +// ordering assertion depends on instead of assuming it. +func (f *rampFixture) updatedAt(ctx context.Context, t *testing.T, execID string) time.Time { + t.Helper() + var ts time.Time + require.NoError(t, f.store.db.QueryRow(ctx, + `SELECT updated_at FROM purchase_executions WHERE execution_id = $1`, execID).Scan(&ts)) + return ts +} + +// supersede points predecessor at successor, the link persistRetryExecution +// writes onto a retried row. +func (f *rampFixture) supersede(ctx context.Context, t *testing.T, predecessor, successor *PurchaseExecution) { + t.Helper() + predecessor.RetryExecutionID = &successor.ExecutionID + require.NoError(t, f.store.SavePurchaseExecution(ctx, predecessor)) +} + +// retryInOneTx inserts a successor for predecessor and stamps the supersession +// link, both inside a single transaction -- the shape api.persistRetryExecution +// writes. It matters that this is one transaction: now() is transaction-scoped +// in Postgres, so both rows end up with the SAME updated_at, and the ordering +// that decides which of them speaks for the account cannot fall back on +// recency. Writing them separately would hand the test a discriminator +// production does not have. +func (f *rampFixture) retryInOneTx(ctx context.Context, t *testing.T, predecessor *PurchaseExecution, successorID, status string) *PurchaseExecution { + t.Helper() + successor := &PurchaseExecution{ + ExecutionID: successorID, + IdempotencyKey: predecessor.IdempotencyKey, + PlanID: f.planID, + CloudAccountID: predecessor.CloudAccountID, + Status: status, + StepNumber: predecessor.StepNumber, + ScheduledDate: time.Now(), + RetryAttemptN: predecessor.RetryAttemptN + 1, + } + updated := *predecessor + updated.RetryExecutionID = &successorID + + require.NoError(t, f.store.WithTx(ctx, func(tx pgx.Tx) error { + if err := f.store.SavePurchaseExecutionTx(ctx, tx, successor); err != nil { + return err + } + return f.store.SavePurchaseExecutionTx(ctx, tx, &updated) + })) + return successor +} + +// gate runs the advance gate for the plan's next step and reports its verdict. +func (f *rampFixture) gate(ctx context.Context, t *testing.T) error { + t.Helper() + return f.store.CompletePlanStep(ctx, f.planID, f.currentStep(ctx, t)+1) +} + +// currentStep re-reads the plan's ramp position. +func (f *rampFixture) currentStep(ctx context.Context, t *testing.T) int { + t.Helper() + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + return plan.RampSchedule.CurrentStep +} + +func (f *rampFixture) acct(i int) *string { return &f.accounts[i] } + +// TestRampGate_DetachingAnAccountReleasesTheStep is the release valve issue +// #1861 requires option 2 to have, and the one the first cut of this fix +// documented but did not implement. +// +// Account C's row is `failed`. It cannot be retried when the provider refuses +// re-drive (Azure savings plans), and it cannot be canceled at all -- +// IsCancelable and CancelExecutionAtomic both admit only pending/notified/ +// scheduled. Without an exit the step is held open forever and the plan never +// buys the rest of its ramp. Detaching the account from the plan is the exit. +func TestRampGate_DetachingAnAccountReleasesTheStep(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 3, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + f.row(ctx, t, 3, f.acct(1), StatusCompleted) + f.row(ctx, t, 3, f.acct(2), StatusFailed) + + require.ErrorIs(t, f.gate(ctx, t), ErrRampStepIncomplete, + "an attached account that has not bought must hold the step open") + require.Equal(t, 2, f.currentStep(ctx, t)) + + // The operator removes the unrecoverable account from the plan. It is no + // longer a target, so it no longer has a share of this step to buy. + require.NoError(t, f.store.SetPlanAccounts(ctx, f.planID, f.accounts[:2])) + + require.NoError(t, f.gate(ctx, t), + "detaching the account the plan no longer targets must release the ramp") + assert.Equal(t, 3, f.currentStep(ctx, t)) +} + +// TestRampGate_DetachedAccountIsNotReportedStuck pins the report to the same +// predicate as the gate. A plan nobody can unstick must be reported stuck; a +// plan that has been released must stop being reported, or the badge keeps +// pointing an operator at an account the plan no longer buys for. +func TestRampGate_DetachedAccountIsNotReportedStuck(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 3, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + f.row(ctx, t, 3, f.acct(1), StatusCompleted) + f.row(ctx, t, 3, f.acct(2), StatusFailed) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + require.Equal(t, RampStepBlock{StepNumber: 3, StuckExecutions: 1}, stuck[f.planID]) + + require.NoError(t, f.store.SetPlanAccounts(ctx, f.planID, f.accounts[:2])) + + stuck, err = f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID) +} + +// TestRampGate_AnAccountThatBoughtStaysBought is the monotonicity guard. A +// purchase cannot be un-made, so a later failed attempt for an account that +// already bought must not re-open the step. If it did, the stuck report would +// name that account and an operator following it would retry a purchase that +// already landed, buying the same commitment twice. +func TestRampGate_AnAccountThatBoughtStaysBought(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + f.row(ctx, t, 3, f.acct(1), StatusCompleted) + + // A later, unsuperseded attempt for account A ends failed: a re-execution + // of a step that was already bought. + f.row(ctx, t, 3, f.acct(0), StatusFailed) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID, + "an account that already bought must never be reported stuck: following that report double-buys") + + require.NoError(t, f.gate(ctx, t), + "a failed re-attempt cannot un-buy a step every account already bought") + assert.Equal(t, 3, f.currentStep(ctx, t)) +} + +// TestRampGate_AllAccountsFailedRootDoesNotBlockAfterRetries pins the +// root-exclusion clause. When every account fails, executeMultiAccount returns +// errAllAccountsFailed and finalizeExecution stamps the ROOT row `failed`. That +// row is a container, not a purchase unit: once each account has been retried +// to success the step is bought, and counting the root alongside its children +// would hold it open forever with nothing left to retry. +// +// The end-to-end fixtures never produce this shape, because a partially-failed +// fan-out stamps the root `partially_completed`, which is a succeeded status. +func TestRampGate_AllAccountsFailedRootDoesNotBlockAfterRetries(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + root := f.row(ctx, t, 3, nil, StatusFailed) + failedA := f.row(ctx, t, 3, f.acct(0), StatusFailed) + failedB := f.row(ctx, t, 3, f.acct(1), StatusFailed) + + require.ErrorIs(t, f.gate(ctx, t), ErrRampStepIncomplete, + "nothing bought this step yet") + + retryA := f.row(ctx, t, 3, f.acct(0), StatusCompleted) + f.supersede(ctx, t, failedA, retryA) + retryB := f.row(ctx, t, 3, f.acct(1), StatusCompleted) + f.supersede(ctx, t, failedB, retryB) + + require.NoError(t, f.gate(ctx, t), + "every account bought; the failed root row must not hold the step open") + assert.Equal(t, 3, f.currentStep(ctx, t)) + require.NotNil(t, root) +} + +// TestRampGate_LatestAttemptDecidesAUnitThatNeverBought pins `updated_at DESC`. +// +// The key only bites on a unit that never bought (once a unit has bought, +// ever_bought answers the gate's other question regardless of which row +// represents it) and that has no supersession link to fall back on -- the shape +// a re-drive produces, since re-fanned rows supersede nothing. Account B fails, +// then a later attempt for B is canceled, which is the operator saying +// that unit will not buy this step, so the step is released. +// +// The two execution_ids are chosen so the FAILED row sorts first by +// execution_id, the ordering's last resort. If updated_at DESC is dropped, that +// dead attempt speaks for B, B reads as outstanding and the ramp freezes. The +// test asserts the updated_at precondition it depends on rather than assuming +// two consecutive writes differ. +func TestRampGate_LatestAttemptDecidesAUnitThatNeverBought(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + + const earlyID = "00000000-0000-4000-8000-000000000001" + const lateID = "ffffffff-ffff-4fff-8fff-ffffffffffff" + f.rowWithID(ctx, t, earlyID, 3, f.acct(1), StatusFailed) + f.rowWithID(ctx, t, lateID, 3, f.acct(1), StatusCanceled) + + require.True(t, f.updatedAt(ctx, t, lateID).After(f.updatedAt(ctx, t, earlyID)), + "the canceled row must be strictly newer, or this test measures nothing") + require.Less(t, earlyID, lateID, + "the dead attempt must sort first by execution_id, or the mutant survives") + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID, + "B's live attempt was canceled, not failed: the plan is released, not frozen") + + require.NoError(t, f.gate(ctx, t), + "B's latest attempt settled the unit; the older failed row must not still speak") + assert.Equal(t, 3, f.currentStep(ctx, t)) +} + +// TestRampGate_ASupersededAttemptNeverSpeaks pins the other ordering key, the +// one updated_at cannot cover. persistRetryExecution writes the successor and +// stamps the predecessor in ONE transaction, so both rows carry the same +// transaction timestamp and only the supersession link separates them. +// +// Measured on a unit that has NOT bought, because that is the only state where +// which row speaks changes an answer: account B failed and its retry is still +// pending, so B is recovering, not stuck. Drop the supersession key and the +// ordering falls through to execution_id, which is chosen here so the dead +// predecessor sorts first -- B would then be reported stuck and an operator +// would be sent to retry a row that already has a retry in flight. +func TestRampGate_ASupersededAttemptNeverSpeaks(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + + const predecessorID = "00000000-0000-4000-8000-00000000000a" + const successorID = "ffffffff-ffff-4fff-8fff-fffffffffffa" + failed := f.rowWithID(ctx, t, predecessorID, 3, f.acct(1), StatusFailed) + f.retryInOneTx(ctx, t, failed, successorID, "pending") + + require.Less(t, predecessorID, successorID, + "the superseded row must sort first by execution_id, or the mutant survives") + require.Equal(t, f.updatedAt(ctx, t, predecessorID), f.updatedAt(ctx, t, successorID), + "one transaction must give both rows the same updated_at, or recency would decide instead") + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID, + "the superseded predecessor must not speak: B has a retry in flight, it is not stuck") + + require.ErrorIs(t, f.gate(ctx, t), ErrRampStepIncomplete, + "the pending retry still holds the step open") +} + +// TestStuckRampSteps_IgnoresPlansWithoutARamp pins the report's scope. An +// "immediate" plan has total_steps 1 and its single execution is stamped step 1 +// by the schema default, so an ordinary failed purchase on it looked exactly +// like a blocked ramp step: a -25 health penalty, quoting a ramp step, on a plan +// that has no ramp, for the same row failed_executions already counts. +func TestStuckRampSteps_IgnoresPlansWithoutARamp(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 0, 1) + + f.row(ctx, t, 1, f.acct(0), StatusFailed) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID, + "a plan with a single step has no ramp to block") +} + +// TestStuckRampSteps_IgnoresFinishedRamps: a ramp that has bought every step +// has no next step, so nothing can be blocking it. +func TestStuckRampSteps_IgnoresFinishedRamps(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 4, 4) + + f.row(ctx, t, 5, f.acct(0), StatusFailed) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID) +} + +// TestStuckRampSteps_ReportsAMultiStepRamp is the positive control for the two +// scope tests above: without it, a filter that excluded everything would still +// let them pass. +func TestStuckRampSteps_ReportsAMultiStepRamp(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + f.row(ctx, t, 3, f.acct(1), StatusFailed) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.Equal(t, RampStepBlock{StepNumber: 3, StuckExecutions: 1}, stuck[f.planID]) +} + +// TestStuckRampSteps_RetryInFlightIsNotStuck separates the gate's question from +// the report's. A pending retry holds the step open, so the gate refuses, but +// the plan is recovering rather than frozen and must not be flagged. +func TestStuckRampSteps_RetryInFlightIsNotStuck(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + failed := f.row(ctx, t, 3, f.acct(1), StatusFailed) + retry := f.row(ctx, t, 3, f.acct(1), "pending") + f.supersede(ctx, t, failed, retry) + + stuck, err := f.store.GetStuckRampSteps(ctx) + require.NoError(t, err) + assert.NotContains(t, stuck, f.planID, "a retry in flight is recovery, not a frozen ramp") + + require.ErrorIs(t, f.gate(ctx, t), ErrRampStepIncomplete, + "the gate still waits for the retry to land") +} + +// TestBoughtRampStepsInRange_FindsThePartlyBoughtStep backs the create-purchases +// guard: the step an operator would re-schedule while the ramp is frozen is the +// one an account has already bought, and re-fanning it out double-buys. +func TestBoughtRampStepsInRange_FindsThePartlyBoughtStep(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + f.row(ctx, t, 3, f.acct(1), StatusFailed) + + bought, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 3, 5) + require.NoError(t, err) + assert.Equal(t, []int{3}, bought) + + // Step 4 has no rows at all, so a create that starts there is fine. + bought, err = f.store.BoughtRampStepsInRange(ctx, f.planID, 4, 5) + require.NoError(t, err) + assert.Empty(t, bought) +} + +// TestBoughtRampStepsInRange_SeparatesStepsOfTheSameAccount pins the step key +// in the unit reduction. The range query is the only caller whose scope spans +// more than one step, so without step_number in the DISTINCT ON and the +// PARTITION BY, one account collapses to a single row across the whole range: +// the step it bought and the step it merely has scheduled become one unit, and +// the answer is whichever row happened to sort first. +// +// Account A bought step 3 and has a pending row for step 4. Only step 3 is +// bought. Reporting step 4 would refuse a legitimate create; reporting step 3 +// only when the ordering happens to favor it is worse, because it is the +// double-buy guard that goes quiet. +func TestBoughtRampStepsInRange_SeparatesStepsOfTheSameAccount(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 2, 6) + + // The step-4 row is written second and sorts last by execution_id, so it + // wins every tie-break the reduction has if the step key is missing. + f.rowWithID(ctx, t, "00000000-0000-4000-8000-000000000003", 3, f.acct(0), StatusCompleted) + f.rowWithID(ctx, t, "ffffffff-ffff-4fff-8fff-fffffffffff4", 4, f.acct(0), "pending") + + bought, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 3, 5) + require.NoError(t, err) + assert.Equal(t, []int{3}, bought, + "only the step the account actually bought may be reported") +} + +// TestBoughtRampStepsInRange_RejectsAnInvertedRange: generate_series over an +// inverted range yields no rows, so a silent empty answer would read as "no +// step is bought" and wave the create through. A guard on a money path must not +// fail open on a nonsense argument. +func TestBoughtRampStepsInRange_RejectsAnInvertedRange(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 2, 6) + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + + _, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 5, 3) + require.Error(t, err, "an inverted range must error, not report nothing bought") +} + +// TestBoughtRampStepsInRange_IgnoresDetachedAccounts keeps the create guard on +// the same predicate as the gate: once the plan stops targeting an account, its +// rows stop speaking for the plan in both places. +func TestBoughtRampStepsInRange_IgnoresDetachedAccounts(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 2, 2, 4) + + f.row(ctx, t, 3, f.acct(0), StatusCompleted) + require.NoError(t, f.store.SetPlanAccounts(ctx, f.planID, f.accounts[1:])) + + bought, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 3, 5) + require.NoError(t, err) + assert.Empty(t, bought, + "the only row that bought belongs to an account the plan no longer targets") +} + +// TestRampStepStatusListsPartitionEveryWrittenStatus guards the classification +// itself. Every status the product writes to purchase_executions has to land in +// exactly one of: succeeded (bought), excused (settled without buying), stuck +// (terminal, reported), or in-flight (holds the step open, not reported). A +// status in none of them silently blocks a ramp AND is invisible to the health +// report, which is the one combination an operator cannot diagnose. +func TestRampStepStatusListsPartitionEveryWrittenStatus(t *testing.T) { + // The statuses the History view loads, which is the product's own + // enumeration of what a purchase_executions row can be. + written := []string{ + "pending", "notified", "scheduled", "approved", "running", "paused", + StatusCompleted, StatusPartiallyCompleted, StatusFailed, StatusExpired, + StatusCanceled, LegacyStatusCanceled, + } + // In-flight statuses hold a step open on purpose and are deliberately not + // reported as stuck; they are listed here so adding a status to the schema + // forces a decision rather than defaulting into invisibility. + inFlight := map[string]bool{ + "pending": true, "notified": true, "scheduled": true, + "approved": true, "running": true, "paused": true, + } + + classify := func(status string) []string { + var in []string + if slices.Contains(RampStepSucceededStatuses, status) { + in = append(in, "succeeded") + } + if slices.Contains(RampStepSettledStatuses, status) && !slices.Contains(RampStepSucceededStatuses, status) { + in = append(in, "excused") + } + if slices.Contains(RampStepStuckStatuses, status) { + in = append(in, "stuck") + } + if inFlight[status] { + in = append(in, "in-flight") + } + return in + } + + require.NotEmpty(t, written) + for _, status := range written { + classes := classify(status) + assert.Len(t, classes, 1, + "status %q must fall in exactly one ramp-step class, got %v", status, classes) + } +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 9353602ac..0defc4e2b 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -218,6 +218,26 @@ func (m *MockConfigStore) GetStuckRampSteps(ctx context.Context) (map[string]con return v, args.Error(1) } +// BoughtRampStepsInRange mocks the partly-bought ramp-step probe. Unregistered +// by default so the many create-purchases tests that predate the probe keep +// exercising the create path: no bought steps in range is the shape of a plan +// whose next steps are still unstarted, which is what those tests set up. +func (m *MockConfigStore) BoughtRampStepsInRange(ctx context.Context, planID string, from, to int) ([]int, error) { + m.record("BoughtRampStepsInRange", ctx, planID, from, to) + if !isExpected(&m.Mock, "BoughtRampStepsInRange") { + return nil, nil + } + args := m.Called(ctx, planID, from, to) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).([]int) + if !ok { + panic(fmt.Sprintf("mock: expected []int, got %T", args.Get(0))) + } + return v, args.Error(1) +} + // UpdatePurchasePlanTx mocks the UpdatePurchasePlanTx operation. Falls // back to UpdatePurchasePlan when no expectation is registered so tests // that don't care about the Tx variant stay green -- same pattern as diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 4e599c676..021e48bb5 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -522,15 +522,15 @@ func TestExecuteAndFinalize_RefusedRampAdvanceIsRecordedOnTheRow(t *testing.T) { mockStore.AssertNotCalled(t, "CompletePlanStep", mock.Anything, mock.Anything, mock.Anything) } -// TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow is the -// counterpart guard for issue #1861: a step still waiting on its other accounts -// is the ordinary shape of a multi-account fan-out being repaired one account -// at a time, and it stops being true the moment the last account buys. Stamping -// it would leave a permanent note describing a ramp that has since advanced, -// and would flip a cleanly-completed row into History's audit-gap rendering, -// which keys on a non-empty Error. Plan health derives that state live instead -// (config.GetStuckRampSteps). -func TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow(t *testing.T) { +// runRampAdvanceOutcome drives executeAndFinalize for a plan-attributed +// execution whose ramp advance the store declines with declineErr, and returns +// the last execution row that reached the store. +// +// Shared by the cases below because what separates them is only which sentinel +// CompletePlanStep returns; duplicating the whole fan-out fixture per case would +// bury that one line. +func runRampAdvanceOutcome(t *testing.T, execID string, declineErr error) config.PurchaseExecution { + t.Helper() ctx := context.Background() mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) @@ -540,7 +540,7 @@ func TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow(t *testing.T) t.Cleanup(func() { mockStore.AssertExpectations(t) }) exec := &config.PurchaseExecution{ - ExecutionID: "exec-incomplete-step", + ExecutionID: execID, PlanID: "plan-multi", Status: "running", StepNumber: 3, @@ -560,8 +560,7 @@ func TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow(t *testing.T) mockStore.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { return nil, nil } - mockStore.On("CompletePlanStep", mock.Anything, "plan-multi", 3). - Return(fmt.Errorf("%w: 1 account(s) of plan plan-multi ramp step 3 have not bought", config.ErrRampStepIncomplete)) + mockStore.On("CompletePlanStep", mock.Anything, "plan-multi", 3).Return(declineErr) mockStore.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Maybe() mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Maybe() @@ -578,14 +577,58 @@ func TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow(t *testing.T) } require.NoError(t, manager.executeAndFinalize(ctx, exec), - "a step waiting on a sibling account must not fail the purchase that just completed") - + "a declined ramp advance must never fail the purchase that just completed") require.NotEmpty(t, saves, "the terminal save must reach the store") - final := saves[len(saves)-1] + mockStore.AssertCalled(t, "CompletePlanStep", mock.Anything, "plan-multi", 3) + return saves[len(saves)-1] +} + +// TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow is the +// counterpart guard for issue #1861: a step still waiting on its other accounts +// is the ordinary shape of a multi-account fan-out being repaired one account +// at a time, and it stops being true the moment the last account buys. Stamping +// it would leave a permanent note describing a ramp that has since advanced, +// and would flip a cleanly-completed row into History's audit-gap rendering, +// which keys on a non-empty Error. Plan health derives that state live instead +// (config.GetStuckRampSteps). +func TestExecuteAndFinalize_IncompleteRampStepIsNotStampedOnTheRow(t *testing.T) { + final := runRampAdvanceOutcome(t, "exec-incomplete-step", + fmt.Errorf("%w: 1 account(s) of plan plan-multi ramp step 3 have not bought", config.ErrRampStepIncomplete)) + assert.Equal(t, "completed", final.Status) assert.NotContains(t, final.Error, "ramp not advanced", "a transient wait must leave no note that will outlive it") - mockStore.AssertCalled(t, "CompletePlanStep", mock.Anything, "plan-multi", 3) +} + +// TestExecuteAndFinalize_SiblingCountedStepIsNotStampedOnTheRow closes the +// second half of the same hazard. When the last two accounts of a step finish +// together, one advances the ramp and the other is told the step is already +// counted -- routine, and the step WAS counted, so nothing is wrong with that +// purchase. Stamping it would put "ramp not advanced" on a clean row and make +// History announce that the purchase's history record could not be saved. +func TestExecuteAndFinalize_SiblingCountedStepIsNotStampedOnTheRow(t *testing.T) { + final := runRampAdvanceOutcome(t, "exec-sibling-counted", + fmt.Errorf("%w: plan plan-multi is already on ramp step 3", config.ErrRampStepCountedBySibling)) + + assert.Equal(t, "completed", final.Status) + assert.NotContains(t, final.Error, "ramp not advanced", + "losing a race to a sibling of the same step is not an anomaly worth recording") +} + +// TestExecuteAndFinalize_PassedStepIsStampedOnTheRow is the positive control for +// the two above: the discrimination has to keep recording the case issue #1861 +// actually asks for. A row completing a step the ramp moved PAST bought +// commitment the plan counted earlier and will never count again, and that is +// invisible everywhere else once the Lambda log ages out. +func TestExecuteAndFinalize_PassedStepIsStampedOnTheRow(t *testing.T) { + final := runRampAdvanceOutcome(t, "exec-passed-step", + fmt.Errorf("%w: plan plan-multi is on ramp step 5, past the completing step 3", config.ErrRampStepAlreadyCounted)) + + assert.Equal(t, "completed", final.Status, + "the purchase completed; only the plan's progress accounting did not") + assert.Contains(t, final.Error, "ramp not advanced", + "a step the ramp has passed must survive on the row, not only in the log") + assert.Contains(t, final.Error, "past the completing step 3") } func TestManager_GetAWSAccountID_Success(t *testing.T) { diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index ac12a63b1..0ab6aa5f9 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -263,19 +263,25 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE // reads as daysUntil < 0 and stops notifying that plan. Such a stall is not // self-correcting, and a logging.Errorf was its only trace. // -// A step still waiting on its other accounts (issue #1861) is the exception and -// is deliberately NOT stamped. It is the ordinary shape of a partially-failed -// multi-account step being repaired one account at a time, and it stops being -// true the moment the last account buys -- a note written here would outlive the -// fact and describe a ramp that has since advanced. That state is derived live -// instead, by config.GetStuckRampSteps for the plan-health ramp_blocked factor. +// Two outcomes are deliberately NOT stamped, because both are ordinary and +// neither stays true (issue #1861). A step still waiting on its other accounts +// is the shape of a partially-failed multi-account step being repaired one +// account at a time, and it stops being true the moment the last account buys. +// A step a sibling execution counted moments earlier is the shape of that +// step's last two accounts finishing together, and it means the step WAS +// counted. A note for either would outlive the fact and, worse, would flip a +// cleanly-completed row into History's audit-gap rendering, which keys on a +// non-empty Error and would then tell the operator the purchase's history +// record could not be saved. The waiting state is derived live instead, by +// config.GetStuckRampSteps for the plan-health ramp_blocked factor. // // The status stays "completed": the purchase did complete, and only the plan's // progress accounting did not. Recovery is deliberately not scheduled here; this // records the fact so History shows it and an operator can act on it. func (m *Manager) recordRampAdvanceRefusal(ctx context.Context, exec *config.PurchaseExecution, cause error) { - if errors.Is(cause, config.ErrRampStepIncomplete) { - logging.Warnf("Plan %s ramp step %d not advanced yet: %v", exec.PlanID, exec.StepNumber, cause) + if errors.Is(cause, config.ErrRampStepIncomplete) || errors.Is(cause, config.ErrRampStepCountedBySibling) { + logging.Warnf("Plan %s ramp step %d not advanced by execution %s: %v", + exec.PlanID, exec.StepNumber, exec.ExecutionID, cause) return } logging.Errorf("Failed to update plan progress: %v", cause) diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index b4299d16a..9c9ba7d0b 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -318,9 +318,11 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { // write and both leave CurrentStep at 3. Assert on the return value // directly against real DB state rather than against pgxmock. err := f.store.CompletePlanStep(ctx, f.plan.ID, 3) - require.ErrorIs(t, err, config.ErrRampStepAlreadyCounted, - "a counted step must report already-counted, not a skipped predecessor") + require.ErrorIs(t, err, config.ErrRampStepCountedBySibling, + "the plan is sitting on step 3, which a sibling of this step counted") require.NotErrorIs(t, err, config.ErrRampStepIncomplete) + require.NotErrorIs(t, err, config.ErrRampStepAlreadyCounted, + "the ramp has not moved PAST step 3, so this is not the anomalous case") // The ramp is not complete, so the plan still points at a next execution. plan, err := f.store.GetPurchasePlan(ctx, f.plan.ID) @@ -467,8 +469,8 @@ func TestCompletePlanStepIsIdempotentUnderConcurrency(t *testing.T) { advanced++ continue } - require.ErrorIs(t, err, config.ErrRampStepAlreadyCounted, - "a concurrent completion of step 3 either advances or reports already-counted") + require.ErrorIs(t, err, config.ErrRampStepCountedBySibling, + "a concurrent completion of step 3 either advances or loses to a sibling") } require.Equal(t, racers, seen, "all racers must have reported") require.Equal(t, 1, advanced, "exactly one racer may advance the ramp") @@ -570,8 +572,8 @@ func TestCompletePlanStepBlocksOnTheRowLockAndSeesTheWinner(t *testing.T) { select { case loserErr := <-done: - require.ErrorIs(t, loserErr, config.ErrRampStepAlreadyCounted, - "the loser of the race must report already-counted, not re-advance") + require.ErrorIs(t, loserErr, config.ErrRampStepCountedBySibling, + "the loser of the race must report the sibling win, not re-advance") case <-time.After(15 * time.Second): t.Fatal("CompletePlanStep never returned after the row lock was released") } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index c43865579..768ec1c69 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -57,6 +57,10 @@ func (m *mockConfigStoreForHealth) GetStuckRampSteps(_ context.Context) (map[str return nil, nil } +func (m *mockConfigStoreForHealth) BoughtRampStepsInRange(_ context.Context, _ string, _, _ int) ([]int, error) { + return nil, nil +} + func (m *mockConfigStoreForHealth) UpdatePurchasePlanTx(_ context.Context, _ pgx.Tx, _ *config.PurchasePlan) error { return nil } From 67de14e940bec9d21db145a2b768dce6cab8513d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 20 Aug 2026 21:01:34 +0200 Subject: [PATCH 3/3] fix(plans): decide the create-side ramp guard under the plan lock (#1861) The bought-step check ran before WithTx opened, so it was advisory rather than a guard. Two creates could both read the step as free and each mint a root row for it, and a pending account execution could succeed between the check and the insert. Either way the row that gets approved re-fans-out across accounts that already bought, under a fresh idempotency lineage whose derived tokens miss the provider dedupe: the duplicate commitment the check exists to prevent. The completion path already decided under SELECT FOR UPDATE on the plan row, so the two halves of this change disagreed about their own concurrency model. The whole decision now happens inside the transaction, opened by taking that same per-plan lock. The plan is re-read under it, so the step range derives from a CurrentStep no concurrent completion can move underneath, and the probe and the inserts are separated by nothing. The lock is now defined once, in lockPurchasePlanTx, and both paths call it rather than each spelling out its own locking read. Chose the lock over a unique index because the invariant is per-plan and spans two tables, the completion path already establishes it, and a partial unique index would have to be reconciled against plans that already carry duplicate rows. Serializing alone would have changed nothing observable, because a root row another create just inserted is pending, not bought, and the bought test cannot see it. The predicate is now "bought or still working", which is the same tally the advance gate already computes: a step is free to schedule only when nothing bought it and nothing is in flight for it. A step whose rows were all canceled stays schedulable, so cancelling does not become a dead end. The unlocked read that remains answers the 404 before a transaction is opened; its plan is discarded rather than used, so it cannot be mistaken for the authoritative one. Two smaller review points: the health-score schema said null meant only that execution counts were unavailable, but the stuck-ramp report is now an equally required input and its failure withholds the score the same way; and the CompletePlanStep doc named ErrRampStepAlreadyCounted for the #1669 example it cites, which lands on the equal-step case and so reports ErrRampStepCountedBySibling. Both sentinels are now named with the condition that selects them, since callers branch on the difference. Verified against 733859ce1 itself, with the concurrency tests present and the rest of the tree stashed: six concurrent creates all succeeded and left six execution rows on ramp step 3, and the lock test never saw a waiter. With the fix, exactly one create wins and step 3 ends with one row. A held-lock test additionally forces the interleaving rather than hoping for it: an outside transaction holds the plan row and inserts the step-3 row uncommitted, the create is confirmed blocked on that lock before the holder commits, and it then observes the committed row and refuses. The seven existing SQL mutants were re-run and all still die. Full unit suite, integration suites for config/api/purchase against a real migrated Postgres, go vet with and without the integration tag, golangci-lint at the CI-pinned v2.10.1 and gocyclo -over 10 all clean. --- internal/analytics/collector_test.go | 6 +- internal/api/handler_plans.go | 78 ++++-- ...dler_plans_concurrency_integration_test.go | 256 ++++++++++++++++++ internal/api/handler_plans_test.go | 26 +- internal/api/openapi.yaml | 10 +- internal/config/interfaces.go | 38 ++- internal/config/store_postgres.go | 33 ++- internal/config/store_postgres_ramp_step.go | 75 +++-- ...ore_postgres_ramp_step_integration_test.go | 112 +++++--- internal/mocks/stores.go | 38 ++- internal/server/test_helpers_test.go | 6 +- 11 files changed, 550 insertions(+), 128 deletions(-) create mode 100644 internal/api/handler_plans_concurrency_integration_test.go diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index f784943c7..e5b3a0750 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -168,7 +168,11 @@ func (m *mockConfigStore) GetStuckRampSteps(_ context.Context) (map[string]confi return nil, nil } -func (m *mockConfigStore) BoughtRampStepsInRange(_ context.Context, _ string, _, _ int) ([]int, error) { +func (m *mockConfigStore) LockPurchasePlanTx(_ context.Context, _ pgx.Tx, _ string) (*config.PurchasePlan, error) { + return nil, nil +} + +func (m *mockConfigStore) OccupiedRampStepsInRangeTx(_ context.Context, _ pgx.Tx, _ string, _, _ int) ([]int, error) { return nil, nil } diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 0f4f510cb..f1a855040 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -349,12 +349,12 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La return nil, err } - plan, err := h.getPlanForPurchaseCreation(ctx, planID) - if err != nil { - return nil, err - } - - if err := h.refusePartlyBoughtRampSteps(ctx, plan, planID, req.Count); err != nil { + // Validation only: this answers the 404 (and maps storage errors to a clean + // message) before a transaction is opened. The plan it returns is + // deliberately discarded -- it is an unlocked snapshot whose ramp position + // can be stale by the time the inserts run, and the authoritative read + // happens under the ramp lock inside the transaction below (issue #1861). + if _, err := h.getPlanForPurchaseCreation(ctx, planID); err != nil { return nil, err } @@ -380,15 +380,9 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La creator := resolveCreatorUserID(session) created := 0 if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { - n, txErr := h.createPurchaseExecutionsTx(ctx, tx, plan, planID, req.Count, startDate, creator) - if txErr != nil { - return txErr - } - if planErr := h.updatePlanNextExecutionDateTx(ctx, tx, plan, startDate); planErr != nil { - return planErr - } + n, txErr := h.createPlannedPurchasesTx(ctx, tx, planID, req.Count, startDate, creator) created = n - return nil + return txErr }); err != nil { return nil, err } @@ -396,8 +390,44 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La return &CreatePlannedPurchasesResponse{Created: created}, nil } -// refusePartlyBoughtRampSteps blocks a create whose steps overlap one an -// account has already bought (issue #1861). +// createPlannedPurchasesTx is the transactional body of createPlannedPurchases: +// take the per-plan ramp lock, decide against the state that lock protects, and +// write, all before anyone else can read it. +// +// The lock is the point. The step range derives from the plan's CurrentStep and +// the "is this step already covered" test reads the executions of those steps, +// so both inputs are exactly the state a concurrent completion or a concurrent +// create mutates. Read either outside the lock and the check becomes advisory: +// two creates both see the step free and each mints a root row for it, and +// approving both re-fans-out over accounts that already bought. CompletePlanStep +// takes this same lock, so a completion cannot interleave either. +func (h *Handler) createPlannedPurchasesTx(ctx context.Context, tx pgx.Tx, planID string, count int, startDate time.Time, creator *string) (int, error) { + plan, err := h.config.LockPurchasePlanTx(ctx, tx, planID) + if err != nil { + logging.Errorf("createPlannedPurchases: LockPurchasePlanTx failed (plan=%s): %v", planID, err) + return 0, NewClientError(503, "could not lock the plan for scheduling; try again") + } + if plan == nil { + return 0, NewClientError(404, "plan not found") + } + + if refuseErr := h.refuseOccupiedRampSteps(ctx, tx, plan, planID, count); refuseErr != nil { + return 0, refuseErr + } + + created, createErr := h.createPurchaseExecutionsTx(ctx, tx, plan, planID, count, startDate, creator) + if createErr != nil { + return 0, createErr + } + if planErr := h.updatePlanNextExecutionDateTx(ctx, tx, plan, startDate); planErr != nil { + return 0, planErr + } + return created, nil +} + +// refuseOccupiedRampSteps blocks a create whose steps overlap one that is +// already covered (issue #1861). Must be called with the plan's ramp lock held +// for the same transaction, which createPlannedPurchasesTx does. // // createPurchaseExecutionsTx stamps CurrentStep+1 .. CurrentStep+count. The // completeness gate holds CurrentStep still while any account of a step is @@ -411,20 +441,20 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La // // Fails closed: an unreadable check refuses the create rather than minting rows // that might double-buy. -func (h *Handler) refusePartlyBoughtRampSteps(ctx context.Context, plan *config.PurchasePlan, planID string, count int) error { +func (h *Handler) refuseOccupiedRampSteps(ctx context.Context, tx pgx.Tx, plan *config.PurchasePlan, planID string, count int) error { from := plan.RampSchedule.CurrentStep + 1 - bought, err := h.config.BoughtRampStepsInRange(ctx, planID, from, from+count-1) + occupied, err := h.config.OccupiedRampStepsInRangeTx(ctx, tx, planID, from, from+count-1) if err != nil { - logging.Errorf("createPlannedPurchases: BoughtRampStepsInRange failed (plan=%s): %v", planID, err) - return NewClientError(503, "could not verify which ramp steps are already bought; try again") + logging.Errorf("createPlannedPurchases: OccupiedRampStepsInRangeTx failed (plan=%s): %v", planID, err) + return NewClientError(503, "could not verify which ramp steps are already covered; try again") } - if len(bought) == 0 { + if len(occupied) == 0 { return nil } return NewClientError(409, fmt.Sprintf( - "ramp step(s) %s of this plan have already been bought by at least one cloud account; "+ - "retry the outstanding per-account purchases to finish the step instead of scheduling it again", - formatRampSteps(bought))) + "ramp step(s) %s of this plan already have purchase executions that bought or are still in flight; "+ + "finish or cancel those instead of scheduling the same step again", + formatRampSteps(occupied))) } // formatRampSteps renders step numbers for an operator-facing message. diff --git a/internal/api/handler_plans_concurrency_integration_test.go b/internal/api/handler_plans_concurrency_integration_test.go new file mode 100644 index 000000000..7d278e3a0 --- /dev/null +++ b/internal/api/handler_plans_concurrency_integration_test.go @@ -0,0 +1,256 @@ +//go:build integration +// +build integration + +package api + +// Real-Postgres concurrency tests for the create-planned-purchases path (issue +// #1861). The guard that keeps a create off a ramp step some account already +// bought is only a guard if it is decided and acted on under the same lock the +// completion path takes. Read outside that lock it is advisory: two creates both +// see the step free, each mints a root row for it, and approving both +// re-fans-out over the accounts that already bought under a fresh idempotency +// lineage, which is the duplicate commitment the whole guard exists to prevent. +// +// A mock cannot hold a row lock, so this measures against a real database. + +import ( + "context" + "fmt" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/aws/aws-lambda-go/events" + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// createConcurrencyFixture is a plan sitting on ramp step 2 of 4, wired to a +// handler backed by a real store. +type createConcurrencyFixture struct { + store *config.PostgresStore + pool *pgxpool.Pool + handler *Handler + planID string +} + +func newCreateConcurrencyFixture(ctx context.Context, t *testing.T) *createConcurrencyFixture { + t.Helper() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { _ = container.Cleanup(context.Background()) }) + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + + store := config.NewPostgresStore(container.DB) + + plan := &config.PurchasePlan{ + Name: "Concurrent Create Plan", + Services: map[string]config.ServiceConfig{}, + RampSchedule: config.RampSchedule{ + Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, + CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -14), + }, + } + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + + acct := &config.CloudAccount{ + Name: "acct-a", Provider: "aws", ExternalID: "333333333333", Enabled: true, + } + require.NoError(t, store.CreateCloudAccount(ctx, acct)) + require.NoError(t, store.SetPlanAccounts(ctx, plan.ID, []string{acct.ID})) + + // A real users row: createPurchaseExecutionsTx stamps the session user onto + // created_by_user_id, which is a foreign key, so a synthetic UUID would make + // every insert fail for a reason unrelated to what these tests measure. + userID := uuid.New().String() + _, err = container.DB.Pool().Exec(ctx, ` + INSERT INTO users (id, email, password_hash, salt, active, group_ids, created_at, updated_at) + SELECT $1, 'creator@example.com', 'x', 'y', true, ARRAY[g.id], now(), now() + FROM groups g ORDER BY g.name LIMIT 1`, userID) + require.NoError(t, err) + + mockAuth := new(MockAuthService) + session := &Session{UserID: userID, Email: "creator@example.com"} + mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(session, nil).Maybe() + mockAuth.grantAdmin() + + return &createConcurrencyFixture{ + store: store, + pool: container.DB.Pool(), + handler: &Handler{config: store, auth: mockAuth}, + planID: plan.ID, + } +} + +// create issues one create-planned-purchases request for `count` steps. +func (f *createConcurrencyFixture) create(ctx context.Context, count int) (*CreatePlannedPurchasesResponse, error) { + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: fmt.Sprintf(`{"count":%d,"start_date":"2026-09-01"}`, count), + } + return f.handler.createPlannedPurchases(ctx, req, f.planID) +} + +// executionsForStep counts the plan's execution rows stamped with step. +func (f *createConcurrencyFixture) executionsForStep(ctx context.Context, t *testing.T, step int) int { + t.Helper() + var n int + require.NoError(t, f.pool.QueryRow(ctx, + `SELECT count(*) FROM purchase_executions WHERE plan_id = $1 AND step_number = $2`, + f.planID, step).Scan(&n)) + return n +} + +// TestCreatePlannedPurchases_ConcurrentCreatesMintOneStepEach is the issue +// #1861 / F-A regression guard. +// +// Two creates enter with the same CurrentStep and race. Exactly one may win: +// the loser must see the winner's row and be refused, not mint a second root +// row for the same step. Serialized by hand this proves nothing, because the +// loser would read the winner's committed row anyway; the goroutines are +// released together from a shared gate so they contend for real. +func TestCreatePlannedPurchases_ConcurrentCreatesMintOneStepEach(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + + const racers = 6 + start := make(chan struct{}) + errs := make(chan error, racers) + var wg sync.WaitGroup + for i := 0; i < racers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + _, err := f.create(ctx, 1) + errs <- err + }() + } + close(start) + wg.Wait() + close(errs) + + won, refused, seen := 0, 0, 0 + for err := range errs { + seen++ + if err == nil { + won++ + continue + } + ce, ok := IsClientError(err) + require.True(t, ok, "a losing create must be refused cleanly, got %v", err) + require.Equal(t, 409, ce.code, "the loser must be refused as a conflict, got %d: %v", ce.code, err) + refused++ + } + require.Equal(t, racers, seen, "every racer must report") + assert.Equal(t, 1, won, "exactly one create may mint ramp step 3") + assert.Equal(t, racers-1, refused) + + assert.Equal(t, 1, f.executionsForStep(ctx, t, 3), + "ramp step 3 must end with exactly one execution row, not one per racer") +} + +// TestCreatePlannedPurchases_BlocksOnTheRampLockAndSeesTheWinner forces the +// interleaving the race test can only hope for, so the guard's dependence on the +// plan row lock is measured rather than argued. +// +// A transaction outside the handler takes the plan row's FOR UPDATE lock and +// inserts a pending root row for step 3, exactly as a winning create would. +// While that transaction is open, a concurrent create must BLOCK rather than +// read the pre-insert state: if it could proceed, it would find step 3 free and +// mint a second root row for it. Once the holder commits, the blocked create +// must observe the committed row and refuse. +func TestCreatePlannedPurchases_BlocksOnTheRampLockAndSeesTheWinner(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + + holder, err := f.pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = holder.Rollback(ctx) }() + + var lockedID string + require.NoError(t, + holder.QueryRow(ctx, `SELECT id FROM purchase_plans WHERE id = $1 FOR UPDATE`, f.planID).Scan(&lockedID)) + require.Equal(t, f.planID, lockedID) + + // The winning create's insert, still uncommitted. + _, err = holder.Exec(ctx, ` + INSERT INTO purchase_executions (plan_id, execution_id, status, step_number, scheduled_date) + VALUES ($1, $2, 'pending', 3, now())`, f.planID, uuid.New().String()) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { + _, createErr := f.create(ctx, 1) + done <- createErr + }() + + // Wait until Postgres reports the create's own locking read blocked. Match + // the query text: if the handler stopped taking the row lock on the READ, + // the blocked statement would be something else and this poll would never + // match, failing the test rather than silently passing it. + require.Eventually(t, func() bool { + var blocked int + if qErr := f.pool.QueryRow(ctx, ` + SELECT count(*) FROM pg_stat_activity + WHERE wait_event_type = 'Lock' + AND state = 'active' + AND pid <> pg_backend_pid() + AND query ILIKE '%purchase_plans%FOR UPDATE%'`, + ).Scan(&blocked); qErr != nil { + return false + } + return blocked > 0 + }, 15*time.Second, 25*time.Millisecond, + "the concurrent create's locking read must wait on the plan row lock") + + select { + case earlyErr := <-done: + t.Fatalf("createPlannedPurchases returned (%v) while another transaction held the plan row lock: it did not serialize", earlyErr) + default: + } + + require.NoError(t, holder.Commit(ctx)) + + select { + case loserErr := <-done: + require.Error(t, loserErr, "the blocked create must see the committed row and refuse") + ce, ok := IsClientError(loserErr) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + case <-time.After(15 * time.Second): + t.Fatal("createPlannedPurchases never returned after the plan row lock was released") + } + + assert.Equal(t, 1, f.executionsForStep(ctx, t, 3), + "the blocked create must not have added a second row for step 3") +} + +// TestCreatePlannedPurchases_CanceledStepIsReschedulable is the positive +// control for the widened predicate. A step whose executions were all canceled +// bought nothing and has nothing in flight, so the operator must be able to +// schedule it again; a guard that refused it would turn a cancel into a +// dead end. +func TestCreatePlannedPurchases_CanceledStepIsReschedulable(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + + require.NoError(t, f.store.SavePurchaseExecution(ctx, &config.PurchaseExecution{ + ExecutionID: uuid.New().String(), + PlanID: f.planID, + Status: config.StatusCanceled, + StepNumber: 3, + ScheduledDate: time.Now(), + })) + + resp, err := f.create(ctx, 1) + require.NoError(t, err, "a step whose only rows were canceled must be reschedulable") + assert.Equal(t, 1, resp.Created) +} diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 90dae2741..08eb84dd3 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -1697,7 +1697,9 @@ func TestHandler_createPlannedPurchases_RefusesAPartlyBoughtStep(t *testing.T) { }, }, nil) // Step 3 is the frozen step: one account bought it, another has not. - mockStore.On("BoughtRampStepsInRange", ctx, planID, 3, 4).Return([]int{3}, nil) + mockStore.On("OccupiedRampStepsInRangeTx", ctx, mock.Anything, planID, 3, 4).Return([]int{3}, nil) + mockStore.On("SavePurchaseExecutionTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + mockStore.On("UpdatePurchasePlanTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ @@ -1711,10 +1713,15 @@ func TestHandler_createPlannedPurchases_RefusesAPartlyBoughtStep(t *testing.T) { require.True(t, ok, "a partly-bought step must map to an HTTP status, not a 500") assert.Equal(t, 409, ce.code) assert.Contains(t, err.Error(), "3") - assert.Contains(t, err.Error(), "already been bought") - - // Nothing may be minted, and the plan pointer must not move. - mockStore.AssertNotCalled(t, "WithTx", mock.Anything, mock.Anything) + assert.Contains(t, err.Error(), "already have purchase executions") + + // The transaction now wraps the check, so WithTx running is expected; what + // must not happen is a row being minted or the plan pointer moving. Both + // are registered with .Maybe() so the mock would ALLOW the call: without + // that, AssertNotCalled would be asserting against a call that could only + // have failed the test anyway, and would pass for the wrong reason. + mockStore.AssertNotCalled(t, "SavePurchaseExecutionTx", mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "UpdatePurchasePlanTx", mock.Anything, mock.Anything, mock.Anything) } // TestHandler_createPlannedPurchases_AllowsAnUnstartedStep is the positive @@ -1740,7 +1747,7 @@ func TestHandler_createPlannedPurchases_AllowsAnUnstartedStep(t *testing.T) { }, } mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) - mockStore.On("BoughtRampStepsInRange", ctx, planID, 3, 4).Return([]int{}, nil) + mockStore.On("OccupiedRampStepsInRangeTx", ctx, mock.Anything, planID, 3, 4).Return([]int{}, nil) mockStore.On("SavePurchaseExecutionTx", ctx, mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("UpdatePurchasePlanTx", ctx, mock.Anything, mock.AnythingOfType("*config.PurchasePlan")).Return(nil).Maybe() @@ -1776,7 +1783,9 @@ func TestHandler_createPlannedPurchases_FailsClosedWhenTheProbeFails(t *testing. CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -21), }, }, nil) - mockStore.On("BoughtRampStepsInRange", ctx, planID, 3, 3).Return(nil, errors.New("connection reset")) + mockStore.On("OccupiedRampStepsInRangeTx", ctx, mock.Anything, planID, 3, 3).Return(nil, errors.New("connection reset")) + mockStore.On("SavePurchaseExecutionTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + mockStore.On("UpdatePurchasePlanTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ @@ -1789,5 +1798,6 @@ func TestHandler_createPlannedPurchases_FailsClosedWhenTheProbeFails(t *testing. ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 503, ce.code) - mockStore.AssertNotCalled(t, "WithTx", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "SavePurchaseExecutionTx", mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "UpdatePurchasePlanTx", mock.Anything, mock.Anything, mock.Anything) } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 8159f08a5..e3529bd54 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2761,9 +2761,13 @@ components: maximum: 100 description: > 0-100 plan health score. Green >= 80, amber 50-79, red < 50. - Null when the score could not be computed (the plan's - execution counts were unavailable); clients must render that - as "unknown" rather than substituting a default. + Null when the score could not be computed because one of its + required inputs was unavailable: the plan's execution counts, + or the stuck-ramp report that ramp_blocked is derived from. + Every factor only ever subtracts, so a score computed without + one of them would read healthier than the plan is; the whole + score is withheld instead. Clients must render null as + "unknown" rather than substituting a default. health_factors: type: array items: diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 0c8b185f7..b6749aa26 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -35,10 +35,19 @@ type StoreInterface interface { // prevents the concurrent-write lost-update race of issue #1071. // // It is idempotent in stepNumber: completing a step the plan has already - // counted reports ErrRampStepAlreadyCounted and writes nothing. That is - // what keeps a multi-account ramp step from advancing twice when an - // operator retries two separately-failed accounts of the same step (issue - // #1669). + // counted writes nothing. Which sentinel says so depends on where the ramp + // sits relative to the completing step, and callers branch on the + // difference (purchase.recordRampAdvanceRefusal does): + // + // - CurrentStep == stepNumber reports ErrRampStepCountedBySibling. This + // is the issue #1669 scenario: an operator retries two separately-failed + // accounts of one ramp step, and the second completion finds the ramp + // already sitting on that step because the first advanced it. The step + // WAS counted, so this is routine and is not recorded against the + // execution. + // - CurrentStep > stepNumber reports ErrRampStepAlreadyCounted. The ramp + // moved past the step earlier, so this purchase bought commitment the + // plan will never count. That is an anomaly and IS recorded. // // A step counts as complete only when EVERY cloud account the step fanned // out to has bought, not when any one of them has (issue #1861). Until @@ -53,12 +62,21 @@ type StoreInterface interface { // nothing stuck are absent from the map. Feeds the plan-health // ramp_blocked factor (issue #1861). GetStuckRampSteps(ctx context.Context) (map[string]RampStepBlock, error) - // BoughtRampStepsInRange returns the steps in [from, to] of planID that - // already have a fan-out unit which bought, ascending. Callers about to - // mint executions for a step range use it to refuse a step that is partly - // bought, which would otherwise re-fan-out across accounts that already - // committed under a fresh idempotency lineage (issue #1861). - BoughtRampStepsInRange(ctx context.Context, planID string, from, to int) ([]int, error) + // LockPurchasePlanTx reads a plan under a row lock held for the rest of tx. + // It is the per-plan ramp lock CompletePlanStep takes, exposed so a caller + // that reads a plan's ramp position and then writes against it does so + // atomically with respect to concurrent completions and creations. + // Returns (nil, nil) when the plan does not exist. + LockPurchasePlanTx(ctx context.Context, tx pgx.Tx, planID string) (*PurchasePlan, error) + // OccupiedRampStepsInRangeTx returns the steps in [from, to] of planID that + // already have a fan-out unit which bought or is still working, ascending. + // Callers about to mint executions for a step range use it to refuse a step + // that is already covered, which would otherwise re-fan-out across accounts + // that already committed under a fresh idempotency lineage (issue #1861). + // Requires the caller to hold LockPurchasePlanTx on planID in the same + // transaction; without it the answer is advisory and two concurrent callers + // both see the step free. + OccupiedRampStepsInRangeTx(ctx context.Context, tx pgx.Tx, planID string, from, to int) ([]int, error) // UpdatePurchasePlanTx is the tx-accepting variant of UpdatePurchasePlan. // Used from createPlannedPurchases' WithTx block so the per-row // SavePurchaseExecutionTx writes and the plan's next_execution_date diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 382ecbae3..17295980b 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -634,13 +634,12 @@ func (s *PostgresStore) CompletePlanStep(ctx context.Context, planID string, ste return fmt.Errorf("cannot complete ramp step %d of plan %s: step numbers are 1-based", stepNumber, planID) } return s.WithTx(ctx, func(tx pgx.Tx) error { - row := tx.QueryRow(ctx, purchasePlanSelectCols+` WHERE id = $1 FOR UPDATE`, planID) - plan, err := scanPurchasePlanRow(row) + plan, err := s.LockPurchasePlanTx(ctx, tx, planID) if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil - } - return fmt.Errorf("failed to lock purchase plan %s: %w", planID, err) + return err + } + if plan == nil { + return nil } // A finished ramp cannot be extended by a trailing execution, but it @@ -655,6 +654,28 @@ func (s *PostgresStore) CompletePlanStep(ctx context.Context, planID string, ste }) } +// LockPurchasePlanTx reads a purchase plan under a row lock held for the rest +// of tx. It is the single definition of "the per-plan ramp lock": every path +// that reads a plan's ramp position in order to write against it calls THIS +// method, so completions and creations serialize against each other rather than +// each racing on its own read. CompletePlanStep below is one caller; the +// create-planned-purchases handler is the other. +// +// Returns (nil, nil) when the plan does not exist, which callers must handle +// explicitly: CompletePlanStep treats it as a benign race it cannot control, +// the create path turns it into a 404. +func (s *PostgresStore) LockPurchasePlanTx(ctx context.Context, tx pgx.Tx, planID string) (*PurchasePlan, error) { + plan, err := scanPurchasePlanRow( + tx.QueryRow(ctx, purchasePlanSelectCols+` WHERE id = $1 FOR UPDATE`, planID)) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil + } + return nil, fmt.Errorf("failed to lock purchase plan %s: %w", planID, err) + } + return plan, nil +} + // advanceRampStep moves plan's in-memory ramp position to stepNumber, or // returns the sentinel naming why it may not. The plan row is already locked by // the caller's transaction, so the read-modify-write cannot lose an update. diff --git a/internal/config/store_postgres_ramp_step.go b/internal/config/store_postgres_ramp_step.go index 57ab5ae3c..87770a6ae 100644 --- a/internal/config/store_postgres_ramp_step.go +++ b/internal/config/store_postgres_ramp_step.go @@ -116,10 +116,10 @@ const ( // // `unit` picks the scope and the representative row, once per (step, account). // Keying on the step as well as the account is only visibly load-bearing for a -// caller whose ramp_step spans a RANGE of steps -- BoughtRampStepsInRange does -// -- but it is the correct key regardless: without it one account collapses to -// a single row across every step in scope, so a step it bought and a step it -// has merely scheduled become indistinguishable. +// caller whose ramp_step spans a RANGE of steps -- OccupiedRampStepsInRangeTx +// does -- but it is the correct key regardless: without it one account collapses +// into a single row across every step in scope, so a step it bought and a step +// it has merely scheduled become indistinguishable. // // Scope: a fanned-out step writes one row per cloud account plus a root row // carrying no account. The root row's status is an aggregate of its children @@ -192,14 +192,20 @@ const stuckRampStepQuery = `WITH` + rampStepScopeNextPerPlan + `,` + rampStepUni WHERE NOT ever_bought AND status = ANY($2) GROUP BY plan_id, step_number` -// boughtRampStepsQuery lists the steps in [$3, $4] of plan $2 that already have -// a unit which bought. Used to keep a new fan-out off a step whose commitment -// is partly made (issue #1861); see BoughtRampStepsInRange. -const boughtRampStepsQuery = `WITH +// occupiedRampStepsQuery lists the steps in [$4, $5] of plan $3 that already +// have a unit which bought or is still working. Used to keep a new fan-out off +// a step that is already covered (issue #1861); see +// OccupiedRampStepsInRangeTx. +const occupiedRampStepsQuery = `WITH ramp_step AS ( - SELECT $2::uuid AS plan_id, generate_series($3::int, $4::int) AS step_number),` + + SELECT $3::uuid AS plan_id, generate_series($4::int, $5::int) AS step_number),` + rampStepUnitCTE + ` - SELECT DISTINCT step_number FROM unit WHERE ever_bought ORDER BY step_number` + SELECT step_number + FROM unit + GROUP BY step_number + HAVING count(*) FILTER (WHERE ever_bought) > 0 + OR count(*) FILTER (WHERE NOT ever_bought AND status <> ALL($2)) > 0 + ORDER BY step_number` // requireRampStepBought reports whether ramp step stepNumber of planID may be // counted, using the transaction the caller already holds the plan row locked @@ -225,24 +231,43 @@ func requireRampStepBought(ctx context.Context, tx pgx.Tx, planID string, stepNu return nil } -// BoughtRampStepsInRange returns the steps between from and to (inclusive) of -// planID that already have a fan-out unit which bought, ascending. +// OccupiedRampStepsInRangeTx returns the steps between from and to (inclusive) +// of planID that already have a fan-out unit which bought or is still working, +// ascending. It runs in the caller's transaction so the answer can be acted on +// atomically; see the lock requirement below. // // It exists so a caller about to mint executions for a range of steps can -// refuse to target one that is partly bought. That became reachable with the -// completeness gate: a step stays uncounted while an account is outstanding, so -// CurrentStep stops moving, and the plan-scoped create endpoint keeps stamping -// CurrentStep+1 -- the same step. Approving the result re-fans-out across every -// account including the ones that already bought, under a fresh idempotency -// lineage whose derived tokens miss the provider-side dedupe entirely, so it is -// a genuine duplicate commitment rather than a no-op. -func (s *PostgresStore) BoughtRampStepsInRange(ctx context.Context, planID string, from, to int) ([]int, error) { +// refuse to target one that is already covered. Both halves of the predicate +// are load-bearing and neither subsumes the other: +// +// - A step some account BOUGHT must not get a fresh root row. The completeness +// gate holds CurrentStep still while an account is outstanding, so the +// plan-scoped create endpoint keeps stamping CurrentStep+1, the same step. +// Approving that row re-fans-out across every account including the ones +// that already bought, under a fresh idempotency lineage whose derived +// tokens miss the provider-side dedupe entirely: a genuine duplicate +// commitment, not a no-op. +// - A step that already has a LIVE unit must not get a second one either. Two +// concurrent creates both find nothing bought, and each mints its own root +// row for the same step; approving both double-buys exactly as above. The +// first create's own pending row is what the second must see, and it is not +// "bought", so the bought half alone cannot stop it. +// +// A step whose units all settled without buying (canceled) is NOT occupied: the +// operator abandoned it and rescheduling it is the intended recovery. +// +// CALLER CONTRACT: hold LockPurchasePlanTx on planID for the same transaction +// before calling this and until the resulting inserts commit. Without that lock +// the answer is advisory -- two callers read it concurrently, both see the step +// free and both insert. +func (s *PostgresStore) OccupiedRampStepsInRangeTx(ctx context.Context, tx pgx.Tx, planID string, from, to int) ([]int, error) { if from > to { - return nil, fmt.Errorf("bought ramp steps of plan %s: range %d-%d is empty", planID, from, to) + return nil, fmt.Errorf("occupied ramp steps of plan %s: range %d-%d is empty", planID, from, to) } - rows, err := s.db.Query(ctx, boughtRampStepsQuery, RampStepSucceededStatuses, planID, from, to) + rows, err := tx.Query(ctx, occupiedRampStepsQuery, + RampStepSucceededStatuses, RampStepSettledStatuses, planID, from, to) if err != nil { - return nil, fmt.Errorf("failed to query bought ramp steps of plan %s: %w", planID, err) + return nil, fmt.Errorf("failed to query occupied ramp steps of plan %s: %w", planID, err) } defer rows.Close() @@ -250,12 +275,12 @@ func (s *PostgresStore) BoughtRampStepsInRange(ctx context.Context, planID strin for rows.Next() { var step int if err := rows.Scan(&step); err != nil { - return nil, fmt.Errorf("failed to scan bought ramp step: %w", err) + return nil, fmt.Errorf("failed to scan occupied ramp step: %w", err) } steps = append(steps, step) } if err := rows.Err(); err != nil { - return nil, fmt.Errorf("failed to read bought ramp steps of plan %s: %w", planID, err) + return nil, fmt.Errorf("failed to read occupied ramp steps of plan %s: %w", planID, err) } return steps, nil } diff --git a/internal/config/store_postgres_ramp_step_integration_test.go b/internal/config/store_postgres_ramp_step_integration_test.go index d46ed2db2..2fe0de523 100644 --- a/internal/config/store_postgres_ramp_step_integration_test.go +++ b/internal/config/store_postgres_ramp_step_integration_test.go @@ -173,6 +173,24 @@ func (f *rampFixture) currentStep(ctx context.Context, t *testing.T) int { func (f *rampFixture) acct(i int) *string { return &f.accounts[i] } +// occupied runs the create-side probe the way its caller must: inside a +// transaction that already holds the plan's ramp lock. +func (f *rampFixture) occupied(ctx context.Context, t *testing.T, from, to int) ([]int, error) { + t.Helper() + var steps []int + err := f.store.WithTx(ctx, func(tx pgx.Tx) error { + locked, lockErr := f.store.LockPurchasePlanTx(ctx, tx, f.planID) + if lockErr != nil { + return lockErr + } + require.NotNil(t, locked, "the fixture plan must exist") + var qErr error + steps, qErr = f.store.OccupiedRampStepsInRangeTx(ctx, tx, f.planID, from, to) + return qErr + }) + return steps, err +} + // TestRampGate_DetachingAnAccountReleasesTheStep is the release valve issue // #1861 requires option 2 to have, and the one the first cut of this fix // documented but did not implement. @@ -424,78 +442,88 @@ func TestStuckRampSteps_RetryInFlightIsNotStuck(t *testing.T) { "the gate still waits for the retry to land") } -// TestBoughtRampStepsInRange_FindsThePartlyBoughtStep backs the create-purchases -// guard: the step an operator would re-schedule while the ramp is frozen is the -// one an account has already bought, and re-fanning it out double-buys. -func TestBoughtRampStepsInRange_FindsThePartlyBoughtStep(t *testing.T) { +// TestOccupiedRampSteps_SeparatesStepsOfTheSameAccount pins the step key in the +// unit reduction. The range query is the only caller whose scope spans more +// than one step, so without step_number in the DISTINCT ON and the PARTITION +// BY, one account collapses to a single row across the whole range and the two +// steps become one unit. +// +// Account A bought step 3; its only step-4 row was canceled, which is settled +// without buying and therefore leaves step 4 free to schedule. Only step 3 is +// occupied. Collapse the two steps into one unit and the account's ever_bought +// leaks across, reporting step 4 as occupied and refusing a legitimate create. +func TestOccupiedRampSteps_SeparatesStepsOfTheSameAccount(t *testing.T) { ctx := context.Background() - f := newRampFixture(ctx, t, 2, 2, 4) + f := newRampFixture(ctx, t, 1, 2, 6) - f.row(ctx, t, 3, f.acct(0), StatusCompleted) - f.row(ctx, t, 3, f.acct(1), StatusFailed) + // The step-4 row is written second and sorts last by execution_id, so it + // wins every tie-break the reduction has if the step key is missing. + f.rowWithID(ctx, t, "00000000-0000-4000-8000-000000000003", 3, f.acct(0), StatusCompleted) + f.rowWithID(ctx, t, "ffffffff-ffff-4fff-8fff-fffffffffff4", 4, f.acct(0), StatusCanceled) - bought, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 3, 5) + occupied, err := f.occupied(ctx, t, 3, 5) require.NoError(t, err) - assert.Equal(t, []int{3}, bought) + assert.Equal(t, []int{3}, occupied, + "step 3 bought, step 4 canceled: only step 3 is occupied") +} + +// TestOccupiedRampSteps_LiveRowOccupiesTheStep is the half of the predicate the +// concurrent-create guard depends on. A pending row bought nothing, so the +// bought test alone does not see it, yet a second create for that step mints a +// second root row and approving both double-buys. +func TestOccupiedRampSteps_LiveRowOccupiesTheStep(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 2, 6) - // Step 4 has no rows at all, so a create that starts there is fine. - bought, err = f.store.BoughtRampStepsInRange(ctx, f.planID, 4, 5) + f.row(ctx, t, 3, nil, "pending") + + occupied, err := f.occupied(ctx, t, 3, 4) require.NoError(t, err) - assert.Empty(t, bought) + assert.Equal(t, []int{3}, occupied, + "a step already scheduled must not be scheduled again") } -// TestBoughtRampStepsInRange_SeparatesStepsOfTheSameAccount pins the step key -// in the unit reduction. The range query is the only caller whose scope spans -// more than one step, so without step_number in the DISTINCT ON and the -// PARTITION BY, one account collapses to a single row across the whole range: -// the step it bought and the step it merely has scheduled become one unit, and -// the answer is whichever row happened to sort first. -// -// Account A bought step 3 and has a pending row for step 4. Only step 3 is -// bought. Reporting step 4 would refuse a legitimate create; reporting step 3 -// only when the ordering happens to favor it is worse, because it is the -// double-buy guard that goes quiet. -func TestBoughtRampStepsInRange_SeparatesStepsOfTheSameAccount(t *testing.T) { +// TestOccupiedRampSteps_CanceledStepIsFree keeps the guard from turning a +// cancel into a dead end: a step whose units all settled without buying has +// nothing bought and nothing in flight, so rescheduling it is the intended +// recovery. +func TestOccupiedRampSteps_CanceledStepIsFree(t *testing.T) { ctx := context.Background() f := newRampFixture(ctx, t, 1, 2, 6) - // The step-4 row is written second and sorts last by execution_id, so it - // wins every tie-break the reduction has if the step key is missing. - f.rowWithID(ctx, t, "00000000-0000-4000-8000-000000000003", 3, f.acct(0), StatusCompleted) - f.rowWithID(ctx, t, "ffffffff-ffff-4fff-8fff-fffffffffff4", 4, f.acct(0), "pending") + f.row(ctx, t, 3, f.acct(0), StatusCanceled) - bought, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 3, 5) + occupied, err := f.occupied(ctx, t, 3, 4) require.NoError(t, err) - assert.Equal(t, []int{3}, bought, - "only the step the account actually bought may be reported") + assert.Empty(t, occupied) } -// TestBoughtRampStepsInRange_RejectsAnInvertedRange: generate_series over an +// TestOccupiedRampSteps_RejectsAnInvertedRange: generate_series over an // inverted range yields no rows, so a silent empty answer would read as "no -// step is bought" and wave the create through. A guard on a money path must not -// fail open on a nonsense argument. -func TestBoughtRampStepsInRange_RejectsAnInvertedRange(t *testing.T) { +// step is occupied" and wave the create through. A guard on a money path must +// not fail open on a nonsense argument. +func TestOccupiedRampSteps_RejectsAnInvertedRange(t *testing.T) { ctx := context.Background() f := newRampFixture(ctx, t, 1, 2, 6) f.row(ctx, t, 3, f.acct(0), StatusCompleted) - _, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 5, 3) - require.Error(t, err, "an inverted range must error, not report nothing bought") + _, err := f.occupied(ctx, t, 5, 3) + require.Error(t, err, "an inverted range must error, not report nothing occupied") } -// TestBoughtRampStepsInRange_IgnoresDetachedAccounts keeps the create guard on -// the same predicate as the gate: once the plan stops targeting an account, its +// TestOccupiedRampSteps_IgnoresDetachedAccounts keeps the create guard on the +// same predicate as the gate: once the plan stops targeting an account, its // rows stop speaking for the plan in both places. -func TestBoughtRampStepsInRange_IgnoresDetachedAccounts(t *testing.T) { +func TestOccupiedRampSteps_IgnoresDetachedAccounts(t *testing.T) { ctx := context.Background() f := newRampFixture(ctx, t, 2, 2, 4) f.row(ctx, t, 3, f.acct(0), StatusCompleted) require.NoError(t, f.store.SetPlanAccounts(ctx, f.planID, f.accounts[1:])) - bought, err := f.store.BoughtRampStepsInRange(ctx, f.planID, 3, 5) + occupied, err := f.occupied(ctx, t, 3, 5) require.NoError(t, err) - assert.Empty(t, bought, + assert.Empty(t, occupied, "the only row that bought belongs to an account the plan no longer targets") } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 0defc4e2b..a2dacedac 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -218,16 +218,38 @@ func (m *MockConfigStore) GetStuckRampSteps(ctx context.Context) (map[string]con return v, args.Error(1) } -// BoughtRampStepsInRange mocks the partly-bought ramp-step probe. Unregistered -// by default so the many create-purchases tests that predate the probe keep -// exercising the create path: no bought steps in range is the shape of a plan -// whose next steps are still unstarted, which is what those tests set up. -func (m *MockConfigStore) BoughtRampStepsInRange(ctx context.Context, planID string, from, to int) ([]int, error) { - m.record("BoughtRampStepsInRange", ctx, planID, from, to) - if !isExpected(&m.Mock, "BoughtRampStepsInRange") { +// LockPurchasePlanTx mocks the per-plan ramp lock. Falls back to +// GetPurchasePlan when no expectation is registered, so tests that only care +// about what the transaction writes keep working: the lock is a concurrency +// property, and a mock cannot hold one anyway. The real contract is measured +// against Postgres in the api integration tests. +func (m *MockConfigStore) LockPurchasePlanTx(ctx context.Context, tx pgx.Tx, planID string) (*config.PurchasePlan, error) { + m.record("LockPurchasePlanTx", ctx, tx, planID) + if !isExpected(&m.Mock, "LockPurchasePlanTx") { + return m.GetPurchasePlan(ctx, planID) + } + args := m.Called(ctx, tx, planID) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.PurchasePlan) + if !ok { + panic(fmt.Sprintf("mock: expected *config.PurchasePlan, got %T", args.Get(0))) + } + return v, args.Error(1) +} + +// OccupiedRampStepsInRangeTx mocks the already-covered ramp-step probe. +// Unregistered by default so the many create-purchases tests that predate the +// probe keep exercising the create path: no occupied steps in range is the +// shape of a plan whose next steps are still unscheduled, which is what those +// tests set up. +func (m *MockConfigStore) OccupiedRampStepsInRangeTx(ctx context.Context, tx pgx.Tx, planID string, from, to int) ([]int, error) { + m.record("OccupiedRampStepsInRangeTx", ctx, tx, planID, from, to) + if !isExpected(&m.Mock, "OccupiedRampStepsInRangeTx") { return nil, nil } - args := m.Called(ctx, planID, from, to) + args := m.Called(ctx, tx, planID, from, to) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 768ec1c69..048979524 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -57,7 +57,11 @@ func (m *mockConfigStoreForHealth) GetStuckRampSteps(_ context.Context) (map[str return nil, nil } -func (m *mockConfigStoreForHealth) BoughtRampStepsInRange(_ context.Context, _ string, _, _ int) ([]int, error) { +func (m *mockConfigStoreForHealth) LockPurchasePlanTx(_ context.Context, _ pgx.Tx, _ string) (*config.PurchasePlan, error) { + return nil, nil +} + +func (m *mockConfigStoreForHealth) OccupiedRampStepsInRangeTx(_ context.Context, _ pgx.Tx, _ string, _, _ int) ([]int, error) { return nil, nil }