Repository navigation
fix(purchase): return the row from token approve and answer post-claim failures 502 - #1515
Conversation
…m failures 502 ApproveExecution (the email-link and SQS approve) discarded the row the approve funnel returned, so the token-mode handler could not tell a refusal before the claim from a provider failure after it: every post-claim failure fell through tokenActionError as a bare 500, and a 4-eyes denial from the funnel was a 500 too. ApproveExecution now returns the funnel's row, as ApproveAndExecute and RunPlannedPurchaseNow do: nil when the run never started, the final row otherwise. The token handler maps a failure with a row through the same executeFailure as session approve, direct execute and run-now (502 with execution_id and status for a failed or partially_completed run, 500 for an audit loss, the partial-run email sent from the returned row), and a 4-eyes denial to 403. Refusals before the claim keep tokenActionError. The SQS worker ignores the row. Nothing changes in what is bought or when. Closes LeanerCloud#532 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 22 billable files and costs up to $5.50. Or wait 59 minutes for your next included review. View limit details
Comment |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rF5. 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. |
|
VERDICT: APPROVE e28eaff Findings (no blockers):
Attacked:
|
|
Independent review 2 (claude-fleet, cold): REQUEST_CHANGES e28eaff Base 3bfc2fe; merge-tree onto origin/main ec78d85 (fetched this run) is clean (tree 86a74f62). No migrations in this PR (highest on main is 000147). Findings:
Attacked:
Verified locally (GOTOOLCHAIN=go1.26.10, cloud creds nulled):
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rF7. 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. |
|
Update to our review of e28eaff: we are withdrawing the APPROVE above. One blocking finding has been reported privately to the maintainers, so this PR should not merge at this SHA. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 3 approving independent review(s) on |
Slice 3 of the #532 plan (plan comment). Slice 1 merged as #644, slices 2 and 4 as #1257. This is the last open item, so this PR closes the issue.
Closes #532
Root cause
Manager.ApproveExecution(the email-link approve and the SQS approve worker) threw away the rowtransitionApproveAndExecutereturned (_, revocationToken, err := ...). Without the row,executeTokenApprovecould not tell a refusal before the claim from a failure after it. Every post-claim failure fell throughtokenActionError's default as a bare 500 with noexecution_idor status, even though the purchase had run and money may have moved. A 4-eyes denial raised inside the funnel also came back as a 500. Session approve, direct execute and run-now have used the row since #644.Fix
purchase.Manager.ApproveExecutionnow returns(*config.PurchaseExecution, string, error), with the same contract asApproveAndExecuteandRunPlannedPurchaseNow. The row is nil when the run never started (bad, expired or rotated token, orphan, 4-eyes denial, lost claim). Otherwise it is the claimed row with its final status, including on an execution error.api.executeTokenApprovesends failures to a newtokenApproveFailure:executeFailure, as session approve, direct execute and run-now already use. A failed or partially_completed run answers 502 with{execution_id, status}. An audit loss answers 500. A claimed-row refusal (sec(purchase): constraint checks read recs before the CAS; execution buys from the post-CAS row (TOCTOU) #415) keeps its code and now carries the row details. A partially_completed run still gets its executed email with the revocation token, now built from the returned row rather than a DB re-fetch, the same as the session path.tokenActionErroras before (403 invalid or rotated token, 410 expired, 409 lost claim or wrong status, 500 otherwise).handleApproveMessage) ignores the row. Its error, and therefore its retry behavior, is unchanged.PurchaseManagerInterfaceininternal/api/types.goandinternal/server/interfaces.go,testutil.MockPurchaseManagerand the api test mock now use the new signature. The api mock returns the refused row on a claimed-row refusal, as itsApproveAndExecutealready does.openapi.yaml: the approve 502 now covers both session and token mode, and the 409 text says a lost claim is 409 in either mode.Money path: nothing changes in what is bought or when. The claim, the token pin in the CAS (#1190), the 4-eyes gate, the claimed-row check and the run are untouched. Only the return value, the HTTP status, the response body details, and the source of the partial-run email row changed. The success response stays
{"status":"completed"}.How each #532 item is now covered
executeFailureError)executed_atstamped before the claimManager.DirectExecutestamps after the CAS)TestSingleClaim_StrandedRedriveMidSyncRunCannotBuyAgain)Verification
Local Linux VM, Go 1.26.9. Everything ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. Unit tests use testify mocks. The integration test uses real PostgreSQL (testcontainers) with the ramp-step fixture's fake provider. Nothing calls a cloud API.Fail before (unit, mutation-style). I temporarily put back main's handler logic (
_, revocationToken, approveErr := ..., thentokenActionError), keeping only the new signature, and rango test ./internal/api/ -count=1 -run 'TestTokenApprove_FailureMappedByCause':provider_failure_after_the_claim:expected a client error, got AWS RI purchase failed(bare 500)final_status_not_saved:expected a client error, got audit loss: ...(on main this was already a 500, just a plain one)4-eyes_denial_before_the_claim:expected a client error, got approval declined: ...(500, not 403)Under the same revert,
TestExecutedNotification_PartialTokenApproveEmailsRevocationTokenfails withThis call was unexpectedon the secondGetExecutionByID: main re-fetched the row for the partial email, and the test pins that the email comes from the returned row. The lost-claim, rotated-token and store-error cases pass both before and after. They are regression guards that the pre-claim mapping did not change.Pass after:
go test ./internal/api/ -count=1: ok.go test ./internal/purchase/ -count=1: ok.go test ./internal/server/ -count=1: ok.go test ./internal/mocks/ -count=1: ok.TestTokenApprove_FailureMappedByCause(6 cases),TestExecutedNotification_PartialTokenApproveEmailsRevocationToken(502 with statuspartially_completed, one email from the returned row, no re-fetch),TestManager_ApproveExecution_ReturnsRowOnExecutionFailure(failed row returned), plus row assertions added toTestManager_ApproveExecution_Success(rowcompleted) andTestManager_ApproveExecution_TransitionFails(nil row).go test -tags integration -count=1 -timeout 20m -run 'TestApproveExecution_ReturnsTheFinalRow|TestApproveExecution_TokenRotatedBeforeClaimIsRefused|TestApproveAndExecute_PartialRunMintsRevocationToken|TestApproveAndExecute_FailedRunMintsNoToken' ./internal/purchase/: all pass. The newTestApproveExecution_ReturnsTheFinalRowdrives the token approve on a fan-out root that ends partially_completed: it returns that row, the stored status matches, and the stored hash matches the returned token. It also drives a failed single-account row (returns thefailedrow, no token) and then a second approve of the same row, which loses the CAS withErrExecutionNotInExpectedStatus, returns no row and buys nothing. This test pins the new contract. It cannot fail-before because the old signature does not compile against it.go build ./...,go vet ./...,go vet -tags integration ./...: clean.run --allow-parallel-runners --build-tags integration ./internal/purchase/ ./internal/api/ ./internal/server/ ./internal/testutil/: 0 issues.gocyclo -over 10 -ignore "_test\.go"on the changed non-test files: silent.gofmt -l internal/: clean.No DB or migration change. No new concurrency surface: the claim CAS is unchanged, and the lost-claim case is covered by the integration test above.
Combined-tree verification at local queue commit
2be43e1e= origin/mainb9271e88+fix/issue-363-next,fix/issue-177-next,fix/issue-919-next,fix/issue-313-next,fix/issue-532-nextandfix/issue-316-nextmerged in that order. Passed:go build ./...;go vetwith 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 integrationoninternal/api,internal/auth,internal/config,internal/database/postgres/migrations,internal/purchase,internal/serverandinternal/testutil; the git-secrets scan andscripts/test-git-secrets-allowlist.sh.Out of scope (noted, not changed)
emailTokenApproveOutcome), even though the row is now returned. I left this alone to keep the slice to status and response changes. Switching it to the returned row is a small follow-up.executeSessionApproveemails the pre-approveexecutionsnapshot rather than the returnedfinalrow. This is pre-existing cosmetic email content and does not affect the revoke link.ApproveExecution's own read, the token path returnsexecution not found: <id>without wrappingconfig.ErrNotFound, so it answers 500 rather than 409. The handler has just loaded the row, so this is very unlikely.Frontend follow-up for the UI owner
None required. The approve endpoint already answers 502 in session mode (since #644), and the client shows the server's error message. Token-mode approvals now get the same 502 body (
execution_id,status) on a failed or partial run.Maintainer: please mirror the issue's triage labels onto this PR:
priority/p2,severity/medium,urgency/this-quarter,impact/few,effort/l,type/bug.🤖 Generated with Claude Code
Behavior changes found in review (added after review)