[IAmJSD]

Problem

Owner decision D26 (acting owner, 2026-10-28): treat an ambiguous Azure Return failure as uncertain.

1. When the Azure Return failed without a definite answer (a transport error, a 5xx, the 3m Return deadline from fix(revoke): detach the Azure Return once the in-flight claim is held #1528, a response that could not be read), handleAzureReturnError still cleared the revocation_in_flight flag and answered with a plain failure. If Azure had actually taken the Return, the purchase was refunded but showed as active and failed until an operator fixed the record.
2. On Lambda, the work before the claim (mostly Azure's CalculateRefund) is limited only by the function timeout. A slow quote could leave too little time, so Lambda could end the invocation partway through the Return or before its result was recorded.

Fix

Ambiguous Return failure (item 1), in internal/api/handler_purchases_revoke_uncertain.go and handleAzureReturnError:

• The flag is cleared only after a definite refusal: Azure answered the only request the client sent with a 4xx other than 408. The clear-and-fail path is unchanged for those (422 at the window edge, 400 for mapped client errors, 500 otherwise).
• A per-retry policy (returnAttemptPolicy) on the production Return client counts how many requests were sent. azcore retries a POST after transport errors, 408, 429 and 5xx, so the 4xx that ends a retried call can be Azure refusing a second copy of a Return it already accepted (for example, the connection reset after the 202). The policy runs after the bearer-token policy, so it counts only requests that reach the transport.
• Every other failure keeps the flag, so no request can post a second Return. It also logs the row for an operator and answers 500 "Azure reservation return failed and may already have been accepted by Azure; verify the reservation state in the Azure portal before retrying". That is the same status and wording as the RI exchange handler uses for its ambiguous failures (executeAzureExchange). The SDK accepts only a 202, so an unexpected 2xx also counts as uncertain.
• A retry during that window gets the existing 207 RECONCILE_PENDING and posts nothing. If no Return gets recorded, the fix(revoke): finalize only revocations whose Azure Return was recorded; release stale flags #1524 finalize_revocations sweep clears the flag after its 2h stale window, leaves the purchase unrevoked, and logs the row so an operator can reconcile it. I checked this on real Postgres (test below). The sweep code itself is unchanged; only its comment now mentions this case.

Too little Lambda time (item 2), in checkRevokeTimeLeft, called at the top of claimRevocationInFlight:

• On Lambda, if less than revokeClaimMinRemaining = 3m35s of invocation time is left, the claim is refused with a retryable 503 ("nothing was sent to Azure and the purchase is unchanged, so retry the revoke"). This happens before the flag is set and before any Return is posted.
• How the threshold is built: the detached Return (revokeReturnTimeout, 3m), plus one post-Return write budget (revokePersistTimeout, 30s; that is the accepted-Return record or the clear after a refusal, never both, and an uncertain failure writes nothing), plus 5s (revokeResponseMargin) to build and return the response. Shipped Lambda timeouts are at least these budgets plus the 30s lambdatimeout.Margin (TestLambdaTimeoutCoversRevokeBudget), so at least 25s is left for the work before the claim. A test pins that the threshold stays below that floor; otherwise every Lambda revoke would be refused.
• Non-Lambda runtimes: there the request deadline already does this job, so no extra check runs. The claim runs on the request context, so a request past its 30s deadline (internal/server/http.go) takes no claim and posts nothing (TestCallAzureReturn_RequestCanceledBeforeReturn_PostsNothing). The process also outlives the request, so a Return started just before the deadline still finishes and is recorded on its detached budgets. Applying the 3m35s threshold to a 30s request deadline would refuse every revoke on Fargate, Cloud Run and Container Apps; a test pins that this does not happen.

Money path: the only new write path is not writing (the flag stays set). Nothing new posts a Return. A refunded purchase can no longer be shown as active-and-failed, because only a single-send 4xx clears the flag. There are no DB or migration changes, and no new concurrent writers. A concurrent retry still hits the existing flip CAS, and the integration test checks that a retry posts no second Return.

Closes #1529

Verification

All evidence is local: unit tests with mocks, stub Azure clients, and the real armreservations Return client pointed at a local httptest TLS server; one integration test on real Postgres (testcontainers). Nothing called Azure.

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
export 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
go test ./internal/purchase/ -count=1                                               # ok
go test ./internal/mocks/ -count=1                                                  # ok
go test -tags integration -count=1 -timeout 20m \
  -run 'TestRevokeAzure_UncertainReturn_FlagKeptUntilSweepReleases|TestRevokeAzure_RequestCanceledDuringFailedReturn_ClearsFlag' ./internal/api   # both PASS
golangci-lint run --allow-parallel-runners [--build-tags integration] ./internal/api/ ./internal/purchase/   # 0 issues (both)
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases_revoke.go internal/api/handler_purchases_revoke_uncertain.go internal/purchase/finalize_revocations.go   # clean

New tests (internal/api/handler_purchases_revoke_uncertain_test.go, ..._uncertain_integration_test.go):

Test	Covers
TestCallAzureReturn_AmbiguousFailure_KeepsFlagAndAnswersUncertain	500/502/503/504, 408, unexpected 200, context.DeadlineExceeded (bare and in *url.Error), transport error, cut-off body: flag kept, uncertain 500, nothing recorded or revoked, one Return
TestCallAzureReturn_RealClient_AmbiguousKeepsFlag	real Return client: 503 then 409, and 504 then 400 RefundPolicyViolated (a 4xx after a retry), 503 on all 4 tries, 202 with its body cut off: flag kept
TestCallAzureReturn_RealClient_Single4xxClearsFlag / _AcceptedRevokes	real client: one 400 clears the flag; a 202 still revokes
TestCallAzureReturn_DefiniteRefusal_ClearsFlag	400/403/409/401/429 and window edge clear the flag with today's status mapping
TestIsAzureDefiniteRefusal_AttemptsAndStatus	status bounds and the retry rule
TestCallAzureReturn_LambdaTooLittleTime_RefusesBeforeClaim	1m, and threshold minus 1s: 503, no flip, no Return, no writes
TestCallAzureReturn_EnoughTimeOrNotLambda_Proceeds	Lambda with time to spare or no deadline; non-Lambda at 20s and 1s left: proceeds and revokes
TestRevokeClaimMinRemaining_Budget	threshold = 3m + 30s + 5s, below the shipped-timeout floor
TestRevokeAzure_UncertainReturn_FlagKeptUntilSweepReleases (integration)	503: row flagged, no return_posted_at, not revoked; retry gets RECONCILE_PENDING and posts no second Return; the sweep leaves the fresh flag alone; after aging revocation_in_flight_at past 2h the sweep releases it, still unrevoked

Existing tests changed to match the decision: TestCallAzureReturn_TransientError_ClearsInFlight is now ..._KeepsInFlight, and TestRevokePurchase_AzureReturnClientError now expects the uncertain 500. The two #1527 "canceled during a failed Return clears the flag on a live context" tests (unit and integration) now use a definite 400 instead of a 503, so they still test the live-context clear.

Fail-before / pass-after (unit and integration, run in throwaway copies of the tree):

• Main's handler code (git show origin/main: for handler_purchases_revoke.go and finalize_revocations.go, new files removed) with the changed tests: TestCallAzureReturn_TransientError_KeepsInFlight FAIL and TestRevokePurchase_AzureReturnClientError FAIL. The two 503-to-400 tests PASS on both versions, as intended.
• New code with main's behavior restored (the definite-refusal gate removed, checkRevokeTimeLeft a no-op): ..._AmbiguousFailure_... FAIL, ..._RealClient_AmbiguousKeepsFlag FAIL, ..._LambdaTooLittleTime_... FAIL, and the integration TestRevokeAzure_UncertainReturn_FlagKeptUntilSweepReleases FAIL ("want the uncertain-outcome ClientError, got revoke azure: Return failed"). The definite-refusal, accepted and enough-time tests PASS on both versions; they guard against an over-eager fix.
• Mutation probes: dropping the attempts > 1 rule fails TestIsAzureDefiniteRefusal_AttemptsAndStatus and two RealClient_AmbiguousKeepsFlag cases. Building the default factory's Return client without the counting policy fails the same two cases. Checking against revokeReturnTimeout instead of the full threshold fails LambdaTooLittleTime/just_under_the_needed.
• All pass on the branch head.

Combined-tree verification. Tested on a local, never-pushed tree: queue/platform @ 40dbbdaf = origin/main 6e2033cd plus:

Branch	Tested head
fix/issue-1494-listing-settlements	d7aa121
fix/issue-1529-ambiguous-return	8ecf506
fix/issue-1434-lambda-budget	8b6eebe
fix/issue-1432-retire-cfn-stack	9cb7de8
fix/issue-712-fire-default-aws-schedule	8bd7095

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers.

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests (-tags integration -count=1, one package at a time): internal/api, internal/database/postgres/migrations, internal/purchase and internal/server pass. internal/config had one failure, which comes from the fix(marketplace): close RI Marketplace poller gaps (pending rows with a listing id, superseded listings, deployment coverage) #1494 branch (a mid-claim listing gets settled); that branch is being fixed and isn't opened yet.
• Secrets: git-secrets on the changed files found nothing, and a planted fake key was flagged; scripts/test-git-secrets-allowlist.sh passes.
• Merge order: fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432 (retire the CloudFormation stack) must merge before the fire-default branch. On its own, that branch fails two tests in the CloudFormation stack's test file, which fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432 deletes.

Left out of scope / notes for reviewers

• The answer to a retry during the uncertain window is the existing 207 RECONCILE_PENDING, "Refund already issued. We will reconcile your audit record shortly. Do not retry." For a row whose Return is merely uncertain, "already issued" overstates it. Changing that response (for example keying the wording on return_posted_at) is outside fix(revoke): treat an ambiguous Azure Return failure as uncertain (keep the flag), and refuse the claim when too little Lambda time is left #1529's two items, so I left it alone; it may deserve its own issue.
• Credential failure before anything is sent: a token failure inside the Return call is not a ResponseError, so it counts as uncertain and the flag stays for up to 2h. This is conservative. It is unlikely in practice because CalculateRefund has just used the same credential and cached token.
• A 4xx from a client built without the counting policy (only test stubs) is judged on its status alone (attempts == 0). Production always builds the client through newAzureReturnClient.
• The "verify in the Azure portal" text is a 500 ClientError message, the same as RI exchange; the frontend already shows it as "Failed to revoke: ".

Frontend follow-up for the UI owner

No frontend change is needed for correctness. Optional polish: frontend/src/history.ts shows the uncertain answer as an error toast, "Failed to revoke: Azure reservation return failed and may already have been accepted by Azure; verify the reservation state in the Azure portal before retrying". A warning style that does not say "Failed", and offers no immediate retry, would fit the uncertain case better. The new 503 ("not enough time was left ... retry the revoke") fits the existing error toast as it is.

Maintainers: please mirror the issue's triage labels on this PR: priority/p3, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 6e2033c
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 79b6b830-0b33-424d-a04e-8c4d38757a98

Commits

Reviewing files that changed from the base of the PR and between 6e2033c and 8ecf506.

Files selected for processing: 8

• internal/api/handler_purchases_revoke.go
• internal/api/handler_purchases_revoke_claim_integration_test.go
• internal/api/handler_purchases_revoke_detached_test.go
• internal/api/handler_purchases_revoke_test.go
• internal/api/handler_purchases_revoke_uncertain.go
• internal/api/handler_purchases_revoke_uncertain_integration_test.go
• internal/api/handler_purchases_revoke_uncertain_test.go
• internal/purchase/finalize_revocations.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{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 8ecf506ad93e84c8b2b23eb084affc3d0bc2616a

Blocking:

• none

Non-blocking:

• Already noted in the PR body: a retry inside the uncertain window gets the existing 207 "Refund already issued ... Do not retry". When the Return is only uncertain, that overstates what we know. This is worth its own issue, for example keying the wording on return_posted_at.
• isAzureDefiniteRefusal: after the 2h sweep releases an uncertain row, a new revoke can get a single-send 409/400 from Azure saying the reservation was already returned. This PR treats that as a definite refusal: it clears the flag, answers 400, and the purchase stays active although it was in fact refunded. Main behaves the same way, and the sweep logs the row. A follow-up could send Azure's "already returned" error code to the reconcile path instead.
• An uncertain row reaches an operator only as an Errorf log line, at the Return failure and again at the sweep release. No metric filter or alarm watches either line. The user-facing message tells the user to check the portal, so this is acceptable, but the operator side relies on someone reading the logs.
• The counting policy sits after the bearer and RP-registration policies. A 401 claims-challenge re-send or an RP-registration retry therefore counts as 2 sends, and the result is "uncertain". That is the conservative direction, so it's fine.

Attacked:

• Lambda gate: I traced the Lambda path (HandleLambdaEvent -> handleLambdaHTTPEvent -> API.HandleRequest -> revoke). No shorter WithTimeout wraps the Lambda context before claimRevocationInFlight, so ctx.Deadline() is the invocation deadline. 3m35s also leaves 85s of a 300s timeout for the CalculateRefund work before the claim. The gate is skipped off Lambda, where http.go's 30s request deadline would otherwise refuse every revoke. IsLambda comes from runtime.IsLambda().
• Money path: the only behavior change is that the flag is not cleared. No new path posts a Return. A concurrent retry still hits the flip CAS and gets the 207. The detached Return context keeps the counter value, because context.WithValue survives azcore's per-try timeout contexts.
• Fail-before: in the scratch tree I removed the definite-refusal gate and made checkRevokeTimeLeft a no-op. These then fail: TransientError_KeepsInFlight, AzureReturnClientError, AmbiguousFailure_KeepsFlagAndAnswersUncertain, LambdaTooLittleTime_RefusesBeforeClaim and RealClient_AmbiguousKeepsFlag. Mutation: I dropped the attempts > 1 rule. TestIsAzureDefiniteRefusal_AttemptsAndStatus then fails, and so do the real-client cases "503 then 409" and "504 then 400". I restored everything afterwards.
• Ran: go build ./..., go vet ./..., go vet -tags integration ./...; go test -race -short -count=1 ./internal/api/ ./internal/purchase/ (ok); go test -race -tags integration -count=1 ./internal/api/ ./internal/purchase/ (ok, 218s and 84s); golangci-lint v2.10.1 on ./internal/api/ with and without --build-tags integration (0 issues).

Body-stale:

• none

_{Reviewer: rM2. 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 8ecf506ad93e84c8b2b23eb084affc3d0bc2616a

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases_revoke.go:248-250 (existing revocationInFlightResult): a retry inside the new uncertain window now gets "Refund already issued ... Do not retry", which is false for a Return that may simply have failed. The body already flags this. It deserves its own issue, because this PR makes that answer the usual follow-up to every 5xx or timeout.
• Product consequence for the owner to accept: an uncertain failure locks the purchase for up to 2h, until the finalize_revocations stale window releases it. A Return that failed close to the end of the revoke window can therefore leave the user no time to retry once the flag clears. Credential and token failures before any send (attempts == 0, not a ResponseError) count as uncertain too and take the same 2h lock, though nothing reached Azure. Conservative and disclosed, but the user pays for it.
• isAzureDefiniteRefusal treats a 4xx with attempts == 0 (a client built without the counting policy) as definite. Only test stubs build that today. If someone later builds a Return client outside newAzureReturnClient, retried 4xx answers would quietly count as definite again. A short comment at defaultAzureRevokeFactory saying it is the only supported constructor would help.
• (From an earlier review on this PR, which I agree with:) once the 2h sweep releases a row Azure actually accepted, a new revoke gets a single-send 4xx "already returned". That counts as a definite refusal, so the flag clears and the purchase stays active although it was refunded. Main does the same, so it isn't a regression. A follow-up could send that error code to the reconcile path instead.

Attacked:

• Lambda path: I checked that the ctx reaching claimRevocationInFlight on Lambda is the invocation ctx (StartLambdaHandler → HandleLambdaEvent → HandleRequest; no per-request WithTimeout on the revoke route), so ctx.Deadline() is the real invocation deadline and 3m35s is measured against it. With a 300s shipped timeout, 85s is left for the work before the claim. Off Lambda the check is a no-op; otherwise the 30s HTTP deadline would refuse every revoke.
• Policy order: in azcore v1.23.3, arm/runtime/pipeline.go puts auth and trace into plOpts.PerRetry, and runtime/pipeline.go:81-82 appends the user's PerRetryPolicies after them. So returnAttemptPolicy runs after the bearer policy, once per transport send, retries included. A 401-challenge resend would count twice, which errs toward uncertain.
• Fail-before: in the scratch checkout I made isAzureDefiniteRefusal always return true (main's always-clear behavior) and checkRevokeTimeLeft a no-op. Six tests then failed: TransientError_KeepsInFlight, IsAzureDefiniteRefusal_AttemptsAndStatus, RevokePurchase_AzureReturnClientError, LambdaTooLittleTime_RefusesBeforeClaim, AmbiguousFailure_KeepsFlagAndAnswersUncertain and RealClient_AmbiguousKeepsFlag. Separately, with the counting policy dropped from newAzureReturnClient, RealClient_AmbiguousKeepsFlag/503_then_409 and /504_then_400 failed. Restored both times; tree clean.
• Ran on 8ecf506: go build ./..., go vet ./..., go vet -tags integration ./... clean; go test -race -short -count=1 ./internal/api/ ./internal/purchase/ ok; go test -race -tags integration -count=1 ./internal/api/ ok (231s, whole package incl. the new sweep-release test on Postgres); golangci-lint v2.10.1 0 issues with and without --build-tags integration; gocyclo clean.
• Issue fix(revoke): treat an ambiguous Azure Return failure as uncertain (keep the flag), and refuse the claim when too little Lambda time is left #1529 items 1 and 2: both implemented. No migration and no new write path. The flip CAS still guards concurrent retries (the integration test asserts a single Return).

Body-stale:

• none

_{Reviewer: rM1. 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 8ecf506ad93e84c8b2b23eb084affc3d0bc2616a merged onto main (6e2033cd), together with #1577, #1578, #1579 in that order (combined tree bf700d64, 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 8ecf506a (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 8ecf506ad93e84c8b2b23eb084affc3d0bc2616a.