mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
fix(redis): coerce env var string types and fix param discovery through decorator wrappers (#30644)
* fix(redis): coerce env var string types and fix param discovery through decorator wrappers
inspect.getfullargspec doesn't work on redis.Redis/redis.RedisCluster because
their __init__ is wrapped by @deprecated_args, which replaces the explicit
signature with *args/**kwargs internally. getfullargspec returns an empty arg
list, so _get_redis_kwargs and _get_redis_cluster_kwargs silently dropped every
real constructor parameter not in their hand-picked include_args set --
cluster_error_retry_attempts and connection_error_retry_attempts among them, so
an operator's configured retry bound never reached the Redis Cluster client and
it fell back to redis-py's own default instead.
Rebased onto litellm_internal_staging, which had independently added
_init_arg_names (MRO-walking, inspect.unwrap-based) for the same class of bug
in _get_redis_url_kwargs. Reused that pattern (as _unwrapped_init_args, without
the MRO walk: redis.Redis/RedisCluster declare every real parameter directly on
their own __init__, and MRO-walking breaks the tests here that mock the class
with autospec=True, since inspect.getmro needs a real __mro__) rather than
introducing a second, differently-shaped fix for the same problem.
_get_redis_cluster_kwargs now also honors its own client argument instead of
ignoring it, so the async cluster client's own extra constructor kwargs
(cluster_error_retry_attempts, connection_error_retry_attempts,
decode_responses, ...) are no longer filtered out by introspecting the sync
class regardless of which client is actually built.
Also fixes environment variables and Helm --set values always arriving as
strings: redis-py 8.x changed health_check_interval's arithmetic to require a
real number, so a stringified value raised TypeError on every Redis operation
instead of connecting. _coerce_redis_kwargs_types coerces to each parameter's
declared type at the end of _get_redis_client_logic, with an explicit type
table for max_connections/socket_timeout/socket_connect_timeout since redis-py
8.x changed the timeout defaults from None to int 5, which would otherwise
make a fractional value fail int() and get dropped.
Co-authored-by: mangabits <1457532+mangabits@users.noreply.github.com>
* ci: verify redis-py client version compatibility across a version matrix
* test(redis): assert an async-only cluster kwarg every matrix version declares
connection_error_retry_attempts is on the async cluster constructor in redis-py
5.x only; 6.0 removed it in favor of retry. The 6.4.0, 7.4.1 and 8.0.1 legs were
failing on that missing parameter name rather than on the behavior under test,
while the allow-list itself was doing the right thing on all four versions.
decode_responses is async-cluster-only on every version the matrix covers, so it
stands in for the same property: the sync cluster class takes it through **kwargs
and never names it in its signature. Reverting _get_redis_cluster_kwargs to ignore
its client argument still fails both tests on 5.3.1 and 8.0.1.
test_async_cluster_passes_async_only_kwargs now builds the real async cluster
client and reads connection_kwargs off it, so it no longer needs a patched class
factory; the constructor does no I/O. The retry-attempts test keeps its patch,
since redis-py >= 6 stores no cluster_error_retry_attempts attribute on the built
client and the constructor call is the only place the forwarded value shows up.
The _get_redis_cluster_kwargs docstring cited the same two parameters as its
examples of async-only kwargs, which is what made the test look reasonable;
cluster_error_retry_attempts is on both classes and connection_error_retry_attempts
is gone from 6.0 on, so it now names decode_responses instead.
* test(redis): drop internal patches from the kwarg coercion tests
The test-quality gate flagged the new patch() calls on litellm internals these
tests added. Three of them faked litellm._redis.inspect.signature with a MagicMock
to hand _coerce_redis_kwargs_types a synthetic parameter; that function already
takes a client argument, so they pass stub functions instead, matching the
_redis_signature_8x idiom the file uses elsewhere. The fourth patched
_redis_kwargs_from_environment to {} to prove _get_redis_client_logic raises
without a host or url, which clearing the real env keys through
_get_redis_env_kwarg_mapping does without pinning the test to that call.
Both files now sit one TQ008 below the merge base rather than six above it.
* fix(redis): keep the sync client construction inside the basedpyright budget
_get_redis_client_logic now returns dict[str, object] rather than an untyped
dict, which is the honest type for operator-supplied config, but it turns the
33 reportUnknownArgumentType errors at redis.Redis(**redis_kwargs) into 33
reportArgumentType errors plus one reportCallIssue, both over their budget.
No static type fits: redis-py's constructor declares 40-odd differently typed
parameters and the values arrive from config and env, so the allow-list and
coercion above are derived from that same signature and redis-py validates each
value itself at runtime.
The two suppressions name their exact rule and carry that reason. The file ends
up 42 basedpyright errors below the merge base, with reportArgumentType and
reportCallIssue back at the base counts of 3 and 0.
* fix(redis): coerce cluster-only and None-default bool kwargs
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---------
Co-authored-by: mangabits <1457532+mangabits@users.noreply.github.com>
Co-authored-by: yassin <yassin@berri.ai>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
502b3a2f79
commit
d1320404fe
4 changed files with 443 additions and 27 deletions
77
.github/workflows/test-redis-compat.yml
vendored
Normal file
77
.github/workflows/test-redis-compat.yml
vendored
Normal file
|
|
@ -0,0 +1,77 @@
|
|||
name: "Unit Tests: Redis Client Version Compatibility"
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
branches:
|
||||
- main
|
||||
- litellm_internal_staging
|
||||
- litellm_oss_staging
|
||||
- "litellm_**"
|
||||
paths:
|
||||
- "litellm/_redis.py"
|
||||
- "litellm/_redis_credential_provider.py"
|
||||
- "tests/test_litellm/test_redis.py"
|
||||
- "tests/test_litellm/caching/test_redis_connection_pool.py"
|
||||
- ".github/workflows/test-redis-compat.yml"
|
||||
- "pyproject.toml"
|
||||
- "uv.lock"
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
|
||||
cancel-in-progress: true
|
||||
|
||||
jobs:
|
||||
redis-compat:
|
||||
name: "redis-py ${{ matrix.redis-version }}"
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 15
|
||||
|
||||
strategy:
|
||||
fail-fast: false
|
||||
matrix:
|
||||
# 5.3.1 is the version pinned in uv.lock (redisvl caps it below 6); the
|
||||
# newer legs prove the inspect.signature introspection in litellm/_redis.py
|
||||
# keeps extracting kwargs on the redis-py releases people actually run now.
|
||||
# Only the exact release 6.0.0 is skipped: rq (pulled by the proxy extra)
|
||||
# specifies `redis != 6`, which excludes 6.0.0 alone, so 6.4.0 stands in
|
||||
# for the 6.x line.
|
||||
redis-version: ["5.3.1", "6.4.0", "7.4.1", "8.0.1"]
|
||||
|
||||
steps:
|
||||
- uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0
|
||||
with:
|
||||
persist-credentials: false
|
||||
|
||||
- name: Set up Python
|
||||
uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0
|
||||
with:
|
||||
python-version: "3.12"
|
||||
|
||||
- name: Set up uv
|
||||
uses: ./.github/actions/setup-uv-with-retries
|
||||
with:
|
||||
version: "0.10.9"
|
||||
|
||||
- name: Install dependencies
|
||||
run: |
|
||||
.github/scripts/uv_sync_with_retries.sh --frozen --group ci --group proxy-dev --extra google --extra proxy --extra semantic-router
|
||||
|
||||
- name: Pin redis-py to the matrix version
|
||||
env:
|
||||
REDIS_VERSION: ${{ matrix.redis-version }}
|
||||
run: |
|
||||
uv pip install "redis==${REDIS_VERSION:?}"
|
||||
uv run --no-sync python -c "import redis; assert redis.__version__ == '${REDIS_VERSION:?}', redis.__version__; print('redis-py', redis.__version__)"
|
||||
|
||||
- name: Run redis unit tests
|
||||
run: |
|
||||
uv run --no-sync pytest \
|
||||
tests/test_litellm/test_redis.py \
|
||||
tests/test_litellm/caching/test_redis_connection_pool.py \
|
||||
--tb=short -vv \
|
||||
--reruns 2 \
|
||||
--reruns-delay 1 \
|
||||
--durations=20
|
||||
|
|
@ -13,6 +13,7 @@ import json
|
|||
# s/o [@Frank Colson](https://www.linkedin.com/in/frank-colson-422b9b183/) for this redis implementation
|
||||
import os
|
||||
from collections.abc import Callable, Mapping
|
||||
from types import MappingProxyType
|
||||
from typing import Final
|
||||
from urllib.parse import urlsplit, urlunsplit
|
||||
|
||||
|
|
@ -38,9 +39,25 @@ from ._logging import verbose_logger
|
|||
AZURE_REDIS_SCOPE: Final = "https://redis.azure.com/.default"
|
||||
|
||||
|
||||
def _get_redis_kwargs():
|
||||
arg_spec: Final = inspect.getfullargspec(redis.Redis)
|
||||
def _unwrapped_init_args(cls: type) -> frozenset[str]:
|
||||
"""Every parameter on a single class's own ``__init__``, decorator-unwrapped.
|
||||
|
||||
Unlike ``_init_arg_names`` below, this does not walk the MRO: ``redis.Redis``
|
||||
and ``redis.RedisCluster`` (sync and async) each declare every real
|
||||
constructor parameter directly on their own ``__init__``, so MRO-walking is
|
||||
unnecessary — and it actively breaks the several tests here that mock the
|
||||
class with ``patch(..., autospec=True)``, since ``inspect.getmro`` needs a
|
||||
real ``__mro__`` that an autospec'd stand-in for a class does not provide.
|
||||
|
||||
Still unwraps first: redis-py >= 7.4 decorates these ``__init__``s with
|
||||
``@deprecated_args`` too, which the same class of bug as ``_init_arg_names``
|
||||
would otherwise silently empty this allowlist through (see its docstring).
|
||||
"""
|
||||
spec: Final = inspect.getfullargspec(inspect.unwrap(cls.__init__))
|
||||
return frozenset(spec.args + spec.kwonlyargs)
|
||||
|
||||
|
||||
def _get_redis_kwargs():
|
||||
# Only allow primitive arguments
|
||||
exclude_args: Final = {
|
||||
"self",
|
||||
|
|
@ -60,7 +77,7 @@ def _get_redis_kwargs():
|
|||
"azure_client_secret",
|
||||
}
|
||||
|
||||
available_args: Final = {x for x in arg_spec.args if x not in exclude_args} | include_args
|
||||
available_args: Final = {x for x in _unwrapped_init_args(redis.Redis) if x not in exclude_args} | include_args
|
||||
|
||||
return available_args
|
||||
|
||||
|
|
@ -120,15 +137,23 @@ def _get_redis_url_kwargs(client: type | None = None) -> tuple[str, ...]:
|
|||
return tuple(x for x in _init_arg_names(connection_cls) if x not in exclude_args) + include_args
|
||||
|
||||
|
||||
def _get_redis_cluster_kwargs(client=None):
|
||||
def _get_redis_cluster_kwargs(client: type | None = None):
|
||||
"""Config kwargs the target cluster client's constructor actually accepts.
|
||||
|
||||
Defaults to the sync ``redis.RedisCluster``, but the async cluster client
|
||||
(``redis.asyncio.cluster.RedisCluster``) declares connection settings such as
|
||||
``decode_responses`` on its own constructor, where the sync class takes them
|
||||
through ``**kwargs`` and so never names them in its signature. Introspecting
|
||||
only the sync class regardless of which client is actually built silently
|
||||
drops those for every async cluster caller.
|
||||
"""
|
||||
if client is None:
|
||||
client = redis.Redis.from_url
|
||||
arg_spec: Final = inspect.getfullargspec(redis.RedisCluster)
|
||||
client = redis.RedisCluster
|
||||
|
||||
# Only allow primitive arguments
|
||||
exclude_args: Final = {"self", "connection_pool", "retry", "host", "port", "startup_nodes"}
|
||||
|
||||
available_args = {x for x in arg_spec.args if x not in exclude_args}
|
||||
available_args = {x for x in _unwrapped_init_args(client) if x not in exclude_args}
|
||||
available_args |= {
|
||||
"password",
|
||||
"username",
|
||||
|
|
@ -161,6 +186,79 @@ def _get_redis_env_kwarg_mapping():
|
|||
return {f"{PREFIX}{x.upper()}": x for x in _get_redis_kwargs() if x not in exclude_from_environment}
|
||||
|
||||
|
||||
def _str_to_bool(value: str) -> bool:
|
||||
return value.lower() in ("true", "1", "yes")
|
||||
|
||||
|
||||
def _coerce_redis_kwargs_types(
|
||||
redis_kwargs: Mapping[str, object],
|
||||
client: type | tuple[type, ...] = redis.Redis,
|
||||
) -> dict[str, object]: # mutable-ok: a caller mutates the returned kwargs before constructing its client
|
||||
"""Coerces string values to the numeric/boolean type ``client``'s constructor
|
||||
declares for that parameter. ``client`` may be a tuple of client classes; a
|
||||
parameter's type is taken from the first signature that declares it, which
|
||||
lets cluster callers coerce cluster-only kwargs such as
|
||||
``cluster_error_retry_attempts`` alongside the shared connection kwargs.
|
||||
|
||||
Environment variables are always strings, and Helm ``--set`` stringifies values
|
||||
too, so a config value like ``health_check_interval`` or ``socket_timeout``
|
||||
can arrive as ``"30"``/``"5.5"`` rather than a real number. redis-py's own
|
||||
connection-health-check arithmetic (``loop.time() + self.health_check_interval``)
|
||||
then raises ``TypeError`` on every Redis operation instead of connecting.
|
||||
|
||||
``max_connections``, ``socket_timeout``, and ``socket_connect_timeout`` use an
|
||||
explicit target type rather than the parameter's own signature default: redis-py
|
||||
8.x changed the timeout defaults from ``None`` to int ``5``, so inferring the
|
||||
type from the default would make a fractional ``"5.5"`` fail ``int()`` and get
|
||||
silently dropped on 8.x while working on older versions. ``socket_keepalive``
|
||||
is explicit too: its signature default is ``None``, which carries no type to
|
||||
infer from, and leaving it a string makes ``"false"`` truthy.
|
||||
"""
|
||||
signatures: Final = tuple(inspect.signature(c) for c in (client if isinstance(client, tuple) else (client,)))
|
||||
explicit_param_types: Final = MappingProxyType(
|
||||
{
|
||||
"max_connections": int,
|
||||
"socket_timeout": float,
|
||||
"socket_connect_timeout": float,
|
||||
"socket_keepalive": bool,
|
||||
}
|
||||
)
|
||||
result: Final = dict(redis_kwargs) # mutable-ok: per-key try/except coercion below needs to drop individual keys
|
||||
for key, value in redis_kwargs.items():
|
||||
if not isinstance(value, str):
|
||||
continue
|
||||
param = next((sig.parameters[key] for sig in signatures if key in sig.parameters), None)
|
||||
if param is None:
|
||||
continue
|
||||
explicit_type = explicit_param_types.get(key)
|
||||
if explicit_type is bool:
|
||||
result[key] = _str_to_bool(value)
|
||||
continue
|
||||
if explicit_type is not None:
|
||||
try:
|
||||
result[key] = explicit_type(value)
|
||||
except (ValueError, TypeError):
|
||||
del result[key]
|
||||
continue
|
||||
default: object = param.default # pyright: ignore[reportAny] # inspect.Parameter.default is stubbed as Any
|
||||
if default is inspect.Parameter.empty:
|
||||
continue
|
||||
# bool must be checked before int, since bool subclasses int
|
||||
if isinstance(default, bool):
|
||||
result[key] = _str_to_bool(value)
|
||||
elif isinstance(default, int):
|
||||
try:
|
||||
result[key] = int(value)
|
||||
except (ValueError, TypeError):
|
||||
del result[key]
|
||||
elif isinstance(default, float):
|
||||
try:
|
||||
result[key] = float(value)
|
||||
except (ValueError, TypeError):
|
||||
del result[key]
|
||||
return result
|
||||
|
||||
|
||||
def _redis_kwargs_from_environment():
|
||||
mapping: Final = _get_redis_env_kwarg_mapping()
|
||||
|
||||
|
|
@ -505,7 +603,12 @@ def _get_redis_client_logic(**env_overrides):
|
|||
raise ValueError("Either 'host' or 'url' must be specified for redis.")
|
||||
|
||||
# litellm.print_verbose(f"redis_kwargs: {redis_kwargs}")
|
||||
return redis_kwargs
|
||||
coercion_client: Final = (
|
||||
(redis.Redis, redis.RedisCluster, async_redis.RedisCluster)
|
||||
if redis_kwargs.get("startup_nodes")
|
||||
else redis.Redis
|
||||
)
|
||||
return _coerce_redis_kwargs_types(redis_kwargs, client=coercion_client)
|
||||
|
||||
|
||||
def init_redis_cluster(redis_kwargs) -> redis.RedisCluster:
|
||||
|
|
@ -657,7 +760,9 @@ def get_redis_client(**env_overrides):
|
|||
if "sentinel_nodes" in redis_kwargs and "service_name" in redis_kwargs:
|
||||
return _init_redis_sentinel(redis_kwargs)
|
||||
|
||||
return redis.Redis(**redis_kwargs)
|
||||
return redis.Redis( # pyright: ignore[reportCallIssue] # object-valued kwargs match no overload statically
|
||||
**redis_kwargs, # pyright: ignore[reportArgumentType] # allow-listed and coerced against this signature
|
||||
)
|
||||
|
||||
|
||||
def get_redis_async_client(
|
||||
|
|
@ -669,7 +774,7 @@ def get_redis_async_client(
|
|||
if "startup_nodes" in redis_kwargs:
|
||||
from redis.cluster import ClusterNode
|
||||
|
||||
args = _get_redis_cluster_kwargs()
|
||||
args = _get_redis_cluster_kwargs(async_redis.RedisCluster)
|
||||
cluster_kwargs: Final = {}
|
||||
for arg in redis_kwargs:
|
||||
if arg in args:
|
||||
|
|
|
|||
|
|
@ -1,15 +1,14 @@
|
|||
"""
|
||||
Regression tests for Redis connection pool leak fixes (RC1-RC5).
|
||||
|
||||
Tests are pure unit tests — no Redis server required.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
import redis.asyncio as async_redis
|
||||
|
||||
from litellm._redis import get_redis_async_client, get_redis_connection_pool
|
||||
from litellm._redis import (
|
||||
_coerce_redis_kwargs_types,
|
||||
_get_redis_client_logic,
|
||||
_get_redis_env_kwarg_mapping,
|
||||
get_redis_async_client,
|
||||
get_redis_connection_pool,
|
||||
)
|
||||
|
||||
|
||||
def test_url_config_uses_passed_pool():
|
||||
|
|
@ -60,16 +59,14 @@ def test_max_connections_url_config_string_value(monkeypatch):
|
|||
assert pool.max_connections == 25
|
||||
|
||||
|
||||
def test_max_connections_url_config_invalid_value():
|
||||
"""Invalid max_connections should be silently ignored, falling back
|
||||
to the pool default (50 for BlockingConnectionPool)."""
|
||||
with patch("litellm._redis._get_redis_client_logic") as mock_logic:
|
||||
mock_logic.return_value = {
|
||||
"url": "redis://localhost:6379/0",
|
||||
"max_connections": "not_a_number",
|
||||
}
|
||||
def test_max_connections_url_config_invalid_value(monkeypatch):
|
||||
"""Invalid max_connections from an env var should be silently dropped,
|
||||
falling back to the pool default (50 for BlockingConnectionPool)."""
|
||||
monkeypatch.setenv("REDIS_URL", "redis://localhost:6379/0")
|
||||
monkeypatch.delenv("REDIS_HOST", raising=False)
|
||||
monkeypatch.setenv("REDIS_MAX_CONNECTIONS", "not_a_number")
|
||||
|
||||
pool = get_redis_connection_pool()
|
||||
pool = get_redis_connection_pool()
|
||||
|
||||
# BlockingConnectionPool default is 50
|
||||
assert pool.max_connections == 50
|
||||
|
|
@ -128,3 +125,173 @@ async def test_disconnect_idempotent():
|
|||
|
||||
await cache.disconnect()
|
||||
await cache.disconnect() # should not raise
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_int():
|
||||
"""String values for int-typed Redis params are coerced to int."""
|
||||
result = _coerce_redis_kwargs_types({"health_check_interval": "30", "port": "6380", "db": "1"})
|
||||
assert result["health_check_interval"] == 30
|
||||
assert isinstance(result["health_check_interval"], int)
|
||||
assert result["port"] == 6380
|
||||
assert result["db"] == 1
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_bool():
|
||||
"""String values for bool-typed Redis params are coerced to bool."""
|
||||
result = _coerce_redis_kwargs_types({"ssl": "true", "decode_responses": "false"})
|
||||
assert result["ssl"] is True
|
||||
assert result["decode_responses"] is False
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_none_default_numeric():
|
||||
"""String values for known None-default numeric params are coerced."""
|
||||
result = _coerce_redis_kwargs_types({"max_connections": "20", "socket_timeout": "5.5"})
|
||||
assert result["max_connections"] == 20
|
||||
assert isinstance(result["max_connections"], int)
|
||||
assert result["socket_timeout"] == 5.5
|
||||
assert isinstance(result["socket_timeout"], float)
|
||||
|
||||
|
||||
def _redis_signature_pre_8x(
|
||||
socket_timeout=None,
|
||||
socket_connect_timeout=None,
|
||||
max_connections=None,
|
||||
health_check_interval=0,
|
||||
):
|
||||
"""Stand-in for the redis-py <= 7.x Redis signature, where the timeout defaults are None."""
|
||||
|
||||
|
||||
def _redis_signature_8x(
|
||||
socket_timeout=5,
|
||||
socket_connect_timeout=5,
|
||||
max_connections=None,
|
||||
health_check_interval=0,
|
||||
):
|
||||
"""Stand-in for the redis-py 8.x Redis signature, where the timeout defaults became int 5."""
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"client",
|
||||
[_redis_signature_pre_8x, _redis_signature_8x],
|
||||
ids=["redis-py<=7.x", "redis-py-8.x"],
|
||||
)
|
||||
def test_coerce_fractional_socket_timeout_survives_signature_default_change(client):
|
||||
"""redis-py 8.x changed socket_timeout's default from None to int 5. Deriving the
|
||||
target type from the signature default made int("5.5") raise, so the key was dropped
|
||||
and REDIS_SOCKET_TIMEOUT=5.5 silently disappeared on 8.x."""
|
||||
result = _coerce_redis_kwargs_types(
|
||||
{"socket_timeout": "5.5", "socket_connect_timeout": "2.5", "max_connections": "20"},
|
||||
client=client,
|
||||
)
|
||||
|
||||
assert result["socket_timeout"] == pytest.approx(5.5)
|
||||
assert isinstance(result["socket_timeout"], float)
|
||||
assert result["socket_connect_timeout"] == pytest.approx(2.5)
|
||||
assert isinstance(result["socket_connect_timeout"], float)
|
||||
assert result["max_connections"] == 20
|
||||
assert isinstance(result["max_connections"], int)
|
||||
|
||||
|
||||
def test_coerce_invalid_socket_timeout_is_still_dropped():
|
||||
"""Garbage must not survive the explicit-type path; Redis falls back to its own default."""
|
||||
result = _coerce_redis_kwargs_types({"socket_timeout": "not_a_number"}, client=_redis_signature_8x)
|
||||
|
||||
assert "socket_timeout" not in result
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_invalid_drops_key():
|
||||
"""A string that cannot be coerced to the expected numeric type is dropped."""
|
||||
result = _coerce_redis_kwargs_types({"health_check_interval": "not_a_number"})
|
||||
assert "health_check_interval" not in result
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_non_string_unchanged():
|
||||
"""Non-string values pass through without modification."""
|
||||
result = _coerce_redis_kwargs_types({"health_check_interval": 30, "ssl": True})
|
||||
assert result["health_check_interval"] == 30
|
||||
assert result["ssl"] is True
|
||||
|
||||
|
||||
def test_health_check_interval_from_env_is_int(monkeypatch):
|
||||
monkeypatch.setenv("REDIS_HOST", "localhost")
|
||||
monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "30")
|
||||
|
||||
pool = get_redis_connection_pool()
|
||||
|
||||
assert pool is not None
|
||||
interval = pool.connection_kwargs.get("health_check_interval")
|
||||
assert interval == 30
|
||||
assert isinstance(interval, int), f"Expected int, got {type(interval)}: {interval!r}"
|
||||
|
||||
|
||||
def _signature_without_defaults(testkey):
|
||||
"""Stand-in for a client whose parameter declares no default at all."""
|
||||
|
||||
|
||||
def _signature_with_float_default(myparam=1.0):
|
||||
"""Stand-in for a client whose parameter declares a float default."""
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_empty_default_param_unchanged():
|
||||
"""String params whose signature entry has no default (inspect.Parameter.empty) are left as-is."""
|
||||
result = _coerce_redis_kwargs_types({"testkey": "some_value"}, client=_signature_without_defaults)
|
||||
|
||||
assert result["testkey"] == "some_value"
|
||||
assert isinstance(result["testkey"], str)
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_float_valid():
|
||||
"""String values for params whose signature default is a float are coerced to float."""
|
||||
result = _coerce_redis_kwargs_types({"myparam": "3.14"}, client=_signature_with_float_default)
|
||||
|
||||
assert result["myparam"] == pytest.approx(3.14)
|
||||
assert isinstance(result["myparam"], float)
|
||||
|
||||
|
||||
def test_coerce_redis_kwargs_types_float_invalid_drops_key():
|
||||
"""An unconvertible string for a float-default param is dropped from the result."""
|
||||
result = _coerce_redis_kwargs_types({"myparam": "not_a_float"}, client=_signature_with_float_default)
|
||||
|
||||
assert "myparam" not in result
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("raw", "expected"),
|
||||
[("false", False), ("true", True), ("0", False), ("1", True)],
|
||||
)
|
||||
def test_coerce_socket_keepalive_string(raw, expected):
|
||||
"""socket_keepalive's signature default is None, so it needs an explicit bool
|
||||
coercion: a leftover "false" string is truthy and enables keepalive."""
|
||||
result = _coerce_redis_kwargs_types({"socket_keepalive": raw})
|
||||
|
||||
assert result["socket_keepalive"] is expected
|
||||
|
||||
|
||||
def test_get_redis_client_logic_coerces_cluster_only_kwargs(monkeypatch):
|
||||
"""Cluster-only kwargs (absent from redis.Redis's signature) must still be
|
||||
coerced when routing to a cluster, or Helm-stringified values reach
|
||||
RedisCluster as strings."""
|
||||
for envvar in (*_get_redis_env_kwarg_mapping(), "REDIS_CLUSTER_NODES", "REDIS_SENTINEL_NODES"):
|
||||
monkeypatch.delenv(envvar, raising=False)
|
||||
|
||||
result = _get_redis_client_logic(
|
||||
startup_nodes='[{"host": "localhost", "port": 7000}]',
|
||||
cluster_error_retry_attempts="5",
|
||||
require_full_coverage="false",
|
||||
health_check_interval="30",
|
||||
)
|
||||
|
||||
assert result["cluster_error_retry_attempts"] == 5
|
||||
assert isinstance(result["cluster_error_retry_attempts"], int)
|
||||
assert result["require_full_coverage"] is False
|
||||
assert result["health_check_interval"] == 30
|
||||
assert isinstance(result["health_check_interval"], int)
|
||||
|
||||
|
||||
def test_get_redis_client_logic_raises_without_host_or_url(monkeypatch):
|
||||
"""_get_redis_client_logic raises ValueError when neither host nor url is provided."""
|
||||
for envvar in (*_get_redis_env_kwarg_mapping(), "REDIS_CLUSTER_NODES", "REDIS_SENTINEL_NODES"):
|
||||
monkeypatch.delenv(envvar, raising=False)
|
||||
|
||||
with pytest.raises(ValueError, match="Either 'host' or 'url' must be specified for redis"):
|
||||
_get_redis_client_logic()
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import inspect
|
||||
import json
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
|
@ -600,6 +601,72 @@ def test_reconnect_kwargs_in_cluster_kwargs():
|
|||
assert "socket_keepalive" in kwargs
|
||||
|
||||
|
||||
def test_retry_attempts_in_cluster_kwargs():
|
||||
"""cluster_error_retry_attempts must survive the cluster kwarg allow-list so
|
||||
operators can bound worst-case retry latency on a Redis Cluster: it was being
|
||||
silently dropped because the allow-list was built from redis.RedisCluster's
|
||||
decorated __init__ without unwrapping it, so getfullargspec saw an empty
|
||||
(self, *args, **kwargs) wrapper signature."""
|
||||
kwargs = _get_redis_cluster_kwargs()
|
||||
assert "cluster_error_retry_attempts" in kwargs
|
||||
|
||||
|
||||
def test_async_only_kwargs_in_cluster_kwargs_when_async_client_requested():
|
||||
"""decode_responses is on the async cluster client's constructor and not the sync
|
||||
one, on every redis-py the matrix covers. Introspecting the sync class regardless
|
||||
of which client is actually built silently drops it for every async cluster caller."""
|
||||
sync_kwargs = _get_redis_cluster_kwargs()
|
||||
async_kwargs = _get_redis_cluster_kwargs(async_redis.RedisCluster)
|
||||
|
||||
assert "decode_responses" not in sync_kwargs
|
||||
assert "decode_responses" in async_kwargs
|
||||
|
||||
|
||||
@patch( # test-quality-ok: redis-py >= 6 keeps no cluster_error_retry_attempts attribute on the built client, so the constructor call is the only place the value is observable
|
||||
"litellm.caching.redis_cluster_node_isolation.get_litellm_async_redis_cluster_class"
|
||||
)
|
||||
def test_async_cluster_forwards_retry_attempts(mock_get_cluster_class):
|
||||
"""Regression: cluster_error_retry_attempts must reach the constructed async
|
||||
cluster client. Silently dropping it removes an operator's only lever for
|
||||
bounding a stuck node's worst-case retry latency, and the client falls back
|
||||
to redis-py's own default (3 retries) instead."""
|
||||
mock_cluster_cls = mock_get_cluster_class.return_value
|
||||
get_redis_async_client(
|
||||
startup_nodes=[{"host": "cluster-node", "port": 6379}],
|
||||
cluster_error_retry_attempts=2,
|
||||
)
|
||||
|
||||
call_kwargs = mock_cluster_cls.call_args[1]
|
||||
assert call_kwargs["cluster_error_retry_attempts"] == 2
|
||||
|
||||
|
||||
def test_async_cluster_passes_async_only_kwargs():
|
||||
"""Regression: decode_responses is an async-cluster-only constructor arg. When
|
||||
the allow-list came from the sync class it was filtered out and values came
|
||||
back as bytes instead of str."""
|
||||
client = get_redis_async_client(
|
||||
startup_nodes=[{"host": "cluster-node", "port": 6379}],
|
||||
decode_responses=True,
|
||||
)
|
||||
|
||||
assert client.connection_kwargs["decode_responses"] is True
|
||||
|
||||
|
||||
@pytest.mark.parametrize("cluster_client", [redis.RedisCluster, async_redis.RedisCluster], ids=["sync", "async"])
|
||||
def test_cluster_kwargs_exclude_variadic_parameters(cluster_client):
|
||||
"""*args / **kwargs are signature placeholders, not connection settings, and
|
||||
must never land in the allow-list regardless of which cluster client is
|
||||
introspected."""
|
||||
variadic = {
|
||||
name
|
||||
for name, param in inspect.signature(cluster_client).parameters.items()
|
||||
if param.kind in (param.VAR_POSITIONAL, param.VAR_KEYWORD)
|
||||
}
|
||||
|
||||
leaked = variadic & set(_get_redis_cluster_kwargs(cluster_client))
|
||||
assert not leaked, f"variadic params leaked into the allow-list: {leaked}"
|
||||
|
||||
|
||||
@patch("litellm.caching.redis_cluster_node_isolation.get_litellm_async_redis_cluster_class")
|
||||
def test_async_cluster_sets_reconnect_defaults(mock_get_cluster_class):
|
||||
"""
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue