Repository navigation
fix(ri-exchange): answer the go exchange refusals with 422 instead of 500 - #1212
Conversation
… 500 The pinned cloud-commitments-go versions already carry go#203 (exchange.ErrQuoteNotUSD) and go#209 (azurecompute.ErrUnsupportedAppliedScope), and toAzureExchangeSources already builds sources from the owned reservations (LeanerCloud#475). But mapAWSExchangeError and mapAzureExchangeError only knew provider 4xx faults, so both refusals came back as a generic 500 ("exchange execution failed", "failed to price the exchange ...") and the AWS direct-execute audit row was recorded as unknown although the refusal happens before AcceptReservedInstancesExchangeQuote. Both mappers now check the exported refusal sentinels first and return a 422 carrying the go message, matching the Azure execute guardrails' currency-mismatch 422. openapi.yaml documents the Azure list fields (applied_scope_type, applied_scopes) and the new 422s. Refs LeanerCloud#521 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 5 billable files and costs up to $1.25. Or wait 8 minutes for your next included review. View limit details
Comment |
Resolve the conflict in internal/api/openapi.yaml: the POST /api/ri-exchange/execute description now keeps both main's out-of-scope 403 sentence (LeanerCloud#1171) and this branch's 422 non-USD quote sentence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged origin/main ( Conflict, and how I resolved it
New head: Verification on the merged tree (with
No PR-body change was needed. The PR stays a draft. 🤖 Generated with Claude Code |
|
Independent review 2 (claude-fleet, cold): APPROVE 2d0d8ec Findings (none blocking):
Against #521:
Attacked:
Verified locally, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review 2 (claude-fleet, cold), re-review: APPROVE c8e1017 This is a delta from my APPROVE at 2d0d8ec. The only new commit is a merge of origin/main (503eb2e):
Verified locally on a
My earlier findings and notes still apply unchanged. Same reviewer as the earlier review, re-reviewing the new head. Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
VERDICT: APPROVE c8e1017 Independent adversarial review of #1212 at c8e1017 (single commit 090493f on origin/main). The PR meets #521's acceptance line. go.mod on this head already pins pkg 541e05e5, providers/aws aeb8a6f7 and providers/azure 3def3f89, and those module sources contain both sentinels (pkg exchange/exchange.go:348, azure services/compute/exchange.go:283). Both refusals now return 422 through the real request builders, and the Azure sources still carry applied scope (pinned by TestAzureExchangeScopeRefused). Findings (none blocking):
Correctness checks against the pinned go source:
Attacked:
Not verified here: no real AWS account billed in a non-USD currency, and no real Azure reservation with a ManagementGroup scope. |
|
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: rA1. 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. |
|
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: 2 approving independent review(s) on |
|
Local verification at
|
Summary
#521 asked for a cloud-commitments-go bump plus refusal mapping. The bump is already on main, and so is the source-building item. The error mapping was still missing.
pkgis at541e05e5,providers/azureat3def3f89. In cloud-commitments-go,git merge-base --is-ancestorputs both81565ea1(go#203) and2d7f3f31(go#209) inside every pinned version.toAzureExchangeSourcesbuilds sources from theownedreservations, carries AppliedScopeType/AppliedScopes, and keeps the request QuantityTestAzureExchangeScopePreservedErrQuoteNotUSD/ErrUnsupportedAppliedScopeto a 4xx inmapAzureExchangeErrorandmapAWSExchangeErrorapplied_scope_type/applied_scopesin openapi.yamltype: object.TestAzureExchangeScopeRefusedran the realCalculateExchangebut only checkedrequire.Error, so a 500 still passedRoot cause
Before this PR,
mapAWSExchangeErrorandmapAzureExchangeErroronly recognized provider 4xx faults (smithy client-fault codes, and azcore 4xx throughisAzureClientError). Both go refusals are plain wrapped sentinels, so they fell through to the 500 path:POST /api/ri-exchange/execute.exchange.ExecuteExchangerefuses a quote in a currency other than USD, or a quote with no currency, usingexchange.ErrQuoteNotUSD. The refusal happens incheckInitialQuote/checkReQuote, beforeAcceptReservedInstancesExchangeQuote. The caller got500 "exchange execution failed", anddirectExchangeFailureStatusmarked the direct-execute audit row unknown, even though nothing had been submitted.CalculateExchangerefuses a source whose applied scope is missing, unsupported or Single-with-no-scope, or a set of sources with mixed scopes, usingazurecompute.ErrUnsupportedAppliedScope. It does this before any Azure call. The caller got500 "failed to find compatible offerings"or500 "failed to price the exchange before execution".Fix
internal/api/ri_exchange_refusals.goaddsexchangeRefusalError. It checks the two exported sentinels witherrors.Isand returns a 422 that carries the go message.checkAzureExchangeMoneyGuardrailsalready answers a quoted-currency mismatch with 422. A 4xx also makes the AWS audit row record failed instead of unknown, which is correct here because both refusals happen before the provider's commit call.AzureExchangeableReservationschema, includingapplied_scope_typeandapplied_scopes, used by theGET /api/ri-exchange/azure-instancesresponse.422is added to the responses ofPOST /api/ri-exchange/execute.About #361 (issue item 5)
With go#203 pinned,
ExecuteExchangerefuses any quote that is not in USD, on both the direct path and the approved path. So:max_payment_due_usdis now only ever compared against a USD amount.handlerAcceptedAmount(PaymentDueUSDStr, or the USD ceiling as fallback) is USD whenever an exchange completes.The new AWS test drives that real guardrail through the handler. On the approved path (
executeApprovedExchange), the refusal already ends infailExchangewith the go message as the reason, which is a clear answer, so I left it alone.Verification
All evidence below is unit tests against fakes. No real AWS or Azure calls were made.
New and changed tests:
TestExecuteExchange_NonUSDQuoteRefusedAs422(new). It runs the real goExecuteExchangeguardrail (exchange.NewExchangeClientFromAPI(fakeEC2).Execute, injected throughexecuteExchangeFn) with an EUR quote and with a quote that names no currency. It asserts:failedTestAzureExchangeScopeRefused(tightened). It runs the realazurecompute.Client.CalculateExchangevalidation for the missing, unsupported (ManagementGroup), single-without-scopes and mixed cases, on both preview and execute. It now asserts a 422 whose message contains "unsupported applied scope" and does not contain the generic "failed to". The foreign-billing case stays 403.TestMapExchangeErrors_Refusals(new). Both sentinels, wrapped, give 422 with the original message in both mappers. Provider 4xx stays 400. Other errors stay 500 with the opMsg.Fail before. I put back the
origin/mainhandler_ri_exchange.goand kept the new tests. 8TestAzureExchangeScopeRefusedsubtests failed withexpected: 422, actual: 500.TestExecuteExchange_NonUSDQuoteRefusedAs422failed withexpected: 422, actual: 500and"exchange execution failed" does not contain "not denominated in USD".TestMapExchangeErrors_Refusalsalso failed.Pass after. Commands:
I ran the tests with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause this machine's default AWS profile is invalid.Coverage gap. Nothing here was checked against a real AWS account billed in a currency other than USD, or a real Azure reservation with a ManagementGroup scope. The fakes return the same response shapes the go code parses.
Found but left alone
ErrUnsupportedPartition(go#566) is not in the pinned aws version. go#566 (92722e67) merged afteraeb8a6f7. It needs a separate four-module bump, so I didn't bump go.mod here.ErrNotUSDdoes not exist in the pinned modules. The only USD sentinel isexchange.ErrQuoteNotUSD, which is now mapped.common.ErrOutcomeUnknown(accept returned no exchange ID) is reachable on AWS execute. It correctly stays a 500 with audit status unknown, because the exchange may have committed.fmt.Errorfrefusals with no exported sentinel: "exchange is not valid: …", "paymentDue X exceeds max Y", "quote reported no PaymentDue", and the re-quote cap breach. Mapping them safely needs exported sentinels on the go side first; string matching would be fragile.azure ErrReservationOrderBlocked(purchase),common.ErrCommitmentPurchaseNotSupportedandErrInvalidIdempotencyToken.Frontend follow-up for the UI owner
No frontend files are changed.
frontend/src/riexchange.ts(around line 2428, "Previously quoted payment: USD ${record.payment_due}") always labels a stored quote "USD". With go#203, any exchange that actually executes is in USD. But a pending record'spayment_duecomes from the proposal-time quote, and older history rows were written before the guard. When the record carries a currency (part of the remaining #361 platform work), the label should use it instead of the hard-coded "USD". The 422 messages added here are plain text in the standard{"error": ...}body, so the existing error toast shows them with no UI change.Labels
A maintainer, please mirror the issue's triage labels:
priority/p2,severity/medium,urgency/this-sprint,type/chore,triaged,impact/few,effort/m.Closes #521
🤖 Generated with Claude Code