diff --git a/docs/agents.md b/docs/agents.md index 82adc3b..8480381 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -155,7 +155,7 @@ raises `AttributeError` on the first one missing. | | `socket_connect_timeout` | `5.0` | seconds | | | `socket_keepalive` | `True` | | | | `socket_keepalive_options` | `None` | `dict[int, int | bytes]` | -| `RedisRetrySettings` | `enabled` | `False` | | +| `RedisRetrySettings` | `enabled` | `False` | off hands `redis-py` `Retry(NoBackoff(), 0)`, not nothing — rule 5 | | | `max_attempts` | `0` | `0` means no retry even when `enabled` | | | `backoff_base` | `1.0` | seconds | | | `backoff_cap` | `10.0` | seconds | @@ -206,7 +206,7 @@ Everything in this table is importable from `redis_client_kit` itself. | `close_async_redis_client` | `await (client)` | `None` — shielded, 10 s timeout, never raises | | `close_redis_client` | `(client)` | `None` — never raises | | `build_base_redis_kwargs` | `(settings)` | `dict[str, object]` of `redis-py` keyword arguments | -| `build_redis_retry` | `(settings)` | `redis.retry.Retry | None` | +| `build_redis_retry` | `(settings)` | `redis.retry.Retry` — `Retry(NoBackoff(), 0)` when retries are off | | `parse_redis_url_node` | `(node)` | `tuple[str, int]` — `ValueError` on a node with no port | | `AsyncRedisClient` | type alias | `redis.asyncio.Redis | redis.asyncio.cluster.RedisCluster` | | `SyncRedisClient` | type alias | `redis.Redis | redis.cluster.RedisCluster` | @@ -303,9 +303,13 @@ See rules 15 to 17. 4. **Every cluster node string needs an explicit port.** `host:6379`, `[::1]:6379` and `redis://host:6379` parse; `redis://host` and `host` raise `ValueError`. The scheme is parsed and then ignored — `rediss://` does not enable TLS, `ssl.enabled` does. -5. **`retry.enabled=True` on its own retries nothing.** The `Retry` object is built only - when `enabled` **and** `max_attempts` are both truthy, and `max_attempts` defaults to - `0`. With no `Retry` handed to it, `redis-py` does not retry at all. +5. **`retry.enabled=True` on its own retries nothing, and neither does `redis-py`.** The + exponential `Retry` is built only when `enabled` **and** `max_attempts` are both + truthy, and `max_attempts` defaults to `0`. Otherwise the factory hands `redis-py` + `Retry(NoBackoff(), 0)` explicitly — never nothing, because `redis-py` given no + `Retry` retries on its own since 6.0 (three times; ten since 8.0, with jittered + backoff), which turns a `socket_timeout=0.5` failure into ten seconds or more. With + retries off, the first `ConnectionError` or `TimeoutError` is the one you get. 6. **Retries cover connection failures, not command failures.** `redis-py`'s `Retry` defaults to `ConnectionError`, `TimeoutError` and `socket.timeout`; a `ResponseError` from a bad command is raised on the first try. The delay is diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index d6a945c..70b03ba 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -152,8 +152,14 @@ settings = BaseRedisSettings( ) ``` -`max_attempts` defaults to `0`, and `enabled=True` on its own retries nothing: the `Retry` -object is handed to redis-py only when both `enabled` and `max_attempts` are truthy. +`max_attempts` defaults to `0`, and `enabled=True` on its own retries nothing: the +exponential `Retry` is built only when both `enabled` and `max_attempts` are truthy. +Otherwise the factory hands redis-py `Retry(NoBackoff(), 0)` rather than nothing. Given +no `Retry`, redis-py 6.0 and later retry on their own — three times, ten since 8.0, with +jittered backoff — so a client built to fail in `socket_timeout` seconds would take ten +or more. Disabled means the first `ConnectionError` or `TimeoutError` is raised as is: +with `socket_timeout=0.5`, a command against an unreachable Redis fails in about half a +second. Retry logic uses exponential backoff: ``` diff --git a/redis_client_kit/utils.py b/redis_client_kit/utils.py index a0d55cf..486f88f 100644 --- a/redis_client_kit/utils.py +++ b/redis_client_kit/utils.py @@ -5,7 +5,7 @@ from pathlib import Path from urllib.parse import urlparse -from redis.backoff import ExponentialBackoff +from redis.backoff import ExponentialBackoff, NoBackoff from redis.retry import Retry from .config import RedisSettingsProtocol @@ -36,8 +36,6 @@ def parse_redis_url_node(node: str) -> tuple[str, int]: def build_base_redis_kwargs(settings: RedisSettingsProtocol) -> dict[str, object]: """Build base Redis client keyword arguments.""" - retry = build_redis_retry(settings) - if settings.ssl.enabled: if settings.ssl.ca_certs: _validate_pem_format(settings.ssl.ca_certs, "CERTIFICATE") @@ -54,6 +52,7 @@ def build_base_redis_kwargs(settings: RedisSettingsProtocol) -> dict[str, object "socket_keepalive": settings.pool.socket_keepalive, "socket_keepalive_options": settings.pool.socket_keepalive_options, "health_check_interval": settings.health_check_interval, + "retry": build_redis_retry(settings), "decode_responses": settings.response.decode_responses, "encoding": settings.response.encoding, "client_name": settings.connection.client_name, @@ -70,14 +69,16 @@ def build_base_redis_kwargs(settings: RedisSettingsProtocol) -> dict[str, object kwargs["require_full_coverage"] = settings.cluster.require_full_coverage kwargs["read_from_replicas"] = settings.cluster.read_from_replicas - if retry is not None: - kwargs["retry"] = retry - return kwargs -def build_redis_retry(settings: RedisSettingsProtocol) -> Retry | None: - """Build Redis Retry object from settings.""" +def build_redis_retry(settings: RedisSettingsProtocol) -> Retry: + """Build Redis Retry object from settings. + + Retries are off unless both ``retry.enabled`` and ``retry.max_attempts`` are set. Off is + still a ``Retry``, with zero retries: handed nothing, redis-py retries on its own (three + times since 6.0, ten since 8.0, with jittered backoff). + """ if settings.retry.enabled and settings.retry.max_attempts: return Retry( backoff=ExponentialBackoff( @@ -86,7 +87,7 @@ def build_redis_retry(settings: RedisSettingsProtocol) -> Retry | None: ), retries=settings.retry.max_attempts, ) - return None + return Retry(backoff=NoBackoff(), retries=0) def _validate_pem_format(path: str | Path, file_type: str) -> None: diff --git a/tests/integration/test_redis_lifecycle.py b/tests/integration/test_redis_lifecycle.py index e198dec..a0260f3 100644 --- a/tests/integration/test_redis_lifecycle.py +++ b/tests/integration/test_redis_lifecycle.py @@ -1,11 +1,19 @@ """Integration tests for Redis client lifecycle and cancellation safety.""" import asyncio +import time import pytest +from redis.exceptions import TimeoutError as RedisTimeoutError from testcontainers.redis import RedisContainer -from redis_client_kit import check_async_redis_health, close_async_redis_client, create_async_redis_client +from redis_client_kit import ( + check_async_redis_health, + close_async_redis_client, + close_redis_client, + create_async_redis_client, + create_redis_client, +) from .conftest import FakeRedisSettings, is_docker_available @@ -67,3 +75,62 @@ async def run_and_cancel(): # Assert - This should not raise even after parent task cancellation # because of asyncio.shield in close_async_redis_client await close_async_redis_client(client) + + +@pytest.mark.asyncio +async def test__async_redis_client__retry_disabled_and_redis_paused__raises_within_socket_timeout( + redis_container: RedisContainer, +) -> None: + # Arrange + settings = FakeRedisSettings() + settings.connection.host = redis_container.get_container_host_ip() + settings.connection.port = int(redis_container.get_exposed_port(redis_container.port)) + settings.pool.socket_timeout = 0.5 + settings.pool.socket_connect_timeout = 0.5 + settings.retry.enabled = False + client = create_async_redis_client(settings) + container = redis_container.get_wrapped_container() + await client.ping() + + # Act + container.pause() + try: + started = time.perf_counter() + with pytest.raises(RedisTimeoutError): + await client.get("paused") + elapsed = time.perf_counter() - started + finally: + container.unpause() + await close_async_redis_client(client) + + # Assert + assert elapsed < 2.0 + + +def test__sync_redis_client__retry_disabled_and_redis_paused__raises_within_socket_timeout( + redis_container: RedisContainer, +) -> None: + # Arrange + settings = FakeRedisSettings() + settings.connection.host = redis_container.get_container_host_ip() + settings.connection.port = int(redis_container.get_exposed_port(redis_container.port)) + settings.pool.socket_timeout = 0.5 + settings.pool.socket_connect_timeout = 0.5 + settings.retry.enabled = False + client = create_redis_client(settings) + container = redis_container.get_wrapped_container() + client.ping() + + # Act + container.pause() + try: + started = time.perf_counter() + with pytest.raises(RedisTimeoutError): + client.get("paused") + elapsed = time.perf_counter() - started + finally: + container.unpause() + close_redis_client(client) + + # Assert + assert elapsed < 2.0 diff --git a/tests/unit/aio/test_client.py b/tests/unit/aio/test_client.py index aa5c0dd..847507c 100644 --- a/tests/unit/aio/test_client.py +++ b/tests/unit/aio/test_client.py @@ -3,7 +3,9 @@ import pytest from redis.asyncio.cluster import RedisCluster +from redis.backoff import NoBackoff from redis.exceptions import RedisError +from redis.retry import Retry from redis_client_kit.aio import ( check_async_redis_health, @@ -98,21 +100,31 @@ def test__create_async_redis_client__cluster_without_metrics__creates_uninstrume assert len(kwargs["startup_nodes"]) == 2 -def test__create_async_redis_client__retry_disabled__excludes_retry_from_kwargs( - mock_redis_settings: MagicMock, +@pytest.mark.parametrize( + "cluster_mode, expected_class", + [ + (False, "InstrumentedRedis"), + (True, "InstrumentedRedisCluster"), + ], + ids=["single-node", "cluster"], +) +def test__create_async_redis_client__retry_disabled__passes_zero_retries_to_client( + mock_redis_settings: MagicMock, cluster_mode: bool, expected_class: str ) -> None: # Arrange - mock_redis_settings.cluster.enabled = False + mock_redis_settings.cluster.enabled = cluster_mode mock_redis_settings.retry.enabled = False mock_metrics = MagicMock() # Act - with patch("redis_client_kit.aio.factory.InstrumentedRedis") as mock_redis: + with patch(f"redis_client_kit.aio.factory.{expected_class}") as mock_class: create_async_redis_client(mock_redis_settings, metrics=mock_metrics) # Assert - kwargs = mock_redis.call_args[1] - assert "retry" not in kwargs + retry = mock_class.call_args[1]["retry"] + assert isinstance(retry, Retry) + assert retry._retries == 0 + assert isinstance(retry._backoff, NoBackoff) @pytest.mark.asyncio diff --git a/tests/unit/sync/test_sync_client.py b/tests/unit/sync/test_sync_client.py index 5d8ab37..d71b357 100644 --- a/tests/unit/sync/test_sync_client.py +++ b/tests/unit/sync/test_sync_client.py @@ -3,6 +3,8 @@ from unittest.mock import MagicMock, patch import pytest +from redis.backoff import NoBackoff +from redis.retry import Retry from redis_client_kit.sync import ( check_redis_health, @@ -90,21 +92,28 @@ def test__create_redis_client__cluster_without_nodes__uses_primary_host( assert kwargs["startup_nodes"][0].host == "cluster-host" -def test__create_redis_client__retry_disabled__excludes_retry_from_kwargs( - mock_redis_settings: MagicMock, +@pytest.mark.parametrize( + "cluster_mode, expected_class", + [(False, "InstrumentedRedis"), (True, "InstrumentedRedisCluster")], + ids=["single-node", "cluster"], +) +def test__create_redis_client__retry_disabled__passes_zero_retries_to_client( + mock_redis_settings: MagicMock, cluster_mode: bool, expected_class: str ) -> None: # Arrange - mock_redis_settings.cluster.enabled = False + mock_redis_settings.cluster.enabled = cluster_mode mock_redis_settings.retry.enabled = False mock_metrics = MagicMock() # Act - with patch("redis_client_kit.sync.factory.InstrumentedRedis") as mock_redis: + with patch(f"redis_client_kit.sync.factory.{expected_class}") as mock_class: create_redis_client(mock_redis_settings, metrics=mock_metrics) # Assert - kwargs = mock_redis.call_args[1] - assert "retry" not in kwargs + retry = mock_class.call_args[1]["retry"] + assert isinstance(retry, Retry) + assert retry._retries == 0 + assert isinstance(retry._backoff, NoBackoff) def test__close_redis_client__valid_client__calls_close() -> None: diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index c03c246..130ec77 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -1,6 +1,8 @@ from unittest.mock import MagicMock, patch import pytest +from redis.backoff import NoBackoff +from redis.retry import Retry from redis_client_kit.utils import ( _validate_pem_format, @@ -44,15 +46,17 @@ def test__parse_redis_url_node__invalid_input__raises_value_error() -> None: parse_redis_url_node("invalid-node") -def test__build_redis_retry__retry_disabled__returns_none(mock_redis_settings: MagicMock) -> None: +def test__build_redis_retry__retry_disabled__returns_zero_retries(mock_redis_settings: MagicMock) -> None: # Arrange mock_redis_settings.retry.enabled = False # Act - result = build_redis_retry(mock_redis_settings) + retry = build_redis_retry(mock_redis_settings) # Assert - assert result is None + assert isinstance(retry, Retry) + assert retry._retries == 0 + assert isinstance(retry._backoff, NoBackoff) def test__build_redis_retry__retry_enabled__returns_retry_object(mock_redis_settings: MagicMock) -> None: @@ -68,16 +72,18 @@ def test__build_redis_retry__retry_enabled__returns_retry_object(mock_redis_sett assert getattr(retry, "_retries", 0) == 5 -def test__build_redis_retry__no_max_attempts__returns_none(mock_redis_settings: MagicMock) -> None: +def test__build_redis_retry__no_max_attempts__returns_zero_retries(mock_redis_settings: MagicMock) -> None: # Arrange mock_redis_settings.retry.enabled = True - mock_redis_settings.retry.max_attempts = None + mock_redis_settings.retry.max_attempts = 0 # Act - result = build_redis_retry(mock_redis_settings) + retry = build_redis_retry(mock_redis_settings) # Assert - assert result is None + assert isinstance(retry, Retry) + assert retry._retries == 0 + assert isinstance(retry._backoff, NoBackoff) def test__build_base_redis_kwargs__full_config__returns_all_parameters(mock_redis_settings: MagicMock) -> None: @@ -99,7 +105,7 @@ def test__build_base_redis_kwargs__full_config__returns_all_parameters(mock_redi assert "retry" in kwargs -def test__build_base_redis_kwargs__retry_disabled__excludes_retry_parameter( +def test__build_base_redis_kwargs__retry_disabled__passes_zero_retries( mock_redis_settings: MagicMock, ) -> None: # Arrange @@ -109,7 +115,10 @@ def test__build_base_redis_kwargs__retry_disabled__excludes_retry_parameter( kwargs = build_base_redis_kwargs(mock_redis_settings) # Assert - assert "retry" not in kwargs + retry = kwargs["retry"] + assert isinstance(retry, Retry) + assert retry._retries == 0 + assert isinstance(retry._backoff, NoBackoff) def test__build_base_redis_kwargs__ssl_enabled__includes_ssl_parameters(mock_redis_settings: MagicMock) -> None: