[IAmJSD]

Root cause

After #1526, the writes that follow an accepted Azure Return survive request cancellation, but the Return call itself still ran on the request context. The app's HTTP server gives every request a 30s deadline (handleHTTPRequest in internal/server/http.go). If that deadline or a client disconnect landed while the Return was in flight, Azure could still accept it, but the handler saw context.Canceled. It then recorded nothing and called ClearRevocationInFlight on the same dead context, which also failed. The finalize sweep released the row 2h later, and a refunded purchase read as active. Azure refuses a second Return, so there was no double refund.

Fix (internal/api/handler_purchases_revoke.go)

• Detached Return. Once claimRevocationInFlight holds the claim, the new postAzureReturn posts the Return under ctxutil.DetachedWithTimeout(ctx, revokeReturnTimeout). This is the pattern the RI exchange handler (fix(ri-exchange): run ExecuteExchange outside the request context #790, handler_ri_exchange.go) and purchase/manager.go use. The claim and everything before it (CalculateRefund, the quote check) stay on the request context, so a request canceled before the claim still takes no claim and posts no Return.

• revokeReturnTimeout = 3m, sized from the Azure client's own limits. The armreservations client runs on azcore v1.23.3's default pipeline:

  • up to 4 tries (MaxRetries 3);
  • backoff of 0.8s, 2.4s and 5.6s with up to 1.3x jitter, so at most about 11.4s in total;
  • a transport with a 30s dial timeout and a 10s TLS handshake timeout (sdk/internal/transport), and no per-try timeout.

  If all four tries use up the dial and handshake timeouts, that is about 171s, so 3m lets the client end a failing call with its own error. Only a response that never arrives, or Azure throttling with long Retry-After waits, runs into the 3m deadline. On Lambda, 3m plus the 30s persist budget plus the 30s margin is 240s, under the 300s lambda_timeout floor. The new TestLambdaTimeoutCoversRevokeBudget pins this, using the same lambdatimeout.AssertCovers helper as the RI exchange and purchase budgets (fix(iac): default Lambda timeout (30s) is shorter than detached in-process budgets #913).

• Error path. handleAzureReturnError now runs ClearRevocationInFlight on its own detached revokePersistTimeout (30s) context, so the clear can land after the request is gone. The status mapping (422/400/500) is unchanged.

• Retry waits respect the budget. The time.Sleep between persist attempts is replaced by waitRevokeRetry. It skips a backoff that would outlast the 30s budget and stops early if the context ends. Before, the worst case was about 43s against a 30s budget.

• Per-attempt cap. Each persist attempt (two single-row UPDATEs by primary key) is now capped at revokePersistAttemptTimeout = 4s, so a first attempt stuck on a hung connection cannot use up the whole budget (issue proposal item 2). Four attempts at the cap plus 13s of backoff is 29s, which fits in 30s; TestRevokePersistScheduleFitsBudget pins this. Both writes are idempotent (COALESCE on return_posted_at; MarkPurchaseRevoked treats an already-revoked row as success), so retrying an attempt that was cut off after its commit is safe. The 207 log line now reports the number of attempts actually made.

• Comments. The staleRevocationInFlightAfter comment in internal/purchase/finalize_revocations.go and its test comment said Fargate and Container Apps don't cap request time. They now say that the app's own server caps every request at 30s (internal/server/http.go), and that past the flip the handler bounds the Return and its writes at 3m and 30s.

No migration and no schema change. The claim (the only concurrency-relevant DB operation) is unchanged, so this adds no new race. A concurrent revoke during the detached Return still sees the flag and gets RECONCILE_PENDING, as before.

Closes #1527

Verification

All of this is unit, mock or fixture evidence plus local integration on real Postgres (testcontainers), run with stub Azure clients. Nothing called Azure.

Commands run on this branch (head 69c05a5d), with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true:

go build ./... && go vet ./... && go vet -tags integration ./...            # OK
go test ./internal/api/ -count=1                                            # ok 15.3s
go test ./internal/purchase/ -count=1                                       # ok
go test ./internal/mocks/ -count=1                                          # ok
go test -tags integration -count=1 -timeout 20m \
  -run 'TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes|TestRevokeAzure_RequestCanceledDuringFailedReturn_ClearsFlag' ./internal/api/   # both PASS
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/ ./internal/purchase/            # 0 issues
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/api/        # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases_revoke.go internal/purchase/finalize_revocations.go   # nothing over 10

Fail-before / pass-after. I swapped in the origin/main handler, keeping only the two new constant names so the tests compile, and ran the new and changed tests:

Test	Kind	Before fix	After fix
TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks: request canceled while the Return is in flight, then accepted	unit (store fails writes on a done context, as pgx does)	FAIL: Return saw context.Canceled	PASS: Return recorded, revoked, Return deadline about 3m
TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes: same case	integration, real Postgres	FAIL	PASS: return_posted_at and revoked_at set, flag cleared
TestCallAzureReturn_RequestCanceledDuringFailedReturn_ClearsInFlight: canceled mid-Return, Azure answers 503	unit	FAIL	PASS: clear runs on a live, bounded context
TestRevokeAzure_RequestCanceledDuringFailedReturn_ClearsFlag: same case	integration, real Postgres	FAIL	PASS: revocation_in_flight=false, nothing recorded
TestPersistAzureRevocation_RetryWaitRespectsBudget: 1h backoff against the 30s budget	unit	FAIL: still sleeping at 5s	PASS: 207 at once
TestPersistAzureRevocation_HungAttemptLeavesBudgetForRetry: first attempt hangs	unit	FAIL: hung for 30.0s	PASS: cut at 4s, retry revokes
TestCallAzureReturn_RequestCanceledBeforeReturn_PostsNothing: canceled before the claim	unit (kept from #1526)	PASS	PASS: no claim, no Return
TestLambdaTimeoutCoversRevokeBudget, TestRevokePersistScheduleFitsBudget	budget pins	n/a (new invariants)	PASS

Mutation probes (each applied alone to the fixed code; every one was caught):

• clear on the request context: the unit and integration clear tests fail;
• time.Sleep instead of waitRevokeRetry: the budget test fails;
• no per-attempt cap: the hung-attempt test fails;
• Return on context.WithoutCancel with no deadline: the detached-deadline assertion fails.

Test changes:

• The fix(revoke): record an accepted Azure Return even if the request is canceled right after it #1526 stub cancelAfterReturnClient now behaves like the real SDK: it returns the context's error if its context was canceled. Its old assertion "the Return itself must run under the request's context" is replaced by asserting that the Return is detached with a 3m deadline.
• Three ClearRevocationInFlight mock expectations now match any context.

Full-suite verification at local queue commit 1c0ef0b0 = origin/main e3de1523 + fix/issue-1527. 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 and internal/purchase; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Out of scope (noted, not changed)

• Ambiguous Return failures still clear the flag. A transport error, a 5xx, or hitting the new 3m deadline can each mean Azure accepted the Return, yet handleAzureReturnError clears the in-flight flag as before (the brief said to keep this path). Azure refuses a second Return, so there is no double refund, but the row can read as active while it has been refunded. The Azure RI exchange path treats non-4xx failures as ambiguous (#1642). A follow-up could keep the flag set for non-4xx Return failures and leave them to the sweep or an operator.
• The caller can lose the response. The HTTP server's WriteTimeout is 30s, so if the detached Return takes longer, the response cannot be delivered. The record is still written. A retry from the UI gets the existing RECONCILE_PENDING (207), because the flag is set. This matches the RI exchange behavior after fix(ri-exchange): run ExecuteExchange outside the request context #790.
• Container shutdown. A container stopping mid-Return (30s termination grace) can still cut the Return short. The 2h sweep release remains the backstop.

No frontend change is needed.

Maintainer: please mirror the issue's triage labels on this PR: priority/p3, type/bug (the issue has no triaged label).

🤖 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 69c05a5d50a583b4c1a0fdbb205ada809f5622ea

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:833 (Lambda budget): TestLambdaTimeoutCoversRevokeBudget covers only the time after the claim (3m + 30s). Everything before the claim runs on the request context, which on Lambda carries the function deadline itself. That includes azureCalculateRefund, which runs on the same azcore pipeline the PR body sizes at up to about 171s. With a slow CalculateRefund, the claim can be taken with less than 3m30s left. Lambda then kills the invocation mid-Return or mid-persist, and the row ends up exactly as in fix(revoke): detach the Azure Return call once the claim is held, so a request timeout mid-Return can't lose the record #1527 (flag set, nothing recorded, released by the 2h sweep). It is rare and fails safe (Azure refuses a second Return), and the RI exchange budget has the same shape. A cheap guard would be to refuse the claim, with a retryable 503, when ctx.Deadline() leaves less than revokeReturnTimeout+revokePersistTimeout. Another option is to bound CalculateRefund.
• handleAzureReturnError now clears the flag right away after an ambiguous failure, including the new 3m self-imposed deadline. Before, a dead request context made the clear fail, which left the flag set until the 2h sweep. That was safe only by accident. There is no money risk, because Azure refuses a second Return and the sweep would have released the row anyway. The body discloses this and proposes a follow-up (keep the flag for non-4xx). I agree that follow-up is worth filing.
• revokePersistAttemptTimeout = 4s also covers waiting for a pool connection. Under pool saturation, the attempts could each time out where one 30s attempt would have succeeded. The worst result is the existing 207 RECONCILE_PENDING plus a sweep reconcile, so this is acceptable.

Attacked: I checked that the claim and everything before it (CalculateRefund, quote check) stay on the request context, so a request canceled before the claim still takes no claim (the #1526 test still passes). I checked that only the Return, the error-path clear, and the persist are detached, each with its own bound. I checked that the attempt cap plus backoff (4x4s + 1+3+9s = 29s) fits the 30s budget and that waitRevokeRetry breaks out cleanly to the 207. Both writes are idempotent under retry (COALESCE on return_posted_at; MarkPurchaseRevoked's revoked_at IS NULL plus its existence check), so a retry after a cut-off commit is safe. I verified the body's numbers: azcore v1.23.3 in go.mod, the terraform lambda_timeout >= 300 validation, CFN 900. Commands: go build ./...; go vet (with and without -tags integration) on internal/api and internal/purchase; go test -race -short -count=1 on both (ok); go test -race -tags integration -count=1 on all of internal/api (268s, ok) and internal/purchase (ok); golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 (nothing). Fail-before: I replaced handler_purchases_revoke.go with the origin/main version plus only the two new constants, then ran the tests. All six new or changed behavior tests FAIL: unit cancel-after-Return, unit cancel-during-failed-Return, both integration tests on real Postgres, retry-wait-budget (stuck at 5s), and hung-attempt (hung 30.01s). Afterwards I restored the file.

Body-stale: none. Every claim I checked holds (root cause, the 30s server timeout in internal/server/http.go, the budget arithmetic, the Lambda floor, and the fail-before table).

_{Reviewer: rF18. 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 69c05a5d50a583b4c1a0fdbb205ada809f5622ea

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:985 (handleAzureReturnError): an ambiguous Return failure still clears the in-flight flag. That covers a transport error, a 5xx, and now also the new 3m revokeReturnTimeout. After the clear, the user is invited to retry. Azure refuses the second Return, so no money moves twice, but the user sees a failure on a purchase that was refunded, and the row reads active. The PR body declares this out of scope, and overall this PR shrinks the window: a Return now has to run past 3m instead of 30s. The Azure RI exchange path (handler_ri_exchange.go:1329) already answers "may already have been submitted, verify before retrying" for non-4xx errors. A follow-up should do the same here and keep the flag set for non-4xx failures.
• On Lambda, the pre-claim work (CalculateRefund, quote check, claim) runs on the request context, which is bounded only by the function deadline. The 210s detached budget is covered only up to lambdatimeout.Margin (30s). If CalculateRefund is slow (the azcore retries can take minutes), Lambda can end the invocation mid-Return, and the 2h sweep is then the backstop. This is the same pattern as the RI exchange and purchase budgets (fix(iac): default Lambda timeout (30s) is shorter than detached in-process budgets #913), so it is not new to this PR.
• recordAzureReturnAndMark: both writes share one 4s attempt context. If MarkRevocationReturnPosted hangs, MarkPurchaseRevoked in the same attempt fails at once and the retry picks it up. That is harmless with the current schedule. I note it only because the comment says "the mark may still land".

Attacked:

• Read git diff origin/main...69c05a5d against fix(revoke): detach the Azure Return call once the claim is held, so a request timeout mid-Return can't lose the record #1527. All three proposal items and both acceptance criteria are met, so Closes #1527 is right.
• The claim stays on the request context. Mutation: I moved the claim onto a detached context, and TestCallAzureReturn_RequestCanceledBeforeReturn_PostsNothing failed.
• Checked the budgets by hand. Persist: 4x4s + 1+3+9s = 29s, under 30s, and waitRevokeRetry skips waits that would overrun. Lambda: 3m + 30s + 30s margin = 240s, under the 300s floor in both TF variables.
• Checked the store writes. They are idempotent, so retrying a cut-off attempt is safe. The claim and ClearRevocationInFlight are unchanged, so no new concurrency race. A concurrent revoke during the detached Return still gets RECONCILE_PENDING.
• Fail-before: I swapped in main's handler, adding only the two new constants. Four new or changed unit tests failed: RetryWaitRespectsBudget, HungAttempt (30.0s hang), RequestCanceledAfterReturn and RequestCanceledDuringFailedReturn. Both new integration tests also failed against main's handler. All pass on the head.
• Ran: go build ./...; go vet with and without -tags integration on internal/api and internal/purchase; go test -race -short -count=1 on both (ok); go test -race -tags integration -count=1 on both (ok, 250s/82s); golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 on the changed non-test files (none).

Body-stale:

• none. The #1642 reference resolves to commit a9e7209 (RI exchange submit idempotency), and the ambiguity handling it cites is in handler_ri_exchange.go. On GitHub, though, #1642 does not resolve to an issue or PR in this repo.

_{Reviewer: rF17. 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 69c05a5d50a583b4c1a0fdbb205ada809f5622ea merged onto main (e3de1523), together with #1528 in that order (combined tree 2be38efb, 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 69c05a5d (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 69c05a5d50a583b4c1a0fdbb205ada809f5622ea.