[IAmJSD]

Root cause

After #829 (#833), the executors claim running by CAS and guard their own audit stamp and terminal save on running. Six other saves that run after a status CAS (or right after a fresh read) still wrote the whole row with SavePurchaseExecution(Tx), an upsert that sets status from an in-memory copy. If another writer changed the status between the CAS and the save, the save put the old status back.

Confirmed on current main (8caed39c). Nothing merged covers it: gh pr list --state merged --search 835 is empty, and git log --grep '#835' only matches 1c632e6e, an unrelated frontend PR whose own number was #835.

Two of these races are reachable today:

• Delayed approve vs. revoke (scheduleApprovedExecution, internal/api/handler_purchases.go). The approve CAS moves the row pending|notified -> scheduled, then saves the scheduled_execution_at / approved_by stamp. If a revoke (CancelScheduledExecutionAtomic) lands between the two, the stamp writes scheduled back over canceled. The row now has a scheduled_execution_at, so the scheduler buys a purchase the user revoked for free. The real-Postgres replay below shows exactly this before the fix.
• Post-execution writes vs. a session revoke (rotateApprovalToken, recordRampAdvanceRefusal). Both write the executor's in-memory completed row after the terminal save. A revokeViaSession CAS (completed -> revocation_requested) landing first was erased.

The other two are not reachable today. The guard is defense in depth for them:

• safeFail (manager.go, stranded-approval sweep). Its stamp could turn a row that the late original run had already recovered to completed (via RecoverReapedExecution) back into failed, which invites a Retry that buys a second commitment. This only applies to legacy approved rows.
• failExistingPlanStep (notifications.go) and clearApprovalToken (approvals.go, after cancel). No production writer moves a failed plan-step row or a canceled row today. Since fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 (now on main), failExistingPlanStep runs its CAS, save and suppression release in one transaction, so the CAS's row lock already keeps other writers out until commit.

Fix

Each site now uses SaveExecutionIfStatus(Tx), which locks the row FOR UPDATE, checks the status and writes in one transaction. Each one expects the status its own CAS or read established:

