fix(purchase): keep account scope on per-account retry successors by cristim · Pull Request #1655 · LeanerCloud/cloud-commitments-cli

cristim · 2026-07-28T19:25:31Z

The double spend

A multi-account plan fans out into one purchase_executions row per cloud account (purchase.executeForAccount), each stamped with cloud_account_id and a per-account idempotency key "<root-key>:<accountID>". Those rows reach status="failed" on their own, and loadAndValidateRetryRequest only requires Status == "failed" — so History offers Retry on them.

persistRetryExecution copied IdempotencyKey, PlanID, StepNumber, Recommendations and CapacityPercent onto the successor, but not CloudAccountID. The successor was born account-scopeless, and executePurchase reads a nil cloud_account_id on a planned execution as "root execution" and re-enters the fan-out.

Concretely, for plan P over accounts A, B, C where A and B committed real RIs and only C failed:

	key used	provider dedupe
original A	`K:A`	—
original B	`K:B`	—
retry of C, account A	`K:C:A`	misses `Derive("K:A", i)`
retry of C, account B	`K:C:B`	misses `Derive("K:B", i)`

The re-suffixed keys derive tokens matching neither the AWS ClientToken, the EC2 RI tag-guard, nor the Azure two-step lookup, so nothing deduped them. At $300 upfront per reserved instance that is $600 of unintended spend from a single Retry click on a three-account plan, scaling linearly with the plan's account count.

The fix

Two gates.

internal/api/handler_purchases.go — persistRetryExecution carries CloudAccountID onto the successor, by value (via a new clonePtr) so the successor and the historical failed row never alias the same pointee, matching the existing defensive deep copy of Recommendations. A per-account retry is then a single-account re-drive against the identical lineage key, so the provider dedupes it.
internal/purchase/execution.go — executePurchase refuses to fan out a row whose lineage key already terminates in one of the plan's own account IDs while the row carries no cloud_account_id. This is the issue's second fix direction ("refuse to re-derive a lineage key that already contains an account suffix") and it covers successors minted by the pre-fix handler that are still pending/approved when this deploys: they fail loud with zero purchases rather than re-buying the accounts that already committed. Matching against the plan's account set (rather than parsing for a UUID-shaped tail) is what makes it precise — a root key is a UUID or a legacy execution ID and cannot end in ":" plus one of this plan's account IDs.

The refusal is deliberately narrow. A genuine root row still fans out across every plan account under its own per-account key, because a legitimate multi-account purchase silently never happening is the same defect with the opposite sign.

No schema change.

Regression coverage

The guards reproduce the real scenario end to end — the retry HTTP handler building the successor, then the real purchase.Manager executing it — and count the commitments that actually reach the provider. A unit test on either half alone would stay green while the double-spend lived in the seam between them.

