Skip to content

fix(redis): only use SSLConnection when ssl is truthy in connection pool kwargs - #32825

Merged
yucheng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_fix_redis_ssl_falsy
Jul 11, 2026
Merged

fix(redis): only use SSLConnection when ssl is truthy in connection pool kwargs#32825
yucheng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_fix_redis_ssl_falsy

Conversation

@yucheng-berri

@yucheng-berri yucheng-berri commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Fixes the coordination Redis connection test and save flow added in #32661; without this, every request the new admin UI form sends against a non-TLS Redis fails

Linear ticket

Pre-Submission checklist

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem

Screenshots / Proof of Fix

Live proxy on localhost:4000 (dev_config with a plaintext Redis on 6379). Before the fix, the exact payload the coordination Redis UI sends (it always includes an explicit ssl boolean) times out:

$ curl -s -X POST http://localhost:4000/coordination_redis/settings/test \
    -H 'Authorization: Bearer sk-1234' -H 'Content-Type: application/json' \
    -d '{"settings":{"host":"127.0.0.1","port":6379,"ssl":false}}'
{"status":"unhealthy","error":"Connection timed out after 5.0s"}

After the fix, the same request succeeds, and ssl: true still attempts TLS and correctly reports the plaintext server as unreachable:

$ curl -s -X POST http://localhost:4000/coordination_redis/settings/test \
    -H 'Authorization: Bearer sk-1234' -H 'Content-Type: application/json' \
    -d '{"settings":{"host":"127.0.0.1","port":6379,"ssl":false}}'
{"status":"healthy","error":null}

$ curl -s -X POST http://localhost:4000/coordination_redis/settings/test \
    -H 'Authorization: Bearer sk-1234' -H 'Content-Type: application/json' \
    -d '{"settings":{"host":"127.0.0.1","port":6379,"ssl":true}}'
{"status":"unhealthy","error":"Connection timed out after 5.0s"}

UI check: on http://localhost:3000/caching, Coordination Redis tab, Test Connection with host 127.0.0.1, port 6379 and the SSL toggle off now shows the success toast instead of "Connection test failed: Connection timed out after 5.0s"

Type

🐛 Bug Fix

Changes

get_redis_connection_pool in litellm/_redis.py chose SSLConnection whenever the ssl key was present in the kwargs, regardless of its value, so ssl: false enabled TLS. The check dates to the original REDIS_SSL support (d1217b9), where the key was only ever set when the env var was literally "true", so presence and truth were the same thing back then. The coordination Redis UI from #32661 is the first caller that always sends an explicit ssl boolean, which makes the presence check select TLS against plaintext servers; the handshake hangs until the 5s ping timeout and every connection test and save from the UI fails

The fix switches the check to the value: if redis_kwargs.pop("ssl", False):. The pop is unconditional so a falsy ssl never leaks into BlockingConnectionPool kwargs, which plain Connection would reject. ssl: true behavior is unchanged

Regression tests in tests/test_litellm/test_redis.py assert that each falsy form (False, None, 0, "") yields a plain connection, that ssl=True still selects SSLConnection, and that omitting ssl keeps the plain default. With the fix reverted, exactly the four falsy cases fail


Note

Low Risk
Small, localized change to connection-pool construction with explicit regression tests; ssl=True behavior is unchanged.

Overview
get_redis_connection_pool no longer treats any present ssl key as “use TLS.” It now uses redis_kwargs.pop("ssl", False) so only a truthy ssl selects async_redis.SSLConnection; falsy values (false, null, 0, "") keep a plain connection and ssl is removed so it is not passed through to BlockingConnectionPool.

This fixes coordination Redis test/save from the admin UI, which always sends an explicit ssl boolean—previously ssl: false still opened TLS and timed out against plaintext Redis.

Regression tests cover falsy ssl, ssl=True, and omitting ssl.

Reviewed by Cursor Bugbot for commit 195c898. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a one-character logic bug in get_redis_connection_pool where the SSL connection class was chosen based on whether the ssl key was present in kwargs rather than whether its value was truthy. Because the admin UI introduced in #32661 always sends an explicit ssl boolean, ssl=false was silently enabling TLS, causing every connection test against a non-TLS Redis to time out.

  • litellm/_redis.py: Replaces if "ssl" in redis_kwargs with if redis_kwargs.pop("ssl", False) — a single-expression change that both gates on truthiness and unconditionally removes ssl from kwargs so it never leaks into BlockingConnectionPool.
  • tests/test_litellm/test_redis.py: Adds three parameterized mock tests covering all falsy forms (False, None, 0, ""), ssl=True, and the no-ssl-kwarg case; no real network calls are made.

Confidence Score: 5/5

Safe to merge — the change is a single-line targeted fix with no effect on callers that omit ssl or pass ssl=True.

The fix is minimal and correctly scoped: switching from a key-presence check to a value-truthiness check restores the intended behavior without touching any other code path. The ssl key is unconditionally consumed by pop, so it can never leak into BlockingConnectionPool for either truthy or falsy values. The new tests cover all falsy variants, the truthy case, and the historical omit-ssl default, all using mocks with no real network calls.

No files require special attention.

Important Files Changed

Filename Overview
litellm/_redis.py Fixes the SSL selection logic: changed presence check "ssl" in redis_kwargs to a truthiness check via redis_kwargs.pop("ssl", False), so falsy ssl values no longer incorrectly enable TLS.
tests/test_litellm/test_redis.py Adds three new mock-only unit tests covering all falsy ssl variants (False, None, 0, ""), ssl=True, and the omit-ssl case; no real network calls are made.

Reviews (1): Last reviewed commit: "fix(redis): only use SSLConnection when ..." | Re-trigger Greptile

@yucheng-berri
yucheng-berri force-pushed the litellm_fix_redis_ssl_falsy branch from f68f7a1 to 195c898 Compare July 10, 2026 21:29
@CLAassistant

CLAassistant commented Jul 10, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@yucheng-berri
yucheng-berri changed the base branch from litellm_coordination_redis_config to litellm_internal_staging July 10, 2026 21:29
@yucheng-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 195c898. Configure here.

@yucheng-berri
yucheng-berri force-pushed the litellm_fix_redis_ssl_falsy branch from 195c898 to 1881083 Compare July 10, 2026 23:22
@codspeed-hq

codspeed-hq Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_fix_redis_ssl_falsy (1881083) with litellm_internal_staging (eb7e4a5)1

Open in CodSpeed

Footnotes

  1. No successful run was found on litellm_internal_staging (4e3c437) during the generation of this report, so eb7e4a5 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@yucheng-berri
yucheng-berri merged commit 14338a2 into litellm_internal_staging Jul 11, 2026
126 checks passed
@yucheng-berri
yucheng-berri deleted the litellm_fix_redis_ssl_falsy branch July 11, 2026 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants