Bound the rate-limit retry budget at 5 minutes - #284
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 a backstop on the assumption a retry count would stop us reaching it. Rate-limited attempts are deliberately uncounted, so it was the operative limit instead. With one worker thread that meant a stuck batch stalled all delivery for half a day, filled the 10,000-message queue, and blocked flush for the same period. Five minutes matches the counted path's ~4 minute worst case. rate_limit_retry_after_cap 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 delay is also clamped to the remaining budget, since the elapsed check runs before the wait. The RetryBudget spec helper now takes the shipped defaults from Defaults::Request rather than restating them, so a spec cannot pass against numbers the library no longer uses — which is exactly what happened when the cap moved and the helper kept its own 300. 214 examples, rubocop 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.
The budget test and the remaining-time calculation each took their own reading, so the budget could expire between them. That yields a negative remaining and a negative delay, and Kernel#sleep raises ArgumentError on a negative interval rather than returning immediately — so the worker thread would die rather than the episode ending. One reading now serves both, and the guard tests the remaining time directly. The condition is equivalent: remaining <= 0 is elapsed >= limit. Found while reviewing my own change. php samples once and is unaffected; python reuses one sample and guards on a positive wait; go and C# express the overshoot as a timer or a timestamp, where a negative is harmless. Two of the three new specs are worth noting as nearly useless: asserting the boundary passes against the two-reading version as well, because the defect is the gap between readings rather than the boundary itself. The third stubs the clock so the later reading falls outside the budget, and that one does fail against the old code, reporting the negative it would have handed to sleep. 217 examples, rubocop clean, 61-test e2e suite passes.
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.
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. Rate-limited attempts are uncounted, so it was the only limit on that path rather than the guard behind one. With a single worker thread, a stuck batch stalled all delivery for half a day, filled the 10,000-message queue — dropping new events with nothing louder than alogger.warn— and blockedflushfor the same period.What
max_rate_limit_durationdefaults to 5 minutes, in line with the counted path's ~4 minute worst case.rate_limit_retry_after_capdrops to 60s. At 300s it equalled the whole budget, so one wait consumed it and the path gave a single attempt.next_rate_limit_delaytakes a single clock reading for both the test and the clamp. Two readings let the budget expire between them and produce a negative delay, andKernel#sleepraisesArgumentErroron a negative rather than returning — so the worker thread would die rather than the episode ending.Testing
217 examples,
rubocopclean onlibandspec, and the full 61-test shared e2e suite passes.Three specs cover the clamp. The one that stubs the clock so the later reading falls outside the budget fails against a two-reading implementation, reporting the negative it would have handed to
sleep. The two that assert the boundary pass either way and are labelled as such — the defect is the gap between readings, not the boundary.The
RetryBudgetspec helper now readsDefaults::Request::*rather than restating the numbers, so a spec cannot pass against values the library no longer uses. That is how the old 300s cap slipped through.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. The exception is the backoff pacing change (base 100ms → 500ms, ceiling 10s → 60s, multiplier 1.5 → 2), which is a real change from 2.5.0 and carries its own upgrade note.