[IAmJSD]

Summary

Root cause. The RI exchange execute paths called the provider on the HTTP request context. A client disconnect, or the container server's 30s per-request timeout (internal/server/http.go), could cancel ExecuteExchange after the provider had already committed the exchange. The terminal ledger write that records the outcome then ran on the same dead context and failed, so the record stayed processing. This is the same class of bug as #706, which #789 fixed for purchases.

Fix. Mirrors #789's detachedTimeout pattern:

• The provider call runs on context.WithTimeout(context.WithoutCancel(ctx), 4m) (riExchangeExecutionTimeout). It keeps the request's values, drops its cancellation, and is bounded by its own budget.
• Terminal writes after execution run on a separate detached 30s context (riExchangeTerminalSaveTimeout). These are CompleteRIExchangeWithPayment (with its retries), FailRIExchange on an execution error, and the best-effort approved_by stamp.
• Applied to:
  • the direct AWS endpoint (executeExchange, the line the issue names)
  • the approval flow (executeApprovedExchange, which owns the terminal save)
  • the Azure execute endpoint (executeAzureExchange). Its ExecuteExchange polls a long-running operation, so it is the path most exposed to the 30s timeout.
• The direct AWS path now uses the same injectable executor (awsExchangeExecutor) as the approval flow. Before this, it called exchange.ExecuteExchange directly and could not be tested without AWS.
• Gates before the commit (auth, re-quote, claim, reservation) still use the request context; cancelling them before any money moves is correct. Their failure writes, however, are detached (see below).

Lambda lifetime. On Lambda the invocation is still capped by the function timeout (default lambda_timeout = 30). No context can extend that, so detaching only helps where the request context is shorter-lived than the process, i.e. container deployments and client disconnects. #789 needs lambda_timeout >= 270s for the same reason, and the 4m and 30s budgets here assume the same.

Shared helper. The detaching is ctxutil.DetachedWithTimeout(ctx context.Context, d time.Duration) (context.Context, context.CancelFunc) in internal/ctxutil/detached.go. It is now the single copy: purchase.detachedTimeout was removed and its two call sites use it. #895 (#784) can adopt it once this merges.

Pre-commit failure writes. failExchange detaches its own write (30s). Every caller runs after the pending -> processing transition has committed, so a cancel before ReserveApprovedRIExchange can no longer strand the record in processing.

Direct execute paths persist nothing (known gap, out of scope). The direct AWS and Azure execute endpoints write no ledger row. If the client has disconnected, the detached call can still commit, and the exchange ID and result end up only in the server log. The 15-minute submit claim (ri_exchange_idempotency.go, never released) turns a client retry into a 409, so the exchange can't run twice. The caller's only way to learn the outcome is the provider console, which the 409 text already tells them to check. A ledger row for those paths would be a follow-up if the maintainer wants one.

Closes #790

Verification

All checks ran locally on macOS against mocks or stubs. No real AWS or Azure calls were made. There is no real-account coverage of an actual exchange being committed while the request is cancelled.

New regression tests in internal/api/ri_exchange_detached_test.go. Each one cancels the request context while the provider call is in flight, then asserts two things. First, the provider call's context is still live and carries its own ~4m deadline. Second, the terminal write receives a context that has not been cancelled. The second check uses a mock.MatchedBy(ctx.Err() == nil) expectation, standing in for a real DB driver refusing a cancelled context.

Fail before the fix. The executor seam was already in place, so the direct path could be stubbed. Command, run once per test:
go test ./internal/api/ -run '^<Test>$' -count=1

• TestExecuteApprovedExchange_RequestCancelDoesNotCancelExchangeOrLedger: FAIL. mock: Unexpected Method Call ... (*context.cancelCtx=context.Background.WithCancel) not matched by func(context.Context) bool, meaning the ledger write was attempted on the cancelled request context.
• TestExecuteApprovedExchange_RequestCancelStillRecordsFailure: FAIL, same mismatch on FailRIExchange.
• TestExecuteExchange_RequestCancelDoesNotCancelAWSExchange: FAIL, cancelling the request must not cancel an exchange already submitted to the provider.
• TestExecuteAzureExchange_RequestCancelDoesNotCancelExchange: FAIL, same message.

