diff --git a/python/packages/redis/README.md b/python/packages/redis/README.md index bfe812401a..4f826b8962 100644 --- a/python/packages/redis/README.md +++ b/python/packages/redis/README.md @@ -72,8 +72,9 @@ with `SecretString`; use `rediss://` when your server requires TLS. `RedisHistoryProvider` can create its own client from a URL or credential provider, or borrow a standalone `redis.asyncio.Redis` client configured with `decode_responses=True`. A borrowed client lets the application configure -connection policy such as bounded timeouts and health checks. It remains -caller-owned and must not be combined with `redis_url` or `credential_provider`. +connection policy such as bounded timeouts and health checks. It takes +precedence over `redis_url`, `credential_provider`, and the individual +connection settings, which are ignored, and remains caller-owned. Retry behavior also remains caller-controlled; transcript appends are not idempotent if a client replays a write after an ambiguous connection loss. diff --git a/python/packages/redis/agent_framework_redis/_history_provider.py b/python/packages/redis/agent_framework_redis/_history_provider.py index 07827ac62a..f0e11d3ee3 100644 --- a/python/packages/redis/agent_framework_redis/_history_provider.py +++ b/python/packages/redis/agent_framework_redis/_history_provider.py @@ -95,16 +95,21 @@ def __init__( Args: source_id: Unique identifier for this provider instance. redis_url: Redis connection URL (e.g., "redis://localhost:6379"). - Mutually exclusive with credential_provider and redis_client. + Ignored when redis_client is supplied. Mutually exclusive with credential_provider. credential_provider: Redis credential provider for Azure AD authentication. - Requires host parameter. Mutually exclusive with redis_url and redis_client. - host: Redis host name. Required when using credential_provider. - port: Redis port number. Defaults to 6380 (Azure Redis SSL port). - ssl: Enable SSL/TLS connection. Defaults to True. - username: Redis username. + Requires host parameter. Ignored when redis_client is supplied. + Mutually exclusive with redis_url. + host: Redis host name. Required when credential_provider is used to build a client. + port: Redis port number. Defaults to 6380 (Azure Redis SSL port). Used only when + the provider builds a client. + ssl: Enable SSL/TLS connection. Defaults to True. Used only when the provider + builds a client. + username: Redis username. Used only when the provider builds a client. redis_client: Borrowed standalone async Redis client configured with - ``decode_responses=True``. The caller owns its lifetime. Mutually exclusive - with redis_url and credential_provider. + ``decode_responses=True``. The caller owns its lifetime and ``aclose`` leaves + it open. It takes precedence over redis_url, credential_provider and the + individual connection settings, which are ignored rather than treated as a + conflict, matching the Redis vector stores. key_prefix: Base prefix for Redis keys. Scoped mode appends independently encoded tenant, application, agent, provider source, and session segments. Defaults to 'chat_messages'. @@ -130,7 +135,7 @@ def __init__( Raises: TypeError: If redis_client is not a standalone async Redis client. ValueError: If no Redis connection source is provided. - ValueError: If more than one Redis connection source is provided. + ValueError: If both redis_url and credential_provider are provided. ValueError: If credential_provider is used without host parameter. ValueError: If redis_client does not use decoded string responses. ValueError: If max_messages is negative. @@ -145,19 +150,26 @@ def __init__( store_context_from=store_context_from, ) - connection_sources = sum(source is not None for source in (redis_client, redis_url, credential_provider)) - if connection_sources == 0: - raise ValueError("Either redis_client, redis_url, or credential_provider must be provided") - if connection_sources > 1: - raise ValueError("redis_client, redis_url, and credential_provider are mutually exclusive") - if credential_provider is not None and host is None: - raise ValueError("host is required when using credential_provider") if redis_client is not None: + # A caller-owned client is the connection. redis_url, credential_provider and the + # individual connection settings describe how to build one, so supplying them + # alongside it is redundant rather than conflicting, and none of them is + # validated. Only what the provider needs of the client itself still has to + # hold; its lifetime stays with the caller. This is the precedence the Redis + # vector stores already apply in _create_client. if not isinstance(redis_client, redis.Redis): raise TypeError("Redis history providers require a standalone redis.asyncio.Redis client") connection_kwargs = cast(dict[str, Any], redis_client.connection_pool.connection_kwargs) # pyright: ignore[reportUnknownMemberType] if not connection_kwargs.get("decode_responses", False): raise ValueError("Redis history providers require decode_responses=True") + else: + connection_sources = sum(source is not None for source in (redis_url, credential_provider)) + if connection_sources == 0: + raise ValueError("Either redis_client, redis_url, or credential_provider must be provided") + if connection_sources > 1: + raise ValueError("redis_url and credential_provider are mutually exclusive") + if credential_provider is not None and host is None: + raise ValueError("host is required when using credential_provider") if max_messages is not None and max_messages < 0: raise ValueError("max_messages must be None (unlimited) or a non-negative integer") if key_format not in ("scoped", "legacy"): @@ -185,7 +197,9 @@ def __init__( self.agent_id = agent_id self.key_format = key_format self.max_messages = max_messages - self.redis_url = redis_url + # Recorded as the source the provider actually used: a URL passed alongside an + # injected client is ignored, so reporting it would claim a connection never made. + self.redis_url = redis_url if redis_client is None else None self._owns_client = redis_client is None if redis_client is not None: diff --git a/python/packages/redis/tests/test_providers.py b/python/packages/redis/tests/test_providers.py index c8aba3a2e5..ec66992d7f 100644 --- a/python/packages/redis/tests/test_providers.py +++ b/python/packages/redis/tests/test_providers.py @@ -416,21 +416,70 @@ def test_borrowed_client_is_used_directly(self): @pytest.mark.parametrize( "connection_kwargs", [ - {"redis_url": "redis://localhost:6379"}, - {"credential_provider": MagicMock(), "host": "myhost"}, + {"redis_url": "redis://ignored:6379"}, + {"credential_provider": MagicMock()}, + { + "credential_provider": MagicMock(), + "host": "ignored", + "port": 1234, + "ssl": False, + "username": "ignored", + }, + { + "redis_url": "redis://ignored:6379", + "credential_provider": MagicMock(), + }, ], ) - def test_borrowed_client_rejects_other_connection_sources(self, connection_kwargs: dict[str, Any]): + def test_borrowed_client_takes_precedence_over_other_connection_sources(self, connection_kwargs: dict[str, Any]): + # A caller-owned client is the connection. URL, credential provider and the + # individual connection settings describe how to build one, so supplying + # them alongside it is redundant rather than conflicting, and none of them + # is validated. This matches what the Redis vector stores already do. borrowed = Redis(decode_responses=True) - - with pytest.raises(ValueError, match="mutually exclusive"): - RedisHistoryProvider( + with patch("agent_framework_redis._history_provider.redis.from_url") as mock_from_url: + provider = RedisHistoryProvider( "mem", redis_client=borrowed, application_id="test-app", **connection_kwargs, ) + assert provider._redis_client is borrowed + assert provider._owns_client is False + assert provider.redis_url is None + mock_from_url.assert_not_called() + + def test_borrowed_client_still_passes_compatibility_checks_with_redundant_settings(self): + # The settings are ignored; the client itself is not. + with pytest.raises(ValueError, match="decode_responses=True"): + RedisHistoryProvider( + "mem", + redis_client=Redis(decode_responses=False), + redis_url="redis://ignored:6379", + application_id="test-app", + ) + + def test_borrowed_client_keeps_provider_scoping_and_retention(self): + # Connection precedence does not reach provider-level storage settings. + borrowed = Redis(decode_responses=True) + provider = RedisHistoryProvider( + "mem", + redis_client=borrowed, + redis_url="redis://ignored:6379", + application_id="app", + tenant_id="tenant", + agent_id="agent", + max_messages=7, + key_prefix="hist", + ) + + assert provider.max_messages == 7 + assert provider.key_prefix == "hist" + key = provider._redis_key("session") + assert "|v2|" in key + assert "tenant" in key and "agent" in key + def test_borrowed_client_must_be_async_redis(self): with pytest.raises(TypeError, match=r"redis\.asyncio\.Redis"): RedisHistoryProvider(