[IAmJSD]

Root cause

RecoverStrandedApprovals lists stale approved rows with a SELECT (GetStaleApprovedExecutions), picks the in-place re-drive path from that snapshot (allRecsSafeToRedrive && ExecutionID != ""), then calls claimAndRedrive. That claims the row (TransitionExecutionStatus approved -> running ... RETURNING), but only copied claimed.Status onto the snapshot and passed the snapshot to executeAndFinalize. The claim isn't pinned to the snapshot, so if an edit or another writer changed the row's recommendations, amount or account between the SELECT and the CAS:

• the re-drive bought the stale snapshot rather than the row it had claimed, and
• the terminal save then wrote that snapshot back over the edited row.

The re-drive safety gate also only ever saw the snapshot. A row edited from RDS (safe) to EC2 (no provider duplicate guard) was still re-driven.

Confirmed on current main (8caed39c, which includes #1036). #1036 closed the same check-then-buy gap on the user-triggered paths; the re-drive path doesn't go through transitionApproveAndExecute, so #1036 didn't cover it. (git log --grep '#1037' matches 4a56a2dc, but that's an older, unrelated PR-number collision about idempotency keys and doesn't touch this path.)

Fix

The issue offered two options. This PR takes "re-drive executes the row as returned by the claim", following the pattern claimAndExecute already uses:

• claimAndRedrive now copies the whole claimed row (*exec = *claimed), not just its status. The purchase, the pre-execution refusals (preExecutionRefusal) and the terminal save all run on the claimed row.
• The re-drive gate is now one helper, redrivable(exec): every rec safe to re-drive and a durable ExecutionID. It runs on the listed snapshot to pick the path, as before, and again on the claimed row before any provider call.
• If the claimed row no longer qualifies (recs changed to EC2/Redshift, Azure savings plans or an unknown provider, or emptied), failClaimedRedrive fails it with the result safeFail gives a row listed in that state:
  • status = failed, with the same operator message (strandedRecoveryError, now shared with safeFail)
  • the save is guarded on running, the status this run's claim holds
  • no no_submission stamp and no suppression release. This is deliberately not a beforeSubmission refusal: the interrupted original run may already have bought.
  • it counts as recovered, like safeFail. A failed terminal save returns ErrAuditLoss, like the existing re-drive persistence failure.

New helpers are in internal/purchase/stranded_redrive.go. manager.go was already over the 500-line guideline, so this keeps its growth to doc comments.

Verification

All with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null: this machine's ~/.aws/config default profile is invalid, and nothing here needs AWS config.

Unit tests (mock store, mock provider), internal/purchase/redrive_claimed_row_test.go:

• TestRecoverStrandedApprovals_RedrivesClaimedRowNotSnapshot: listed 1 instance at $200, claimed row repriced to 7 at $1,400. The provider must be asked for 7, and the saved row must keep 7 and $1,400.
• TestRecoverStrandedApprovals_ClaimedRowNoLongerRedrivableIsFailed (claimed rec changed to EC2; recs emptied): no provider call, row failed with no_submission=false, save guarded on running, and the recovery message with the right next step.
• TestRecoverStrandedApprovals_ClaimedRowFailSaveErrorIsAuditLoss

Integration tests (integration tag, real Postgres via testcontainers, real store and Manager, counting fake provider; no cloud API), internal/purchase/redrive_claimed_row_integration_test.go:

• TestRecoverStrandedApprovals_ConcurrentSweepsBuyClaimedRowOnce: 4 concurrent sweeps all list the row. A barrier holds them until the row is repriced from 1 to 5 instances through the production upsert, then they race the claim. Result: exactly one provider call, for 5, and the row is completed with 5.
• TestRecoverStrandedApprovals_ClaimedRowTurnedUnsafeIsFailedNotBought: after the listing, the RDS rec is edited to EC2. Zero provider calls; the row is failed with no_submission=false, keeps the EC2 rec, and carries the "start a new purchase" message.
• Test seam, stated honestly: the wrapper store calls the real GetStaleApprovedExecutions with a zero age instead of 15 minutes, because the fixture row was written moments earlier. Every other store call is the production one.

Fail before / pass after (production manager.go reverted to 8caed39c, tests unchanged):

# unit, before
--- FAIL: TestRecoverStrandedApprovals_RedrivesClaimedRowNotSnapshot      expected: []int{7}  actual: []int{1}
--- FAIL: TestRecoverStrandedApprovals_ClaimedRowNoLongerRedrivableIsFailed/recommendation_changed_to_EC2   (provider reached for the listed rds rec; saved recs are the snapshot's)
--- FAIL: TestRecoverStrandedApprovals_ClaimedRowNoLongerRedrivableIsFailed/recommendations_emptied
--- FAIL: TestRecoverStrandedApprovals_ClaimedRowFailSaveErrorIsAuditLoss
# integration (real Postgres), before
--- FAIL: TestRecoverStrandedApprovals_ConcurrentSweepsBuyClaimedRowOnce          expected: []int{5}  actual: []int{1}; stored count reverted to 1
--- FAIL: TestRecoverStrandedApprovals_ClaimedRowTurnedUnsafeIsFailedNotBought    1 provider call; status "completed"; stored rec reverted from ec2 to rds
# after: all PASS (integration pair also passes -race -count=3)

Mutation probe: keep *exec = *claimed but drop the post-claim redrivable check, and both ClaimedRowNoLongerRedrivableIsFailed subtests fail.

Commands run:

go build ./...
go vet ./internal/purchase/ && go vet -tags integration ./internal/purchase/
go test -race ./internal/purchase/ -count=1
go test ./internal/mocks/ -count=1
go test -tags integration ./internal/purchase/ -run 'Redrive|Stranded|SingleClaim|Reap|ClaimPin|NotifiedTransition|ClaimedRow' -count=1
go test -race -tags integration ./internal/purchase/ -run 'TestRecoverStrandedApprovals_(ConcurrentSweeps|ClaimedRowTurnedUnsafe)' -count=3
golangci-lint v2.10.1 run --allow-parallel-runners [--build-tags integration] ./internal/purchase/...   # 0 issues both ways
gocyclo -over 10 -ignore "_test\.go" internal/purchase/                                               # silent
bash scripts/gosec-hook.sh <changed files>                                                             # clean
git-secrets --scan (fresh temp repo, isolated HOME, scripts/setup-git-secrets.sh) + scripts/test-git-secrets-allowlist.sh  # clean, 42 PASS
gofmt -l, git diff --check                                                                             # clean

Real-cloud coverage gap: nothing here touched a real provider. Re-drive idempotency against real AWS, Azure and GCP is unchanged and still covered only by the existing provider-level tests.

Out of scope / for the reviewer

• No snapshot pin on the claim. The claim is still unpinned. A row edited after the listing that still qualifies for a re-drive is bought as claimed, without waiting another staleApprovedThreshold. Pinning to updated_at would need updated_at on PurchaseExecution and in the shared scanExecutionRows column list, which all 14 execution queries use. That's a much wider change than this issue needs, and executing the claimed row already closes the check-then-buy gap. Happy to add a pin if you'd rather refuse any change.
• Pre-existing, not changed: preExecutionRefusal's stale-Azure-pricing and ladder refusals are beforeSubmission errors on every executor path, including this re-drive. On a stranded row, where the interrupted run may have submitted, a refusal there stamps no_submission and releases plan suppressions. Worth a separate issue if you agree.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/security, triaged.

Closes #1037

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8caed39
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 01cd26df-1acb-4a2b-af71-5a85a414071d

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and 1372642.

📒 Files selected for processing: 4

• internal/purchase/manager.go
• internal/purchase/redrive_claimed_row_integration_test.go
• internal/purchase/redrive_claimed_row_test.go
• internal/purchase/stranded_redrive.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

VERDICT: APPROVE 1372642

Reviewed the full diff of #1095 (one commit) against origin/main 56e226a. The base is 8caed39, 39 commits behind. git merge-tree against current main is clean (tree e7d49526), and every test below ran on that merged tree.

Findings (none blocking):

• nit internal/purchase/stranded_redrive.go:42 - failClaimedRedrive fails the row but does not call notifyExecutionFailure (chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714, merged on main after this PR's base), so the purchase_execution_failed alarm marker and the email are not emitted. This matches safeFail (manager.go:646) and failClaimedBeforeSubmission (step_cap.go:63), which also skip it, so the gap already exists on main. It still matters most here, because the interrupted run may have bought. Consider a follow-up issue.
• nit internal/purchase/stranded_redrive.go:29 - mutation probe P5 (drop exec.ExecutionID != "" from redrivable) survives all TestRecoverStrandedApprovals* tests. Coverage of the legacy empty-ID branch already had this gap before the PR. The PR moved the condition but did not add the test.
• nit process - the single commit changes 438 lines (425+/13-), over the 400-line cap. Non-blocking.

Checked and correct:

• The claim's RETURNING list (store_postgres.go transitionExecutionStatus) has the same 29 columns as GetStaleApprovedExecutions, and both go through scanExecutionRows. *exec = *claimed therefore drops no field. The Postgres store is the only real implementer of TransitionExecutionStatus.
• Every field the re-drive reads (Recommendations, TotalUpfrontCost, CloudAccountID, PlanID, StepNumber, RetryAttemptN, IdempotencyKey, ExecutionID) now comes from the claimed row. The checks that run on it are preExecutionRefusal, armedRedriveRefusal, executePurchase, saveTerminalExecution (guarded on running) and updatePlanProgress.
• Double purchase: claimRunning is an approved->running CAS. A lost CAS is a benign skip, and the terminal and fail saves are guarded on running. The PR's 4-sweep barrier integration test passes on real Postgres.
• failClaimedRedrive does not use beforeSubmission, so it stamps no no_submission and releases no suppression. That is consistent with safeFail's semantics.
• A failed save returns ErrAuditLoss, which propagates out of RecoverStrandedApprovals.

Local results (merged tree, macOS):

• go build ./..., go vet and go vet -tags integration on purchase and config: clean.
• go test ./internal/purchase/ ./internal/mocks/: 808 passed.
• go test -race -tags integration ./internal/purchase/ ./internal/config/ (testcontainers Postgres): ok (283s, 543s).

Attacked (mutation probes in a scratch copy, reverted after each):

• P1, keep only claimed.Status (pre-fix behavior): RedrivesClaimedRowNotSnapshot, both NoLongerRedrivable subtests and FailSaveErrorIsAuditLoss failed. Killed.
• P2, drop the post-claim redrivable gate: both NoLongerRedrivable subtests failed. Killed.
• P3, wrap the cause in beforeSubmission: the EC2 subtest failed. Killed.
• P4, swallow the terminal save error: FailSaveErrorIsAuditLoss failed. Killed.
• P5, redrivable without the ExecutionID check: survived (nit above).

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 1372642

Findings (none blocking):

• [non-blocking] internal/purchase/manager.go:679 (safeFail) - The mirror of the re-drive gap is still open here.
  • The operator message is built from the pre-claim snapshot (strandedRecoveryError(exec)), but the saved row is the updated row the CAS returned.
  • Failure scenario: the snapshot has empty recs, so the sweep takes safeFail. An EC2 rec is attached before the CAS, and the row fails saying "then Retry", although Retry is refused for EC2.
  • Low impact, because the Retry endpoint still refuses, so nothing double-buys. The fix is one token: strandedRecoveryError(updated).
• [non-blocking] internal/purchase/stranded_redrive.go:42 (failClaimedRedrive), merged with origin/main - Main's chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714 failure alert (notifyExecutionFailure) is deferred only inside executeAndFinalize.
  • So a row failed here logs no purchase_execution_failed marker and sends no email.
  • safeFail and failClaimedBeforeSubmission behave the same way on main, so this is consistent, not a regression.
  • Worth a follow-up so that every failed row raises the alert.
• Checked and fine:
  • The claim's RETURNING list (internal/config/store_postgres.go:1320-1327) uses the same queryExecutions scanner as the stale SELECT, so *exec = *claimed drops no field.
  • failClaimedRedrive deliberately avoids beforeSubmission. saveTerminalExecution (manager.go:326-340) stamps no no_submission, releases no suppressions and stays guarded on running, which is right for a run that may already have bought.

Attacked:

• Root cause vs fix(purchase): re-drive buys the pre-claim snapshot instead of the claimed row #1037: the PR takes the issue's "execute the claimed row" option, mirroring claimAndExecute. It also re-runs the safety gate on the claimed row, which goes beyond the issue.
• Blast radius: redrivable, strandedRecoveryError and failClaimedRedrive are unexported and called only at manager.go:605, 606, 679 and 736. No exported API changed, and there is no migration.
• Merge: merge-tree with origin/main is clean. The only change to manager.go on main is the chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714 notify defer, covered above.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 5 were caught:
  • M1, copy only the status instead of the whole row: 3 unit and 2 integration tests FAIL.
  • M2, drop the post-claim redrivable gate: ClaimedRowNoLongerRedrivable and ClaimedRowTurnedUnsafe FAIL.
  • M3, mark the failure as beforeSubmission: a unit and an integration test FAIL.
  • M4, failClaimedRedrive returns false: ClaimedRowNoLongerRedrivable FAILS.
  • M5, terminal-save guard widened to running+approved: 4 tests FAIL.

Verified locally, on git archive exports of head and of the merged tree with no cloud credentials, under the heavy lock. All of the following were OK on both:

• go build ./....
• go vet ./internal/purchase/, with and without -tags integration.
• gofmt -l on the 4 changed files: clean.
• go test -count=1 ./internal/purchase/.
• -race -run 'Stranded|Redrive|ClaimedRow'.
• -tags integration -run 'Redrive|Stranded|ClaimedRow|SingleClaim|Reap|ClaimPin' (testcontainers Postgres).

Fail-before: both new test files on base fail as the PR body describes:

• Unit: it buys [1] instead of [7], and the provider is called in the EC2 and emptied cases.
• Integration: it buys [1] instead of [5], the row ends completed instead of failed, and the stored rec reverts to rds.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at 13726429791e82fccc43f95886d752393161313d merged onto current main (5c2dcbd5; merge-tree 9f03b2bd; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/purchase.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at 13726429791e82fccc43f95886d752393161313d).