Recover prefect-redis connection pool after a Redis outage#22479
Draft
devin-ai-integration[bot] wants to merge 5 commits into
Draft
Recover prefect-redis connection pool after a Redis outage#22479devin-ai-integration[bot] wants to merge 5 commits into
devin-ai-integration[bot] wants to merge 5 commits into
Conversation
Give RedisMessagingSettings sane default socket_timeout (60s) and socket_connect_timeout (10s) so blocking reads against an unreachable broker fail fast instead of hanging forever, and disconnect/close cached clients in clear_cached_clients() so stuck in-use connections are reaped rather than permanently exhausting the pool (MaxConnectionsError). Warn when a consumer block interval exceeds the socket timeout. Co-authored-by: Alexander Streed <alex.s@prefect.io> Co-Authored-By: alex.s <ajstreed1@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-authored-by: Alexander Streed <alex.s@prefect.io> Co-Authored-By: alex.s <ajstreed1@gmail.com>
Co-authored-by: Alexander Streed <alex.s@prefect.io> Co-Authored-By: alex.s <ajstreed1@gmail.com>
Address review: apply an explicit bounded retry policy consistently to both the Redis(...) and Redis.from_url() construction paths (read timeouts are no longer retried so blocking reads fail fast); detach entries before awaiting in clear_cached_clients so a concurrently-inserted replacement survives; and treat BLOCK 0 as infinite while reading the effective socket_timeout off the client (respecting URL overrides) in the consumer warning. Co-authored-by: Alexander Streed <alex.s@prefect.io> Co-Authored-By: alex.s <ajstreed1@gmail.com>
Fail the simulated read once (not twice) so recovery doesn't span multiple backoff sleeps, and always cancel/await the background consumer task so it can't leak into later tests (was intermittently timing out test_trimming_streams on Python 3.10 CI). Co-authored-by: Alexander Streed <alex.s@prefect.io> Co-Authored-By: alex.s <ajstreed1@gmail.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 #22478
This PR makes the
prefect-redisshared async client recover after the Redis broker becomes unreachable, instead of permanently raisingredis.exceptions.MaxConnectionsError: Too many connectionsuntil a process restart.Root cause
The cached client's single
ConnectionPoolhad several problems that combined into a permanent failure:socket_timeout/socket_connect_timeoutdefaulted toNone, so a blockingXREADGROUPon a dead socket hangs forever. The connection stays in_in_use_connections, never released. As reads pile up, the pool hits its default cap (100) and every op raisesMaxConnectionsError— even after Redis is healthy again. The indefinite hang also means the consumer'sexcept RedisErrorreconnect path never fires.Redis(...)path a dead broker can therefore occupy a pool slot for ~11 ×socket_timeoutbefore the consumer ever sees the error, while theRedis.from_url()path gets a different (near-zero) policy — so the two construction paths behaved inconsistently.clear_cached_clients()only did_client_cache.clear(); the orphaned pool (with its stuck in-use connections) was never disconnected.Changes
RedisMessagingSettings: defaultsocket_timeout=60.0andsocket_connect_timeout=10.0(still overridable/None-able). A read against an unreachable broker now fails fast, so its connection is released and the reconnect path engages.socket_timeoutis deliberately well above the default consumerblock(1s) so idle blocking reads don't time out spuriously.Redis(...)andRedis.from_url()construction paths (inget_async_redis_clientandasync_redis_from_settings) via a newretriessetting (default 3). OnlyConnectionErroris retried — read timeouts are not retried, so a blocking read fails fast into the consumer's reconnect loop instead of pinning the pool slot for many multiples of the socket timeout.clear_cached_clients(): detaches the current-loop entries it will close (_client_cache.pop) before the firstawait, then disconnects (disconnect(inuse_connections=True)) andaclose()s only that detached snapshot. It no longer clears the cache wholesale, so a replacement client cached concurrently while it awaits survives (previously the freshly recovered pool could be orphaned). Clients bound to other event loops are left untouched.BLOCK 0as infinite (it out-lives any finite socket timeout, so it's flagged even though its numeric value is 0), and reads the effectivesocket_timeoutoff the client's connection kwargs — respecting URL query overrides rather than only the settings default. Moved intorun()where the actual client is available.Before/after (reproduced with a black-hole TCP proxy in front of Redis)
Before — after the broker recovers, the pool stays saturated:
After — blocking reads fail fast, connections are reaped, and the client resumes:
Relationship to #22457
#22457 (open, draft) reworks the client lifecycle (removes the global cache, adds
managed_async_redis_client()) and closes the related FD-leak issue #22442. It does not change the socket-timeout defaults or the retry policy, so on its own it does not resolve this outage-recovery scenario (withsocket_timeout=Nonethe blocking read still never raises, so the managed client is never closed). The timeout/retry fixes here are required regardless of that refactor; theclear_cached_clients()change targets the current (still-cached) code onmain.Checklist
closes #22478)clear_cached_clients()disconnect/close + concurrent-replacement race, and the block/timeout warning (incl.BLOCK 0and URL-override cases)Link to Devin session: https://app.devin.ai/sessions/f4e8b325760a40da9c792f28edf69a21
Requested by: @desertaxle