Skip to content

fix(omni): retry a mint the server rate-limited instead of refused - #432

Open
bdchatham wants to merge 1 commit into
mainfrom
fix/mint-retries-on-rate-limit
Open

bdchatham wants to merge 1 commit into
mainfrom
fix/mint-retries-on-rate-limit

fix(omni): retry a mint the server rate-limited instead of refused

3c06554
Select commit
Loading
Failed to load commit list.
Seidroid / AI Review succeeded Sep 10, 2026 in 0s

12 findings, 1 pre-existing issue

Re-fetched: the diff and merge tree are byte-identical to the state reviewed earlier in this session (head 3c06554, merge 179388f), so nothing material changed and the verdict stands. The 429 carve-out and retry loop are correct; the non-blocking notes — chiefly that an exhausted rate limit still classifies as a configuration fault at the driver boundary — are carried forward unaddressed.

Non-blocking

  • Nothing material changed since the earlier review in this session: same head commit 3c06554, same merge tree 179388f, byte-identical 259-line diff and description, so every note below is carried forward rather than newly found.
  • With three attempts and a 5s cap, a server naming a conventional 60s rate-limit window is retried at 5s and 5s and then fails anyway, so the concurrent-burst case in the description is only partly addressed by this fix; the cap and the attempt budget together decide which rate limits are actually survivable.
  • No test covers a 429 that never clears: that the loop stops at transportAttempts and that the surfaced error is the "asking to slow down" message rather than the generic status line. TestMintDoesNotRetryARefusal pins the equivalent for a refusal.
  • Test-only: calls is written in the httptest handler goroutine and read in the test goroutine while srv.Close() is still deferred, so there is no synchronisation edge for -race; TestMintDoesNotRetryARefusal closes the server before reading calls, and the new tests deviate from that.
  • Test-only nit: the "over the cap" subtest spends a real ~5s of wall clock (mint_test.go:467, Retry-After: 3600), on top of the ~2.5s TestMintRetriesOnRateLimit costs on the fixed backoff.
  • Nit: client.go:130's parenthetical "transportBackoff's 0.5s/2s are both far under the 20s idle bound" now enumerates only part of the possible waits — a Retry-After retry can wait up to 5s. The conclusion still holds (5s < 20s); only the enumeration is stale.
  • Nit: host.go:314 says the run-key walk is retried "on the pattern [mintToken]", but mintToken no longer goes through retryUnreached, so that cross-reference now names a loop that has diverged from the helper host.go actually calls.
  • Nit: errRateLimited has no caller outside rateLimited.Is; its doc describes a use ("by anything that only needs to know it happened") that does not exist yet.
  • No Go toolchain is available in this sandbox, so gofmt/go vet/go build/go test were not run here — the description's claim that they are clean is unverified by this review.

Pre-existing

Already true on the base branch, not introduced here.

  • suggestionmintOnce issues a state-changing POST that the retry loop may repeat, so a request that was written and lost its response mints a token nobody reads — at-least-once, as retry.go's own doc warns. Harmless for short-lived tokens, but it means the server sees more grants than the driver uses, which matters if the limiter counts grants.