fix(redis): drop ssl_* kwargs when building a non-TLS async connection pool#34615
fix(redis): drop ssl_* kwargs when building a non-TLS async connection pool#34615devin-ai-integration[bot] wants to merge 1 commit into
Conversation
🤖 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:
|
|
|
Greptile SummaryThis PR prevents TLS-only Redis options from reaching plaintext asynchronous connection pools.
Confidence Score: 3/5The string-valued false configuration path must be fixed before merging because it still selects TLS for plaintext Redis. The new filtering only runs after the raw Files Needing Attention: litellm/_redis.py, tests/test_litellm/test_redis.py
|
| Filename | Overview |
|---|---|
| litellm/_redis.py | Strips TLS-only options from the non-TLS pool path, but string-valued false settings still select the TLS path. |
| tests/test_litellm/test_redis.py | Adds focused, network-free regression tests for plaintext and TLS options but does not cover a direct string "false" value. |
Reviews (1): Last reviewed commit: "fix(redis): drop ssl_* kwargs when build..." | Re-trigger Greptile
| else: | ||
| redis_kwargs = {k: v for k, v in redis_kwargs.items() if not k.startswith("ssl_")} |
There was a problem hiding this comment.
String false still selects TLS
When a direct caller or an environment-backed secret supplies ssl="false", the raw truthiness check selects SSLConnection instead of entering this filtering branch, causing plaintext Redis connections to attempt TLS and fail or time out. Normalize string boolean values before choosing the connection class so the non-TLS fix covers this supported configuration path.
Knowledge Base Used: Response Caching
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TLDR
Problem this solves:
ssl_check_hostnameHow it solves it:
ssl_*kwargs when the pool is non-TLSsslis truthy (SSLConnection)Relevant issues
Fixes #34614
Linear ticket
Pre-Submission checklist
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Local plaintext Redis 7.4.9 (
docker run -d --rm --name lit-redis -p 6379:6379 redis:7-alpine) and a proxy config carrying the samessl_check_hostnamethe Admin UI cache settings form always posts:Before, at 4ea58b7 with
litellm/_redis.pyreverted to its parent (git checkout HEAD~1 -- litellm/_redis.py):After, at 4ea58b7:
Type
🐛 Bug Fix
Changes
get_redis_connection_poolconsumesssland only swaps inasync_redis.SSLConnectionwhen it is truthy, but every otherssl_*option stayed in the kwargs that get handed to the pool's connection class. On a plaintext deployment those land onredis.asyncio.Connection, whoseAbstractConnection.__init__has nossl_check_hostname, so every cache write and every spend-counter increment raises aTypeError; the proxy stays HTTP-healthy, which is why it shows up as silently broken caching rather than a startup failure. The Admin UI cache settings form always postsssl_check_hostname, so a non-TLS setup hits this as soon as anything writes cache settingsTwo regression tests in
tests/test_litellm/test_redis.pycover both directions: a plain pool must carry nossl*connection kwargs and must be able to instantiate its connection class, and a TLS pool must still keepssl_check_hostname/ssl_cert_reqsFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/780c6d9bd4bb456397e53592ec022e78