Fix Redis client lifecycle to prevent connection leaks in concurrent requests#653
Conversation
- Create client per request in run() method, not in __post_init__ - Add validation requiring exactly one of client or client_factory - Only close factory-created clients, not user-provided ones - Move deprecation warning to __post_init__ for early feedback - Update tests to verify new behavior and lifecycle management Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## issues/643/redis-client #653 +/- ##
============================================================
+ Coverage 95.88% 100.00% +4.11%
============================================================
Files 13 13
Lines 729 737 +8
============================================================
+ Hits 699 737 +38
+ Misses 30 0 -30 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical bug in the Redis health check where a single client instance was reused across multiple requests, causing connection leaks and race conditions when the client was closed after each check. The fix aligns the Redis health check with the patterns already established in RabbitMQ and Kafka health checks.
Changes:
- Moved client creation from
__post_init__to therun()method to create fresh clients per request - Added
client_factoryparameter that's called per health check request (similar to RabbitMQ/Kafka patterns) - Added parameter validation in
__post_init__to ensure exactly one ofclientorclient_factoryis provided - Implemented proper deprecation handling where user-provided clients are not closed during the deprecation period
- Added comprehensive test coverage for all lifecycle and validation scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| health_check/contrib/redis.py | Implements factory pattern for client creation per request, adds validation logic in __post_init__, and ensures proper lifecycle management with should_close flag |
| tests/contrib/test_redis.py | Updates and adds tests to verify factory is called per request, user-provided clients aren't closed, validation works correctly, and each client is properly closed after use |
Redis health check reused a single client instance across multiple requests, causing connection leaks and race conditions when the client was closed after each check.
Changes:
run()method instead of__post_init__, matching RabbitMQ/Kafka patternclient_factoryparameter that's called per health check requestclientorclient_factorywith clear error messages in__post_init__Before:
After:
The health check instance can now safely handle concurrent requests without client reuse issues.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.