Review round 1 (d4fd03d). Two more tests, plus the existing ones now checking the budgets against literals (4m execution, 30s terminal save):

• TestExecuteApprovedExchange_RequestCancelBeforeReserveRecordsFailure: FAIL before the failExchange detach (FailRIExchange got the canceled request ctx), PASS after.
• TestStampRIExchangeApprover_SurvivesRequestCancel: FAIL with the stamp detach reverted (mutation), PASS with it.
• Probe: execution budget changed from 4m to 40m makes 3 tests FAIL (it previously survived).
• internal/ctxutil/detached_test.go covers: values kept, parent cancel dropped, own deadline, CancelFunc works.

Pass after the fix.

• go test ./internal/api/ -run 'RequestCancel' -count=1 -v: all 4 PASS.
• go build ./...: ok.
• go vet ./...: ok.
• go test -race -short -count=1 ./...: ok, whole repo.
• go test -race -count=1 ./internal/api/: ok.
• golangci-lint run --timeout=10m ./... with v2.10.1, the CI-pinned version: 0 issues.

Notes:

• Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. The local ~/.aws/config breaks the unrelated TestDiscoverOrgAccounts_* tests.
• Existing tests that pinned the exact request ctx on ExecuteExchange, CompleteRIExchangeWithPayment or StampRIExchangeApprovedBy now use mock.Anything, because those calls deliberately get a detached context.
• With this change, unparam started flagging the unchanged failExchange (its error result is always nil). I couldn't pin down why. It has a //nolint:unparam with a justification, matching existing usage in the repo.

Labels

The author lacks label permissions. A maintainer should mirror #790's triage labels onto this PR: priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/bug, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 12 billable files and costs up to $3.00.}

Or wait 58 minutes for your next included review.

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

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 25e69dc9-6382-4bf5-9825-699c1a12034a

📥 Commits

Reviewing files that changed from the base of the PR and between e003025 and d4fd03d.

📒 Files selected for processing: 12

• internal/api/handler.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_azure_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/middleware_test.go
• internal/api/ri_exchange_detached.go
• internal/api/ri_exchange_detached_test.go
• internal/api/ri_exchange_idempotency_test.go
• internal/api/router_handlers_test.go
• internal/ctxutil/detached.go
• internal/ctxutil/detached_test.go
• internal/purchase/manager.go

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

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

[cristim]

Independent review of 15e3ec8: draft: blocks ready

Findings

1. Actionable, internal/api/handler_ri_exchange.go:2295-2303 and :2658-2667 (executeApprovedExchange pre-commit failure writes). The PR body says gates before the commit correctly keep the request context. That holds for the provider call, but not for the ledger. TransitionRIExchangeStatus(pending -> processing) commits on the request ctx, then GetGlobalConfig, ReserveApprovedRIExchange and their failExchange(ctx, ...) calls run on the same ctx. Scenario: the client disconnects (or the 30s server timeout fires, internal/server/http.go:253) right after the transition. ReserveApprovedRIExchange fails with context canceled, failExchange(ctx) then fails on the dead ctx (only logged), and the record stays processing. The approval retry gets 409 from the transition guard, so it cannot be retried, and the row's initial quote keeps counting against the daily cap (GetRIExchangeDailySpend includes processing). Fix: run the failExchange calls before the provider call on a detached riExchangeTerminalSaveTimeout context too (simplest: make failExchange itself detach), and add a test that cancels before ReserveApprovedRIExchange.
2. Actionable (test gap), internal/api/ri_exchange_detached_test.go. Mutation probe: reverting the stampRIExchangeApprover detach (handler_ri_exchange.go:2313-2315) leaves the whole internal/api suite green. The PR body lists the approved_by stamp as detached but no test pins it. Add one that cancels the request and expects StampRIExchangeApprovedBy with liveCtx().
3. Nit, ri_exchange_detached_test.go:41. assertDetached computes the expected deadline from riExchangeExecutionTimeout, so changing the constant (probe: 4m to 40m) still passes. The test proves "has a deadline", not "4m". Acceptable, but say so or assert against a literal.
4. Nit, handler_ri_exchange.go:1243-1247 comment. It still describes "a ctx cancellation mid-poll" as an ambiguous case. After this change a request cancel can no longer cause it; only the 4m budget can. Update the comment.
5. Note (not blocking, document in PR body). Direct AWS and Azure execute paths persist nothing: when the client has disconnected, the detached call can commit and the exchange ID and result are only in the server log (logging.Errorf/Infof). The 15 minute claim (ri_exchange_idempotency.go:30, never released) correctly turns the client retry into a 409, so there is no double-execute, but the caller has no way to learn the outcome except the provider console. The 409 text already says to verify. Out of scope for #790 unless the maintainer wants a ledger row there.

