fix(http): release global rate limit lock when a request is cancelled - #1585
Open
zachnieto wants to merge 2 commits into
Open
fix(http): release global rate limit lock when a request is cancelled#1585zachnieto wants to merge 2 commits into
zachnieto wants to merge 2 commits into
Conversation
On a global 429 the request clears `_global_over`, sleeps for `retry_after`, then sets it again. The set was not protected, so a request cancelled while sleeping never re-set the event. Once that happens every subsequent request blocks forever on `await self._global_over.wait()` near the top of `request`, and the client cannot recover without a restart. The gateway is unaffected, so the bot stays connected and looks healthy while no HTTP request ever completes again. Cancellation during that window is easy to hit in normal use, for example a send wrapped in `asyncio.wait_for`, a task cancelled on shutdown, or any user code calling `Task.cancel()`. Move the set into a `finally` so the lock is always released. Co-Authored-By: Claude Opus 5 <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.
Summary
A request that is cancelled while sleeping off a global rate limit permanently deadlocks the entire HTTP client.
In
HTTPClient.request:The
set()is not protected. If the task is cancelled duringasyncio.sleep(retry_after),CancelledErrorpropagates out and the event is never re-set.From then on, every request in the process blocks forever near the top of
request:There is no recovery path. The client has to be restarted.
Why this is easy to hit
The gateway is a separate websocket and is unaffected, so the bot stays connected, keeps receiving events, and keeps passing any health check that does not exercise the HTTP API. It simply stops being able to send anything. That makes it look like an intermittent "some commands stopped responding" problem rather than a deadlock.
Cancellation inside that window happens in ordinary usage:
asyncio.wait_forthat times outTask.cancel()It needs a global 429 to coincide with a cancellation, so it is rare, but it is unrecoverable when it does happen. We hit it in production on a bot across a few thousand guilds: online, healthy, gateway fine, and not a single outbound request completing until a restart.
Fix
Move the release into a
finally, so the global lock is dropped whether the sleep finishes or the request is cancelled. Behaviour is otherwise unchanged: on the normal path the event is still set right after the sleep, before the retry.Tests
Added
test_global_rate_limit_released_on_cancellationtotests/test_http.py. It drives a realHTTPClient.requestagainst a stub session that returns a global 429, cancels the request while it is sleeping, and asserts_global_overends up set.Verified it fails on
masterand passes with this change:Notes
discord.pyhas the same issue in the equivalent code path, in case it is worth flagging upstream.Changelog fragment added as
changelog/1585.bugfix.rst.Checklist
uv run nox -s lintuv run nox -s pyright