Repository navigation
fix(purchase): refuse to execute armed pre-#1668 retry successors (#1718) - #1721
Conversation
) #1668 gates the user-facing Retry at creation time, so no new Azure savings-plans successor can be built. A creation-time gate cannot reach successors a pre-#1668 retry already created. Those rows sit in pending/notified/approved/scheduled with retry_attempt_n > 0 and buy a second, non-cancelable savings plan the moment they are approved. Refuse them at the executor, on the conjunction: RetryAttemptN > 0 && RedriveRefusalReason(exec) != "" Both halves are load-bearing. RetryAttemptN > 0 restricts the refusal to retry successors, so a first savings-plan purchase still goes through; re-drive safety alone would block every legitimate first buy. RedriveRefusalReason reuses the predicate #1668 introduced rather than adding a second notion of re-drive safety. The check goes in executeAndFinalize rather than at the individual executors. That function is the single funnel every executor reaches money through, and executePurchase has exactly one production caller immediately below the check. The issue named two executors, claimAndExecute and ApproveAndExecute, but there are four: fireOneDue also reaches it, and that is the path an approved successor is most likely to take, because approveWithDelay transitions a row pending -> scheduled and returns, so the row fires later from the scheduler sweep without passing through ApproveAndExecute at all. Gating only the two named executors would have left that path open for exactly the rows this issue is about. The fourth caller, claimAndRedrive, is already gated by allRecsSafeToRedrive, which subsumes this condition, so the new check cannot fire there. Refusing with an error rather than skipping means finalizeExecution stamps the row failed with the reason and executeAndFinalize persists it, so an armed row is defused and visible in History instead of staying armed for the next sweep. Retrying it from the UI then hits the #1668 gate, which refuses, so it cannot be re-armed either. Regression coverage drives each executor entry point and asserts on the provider call count rather than on statuses: approve, SQS/cron, and the delayed-approval scheduler each buy nothing for an armed row, and the refused row lands in failed with the reason stored. Two negative controls keep the guard narrow and are as load-bearing as the refusals: a first Azure savings-plans purchase (retry_attempt_n = 0) and an AWS retry successor must both still buy exactly once. Without them a gate that blocked every savings-plan purchase would pass every refusal test. Closes #1718
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Adversarial review — independent, against the committed diff at
|
| Gate | Result |
|---|---|
go build ./... |
exit 0 |
go vet ./... |
exit 0 |
go test ./... -count=1 |
6673 passed, 42 packages |
go test ./internal/purchase -race -count=1 |
300 passed |
golangci-lint run ./... at CI-pinned v2.10.1 (not the newer local 2.11.4) |
0 issues. |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, no output |
gofmt -l internal/ |
clean |
merge against current origin/main (148c0cad0, 4 ahead) |
clean; no Go file overlap |
Claim-by-claim
1. "There are four executors, not two" — holds, and the funnel is genuinely complete.
Every production caller of executeAndFinalize: approvals.go:352 (ApproveAndExecute), manager.go:193 (claimAndExecute), scheduled_fire.go:100 (fireOneDue), manager.go:427 (claimAndRedrive). Four.
I also checked the stronger question — does anything reach a provider purchase without passing through executeAndFinalize — rather than only counting callers. PurchaseCommitment has exactly two call sites in the tree: internal/purchase/execution.go:1086, reachable only under m.executePurchase, whose sole production caller is manager.go:225 immediately below the new check; and cmd/multi_service_helpers.go:247, the standalone CLI, which never reads purchase_executions and so cannot see a retry successor. ApproveExecution (approvals.go:55) and directExecutePurchase (handler_purchases.go:2768) both delegate to ApproveAndExecute and are covered transitively. The multi-account fan-out inherits RetryAttemptN by struct copy (execution.go:~225) but is unreachable once the root is refused.
claimAndRedrive really is unreachable for this check: RecoverStrandedApprovals calls it only under allRecsSafeToRedrive(exec) (manager.go:554), which requires RedriveRefusalReason(exec) == "" — the exact negation of the second conjunct.
2. The fireOneDue argument is load-bearing and correct. approveWithDelay is invoked from both approval routes (handler_purchases.go:613 token, :713 session); it calls scheduleApprovedExecution, which CASes pending|notified -> scheduled, stamps ScheduledExecutionAt, and returns a response map — no SDK call, no ApproveAndExecute. fireOneDue later CASes scheduled -> approved and calls executeAndFinalize. With the pre-fire delay configured, gating only the two named executors would indeed have left the primary approval route open.
3. Defusal composes with #1713, which is on main. The refusal error hits finalizeExecution's default: branch (manager.go:157-159) → Status = "failed", Error = <reason>, then persisted by executeAndFinalize. Retrying that row from the UI hits checkRetryEligibilityGates (internal/api/handler_purchases.go:1757), which refuses on RedriveRefusalReason before the threshold gate and which ?force=true explicitly does not override. The row cannot be re-armed. Verified against merged main, not against the PR's description of it.
The marker itself is durable — checked because the guard is worthless if it can be silently disarmed
This is the failure mode the PR body does not claim and does not need to, but it is where a gate like this usually dies, so I verified it:
retry_attempt_nisINTEGER NOT NULL DEFAULT 0(migrations/000042, re-asserted idempotently by000044step 4).- It is deliberately INSERT-only in
SavePurchaseExecution: present in the INSERT column list but omitted fromON CONFLICT DO UPDATE SET(store_postgres.go:910-931, with the rationale comment at ~945). No upsert of a partially-populated struct can zero it. - Every projection that materialises a
PurchaseExecutionincludes it. I enumerated all 14FROM purchase_executions/RETURNINGsites; the only three without it areCountExecutionsByPlanAndStatus,CountPendingExecutionsForAccountandListPendingExecutionIDsForAccount, which return counts/IDs, never structs. The load-bearing one is the CASRETURNINGatstore_postgres.go:1003-1010— all three newly-gated executors overwrite their struct with the CAS result beforeexecuteAndFinalize, so a missing column there would have made the guard blind while every test stayed green. It is present. - No retry successor can pre-date the column: the retry feature and
000042landed together (feat(history): inline Retry button for failed purchase rows #47), and the handler always stampsRetryAttemptN = failedExec.RetryAttemptN + 1(handler_purchases.go:1902). There is no legacy class withretry_attempt_n = 0that the guard would miss.
Mutation testing — the tests fail for the right reasons
Run in a throwaway worktree at this PR's head; each mutation applied alone, tests unmodified.
| Mutation | Result |
|---|---|
M1 — remove the guard from executeAndFinalize (revert to execErr := m.executePurchase(...)) |
4 refusal tests FAIL; both negative controls PASS |
M2 — delete only the if exec.RetryAttemptN <= 0 { return nil } term |
TestFirstAzureSavingsPlanPurchaseStillExecutes FAILs alone |
M3 — delete only the RedriveRefusalReason term (refuse every retry) |
TestSafeProviderRetrySuccessorStillExecutes FAILs |
M4 — drop the hyphenated "savings-plans" spelling from recRedriveRefusalReason |
TestFireScheduledDelayedPurchasesRefusesArmedAzureSavingsPlanRetry FAILs (plus the two #1713 dispatch-parity guards) |
M2 and M3 are the ones that matter: each half of the conjunction has a test that dies when it is removed, so the "both halves are load-bearing" claim is demonstrated, not asserted. M4 confirms the hyphenated spelling in the fireOneDue test is deliberate coverage rather than decoration.
On vacuity: these are not MockConfigStore expectation assertions. The counter is incremented from a real Run callback on MockServiceClient.PurchaseCommitment, and the two negative controls reaching count == 1 on identical harness wiring prove the path actually reaches the provider. The 0 in the refusal tests is a measured zero, not an unreached one.
Findings
F1 — merge gate, not code: CodeRabbit has never reviewed this PR
The only CodeRabbit artifact is the comment at 2026-08-05T09:47:59Z, and its body is a "Review limit reached" rate-limit warning (Run ID ef653709-..., "Next review available in: 35 minutes"). reviewThreads is empty. Zero unresolved threads here means not reviewed, not clean — the exact false-clean this repo has hit before. The CI rollup showing a green CodeRabbit status does not contradict this; that status is unreliable in both directions.
Recovery must be @coderabbitai full review, not the incremental @coderabbitai review — a throttled incremental pass silently skips the in-flight commits and yields a second false clean.
F2 — minor, test strength: TestClaimAndExecuteRefusesArmedAzureSavingsPlanRetry cannot tell refusal from non-arrival
internal/purchase/armed_redrive_test.go:158-170 discards the return value (_ = mgr.ProcessMessage(...)) and asserts only rec.count() == 0, and no store.AssertExpectations(t) runs, so the .Once() in expectClaim is never enforced. If a future change made claimAndExecute bail before executeAndFinalize — a stricter checkAutoExecuteGate, or CAS-argument drift making the mock return an error — this test would still pass while proving nothing.
Its siblings do not have this gap: the approve test pins the reason (:152, assert.Contains(err, "refusing to execute retry attempt 1")) and the scheduler test pins result.Errored == 1 (:194). Suggest asserting on the returned error text, or adding store.AssertExpectations(t). It is live today (M1 kills it), so this is hardening, not a defect.
F3 — minor, ops doc: the corrected #1718 query is narrower than the guard
For #1718's stated scope the corrected query is exact — I checked the status set, both service spellings, the provider filter and the JSON keys (provider / service, config/types.go:447-448) against recRedriveRefusalReason, and they agree.
But recRedriveRefusalReason's default: branch (manager.go:388) also refuses any unrecognized provider, while the query filters r->>'provider' = 'azure'. A retry_attempt_n > 0 row carrying an unknown or typo'd provider will be refused by the executor and will not appear in the ops list. Worth one sentence in the issue saying the query enumerates the Azure-SP subset rather than the guard's full refusal set, so nobody reads "0 rows" as "the guard will never fire".
F4 — informational: two behaviours the PR body does not state
- The refusal is all-or-nothing per execution.
RedriveRefusalReasonreturns on the first unsafe rec, so a mixed retry successor (Azure SP + AWS recs) is refused entirely, including the AWS recs that would have deduped safely. That is the correct conservative call and matches fix(purchase): gate user-facing Retry on provider re-drive safety (#1668) #1713's creation-time behaviour, but it is a real semantic worth stating. - No notification is emitted when the scheduler path defuses a row.
fireOneDuelogs and countsErrored; the operator learns via History or CloudWatch. This is consistent with every otherexecuteAndFinalizefailure —SendPurchaseFailedNotificationhas no production caller at all — so it is not a regression. Flagging only because defused rows are precisely the ones an operator wants surfaced; fix(frontend/history): Retry button still offered on Azure savings-plans rows the backend now refuses cloud-commitments-platform#161 (History terminal badge) is the natural home.
Proportionality
8 lines of guard, 43 of comment, one reused predicate. No new mode, enum, flag or config knob; RedriveRefusalReason remains the single source of truth for re-drive safety, shared with #1713's creation-time gate and the reaper. The funnel placement is justified by evidence (the issue's own undercount), not by preference. Nothing here needs simplifying.
Nothing in F2-F4 blocks merge. F1 does: this needs a real CodeRabbit pass via @coderabbitai full review before it goes in.
|
I will run a full review of PR
|
|
Requesting a review. CodeRabbit has never reviewed this PR — its single comment here ( Using the full form deliberately: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and produce a second false clean on a diff it never read. Context for the review: this is a money path. It refuses execution of Azure savings-plan retry successors that a pre-#1668 retry already armed, gating at An independent adversarial review is already posted above and came back clean, including a mutation matrix where each half of the conjunction has a test that dies when that half is removed, and a check that the @coderabbitai full review |
|
|
1 participant
The gap
#1668 gates the user-facing Retry at creation time, so no new Azure savings-plans successor can be built. A creation-time gate cannot reach successors a pre-#1668 retry already created. Those rows sit in
pending/notified/approved/scheduledwithretry_attempt_n > 0and buy a second, non-cancelable savings plan the moment they are approved.The fix
Refuse at the executor, on the conjunction:
Both halves are load-bearing.
RetryAttemptN > 0restricts the refusal to retry successors, so a first savings-plan purchase still goes through; re-drive safety alone would block every legitimate first buy.RedriveRefusalReasonreuses the predicate #1668 introduced rather than adding a second notion of re-drive safety.There are four executors, not two
The issue named
claimAndExecuteandApproveAndExecute. Enumerating every caller ofexecuteAndFinalizegives four:claimAndExecute(manager.go) — SQSexecute_purchase+ cron sweepApproveAndExecute(approvals.go) — token + session approvefireOneDue(scheduled_fire.go) — delayed-approval scheduler sweepclaimAndRedrive(manager.go) — reaper re-driveallRecsSafeToRedrive, which subsumes this condition, so the new check cannot fire therefireOneDuematters becauseapproveWithDelay(internal/api/handler_purchases.go) transitions an approved rowpending→scheduledand returns; the row fires later from the scheduler sweep without passing throughApproveAndExecuteat all. Gating only the two named executors would have left that path open for exactly the rows this issue is about.So the check goes in
executeAndFinalize, the single funnel all four reach money through:executePurchasehas exactly one production caller, immediately below the check.The refused row is defused, not just skipped
Refusing with an error rather than skipping means
finalizeExecutionstamps the rowfailedwith the reason andexecuteAndFinalizepersists it, so an armed row leaves the executable set and is visible in History instead of staying armed for the next sweep. Retrying it from the UI then hits the #1668 gate, which refuses, so it cannot be re-armed either.Tests: fail before, pass after
Each test drives a real executor entry point and asserts on the provider call count, not on statuses.
Against current
main(fix reverted, tests kept)The two negative controls pass before the fix, as they must: they exist to catch a fix that is too broad.
After
The negative controls are not filler — they are half the proof
TestFirstAzureSavingsPlanPurchaseStillExecutes(retry_attempt_n = 0) andTestSafeProviderRetrySuccessorStillExecutes(an AWS retry successor) each assert the purchase still fires exactly once. They are as load-bearing as the refusals, and they are the reason theRetryAttemptN > 0half of the conjunction is testable at all.A gate that simply blocked every Azure savings-plans purchase — including every legitimate first buy — would pass all four refusal tests above. Only these two catch it. They also pass against unpatched
main, by design: their job is to fail if a future change makes the guard too broad, not to demonstrate the bug.The ops query in #1718 was wrong, and I have corrected it
Column names all check out against the schema (
execution_id,plan_id,status,retry_attempt_n,scheduled_date,recommendations JSONB). The predicates were wrong in three ways, two of them safety-critical.I did not just eyeball it: I stood up a throwaway Postgres 16, recreated the table from
migrations/000001plus the retry columns, seeded rows for each case, and ran both queries. Original:status = 'scheduled'— the delayed-approval window, i.e. precisely the rows sitting armed in thefireOneDuepathsavings-plansspelling —ILIKE '%savingsplans%'does not match the hyphenated form, which is equally unsafe and dispatches to the same Azure clientClientTokenMeasured: the original returned 5 rows, missing 2 genuinely armed ones and including 1 that is not armed. The corrected query returned all 6 armed rows and nothing else.
Posted on #1718 as a comment so the original reasoning stays intact.
Gates
go build ./.../go vet ./...go test ./... -count=1golangci-lint run ./...at CI-pinned v2.10.10 issues.gocyclo -over 10 -ignore "_test\.go"gofmt -l internal/Closes #1718