[IAmJSD]

Follow-up to #1516 for the concurrency gap that PR left to the owner.

Builds on #1516 (merged). #1516 and this PR both change FlipPurchaseRevocationInFlight and flipRevocationInFlight. Main was merged into this branch after #1516's squash merge, so the PR diff is just this change (commit a81a3c35). Follow-ups: #1522 (an in-progress answer for the losing request) and #1523 (the sweep marks rows revoked without a Return).

Closes #1518

Root cause

FlipPurchaseRevocationInFlight was UPDATE … SET revocation_in_flight = true WHERE purchase_id = $1 [AND revoked_at IS NULL]. A row that was already in flight could be flipped again, and the flip reported success. So two first-time revokes that both loaded the row unrevoked and unflagged could both pass the flip and both post an Azure Return. Azure refused the second one, and that request's handleAzureReturnError called ClearRevocationInFlight while the first request's refund was still in flight. If the first request's MarkPurchaseRevoked then failed after all its retries, the row was left neither revoked nor flagged. The finalize_revocations sweep never picked it up, even though that user had been told RECONCILE_PENDING.

Fix

• internal/config/store_postgres.go: the flip is now a claim. The UPDATE also requires revocation_in_flight = false. When nothing matches, one follow-up count separates three cases. A missing row keeps the not found error. A revoked row returns ErrPurchaseAlreadyRevoked, as in fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516. An unrevoked row that another request has claimed returns the new config.ErrRevocationInFlight. The row lock serializes concurrent flips, and under READ COMMITTED a waiting UPDATE re-checks both predicates against the committed row, so exactly one flip of a free row succeeds. If the holder clears the flag between the loser's UPDATE and its count, the loser still gets in-flight. It never posts a Return it didn't claim.

• internal/config/errors.go, internal/config/interfaces.go: the new sentinel and the updated contracts for the flip and for ClearRevocationInFlight (only the claim holder calls it).

• internal/api/handler_purchases_revoke.go: flipRevocationInFlight becomes claimRevocationInFlight:

  • claimed: post the Return, as before;
  • ErrRevocationInFlight: no Return. Answer RECONCILE_PENDING, the same body the handler's up-front record.RevocationInFlight check already returns (now shared through revocationInFlightResult());
  • ErrPurchaseAlreadyRevoked: unchanged from fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 (already_revoked, no Return);
  • any other flip error: no Return, 500. Before this, the handler logged the error and posted the Return anyway. See "Judgment call" below.

  So only the claim holder ever reaches the Return. The ClearRevocationInFlight in handleAzureReturnError therefore only ever clears the caller's own flag, which is the issue's second criterion. ClearRevocationInFlight itself is unchanged. Without a migration there is no owner token to key it on, and the handler now guarantees the precondition.

• internal/api/openapi.yaml: the descriptions of the revoke endpoint and of RevokePurchaseResponse now say RECONCILE_PENDING also covers a return that a concurrent revoke of the same purchase is posting, and that only one revoke at a time posts the return. The status codes and response shapes don't change.

No migration.

Judgment call: a failed flip no longer posts the Return

Before, the flip was best-effort ("a safety net, not a hard precondition"), and #1516 kept that. Now the flip is what keeps two Returns apart, and a request that posts without it can still race the holder. There's a second risk too. A flip that failed on a DB error is often followed by a MarkPurchaseRevoked that also fails, and a refund then goes out with no flag pointing the sweep at it, which is exactly the state this issue is about. Failing closed only removes Returns: it never posts one that main wouldn't post. The cost is that a revoke during a DB blip gets a 500 instead of going through. No Return has been posted at that point, so a retry is safe and the 7-day window leaves room for it. If the owner prefers best-effort, the alternative is to keep posting on a flip error but skip ClearRevocationInFlight for a request that didn't claim the row. That still allows two Returns in that narrow case.

Verification

