[IAmJSD]

Root cause

After Azure accepts a Return, persistAzureRevocation records it (MarkRevocationReturnPosted, #1524) and marks the purchase revoked (MarkPurchaseRevoked), with the 1s/3s/9s retry loop. All of that ran under the request context. If the request was canceled right after the Return (a client disconnect, an upstream timeout), pgx refused every attempt with context canceled: the Return went unrecorded, the row stayed unrevoked, the handler answered 207 RECONCILE_PENDING, and because no return_posted_at was recorded, the finalize_revocations sweep would eventually release the row after 2h instead of finalizing it. The purchase then shows as active even though Azure refunded it. Azure refuses a second Return, so there was no double refund, but the record was wrong.

Fix

• internal/api/handler_purchases_revoke.go: persistAzureRevocation now runs the post-Return writes under ctxutil.DetachedWithTimeout(reqCtx, revokePersistTimeout). That is the repo's existing helper for context.WithTimeout(context.WithoutCancel(ctx), d), already used for the RI-exchange and purchase terminal writes. The retry loop is unchanged.
  • Why 30s (revokePersistTimeout): it covers the full existing retry schedule (13s of backoff) plus four attempts of two single-row UPDATEs. It still limits how long a hung database can hold the handler after its caller has gone. It matches riExchangeTerminalSaveTimeout (30s), which bounds the same kind of write. If the budget runs out, the result is the same 207 as exhausted retries.
  • Only the post-Return writes are detached. CalculateRefund, the TOCTOU check, the in-flight claim (FlipPurchaseRevocationInFlight) and the Return call itself still run under the request context and honor cancellation. handleAzureReturnError's ClearRevocationInFlight is also unchanged (it runs only when the Return failed).
• internal/purchase/finalize_revocations.go: corrected the staleRevocationInFlightAfter comment. It used to say 2h is "twice the longest request a deployment can run", which is wrong because Fargate and Container Apps don't cap request time. The comment now says why correctness doesn't depend on that number: a Return recorded after the release is still finalized whether or not the flag is set, and a second revoke can't double-refund because Azure refuses a second Return of the same reservation. I made the matching comment fix in finalize_revocations_test.go.

No migration. No frontend change.

Closes #1525

Tests

• TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks (unit, mock store): a stub Return client accepts the Return and then cancels the request context. The store wrapper (ctxHonoringStore) fails writes with ctx.Err() once the context is done, the way pgx does, and records the context state at write time. The test asserts:
  • the result is revoked;
  • both writes ran under a context that was not done;
  • each write had a deadline no later than revokePersistTimeout;
  • the Return itself ran under the request's cancelable context.
• TestCallAzureReturn_RequestCanceledBeforeReturn_PostsNothing (unit): CalculateRefund succeeds and then the request is canceled. The test asserts the claim does not land, no Return is posted, and nothing is recorded. This guards against detaching too early.
• TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes (integration, real Postgres, through loadAndRevokePurchaseHistory): the request is canceled right after the stub Azure client accepts the Return. The row must end with return_posted_at set, revoked_at set, and revocation_in_flight cleared.
• Existing Azure-path mock expectations for MarkPurchaseRevoked / MarkRevocationReturnPosted now match mock.Anything for the context, because those writes no longer receive the request context.

Verification

All commands were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. Azure is a stub throughout; there are no real cloud calls.

go build ./...                        # ok
go vet ./...                          # ok
go vet -tags integration ./...        # ok
go test ./internal/api/ -count=1      # ok
go test ./internal/purchase/ -count=1 # ok
go test ./internal/mocks/ -count=1    # ok
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases_revoke.go internal/purchase/finalize_revocations.go   # no output
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/api/ ./internal/purchase/   # 0 issues
go test -tags integration -count=1 -timeout 20m \
  -run 'TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes|TestCallAzureReturn_RequestCanceled' ./internal/api/

Fail-before / pass-after. I ran the three new tests with handler_purchases_revoke.go restored from main. A temporary file defined only the revokePersistTimeout constant so the tests would compile.

Test	Before (main handler)	After
TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes (real Postgres)	FAIL: want revoked, got *api.revokeReconcilePendingResult {Code:RECONCILE_PENDING ...}	PASS
TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks (unit, mock store)	FAIL: same RECONCILE_PENDING; log shows 4 attempts all context canceled, return recorded=false	PASS
TestCallAzureReturn_RequestCanceledBeforeReturn_PostsNothing (unit)	PASS (guard test)	PASS

Mutation probes (unit tests). Each mutation was applied to the fixed handler, then reverted.

• Detach without a timeout (context.WithCancel(context.WithoutCancel(reqCtx))): FAIL, write 0 must have its own bounded timeout.
• Detach the whole callAzureReturn (ctx = context.WithoutCancel(ctx) at the top): both new unit tests FAIL.
• Pass the Return context.WithoutCancel(ctx): FAIL, the Return itself must run under the request's context.

Concurrency. The change adds no new DB statement and no new lock. It only changes which context the two existing post-Return UPDATEs run under, and both are idempotent (COALESCE stamp and the existing mark). The existing concurrent-revoke race test (TestRevokeAzure_ConcurrentFirstRevokes_OneReturn, #1518) covers the claim and is unaffected. I did not rerun it here; it is included in the combined-tree run.

Full-suite verification at local queue commit c69ce9fa = origin/main da38fa74 + fix/issue-1525. 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 the changed packages (internal/api, internal/purchase); the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Out of scope / not addressed

• The database being unavailable for the whole retry window (also mentioned in the issue's Problem section): the issue's proposal doesn't cover it and this PR doesn't either. If every attempt within the 30s budget fails, the handler still returns 207, no return_posted_at is recorded, and the sweep releases the row after 2h, which an operator then fixes from the error log. Fixing that would need a durable record outside this DB (or a write-ahead record before the Return), which is a design decision for the maintainer.
• A hard process kill (a Lambda function timeout, or a container being killed) can't be survived in-process. A detached context only protects against cancellation of the request context. The 30s budget fits well inside lambda_timeout (>= 300s, enforced by Terraform validation).
• internal/api/handler_purchases_revoke.go was already over the 500-line guideline before this change (~1090 lines). I didn't split it here.

Labels

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, type/bug.

🤖 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 c16d3ecd62001abfe3c6623bd7ce54291f9bdf5a

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:804-816: the ambiguous case is still open. If the request is canceled while returnClient.Post is in flight, Azure may have accepted the Return but the SDK returns context canceled. handleAzureReturnError then runs ClearRevocationInFlight under the canceled ctx (it fails, so the flag at least stays set), the handler answers 500, return_posted_at is never stamped, and the sweep releases the row after 2h. That matches the issue's scope ("right after the Return"), and Azure refuses a second Return, so there is no money risk. It's worth a follow-up issue, or a line under "Out of scope" in the PR body.
• internal/api/handler_purchases_revoke.go:979-987: the retry loop's time.Sleep(backoff) doesn't watch the detached ctx. If a hung DB uses up the 30s budget on the first attempt, the loop still sleeps 1+3+9s and makes three attempts that fail instantly. A hung DB can therefore hold the handler for about 43s, not the 30s the revokePersistTimeout comment implies. This behavior existed before the PR and does no harm; a select on ctx.Done() would make the bound exact.
• handler_purchases_revoke_return_posted_test.go: assert.LessOrEqual(w.remaining, revokePersistTimeout) is weak on its own. Together with the bounded check and a request ctx that has no deadline, it does pin the detached timeout, so this is fine as written.

Attacked:

• Read the diff (three-dot against origin/main) and the issue's acceptance criteria. returnClient.Post is the only Azure Return call site. Only persistAzureRevocation is detached. The CalculateRefund, TOCTOU, FlipPurchaseRevocationInFlight, Post and handleAzureReturnError steps all still get the request ctx. ctxutil.DetachedWithTimeout keeps ctx values, but the store doesn't carry a tx in ctx, so nothing stale is reused. Both detached writes are idempotent (COALESCE stamp; mark).
• Fail-before (separate scratch worktree, handler restored from origin/main, plus a temp file defining only the constant): TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes (real Postgres) and TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks both FAIL with RECONCILE_PENDING; the guard test passes. Mutation: adding ctx = context.WithoutCancel(ctx) before the claim makes TestCallAzureReturn_RequestCanceledBeforeReturn_PostsNothing FAIL, so the "nothing before the Return is detached" criterion has a test. Restored afterwards.
• Ran at the head: go build ./...; go vet ./... with and without -tags integration; go test -race -short -count=1 ./internal/api/ ./internal/purchase/ ./internal/ctxutil/ (ok); go test -race -tags integration -count=1 ./internal/api/ ./internal/purchase/ (ok, which includes the fix(revoke): let only one request flip an Azure purchase to in-flight, so concurrent first revokes can't post two Returns #1518 concurrent-revoke test); golangci-lint v2.10.1 with and without the integration tag, and gocyclo -over 10 (0 issues each; gocyclo no output).
• Checked the PR body's lambda_timeout >= 300 validation claim in terraform/environments/aws/variables.tf: accurate.

Body-stale: none

_{Reviewer: rF15. 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 c16d3ecd62001abfe3c6623bd7ce54291f9bdf5a

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:803 (returnClient.Post(ctx, ...)): the Return itself still runs under the request context, and the new unit test pins that (sawRequestCtx). On the container deployments that context has a 30s deadline (internal/server/http.go:274, context.WithTimeout(r.Context(), 30*time.Second)) and is canceled on client disconnect. If CalculateRefund, the claim and the Return together hit that deadline while the Return POST is in flight, azcore aborts with a context error even though Azure may already have accepted the Return. handleAzureReturnError then runs ClearRevocationInFlight under the same dead context, so that call fails and the flag stays set. Nothing records return_posted_at, and the sweep releases the row after 2h, which is the outcome fix(revoke): record an accepted Azure Return even if the request is canceled right after it #1525 is meant to prevent, in a narrower window. This was already true before this PR and the issue's scope doesn't cover it, so it doesn't block. Repo precedent goes the other way, though: fix(ri-exchange): run ExecuteExchange outside the request context #790 detaches the provider call once the claim is held (handler_ri_exchange.go:1322, "Past the claim the exchange may commit at Azure; a cancelled request must not abort the submit"), and purchase/manager.go:279 does the same. I suggest a follow-up issue to run the Return under DetachedWithTimeout after claimRevocationInFlight. The issue's "no write before the Return ignores cancellation" would still hold, because the claim stays on the request context.
• internal/purchase/finalize_revocations.go:30-33: the new comment says "Fargate and Container Apps do not cap it at all". The platforms may not, but the app's own HTTP server wraps every request in a 30s context (internal/server/http.go:274, plus WriteTimeout: 30s at :75), and the flip-to-Return section runs under that context. So the flip-to-Return window is bounded on every container deployment, by the app rather than the platform. The comment would be more accurate if it said that. It doesn't change behavior, and the "correctness does not depend on it" reasoning stands.
• internal/api/handler_purchases_revoke.go:974-986: all four attempts share one 30s budget, and time.Sleep(backoff) ignores the context. If the first attempt hangs (for example on pool acquire), it uses the whole budget, and the remaining attempts fail immediately after sleeping 13s. The worst case is about 43s, not 30s, and the retries don't help in that case. A per-attempt timeout, or a select on ctx.Done() in the sleep, would make the retries meaningful. Minor, and the same as before this PR.

Attacked:

• Confirmed nothing before the Return was detached: the CalculateRefund, TOCTOU, claim and handleAzureReturnError paths all still use the request context. The detached context only carries the principal value, with no tx in context. Nothing after persistAzureRevocation uses the request context. The 30s budget fits within lambda_timeout >= 300.
• Fail-before: I replaced the DetachedWithTimeout line with ctx := reqCtx. TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes (real Postgres) then failed with RECONCILE_PENDING, and so did TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks. Both pass after restoring the line (worktree confirmed clean). The before-Return guard test would catch a detached claim, because the unexpected mock call panics.
• Ran: go build ./...; go vet with and without -tags integration on internal/api and internal/purchase; go test -race -short -count=1 and go test -race -tags integration -count=1 on both packages (all ok); golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the changed files (clean).

Body-stale:

• "Fargate and Container Apps don't cap request time": the app's HTTP server caps every request context at 30s (internal/server/http.go:274), so this is only true of the platform. The issue makes the same claim.
• "It still limits how long a hung database can hold the handler": true, but the limit is about 43s (30s budget plus 13s of sleeps that ignore the context), not 30s.

_{Reviewer: rF16. 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 c16d3ecd62001abfe3c6623bd7ce54291f9bdf5a merged onto main (da38fa74), together with #1526 in that order (combined tree 42d454ce, 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 c16d3ecd (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly c16d3ecd62001abfe3c6623bd7ce54291f9bdf5a.