Bound the rate-limit retry budget at 5 minutes - #536
Open
MichaelGHSeg wants to merge 4 commits into
Open
MichaelGHSeg wants to merge 4 commits into
MichaelGHSeg wants to merge 4 commits into
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.
This branch has not been deployed
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
1 participant
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.
Why
Retry handling has two budgets. Responses carrying
Retry-Aftertake a rate-limit path bounded only by elapsed time and deliberately not counted against the retry count; everything else takes a counted exponential-backoff path.max_rate_limit_durationwas 12 hours, intended as a last-ditch guard that a retry count would stop us ever reaching. Nothing counts rate-limited attempts, so it was the only limit on that path rather than the guard behind one. A server that kept sendingRetry-Aftercould hold a batch for half a day — and with the default single consumer thread, hold up all delivery, fill the 10,000-message queue, and blockflush()andshutdown()for the same period.What
max_rate_limit_durationdefaults to 5 minutes, in line with the counted path's ~4 minute worst case, so neither failure mode costs much more than the other.Retry-Afteris capped at 60s rather than 300s. At 300s the cap equalled the whole budget, so a single wait consumed it and the rate-limit path degenerated to one attempt. 60s buys roughly five.Retry-Afteron top — negligible against 12 hours, double the bound against 5 minutes.Testing
135 unit tests,
ruff checkandruff formatclean, and the full 61-test shared e2e suite passes.test_rate_limit_wait_never_overshoots_the_budgetfails without the clamp, waiting 59.5s with ~1s of budget left.CI cannot currently run the e2e suite — the private
sdk-e2e-testscheckout lost its token during the CI-hardening work — so it was run locally.Notes
Release notes describe the behaviour this version has rather than the delta from an unreleased state, since the whole retry feature ships in this same version.