Local Linux VM, Go 1.26.9, every command run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. "Integration" means real PostgreSQL in Docker (testcontainers, the packages' shared containers). The handler tests use stub Azure CalculateRefund/Return clients injected through azureRevokeFactory. Nothing calls a cloud API.

go build ./... ; go vet ./... ; go vet -tags integration ./...                 # clean
go test -count=1 ./internal/config/ ; go test -count=1 ./internal/api/ ; go test -count=1 ./internal/mocks/   # ok
golangci-lint run --allow-parallel-runners [--build-tags integration] ./internal/config/ ./internal/api/       # 0 issues (both)
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases_revoke.go internal/config/{store_postgres,errors,interfaces}.go   # silent
go test [-race] -tags integration -count=1 -timeout 20m -run 'TestRevokeAzure_ConcurrentFirstRevokes_OneReturn' ./internal/api/
go test [-race] -tags integration -count=1 -timeout 20m -run 'TestFlipPurchaseRevocationInFlight|TestMarkPurchaseRevoked_ClearsRevocationInFlight' ./internal/config/
go test -count=1 -run 'TestCallAzureReturn_FlipFindsRowRevoked' -v ./internal/api/

New and changed tests:

• TestRevokeAzure_ConcurrentFirstRevokes_OneReturn (integration, internal/api, 5 rounds). Two loadAndRevokePurchaseHistory calls on one Azure row in real Postgres. A CalculateRefund barrier holds both until each has loaded the row and got its quote, so they reach the flip together. The stub Return succeeds once, but only after the other request has finished. Any later Return gets a 400, like Azure. The test asserts exactly one Return. One request answers revoked and the other RECONCILE_PENDING. The flag is still set at the moment the other request has finished while the winner's Return is in flight (criterion 2). Finally the row is revoked with the flag clear.
• TestFlipPurchaseRevocationInFlight_ExclusiveClaim (integration, internal/config). 20 rounds of 6 concurrent flips of a fresh row: exactly one succeeds and the rest get ErrRevocationInFlight. A second subtest checks that after the holder's clear, the row can be claimed again.
• fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516's TestFlipPurchaseRevocationInFlight_CompareAndSet is updated for the new contract. A second flip of an in-flight row is now ErrRevocationInFlight, not a no-op. In the stale-flips-vs-mark race, every flip now reports either in-flight or already-revoked; none succeeds.
• TestCallAzureReturn_FlipFindsRowRevoked (unit, mock store). New in flight subtest: no Return, no clear, no mark, and RECONCILE_PENDING. fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516's other flip error: still returns and marks becomes other flip error: no Return, no clear, 500.

Fail-before evidence. I swapped in this branch's merge-base code for store_postgres.go and handler_purchases_revoke.go (the #1516 versions), kept the new tests, ran them, then restored:

Test	Before (#1516's code)	After
TestRevokeAzure_ConcurrentFirstRevokes_OneReturn (integration)	FAIL in all 5 rounds: 2 Returns posted; the losing request got Azure's 400; the flag was cleared while the winner's Return was in flight	PASS (also with -race)
TestFlipPurchaseRevocationInFlight_ExclusiveClaim/concurrent_first_flips… (integration)	FAIL round 0: "exactly one concurrent flip may claim the row"	PASS
…/the_holder's_clear_releases_the_claim (integration)	FAIL: the second flip succeeded	PASS
TestFlipPurchaseRevocationInFlight_CompareAndSet/an_unrevoked_row_flips_once;…, …/stale_flips_racing_a_mark (integration)	FAIL	PASS
…/a_revoked_row_is_refused…, …/a_missing_row_is_still_not_found (integration, preservation)	PASS	PASS
TestCallAzureReturn_FlipFindsRowRevoked/other_flip_error:_no_Return,_no_clear,_500 (unit)	FAIL	PASS

Mutation probes on this branch's handler (each reverted afterwards):

• The loser proceeds on ErrRevocationInFlight instead of answering RECONCILE_PENDING: the unit in flight subtest fails ("the request that loses the claim must not post a Return"), and the integration race fails all 5 rounds (2 Returns, the flag cleared during the winner's Return).
• A generic flip error goes back to log-and-continue: the unit other flip error subtest fails.

I didn't run the other integration tests in the touched packages (whole packages, -race ./...) or git-secrets. Per the queue process, those run on the combined tree.

Combined-tree verification at local queue commit ccab8379 = origin/main ec78d85c + fix/issue-1488 (#1509), fix/issue-316-next (#1516), fix/issue-1517 and fix/issue-1518 merged in that order. Passed: go build ./...; go vet with and without -tags integration; go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10; go test -race -tags integration on internal/api, internal/commitmentopts, internal/config and internal/database/postgres/migrations; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Merge order: #1516 has merged (squash). This branch then conflicted with main in the six files both PRs touch, so main was merged in at 33ab2725 and resolved to main plus this PR's own commit a81a3c35. The resulting tree is identical to cherry-picking a81a3c35 onto origin/main 2faf67fc, and the PR diff against main is now just that change.

Out of scope (noted, not changed)

• RECONCILE_PENDING wording. The body says "Refund already issued … Do not retry." The loser now gets it while the holder's Return may still be in progress, and that Return could still fail (the holder then clears the flag and the row becomes revocable again). On main, the up-front record.RevocationInFlight check already answers the same thing in that same window, so this PR stays consistent with it rather than adding a new response. A more precise "revocation in progress" answer would be an API change for the owner.
• The sweep can mark a revocation that is still in progress. finalize_revocations calls MarkPurchaseRevoked on every flagged, unrevoked row. It can't tell "Return succeeded, mark failed" from "Return in progress" or "the process died between the flip and the Return". This is pre-existing and unchanged; this PR doesn't widen that window.
• A flip error after commit. If a flip commits but its result is lost (the connection drops after commit), the handler now returns 500 without posting the Return, and the flag stays set. That is the same state as a crash between the flip and the Return, and it already happens on main.
• Keying on purchase_id alone. The flip, the clear and MarkPurchaseRevoked still key on purchase_id alone, not on the unique key (provider, account_id, purchase_id). This is pre-existing (also noted on fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516).

Frontend follow-up for the UI owner

None required for correctness. RECONCILE_PENDING is an existing, documented 2xx response. Today frontend/src/history.ts doesn't read its code and shows the generic "Purchase revocation submitted" success toast for it, and that is a fine answer for the losing request too. The flip-error path returns a plain 500, which the existing "Failed to revoke: …" toast shows. An optional polish for the UI owner: read code === "RECONCILE_PENDING" and show a "refund in progress, don't retry" message, as the handler's revokeReconcilePendingResult comment intends.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, type/bug. The issue has no severity/*, urgency/*, impact/* or effort/* labels and isn't triaged.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE a81a3c35f906b5e81c671381b1d6c9fa86075103

Blocking:

• none in the code. This head can't merge as it stands, though: GitHub reports CONFLICTING. fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 was squash-merged (2faf67fc), so merging main into this branch conflicts in all six files the two PRs share. The branch needs git merge origin/main, taking the branch side in those hunks, and the new head then needs re-review. I checked that this commit (a81a3c35) cherry-picks cleanly onto origin/main, and I ran everything below on that tree, which is exactly what would land.

Non-blocking:

• internal/api/handler_purchases_revoke.go revocationInFlightResult: the request that loses the claim now gets AzureReturned: true / "Refund already issued ... Do not retry" while the winner's Return is still in flight and may still fail. If it fails, the winner clears the flag and nothing was refunded, but the loser was told not to retry. On main the same answer already comes from the up-front record.RevocationInFlight check in the same window, so this PR is consistent with it. Before this PR, though, the loser would have seen Azure's 400 instead. A distinct "revocation in progress" answer is an API decision for the owner, as the body says.
• The fail-closed change (a generic flip error now returns 500 with no Return; before, it logged and posted anyway) is the right call to me. No Return has been posted by then, so a retry is safe. It is a behavior change on a money path, though, and the owner should know about it (the body flags it).
• Pre-existing and correctly listed as out of scope: finalize_revocations marks any flagged, unrevoked row revoked. With a strict claim, a holder that crashes between the flip and the Return leaves the row flagged, and the sweep will mark it revoked even though no refund was ever posted. That was already true on main, and this PR doesn't widen it, but it is worth an issue.

Attacked:

• Claim correctness: UPDATE ... WHERE purchase_id=$1 AND revoked_at IS NULL AND revocation_in_flight=false. The column is NOT NULL DEFAULT false (000072), so there's no NULL trap. Under READ COMMITTED a waiting UPDATE re-evaluates both predicates on the committed row, so exactly one flip wins. The follow-up count classifies the losers (missing, revoked, or in flight), and a cleared-in-between row is still reported as in flight, so the loser never posts. FlipPurchaseRevocationInFlight has a single caller. claimRevocationInFlight's (nil, nil) is untyped, so the stop != nil check is sound. Only the claim holder reaches handleAzureReturnError, so its ClearRevocationInFlight only clears its own flag (criterion 2).
• Fail-before: with only internal/config/store_postgres.go reverted to main (fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516's idempotent flip), TestRevokeAzure_ConcurrentFirstRevokes_OneReturn failed all 5 rounds (two Returns, and the flag was cleared during the winner's Return). TestFlipPurchaseRevocationInFlight_ExclusiveClaim (both subtests) and the two updated _CompareAndSet subtests also failed. All pass after restoring.
• Ran on origin/main plus this commit: go build ./...; go vet with and without -tags integration on internal/api, internal/config and internal/mocks; golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the 4 changed non-test files (silent); go test -race -short -count=1 on ./internal/api/, ./internal/config/ and ./internal/mocks/ ok; and go test -race -tags integration -count=1 ./internal/api/ ./internal/config/ (whole packages) ok.

Body-stale:

• "Stacked on fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 ... Until fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 merges, this diff includes its changes too. Please merge fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 first." fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 has merged.
• "After that, this PR's diff is only its own commit." This is false because of the squash merge. The branch still carries fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516's original commits, and it conflicts with main until main is merged in.

_{Reviewer: rF9. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE a81a3c35f906b5e81c671381b1d6c9fa86075103

Blocking:

• none in the code. Merge state: GitHub reports this PR as CONFLICTING. fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 was squash-merged (2faf67f), so this branch's copy of fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516's commits (85d6884 via 73d0e71) conflicts with main in six files. The fix's own commit a81a3c3 applies cleanly onto main by cherry-pick. On that tree (main + fix(config): resolve arming grants before the global-config transaction #1519 + a81a3c3), go build, go vet (with and without -tags integration, and e2e) and the unit and targeted integration tests all pass. The branch needs git merge origin/main, resolving each conflict to this branch's side for the fix(revoke): let only one request flip an Azure purchase to in-flight, so concurrent first revokes can't post two Returns #1518 hunks. The new head needs its own review, because this approval is pinned to a81a3c3.

Non-blocking:

• internal/api/handler_purchases_revoke.go:847 (ErrRevocationInFlight → RECONCILE_PENDING): the request that loses gets "Refund already issued ... Do not retry" with azure_returned: true while the holder's Return is still in progress. If the holder's Return then fails, it clears the flag and the row is revocable again, but the loser was told not to retry. The PR body names this as out of scope, and the up-front record.RevocationInFlight check on main already has the same window. A distinct "revocation in progress" code is an API decision for the owner.
• internal/config/store_postgres.go:2993: the flip, the clear and the follow-up count still key on purchase_id alone. If two purchase_history rows ever share a purchase_id across provider or account, count(*) FILTER (...) can call the result "in flight" when one row is revoked and the other is free. This is pre-existing and noted in the body.
• (Raised in the earlier rF9 review; I agree.) Now that the claim is strict, a holder that crashes between the flip and the Return leaves the row flagged. finalize_revocations will then mark it revoked although no refund was posted. This is pre-existing (main has the same exposure) and worth its own issue.
• Failing closed on a generic flip error (500, no Return) is the right call for a money path. It never posts a Return that main wouldn't post.

Attacked:

• Reviewed only the PR's own commit (git diff 73d0e714 a81a3c35), since fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 is already on main.
• FlipPurchaseRevocationInFlight has one caller (claimRevocationInFlight). Nothing else relied on the old idempotent "already true is a no-op" behavior.
• Only the claim holder reaches returnClient.Post, so the unconditional ClearRevocationInFlight in handleAzureReturnError only clears the caller's own flag. That meets the issue's second criterion.
• The UPDATE predicate re-check under READ COMMITTED gives exactly one winner. If the holder clears the flag between the loser's UPDATE and its count, the loser still reports "in flight" (unrevoked > 0) and never posts.
• claimRevocationInFlight returns any. The win case returns an untyped nil, nil, so the stop != nil check can't be fooled by a typed nil.
• Ran at a81a3c3: go build ./..., go vet with and without -tags integration (clean); go test -race -short -count=1 ./internal/api/ ./internal/config/ ./internal/mocks/ (ok); go test -race -tags integration -run 'Revoke|Revocation' ./internal/api/ (ok); go test -race -tags integration ./internal/config/ (whole package, ok).
• Fail-before: deleted only the AND revocation_in_flight = false predicate. TestRevokeAzure_ConcurrentFirstRevokes_OneReturn then fails all 5 rounds (2 Returns, flag not held), and TestFlipPurchaseRevocationInFlight_ExclusiveClaim plus the two updated _CompareAndSet subtests fail. Restored the predicate afterwards.

Body-stale:

• "Until fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 merges, this diff includes its changes too ... After that, this PR's diff is only its own commit": fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 is merged, but as a squash. The branch still carries the original commits, now conflicts with main, and its diff against main won't shrink to the own commit until main is merged in.
• "Combined-tree verification at ... origin/main ec78d85c": main is now 2faf67f.

_{Reviewer: rF10. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 33ab2725d7f0da5992aeded033325c3cd217cbb1

Blocking:

• none

Non-blocking:

• internal/config/store_postgres.go:3082-3095 (the follow-up count): if the holder's Return fails and it clears the flag between the loser's UPDATE and its SELECT, the loser reports ErrRevocationInFlight. It then answers RECONCILE_PENDING ("Refund already issued ... Do not retry") for a row that is free again with no refund issued. Failing closed is the right call here (never post an unclaimed Return), and the window is tiny. But the user-facing text is wrong in that case, and a user who obeys "do not retry" could let the 7-day window lapse. The PR body already flags the wording as an owner decision. A distinct "revocation in progress, check back" response would close it later.
• Same text, wider window: the loser now gets RECONCILE_PENDING with azure_returned: true while the holder's Return is still in progress and may fail. That matches the existing up-front record.RevocationInFlight answer, so it adds no new inconsistency, only more traffic through it.
• Pre-existing, unchanged: the flip, clear and mark key on purchase_id alone, not on (provider, account_id, purchase_id) (migration 000108). With two rows sharing a purchase_id, the claim is per-UPDATE, not per-row. Azure ARM reservation paths make a collision unlikely. Already noted in the body.
• (From the earlier rF9/rF10 reviews; I agree.) Pre-existing and listed as out of scope: a holder that crashes between the flip and the Return leaves the row flagged, and finalize_revocations will mark it revoked although no refund was posted. The stricter claim doesn't widen this, but it deserves its own issue.

Attacked:

• Head: still 33ab272. A cherry-pick of a81a3c3 onto 2faf67f gives a tree identical to 33ab272 (empty git diff). Main has since moved to eb313af (fix(config): resolve arming grants before the global-config transaction #1519), which touches none of this PR's files. Reviewed git diff origin/main...33ab2725.
• Logic: the UPDATE now requires revocation_in_flight = false, a NOT NULL DEFAULT false column (000072), so no NULL trap. Under READ COMMITTED a blocked UPDATE re-evaluates both predicates on the committed row, so exactly one flip wins. The only callers of Flip/Clear are in handler_purchases_revoke.go, so the sweep is unaffected. Clear is only reached from handleAzureReturnError, after a successful claim. No typed-nil issue in the (any, error) return. A generic flip error now fails closed (500, no Return), which can only remove Returns. Issue fix(revoke): let only one request flip an Azure purchase to in-flight, so concurrent first revokes can't post two Returns #1518's criteria are met: the real-PG two-request race test shows one Return and the loser answering RECONCILE_PENDING, and the flag holds during the winner's Return. "Closes fix(revoke): let only one request flip an Azure purchase to in-flight, so concurrent first revokes can't post two Returns #1518" is appropriate.
• Ran: go build ./..., go vet ./..., go vet -tags integration ./.... golangci-lint v2.10.1 on internal/config and internal/api, with and without --build-tags integration: 0 issues. gocyclo -over 10 on the changed non-test files: silent. go test -race -short -count=1 on internal/config, internal/api and internal/mocks: ok. go test -race -tags integration -count=1 on internal/config and internal/api: ok.
• Fail-before: in a second worktree I restored origin/main's store_postgres.go and handler_purchases_revoke.go and kept the new tests. TestRevokeAzure_ConcurrentFirstRevokes_OneReturn failed every round: 2 Returns, the flag cleared during the winner's Return, and the loser got an error. TestFlipPurchaseRevocationInFlight_ExclusiveClaim (both subtests) and the two updated CompareAndSet subtests failed. The unit other flip error: no Return, no clear, 500 subtest failed. All pass on the head.

Body-stale:

• "Stacked on fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 ... Until fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 merges, this diff includes its changes too. Please merge fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 first." fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 has merged, and the body's own "Merge order" paragraph says so. The diff against main is now only a81a3c3.

_{Reviewer: rF11. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 33ab2725d7f0da5992aeded033325c3cd217cbb1

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:814-820 (revocationInFlightResult): the losing request now gets "Refund already issued ... Do not retry." while the holder's Return is still in progress. If that Return then fails (400 or window edge), the holder clears the flag and the row is revocable again, but the loser has been told a refund went out and not to retry, inside a 7-day window. The up-front record.RevocationInFlight check on main already answers this in the same window, so the PR is consistent with existing behavior, and the body flags it as out of scope. The openapi text added here ("must not be retried") carries the same imprecision. An owner/product call on a distinct "revocation in progress" answer is worth a follow-up issue.
• internal/api/handler_purchases_revoke.go:853 (fail-closed default branch): if a flip commits but the error is reported (connection lost after commit), the handler now returns 500 with no Return and the flag stays set. The next finalize_revocations tick then stamps the row revoked with no refund issued (internal/purchase/finalize_revocations.go marks every flagged row blindly). On main, that same case would have gone on to post the Return. This is the same end state as a crash or ctx cancel between the flip and the Return, which already exists, and it is very narrow, so I agree with failing closed. It is worth knowing that the sweep, not this PR, is the weak point.
• internal/config/store_postgres.go:3081-3095: the classification count keys on purchase_id alone. If two rows share a purchase_id (different account_id), a revoked row next to an unrevoked, flagged one reports ErrRevocationInFlight rather than already-revoked. This is the existing purchase_id-only keying the body already notes, and it isn't made worse in any way that matters.
• PR body: see Body-stale.

Attacked:

• Checked the head is still 33ab272. git diff 2faf67fc 33ab2725 | git patch-id equals git diff a81a3c35~1 a81a3c35 | git patch-id (a8194f19), so the PR is exactly a81a3c3 on main. origin/main has since moved to eb313af (fix(config): resolve arming grants before the global-config transaction #1519, unrelated files), and git merge-tree against it is clean. Flip has a single caller (claimRevocationInFlight). The column is NOT NULL DEFAULT false (000072), so = false can't strand a NULL row. Under READ COMMITTED the re-checked UPDATE gives one winner. A count after a holder's clear reports in-flight (fail safe), and a count after a holder's mark reports already_revoked. Only the holder reaches the Return, so ClearRevocationInFlight only clears the caller's own flag. CalculateRefund has no side effects, so a 500 before the Return is safe to retry. Both acceptance criteria are met; "Closes fix(revoke): let only one request flip an Azure purchase to in-flight, so concurrent first revokes can't post two Returns #1518" is right.
• Fail-before proof in a second scratch worktree. With only store_postgres.go reverted to 2faf67f, TestFlipPurchaseRevocationInFlight_ExclusiveClaim (both subtests), _CompareAndSet/{second flip, stale flips racing a mark} and the end-to-end TestRevokeAzure_ConcurrentFirstRevokes_OneReturn FAIL (2 Returns, flag cleared mid-Return). With only the handler reverted (plus a test shim for revocationInFlightResult), the unit in flight and other flip error subtests and the end-to-end race test FAIL. All pass on the head.
• Ran on the head: go build ./..., go vet ./... with and without -tags integration, go test -race -short -count=1 on internal/{config,api,mocks,purchase}, and go test -race -tags integration -count=1 on all of ./internal/config/ (184s) and ./internal/api/ (256s), all ok. golangci-lint v2.10.1 found 0 issues with and without --build-tags integration on config and api, and gocyclo -over 10 on the changed non-test files was silent.

Body-stale:

• "Stacked on fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516. ... Until fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 merges, this diff includes its changes too. Please merge fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 first." fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 has merged, and the diff against main is now only a81a3c3. The later "Merge order" paragraph says so, but the opening paragraph contradicts it.

_{Reviewer: rF12. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 33ab2725d7f0da5992aeded033325c3cd217cbb1 merged onto main (eb313af1), together with #1520 in that order (combined tree 205ca6bb, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 33ab2725 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 33ab2725d7f0da5992aeded033325c3cd217cbb1.