Site	Guarded on	On a lost guard
scheduleApprovedExecution	scheduled	nothing written; approveWithDelay returns 409 (same as a lost CAS)
safeFail stamp	failed	AUDIT GAP log; still counted as recovered (unchanged contract)
failExistingPlanStep	failed	the transaction (shared with the CAS since #1176) rolls back, so the CAS and the suppression release are undone too; error returned
rotateApprovalToken	the terminal status just written	error; the caller already treats rotation as best-effort
recordRampAdvanceRefusal	the terminal status just written	AUDIT LOSS log (unchanged)
clearApprovalToken	canceled	error; the caller already treats the clear as best-effort

The issue's "Also" item: a failed audit stamp after the claim. I chose fail-closed on a lost claim and best-effort otherwise. The new helper is stampClaimedAudit in claim.go, used by transitionApproveAndExecute and fireOneDue:

• If the running-guarded stamp returns ErrExecutionNotInExpectedStatus or ErrNotFound, the claim is provably gone. The run stops before any provider call and returns errClaimLostBeforePurchase, which wraps the store error.
  • Approve, direct execute and run-now return a nil row, so executeFailureError maps it to 409.
  • fireOneDue reports a race loss.
• Any other store error stays best-effort, as before. The claim is still this run's, the terminal save is still guarded on running, and missing attribution should not block a purchase on a transient DB error.

Merge with main

Main has been merged into this branch, with no rebase. Two files conflicted:

• internal/purchase/manager.go (safeFail): fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095 moved the recovery message into strandedRecoveryError(exec), which produces the same text. The merge keeps that helper and this PR's save guarded on failed.
• internal/purchase/notifications.go (failExistingPlanStep): fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 moved the pinned CAS, the stamp and the suppression release into one transaction (failPlanStepTx). The merge keeps that structure and changes only its save to SaveExecutionIfStatusTx(..., failed). The CAS's row lock already makes the guard redundant there, so it is defense in depth. The failExistingPlanStep subtest of TestPostCASSave_SimulatedWriterSurvives was removed, because its replayed writer can no longer land between the CAS and the save. fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176's plan_step_fail_atomic_integration_test.go covers this site's concurrent writers.

Verification

All commands were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's ~/.aws/config default profile is invalid. Nothing here needs AWS config, and nothing reaches a cloud API.

Unit tests (mock store and mock provider). These pin the guarded store methods themselves, because the mock falls back to SavePurchaseExecution when no guarded expectation is set:

• internal/purchase/post_cas_guarded_save_test.go: safeFail, failExistingPlanStep (guard, plus "a lost guard releases nothing"), recordRampAdvanceRefusal, approve and fireOneDue with a lost stamp (no provider call), and stampClaimedAudit (store error stays best-effort; a missing row counts as a lost claim).
• Updated: TestApprove_ClaimsRunningAndStampsGuarded (three guarded writes, no unguarded one), TestClearApprovalToken_PersistsEmpty, TestMintRevocationToken_PersistsFreshToken, TestHandler_scheduleApprovedExecution_HappyPath.
• New: TestApproveWithDelay_StampLostToRevokeMaps409.

Integration tests (integration tag, real PostgreSQL via testcontainers, real store, Manager and handler, counting fake provider). Note that the repo's container helper runs postgres:16-alpine, not 17. In each test the concurrent write is replayed deterministically between the CAS (or read) and the save, using a wrapper store that runs it once before the first execution save of any kind, guarded or not. The replay is therefore identical before and after the fix.

• internal/api/handler_purchases_schedule_revoke_race_integration_test.go, TestApproveWithDelay_Postgres_RevokeBeforeStampStaysCanceled: the revoke is the exact store work revokeScheduledExecution does (CancelScheduledExecutionAtomic plus the suppression delete, in one transaction).
• internal/purchase/post_cas_guarded_save_integration_test.go:
  • TestPostCASSave_ConcurrentStatusChangeSurvives, with production writers:
    • safeFail vs. a late RecoverReapedExecution
    • token rotation vs. revokeViaSession's CAS
    • ramp-refusal note vs. revokeViaSession's CAS
  • TestPostCASSave_ClaimLostBeforeStampBuysNothing: approve and scheduled fire, with the reaper's CAS landing right after the claim. This ordering is not reachable in production today, because the reaper waits 10 minutes.
  • TestPostCASSave_SimulatedWriterSurvives: clearApprovalToken with a simulated writer (a status CAS to expired), labeled as such in the test. The failExistingPlanStep subtest was removed in the main merge (see above).

Fail before / pass after (run at 1faae48d, before the main merge). I reverted the six production files to origin/main, left the tests unchanged, and re-ran them. One temporary shim, not committed, provided the pre-fix stampClaimedAudit / errClaimLostBeforePurchase so the helper's direct tests compile:

# integration (real Postgres), before
TestApproveWithDelay_Postgres_RevokeBeforeStampStaysCanceled  status expected "canceled" actual "scheduled"; scheduled_execution_at set; row returned by GetScheduledExecutionsDue; no 409
TestPostCASSave_ConcurrentStatusChangeSurvives/safeFail_stamp_after_a_late_recovery         expected "completed" actual "failed"
TestPostCASSave_ConcurrentStatusChangeSurvives/revocation_token_rotation_after_a_revoke     expected "revocation_requested" actual "completed"
TestPostCASSave_ConcurrentStatusChangeSurvives/ramp-advance_refusal_note_after_a_revoke     expected "revocation_requested" actual "completed"
TestPostCASSave_ClaimLostBeforeStampBuysNothing/approve          provider calls expected 0 actual 1
TestPostCASSave_ClaimLostBeforeStampBuysNothing/scheduled_fire   provider calls expected 0 actual 1
TestPostCASSave_SimulatedWriterSurvives/failExistingPlanStep     expected "expired" actual "failed"
TestPostCASSave_SimulatedWriterSurvives/clearApprovalToken       expected "expired" actual "canceled"
# unit, before: every new/updated guard test FAILs (unexpected SavePurchaseExecution call, or the run continued past the lost stamp),
#   except TestStampClaimedAudit_StoreErrorStaysBestEffort, which pins unchanged behavior and passes both ways
# after: all PASS

Commands

go build ./...
go vet ./internal/purchase/ ./internal/api/ && go vet -tags integration ./internal/purchase/ ./internal/api/
go test ./internal/purchase/ ./internal/api/ ./internal/mocks/ -count=1
go test -tags integration ./internal/purchase/ -count=1           # full package
go test -tags integration ./internal/api/ -count=1 -run '<touched-area tests, see below>'
go test -tags integration -race ./internal/purchase/ ./internal/api/ -run 'TestPostCASSave_|TestApproveWithDelay_Postgres_' -count=3
golangci-lint v2.10.1 run --build-tags integration ./internal/purchase/... ./internal/api/...   # 0 issues (also without tags)
gocyclo -over 10 -ignore "_test\.go" internal/                                                   # silent
scripts/gosec-hook.sh (gosec v2.28.0) on the changed packages                                    # clean
git-secrets --register-aws --scan on the changed files in a fresh repo with an isolated HOME; scripts/test-git-secrets-allowlist.sh 42/0

Out of scope (noted, not changed)

• finalizePurchaseStatus (handler_purchases.go) writes a just-inserted pending row to failed when the approval email fails. It follows an insert, not a status CAS, so it is not in this issue's class. Its narrow window (an approve landing while the email is being sent) could be closed the same way in a follow-up, by guarding on pending.
• The remaining SavePurchaseExecution(Tx) callers are inserts of brand-new rows: plan step creation, retry successors, submit, plan-step attach/fail, the target-met row, and the per-account fan-out rows, which only their own goroutine writes. None of them follows a CAS.
• If the scheduled stamp fails for a non-guard reason, the row is left scheduled with no scheduled_execution_at, which the scheduler never fires. This is pre-existing and unchanged here.
• No real-cloud coverage. The provider is a counting fake, so all evidence is unit, mock or local-Postgres fixture evidence.

Closes #835

Maintainers: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/m, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• 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 1faae48

Reviewed head 1faae48 (base 8caed39). git merge-tree --write-tree origin/main(b6a0649a) 1faae48d is clean (tree ee83fd89). All tests below ran on that merged tree.

Findings (none blocking):

• minor (pre-existing, disclosed in the PR body) internal/api/handler_purchases.go finalizePurchaseStatus (~:3094-3105) - an unguarded full-row save writes "failed" over a just-inserted "pending" row when the approval email fails. A session approve that claims the row while the email is in flight would be overwritten to failed while the purchase runs. The executor's terminal save is guarded on running and would then log AUDIT LOSS, which leaves a failed row for a bought commitment, and that row is a Retry candidate. The window is narrow and it follows an insert, not a CAS, so it sits outside fix(purchase): convert remaining post-CAS full-row saves to status-guarded saves #835's literal scope. File the follow-up the body proposes (guard on "pending").
• nit: when the scheduled stamp fails for a non-guard reason, the row is left "scheduled" with no scheduled_execution_at (pre-existing, disclosed).
• process nit: one commit with 610 changed lines (cap 400).

Site audit: every remaining SavePurchaseExecution(Tx) caller on the merged tree was checked: execution.go:264/334/364 (per-account rows, first written at completion), plan_step_attach.go:306/335, handler_plans.go:746, handler_purchases.go:2578/3249 (inserts), store_postgres_ramp_target_met.go:60 (insert) and handler_purchases.go:3101 (above). None follows a status CAS, which matches the body. The six converted sites each guard on the status their own CAS or read established. rotateApprovalToken runs only after a successful terminal save, so exec.Status equals the stored status. stampClaimedAudit fails closed only on ErrExecutionNotInExpectedStatus or ErrNotFound (SaveExecutionIfStatusTx returns exactly those). executeFailureError maps nil row plus either error to 409. fireOneDue reports a race loss. A lost guard in failExistingPlanStep rolls back the suppression delete in the same tx. SaveExecutionIfStatusTx locks the row FOR UPDATE, so the check and the write are atomic.

Attacked (mutations in a scratch copy, each restored, the tree diffed clean):

• schedule stamp unguarded -> killed (unit TestHandler_scheduleApprovedExecution_HappyPath; integration TestApproveWithDelay_Postgres_RevokeBeforeStampStaysCanceled)
• stampClaimedAudit returns nil on a lost guard -> killed (TestApprove_StampFindsClaimLostBuysNothing)
• rotateApprovalToken unguarded -> killed (TestMintRevocationToken_PersistsFreshToken)
• failExistingPlanStep unguarded -> killed (TestFailExistingPlanStep_SaveIsGuardedOnFailed)
• safeFail unguarded -> killed (integration TestPostCASSave_ConcurrentStatusChangeSurvives/safeFail_stamp_after_a_late_recovery)

Tests (merged tree, AWS config /dev/null): go build ok. go vet -tags integration on purchase, api and config: ok. Unit tests for purchase, api, mocks and config: 5621 passed. go test -tags integration ./internal/purchase/ ./internal/config/: ok. The touched api integration tests: ok. -race -count=2 on TestPostCASSave_ and TestApproveWithDelay_Postgres_: ok, no races. All ran under heavy.lock.

[cristim]

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

Findings (none blocking):

• [non-blocking, pre-existing, disclosed] internal/api/handler_purchases.go:1269 - If the guarded stamp fails for a reason other than the guard (for example a transient DB error), the row stays scheduled with no scheduled_execution_at, and the scheduler never fires it. Worth a follow-up issue.
• [non-blocking, out of scope per the body] handler_purchases.go:3015 - finalizePurchaseStatus still does an unguarded full-row write (pending -> failed) after an insert.
  • It does not follow a CAS, so it is outside fix(purchase): convert remaining post-CAS full-row saves to status-guarded saves #835's class.
  • It has the same race shape against an approve that lands while the email is being sent. Worth a follow-up.
• Remaining callers: every other non-test SavePurchaseExecution(Tx) caller is one of three things, and none follows a status CAS:
  • an insert;
  • a fan-out row that only its own goroutine writes;
  • the out-of-scope site above.
  • Sites checked: execution.go:264/331/361, plan_step_attach.go:306/335, handler_plans.go:732, and handler_purchases.go:2492/3015/3163.

Against #835:

• All six post-CAS saves now go through SaveExecutionIfStatus(Tx), which does FOR UPDATE, a status check and the write in one transaction (store_postgres_execution_guarded.go:54-63).
• The issue's "Also" item is decided as fail-closed on a lost claim (claim.go:81-92).

Attacked:

• Guard values:
  • rotateApprovalToken and recordRampAdvanceRefusal guard on the terminal in-memory status. They run only after the terminal or RecoverReaped save landed (manager.go:249-288, approvals.go:452-461).
  • safeFail and failExistingPlanStep guard on failed, the status their own CAS set.
  • clearApprovalToken guards on canceled, which runs only after CancelExecutionAtomic won.
• Callers of a nil row with ErrExecutionNotInExpectedStatus:
  • executeFailureError (handler_purchases.go:393) maps it to 409.
  • The SQS/token path propagates it like an existing lost CAS.
  • fireOneDue reports a race loss.
• Merge: merge-tree with origin/main b6a0649 is clean. Main's changes to manager.go (a deferred notifyExecutionFailure) and handler_purchases.go do not touch the changed lines. The new early return happens before executeAndFinalize, so no failure notification is skipped.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 11 were caught:
  • M1, scheduled stamp unguarded: TestHandler_scheduleApprovedExecution_HappyPath FAILS.
  • M1i, guard widened to accept canceled: the Postgres revoke test FAILS.
  • M2, ErrNotFound not counted as a lost claim: TestStampClaimedAudit_MissingRowIsClaimLost FAILS.
  • M3, fireOneDue ignores a lost stamp: TestFireOneDue_StampFindsClaimLostBuysNothing FAILS.
  • M4, approve ignores a lost stamp: TestApprove_StampFindsClaimLostBuysNothing FAILS.
  • M5, rotate guard widened to revocation_requested: the integration rotation subtest FAILS.
  • M5u, rotate unguarded: TestMintRevocationToken_PersistsFreshToken FAILS.
  • M6, safeFail guard widened to completed: the integration safeFail subtest FAILS.
  • M7, ramp note unguarded: TestRecordRampAdvanceRefusal_SaveIsGuardedOnTerminalStatus FAILS.
  • M8, clearApprovalToken unguarded: TestClearApprovalToken_PersistsEmpty FAILS.
  • M9, failExistingPlanStep unguarded: TestFailExistingPlanStep_SaveIsGuardedOnFailed FAILS.
• Duplication:
  • stampClaimedAudit replaces two copies of the stamp logic, and both callers are real.
  • The mock falls back to the plain save, so the new tests assert the guarded calls explicitly.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: OK.
  • go vet on purchase, api and mocks, and with -tags integration on purchase and api: OK.
  • gofmt -l on the 12 changed files: clean.
  • go test -count=1 on purchase, api and mocks: ok.
  • go test -race -tags integration ./internal/purchase/ ./internal/api/ -run 'TestPostCASSave_|TestApproveWithDelay_Postgres_' (testcontainers Postgres): PASS.
  • go test -tags integration ./internal/purchase/ (full package): ok.
• Merged tree: build, vet, unit tests and the targeted integration tests: ok.
• Fail-before, with the new tests on base plus a shim for the pre-fix helper:
  • Every integration test FAILS before the fix.
  • Revoke ends scheduled with no 409. safeFail ends completed. Rotation and the ramp note end revocation_requested. Claim-lost makes 1 provider call. The simulated writers end expired.
  • The unit tests that were observed failing include TestClearApprovalToken_PersistsEmpty and TestHandler_scheduleApprovedExecution_HappyPath.
• No migrations, frontend or terraform.

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

[IAmJSD]

Merged origin/main (a14dc8f2) into the branch to resolve the conflicts. No rebase or force-push. New head: 70e6d0947200bafff86bb91a55d7415e117ce550.

Conflicts

Before resolving, I read the main commits that touched each file: #1095 61bf6070, #1176 e43ba7c7 and #1094 31a31816 (auto-merged).

• internal/purchase/manager.go:687-688 (safeFail, conflict with fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095): kept main's strandedRecoveryError(exec), which produces the same text this branch had inline, and kept this PR's save guarded on failed.
• internal/purchase/notifications.go:340 (failExistingPlanStep, conflict with fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176):
  • Kept main's single-transaction failPlanStepTx: the pinned CAS, the stamp and the suppression release commit together.
  • Changed only its save from SavePurchaseExecutionTx to SaveExecutionIfStatusTx(..., failed). The CAS's row lock already keeps other writers out until commit, so the guard is defense in depth there. A comment says so.
• internal/purchase/post_cas_guarded_save_integration_test.go:179: removed the failExistingPlanStep subtest of TestPostCASSave_SimulatedWriterSurvives. On the merged code, its replayed writer can no longer land between the CAS and the save. fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176's plan_step_fail_atomic_integration_test.go covers this site's concurrent writers, and all of those tests pass with the guarded save. The clearApprovalToken subtest is unchanged.
• PR body: updated to match (merge section instead of the old fix(purchase): re-drive the claimed row, not the pre-claim snapshot #1095 note, the failExistingPlanStep table row, and the integration test list). The fail-before evidence is now marked as run at 1faae48d.

Review findings (both reviews APPROVE on 1faae48d, nothing blocking)

• finalizePurchaseStatus unguarded pending -> failed save: left alone. It is out of fix(purchase): convert remaining post-CAS full-row saves to status-guarded saves #835's scope because it follows an insert, not a CAS. It needs a follow-up issue guarding on pending, as the body proposes.
• Scheduled stamp that fails for a non-guard reason leaves scheduled with no scheduled_execution_at: left alone. Pre-existing and disclosed; also a follow-up issue.
• Process nit, commit size (610 lines): not addressed. Splitting it would mean rewriting pushed history.

Verification on the merged tree (AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, one package at a time)

• go build ./...: ok. go vet on purchase, api and mocks: ok. go vet -tags integration on purchase and api: ok.
• go test -count=1 on ./internal/purchase/, ./internal/api/ and ./internal/mocks/: ok.
• Integration tests (-tags integration, testcontainers Postgres) on ./internal/purchase/ and ./internal/api/:
  • On this loaded VM, neither package finished inside a single 40m timeout, so I ran the tests that hadn't run as a second -run pass of the same package. Every top-level test passed or skipped in at least one pass: purchase 409/409, api 2003/2003.
  • That includes TestPostCASSave_*, TestApproveWithDelay_Postgres_RevokeBeforeStampStaysCanceled and fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176's TestFailExistingPlanStep_*.
  • Seven tests failed at container startup (testcontainers reaper "removing" or a Postgres readiness timeout). Each passed when re-run.
  • In an earlier run that used the default 10m timeout, TestLadderReconcile_TailLockBlocksRealRetryLink and TestMarketplaceListCancelFailureRecordsEmptyAWSStateAsPending also failed. That run did not capture the cause, and both passed when re-run.
• Pre-existing on main, not from this PR: run alone, TestHandler_HandleRequest_GetConfig and TestRouter_listAccountsHandler make the AWS SDK fall back to IMDS. The internal/api tests call real AWS Cost Explorer and probe IMDS #855 no-network guard blocks these calls, but it reports "attempted 24 outbound network call(s)" and fails the package. The same happens on an unmodified git archive of origin/main a14dc8f2. The full non-integration run of ./internal/api/ did not report it.
• Mutation probe: reverting notifications.go:340 to the unguarded save makes TestFailExistingPlanStep_SaveIsGuardedOnFailed fail. The file was restored and diffed clean.
• golangci-lint v2.10.1 on ./internal/purchase/... and ./internal/api/..., with and without --build-tags integration: 0 issues.
• gocyclo -over 10 on the changed non-test files: silent.
• git-secrets --register-aws --scan on the changed files, added to a fresh repo with an isolated HOME: clean. A planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

No frontend, migration or Terraform changes. The PR stays a draft.