Repository navigation
pubsub: stop leaking Redis connections from abandoned subscriptions - #724
Merged
Merged
Conversation
Every pub/sub subscription pins one dedicated Redis connection in the API process. redis-py 8 lowered the default asyncio pool size to 100, and once 100 subscriptions are held every other Redis command (INCR on subscribe, PUBLISH on node updates) fails with MaxConnectionsError, which surfaced as HTTP 500 for all writes and took the whole pipeline down. The subscriptions piled up because a client that subscribes and dies before its first listen() leaves last_poll unset, and cleanup_stale_subscriptions() only ever looked at last_poll, so such entries were never removed. A crash-looping external client exhausted the pool in about an hour. Fixes: - cleanup_stale_subscriptions() ages never-polled subscriptions from their creation time. - On every subscribe, evict that user's never-polled subscriptions older than abandoned_subscription_seconds (60s) and then enforce a cap of max_subscriptions_per_user (20) active subscriptions, returning HTTP 429 when exceeded. - Pass an explicit redis_max_connections (1000) to the Redis pool. - Close the previous PubSub when rebuilding after a connection error, and close the half-initialised PubSub if SUBSCRIBE fails, so neither path leaks a pooled connection. - Keep the keep-alive loop alive across transient Redis errors. - Map Redis MaxConnectionsError / ConnectionError to a 503 with Retry-After instead of a generic 500. All new settings are overridable through the environment via PubSubSettings. Unit tests cover the cleanup, eviction, limit, rebuild and keep-alive paths. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.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.
Every pub/sub subscription pins one dedicated Redis connection in the API process. redis-py 8 lowered the default asyncio pool size to 100, and once 100 subscriptions are held every other Redis command (INCR on subscribe, PUBLISH on node updates) fails with MaxConnectionsError, which surfaced as HTTP 500 for all writes and took the whole pipeline down.
The subscriptions piled up because a client that subscribes and dies before its first listen() leaves last_poll unset, and cleanup_stale_subscriptions() only ever looked at last_poll, so such entries were never removed. A crash-looping external client exhausted the pool in about an hour.
Fixes:
All new settings are overridable through the environment via PubSubSettings. Unit tests cover the cleanup, eviction, limit, rebuild and keep-alive paths.