fix(omni): retry a mint the server rate-limited instead of refused - #432
fix(omni): retry a mint the server rate-limited instead of refused#432bdchatham wants to merge 1 commit into
Conversation
Measured today: rolling out several PRs in quick succession, each minting under the same client_id, tripped the server's own rate limiter -- POST /oauth/token answered 429 (slow_down). mintOnce classified every non-2xx status as a final refusal (the deliberate policy for a 5xx, which this endpoint has been measured returning for a genuinely bad credential), so a rate limit that clears in seconds read identically to a permanently wrong secret. 429 is carved out from that rule: the server names the grant valid and asks only that this wait, which has no "the credential is wrong" reading the way a 5xx might. mintToken now retries on it, honouring the server's own Retry-After when it names one (capped at 5s, so a confused or hostile server cannot hold one attempt hostage), and falling back to the existing fixed backoff when it names none. Not routed through retryUnreached: that helper's backoff is fixed, and this needs to prefer a server-named wait over it. mintToken goes back to its own loop for this reason. The lookup path's retry (using retryUnreached, unaffected) and its own APIError handling are unrelated: this is scoped to the mint path only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview 429 is now a separate, retryable outcome ( Tests cover multi-attempt success after 429 and Retry-After under/over the cap. Reviewed by Cursor Bugbot for commit 3c06554. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@seidroid review |
| // that has no such reading -- there is no credential a 429 could be | ||
| // naming as wrong. | ||
| return "", 0, rateLimited{ | ||
| err: fmt.Errorf("%w: the token endpoint returned 429 (%s), asking to slow down", |
There was a problem hiding this comment.
suggestion — This wraps the 429 in driver.ErrMint, which is the classification the surrounding comment argues a 429 is not. When the retries do not clear it, driver.go:289 matches errors.Is(err, ErrMint) and logs "configuration or request rejected before sending" with ExitConfig — an operator is told to go fix a credential the server just confirmed is valid, which is the second half of the failure the description opens with. Note the asymmetry with the other retryable class: unreached is deliberately not wrapped in ErrMint for exactly this reason (see the comment at the client.Do error), so an exhausted transport failure lands in ExitTransport while an exhausted rate limit lands in ExitConfig. Either give the rate limit its own arm ahead of the ErrConfig, ErrMint case, or leave it unwrapped the way unreached is.
| // as sooner or later than the server meant, and a delay in seconds carries no | ||
| // clock to skew. Empty, unparseable, zero, or negative all return zero, which | ||
| // callers read as "the server named none." | ||
| func parseRetryAfter(v string) time.Duration { |
There was a problem hiding this comment.
suggestion — Retry-After permits delay-seconds or an HTTP-date (RFC 9110 §10.2.3), and the date form returns zero here, which the caller reads as "the server named none" and answers with transportBackoff[0] — 0.5s in place of whatever the server asked for. The clock-skew rationale for not parsing the date is fair, and the assumption that this endpoint only sends delay-seconds is the load-bearing part: a CDN or gateway in front of the limiter is a plausible source of the other form, and if one appears the header is dropped with nothing said about it. Consider logging at debug when the header was present but unparsed, so the ignored value is visible rather than silent. (Raised independently by another reader.)
| srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| calls++ | ||
| if calls == 1 { | ||
| w.Header().Set("Retry-After", "0") |
There was a problem hiding this comment.
suggestion — Retry-After: 0 is read by parseRetryAfter as "named none", so this subtest exercises the fixed fallback, not the honoured path — as its own comment concedes. Combined with "over the cap", which asserts only elapsed > maxRateLimitWait+time.Second, neither subtest would fail if Retry-After were ignored outright: the whole contract in this test's name goes untested. A subtest sending a positive value below the cap and distinguishable from transportBackoff[0] (say Retry-After: 3, asserting elapsed is at least ~3s and under the cap) would pin it; the same lower-bound assertion in "over the cap" would pin that the cap value, and not the fallback, is what was used. (Seeded by another reader's note on this line.)
|
No REVIEW.md on 1. BlockingNothing blocks, unchanged from my first pass. 2. Non-blockingNothing material changed since my earlier review, so I am not restating the findings in prose. The pull request is at the same head commit ( 3. SummaryRe-fetched and re-read the diff and the merge tree rather than trusting memory: both are identical to the state I reviewed earlier in this session, so this is the same review with the same verdict — approve, with the driver-boundary classification of an exhausted rate limit as the most substantive of the non-blocking notes. Codex contributed the same two claims as before; both still hold against the unchanged code and both are kept and credited — the HTTP-date form of seidroid review · decision Findings: 0 blocking | 12 non-blocking | 3 posted inline | 1 pre-existing |
What happened
Rolling out platform#1672 (and the other rollout PRs alongside it) tripped the omnigent server's own rate limiter for the shared
client_id=seidroidcredential:```
{"level":"ERROR","msg":"configuration or request rejected before sending","error":"token exchange: the token endpoint returned 429 (slow_down)"}
```
Why this is a real, narrow gap
mintOnceclassifies every non-2xx status as a final refusal, never retried -- a deliberate policy, because this endpoint has been measured answering503for a genuinely malformed credential, and retrying by status would misreport a bad secret as a down deployment.That reasoning doesn't extend to
429. A rate limit is the one status that is a refusal's opposite: the server is saying the grant is valid, just not right now. There's no "the credential is wrong" reading of a 429.Fix
429is carved out of the blanket refusal rule.mintTokenretries on it, honoring the server's ownRetry-Afterwhen present (capped at 5s -- a confused or hostile server shouldn't be able to hold one attempt hostage), falling back to the existing fixed backoff (transportBackoff) when the server names none.Deliberately not routed through the shared
retryUnreachedhelper#427introduced -- that helper's backoff is fixed, and this needs to prefer a server-named wait over it.mintTokengoes back to owning its own loop for this specific reason. The session-lookup retry path is untouched.Tests
Two new:
TestMintRetriesOnRateLimit(retries and succeeds),TestMintHonoursRetryAfter(both the honored-and-capped case and the named-none-falls-back-to-fixed-backoff case).gofmt,go vet,go build,go test ./...all clean.🤖 Generated with Claude Code