Unified HTTP response handling and retry behavior, enabled by default - #149
Merged
Merged
Conversation
The 12 hour default was a backstop on the assumption a retry count would stop us reaching it. Across the SDKs rate-limited attempts are deliberately uncounted, so a duration is what actually bounds that path — C# was the only one with a count at all, and it sat at 100. Five minutes matches the counted path's ~4 minute worst case. MaxRetryInterval drops to 60s. At 300s it equalled the whole budget, so one sleep consumed it and the rate-limit path gave a single attempt. The wait is clamped to the end of the episode's budget, since ShouldUploadBatch checks elapsed time before waiting. One test assertion is deliberately inverted rather than adjusted: RateLimitCountIsReachedLongBeforeTheDurationBudget asserted that the count trips first and the duration is unreachable. That was the right invariant when the duration was 12 hours; now the duration is the operative limit and the count is the backstop, so it asserts the reverse and says why. 264 unit tests and the 82-test e2e suite pass. The e2e suite failed 6 tests on an earlier run under heavy machine load and passed clean on a re-run at normal speed — it is timing-sensitive, which is worth knowing for CI.
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.
RateLimitStartTime was persisted as an absolute timestamp and restored on load. That was survivable at a 12 hour budget and is not at 5 minutes: an app closed for longer than the budget now loads an already-expired episode and discards the batch on its first flush, having never retried it while actually running. The clock measures how long this process has spent retrying, and time while the process was not running is not that. It is now in-memory only — neither written nor read. The retry counts still persist, so a batch cannot be retried indefinitely across restarts; a relaunch gets a fresh duration budget but inherits the counted one. This only affects targets that persist state at all. Server-side use generally swaps the disk store for an in-memory one, where the field was already per-process. Also corrects a comment I wrote in the previous commit. It described the duration as a last-ditch guard reached long after the count — true at 12 hours, and the inverse of what the same commit made true. The duration is now what stops retrying and the count is the backstop, which is what the test one file over asserts. 264 unit tests and the 82-test e2e suite pass.
The clamp in HandleRateLimitResponse had no coverage at all. Deleting it outright left all 264 tests passing: the two test files this change touched check config validation and arithmetic on default constants, and neither calls the state machine. The one part of the change with any logic in it was the one part unguarded. Two tests now drive HandleResponse directly through a fake clock. The first opens an episode, advances to one second before the budget ends, sends a 429 asking for sixty, and asserts the wait lands on the episode deadline — it fails with the clamp removed. The second asserts a wait that comfortably fits is passed through untouched, so the clamp cannot degenerate into truncating every Retry-After to the deadline. 266 unit tests and the 82-test e2e suite pass.
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.
RateLimitConfig.MaxRetryCount bounds the rate-limit path alongside the duration, and it appeared in no changelog in either SDK generation: the rewrite dropped the old "MaxRetryCount 10 (was 100)" line, and the new entry names only BackoffConfig.MaxRetryCount. This is the one SDK where a rate-limited retry consumes a count at all, so a reader comparing behaviour had no way to find the number.
…ng its clock Four review points. The remaining-budget clamp is replaced by a drop. Shortening a Retry-After to fit the budget resumes inside the window the server named -- one it has already said it will not serve -- and ShouldUploadBatch drops the batch on the elapsed check immediately afterwards, so the shortened wait bought exactly one refused request. A non-retryable response no longer strands the episode clock. RateLimitStartTime was cleared on success and on the two budget-exceeded paths but not when a 4xx ended the batch, and nothing else clears it, so the next batch evaluated after the budget elapsed was dropped for a rate limit that had already ended, without ever being uploaded. Same defect python had; this SDK was wrongly cleared of it. The rate-limit path is bounded by a count as well as a duration, and the comment and changelog claimed the duration was the operative limit. It is not: the crossover sits at MaxRateLimitDuration / MaxRetryCount, 18 seconds at the defaults, so the count bounds every episode with a shorter Retry-After, which is most of them. Raising the budget to 30 minutes moved that crossover from 3s to 18s and made the claim materially wrong rather than marginally so. The test that was supposed to guard this asserted the duration was below the count's theoretical maximum span, 100 x 300s. True, and it passes while the count is the limit actually reached. It now asserts the crossover from both sides and pins the number. Validated() floored maxRateLimitDuration at 0 while its two siblings floor at 1 with comments explaining why. A CDN payload pushing 0 disabled rate-limit retrying entirely. 269 tests pass.
MichaelGHSeg
changed the title
didiergarcia
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
2 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 and makes retry behaviour configurable in code.
Defaults
RateLimitConfig.MaxRateLimitDurationis 30 minutes, was 12 hours. It must stay well aboveMaxRetryInterval(300s): at parity a 429 with no usableRetry-AfterwaitsMaxRetryIntervalby default, which consumes the whole budget, and sinceShouldUploadBatchchecks elapsed time before the wait the batch is dropped after a single attempt having stalled the entire pipeline for the duration.DefaultBudgetLeavesRoomForMoreThanOneMaximalWaitfails if that relationship is lost.Configuration.HttpConfigto null, so server-side users, who get no CDN settings, retried nothing. Mobile is unaffected: CDN settings still win.Behaviour
ShouldUploadBatchdrops the batch on the elapsed check straight afterwards, so it bought one refused request.RateLimitStartTimewas cleared on success and on the two budget-exceeded paths but not when a 4xx ended the batch, and nothing else clears it, so the next batch evaluated after the budget elapsed was dropped for a rate limit that had already ended, without ever being uploaded.Validated()floorsMaxRateLimitDurationat 1. At 0 — reachable from a CDN payload — every rate-limited batch was dropped on its first evaluation, silently disabling rate-limit retrying.Notes for review
MaxRateLimitDuration / MaxRetryCount— 18 seconds at the defaults — the count runs out first, above it the duration does. The comment and changelog previously claimed the duration was the operative limit, and the test guarding it asserted only that the duration was below the count's theoretical maximum span, which is true while the count is the limit actually reached. Both now state the crossover and pin it.RateLimitStartTimeis no longer persisted. It measures how long this process has spent retrying, and time while the process was not running is not that. The retry counts still persist, so a batch cannot be retried indefinitely across restarts.StatusCodeOverridesmerge over the defaults rather than replacing them. Overriding one status previously dropped the defaults for seven others, including letting 511 fall through toDefault5xxBehaviorand start being retried.CHANGELOG.md.