fix(purchase): gate user-facing Retry on provider re-drive safety (#1668) - #1713
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 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 (4)
📝 WalkthroughWalkthroughRetry redrives now use shared provider-safety reasons. Unsafe Azure savings-plan and unknown-provider retries return non-overridable HTTP 409 responses before successor creation. Tests cover refusal, allowed, recommendationless, mixed, and idempotent retry paths. ChangesPurchase redrive safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RetryClient
participant RetryHandler
participant PurchaseManager
participant CloudProvider
RetryClient->>RetryHandler: submit retry request
RetryHandler->>PurchaseManager: evaluate RedriveRefusalReason
PurchaseManager-->>RetryHandler: refusal reason or eligible
alt redrive unsafe
RetryHandler-->>RetryClient: HTTP 409 redrive_unsafe
else redrive eligible
RetryHandler->>PurchaseManager: create and execute successor
PurchaseManager->>CloudProvider: purchase with predecessor-derived token
CloudProvider-->>PurchaseManager: purchase result
PurchaseManager-->>RetryClient: retry result
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/purchase/manager.go (2)
293-296: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the exported function against a nil
exec.
RedriveRefusalReasonis now exported and consumed byinternal/api. Line 294 dereferencesexecdirectly, so a nil pointer panics inside the retry request path. A fail-closed guard keeps the refusal semantics consistent with the "unknown state" branch below it.🛡️ Proposed nil guard
func RedriveRefusalReason(exec *config.PurchaseExecution) string { + if exec == nil { + return "this execution could not be loaded, so what it did or did not purchase cannot be established" + } if len(exec.Recommendations) == 0 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/purchase/manager.go` around lines 293 - 296, Add a nil check at the start of the exported RedriveRefusalReason function, returning the same fail-closed refusal message used for unknown execution state before accessing exec.Recommendations. Preserve the existing recommendation handling for non-nil executions.
322-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the shared Azure savings-plans service constant.
internal/commitmentopts/probe_azure.goalready definesazureSpServicefor this service name. The literals here duplicate that value in a second package. If the canonical spelling changes, this gate silently stops matching. A shared exported constant removes the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/purchase/manager.go` around lines 322 - 328, Expose the canonical Azure savings-plans service name from internal/commitmentopts/probe_azure.go and update the provider handling in the purchase manager to use that shared constant instead of duplicating the literal. Preserve the existing GCP and unknown-provider behavior.internal/api/handler_purchases_retry_redrive_test.go (1)
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the
SavePurchaseHistoryexpectation count.
store.AssertExpectationsruns in cleanup. The twoGetExecutionByIDandTransitionExecutionStatusexpectations use.Once(), so a changed call count fails the test.SavePurchaseHistoryhas no count, so it passes with one call or with many. Add.Once()so a duplicate history write for a single-rec execution fails here too.♻️ Proposed change
- store.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) + store.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Once()Note that only the safe Azure reservation test reaches this helper, and it purchases one recommendation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_purchases_retry_redrive_test.go` around lines 128 - 132, Add .Once() to the SavePurchaseHistory mock expectation in the test setup, keeping the existing expectation arguments and return value unchanged so duplicate history writes fail assertion cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 1744-1751: Update the retry-error handling to inspect the
`redrive_unsafe` field separately from `ops_hint`, ensuring permanent redrive
refusals are handled distinctly rather than treated as operator-fixable hints.
Preserve the existing `ops_hint` behavior for retry errors without
`redrive_unsafe`.
- Around line 1727-1744: The retry gate in checkRetryEligibilityGates must not
permanently reject failed executions whose Recommendations are empty without a
recovery path. Update the execution creation flows in handler_plans.go and
notifications.go to reject or remediate empty recommendations before creating
failed executions, preserving normal retry behavior for valid executions.
---
Nitpick comments:
In `@internal/api/handler_purchases_retry_redrive_test.go`:
- Around line 128-132: Add .Once() to the SavePurchaseHistory mock expectation
in the test setup, keeping the existing expectation arguments and return value
unchanged so duplicate history writes fail assertion cleanup.
In `@internal/purchase/manager.go`:
- Around line 293-296: Add a nil check at the start of the exported
RedriveRefusalReason function, returning the same fail-closed refusal message
used for unknown execution state before accessing exec.Recommendations. Preserve
the existing recommendation handling for non-nil executions.
- Around line 322-328: Expose the canonical Azure savings-plans service name
from internal/commitmentopts/probe_azure.go and update the provider handling in
the purchase manager to use that shared constant instead of duplicating the
literal. Preserve the existing GCP and unknown-provider behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea1126f0-093e-47cb-bb1c-c1653bbfc89c
📒 Files selected for processing (4)
internal/api/handler_purchases.gointernal/api/handler_purchases_retry_redrive_test.gointernal/api/handler_purchases_test.gointernal/purchase/manager.go
Adversarial review — independent, money-pathReviewed the committed diff at Verdict: no blocking findings. The hole is closed, the gate is complete, and it fails closed. Three non-blocking items below, one of which (Finding 1) is a real residual money window that deserves a follow-up issue rather than a change to this PR. Axis 1 — completeness of the gate (built independently)I did not use the PR body's table. I grepped
My enumeration matches the PR body's. One path, correctly identified, correctly gated. Axis 2 — fail-closed on unknown input — verified by executionWrote a throwaway probe in
Axis 3 — was the predicate weakened? No.
Axis 4 — string matching / guard-vs-dispatch reach — verified by execution, this was the highest-risk axisThe dangerous direction is not "a third spelling exists" but "a spelling the guard passes that the dispatch still routes to the Azure savings-plans client". I asserted the invariant directly rather than by inspection: for a candidate set of every
Passes with zero gaps. The reason it holds: Axis 5 — does the refusal actually prevent the purchase?Yes. Axis 6 — test quality — verified by execution, not by reading pasted outputThe test drives I reproduced the pre-fix run myself: checked out with Harness plumbing checked rather than assumed: One honest limit: Axis 7 — proportionalityGood. One predicate renamed from bool to reason-string, one exported wrapper, one call site. No policy framework, no safety-mode enum, no capability table, no config knob. RacesRetry acts only on Findings1. Medium — the gate is at creation time only; successors already persisted by a pre-fix retry still executeThis PR stops new unsafe successors from being created. It does not stop ones that already exist. A row with Not a blocker — the PR closes the ongoing bleed, and gating the executor is a different change with its own blast radius. But the window is real and the check is cheap and precise, because // in claimAndExecute / executeAndFinalize, before the provider call
if exec.RetryAttemptN > 0 {
if reason := RedriveRefusalReason(exec); reason != "" {
// fail the row with `reason` instead of purchasing
}
}Suggest filing this as a follow-up issue alongside LeanerCloud/cloud-commitments-platform#161, and running the ops query for existing rows: SELECT execution_id, status, retry_attempt_n
FROM purchase_executions
WHERE status IN ('pending','notified','approved')
AND retry_attempt_n > 0
AND recommendations::text LIKE '%"provider":"azure"%'
AND (recommendations::text LIKE '%"service":"savingsplans"%'
OR recommendations::text LIKE '%"service":"savings-plans"%');2. Low —
|
| Gate | Command | Result |
|---|---|---|
| build | go build ./... |
exit 0 |
| vet | go vet ./... |
exit 0 |
| tests | go test ./... -count=1 |
exit 0, 29 packages with tests, zero FAIL lines |
| gocyclo | gocyclo -over 10 -ignore "_test\.go" . at CI-pinned v0.6.0 |
exit 0, no output |
| lint | golangci-lint run --timeout=10m ./... at CI-pinned v2.10.1 (ci.yml:49) |
exit 0, 0 issues. |
Lint was run at v2.10.1 specifically, since a newer local version carries a different bundled ruleset and can return a false clean.
CI on the PR: every substantive check is green — Lint Code, Unit Tests, Integration Tests, E2E Tests, pre-commit, Build Docker Image, Snyk, all three Terraform validations, both IAM/role parity checks, GCP grant scope. Security Scanning is red for the unrelated repo-wide npm advisories tracked in #1712. I pulled the CI Success job log to confirm it is a pure aggregator ("One or more CI jobs failed" → exit 1, 3s, no checks of its own), so its red is a consequence of Security Scanning and not an independent failure — but it is also not masking one, since I re-ran every Go gate locally and they pass.
What I could not verify
- No live Azure tenant, so the provider-side claim that an Azure savings-plans OrderAlias has no server-side idempotency key is taken from Azure Savings Plan purchase is non-idempotent (double-buy on re-drive) #1079 / the existing code comments, not re-tested. It is the premise of the original issue, not something this PR introduces.
- Whether any affected rows described in Finding 1 actually exist in production — that needs the query above run against the real database.
Round 2: both CodeRabbit findings addressed (413e390)Detailed replies are on the two threads. Summary of what changed and why. 1. The gate no longer refuses executions with no recommendationsIt did refuse, and not vacuously: That was wrong. An execution with no recommendations buys nothing, so it cannot double-buy: refusing it was failing closed against a hazard that is absent, and a refusal here is permanent. "Empty by construction" vs "empty because loading failed" is decidable, and decides in our favour. A recommendations unmarshal failure propagates as an error from The condition moved back into 2. The frontend renders
|
| Gate | Result |
|---|---|
go build ./... / go vet ./... |
clean |
go test ./... -count=1 |
6543 passed, 42 packages |
internal/purchase (reaper semantics unchanged) |
233 passed |
golangci-lint run ./... at CI-pinned v2.10.1 |
exit 0, 0 issues. |
gocyclo -over 10 -ignore "_test\.go" |
exit 0 |
npx jest history-retry-button |
19 passed, 0 failed |
npx tsc --noEmit / npx eslint |
clean |
package.json and package-lock.json are untouched (npm ci only, which reads the lockfile).
Security Scanning is red for the unrelated repo-wide npm advisories tracked in #1712.
…y prose Drops the frontend change out of this PR and applies three review notes. The frontend terminal-rendering change is reverted to main. The money risk is closed server-side: the retry endpoint returns a 409 and initiates no purchase, and that does not depend on any client honoring it. What remains is a Retry button that is always offered and always fails, which is a real defect but a presentation one, and fixing it properly needs the re-drive verdict on the History row projection (handler_history.go plus the frontend) rather than the in-place suppression this PR carried. That work is tracked in #1714 and does not belong in a p0 whose scope is stopping the spend. Three notes from review: - The redrive_unsafe detail now says at its write site that #1714 is its intended consumer, so the next reader does not have to wonder whether an unread field is dead. - allRecsSafeToRedrive's doc block no longer restates the per-provider policy that RedriveRefusalReason owns. It documents only what it adds, the sweep's own empty-recommendations condition and why that condition is deliberately not part of the shared predicate. The policy table moves to RedriveRefusalReason, and gains the unknown-provider case it had always implemented but never listed. - loadAndValidateRetryRequest is left at gocyclo 10. It measures 10 on origin/main too, this PR does not add to it, and five other functions in the same file sit at the same ceiling, so splitting only this one is an unrelated refactor. Its gates are a documented ordered sequence whose order is the security boundary, which is worth changing under its own review rather than inside a money-path fix. Behavior is unchanged from the previous head: the empty-recommendations narrowing stands, and the full Go suite, golangci-lint v2.10.1 and gocyclo are clean. Refs #1668
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: 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 report clean on a diff it never read. @coderabbitai full review |
|
✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/purchase/manager.go`:
- Around line 322-341: Normalize the Azure service value in
recRedriveRefusalReason before checking for savings plans, accepting supported
casing and separator aliases such as “SavingsPlans” and “savings_plans” and
mapping them to the canonical savings-plans value. Keep the refusal message and
behavior unchanged once normalized, and define the needed canonical value
locally rather than reusing azureSpService.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fd1ecd9-48de-4818-bf18-efb2207851b6
📒 Files selected for processing (3)
internal/api/handler_purchases.gointernal/api/handler_purchases_retry_redrive_test.gointernal/purchase/manager.go
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: 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 report clean on a diff it never read. @coderabbitai full review |
|
✅ Action performedFull review finished. |
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: 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 report clean on a diff it never read. @coderabbitai full review |
|
✅ Action performedFull review finished. |
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: 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 report clean on a diff it never read. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/purchase/manager.go (1)
309-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the re-drive policy out of
internal/purchase/manager.go.The file is 654 lines, exceeding the 500-line guideline. Move
RedriveRefusalReasonand its helpers intointernal/purchase/redrive.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/purchase/manager.go` around lines 309 - 316, Move RedriveRefusalReason and all of its re-drive-specific helper functions from manager.go into a new internal/purchase/redrive.go file, preserving their package, signatures, behavior, and required imports. Remove the moved definitions and any now-unused imports from manager.go.Source: Coding guidelines
internal/api/handler_purchases_retry_redrive_test.go (1)
84-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the mock transaction-save delegation.
SavePurchaseExecutionTxfalls back toSavePurchasewhen no transaction expectation is registered. Add a short comment above the expectation to preserve this test invariant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_purchases_retry_redrive_test.go` around lines 84 - 112, Add a short comment immediately above the SavePurchaseExecution mock expectation in purchasesFiredByRetry documenting that SavePurchaseExecutionTx delegates to SavePurchase when no transaction expectation is registered, preserving the test invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/handler_purchases_retry_redrive_test.go`:
- Around line 84-112: Add a short comment immediately above the
SavePurchaseExecution mock expectation in purchasesFiredByRetry documenting that
SavePurchaseExecutionTx delegates to SavePurchase when no transaction
expectation is registered, preserving the test invariant.
In `@internal/purchase/manager.go`:
- Around line 309-316: Move RedriveRefusalReason and all of its
re-drive-specific helper functions from manager.go into a new
internal/purchase/redrive.go file, preserving their package, signatures,
behavior, and required imports. Remove the moved definitions and any now-unused
imports from manager.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03690a19-04a0-452a-8eab-ad9b1ef02e32
📒 Files selected for processing (4)
internal/api/handler_purchases.gointernal/api/handler_purchases_retry_redrive_test.gointernal/purchase/manager.gointernal/purchase/redrive_guard_reach_test.go
) A purchase_executions row reaching status="failed" does not prove the commitment never landed: a timeout, a lost response or a failed post-purchase write all leave a "failed" row behind an order the provider actually accepted. Every retry is therefore potentially a re-drive, and only a provider-side duplicate guard makes it safe. The codebase already knew this. purchase.recIsSafeToRedrive encoded that Azure savings-plans is unsafe to re-drive (timestamp-derived order alias, no server-side idempotency key) and that an unrecognized provider must be refused. It gated the reaper's automatic re-drive and the reaper's "safe to retry" failure note, and nothing else. The user-facing Retry endpoint gated on status, RBAC, already-retried, the ops-hint map and the attempt threshold, none of which knows anything about provider re-drive safety, so clicking Retry on a landed Azure savings-plans row created a successor that bought a SECOND savings plan. A savings plan is a multi-year commitment that cannot be canceled. Consult the existing predicate on the money path rather than adding a second notion of re-drive safety: - Rework recIsSafeToRedrive into recRedriveRefusalReason (same decisions, expressed as an explanation) and export RedriveRefusalReason over an execution. allRecsSafeToRedrive becomes RedriveRefusalReason(exec) == "", so the reaper and the API cannot disagree and the reaper's semantics, including empty-recs and unknown-provider, are unchanged. - Gate the retry endpoint on it in checkRetryEligibilityGates (renamed from checkRetryRateGates, one call site). The refusal is a 409 naming the provider reason, in the same shape as the ops-hint 409 the History UI already renders, plus a redrive_unsafe flag marking it permanent. - The gate runs first and ?force=true does not override it. force exists to push past the attempt threshold; there is no recovery from a wrongly overridden savings-plan purchase, and an operator who genuinely wants a second commitment can submit a fresh purchase. Regression coverage drives the real chain the operator drives, the retry handler followed by the real purchase.Manager executing the successor, and asserts on the number of purchases reaching the cloud rather than on statuses: a unit test on the predicate alone stays green either way, because the predicate was already correct and simply unconsulted. Covers both savings-plans spellings, a mixed execution whose unsafe rec is not first, an unknown provider, ?force=true, and, as the over-blocking guard, an Azure reservation row that must still retry into exactly one purchase under the predecessor's token. Closes #1668
…rminal (#1668) Two CodeRabbit findings on the re-drive-safety gate. Empty recommendations no longer refuse. createPurchaseExecutionsTx (handler_plans.go) and getOrCreateExecution (purchase/notifications.go) both create executions with no recommendations, and a failed approval email marks those "failed". The gate refused them along with the genuinely unsafe rows, and a refusal here is permanent, so that whole class became unretryable with no operator recovery. Such an execution buys nothing, so it cannot double-buy: refusing it was failing closed against a hazard that is absent. The distinction between "empty as created" and "empty because loading failed" is decidable and lands on the safe side by itself, so nothing is being papered over: a recommendations unmarshal failure propagates as an error from GetExecutionByID (config/store_postgres.go), which the handler turns into a 500 long before the gate runs. Empty at the gate is always empty as created. The condition moves back into allRecsSafeToRedrive, which keeps its byte-for-byte semantics, because it was never a statement about provider duplicate risk. It is the reaper sweep's own "nothing to do, hand it to a human" condition, and the safe-fail path it selects there is benign. RedriveRefusalReason now answers exactly one question, could re-driving these recommendations buy something twice, so exporting it to a caller whose refusal is permanent is safe. The frontend renders the refusal as terminal. redrive_unsafe was reaching the retry-error handler and being displayed through the ops_hint branch, which words a refusal as operator-fixable and re-enables the Retry button. That is the one outcome this p0 exists to prevent: the user reads "failed, try again", clicks again, and buys the duplicate the backend just refused. A redrive_unsafe refusal now replaces the button with the same ops-hint badge renderActionCell already uses for non-retryable rows, carrying the provider's reason, and does not re-enable. The badge is built as a DOM node with textContent rather than an HTML string, since the reason interpolates the row's provider. Regression coverage for both, each confirmed failing on the previous head: a failed recommendationless execution retries into a linked successor, and a redrive_unsafe refusal leaves no retry affordance in the document while an ops_hint-only refusal still leaves the button live. The ops-hint test fixture reverts to its original form: it no longer needs a recommendation to reach the gate under test. Refs #1668
…y prose Drops the frontend change out of this PR and applies three review notes. The frontend terminal-rendering change is reverted to main. The money risk is closed server-side: the retry endpoint returns a 409 and initiates no purchase, and that does not depend on any client honoring it. What remains is a Retry button that is always offered and always fails, which is a real defect but a presentation one, and fixing it properly needs the re-drive verdict on the History row projection (handler_history.go plus the frontend) rather than the in-place suppression this PR carried. That work is tracked in #1714 and does not belong in a p0 whose scope is stopping the spend. Three notes from review: - The redrive_unsafe detail now says at its write site that #1714 is its intended consumer, so the next reader does not have to wonder whether an unread field is dead. - allRecsSafeToRedrive's doc block no longer restates the per-provider policy that RedriveRefusalReason owns. It documents only what it adds, the sweep's own empty-recommendations condition and why that condition is deliberately not part of the shared predicate. The policy table moves to RedriveRefusalReason, and gains the unknown-provider case it had always implemented but never listed. - loadAndValidateRetryRequest is left at gocyclo 10. It measures 10 on origin/main too, this PR does not add to it, and five other functions in the same file sit at the same ceiling, so splitting only this one is an unrelated refactor. Its gates are a documented ordered sequence whose order is the security boundary, which is worth changing under its own review rather than inside a money-path fix. Behavior is unchanged from the previous head: the empty-recommendations narrowing stands, and the full Go suite, golangci-lint v2.10.1 and gocyclo are clean. Refs #1668
A review finding proposed normalizing rec.Service before the Azure
savings-plans re-drive guard, since exact matching lets variants such as
"SavingsPlans" or "savings_plans" past it.
They do get past the guard, and they cannot reach a purchase, which is the
property that decides whether money moves. mapServiceType is the only step
between rec.Service and the provider's service client, and Azure routes
exactly one ServiceType to its savings-plans client:
common.ServiceSavingsPlansAll, matched exactly in
newServiceClientForSubscription, which normalizes nothing and constructs
NewSavingsPlansClient nowhere else. Everything else lands in default and
returns "unsupported service" without purchasing. So the set of values that
can reach an Azure savings-plans purchase is exactly {"savingsplans",
"savings-plans"}, which is exactly the set the guard refuses.
Normalizing before the guard would widen what the guard treats as a savings
plan while the dispatch axis stayed exact, creating a second normalization
axis to keep in lockstep with the first forever. Two axes of one guard
drifting apart is a shape this repo has already been bitten by, so the
guard stays an exact match against the canonical constant and the coupling
gets a test instead.
The test asserts the biconditional over every key of both slug maps, the
literal value of all 20 ServiceType constants, and case, separator,
whitespace and near-miss mutations: a value dispatches to the Azure
savings-plans client if and only if the guard refuses it. 61 cases, no
gaps. Stated a second time as a closed set, so widening either axis shows
up as a diff to an explicit list and not only as a loop failure.
Confirmed non-vacuous: dropping the "savings-plans" spelling from the guard
fails both tests, naming the resulting hole.
Refs #1668
9a8049b to
f47edb4
Compare
|
Rebased onto Rebase was clean, no conflicts. Verification after rebase (new head
|
|
Merging on a verdict whose SHA does not match head, with the justification recorded rather than waived. CodeRabbit's latest verdict is against Verified the rebase preserved the reviewed diff exactly: So the verdict at Everything else is clean at head: all checks green, no failing, no pending, zero unresolved threads. Also on record for this PR beyond CodeRabbit: an independent adversarial review with no blocking findings, which re-derived the guard's completeness by probing every service-slug variant, independently reproduced the pre-fix failure (five tests failing on the money assertion with a recorded purchase token), and confirmed the test drives the real handler into a real Two follow-ups are tracked separately and deliberately excluded here: #1714 (History still renders a Retry affordance the backend now refuses — presentational, the money risk is closed server-side) and #1718 (successors created by a pre-fix retry are still armed; creation-time gating cannot reach them, so the executors need the |
1 participant
The defect
A
purchase_executionsrow reachingstatus="failed"does not prove the commitment never landed. A timeout, a lost response, or a failed post-purchase history write all leave afailedrow behind an order the provider actually accepted. Every retry is therefore potentially a re-drive, and only a provider-side duplicate guard makes it safe.The codebase already knew this.
purchase.recIsSafeToRedrive(internal/purchase/manager.go) encoded that Azure savings-plans is unsafe to re-drive (order alias named fromtime.Now().UnixNano(), no server-side idempotency key) and that an unrecognized provider must be refused. Its wrapperallRecsSafeToRedrivewas called from exactly two places, both in the reaper.It was never called from the user-facing retry path. So an operator clicking Retry on a landed Azure savings-plans row created a successor that, once approved, bought a second savings plan: a multi-year commitment that cannot be canceled.
The fix
Consult the existing predicate on the money path, rather than adding a second notion of re-drive safety.
recIsSafeToRedrivebecomesrecRedriveRefusalReason(the same decisions, expressed as an explanation), andRedriveRefusalReason(exec)is exported over an execution.allRecsSafeToRedriveis nowRedriveRefusalReason(exec) == "", so the reaper and the API cannot disagree. The reaper's semantics are unchanged, including empty-recs (refused) and unknown-provider (refused).checkRetryEligibilityGates(renamed fromcheckRetryRateGates; one call site). The refusal is a409naming the provider reason, in the same shape as the existing ops-hint409, plus aredrive_unsafe: trueflag marking it as permanent rather than operator-fixable.?force=truedoes not override it.forceexists to push past the attempt threshold; there is no recovery from a wrongly-overridden savings-plan purchase, and an operator who genuinely wants a second commitment can submit a fresh purchase, which is an explicit buy rather than a retry of one that may already exist.Every path that can create a retry successor
RetryExecutionID/RetryAttemptNwriters and all re-drive sites onmain, enumerated rather than assumed:internal/api/handler_purchases.goretryPurchase→persistRetryExecution(routePOST /api/purchases/retry/{id},router.go:178)checkRetryEligibilityGatesinternal/purchase/manager.goRecoverStrandedApprovals→claimAndRedriveapprovedrowsallRecsSafeToRedrive, unchangedinternal/purchase/reaper.go:224internal/api/handler_purchases.gopersistExecutionAndSuppressionsexecutePurchasesubmissioninternal/api/handler_plans.go:462createPurchaseExecutionsTxinternal/api/handler_purchases.gofinalizePurchaseStatus, approve/cancel writers (:841,:2279,:2707)cmd/), SQS message types (execute_purchase,approve,cancel,send_notification)So the answer is "one path", but only after checking all of them.
internal/purchase/**andinternal/api/handler_purchases.gowere grepped for everyRetryExecutionID/RetryAttemptNwrite, everySavePurchaseExecution*call, and every re-drive mention.Slug coverage was verified rather than assumed.
mapSavingsPlansSlugrecognizes six savings-plans spellings, while the predicate excludes onlysavingsplansandsavings-plans. The other four (savings-plans-compute,-ec2instance,-sagemaker,-database) map to AWS-onlySupportedSavingsPlansTypevalues; Azure'sGetServiceClientaccepts onlyServiceSavingsPlansAlland returns "unsupported service" for the rest, so those spellings can never purchase on Azure. The dispatch axis and the guard axis have the same reach, including case sensitivity (both are exact-match).Regression test: fails pre-fix, passes post-fix
internal/api/handler_purchases_retry_redrive_test.godrives the real chain the operator drives: the retry HTTP handler, then the realpurchase.Managerexecuting the successor it persisted, counting commitments that reach the provider. It asserts on purchase counts, not statuses: a unit test onRedriveRefusalReasonalone stays green either way, because the predicate was already correct and simply unconsulted.Pre-fix (source reverted to
origin/main, new tests kept)with the money assertion naming the second purchase, and the purchase log line above it showing the duplicate reaching the "cloud":
The over-blocking guard (
TestRetryOfFailedAzureReservationStillPurchasesOnce) passes pre-fix, as it must: it exists to catch a fix that is too broad.Post-fix
Cases covered: both savings-plans spellings; a mixed execution whose unsafe rec is not first (so a head-only check would let it through); an unknown provider;
?force=true; and the over-blocking guard, anazure/computerow of identical shape that must still retry into exactly one purchase, carrying the predecessor's token so Azure's two-step lookup dedupes it.Gates
go build ./...go vet ./...go test ./... -count=1gocyclo -over 10 -ignore "_test\.go" .(v0.6.0, as CI runs it)golangci-lint run ./...at CI-pinned v2.10.10 issues.gofmt -l internal/Behaviour change: executions with no recommendations
Recorded explicitly because it is the one behaviour outside the Azure SP case that this PR was at risk of changing, and it went through two revisions.
Retry path: unchanged from
main. Afailedexecution carrying no recommendations stays retryable. An interim revision of this PR refused it (the shared predicate had inherited the reaper's empty-recs condition), which would have been a regression:createPurchaseExecutionsTx(handler_plans.go:452) andgetOrCreateExecution(purchase/notifications.go:130) both create rec-less executions, a failed approval email marks themfailed, and pre-fixvalidateAndTotalRecommendations([])returned nil so they were retryable. Since such a row buys nothing, it cannot double-buy, so refusing it bought no safety while removing a real recovery path. The refusal now fires only where the duplicate hazard exists.Reaper path: unchanged from
main.allRecsSafeToRedrivestill refuses the empty case, because there a refusal selects a benign safe-fail (mark the row failed, surface it for a human) rather than a terminal one. Same input, different consequence, so the condition belongs to that caller and not to the shared predicate.Net: neither caller's behaviour on empty recommendations differs from
origin/main.RedriveRefusalReasonanswers exactly one question, could re-driving these recommendations buy something twice, which is what makes it safe to export to a caller whose refusal is permanent.Notes for review
git diff origin/main -- frontend/is empty). An interim revision rendered theredrive_unsaferefusal as a terminal state in History; it was reverted. The money risk is closed server-side and does not depend on any client honoring the flag, so what remained was presentation: a Retry button that is always offered and always fails. Fixing that properly needs the re-drive verdict on the History row projection (handler_history.go+ frontend), which supersedes rather than extends the interim in-place suppression. Tracked in #1714 (p2).redrive_unsafehas no consumer yet by design; a comment at its write site records fix(frontend/history): Retry button still offered on Azure savings-plans rows the backend now refuses cloud-commitments-platform#161 as the intended one so it does not read as dead.loadAndValidateRetryRequestsits at gocyclo 10. It measures 10 onorigin/maintoo and this PR does not add to it (only the callee was renamed); five other functions in the same file are at the same ceiling. Its gates are a documented ordered sequence whose order is the security boundary, so splitting it belongs in its own review rather than a money-path fix.pending/notified/approvedfrom a pre-fix retry will still purchase on approval. Creation-time gating cannot reach those rows; this PR stops the ongoing bleed.Closes #1668
Summary by CodeRabbit