Deployment reality (the "background goroutine" question)

There is no background goroutine. The handler still blocks on the detached call, so the change only helps where the request ctx dies earlier than the process.

• Lambda: no effect. lambda_timeout defaults to 30 (terraform/environments/aws/variables.tf:231), and the 4m execution plus 30s save budgets need >= 270s. The PR and #789 both state that requirement but nothing enforces it (no validation or default change). A default AWS deployment still dies at 30s with the exchange in flight, the same as before. A follow-up should either add a variable validation or document it in the deploy guide.
• Container (Cloud Run / Container Apps): helps. handleHTTPRequest uses a 30s ctx (internal/server/http.go:253); http.Server.WriteTimeout is 30s (:76) but does not cancel the handler, so the detached call runs to completion. Residual risk: SIGTERM on scale-in/deploy has a 30s shutdownGracePeriod (:160) against a 4m budget, so an in-flight exchange can still be cut by a revision rollout. Cloud Run request_timeout is a variable (terraform/modules/compute/gcp/cloud-run/variables.tf:62) and must be >= the 4m budget; not checked here.

Other attack results

• Duplicate execution on retry: AWS and Azure direct paths take claimExchangeSubmit before the detached call and never release it, so a retry after a cancelled request gets 409. The approval flow is guarded by the atomic pending -> processing transition (:2295, :2358), so a retry gets 409 and cannot reach the provider again. No double-execute found. (Existing tests in ri_exchange_idempotency_test.go still pass.)
• Terminal-save budget: saveCtx is created after the provider call returns, so a slow provider does not eat the 30s. retryCompleteWithPayment makes 3 attempts with no backoff inside that one budget; fine.
• Values lost from ctx: context.WithoutCancel keeps all values (logger, auth, tenant, request id). No test pins this, but the stdlib guarantees it and no code path in the diff swaps the ctx for Background().
• Missing deadline: both detached contexts carry a deadline; defer cancel is present at every site.
• Duplication/overlap: PR #895 (draft, internal/server/http.go + its test only) adds no shared helper, so there is no conflict. This is a third copy of purchase.detachedTimeout (internal/purchase/manager.go:236, same body). The PR declares this openly. Consolidation is a maintainer call, but a one-line shared helper would remove it; I did not treat it as blocking.
• Scope: 8 files, +280/-35 of which about 190 are new tests; under the 400-line cap for production change. handler_ri_exchange.go is 2789 lines, pre-existing over the 500 guideline.

What I attacked

• Ran the affected package tests at the SHA (go test ./internal/api -run 'RequestCancel|ExchangeIdempotency|ExecuteApprovedExchange|ExecuteExchange|ExecuteAzure|Stamp'): 103 passed, AWS_CONFIG_FILE=/dev/null.
• Mutation probes (reverted after each, worktree clean):
  • helper keeps cancellation (WithTimeout(ctx, d)): approval test FAILS (caught).
  • Azure ExecuteExchange back on request ctx: TestExecuteAzureExchange_RequestCancelDoesNotCancelExchange FAILS (caught).
  • AWS direct and approval provider call back on request ctx: both TestExecuteExchange_RequestCancel... and TestExecuteApprovedExchange_RequestCancel... FAIL (caught).
  • CompleteRIExchangeWithPayment on request ctx instead of saveCtx: FAILS (caught).
  • FailRIExchange on request ctx: ...RequestCancelStillRecordsFailure FAILS (caught).
  • stampRIExchangeApprover detach removed: all tests pass (SURVIVED, finding 2).
  • execution timeout 4m to 40m: passes (SURVIVED, finding 3, tautological).
