[IAmJSD]

Root cause

finalize_revocations (FinalizeInFlightRevocations → GetPurchaseHistoryInFlight) marked every row with revocation_in_flight = true AND revoked_at IS NULL as revoked. The flag is set by the flip before the Azure Return, so it can't tell "Azure refunded, but MarkPurchaseRevoked failed" apart from "the request died between the flip and the Return" (or "the flip committed but its result was lost"). In the second case the sweep recorded a refund that never happened. The purchase read back as revoked, the user could no longer revoke it, and the reservation stayed active and billed.

Fix

The sweep now finalizes only rows where an accepted Return was actually recorded. A row that is only flagged is never marked revoked. Once it has been flagged for 2h, the flag is cleared so the user can retry.

• Migration 000148_purchase_history_revocation_return_posted (number reserved for this branch; main is at 000147, and no open PR uses 148). It adds two nullable columns, purchase_history.return_posted_at and revocation_in_flight_at, and a partial index for the finalize queue (WHERE return_posted_at IS NOT NULL AND revoked_at IS NULL). There is no backfill. The down drops both columns and the index.
• Handler, "Return accepted" step (internal/api/handler_purchases_revoke.go). After returnClient.Post succeeds, persistAzureRevocation now runs recordAzureReturnAndMark on each attempt. That stamps return_posted_at (new MarkRevocationReturnPosted, retried on every MarkPurchaseRevoked attempt until it lands) and then calls MarkPurchaseRevoked as before. A failed or refused Return records nothing. If every attempt fails, the error log now says whether the Return was recorded.
• Store (internal/config/store_postgres.go, new store_postgres_revocation_return.go):
  • GetPurchaseHistoryInFlight (the sweep's queue) now returns return_posted_at IS NOT NULL AND revoked_at IS NULL, and reads revocation_in_flight from the row instead of hard-coding true. The flag is no longer required. Once Azure has accepted a Return the purchase is refunded, even if something cleared the flag afterwards.
  • FlipPurchaseRevocationInFlight also sets revocation_in_flight_at = now(). The WHERE clause and its fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516/fix(revoke): make the Azure in-flight flip an exclusive claim so only one revoke posts a Return #1520 semantics are unchanged.
  • MarkRevocationReturnPosted(ctx, id): SET return_posted_at = COALESCE(return_posted_at, now()) WHERE purchase_id = $1 AND revoked_at IS NULL. The first stamp stands. A revoked row is a no-op and a missing row is not-found.
  • ReleaseStaleRevocationsInFlight(ctx, after) refuses after <= 0. It (1) stamps revocation_in_flight_at = now() on flagged, unrevoked rows with no recorded Return and no stamp, then (2) clears the flag (and the stamp) on flagged, unrevoked rows with no recorded Return whose stamp is older than after, and returns their IDs. Both steps use the database clock.
• Sweep (internal/purchase/finalize_revocations.go): it first releases stale flags (staleRevocationInFlightAfter = 2h), logging each one at error level for an operator and counting it in the new FinalizeResult.Released. It then finalizes the queue as before. The two sets are disjoint. A release failure doesn't skip the finalize pass, but it is returned (joined), so the tick still fails visibly. The task's JSON output gains a Released key; the existing keys are unchanged.
• Interface, mock (default-or-dispatch, like the flip and clear mocks) and docs updated. internal/server/handler.go logs released=.

Stale flags: clear after a timeout, or leave them for an operator? (owner decision)

I chose clear after a timeout, and log every release for an operator. Leaving the flag set forever looks safer but isn't:

• While a row stays flagged, the handler's up-front check answers every revoke attempt with RECONCILE_PENDING ("Refund already issued… Do not retry"). If no Return actually went out, the user is locked out until the 7-day free-cancel window closes, and then keeps paying for a reservation they tried to cancel.
• Clearing can't cause a wrong refund. Releasing never marks anything revoked and never posts a Return. A retry goes through the normal CalculateRefund → confirm → claim → Return flow. If Azure did accept a Return that we failed to record, Azure refuses the second one, and the user sees an error rather than a double refund. The only cost in that case is that the row reads as active when it isn't, which is what the error log line is for.
• Timeout = 2h. That is twice the longest request any deployment can run: Cloud Run's request_timeout is validated ≤ 3600s and Lambda ≤ 900s. So a revoke still between its flip and its Return is never released. The sweep runs hourly, so in practice a release lands 2–3h after the flip, which is small next to the 7-day window. Correctness doesn't depend on the exact value. A Return recorded after a release is still finalized, because the queue doesn't require the flag (the race is tested below).

If the owner prefers operator-only handling, the alternative is to drop step (2) and only log. That reintroduces the lock-out described above.

Rows already flagged when 000148 lands

Interpretation: "no Return recorded". Nothing is backfilled. Such rows are never finalized by the new sweep. A legacy row has no revocation_in_flight_at, so the sweep stamps it on first sight and releases it 2h after that. Its age is never overstated, and a request from a pre-000148 binary still running during a rolling deploy gets the full timeout too. Backfilling return_posted_at would have kept the old behavior and kept the bug for exactly the rows it hurts. Backfilling revocation_in_flight_at with the migration time would have been equivalent, except that it misses flips from old binaries during the rollout. The trade-off is the same as above. A legacy row whose Return did go through (handler mark plus sweep both failed before the deploy) ends up reading as active, and the release log line flags it for manual reconciliation.

How this combines with #1520 (merged while this was in progress; this branch merges origin/main at 95836478)

The changes are orthogonal, and the merge had no conflicts. #1520 made the flip an exclusive claim (AND revocation_in_flight = false) and made sure only the claim holder posts a Return or calls ClearRevocationInFlight. This PR only adds revocation_in_flight_at = now() to the flip's SET list and adds the Return-accepted stamp in persistAzureRevocation, which only the claim holder reaches. A release clears a claim, so the row becomes claimable again. That's intended, and given the 2h timeout the previous holder is long gone. Even if it weren't, Azure accepts only one Return per reservation. TestReleaseStaleRevocationsInFlight_RacesFlip covers both claim semantics: if the flip refuses the row, nothing is posted, and if it succeeds, the row ends up flagged with a fresh stamp.

Closes #1523

Verification

Linux VM, Go 1.26, 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 and the mock store. Nothing calls a cloud API. All results below are on the pushed head (origin/main merged in).

go build ./... ; go vet ./... ; go vet -tags integration ./...                       # clean
go test -count=1 ./internal/{config,api,purchase,server,mocks,database/postgres/migrations}/   # one at a time, all ok
golangci-lint run --allow-parallel-runners [--build-tags integration] ./internal/{config,api,purchase,server,mocks,database/postgres/migrations}/   # 0 issues (both)
gocyclo -over 10 -ignore "_test\.go" <changed non-test .go files>                    # silent
go test -race -tags integration -count=1 -timeout 20m -run 'TestGetPurchaseHistoryInFlight_RequiresRecordedReturn|TestMarkRevocationReturnPosted|TestReleaseStaleRevocationsInFlight|TestMarkPurchaseRevoked_ClearsRevocationInFlight|TestFlipPurchaseRevocationInFlight' ./internal/config/
go test -race -tags integration -count=1 -timeout 20m -run 'TestRevokeAzure_ConcurrentFirstRevokes_OneReturn|TestNullablePurchaseCostPostgres' ./internal/api/
go test -tags integration -count=1 -timeout 20m -run 'TestFinalizeInFlightRevocations_' ./internal/purchase/
go test -tags integration -count=1 -timeout 20m -run 'TestMigration_000148_RevocationReturnPosted|TestMigration_000142_ClearRevokedInFlight' ./internal/database/postgres/migrations/

New tests:

• TestFinalizeInFlightRevocations_FlagWithoutReturnIsNotRevoked (integration, internal/purchase) is acceptance criterion 1. It uses a row flipped with no Return, plus a legacy row whose flag was set by raw SQL with no stamps. After the real sweep, neither row is revoked and both are still flagged.
• TestFinalizeInFlightRevocations_RecordedReturnFinalizedStaleFlagReleased (integration, internal/purchase) is acceptance criterion 2. A row with a recorded Return is finalized as before (revoked_via=direct-api, flag cleared). A flipped row backdated past the timeout is released (flag cleared, still unrevoked). A second tick finds nothing.
• TestGetPurchaseHistoryInFlight_RequiresRecordedReturn (integration, internal/config): only rows with a recorded Return are in the queue, including one whose flag was cleared after its Return was recorded. The flag is read from the row.
• TestMarkRevocationReturnPosted (integration): missing row → not-found; the first stamp stands; a revoked row is left alone.
• TestReleaseStaleRevocationsInFlight (integration):
  • a non-positive timeout is refused;
  • only stale flags with no recorded Return are released (a fresh row, one just under the timeout, one with a recorded Return and a revoked row all stay flagged);
  • a released row can be flipped again;
  • a legacy flag (no stamp) is stamped on first sight, isn't released or finalized, and is released once backdated.
• TestReleaseStaleRevocationsInFlight_RacesReturnRecord (integration, 30 rounds, also run with -race): MarkRevocationReturnPosted runs concurrently with the release of the same stale row. In every round the recorded Return ends up in the finalize queue and nothing is revoked by the release. Both orders occurred: the release won 3/30 and 1/30 rounds in two runs. The test doesn't force a given split.
• TestReleaseStaleRevocationsInFlight_RacesFlip (integration, 30 rounds): a flip runs concurrently with the release. A flip that succeeds always leaves the row flagged with a fresh stamp.
• TestMigration_000148_RevocationReturnPosted (integration): pre-000148 rows keep their flag and revoked_at, and get no return_posted_at or revocation_in_flight_at. The index exists. The down drops both columns and leaves the flag. The up re-applies after the rollback.
• TestCallAzureReturn_RecordsReturnBeforeMark, …_ReturnErrorRecordsNothing, …_RetriesReturnRecordUntilItLands (unit, mock store): the stamp comes before the mark; a failed Return records nothing; the stamp is retried with the mark until it lands, then not again.
• TestFinalizeInFlightRevocations_ReleasesStaleFlagsWithoutMarking, …_ReleaseErrorStillFinalizes, …_ReleaseAndListErrorsJoined, TestStaleRevocationInFlightAfter_OutlastsLongestRequest (unit, mock store).
• Changed: TestNullablePurchaseCostPostgres migrates to 148 before the queue check and stamps return_posted_at on its fixture row. TestFinalizeSweep_PollsMarketplaceListings expects the new "Released":0 key.

Fail-before on unmodified origin/main (temporary worktree, with only the criterion-1 test copied in, since it uses pre-existing APIs only):

--- FAIL: TestFinalizeInFlightRevocations_FlagWithoutReturnIsNotRevoked
    Should be zero, but was 2                      -- no Return was recorded, so nothing may be finalized
    Expected nil, but got: time.Date(2026, …)      -- azure-1523-crashed-before-return: … must not read back as revoked
    Expected nil, but got: time.Date(2026, …)      -- azure-1523-legacy-flag: … must not read back as revoked

It passes on this branch. The other new tests call new store methods or the new columns, so on main they fail to compile or fail with SQLSTATE 42703. For those, the fail-before evidence is these mutation probes on this branch, each reverted afterwards:

Mutation	Fails
Queue back to revocation_in_flight = true AND revoked_at IS NULL (main's predicate)	TestGetPurchaseHistoryInFlight_RequiresRecordedReturn, TestReleaseStaleRevocationsInFlight_RacesReturnRecord, TestFinalizeInFlightRevocations_FlagWithoutReturnIsNotRevoked
Sweep skips the release	the three new sweep unit tests, TestFinalizeInFlightRevocations_RecordedReturnFinalizedStaleFlagReleased
Handler never records the Return	TestCallAzureReturn_RecordsReturnBeforeMark, …_RetriesReturnRecordUntilItLands
Handler records only on the first attempt (no retry of a failed stamp)	…_RetriesReturnRecordUntilItLands
Release ignores return_posted_at IS NULL	TestReleaseStaleRevocationsInFlight/only_stale_flags…
Release never stamps a legacy flag	TestReleaseStaleRevocationsInFlight/a_legacy_flag…
Flip doesn't stamp revocation_in_flight_at	TestReleaseStaleRevocationsInFlight/only_stale_flags…, …_RacesFlip
Release ignores the age (now() + interval)	TestReleaseStaleRevocationsInFlight (both subtests), …_RacesFlip

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

Full-suite verification at local queue commit 825f8d41 = origin/main 95836478 + fix/issue-1523. 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/config, internal/database/postgres/migrations, internal/purchase and internal/server; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Owner decision (D25 in the acting-owner report): clear a stale flag after 2h (this PR) or leave it for an operator. Main today marks such rows revoked, so either choice is an improvement; switching to operator-only means dropping the clearing step.

Out of scope (noted, not changed)

• Wording of the up-front RECONCILE_PENDING answer. A flagged row still answers "Refund already issued… Do not retry." That's now accurate only when a Return was recorded. For a row whose request died before the Return, it is wrong until the release, which is at most ~3h, whereas on main the sweep would have marked it revoked for good. A "revocation in progress, try again later" variant keyed on return_posted_at would be an API change for the owner. fix(revoke): make the Azure in-flight flip an exclusive claim so only one revoke posts a Return #1520 notes the same wording issue.
• 207 when the stamp never lands. If both MarkRevocationReturnPosted and MarkPurchaseRevoked fail on every attempt (a DB outage for the whole retry window), the user still gets RECONCILE_PENDING. The sweep won't finalize the row, and after the timeout it is released and logged. Azure refuses a retry, and an operator has to reconcile the row. The log line now says return recorded=false for this case.
• A Return whose response was lost. If Azure accepted the Return but the HTTP response was lost (the client saw an error), handleAzureReturnError clears the flag and nothing is recorded. This is pre-existing and unchanged.
• Keying on purchase_id alone. As with the flip, the clear and MarkPurchaseRevoked, the new store methods key on purchase_id alone rather than (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 and fix(revoke): make the Azure in-flight flip an exclusive claim so only one revoke posts a Return #1520).
• GetPurchaseHistoryInFlight keeps its name. The documented contract ("Return succeeded, the DB write failed") is unchanged; only the predicate now matches it. Renaming it would have meant churn in other tests.

Frontend follow-up for the UI owner

None required. No response shape changes. The task output's new Released count is backend-only.

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 7e984035cf07a8c87e95ca8e09de673c427534b0

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:998 (recordAzureReturnAndMark): the new return_posted_at stamp is written with the same request ctx and the same DB as MarkPurchaseRevoked, so it fails for the same reasons. The sec(iac): CFN templates accept an 8-char external ID where Terraform generates a UUID #290 F6 case (Azure accepted the Return, then the DB write fails because the DB is unreachable for the ~13s retry window, or because the request ctx is canceled or hits its deadline right after the Return, e.g. a Lambda near its timeout or a client disconnect) used to end with the sweep finalizing the row correctly. Now the row is released after 2h and reads as active even though Azure refunded it, while the 207 told the user the sweep "will reconcile". Nothing is double-refunded, because Azure refuses a second Return, and the release is logged at error level. But the scenario that justified the sweep now needs an operator. The PR body acknowledges this under "207 when the stamp never lands". A cheap mitigation: run the post-Return writes under context.WithoutCancel(ctx) with a bounded timeout, so a canceled ctx can't drop the stamp. The owner should weigh this trade-off along with D25.
• internal/purchase/finalize_revocations.go:35 (and the PR body): "twice the longest request any deployment can run (Cloud Run <= 3600s, Lambda <= 900s)" leaves out the Fargate and Azure Container Apps targets (terraform/modules/compute/aws/fargate, terraform/modules/compute/azure/container-apps). There, the handler goroutine isn't bounded by a platform request timeout, and http.Server's 30s WriteTimeout doesn't cancel the handler's ctx. Correctness doesn't depend on the bound: a Return recorded after a release is still finalized, and Azure refuses a second Return. Still, the comment states a guarantee the code doesn't have on those targets.
• internal/server/handler.go:372: when only the release step fails, handleFinalizeRevocations returns nil, err and discards the counts of a finalize pass that did run, so the tick's log and JSON lose found/finalized/errored. Minor.
• The 000148 down migration is lossy (documented). After a rollback, the pre-000148 sweep goes back to marking every flagged row revoked, including rows still waiting for release. That is acceptable for a rollback, but it's worth knowing.

Attacked:

• Checked that only the Azure path sets or clears revocation_in_flight (grep), so no AWS or GCP flow relied on the old queue predicate. Walked through these races against the SQL: release vs. MarkRevocationReturnPosted, release vs. a new flip, release vs. a late Return, and old binaries during a rolling deploy (an old flip leaves no stamp, so the legacy path stamps it on first sight).
• Migration: 000148 matches the reserved number (main is at 000147, and the branch is up to date with origin/main 9583647). The columns are nullable with no default, so adding them is metadata-only. The partial index is non-concurrent on purchase_history, which is fine at this size. The test DBs are per-test (requireSharedDatabase clones a template), so the global release can't flake the exact-count assertions.
• Fail-before: in a second worktree at this head, I restored main's queue predicate (revocation_in_flight = true), turned off the sweep's release call and disabled the handler's stamp. The following then failed: TestFinalizeInFlightRevocations_FlagWithoutReturnIsNotRevoked ("Should be zero, but was 2"; both rows read back revoked), ..._RecordedReturnFinalizedStaleFlagReleased, the three new sweep unit tests, TestCallAzureReturn_RecordsReturnBeforeMark and ..._RetriesReturnRecordUntilItLands. After restoring the code, the integration revoke tests pass in internal/api.
• Ran: go build ./...; go vet with and without -tags integration; go vet -tags e2e in tests/e2e; go test -race -short -count=1 on config, api, purchase, server, mocks and migrations; go test -race -tags integration -count=1 on purchase, migrations, config and server (whole packages); -tags integration -run 'Revoke|Revocation|TestNullablePurchaseCostPostgres' on api; golangci-lint v2.10.1 on the changed packages with and without the integration tag (0 issues); gocyclo -over 10 on the changed non-test files (silent).

Body-stale:

• "twice the longest request any deployment can run": only Cloud Run and Lambda are bounded. Fargate and Container Apps are deployment targets too (see the second non-blocking item).

_{Reviewer: rF13. 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 7e984035cf07a8c87e95ca8e09de673c427534b0

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:998 (recordAzureReturnAndMark): the stamp uses the request ctx. If Azure accepts the Return and the ctx is then canceled (client disconnect, Lambda/Cloud Run deadline) or the DB is down for the whole retry window, nothing is recorded. The row is released after 2h and reads as active even though it was refunded. Main's sweep would have finalized that row correctly, so this is a narrow regression, traded for fixing the much worse case of a refund that never happened. The PR body says so, and the release logs at error level. A cheap hardening would be to run MarkRevocationReturnPosted on context.WithoutCancel(ctx) with a short timeout of its own, so a cancel right after the Return can't skip the stamp.
• Rolling deploy: during the rollout, an old-binary handler whose Return succeeded but whose MarkPurchaseRevoked failed doesn't stamp return_posted_at. The new sweep releases that row instead of finalizing it. An old-binary sweep still running in the same window uses main's predicate. Both last only as long as the rollout, and the body's "legacy row" paragraph covers the outcome. Worth an operator glance at the release log lines after deploy.
• For up to about 3h, a flagged row with no Return still answers RECONCILE_PENDING ("Refund already issued... Do not retry"). The body already lists this as out of scope and as an owner wording decision. I agree it's an improvement over main, which made that answer permanent.
• ClearRevocationInFlight and MarkPurchaseRevoked leave revocation_in_flight_at behind on unflagged rows. That's harmless, because the release requires the flag and the next flip overwrites the stamp. I mention it only so nobody later reads the column as "currently in flight".

Attacked:

• Head checked against gh pr view: 7e98403, and main is still 9583647. I read the three-dot diff against the fix(revoke): finalize sweep marks a flagged row revoked even when no Azure Return was posted #1523 acceptance criteria (both are met by real-Postgres tests).
• Money paths: a row is marked revoked only when return_posted_at is set, and that is written only after returnClient.Post succeeds, by the claim holder. A failed Return records nothing. The release never revokes and never posts. A Return after a release is still finalized, because the queue doesn't require the flag. A double Return after a release is refused by Azure.
• Timeout: checked the 2h value against the Cloud Run module validation (<=3600s, terraform/modules/compute/gcp/cloud-run/variables.tf:68), the GCP default of 300s and the HTTP server's 30s WriteTimeout.
• SQL: both release UPDATEs re-check their predicate on the locked row under READ COMMITTED, and both race tests hold. The migration is nullable ADD COLUMN plus a small partial index, numbered 148 as reserved, with a reversible down. Searched terraform, frontend and CI for consumers of the sweep log line and output keys: there are none.
• Ran: go build ./..., go vet (plain, -tags integration, and e2e), golangci-lint v2.10.1 with and without the integration tag (0 issues), gocyclo -over 10 (silent), and go test -race -short -count=1 on config/api/purchase/server/mocks/migrations (all ok). Also ran go test -race -tags integration -count=1 on the whole of config, purchase, migrations and server (all ok), and on api -run 'TestRevokeAzure_|TestNullablePurchaseCostPostgres' (ok).
• Fail-before: I reverted the GetPurchaseHistoryInFlight predicate to main's revocation_in_flight = true, and TestFinalizeInFlightRevocations_FlagWithoutReturnIsNotRevoked failed ("Should be zero, but was 2"; both rows read back as revoked). I also removed the handler's MarkRevocationReturnPosted call, and TestCallAzureReturn_RecordsReturnBeforeMark and _RetriesReturnRecordUntilItLands failed. Both files were restored, and the tests pass again.

Body-stale: none. The claims I checked (main at 9583647, migration 148, the Cloud Run cap, the test names, and the Released key being backend-only) are accurate.

This is a sensitive (money-path) PR, so it needs a second independent approval. The 2h-release vs operator-only choice is D25 for the acting owner. I recommend the PR's choice.

_{Reviewer: rF14. 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 7e984035cf07a8c87e95ca8e09de673c427534b0 merged onto main (95836478), together with #1524 in that order (combined tree bfbeaba5, 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 7e984035 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 7e984035cf07a8c87e95ca8e09de673c427534b0.