A budget spent on the server's block no longer blames the client limit - #766
Merged
Merged
Conversation
When a gated operation runs out of its wait budget, the rejection told the
user "Too many requests (client limit 10/s); waited 10s" whatever it had
actually been waiting on. The branch that reports a spent budget passed
blocked=false unconditionally, so a server Retry-After that the gate slept
out until the deadline passed came back named as our own token bucket, with
"Re-run, or lower parallelism." underneath it.
That is wrong advice, not just an imprecise one. Parallelism is the knob for
the client limit; a Retry-After is the server asking everybody to wait, and
turning workers down does nothing about it. This is exactly why the CI
failure in "Check the clamp, not the clock, in the deadline test" read
"client limit 10/s" when the test had blocked on a server Retry-After — the
wording was left alone there because it is a call, not a refactor.
The rejection now names the limit that held the gate. A budget spent on a
server block says so and stops there:
Rate limited by the server; waited 10s
Re-run.
and a budget spent on our own bucket is unchanged. Which one is true is read
from the store at the point the error is produced: a Retry-After whose end
falls after this gate started queueing covered the wait, whether it is still
in force or lifted while being slept out. The block that lifted is the
common case — sleeping it out to within a wakeup of the deadline is how the
budget usually goes — so a check for a block still in force would miss the
failure this came from. A Retry-After that expired before the gate started
takes no blame.
The block that outlasts the budget outright is untouched: it is still
reported immediately as "Rate limited by the server; retry after 30s", which
already named the right limit and carries a number worth acting on.
Two of the three new tests are red without the fix, both server ones. The
client-limit test passes either way by construction — that behaviour is
unchanged and was already correct — and it is there so the fix cannot reach
past the case it is for.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The classification can misattribute concurrent Retry-After updates and may extend the wait beyond MaxWait.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates rate-limit timeout errors to distinguish server Retry-After blocks from client token limits.
Changes:
- Adds server/client attribution for exhausted wait budgets.
- Adds regression tests for active, expired, and absent server blocks.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/resilience/rate_limiter.go |
Classifies exhausted budgets and updates error messages. |
internal/resilience/gate_test.go |
Tests server- and client-side timeout messages. |
Review details
Suppressed comments (1)
internal/resilience/rate_limiter.go:203
RetryAfterUntil.After(start)does not prove this gate waited on that block. The store is shared, so another invocation can set a Retry-After while this gate is already sleeping for a client-token refill; a block can also expire betweenstartand the firsttake. Both cases relabel a client-budget timeout as server-side. Preserve whether the sleep that reached the deadline came fromtake'sblockedresult and use that observed cause instead of reconstructing it from the latest endpoint.
return state.RateLimiter.RetryAfterUntil.After(start)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The first pass named the limit by loading the store at the moment the error was produced and asking whether the Retry-After it held ended after this gate started queueing. That is a reconstruction, and the store is shared, so it can be wrong in the direction this card is about, only mirrored: another invocation's 429 lands while this gate sleeps for a refill of its own, and a client-side timeout is handed the server's name. A block that expired between the gate's start and its first take does the same. The reader is then told to sit and wait when they should lower parallelism. It also put a locking read on the path after the budget is spent. Store.Load takes the file lock with LockTimeout, 2s, so a contended store could stretch a 10s gate to 12s to choose a message. take already distinguishes the two: blocked is true only for a Retry-After still in force. waitSince now keeps that answer for the sleep it is in, and a spent budget reports the cause of the sleep that reached the deadline. No second read, no reconstruction, and the extra lock wait goes with it. The tests are red on both halves. The server one fails 10 of 10 against main, where a budget spent on a block reports the client limit. The client one fails 10 of 10 against the store-reading first pass, where an expired Retry-After in the shared store takes the blame for a wait the bucket imposed; it passes against main, since main never gets that wrong. Neither test times a sleep. A gate reaches a spent budget only by sleeping to the deadline, so both set the deadline to the instant the thing being waited for ends -- the block lifting, the token refilling -- and pin the jitter to zero. The sleep is then exactly that wait, and a timer that never fires early wakes past the deadline by construction. 20 of 20 each, and 5 of 5 under -race.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The server-attribution test retains a timing-sensitive 20 ms setup window that can fail under load.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Both new tests raced the clock they were testing. The server one set a Retry-After 20ms out and made that the deadline, with two store writes between the deadline being set and the take that had to observe the block: on a loaded runner the budget is gone before take ever looks, the gate never records a server block, and the test reports the client limit — the very defect it exists to catch, passing itself off as the fix failing. The client one had the same shape with a 100ms window, and both computed the reported wait from a real elapsed time that a slow enough machine rounds up a second. This is the 20ms margin from #763 again, the thing this whole card came out of. A gate reaches a spent budget by waking past its deadline, and no real timer can be asked for that. So the tests no longer ask: the limiter reads a clock it is given, pause is a variable beside jitter, and the test's pause does not sleep — it advances the held clock by the sleep it was handed plus a millisecond of lateness. The deadline boundary is now something the test states. Both run in 0.00s and there is no wall clock left in either. The seam buys the other half too. The interloping 429 that the client test needs — another invocation blocking the shared store while this gate sleeps for a refill of its own — is now written inside the pause, so the order is the test's rather than a goroutine's. That is the case the suppressed review comment named, and it is the one the wall-clock version could not reach at all. take reads the Retry-After against the transaction's own now while it is there, rather than against a second clock reading inside the state, which is what the refill beside it already did. Red again, and on more than before. The server case fails 10 of 10 with main's spent-budget branch restored. The client case fails 10 of 10 with the store-reading first pass restored, on the mid-sleep 429 specifically. 50 of 50 and 3 of 3 under -race with both in place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a gated operation runs out of its wait budget, the error it fails with always said the wait was our own:
Too many requests (client limit 10/s); waited 10s, withRe-run, or lower parallelism.underneath it. It said that even when what had actually held the request was a serverRetry-After— the server asking everybody to wait.Originally tracked in A wait that runs out blames the client limit even when the server asked for the wait.
That is wrong advice, not merely an imprecise message. Parallelism is the knob for the client limit; it does nothing about a server block, so a person reading this turns their workers down for a problem their workers did not cause. It is also why the CI failure in Check the clamp, not the clock, in the deadline test read "client limit 10/s" when the test had blocked on a
Retry-After. The wording was deliberately left alone there, since it is a call about user-facing text rather than a refactor. The message itself dates from Queue for a resilience slot instead of failing parallel invocations, where the gate learned to wait at all.A spent budget now names the limit that held the gate, and nothing else:
A budget spent on our own bucket reads exactly as it did before. Which of the two is true is observed where it happens rather than worked out afterwards: the gate already learns why each attempt failed when it makes it, and it keeps that answer for the sleep it is in, so a budget that runs out reports the cause of the sleep that reached the deadline. Reading it back from the resilience state at the end would have been wrong in the other direction — the state file is shared between invocations, so another process's 429 landing mid-sleep, or a block expiring before this gate ever looked, would have pinned a client-side timeout on the server. It would also have put a locking read, and up to two more seconds of lock wait, on the path after the budget is already gone.
The block that outlasts the budget outright is untouched. It is still reported immediately as
Rate limited by the server; retry after 30s, which already named the right limit and carries a number worth acting on.Both new tests are red, each against the thing it guards: the server one fails against
main, the client one against the first pass of this branch that read the state back. Neither races a clock. A gate only reaches a spent budget by waking past its deadline, which is the one thing a real timer cannot be asked for, so the limiter reads a clock it is given and the test's sleep does not sleep — it advances that clock by the sleep it was handed plus a millisecond of lateness. The overshoot, and the 429 that another invocation lands mid-sleep, are both stated by the test rather than hoped for from the scheduler.