• Fail-before/pass-after: the mutation probes above reproduce the pre-fix behaviour on each site and the new tests fail on each, so the regression coverage is real. One correction to the PR body: the direct AWS test cannot be run against the parent as-is, because the parent's executeExchange called exchange.ExecuteExchange directly and does not read executeExchangeFn; the seam is part of this PR. The reverted-site mutation is the equivalent proof.
• Did not run the repo-wide race suite or lint; CI has not run (see below).

Gate status (origin/main Review gate)

• Independent review naming the SHA: this comment (draft, so not a merge verdict).
• Actionable findings resolved: no, findings 1 and 2 are open.
• CI on the SHA: not run. Both workflows (pre-commit, CI - Build & Test) are action_required (external contributor, needs maintainer approval). CodeRabbit skipped (draft). CodeRabbit has no actionable items to count.
• Local real-path verification evidence on the PR: present from the author (mocks and stubs only, macOS, no real AWS/Azure; labeled honestly). I reproduced the package results above, not the repo-wide runs.
• Verdict recorded on the PR: this comment.
• Labels: the PR has none (author lacks permission). A maintainer must mirror #790: priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/bug, triaged.

Blocks ready

1. Fix finding 1 (pre-commit failExchange on a cancelled ctx strands a processing record) with a test.
2. Add the stamp-detach test (finding 2).
3. Approve and get both CI workflows green on the final SHA.
4. Maintainer applies the #790 labels.
5. Decide on and record the Lambda lambda_timeout >= 270 handling (validation or doc), since the default 30s makes this PR a no-op on the default AWS deployment.
6. Fresh review of the new head SHA after the above (any push restarts the gate).

[cristim]

Clarification of the review above (15e3ec8), after maintainer calibration. Only these block ready:

1. failExchange and the pending->processing transition still run on the request context (handler_ri_exchange.go:2295-2303, :2658-2667). A cancel after the transition leaves the record in "processing", so retries get 409 and the row keeps counting against the daily cap. Detach those calls.
2. Labels, now mirrored from fix(ri-exchange): run ExecuteExchange outside the request context #790.

Non-blocking: the untested stampRIExchangeApprover detach (surviving probe), the tautological deadline assertion, the stale comment, and consolidating the detached-context helper with #895 are nits or follow-ups.

[IAmJSD]

Response to the independent review of 15e3ec8. New head: d4fd03d (a new commit on top, no force-push).

1. Fixed in d4fd03d. failExchange now detaches its own write with ctxutil.DetachedWithTimeout(ctx, riExchangeTerminalSaveTimeout). That covers every pre-commit failure write after the pending -> processing transition: config load, missing region, cap config, reservation refusal or error, and a bad ceiling. A cancel in that window now marks the record failed instead of stranding it in processing. The provider call and success-path writes were already detached. New test TestExecuteApprovedExchange_RequestCancelBeforeReserveRecordsFailure: ReserveApprovedRIExchange cancels the request and returns context.Canceled, then the test expects FailRIExchange with a live context carrying the 30s budget, and checks the provider is never called. Before the fix it FAILS (Diff: 0: FAIL: (*context.cancelCtx=context.Background.WithCancel) not matched by func(context.Context) bool). After the fix it passes. Existing tests that pinned FailRIExchange to the exact request ctx now use mock.Anything.
2. Fixed in d4fd03d. New test TestStampRIExchangeApprover_SurvivesRequestCancel calls the stamp with an already-cancelled request context and expects StampRIExchangeApprovedBy with a live context. Mutation check: with the stamp detach reverted it FAILS (same cancelCtx ... not matched diff); restored, it passes.
3. Fixed in d4fd03d. assertDetached now checks against the literal 4*time.Minute, and liveCtx() also requires a deadline at most 30s away, which pins the terminal-save budget. Mutation check: changing riExchangeExecutionTimeout to 40m now makes 3 tests FAIL; it passed before this commit.
4. Fixed in d4fd03d. The Azure comment now says that only exhausting riExchangeExecutionTimeout mid-poll is ambiguous. A request cancel no longer reaches execCtx.
5. Documented in the PR body under "Direct execute paths persist nothing (known gap, out of scope)". No code change; a ledger row for the direct paths would be a follow-up if wanted.

