Keep a RetryAfter halt until every backoff has finished - #5339
Open
0xSoftBoi wants to merge 2 commits into
Open
Conversation
AIORateLimiter documents that a RetryAfter halts all requests for retry_after + 0.1 seconds, but _retry_after_event was set in a finally block that runs for every request, not just the ones that cleared it. A request already past inner()'s wait() when the halt began would therefore release it on completion, and a shorter backoff would release a longer one still in effect. Under load there is nearly always another request in flight, so the halt was weakest exactly when it mattered. Track the number of requests currently backing off and set the event only when that reaches zero. Both cases are covered by regression tests; each fails without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #5338.
AIORateLimiterdocuments that aRetryAfterhalts all requests forretry_after + 0.1seconds._retry_after_eventis shared by the whole limiter, but it wasset()in afinallythat runs for every request — including requests that never cleared it. So any request already pastinner()'sawait self._retry_after_event.wait()when the halt began would release it on completion.Two reachable consequences:
Both need a request to be past the wait when the halt starts, which is the normal case under load — so the halt was weakest exactly when it mattered, and a 429 storm kept being fed.
Fix
Count the requests currently backing off and set the event only when that count reaches zero. A plain "only the clearer may set it" flag would fix the first case but not the second.
Tests
Two regression tests, both of which fail on
masterand pass here:test_retry_after_not_released_by_in_flight_requesttest_retry_after_not_released_by_shorter_backoffThey use a small
ScriptedRequesthelper mappingchat_id -> (latency, retry_after). The latency matters: the existingtest_delay_all_pending_on_retrystarts its second request after the halt is already in place, so that request blocks at thewait()and the bug is invisible. Reproducing it needs a request that is already in flight, which in turn needs the 429 to arrive after some delay, as it does in practice.tests/ext/test_ratelimiter.py: 18 passed, 1 skipped.ruff check,ruff format --checkandmypyare clean on both files.No behaviour change other than restoring what the docstring already promises, so I have not touched the docs.