Bound the rate-limit retry budget at 30 minutes - #536
Merged
Merged
Conversation
The 12 hour default was designed as a last-ditch backstop, on the assumption that a retry count would stop us ever reaching it. Nothing counts rate-limited attempts — that exemption is deliberate, so a compliant server-directed wait does not burn the error budget — which left the duration as the only limit rather than the backstop. A server that kept sending Retry-After could hold a batch for half a day, and with the default single consumer thread that stalls all delivery and blocks flush() and shutdown() for the same period. Five minutes matches the counted path's ~4 minute worst case, so the two failure modes now cost about the same. Retry-After is capped at 60s rather than 300s. At 300s the cap equalled the whole budget, so a single sleep consumed it and the rate-limit path degenerated to one attempt. 60s buys roughly five. Segment serving a longer Retry-After would mean something has gone badly wrong upstream. The wait is also clamped to the remaining budget. The budget is checked before sleeping, so a check passing at 4:59 would sleep a full Retry-After on top — at 12 hours that was a rounding error, at 5 minutes it doubled the bound. The new test waits 59.5s without the clamp and 1s with it. 135 unit tests, ruff clean, 61-test e2e suite passes.
Three problems. The notes described changes between states that never shipped, so a customer read that a default moved from 12 hours to 5 minutes when only the 5 minutes was ever released. They referred to other SDKs, which means nothing to someone reading one library's notes. And they had accumulated over several passes into contradictions — Retry-After was documented as capped at both 300s and 60s, and the rate-limit budget as both 12 hours and 5 minutes. Rewritten to describe the behaviour this version has, in a consistent structure: upgrade notes that need action first, then retry handling, then everything else. Entries covering fixes to code that has not shipped are dropped, since there is nothing for a reader to compare against.
Applying the team convention to my own work from today. The comments explaining these changes had accumulated into potted histories: why a value had been twelve hours, what a test used to assert, which path used to be unreachable. Six months from now none of that resolves to anything — the diff and the commit messages hold it, and the comment should say why the code is the way it is. What stayed is what a maintainer would undo without it: that Kernel#sleep raises on a negative interval, that Thread#wakeup only interrupts a sleep already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an interrupt does not reach them, and that inverting one assertion would make the duration budget unreachable again. Comments only, no behaviour change.
Capping at 60s meant waiting less than the server asked for, which does not make the next attempt more likely to succeed — it just sends more requests at something already rate-limiting us. Against a Retry-After of 180s inside a 5 minute budget it turns 3 requests into 6; against 300s it turns 2 into 6. The cap is a guard against an absurd header, not a second budget. How long we keep trying is max_rate_limit_duration's job, and the clamp to the remaining budget already stops a single wait running past it, so the cap now rarely binds at all. It also bought nothing for the client this was partly aimed at: with no background thread, a shorter cap turns one long wait into several short ones for the same total blocking time and more requests. Tests that pinned 60 are updated, and each SDK gains one asserting that a Retry-After inside the cap is used as given rather than shortened.
The budget and the Retry-After cap were both 300s, and at parity the rate-limit path degenerates. A response with no usable Retry-After waits the cap by default, the elapsed check runs before the wait, so that one wait spends the whole budget and the batch is dropped having been tried once. A legitimate Retry-After of 300 does the same. The cap also stops binding: whatever is left of the budget is always the smaller term, so the cap can never be the value that clamps. Thirty minutes restores the relationship the two knobs are meant to have — the cap bounds one wait, the budget bounds the episode — and leaves room for several attempts. It costs nothing in normal operation, since the budget only binds when the server has been rate-limiting us for a long time, and in that case keeping the data is the point.
rate_limit_start_time marks an episode and is deliberately not cleared when a wait is served, so the budget measures the whole episode. But it was only cleared on success and on budget exhaustion. A request that completed and carried no rate-limit signal -- a 400, a 401 after a key rotation, a network error that exhausted counted retries -- left it set. The consumer outlives the batch, and upload() returns at the empty-batch guard before the budget block, so nothing else clears it. The next batch to arrive after the budget elapses is then dropped with "Rate limit duration exceeded", having never been sent and never been rate-limited. This is the only SDK where it can happen: go, ruby and php scope the episode to one batch and java clears it after the retry loop. The 12h default made it effectively unreachable; at 30 minutes a quiet app reaches it. Also widened the clamp test's margin. It sat 1s from the end of the budget, but next() blocks out the rest of upload_interval before the budget check, so any scheduling stall sent it down the drop path and failed on an empty `waits` -- a confusing failure for an unrelated reason. It now sits 30s out and also asserts the wait is non-zero, which the upper bound alone did not. 139 passed, ruff and format clean.
…ts window Two things from review. Client.DefaultConfig carried its own literal 300 and Client passes it into every Consumer it builds, so raising only the Consumer default left every real caller on the old value -- which equals the Retry-After cap, the exact configuration the change was meant to eliminate. HISTORY advertised 1800 and the suite was green. Both DefaultConfig entries now derive from the consumer constants, so they cannot drift apart again. The test that was supposed to cover this constructed a Consumer directly and so pinned the one default a caller never gets. It now goes through Client, and fails with "300 != 1800" against the old code. The remaining-budget clamp is replaced by a drop. Shortening a Retry-After to fit the budget sends the next request inside the window the server asked us to wait out -- a request it has already said it will not serve -- and since the budget is spent by then it would be the final attempt regardless. So the clamp bought one guaranteed-refused request per episode. Giving up at that point loses the same batch and sends one request fewer at a server that is already rate-limiting us. The clamp test became a pair: one that a wait which cannot fit drops without waiting or posting, and one that a wait which does fit is still honoured in full, since "never shorten" must not become "never wait". 140 passed, ruff and format clean.
MichaelGHSeg
changed the title
didiergarcia
previously approved these changes
Sep 25, 2026
The CI Lint job runs ruff format --check, which this file failed: the repo's line length is 140 and the new tests were hand-wrapped tighter. No behaviour change. Caught by CI rather than locally because the format check shared a shell line with the commit that followed it, so its output was never actually read.
Against the team convention: say why not what, no archaeology, keep a warning only where it stops someone undoing the thing it guards. - DEFAULT_MAX_RATE_LIMIT_DURATION: states the invariant as an instruction to whoever changes it next, and adds the second consequence of parity -- the cap stops binding, because the remaining budget is always the smaller term. - The drop branch: cut the rhetorical tail; the reason stands without it. - The episode-clearing comment: reordered so the hazard leads and the mechanism supports it, rather than the other way round. - "Same reasoning as above" now names the branch it refers to. - request.py: dropped a line that restated the constant in words, directly above the line that gives its reason. - test_client_defaults_to_the_documented_rate_limit_budget: rewritten in the present tense. It described how the bug had happened; it now states the constraint that makes asserting Consumer's default insufficient, which is what stops the test being "simplified" back. 140 passed, ruff and format clean.
The why and the hazard earn their place; the clauses walking through upload()'s control flow to connect them do not, since the reader can follow the code. Six lines to four, same two facts.
danieljackins
approved these changes
Sep 25, 2026
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
3 participants
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.
Bounds the rate-limit retry path, which was limited only by a 12 hour duration.
Defaults
max_rate_limit_durationis 30 minutes, was 12 hours. It must stay well above the 300sRetry-Aftercap: at parity a single maximal wait consumes the whole budget, the episode makes one attempt, and the cap stops binding because whatever is left of the budget is always the smaller term.test_default_rate_limit_budget_exceeds_the_retry_after_capfails if that relationship is lost.Retry-Aftercap stays at 300s. A shorter cap means retrying before the server said it would serve us, which just adds requests at something already rate-limiting us.Behaviour
Retry-Afterthat will not fit in what is left of the budget ends the episode rather than being shortened to fit. Shortening it resumes inside the window the server named — one it has already declined to serve — and the budget is spent by then, so that attempt would be the last either way. It bought exactly one refused request.rate_limit_start_timewas cleared only on success and on budget exhaustion, so a 429 followed by a 400 left it set; sinceConsumeroutlives the batch andupload()returns at the empty-batch guard before the budget block, the next batch to arrive after the budget elapsed was dropped for a rate limit that had already ended, without ever being sent. Python was the only SDK exposed — the others scope the episode per batch or clear it on every exit.Notes for review
Client.DefaultConfigcarried its own literal for both budgets and passes them into everyConsumer, so raising the consumer default alone left every real caller on the old value while the suite stayed green. Both now derive from the consumer constants. The test goes throughClientfor that reason; against the old code it fails300 != 1800.