Shared helper (the #895 review's finding 5). There is now a single copy:

• Location: internal/ctxutil/detached.go (new package ctxutil, with its own unit test).
• Signature: func DetachedWithTimeout(ctx context.Context, d time.Duration) (context.Context, context.CancelFunc).
• Body: context.WithTimeout(context.WithoutCancel(ctx), d), which is the same logic as before.
• purchase.detachedTimeout is removed, and executeAndFinalize uses the helper for both of its contexts. The exchange-local detachedRIExchangeContext is gone.
• fix(server): detach HTTP-triggered scheduled tasks from the request context #895 can switch to ctxutil.DetachedWithTimeout once this merges. I have not touched that branch.

Deployment notes (Lambda lambda_timeout default 30 vs the 270s requirement, the SIGTERM grace period, Cloud Run request_timeout): these are not changed here. They are blocks-ready item 5 and need a maintainer decision.

Verification at d4fd03d (macOS, mocks and stubs only, no real AWS or Azure; tests run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null):

• go build ./...: ok.
• go vet ./...: ok.
• go test -race -short -count=1 ./...: ok across the whole repo.
• go test ./internal/api/ -run 'RequestCancel|SurvivesRequestCancel' -count=1 -v: all 6 PASS.
• golangci-lint run --timeout=10m ./... (v2.10.1, the CI pin): 0 issues.

CI on the fork still needs maintainer approval. I have not marked the PR ready or applied labels.

[cristim]

Independent delta review of d4fd03d (previous: 15e3ec8): draft: no blocking findings (CI and gate items still open)

OLD_SHA is an ancestor of NEW_SHA. Base origin/main has not moved (merge-base is e003025), and the PR is mergeable.

Previous findings

#	Finding	Status	Evidence
1 (blocking)	Pre-commit failExchange writes on the request ctx strand a processing record	RESOLVED	handler_ri_exchange.go:2589-2592 now detaches inside failExchange with the 30s terminal-save budget. All 6 pre-commit callers (:2656-2681) go through it.
2	Untested stampRIExchangeApprover detach	RESOLVED	TestStampRIExchangeApprover_SurvivesRequestCancel fails when the detach is reverted.
3	Tautological deadline assertion	RESOLVED	assertDetached uses a literal 4*time.Minute. liveCtx() also requires a deadline within 30s.
4	Stale Azure comment	RESOLVED	handler_ri_exchange.go:1243-1248.
5	Helper consolidation	RESOLVED	One copy in internal/ctxutil/detached.go. The local exchange helper and purchase.detachedTimeout are removed. #895 can adopt it after merge.

Proof for finding 1 (the exact scenario: cancel after the pending -> processing transition, during ReserveApprovedRIExchange): TestExecuteApprovedExchange_RequestCancelBeforeReserveRecordsFailure cancels the request inside ReserveApprovedRIExchange, which returns context.Canceled. It expects FailRIExchange with a live ctx and a deadline within 30s, and expects zero provider calls. With the failExchange detach removed at NEW_SHA (equivalent to OLD_SHA behaviour for these call sites) the test FAILS; at NEW_SHA it passes. The author reports a FAIL on the parent as well.

New findings

1. NON-BLOCKING, handler_ri_exchange.go:2297 and :2360. The pending -> processing transition itself still runs on the request ctx. If the cancel lands while the transition's commit is in flight and the driver returns an error although the row committed, the record is stuck in processing. This is a much narrower window than the one fixed here, and no provider call has been made. Follow-up candidate: detach the transition, or treat a cancelled-ctx error as "re-read the row".
2. NON-BLOCKING, behaviour note. A cancel before GetGlobalConfig or ReserveApprovedRIExchange now records the exchange as failed (clean terminal state) instead of continuing. That is fail-loud and consistent with the issue. The caller retries by creating a new exchange, not by retrying the approval.

No double execution

• failExchange only writes the terminal failed state. It never calls the provider.
• Retry needs pending. The transition guard (:2297, :2360) returns 409 for processing, failed and completed. The new path moves the record from processing to failed and does not make it pending again.
• Direct AWS and Azure paths are unchanged apart from the helper rename. They keep claimExchangeSubmit, which is never released, so a retry gets 409.
• The nested detach (failExchange(saveCtx, ...) re-detaches) only resets the 30s budget. It cannot cancel or re-run anything.

What I attacked

• Tests at NEW_SHA (AWS_CONFIG_FILE=/dev/null): go build ./... ok. ./internal/api 3110 passed. ./internal/ctxutil and ./internal/purchase 682 passed. The 6 RequestCancel and SurvivesRequestCancel tests pass.
• Mutation probes (worktree reverted after each, clean):
  • failExchange detach removed: ...RequestCancelBeforeReserveRecordsFailure FAILS (caught).
  • stampRIExchangeApprover detach removed: TestStampRIExchangeApprover_SurvivesRequestCancel FAILS (caught).
  • riExchangeExecutionTimeout 4m to 40m: 3 tests FAIL (caught; it survived before).
• Read the full delta (git diff OLD NEW): production change is +40/-23 over 4 files, plus tests. Full PR vs main is 12 files, +411/-59, including tests.
• I did not re-run lint or the repo-wide race suite. The author reports both green.

Gate status (origin/main Review gate)

• Independent review naming the SHA: this comment (draft, so not a merge verdict).
• Actionable findings resolved: yes for the previous round. New items above are non-blocking.
• CI on the SHA: not run. pre-commit (38822610708) and CI - Build & Test (38822610718) are both action_required and need maintainer approval. CodeRabbit skipped the draft and has no threads.
• Local verification evidence on the PR: present from the author (mocks and stubs only, macOS, no real AWS or Azure). I reproduced the package results above.
• Labels: mirrored from #790 (priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/bug, triaged).
• Verdict recorded: this comment.

Blocks ready

1. A maintainer approves the two action_required runs and both go green on the final SHA.
2. Decide and record the Lambda lambda_timeout >= 270 handling (validation or deploy-guide note), since the default 30s makes this a no-op on the default AWS deployment (carried from the previous review).
3. The author marks the PR ready, then a fresh review of the final head (any push restarts the gate).

[cristim]

Gate record for d4fd03d (origin/main CLAUDE.md "Review gate"):

• Independent delta review of d4fd03d (previous 15e3ec8): the stuck-in-processing finding is resolved, because failExchange now writes on a detached 30s context. All nits are resolved, with one shared ctxutil.DetachedWithTimeout. No blocking findings.
  • Non-blocking: the pending->processing transition itself still uses the request context. The window is narrow, and no provider call has happened by then.
  • The Lambda 30s timeout is tracked separately in fix(iac): default Lambda timeout (30s) is shorter than detached in-process budgets #913.
• CI on d4fd03d: CI Success pass, Run pre-commit hooks pass. Merges cleanly onto main after fix(server): detach HTTP-triggered scheduled tasks from the request context #895, which ships the same internal/ctxutil helper.
• Local verification (macOS, mocks and stubs):
  • The new cancel-before-reserve test asserts FailRIExchange runs on a live context with zero provider calls. Removing the detach fails it.
  • The stamp-detach and 4m-literal tests are killed by their probes.
  • internal/api: 3110 tests pass. internal/ctxutil and internal/purchase: 682 tests pass.
  • No real AWS or Azure call.
• CR skipped: draft and quota-limited; covered by the independent review plus local verification.
• Labels mirrored from fix(ri-exchange): run ExecuteExchange outside the request context #790.

Marking ready (owner-authorized) and merging at d4fd03d with --match-head-commit.