Against pre-fix code, TestRetryOfFailedPerAccountRowDoesNotRefanOut fails with three purchases where one was expected:

    Error:      	Not equal:
        expected: []string{"95c39ad214adf27b9b4aa2b25f66675cab3bfba9c80544bf0540df13033c31d4"}
        actual  : []string{"39e31f5d2561bd65c043aa27883f0379743c3adca0a584db1c2927eaba3328ce",
                           "a4257f415adf9563365a4ea8c395da7ba99e090c5e6f586bec2cf2e2feb9b186",
                           "acec650491292824ddd2e10c7a04ff75f3a0bf4df5aae0df1e5cfaa1c5b27056"}
    Messages:   	retrying account C's failed row must fire exactly ONE purchase, carrying the
                 	ORIGINAL C attempt's provider token. Extra tokens are accounts that already
                 	committed being bought AGAIN (issue #1537)

and TestScopelessPerAccountKeyRefusesToFanOut fails with Should be zero, but was 2.

Both directions are guarded:

test	direction	pre-fix
`TestRetryOfFailedPerAccountRowDoesNotRefanOut`	(a) two attempts at the same purchase converge on one	FAIL (3 purchases)
`TestRetryOfPerAccountRowsKeepsAccountsDistinct`	(b) different accounts keep distinct tokens	FAIL (3 purchases)
`TestRetryOfFailedRootRowStillFansOutToEveryAccount`	(b) a legitimate fan-out is not suppressed	PASS (must stay)
`TestScopelessPerAccountKeyRefusesToFanOut`	second gate buys nothing	FAIL (2 purchases)
`TestRootKeyStillFansOutThroughExecutePurchase`	second gate stays narrow	PASS (must stay)

Gates

gate	exit
`go build ./...`	0
`go vet ./internal/...`	0
`go test ./internal/...`	0
`go test -race ./internal/purchase/...`	0
`go test -race ./internal/api/...`	0
`gocyclo -over 10` (pre-commit scope: non-test files)	0
`golangci-lint run ./internal/...` (v2.10.1, the CI pin)	0 — `0 issues.`
`gofmt -l internal/`	clean

persistRetryExecution sat at complexity 10 with the propagation inlined; extracting clonePtr returns it to 9.

Note on the reaper

The issue observes that reaper.go appends "; safe to retry" whenever allRecsSafeToRedrive is true, "blind to the per-account-row shape". With both gates in place that hint is now accurate for a per-account row — the retry is a single-account re-drive under the identical token — so it is left unchanged.

Closes #1537

A multi-account plan fans out into one purchase_executions row per cloud account, each stamped with cloud_account_id and a per-account idempotency key "<root-key>:<accountID>". Those rows reach status="failed" on their own, so History offers Retry on them. persistRetryExecution copied the lineage key, plan and step onto the successor but not cloud_account_id. The successor was therefore born account-scopeless, and executePurchase reads a nil cloud_account_id on a planned execution as "root execution" and re-enters the fan-out: for a plan covering accounts A, B and C where only C failed, one Retry click on C re-purchased A and B as well, under re-suffixed keys "<root-key>:C:A" whose derived tokens match neither the AWS ClientToken, the EC2 RI tag-guard nor the Azure two-step lookup, so nothing deduped them. At $300 upfront per reserved instance that is $600 of unintended spend from a single click on a three-account plan, scaling linearly with the plan's account count. Two gates now: - persistRetryExecution carries cloud_account_id onto the successor (by value, so the successor and the historical failed row never alias the same pointee). A per-account retry stays a single-account re-drive against the identical lineage key, so the provider dedupes it. - executePurchase refuses to fan out a row whose lineage key already terminates in one of the plan's own account IDs while the row carries no cloud_account_id. That covers successors minted by the old handler and still pending/approved when this deploys: they fail loud with zero purchases instead of re-buying the accounts that already committed. The refusal is deliberately narrow: a genuine root row still fans out across every plan account under its own per-account key, because a legitimate multi-account purchase silently never happening is the same defect with the opposite sign. Regression coverage reproduces the real scenario end to end (retry handler -> real purchase.Manager, counting the commitments that reach the provider). Against pre-fix code the per-account retry fires three purchases instead of one; both directions are guarded: retries of different accounts' rows keep distinct provider tokens, and a root-row retry still buys once per account. No schema change. Closes #1537

coderabbitai · 2026-07-28T19:25:40Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 19 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 87577c3a-33a9-421c-aea2-75f70bd8db82

📥 Commits

Reviewing files that changed from the base of the PR and between 887d51f and 85c0d94.

📒 Files selected for processing (4)

internal/api/handler_purchases.go
internal/api/handler_purchases_retry_fanout_test.go
internal/purchase/execution.go
internal/purchase/money_path_regression_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/1537-fanout-retry-double-purchase

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-07-28T19:26:55Z

@coderabbitai review

coderabbitai · 2026-07-28T19:27:01Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Review follow-up on the #1537 guard. Two defects in the second gate, both in internal/purchase/execution.go. The gate detected a per-account row by matching the lineage key's terminal segment against the plan's CURRENT account set. That fails OPEN precisely when the scoped account has since been removed from the plan, or deleted and re-added under a new ID: nothing matches, no refusal fires, and the row re-fans-out exactly as it did before the fix. Demonstrated against the previous commit -- plan {A, B}, scopeless row keyed "root-lineage:acct-C" -> no error and two commitments, under tokens derived from "root-lineage:acct-C:acct-A" and ":acct-B", neither of which matches anything the first attempt used. At $300 upfront per reserved instance that is $600 undeduplicated. Detect per-account-ness by the colon instead. executeForAccount is the only writer that puts a colon in a lineage key; every other producer yields a colon-free value (SavePurchaseExecution mints a bare UUID, persistRetryExecution copies verbatim or falls back to an ExecutionID, which is also a UUID, and the read path round-trips the column). So on a scopeless row a colon is by itself proof the row is per-account, and the detector no longer depends on mutable plan membership. The gate also had no way out. It errored, the row landed failed, and retrying it produced another scopeless successor carrying the same key, which the gate refused again -- through the retry threshold and through force. The predecessor could not be re-driven either: it already has retry_execution_id set, so the retry endpoint 409s with "act on its descendant instead", and the descendant is the row that can never execute. Only DB surgery or cancelling ended it. So when the terminal segment does match a current plan account, repair rather than refuse: stamp CloudAccountID and run the single-account re-drive the row was always meant to be. The lineage key is durable proof of the intended scope, and the derived per-rec token is then reproduced exactly, which makes this strictly safer than erroring -- same purchase count, correct dedupe, plus a way out. The repair persists: executeAndFinalize saves the row and cloud_account_id is in SavePurchaseExecution's ON CONFLICT SET. The fail-loud error is reserved for the unrecoverable case above, and now names the departed account so the operator knows which one to re-add. Also corrects an overstatement in persistRetryExecution's comment: carrying the account scope is the precondition for provider-side dedupe, but it is not sufficient for Azure savings-plans, which has no server-side idempotency key and names its order alias from time.Now().UnixNano(). recIsSafeToRedrive encodes that exclusion but gates only the reaper, not the user-facing retry; filed as #1668. Refs #1537

cristim · 2026-07-28T20:03:08Z

Pushed 85c0d94 addressing two confirmed findings from an independent review of the first commit. Both are in the second (internal/purchase) gate; the primary fix in persistRetryExecution is unchanged.

1. The guard failed open when the scoped account had left the plan. It detected per-account-ness by matching the lineage key's terminal segment against the plan's current account set, so a scopeless row keyed "root-lineage:acct-C" on a plan that no longer contains C matched nothing, the refusal never fired, and the row re-fanned-out exactly as pre-fix. Reproduced against the previous commit: err=<nil>, two commitments, under tokens derived from "root-lineage:acct-C:acct-A" and ":acct-B" — $600 undeduplicated at $300/RI.

It now detects the colon instead. executeForAccount is the only writer that puts a colon in a lineage key; SavePurchaseExecution mints a bare UUID, persistRetryExecution copies verbatim or falls back to an ExecutionID (also a UUID), and the read path round-trips the column. So on a scopeless row a colon is by itself proof the row is per-account, and the detector no longer depends on mutable plan membership.

2. The refusal had no recovery path. A refused row lands failed; retrying it produces another scopeless successor carrying the same key, which is refused again — through the threshold and through ?force=true. The predecessor cannot be re-driven either: it already has retry_execution_id, so the endpoint 409s with "act on its descendant instead", and the descendant is the row that can never execute.

When the terminal segment does match a current plan account, the executor now repairs rather than refuses: it stamps CloudAccountID and runs the single-account re-drive the row was always meant to be. The lineage key is durable proof of the intended scope and the derived token is reproduced exactly, so this is strictly safer than erroring — same purchase count, correct dedupe, plus a way out. The repair persists (executeAndFinalize saves the row; cloud_account_id is in SavePurchaseExecution's ON CONFLICT SET). Fail-loud is reserved for the unrecoverable case above, and the error now names the departed account so the operator knows which one to re-add.

Tests. TestScopelessPerAccountKeyRefusesToFanOut is replaced by two, both asserting purchase counts and derived tokens rather than statuses. Against the previous commit:

=== RUN   TestScopelessPerAccountRowSelfHealsToItsAccount
    Error:  Received unexpected error:
            refusing to fan out execution successor-of-acct-A-row across 2 plan accounts: ...
--- FAIL: TestScopelessPerAccountRowSelfHealsToItsAccount (0.00s)
=== RUN   TestScopelessPerAccountRowRefusesWhenAccountLeftThePlan
    Error:  Should be empty, but was [867627339f2fdcd453a7ac0edf9b055bae491f29cb21c9dcd033b446f0dcdc6c 93fd6c678f00d450e57b1ba46a3215f78fd241cecbee8cbb37634743b4d2fb9f]
    Error:  An error is expected but got nil.
--- FAIL: TestScopelessPerAccountRowRefusesWhenAccountLeftThePlan (0.00s)

Those two hashes are exactly Derive("root-lineage:acct-C:acct-B", 0) and Derive("root-lineage:acct-C:acct-A", 0) — the re-suffixed keys. Both pass now, and TestRootKeyStillFansOutThroughExecutePurchase still passes, so the refusal stayed narrow.

Also corrected an overstatement. persistRetryExecution's comment claimed "so the provider dedupes it" unqualified. Carrying the scope is the precondition for dedupe, but it is not sufficient for Azure savings-plans, which has no server-side idempotency key and names its order alias from time.Now().UnixNano(). recIsSafeToRedrive encodes that exclusion but gates only the reaper (reaper.go:224, manager.go:469), never this user-facing retry. Filed as #1668 (p1) — pre-existing, out of scope here.

Filed, not fixed: #1669 (p1) — IncrementPlanCurrentStep does an unconditional CurrentStep++, so retrying two failed accounts of one ramp step advances the ramp twice and a later step silently never purchases. Pre-existing and unchanged by this PR, but per-account retry becomes the canonical recovery flow after it, which raises the exposure.

Gates, all exit 0: go build ./..., go vet ./internal/..., go test ./internal/..., go test -race on both ./internal/purchase/... and ./internal/api/..., gofmt -l internal/ clean, gocyclo -over 10 over the pre-commit scope (executePurchase is 7, reattachAccountScope 5, executeScopeAware 3), and golangci-lint run ./internal/... reporting 0 issues. on the CI-pinned version:

golangci-lint has version 2.10.1 built with go1.26.5 from (unknown, modified: ?, mod sum: "h1:flhw5Px6ojbLyEFzXvJn5B2HEdkkRlkhE1SnmCbQBiE=") on (unknown)

@coderabbitai full review

coderabbitai · 2026-07-28T20:03:22Z

@cristim: Understood. I’ll perform a full review of the updated PR, with particular attention to the colon-based lineage invariant, account-scope self-healing versus departed-account refusal, persistence of repaired scope, and the end-to-end token/count regressions. I’ll treat #1668 and #1669 as documented out-of-scope follow-ups.

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1655

Timestamp: 2026-07-28T20:03:21.442Z
Learning: In the Go purchase execution flow, a colon in a `purchase_executions` lineage/idempotency key is a durable indicator that `executeForAccount` created a per-account execution: root executions use bare UUID or legacy execution-ID keys, while retry persistence copies the lineage key verbatim.

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1655

Timestamp: 2026-07-28T20:03:21.442Z
Learning: For a legacy scopeless per-account purchase execution, `internal/purchase` should recover scope by matching the lineage key’s terminal account segment to a current plan account and persist `CloudAccountID`; if that account has left the plan, execution must fail without fan-out to prevent re-suffixed provider tokens and duplicate purchases.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 19 minutes.

cristim added priority/p0 Drop everything; same-day fix severity/critical Major harm when it happens urgency/now Drop other things impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged labels Jul 28, 2026

cristim merged commit 1dbff53 into main Jul 28, 2026
19 checks passed

cristim mentioned this pull request Aug 19, 2026

fix(plans): a ramp step counts as complete when one of N accounts buys, and both silent-under-buy paths are log-only #1861

Closed

Navigation Menu

Repository navigation

fix(purchase): keep account scope on per-account retry successors - #1655

fix(purchase): keep account scope on per-account retry successors#1655
cristim merged 2 commits into
mainfrom
fix/1537-fanout-retry-double-purchase

cristim commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

Review limit reached

Uh oh!

cristim commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

cristim commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cristim commented Jul 28, 2026

The double spend

The fix

Regression coverage

Gates

Note on the reaper

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading