From c7d1bbb3e1b32beb00484699791dc036d30d262a Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 09:49:05 +0200 Subject: [PATCH 01/13] fix(cache): build async cache clients without a deprecation warning BaseCache._get_async_redis_client called get_async_redis_connection, which warns unconditionally, so anyone who built a cache from a redis_url and awaited a cache method saw a DeprecationWarning for an API they never called. A suite-wide filter in pyproject.toml hid it. Point the method at _get_aredis_connection, the async form already used everywhere else, and drop the filter. Cache clients now also report their library name via CLIENT SETINFO like every other RedisVL client, and a connection failure surfaces when the client is created rather than at the first command. The regression guard lives in tests/unit/test_connection_normalization.py and lands with the next commit, which is the first to touch that file. --- pyproject.toml | 3 --- redisvl/extensions/cache/base.py | 2 +- tests/unit/test_error_handling.py | 6 +++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 450df15c6..afdaf3b2c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -169,9 +169,6 @@ exclude = ''' [tool.pytest.ini_options] log_cli = true asyncio_mode = "auto" -filterwarnings = [ - "ignore:get_async_redis_connection will become async in the next major release:DeprecationWarning", -] [tool.mypy] warn_unused_configs = true diff --git a/redisvl/extensions/cache/base.py b/redisvl/extensions/cache/base.py index 8ff3566db..6af540aa3 100644 --- a/redisvl/extensions/cache/base.py +++ b/redisvl/extensions/cache/base.py @@ -139,7 +139,7 @@ async def _get_async_redis_client(self) -> AsyncRedisClient: url = cast(str | None, self.redis_kwargs["redis_url"]) kwargs = cast(dict[str, Any], self.redis_kwargs["connection_kwargs"]) self._async_redis_client = ( - RedisConnectionFactory.get_async_redis_connection( + await RedisConnectionFactory._get_aredis_connection( redis_url=url, **kwargs ) ) diff --git a/tests/unit/test_error_handling.py b/tests/unit/test_error_handling.py index 4dbc47d7c..c6ce31377 100644 --- a/tests/unit/test_error_handling.py +++ b/tests/unit/test_error_handling.py @@ -11,7 +11,7 @@ import asyncio from collections.abc import Mapping -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest import redis.exceptions @@ -204,11 +204,11 @@ async def test_connection_kwargs_valid_dict(self): "redisvl.extensions.cache.base.RedisConnectionFactory" ) as mock_factory: mock_client = Mock() - mock_factory.get_async_redis_connection.return_value = mock_client + mock_factory._get_aredis_connection = AsyncMock(return_value=mock_client) result = await cache._get_async_redis_client() assert result == mock_client - mock_factory.get_async_redis_connection.assert_called_once() + mock_factory._get_aredis_connection.assert_awaited_once() class TestRouterConfigErrorHandling: From 13c00479dd4b6be596ed81c11895b5b44da07bea Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 09:49:25 +0200 Subject: [PATCH 02/13] feat(index): add owns_client for explicit client ownership handover An index closed only a client it created itself, and callers who needed to override that wrote to the private _owns_redis_client keyword or poked the attribute afterwards. The MCP server did the latter, which never worked as intended: _register_client_finalizer gates on the flag, so a post-construction flip lands after registration has already declined and no finalizer is ever created. owns_client states ownership once, at construction, before the finalizer is registered. It replaces the private keyword rather than sitting alongside it, so there is one spelling and no precedence question. An explicit value also wins over the ownership from_existing would otherwise assume for a client it created, which is why the assignment there uses setdefault. Also documents the accessor asymmetry between the two classes: _redis_client lazily creates on SearchIndex but is a plain nullable attribute on AsyncSearchIndex, whose lazy getter is _get_client. --- redisvl/extensions/router/semantic.py | 2 +- redisvl/index/index.py | 47 +++++++++++++--- redisvl/mcp/server.py | 8 +-- redisvl/redis/connection.py | 2 +- tests/unit/test_connection_normalization.py | 61 ++++++++++++++++++++- tests/unit/test_index_gc_finalizer.py | 54 ++++++++++++++++++ 6 files changed, 159 insertions(+), 15 deletions(-) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 16472615b..4d9111e19 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -192,7 +192,7 @@ def from_existing( **factory_kwargs, ) index_kwargs["_client_validated"] = True - index_kwargs["_owns_redis_client"] = True + index_kwargs["owns_client"] = True if lib_name is not None: index_kwargs["lib_name"] = lib_name created_redis_client = True diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 5a34c03f0..4e03f489f 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -816,11 +816,11 @@ def __init__( redis_url: str | None = None, connection_kwargs: dict[str, Any] | None = None, validate_on_load: bool = False, + owns_client: bool | None = None, **kwargs, ): - """Initialize the RedisVL search index with a schema, Redis client - (or URL string with other connection args), connection_args, and other - kwargs. + """Initialize the RedisVL search index with a schema and either a Redis + client or a URL string with other connection kwargs. Args: schema (IndexSchema): Index schema object. @@ -832,6 +832,12 @@ def __init__( args. validate_on_load (bool, optional): Whether to validate data against schema when loading. Defaults to False. + owns_client (Optional[bool]): Whether the index should close the + Redis client when it is disconnected or garbage collected. By + default the index owns only a client it created itself from + `redis_url`, and never closes a client passed as + `redis_client`. Pass True to hand over a client you created, + or False to keep one the index would otherwise own. """ if "connection_args" in kwargs: connection_kwargs = kwargs.pop("connection_args") @@ -851,7 +857,13 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - self._owns_redis_client = kwargs.pop("_owns_redis_client", redis_client is None) + # An index owns the client it created itself, so a caller-supplied + # client is not closed unless the caller hands ownership over with + # owns_client=True. Assigned before the finalizer is registered, + # because _register_client_finalizer reads this flag. + self._owns_redis_client = ( + redis_client is None if owns_client is None else owns_client + ) self._client_finalizer = None # Close the owned client when this index is garbage collected. When # the client is created lazily, registration happens at creation time @@ -923,7 +935,9 @@ def from_existing( schema_dict = convert_index_info_to_schema(index_info) schema = IndexSchema.from_dict(schema_dict) if created_redis_client: - init_kwargs["_owns_redis_client"] = True + # The index created this client, so it owns it unless the caller + # explicitly said otherwise. + init_kwargs.setdefault("owns_client", True) return cls( schema, redis_client=redis_client, @@ -2131,6 +2145,7 @@ def __init__( redis_client: AsyncRedisClient | None = None, connection_kwargs: dict[str, Any] | None = None, validate_on_load: bool = False, + owns_client: bool | None = None, **kwargs, ): """Initialize the RedisVL async search index with a schema. @@ -2145,6 +2160,12 @@ def __init__( args. validate_on_load (bool, optional): Whether to validate data against schema when loading. Defaults to False. + owns_client (Optional[bool]): Whether the index should close the + Redis client when it is disconnected or garbage collected. By + default the index owns only a client it created itself from + `redis_url`, and never closes a client passed as + `redis_client`. Pass True to hand over a client you created, + or False to keep one the index would otherwise own. """ if "redis_kwargs" in kwargs: connection_kwargs = kwargs.pop("redis_kwargs") @@ -2157,7 +2178,11 @@ def __init__( self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) - # Store connection parameters + # Store connection parameters. Note the asymmetry with SearchIndex: + # there, _redis_client is a property that lazily creates the client, + # whereas here it is a plain attribute that stays None until + # _get_client() creates one. Read it through _get_client(), never + # directly, unless you specifically want the un-created state. self._redis_client = redis_client self._redis_url = redis_url self._connection_kwargs = connection_kwargs or {} @@ -2165,7 +2190,11 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - self._owns_redis_client = kwargs.pop("_owns_redis_client", redis_client is None) + # See the note on SearchIndex.__init__: ownership follows from who + # created the client, and owns_client=True hands it to the index. + self._owns_redis_client = ( + redis_client is None if owns_client is None else owns_client + ) self._client_finalizer = None # Close the owned client when this index is garbage collected. When # the client is created lazily, registration happens at creation time @@ -2231,7 +2260,9 @@ async def from_existing( schema_dict = convert_index_info_to_schema(index_info) schema = IndexSchema.from_dict(schema_dict) if created_redis_client: - init_kwargs["_owns_redis_client"] = True + # The index created this client, so it owns it unless the caller + # explicitly said otherwise. + init_kwargs.setdefault("owns_client", True) return cls( schema, redis_client=redis_client, diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 554ccc18e..53b894661 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -643,11 +643,11 @@ async def _load_effective_schema( @staticmethod def _make_index(schema: IndexSchema, client: Any) -> AsyncSearchIndex: """Bind an inspected schema and Redis client into an async index.""" - index = AsyncSearchIndex(schema=schema, redis_client=client) # The server acquired this client explicitly during startup, so hand - # ownership to the index for a single shutdown path. - index._owns_redis_client = True - return index + # ownership to the index for a single shutdown path. Passing + # owns_client at construction also registers the GC finalizer, which a + # post-construction flag flip would be too late for. + return AsyncSearchIndex(schema=schema, redis_client=client, owns_client=True) async def _initialize_vectorizer( self, binding: MCPIndexBindingConfig, schema: IndexSchema, timeout: int diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 53edec80c..eca8a2f78 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -35,7 +35,7 @@ def _split_from_existing_kwargs( init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} - for key in ("validate_on_load", "lib_name"): + for key in ("validate_on_load", "lib_name", "owns_client"): if key in kwargs: init_kwargs[key] = kwargs.pop(key) diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index 98622eb1c..f5915ddd9 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -7,6 +7,7 @@ from redisvl.extensions.router.semantic import SemanticRouter from redisvl.index import AsyncSearchIndex, SearchIndex from redisvl.query.sql import SQLQuery +from redisvl.utils.utils import assert_no_warnings def _schema_dict(name: str = "idx") -> dict: @@ -105,6 +106,41 @@ def test_search_index_from_existing_owns_factory_created_client(): created_client.close.assert_called_once_with() +def test_search_index_from_existing_honours_explicit_owns_client(): + """``owns_client`` is an init kwarg, not a connection kwarg. + + Without the allow-list entry in ``_split_from_existing_kwargs`` it would + fall through to ``connection_kwargs`` and redis-py would reject it. An + explicit value also wins over the ownership ``from_existing`` would + otherwise assume for a client it created itself. + """ + created_client = MagicMock() + + with ( + patch( + "redisvl.index.index.RedisConnectionFactory.get_redis_connection", + return_value=created_client, + ) as mock_get_connection, + patch.object(SearchIndex, "_info", return_value={}), + patch( + "redisvl.index.index.convert_index_info_to_schema", + return_value=_schema_dict("search-index"), + ), + ): + index = SearchIndex.from_existing( + "search-index", + redis_url="redis://localhost:6380", + owns_client=False, + ) + + mock_get_connection.assert_called_once_with(redis_url="redis://localhost:6380") + assert index._owns_redis_client is False + + index.disconnect() + + created_client.close.assert_not_called() + + @pytest.mark.asyncio async def test_async_search_index_from_existing_prefers_provided_client(): """Use the provided async Redis client instead of constructing a new one.""" @@ -278,7 +314,7 @@ def test_semantic_router_from_existing_rebuilds_from_redis_url(): assert mock_from_dict.call_args.kwargs["_index_kwargs"] == { "_internal_flag": True, "_client_validated": True, - "_owns_redis_client": True, + "owns_client": True, } assert result is loaded_router @@ -300,6 +336,29 @@ def test_base_cache_sync_client_creation_uses_connection_factory(): assert client is mock_client +@pytest.mark.asyncio +async def test_base_cache_async_client_creation_emits_no_warning(): + """Creating a cache's async client must not warn. + + ``_get_async_redis_client`` used to call ``get_async_redis_connection``, + which warns unconditionally, so cache users saw a DeprecationWarning for + an API they never called. A suite-wide filter in pyproject.toml hid it. + This is the guard that replaced that filter. + """ + cache = EmbeddingsCache(redis_url="redis://localhost:6379") + mock_client = MagicMock() + + with patch( + "redisvl.extensions.cache.base.RedisConnectionFactory._get_aredis_connection", + new=AsyncMock(return_value=mock_client), + ) as mock_get_connection: + with assert_no_warnings(): + client = await cache._get_async_redis_client() + + mock_get_connection.assert_awaited_once_with(redis_url="redis://localhost:6379") + assert client is mock_client + + def test_sql_query_uses_connection_factory_for_redis_url(): """Build SQL query helper connections through the shared connection factory.""" fake_sql_redis_module = _fake_sql_redis_module() diff --git a/tests/unit/test_index_gc_finalizer.py b/tests/unit/test_index_gc_finalizer.py index 7a36b5b49..37bb2ee3c 100644 --- a/tests/unit/test_index_gc_finalizer.py +++ b/tests/unit/test_index_gc_finalizer.py @@ -223,3 +223,57 @@ def test_async_injected_client_not_closed_on_collection(self): collect() fake_client.aclose.assert_not_awaited() + + +class TestOwnsClientHandover: + """``owns_client`` overrides who closes the client. + + By default an index closes only a client it created itself. These tests + cover the two explicit overrides, which are the only way to hand a + pre-built client over (or to keep one the index would otherwise own) now + that ``set_client()`` is gone. + """ + + def test_sync_injected_client_closed_when_ownership_handed_over(self): + schema = IndexSchema.from_dict(SCHEMA_DICT) + fake_client = mock.MagicMock(name="handed_over_sync_client") + index = SearchIndex(schema, redis_client=fake_client, owns_client=True) + + del index + collect() + + fake_client.close.assert_called_once() + + def test_async_injected_client_closed_when_ownership_handed_over(self): + schema = IndexSchema.from_dict(SCHEMA_DICT) + fake_client = mock.MagicMock(name="handed_over_async_client") + fake_client.aclose = mock.AsyncMock() + index = AsyncSearchIndex(schema, redis_client=fake_client, owns_client=True) + + del index + collect() + + fake_client.aclose.assert_awaited_once() + + def test_sync_lazily_created_client_kept_when_ownership_declined(self): + """``owns_client=False`` keeps a client the index would have owned. + + Only covered on the sync class: the flag is read by shared + ``__init__`` code, and the per-flavour close paths are covered above. + """ + schema = IndexSchema.from_dict(SCHEMA_DICT) + fake_client = mock.MagicMock(name="lazily_created_sync_client") + + with mock.patch( + "redisvl.index.index.RedisConnectionFactory.get_redis_connection", + return_value=fake_client, + ): + index = SearchIndex( + schema, redis_url="redis://fake:6379", owns_client=False + ) + assert index._redis_client is fake_client + + del index + collect() + + fake_client.close.assert_not_called() From 8877e4a62fd9db3108a9724a6618ed3c4d520382 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 09:49:37 +0200 Subject: [PATCH 03/13] fix(migration): read the lazily created client, not the raw property The four migration modules read index.client, which is None until the client is lazily created, and each handled that differently: one raised, two recorded an error, one dereferenced unguarded, and the planner degraded to an empty key sample that then made the key-sample check vacuously true. All four build their index through from_existing, which always yields a client, so none of this was reachable in practice. Reading through _redis_client and _get_client removes the disagreement and the dead guards with it. The test doubles are renamed to match the real accessors so they still stand in for an index. --- redisvl/migration/async_planner.py | 4 +-- redisvl/migration/async_validation.py | 34 +++++++--------------- redisvl/migration/planner.py | 20 ++++++++----- redisvl/migration/validation.py | 12 ++------ tests/unit/test_async_migration_planner.py | 4 +-- tests/unit/test_migration_planner.py | 9 +++--- 6 files changed, 33 insertions(+), 50 deletions(-) diff --git a/redisvl/migration/async_planner.py b/redisvl/migration/async_planner.py index 6c75efda2..99972208e 100644 --- a/redisvl/migration/async_planner.py +++ b/redisvl/migration/async_planner.py @@ -235,9 +235,7 @@ async def snapshot_source( prefixes = index.schema.index.prefix prefix_list = prefixes if isinstance(prefixes, list) else [prefixes] - client = index.client - if client is None: - raise ValueError("Failed to get Redis client from index") + client = await index._get_client() return SourceSnapshot( index_name=index_name, diff --git a/redisvl/migration/async_validation.py b/redisvl/migration/async_validation.py index ce742a3d0..2575daaf6 100644 --- a/redisvl/migration/async_validation.py +++ b/redisvl/migration/async_validation.py @@ -74,13 +74,10 @@ async def validate( validation.doc_count_match = source_total == target_total key_sample = plan.source.keyspace.key_sample - client = target_index.client if not key_sample: validation.key_sample_exists = True - elif client is None: - validation.key_sample_exists = False - validation.errors.append("Failed to get Redis client for key sample check") else: + client = await target_index._get_client() # Handle prefix change: transform key_sample to use new prefix. # Must match the executor's RENAME logic exactly: # new_key = new_prefix + key[len(old_prefix):] @@ -140,9 +137,7 @@ async def validate( async def _count_index_keys(self, index: AsyncSearchIndex) -> int: """Count keys matching the target index prefixes with SCAN.""" - client = index.client - if client is None: - raise ValueError("Redis client is required to count index keys") + client = await index._get_client() prefixes = index.schema.index.prefix prefix_list = prefixes if isinstance(prefixes, list) else [prefixes] @@ -181,25 +176,16 @@ async def _run_query_checks( ) ) - client = target_index.client + client = await target_index._get_client() for key in query_checks.get("keys_exist", []): - if client is None: - results.append( - QueryCheckResult( - name=f"key:{key}", - passed=False, - details="Failed to get Redis client", - ) - ) - else: - exists = bool(await client.exists(key)) - results.append( - QueryCheckResult( - name=f"key:{key}", - passed=exists, - details="Key exists" if exists else "Key not found", - ) + exists = bool(await client.exists(key)) + results.append( + QueryCheckResult( + name=f"key:{key}", + passed=exists, + details="Key exists" if exists else "Key not found", ) + ) return results diff --git a/redisvl/migration/planner.py b/redisvl/migration/planner.py index 4c09fe04c..cd6f711f8 100644 --- a/redisvl/migration/planner.py +++ b/redisvl/migration/planner.py @@ -2,7 +2,7 @@ from copy import deepcopy from pathlib import Path -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple, cast import yaml @@ -18,6 +18,7 @@ ) from redisvl.redis.connection import supports_svs from redisvl.schema.schema import IndexSchema +from redisvl.types import SyncRedisClient class MigrationPlanner: @@ -226,7 +227,7 @@ def snapshot_source( prefixes=prefix_list, key_separator=index.schema.index.key_separator, key_sample=self._sample_keys( - client=index.client, + client=index._redis_client, prefixes=prefix_list, key_separator=index.schema.index.key_separator, ), @@ -647,10 +648,10 @@ def write_plan(self, plan: MigrationPlan, plan_out: str) -> None: yaml.safe_dump(plan.model_dump(exclude_none=True), f, sort_keys=False) def _sample_keys( - self, *, client: Any, prefixes: List[str], key_separator: str + self, *, client: SyncRedisClient, prefixes: List[str], key_separator: str ) -> List[str]: key_sample: List[str] = [] - if client is None or self.key_sample_limit <= 0: + if self.key_sample_limit <= 0: return key_sample for prefix in prefixes: @@ -666,10 +667,13 @@ def _sample_keys( match_pattern = f"{prefix}*" cursor = 0 while True: - cursor, keys = client.scan( - cursor=cursor, - match=match_pattern, - count=max(self.key_sample_limit, 1000), + cursor, keys = cast( + tuple[int, list[Any]], + client.scan( + cursor=cursor, + match=match_pattern, + count=max(self.key_sample_limit, 1000), + ), ) for key in keys: decoded_key = key.decode() if isinstance(key, bytes) else str(key) diff --git a/redisvl/migration/validation.py b/redisvl/migration/validation.py index f8735a443..6735d6479 100644 --- a/redisvl/migration/validation.py +++ b/redisvl/migration/validation.py @@ -12,7 +12,6 @@ QueryCheckResult, ) from redisvl.migration.utils import build_scan_match_patterns, load_yaml, schemas_equal -from redisvl.types import SyncRedisClient class MigrationValidator: @@ -89,7 +88,7 @@ def validate( # Check keys one at a time to avoid Redis Cluster cross-slot # errors from multi-key EXISTS commands. existing_count = sum( - target_index.client.exists(key) for key in keys_to_check + target_index._redis_client.exists(key) for key in keys_to_check ) validation.key_sample_exists = existing_count == len(keys_to_check) @@ -128,10 +127,7 @@ def validate( def _count_index_keys(self, index: SearchIndex) -> int: """Count keys matching the target index prefixes with SCAN.""" - raw_client = index.client - if raw_client is None: - raise ValueError("Redis client is required to count index keys") - client = cast(SyncRedisClient, raw_client) + client = index._redis_client prefixes = index.schema.index.prefix prefix_list = prefixes if isinstance(prefixes, list) else [prefixes] @@ -173,10 +169,8 @@ def _run_query_checks( ) ) + client = target_index._redis_client for key in query_checks.get("keys_exist", []): - client = target_index.client - if client is None: - raise ValueError("Redis client not connected") exists = bool(client.exists(key)) results.append( QueryCheckResult( diff --git a/tests/unit/test_async_migration_planner.py b/tests/unit/test_async_migration_planner.py index 93ce3d49d..c4483d974 100644 --- a/tests/unit/test_async_migration_planner.py +++ b/tests/unit/test_async_migration_planner.py @@ -35,8 +35,8 @@ def __init__(self, schema, stats, keys): self._stats = stats self._client = AsyncDummyClient(keys) - @property - def client(self): + async def _get_client(self): + """Mirrors AsyncSearchIndex._get_client, the lazy async getter.""" return self._client async def info(self): diff --git a/tests/unit/test_migration_planner.py b/tests/unit/test_migration_planner.py index b07f9df93..d8f5df0cc 100644 --- a/tests/unit/test_migration_planner.py +++ b/tests/unit/test_migration_planner.py @@ -36,7 +36,8 @@ def __init__(self, schema, stats, keys): self._client = DummyClient(keys) @property - def client(self): + def _redis_client(self): + """Mirrors SearchIndex._redis_client, the lazily creating property.""" return self._client def info(self): @@ -1207,7 +1208,7 @@ def test_exists_called_per_key(self, monkeypatch): mock_client.exists.return_value = 1 # Each key exists mock_index = MagicMock() - mock_index.client = mock_client + mock_index._redis_client = mock_client mock_index.info.return_value = {"num_docs": 3, "hash_indexing_failures": 0} mock_index.schema.to_dict.return_value = plan.merged_target_schema mock_index.search.return_value = MagicMock(total=3) @@ -1242,7 +1243,7 @@ def test_multi_prefix_keys_translated(self, monkeypatch): mock_client.exists.return_value = 1 mock_index = MagicMock() - mock_index.client = mock_client + mock_index._redis_client = mock_client mock_index.info.return_value = {"num_docs": 3, "hash_indexing_failures": 0} mock_index.schema.to_dict.return_value = plan.merged_target_schema mock_index.search.return_value = MagicMock(total=3) @@ -1287,7 +1288,7 @@ def test_expected_source_count_uses_scanned_target_keys(self, monkeypatch): } mock_index = MagicMock() - mock_index.client = DummyClient( + mock_index._redis_client = DummyClient( [b"target:1", b"target:2", b"target:3", b"target:4", b"target:5"] ) mock_index.info.return_value = {"num_docs": 5, "hash_indexing_failures": 0} From 59348340310c9be9be49a09e2490280e1ce460f1 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 09:49:48 +0200 Subject: [PATCH 04/13] docs(utils): stop promising removal in the next major release The deprecation decorators told users each deprecated argument, function and class would be removed "in the next major release". Every release so far has been 0.x and breaking changes ship on minor bumps per project convention, so that promise has never been accurate and each removal would otherwise need a release note explaining the mismatch. Say "in a future release" instead. Warning text only; no behaviour change. --- redisvl/utils/utils.py | 8 +++++--- tests/unit/test_url_deprecation.py | 2 +- tests/unit/test_utils.py | 16 +++++++--------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/redisvl/utils/utils.py b/redisvl/utils/utils.py index 85f74397c..bd53af0ae 100644 --- a/redisvl/utils/utils.py +++ b/redisvl/utils/utils.py @@ -93,7 +93,9 @@ class MyClass: def test_method(cls, old_arg=None, new_arg=None): pass """ - message = f"Argument {argument} is deprecated and will be removed in the next major release." + message = ( + f"Argument {argument} is deprecated and will be removed in a future release." + ) if replacement: message += f" Use {replacement} instead." @@ -157,7 +159,7 @@ def decorator(func): fn_name = name or func.__name__ warning_message = ( f"Function {fn_name} is deprecated and will be " - "removed in the next major release. " + "removed in a future release. " ) if replacement: warning_message += replacement @@ -194,7 +196,7 @@ def decorator(cls): class_name = name or cls.__name__ warning_message = ( f"Class {class_name} is deprecated and will be " - "removed in the next major release. " + "removed in a future release. " ) if replacement: warning_message += replacement diff --git a/tests/unit/test_url_deprecation.py b/tests/unit/test_url_deprecation.py index d70ff4cb8..7b01cefa5 100644 --- a/tests/unit/test_url_deprecation.py +++ b/tests/unit/test_url_deprecation.py @@ -26,7 +26,7 @@ async def test__get_aredis_connection_deprecates_url_kwarg_only(): assert any( str(w.message) == ( - "Argument url is deprecated and will be removed in the next major release. " + "Argument url is deprecated and will be removed in a future release. " "Use redis_url instead." ) for w in record diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index d7c1e2f0e..ef0a17670 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -215,7 +215,7 @@ def test_func(old_arg=None, new_arg=None, neutral_arg=None): assert len(record) == 1 assert str(record[0].message) == ( - "Argument old_arg is deprecated and will be removed in the next major release. Use new_arg instead." + "Argument old_arg is deprecated and will be removed in a future release. Use new_arg instead." ) # Test that passing the deprecated argument as a positional argument also triggers the warning. @@ -224,7 +224,7 @@ def test_func(old_arg=None, new_arg=None, neutral_arg=None): assert len(record) == 1 assert str(record[0].message) == ( - "Argument old_arg is deprecated and will be removed in the next major release. Use new_arg instead." + "Argument old_arg is deprecated and will be removed in a future release. Use new_arg instead." ) with assert_no_warnings(): @@ -242,8 +242,7 @@ def test_func(old_arg=None, neutral_arg=None): assert len(record) == 1 assert str(record[0].message) == ( - "Argument old_arg is deprecated and will be removed" - " in the next major release." + "Argument old_arg is deprecated and will be removed" " in a future release." ) # As a positional arg @@ -252,8 +251,7 @@ def test_func(old_arg=None, neutral_arg=None): assert len(record) == 1 assert str(record[0].message) == ( - "Argument old_arg is deprecated and will be removed" - " in the next major release." + "Argument old_arg is deprecated and will be removed" " in a future release." ) with assert_no_warnings(): @@ -547,7 +545,7 @@ def __init__(self, value): assert len(record) == 1 assert str(record[0].message) == ( - "Class OldClass is deprecated and will be removed in the next major release. " + "Class OldClass is deprecated and will be removed in a future release. " "Use NewClass instead." ) assert obj.value == 42 @@ -563,7 +561,7 @@ def __init__(self, value): assert len(record) == 1 assert str(record[0].message) == ( - "Class OldClass is deprecated and will be removed in the next major release. " + "Class OldClass is deprecated and will be removed in a future release. " ) assert obj.value == 42 @@ -577,7 +575,7 @@ class OldClass: assert len(record) == 1 assert str(record[0].message) == ( - "Class CustomOldClass is deprecated and will be removed in the next major release. " + "Class CustomOldClass is deprecated and will be removed in a future release. " "Use NewClass instead." ) From 4558ffea5fd617f58ea10a5367ed471a2629e349 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 10:31:26 +0200 Subject: [PATCH 05/13] fix(cache): serialise lazy creation of the async cache client Moving this method onto _get_aredis_connection introduced an await between its "is the client None" check and the assignment, because the async factory issues a CLIENT SETINFO round trip. BaseCache has no lock, so two concurrent callers each built a client and the first was left unreachable: adisconnect only closes the client currently on the instance, so the orphan's connection pool was never released. Wrap the lazy path in a double-checked lock, the same shape AsyncSearchIndex._get_client already uses over the same factory. The regression test fails without the lock and passes with it. --- redisvl/extensions/cache/base.py | 43 +++++++++++++-------- tests/unit/test_connection_normalization.py | 30 ++++++++++++++ 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/redisvl/extensions/cache/base.py b/redisvl/extensions/cache/base.py index 6af540aa3..212faec2b 100644 --- a/redisvl/extensions/cache/base.py +++ b/redisvl/extensions/cache/base.py @@ -4,6 +4,7 @@ specific cache types such as LLM caches and embedding caches. """ +import asyncio from collections.abc import Mapping from typing import Any, cast @@ -59,7 +60,13 @@ def __init__( # Initialize Redis clients self._async_redis_client = async_redis_client self._redis_client = redis_client + # Guards lazy async client creation, which suspends on an await and so + # cannot rely on a bare check-then-set. Mirrors AsyncSearchIndex._lock. + self._async_client_lock = asyncio.Lock() + # Caches never close a caller-supplied client and register no GC + # finalizer, so the index's owns_client handover has no cache + # equivalent by design. if redis_client or async_redis_client: self._owns_redis_client = False else: @@ -128,22 +135,26 @@ async def _get_async_redis_client(self) -> AsyncRedisClient: Returns: AsyncRedisClient: An async Redis client instance. """ - if not hasattr(self, "_async_redis_client") or self._async_redis_client is None: - client = self.redis_kwargs.get("redis_client") - - if client and isinstance(client, (Redis, RedisCluster)): - self._async_redis_client = RedisConnectionFactory.sync_to_async_redis( - client - ) - else: - url = cast(str | None, self.redis_kwargs["redis_url"]) - kwargs = cast(dict[str, Any], self.redis_kwargs["connection_kwargs"]) - self._async_redis_client = ( - await RedisConnectionFactory._get_aredis_connection( - redis_url=url, **kwargs - ) - ) - return self._async_redis_client + client = getattr(self, "_async_redis_client", None) + if client is None: + async with self._async_client_lock: + # Double-check: another task may have created the client while + # this one waited on the lock or on the factory's round trip. + client = getattr(self, "_async_redis_client", None) + if client is None: + provided = self.redis_kwargs.get("redis_client") + if provided and isinstance(provided, (Redis, RedisCluster)): + client = RedisConnectionFactory.sync_to_async_redis(provided) + else: + url = cast(str | None, self.redis_kwargs["redis_url"]) + kwargs = cast( + dict[str, Any], self.redis_kwargs["connection_kwargs"] + ) + client = await RedisConnectionFactory._get_aredis_connection( + redis_url=url, **kwargs + ) + self._async_redis_client = client + return client def expire(self, key: str, ttl: int | None = None) -> None: """Set or refresh the expiration time for a key in the cache. diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index f5915ddd9..28560f79a 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -1,3 +1,4 @@ +import asyncio from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -359,6 +360,35 @@ async def test_base_cache_async_client_creation_emits_no_warning(): assert client is mock_client +@pytest.mark.asyncio +async def test_base_cache_async_client_creation_is_serialised(): + """Concurrent callers must share one client, not orphan a connection pool. + + Building the client awaits a CLIENT SETINFO round trip, so a bare + check-then-set would let two tasks each create one and leave the first + unreachable and never closed. + """ + cache = EmbeddingsCache(redis_url="redis://localhost:6379") + created = [] + + async def factory(*args, **kwargs): + await asyncio.sleep(0) # the suspension the real factory introduces + client = MagicMock(name=f"client{len(created)}") + created.append(client) + return client + + with patch( + "redisvl.extensions.cache.base.RedisConnectionFactory._get_aredis_connection", + new=factory, + ): + first, second = await asyncio.gather( + cache._get_async_redis_client(), cache._get_async_redis_client() + ) + + assert len(created) == 1 + assert first is second + + def test_sql_query_uses_connection_factory_for_redis_url(): """Build SQL query helper connections through the shared connection factory.""" fake_sql_redis_module = _fake_sql_redis_module() From c2d54c90c36a75f98164e419c41446e2e354eb75 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 10:31:46 +0200 Subject: [PATCH 06/13] fix(router): let an explicit owns_client survive from_existing SemanticRouter.from_existing merges {**init_kwargs, **index_kwargs} with index_kwargs second, and set owns_client there unconditionally on the branch where it creates the client. A caller's owns_client=False was therefore discarded in silence, while SearchIndex.from_existing honoured the same argument via setdefault. Same keyword, same verb, opposite answer. Claim ownership only when the caller has not already answered, so all three public from_existing entry points agree. --- redisvl/extensions/router/semantic.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 4d9111e19..f3490d7b4 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -192,7 +192,11 @@ def from_existing( **factory_kwargs, ) index_kwargs["_client_validated"] = True - index_kwargs["owns_client"] = True + # index_kwargs wins the merge below, so only claim ownership when + # the caller has not already answered. Matches the setdefault in + # SearchIndex.from_existing, where an explicit value also wins. + if "owns_client" not in init_kwargs: + index_kwargs["owns_client"] = True if lib_name is not None: index_kwargs["lib_name"] = lib_name created_redis_client = True From e42acf5671db1de4384898eb58a9e2eb4183c55b Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 10:31:48 +0200 Subject: [PATCH 07/13] docs(index): document ownership and correct what the code claims Review of the owns_client work turned up several statements that were wrong or missing rather than merely terse. The owns_client entry said the index owns a client it created "from redis_url", but get_redis_connection falls back to REDIS_URL, so an index built with no connection arguments at all still creates and owns one. It also left the caller's obligation unstated: declining ownership of a client the index created means closing it yourself. disconnect was documented as "Disconnect from the Redis database" on the base and sync classes and not at all on the async one, which now misleads: it is a no-op for an unowned client, and with owns_client public that is a state callers choose. Its log line claimed the index did not own the client even when the index had created it. A test docstring asserted set_client() was already gone. It is not, until the next branch removes it, and pointing readers away from it hides the ownership footgun owns_client exists to fix. Also: coerce owns_client with bool(), since the finalizer gate tests truthiness while disconnect tested "is False", so a falsy non-bool made the two paths disagree; reject the retired private _owns_redis_client keyword loudly, because underscore-prefixed keywords are forwarded verbatim and it would otherwise be dropped in silence; hoist a lazily created client out of a per-key loop; drop the last dead client-is-None guard in the migration package; and stop one more warning promising removal in the next major release. --- redisvl/index/index.py | 99 ++++++++++++++++++--------- redisvl/mcp/server.py | 4 +- redisvl/migration/async_executor.py | 2 - redisvl/migration/validation.py | 5 +- redisvl/redis/connection.py | 2 +- tests/unit/test_index_gc_finalizer.py | 7 +- tests/unit/test_utils.py | 4 +- 7 files changed, 77 insertions(+), 46 deletions(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 4e03f489f..8eddcfffc 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -748,7 +748,12 @@ def from_dict(cls, schema_dict: dict[str, Any], **kwargs): return cls(schema=schema, **kwargs) def disconnect(self): - """Disconnect from the Redis database.""" + """Close the Redis client if this index owns it. + + Always invalidates the cached SQL schema. When the index does not own + the client (see ``owns_client``), the client is left open and the + index remains usable. + """ raise NotImplementedError("This method should be implemented by subclasses.") def key(self, id: str) -> str: @@ -832,12 +837,12 @@ def __init__( args. validate_on_load (bool, optional): Whether to validate data against schema when loading. Defaults to False. - owns_client (Optional[bool]): Whether the index should close the - Redis client when it is disconnected or garbage collected. By - default the index owns only a client it created itself from - `redis_url`, and never closes a client passed as - `redis_client`. Pass True to hand over a client you created, - or False to keep one the index would otherwise own. + owns_client (Optional[bool], optional): Whether the index closes + the Redis client when the index is disconnected or garbage + collected. Defaults to None, meaning the index owns a client + only if it created one itself. Pass True to hand over a client + you created, or False to keep one the index would otherwise + close, in which case closing it becomes your responsibility. """ if "connection_args" in kwargs: connection_kwargs = kwargs.pop("connection_args") @@ -857,12 +862,17 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - # An index owns the client it created itself, so a caller-supplied - # client is not closed unless the caller hands ownership over with - # owns_client=True. Assigned before the finalizer is registered, - # because _register_client_finalizer reads this flag. + if "_owns_redis_client" in kwargs: + # Underscore-prefixed kwargs are forwarded verbatim by + # _split_from_existing_kwargs, so this would otherwise be dropped + # in silence and leak the connection it used to control. + raise TypeError( + "_owns_redis_client is no longer accepted; use owns_client instead" + ) + # Must be assigned before _register_client_finalizer, which gates on + # this flag. self._owns_redis_client = ( - redis_client is None if owns_client is None else owns_client + redis_client is None if owns_client is None else bool(owns_client) ) self._client_finalizer = None # Close the owned client when this index is garbage collected. When @@ -873,10 +883,15 @@ def __init__( _finalizer_close_client = staticmethod(_close_owned_sync_client) def disconnect(self): - """Disconnect from the Redis database.""" + """Close the Redis client if this index owns it. + + Always invalidates the cached SQL schema. When the index does not own + the client (see ``owns_client``), the client is left open and the + index remains usable. + """ self.invalidate_sql_schema_cache() - if self._owns_redis_client is False: - logger.info("Index does not own client, not disconnecting") + if not self._owns_redis_client: + logger.info("Index does not own its client; leaving it open") return self._detach_client_finalizer() if self.__redis_client: @@ -900,6 +915,9 @@ def from_existing( instantiated redis client. redis_url (Optional[str]): The URL of the Redis server to connect to. + owns_client (Optional[bool], optional): Whether the index closes + the client. Defaults to True when this method created the + client from `redis_url`, and False when you supplied one. Raises: ValueError: If redis_url or redis_client is not provided. @@ -935,8 +953,6 @@ def from_existing( schema_dict = convert_index_info_to_schema(index_info) schema = IndexSchema.from_dict(schema_dict) if created_redis_client: - # The index created this client, so it owns it unless the caller - # explicitly said otherwise. init_kwargs.setdefault("owns_client", True) return cls( schema, @@ -2160,12 +2176,12 @@ def __init__( args. validate_on_load (bool, optional): Whether to validate data against schema when loading. Defaults to False. - owns_client (Optional[bool]): Whether the index should close the - Redis client when it is disconnected or garbage collected. By - default the index owns only a client it created itself from - `redis_url`, and never closes a client passed as - `redis_client`. Pass True to hand over a client you created, - or False to keep one the index would otherwise own. + owns_client (Optional[bool], optional): Whether the index closes + the Redis client when the index is disconnected or garbage + collected. Defaults to None, meaning the index owns a client + only if it created one itself. Pass True to hand over a client + you created, or False to keep one the index would otherwise + close, in which case closing it becomes your responsibility. """ if "redis_kwargs" in kwargs: connection_kwargs = kwargs.pop("redis_kwargs") @@ -2181,8 +2197,8 @@ def __init__( # Store connection parameters. Note the asymmetry with SearchIndex: # there, _redis_client is a property that lazily creates the client, # whereas here it is a plain attribute that stays None until - # _get_client() creates one. Read it through _get_client(), never - # directly, unless you specifically want the un-created state. + # _get_client() creates one. Read it through _get_client(), not + # directly. self._redis_client = redis_client self._redis_url = redis_url self._connection_kwargs = connection_kwargs or {} @@ -2190,10 +2206,17 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - # See the note on SearchIndex.__init__: ownership follows from who - # created the client, and owns_client=True hands it to the index. + if "_owns_redis_client" in kwargs: + # Underscore-prefixed kwargs are forwarded verbatim by + # _split_from_existing_kwargs, so this would otherwise be dropped + # in silence and leak the connection it used to control. + raise TypeError( + "_owns_redis_client is no longer accepted; use owns_client instead" + ) + # Must be assigned before _register_client_finalizer, which gates on + # this flag. self._owns_redis_client = ( - redis_client is None if owns_client is None else owns_client + redis_client is None if owns_client is None else bool(owns_client) ) self._client_finalizer = None # Close the owned client when this index is garbage collected. When @@ -2220,6 +2243,9 @@ async def from_existing( instantiated redis client. redis_url (Optional[str]): The URL of the Redis server to connect to. + owns_client (Optional[bool], optional): Whether the index closes + the client. Defaults to True when this method created the + client from `redis_url`, and False when you supplied one. """ if not redis_url and not redis_client: raise ValueError( @@ -2260,8 +2286,6 @@ async def from_existing( schema_dict = convert_index_info_to_schema(index_info) schema = IndexSchema.from_dict(schema_dict) if created_redis_client: - # The index created this client, so it owns it unless the caller - # explicitly said otherwise. init_kwargs.setdefault("owns_client", True) return cls( schema, @@ -3347,8 +3371,14 @@ async def info(self, name: str | None = None) -> dict[str, Any]: return await self._info(index_name, client) async def disconnect(self): + """Close the Redis client if this index owns it. + + Always invalidates the cached SQL schema. When the index does not own + the client (see ``owns_client``), the client is left open and the + index remains usable. + """ self.invalidate_sql_schema_cache() - if self._owns_redis_client is False: + if not self._owns_redis_client: return self._detach_client_finalizer() if self._redis_client is not None: @@ -3356,7 +3386,12 @@ async def disconnect(self): self._redis_client = None def disconnect_sync(self): - if self._redis_client is None or self._owns_redis_client is False: + """Close an owned Redis client from synchronous code. + + For callers outside an event loop, such as ``__del__`` or a shutdown + hook. Honours ``owns_client`` exactly as :meth:`disconnect` does. + """ + if self._redis_client is None or not self._owns_redis_client: return sync_wrapper(self.disconnect)() diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 53b894661..14097b1f0 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -644,9 +644,7 @@ async def _load_effective_schema( def _make_index(schema: IndexSchema, client: Any) -> AsyncSearchIndex: """Bind an inspected schema and Redis client into an async index.""" # The server acquired this client explicitly during startup, so hand - # ownership to the index for a single shutdown path. Passing - # owns_client at construction also registers the GC finalizer, which a - # post-construction flag flip would be too late for. + # ownership to the index for a single shutdown path. return AsyncSearchIndex(schema=schema, redis_client=client, owns_client=True) async def _initialize_vectorizer( diff --git a/redisvl/migration/async_executor.py b/redisvl/migration/async_executor.py index 149ae0e9d..87310c61f 100644 --- a/redisvl/migration/async_executor.py +++ b/redisvl/migration/async_executor.py @@ -867,8 +867,6 @@ def _notify(step: str, detail: Optional[str] = None) -> None: try: client = await source_index._get_client() - if client is None: - raise ValueError("Failed to get Redis client from source index") aof_enabled = await self._detect_aof_enabled(client) disk_estimate = estimate_disk_space(plan, aof_enabled=aof_enabled) if disk_estimate.has_quantization: diff --git a/redisvl/migration/validation.py b/redisvl/migration/validation.py index 6735d6479..fcf14ed8f 100644 --- a/redisvl/migration/validation.py +++ b/redisvl/migration/validation.py @@ -87,9 +87,8 @@ def validate( keys_to_check.append(translated) # Check keys one at a time to avoid Redis Cluster cross-slot # errors from multi-key EXISTS commands. - existing_count = sum( - target_index._redis_client.exists(key) for key in keys_to_check - ) + client = target_index._redis_client + existing_count = sum(client.exists(key) for key in keys_to_check) validation.key_sample_exists = existing_count == len(keys_to_check) # Run automatic functional checks (always). diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index eca8a2f78..d4e44ea8d 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -726,7 +726,7 @@ def get_async_redis_connection( variable is not set. """ warn( - "get_async_redis_connection will become async in the next major release.", + "get_async_redis_connection will become async in a future release.", DeprecationWarning, ) _deprecated_url = kwargs.pop("url", None) diff --git a/tests/unit/test_index_gc_finalizer.py b/tests/unit/test_index_gc_finalizer.py index 37bb2ee3c..edc0aed05 100644 --- a/tests/unit/test_index_gc_finalizer.py +++ b/tests/unit/test_index_gc_finalizer.py @@ -229,9 +229,10 @@ class TestOwnsClientHandover: """``owns_client`` overrides who closes the client. By default an index closes only a client it created itself. These tests - cover the two explicit overrides, which are the only way to hand a - pre-built client over (or to keep one the index would otherwise own) now - that ``set_client()`` is gone. + cover the two explicit overrides at construction, which is where + ownership should be stated: the deprecated ``set_client()`` inherits + whatever ownership the index already had, so it can hand the index a + caller's client and then close it. """ def test_sync_injected_client_closed_when_ownership_handed_over(self): diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index ef0a17670..e4e2893fd 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -242,7 +242,7 @@ def test_func(old_arg=None, neutral_arg=None): assert len(record) == 1 assert str(record[0].message) == ( - "Argument old_arg is deprecated and will be removed" " in a future release." + "Argument old_arg is deprecated and will be removed in a future release." ) # As a positional arg @@ -251,7 +251,7 @@ def test_func(old_arg=None, neutral_arg=None): assert len(record) == 1 assert str(record[0].message) == ( - "Argument old_arg is deprecated and will be removed" " in a future release." + "Argument old_arg is deprecated and will be removed in a future release." ) with assert_no_warnings(): From 0bbc251fbde597de571defbb21ef9b012faeee77 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 10:51:58 +0200 Subject: [PATCH 08/13] refactor(index)!: remove the deprecated connect and set_client methods SearchIndex.connect, SearchIndex.set_client and their async counterparts have warned since v0.4.0, and AsyncSearchIndex.connect emitted three DeprecationWarnings per call because it delegated to set_client. None had working ownership semantics: set_client attached a finalizer based on whatever ownership the index already had, so it closed a caller's client on an index built from redis_url and never took ownership on one built with a client. Pass connection parameters to the constructor instead, with owns_client when the index should close a client you supplied. AsyncSearchIndex._validate_client goes with them, since set_client was its only caller. It carried the sync-to-async client coercion, so handing an async index a sync client is now rejected rather than silently converted. Both constructors gained a guard for the wrong client flavour, which also restores the rejection set_client used to provide for the sync index. connection_args and redis_kwargs are removed too. They had a second, undecorated entry point through from_existing that warned about nothing, so the deprecated spelling would otherwise have outlived the removal. Both now raise a TypeError naming connection_kwargs, because **kwargs would otherwise swallow them in silence; the message names keywords only, never values, since connection kwargs carry passwords. With those gone every caller of _split_from_existing_kwargs passes the same tuple, so nested_connection_keys is inlined. One side effect worth recording: @deprecated_argument wraps the function it decorates in an untyped wrapper, so mypy had never checked a single call site of either constructor. Removing it surfaced four pre-existing type errors in the semantic cache, fixed here with the cast idiom the cache base module already uses. --- redisvl/extensions/cache/llm/semantic.py | 16 ++- redisvl/extensions/router/semantic.py | 5 +- redisvl/index/index.py | 135 ++++--------------- redisvl/redis/connection.py | 35 ++++- tests/integration/test_async_search_index.py | 41 ++---- tests/integration/test_search_index.py | 28 +--- tests/unit/test_connection_normalization.py | 40 ++++++ 7 files changed, 119 insertions(+), 181 deletions(-) diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index 87383057f..cd99e3f64 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -1,5 +1,5 @@ import asyncio -from typing import Any +from typing import Any, cast from pydantic import ValidationError from redis import Redis @@ -187,8 +187,11 @@ def __init__( self._index = SearchIndex( schema=schema, redis_client=self._redis_client, - redis_url=self.redis_kwargs["redis_url"], - connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, + redis_url=cast(str | None, self.redis_kwargs["redis_url"]), + connection_kwargs=cast( + "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] + ) + or None, ) self._aindex = None @@ -260,8 +263,11 @@ async def _get_async_index(self) -> AsyncSearchIndex: self._aindex = AsyncSearchIndex( schema=self._index.schema, redis_client=async_client, - redis_url=self.redis_kwargs["redis_url"], - connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, + redis_url=cast(str | None, self.redis_kwargs["redis_url"]), + connection_kwargs=cast( + "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] + ) + or None, ) return self._aindex diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index f3490d7b4..a6ffa9e18 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -171,10 +171,7 @@ def from_existing( overwrite = kwargs.pop("overwrite", False) if not create_index and overwrite: raise ValueError(CREATE_INDEX_OVERWRITE_CONFLICT) - init_kwargs, connection_kwargs = _split_from_existing_kwargs( - dict(kwargs), - nested_connection_keys=("connection_kwargs",), - ) + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = init_kwargs.get("lib_name") index_kwargs: dict[str, Any] = {} created_redis_client = False diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 8eddcfffc..5b68089ee 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -39,7 +39,7 @@ make_dict, ) from redisvl.types import AsyncRedisClient, SyncRedisClient, SyncRedisCluster -from redisvl.utils.utils import deprecated_argument, deprecated_function, sync_wrapper +from redisvl.utils.utils import deprecated_argument, sync_wrapper if TYPE_CHECKING: from redis.commands.search.aggregation import AggregateResult @@ -75,6 +75,7 @@ from redisvl.query.filter import FilterExpression from redisvl.redis.connection import ( RedisConnectionFactory, + _reject_removed_kwargs, _split_from_existing_kwargs, convert_index_info_to_schema, supports_svs, @@ -813,7 +814,6 @@ class SearchIndex(BaseSearchIndex): """ - @deprecated_argument("connection_args", "Use connection_kwargs instead.") def __init__( self, schema: IndexSchema, @@ -844,12 +844,21 @@ def __init__( you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. """ - if "connection_args" in kwargs: - connection_kwargs = kwargs.pop("connection_args") + _reject_removed_kwargs(kwargs) if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") + # Reject the wrong client flavour up front. isinstance, not + # issubclass(type(...)): Mock(spec=AsyncRedis) sets __class__, so this + # catches a mis-flavoured spec'd mock while leaving bare mocks alone. + # Rests on the sync and async hierarchies staying disjoint. + if isinstance(redis_client, (AsyncRedis, AsyncRedisCluster)): + raise TypeError( + "SearchIndex requires a sync Redis client; pass an async " + "client to AsyncSearchIndex instead." + ) + self.schema = schema self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) @@ -922,10 +931,7 @@ def from_existing( Raises: ValueError: If redis_url or redis_client is not provided. """ - init_kwargs, connection_kwargs = _split_from_existing_kwargs( - dict(kwargs), - nested_connection_keys=("connection_kwargs", "connection_args"), - ) + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = cast(str | None, init_kwargs.get("lib_name")) created_redis_client = False @@ -996,52 +1002,6 @@ def _redis_client(self) -> SyncRedisClient: self._validated_client = True return self.__redis_client - @deprecated_function("connect", "Pass connection parameters in __init__.") - def connect(self, redis_url: str | None = None, **kwargs): - """Connect to a Redis instance using the provided `redis_url`, falling - back to the `REDIS_URL` environment variable (if available). - - Note: Additional keyword arguments (`**kwargs`) can be used to provide - extra options specific to the Redis connection. - - Args: - redis_url (Optional[str], optional): The URL of the Redis server to - connect to. - - Raises: - redis.exceptions.ConnectionError: If the connection to the Redis - server fails. - ValueError: If the Redis URL is not provided nor accessible - through the `REDIS_URL` environment variable. - ModuleNotFoundError: If required Redis modules are not installed. - """ - self.invalidate_sql_schema_cache() - self.__redis_client = RedisConnectionFactory.get_redis_connection( - redis_url=redis_url, **kwargs - ) - self._register_client_finalizer(self.__redis_client) - - @deprecated_function("set_client", "Pass connection parameters in __init__.") - def set_client(self, redis_client: SyncRedisClient, **kwargs): - """Manually set the Redis client to use with the search index. - - This method configures the search index to use a specific Redis or - Async Redis client. It is useful for cases where an external, - custom-configured client is preferred instead of creating a new one. - - Args: - redis_client (Redis): A Redis or Async Redis - client instance to be used for the connection. - - Raises: - TypeError: If the provided client is not valid. - """ - RedisConnectionFactory.validate_sync_redis(redis_client) - self.invalidate_sql_schema_cache() - self.__redis_client = redis_client - self._register_client_finalizer(redis_client) - return self - def _check_svs_support(self) -> None: """Validate SVS-VAMANA support. @@ -2152,7 +2112,6 @@ class AsyncSearchIndex(BaseSearchIndex): """ - @deprecated_argument("redis_kwargs", "Use connection_kwargs instead.") def __init__( self, schema: IndexSchema, @@ -2183,13 +2142,19 @@ def __init__( you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. """ - if "redis_kwargs" in kwargs: - connection_kwargs = kwargs.pop("redis_kwargs") + _reject_removed_kwargs(kwargs) # final validation on schema object if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") + # See the note in SearchIndex.__init__ on why this is isinstance. + if isinstance(redis_client, (Redis, RedisCluster)): + raise TypeError( + "AsyncSearchIndex requires an async Redis client; pass a sync " + "client to SearchIndex instead." + ) + self.schema = schema self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) @@ -2252,10 +2217,7 @@ async def from_existing( "Must provide either a redis_url or redis_client to fetch Redis index info." ) - init_kwargs, connection_kwargs = _split_from_existing_kwargs( - dict(kwargs), - nested_connection_keys=("connection_kwargs", "redis_kwargs"), - ) + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = cast(str | None, init_kwargs.get("lib_name")) created_redis_client = False @@ -2301,33 +2263,6 @@ def client(self) -> AsyncRedisClient | None: """The underlying redis-py client object.""" return self._redis_client - @deprecated_function("connect", "Pass connection parameters in __init__.") - async def connect(self, redis_url: str | None = None, **kwargs): - """[DEPRECATED] Connect to a Redis instance. Use connection parameters in __init__.""" - warnings.warn( - "connect() is deprecated; pass connection parameters in __init__", - DeprecationWarning, - ) - self.invalidate_sql_schema_cache() - client = await RedisConnectionFactory._get_aredis_connection( - redis_url=redis_url, **kwargs - ) - await self.set_client(client) - - @deprecated_function("set_client", "Pass connection parameters in __init__.") - async def set_client(self, redis_client: AsyncRedisClient | SyncRedisClient): - """ - [DEPRECATED] Manually set the Redis client to use with the search index. - This method is deprecated; please provide connection parameters in __init__. - """ - redis_client = await self._validate_client(redis_client) - self.invalidate_sql_schema_cache() - await self.disconnect() - async with self._lock: - self._redis_client = redis_client - self._register_client_finalizer(redis_client) - return self - async def _get_client(self) -> AsyncRedisClient: """Lazily instantiate and return the async Redis client.""" if self._redis_client is None: @@ -2353,30 +2288,6 @@ async def _get_client(self) -> AsyncRedisClient: self._validated_client = True return self._redis_client - async def _validate_client( - self, redis_client: AsyncRedisClient | SyncRedisClient - ) -> AsyncRedisClient: - # Handle deprecated sync client conversion - if isinstance(redis_client, (Redis, RedisCluster)): - warnings.warn( - "Passing a sync Redis client to AsyncSearchIndex is deprecated " - "and will be removed in the next major version. Please use an " - "async Redis client instead.", - DeprecationWarning, - ) - # Use a new variable name - async_redis_client: AsyncRedisClient = ( - RedisConnectionFactory.sync_to_async_redis(redis_client) - ) - return async_redis_client # Return the converted client - # Check if it's a valid async client (standard or cluster) - elif not isinstance(redis_client, (AsyncRedis, AsyncRedisCluster)): - raise ValueError( - "Invalid async client type: must be AsyncRedis or AsyncRedisCluster" - ) - # If it passed the elif, it's already an AsyncRedisClient - return redis_client - @staticmethod async def _info(name: str, redis_client: AsyncRedisClient) -> dict[str, Any]: try: diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index d4e44ea8d..1bf80646a 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -1,5 +1,6 @@ import os -from typing import Any, Sequence, TypeVar, overload +from collections.abc import Mapping +from typing import Any, TypeVar, overload from urllib.parse import parse_qs, urlencode, urlparse, urlunparse from warnings import warn @@ -29,9 +30,32 @@ logger = get_logger(__name__) +# Connection keywords that were removed rather than renamed. Kept as a table so +# a caller who passes one gets told what to use instead, rather than having the +# name silently ignored or forwarded to redis-py as an unexpected argument. +REMOVED_CONNECTION_KWARGS = { + "connection_args": "connection_kwargs", + "redis_kwargs": "connection_kwargs", +} + + +def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: + """Raise if a caller passed a keyword that has been removed. + + The message names the keyword and its replacement only. Connection + keywords carry passwords, so echoing a rejected value would put a live + credential into an exception string and from there into logs. + """ + for name, replacement in REMOVED_CONNECTION_KWARGS.items(): + if name in kwargs: + raise TypeError(f"{name} is no longer supported; use {replacement} instead") + + def _split_from_existing_kwargs( - kwargs: dict[str, Any], *, nested_connection_keys: Sequence[str] + kwargs: dict[str, Any], ) -> tuple[dict[str, Any], dict[str, Any]]: + _reject_removed_kwargs(kwargs) + init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} @@ -43,10 +67,9 @@ def _split_from_existing_kwargs( if key.startswith("_"): init_kwargs[key] = kwargs.pop(key) - for key in nested_connection_keys: - nested_kwargs = kwargs.pop(key, None) - if nested_kwargs is not None: - connection_kwargs.update(nested_kwargs) + nested_kwargs = kwargs.pop("connection_kwargs", None) + if nested_kwargs is not None: + connection_kwargs.update(nested_kwargs) connection_kwargs.update(kwargs) return init_kwargs, connection_kwargs diff --git a/tests/integration/test_async_search_index.py b/tests/integration/test_async_search_index.py index c52387e6b..863a15ef9 100644 --- a/tests/integration/test_async_search_index.py +++ b/tests/integration/test_async_search_index.py @@ -1,10 +1,8 @@ -import warnings from random import choice from unittest import mock import pytest import redis -from redis import Redis as SyncRedis from redis.asyncio import Redis as AsyncRedis from redisvl.exceptions import QueryValidationError, RedisSearchError, RedisVLError @@ -208,27 +206,15 @@ async def test_search_index_client(async_client, index_schema): assert async_index.client == async_client -@pytest.mark.asyncio -async def test_search_index_set_client(client, redis_url, index_schema): - # Use async with for the index that owns its initial client via redis_url - async with AsyncSearchIndex( - schema=index_schema, redis_url=redis_url - ) as async_index: - # Ignore deprecation warnings for set_client - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - await async_index.create(overwrite=True, drop=True) - assert isinstance(async_index.client, AsyncRedis) - - # Tests deprecated sync -> async conversion behavior - assert isinstance(client, SyncRedis) - - await async_index.set_client(client) - assert isinstance(async_index.client, AsyncRedis) +def test_async_search_index_rejects_sync_client(client, index_schema): + """An async index must not accept a sync client. - if async_index.client: - await async_index.disconnect() - assert async_index.client is None + The removed set_client() silently converted one via + sync_to_async_redis; the constructor now rejects it outright, so the + mismatch surfaces at construction rather than at the first await. + """ + with pytest.raises(TypeError): + AsyncSearchIndex(schema=index_schema, redis_client=client) @pytest.mark.asyncio @@ -779,17 +765,6 @@ async def test_search_index_validates_query_with_hnsw_algorithm( await async_hnsw_index.query(query) -@pytest.mark.asyncio -async def test_async_search_index_connect(index_schema, redis_url): - """Test that AsyncSearchIndex.connect() works with redis_url parameter.""" - async_index = AsyncSearchIndex(schema=index_schema) - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - await async_index.connect(redis_url=redis_url) - assert async_index.client is not None - await async_index.disconnect() - - @pytest.mark.asyncio @pytest.mark.parametrize("ttl", [None, 30]) async def test_search_index_load_with_ttl(async_index, ttl): diff --git a/tests/integration/test_search_index.py b/tests/integration/test_search_index.py index f1c6c8293..55c43b596 100644 --- a/tests/integration/test_search_index.py +++ b/tests/integration/test_search_index.py @@ -1,4 +1,3 @@ -import warnings from random import choice from unittest import mock @@ -276,27 +275,14 @@ def test_search_index_client(client, index_schema): assert index.client == client -def test_search_index_set_client(async_client, redis_url, index_schema): - index = SearchIndex(schema=index_schema, redis_url=redis_url) - - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - index.create(overwrite=True, drop=True) - assert index.client - # should not be able to set an async client here - with pytest.raises(TypeError): - index.set_client(async_client) - assert index.client is not async_client +def test_search_index_rejects_async_client(async_client, index_schema): + """A sync index must not accept an async client. - index.disconnect() - assert index.client is None - - -def test_search_index_connect(index, redis_url): - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - index.connect(redis_url=redis_url) - assert index.client + Previously only reachable through the removed set_client(); the + constructor now rejects it before any connection is attempted. + """ + with pytest.raises(TypeError): + SearchIndex(schema=index_schema, redis_client=async_client) def test_search_index_create(index): diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index 28560f79a..e58cfb1ab 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -8,6 +8,7 @@ from redisvl.extensions.router.semantic import SemanticRouter from redisvl.index import AsyncSearchIndex, SearchIndex from redisvl.query.sql import SQLQuery +from redisvl.schema import IndexSchema from redisvl.utils.utils import assert_no_warnings @@ -436,3 +437,42 @@ def test_sql_query_does_not_create_new_connection_when_client_provided(): schema_cache_strategy="lazy", ) assert command == "FT.SEARCH idx *" + + +@pytest.mark.parametrize( + "factory", + [ + lambda **kw: SearchIndex(IndexSchema.from_dict(_schema_dict()), **kw), + lambda **kw: AsyncSearchIndex(IndexSchema.from_dict(_schema_dict()), **kw), + ], + ids=["sync", "async"], +) +@pytest.mark.parametrize("removed", ["connection_args", "redis_kwargs"]) +def test_constructors_reject_removed_connection_kwargs(factory, removed): + """Removed keywords must fail loudly, not be swallowed by **kwargs.""" + with pytest.raises(TypeError) as excinfo: + factory(**{removed: {"decode_responses": True}}) + + assert removed in str(excinfo.value) + assert "connection_kwargs" in str(excinfo.value) + + +def test_from_existing_rejects_removed_connection_kwargs(): + """The from_existing path routed these into connection_kwargs before.""" + with pytest.raises(TypeError) as excinfo: + SearchIndex.from_existing( + "idx", redis_url="redis://localhost:6379", connection_args={"db": 1} + ) + + assert "connection_kwargs" in str(excinfo.value) + + +def test_rejection_message_never_echoes_the_value(): + """Connection kwargs carry passwords; the message names keys only.""" + with pytest.raises(TypeError) as excinfo: + SearchIndex( + IndexSchema.from_dict(_schema_dict()), + connection_args={"password": "s3cr3t-do-not-log"}, + ) + + assert "s3cr3t-do-not-log" not in str(excinfo.value) From 29b4900c882313ab64250643467035fee1d662c9 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 11:16:56 +0200 Subject: [PATCH 09/13] refactor(index): address review of the deprecated-client removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review perspectives ran against the removal. Their substantive findings, in one commit because they overlap: The removed-keyword table was unreachable on one path that mattered. AsyncSearchIndex.from_existing checked for a client before splitting kwargs, so a caller passing redis_kwargs — the async alias, and so the likeliest caller — got a generic "must provide redis_url or redis_client" instead of being told the current spelling. Split first. The wrong-flavour guard was duplicated in both constructors and unreachable from from_existing, where validate_sync_redis fired first with a message that says what is wrong but not what to do. It now lives on BaseSearchIndex, driven by two class attributes, and runs ahead of the broader validation on both paths. Its message leads with what the index needs, names the class it got — qualified, since redis.Redis and redis.asyncio.Redis share a bare __name__ — and ends with the advice, which for the async index now includes sync_to_async_redis for callers who relied on the coercion that went away with _validate_client. The guard is deliberately one-sided: it rejects the wrong flavour but does not assert the object is a client at all, because a positive check would reject the unspecced mocks much of the suite injects. That is now recorded in the docstring rather than left for a reader to infer, and the isinstance choice is pinned by a test instead of only asserted in a comment. _owns_redis_client folds into the same table, replacing two inline blocks and leaving one mechanism for "that keyword is gone" rather than two. The table drops its claim that these keywords were "removed rather than renamed", which its own values contradicted, and no longer says "no longer supported" — the callers sharing it never all accepted every name, so the router was telling users a keyword it never took had been withdrawn. Both guard tests move to the unit suite: neither touches Redis, and the sync one requested the async fixture, which pytest-asyncio 0.24 stopped allowing. Also fixes a pre-existing duplicate in the async ownership tests, where the disconnect_sync case awaited disconnect() instead and so left disconnect_sync on an owned client untested. --- redisvl/index/index.py | 90 +++++++++++++------- redisvl/redis/connection.py | 19 +++-- tests/integration/test_async_search_index.py | 13 +-- tests/integration/test_search_index.py | 10 --- tests/unit/test_connection_normalization.py | 38 +++++++++ tests/unit/test_index_gc_finalizer.py | 6 +- 6 files changed, 113 insertions(+), 63 deletions(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 5b68089ee..a5112a657 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -11,6 +11,7 @@ Any, AsyncGenerator, Callable, + ClassVar, Generator, Iterable, Iterator, @@ -571,9 +572,44 @@ class BaseSearchIndex: _client_finalizer: "weakref.finalize | None" = None + # Set per subclass: the client classes this index cannot use, what it + # needs instead, and where to send the caller. + _wrong_flavour_clients: ClassVar[tuple[type, ...]] + _wrong_flavour_needs: ClassVar[str] + _wrong_flavour_advice: ClassVar[str] + def __init__(*args, **kwargs): pass + @classmethod + def _reject_wrong_client_flavour(cls, redis_client: Any) -> None: + """Reject a sync client on an async index, or the reverse. + + Uses ``isinstance`` rather than ``issubclass(type(...))`` so a + ``Mock(spec=...)`` of the wrong flavour is caught, since it sets + ``__class__``, while an unspecced mock still passes. Rests on the sync + and async client hierarchies staying disjoint, which holds across the + supported redis-py range. + + Deliberately one-sided: it rejects the wrong flavour but does not + assert the object is a Redis client at all, so a connection pool or + an unrelated object still gets through and fails later. A positive + check would reject the unspecced mocks the test suite relies on. + + Called from ``__init__`` and, ahead of the broader + ``validate_sync_redis``/``validate_async_redis``, from + ``from_existing``, so this message wins on both paths. + """ + if isinstance(redis_client, cls._wrong_flavour_clients): + wrong = type(redis_client) + # Qualified name because redis.Redis and redis.asyncio.Redis share + # a bare __name__, which would make the message useless here. + raise TypeError( + f"{cls.__name__} requires {cls._wrong_flavour_needs}, got " + f"{wrong.__module__}.{wrong.__qualname__}. " + f"{cls._wrong_flavour_advice}" + ) + def _register_client_finalizer(self, client: Any) -> None: """Register a finalizer that closes ``client`` once this index is garbage collected. @@ -849,15 +885,7 @@ def __init__( if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") - # Reject the wrong client flavour up front. isinstance, not - # issubclass(type(...)): Mock(spec=AsyncRedis) sets __class__, so this - # catches a mis-flavoured spec'd mock while leaving bare mocks alone. - # Rests on the sync and async hierarchies staying disjoint. - if isinstance(redis_client, (AsyncRedis, AsyncRedisCluster)): - raise TypeError( - "SearchIndex requires a sync Redis client; pass an async " - "client to AsyncSearchIndex instead." - ) + self._reject_wrong_client_flavour(redis_client) self.schema = schema self._validate_on_load = validate_on_load @@ -871,13 +899,6 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - if "_owns_redis_client" in kwargs: - # Underscore-prefixed kwargs are forwarded verbatim by - # _split_from_existing_kwargs, so this would otherwise be dropped - # in silence and leak the connection it used to control. - raise TypeError( - "_owns_redis_client is no longer accepted; use owns_client instead" - ) # Must be assigned before _register_client_finalizer, which gates on # this flag. self._owns_redis_client = ( @@ -890,6 +911,11 @@ def __init__( self._register_client_finalizer(redis_client) _finalizer_close_client = staticmethod(_close_owned_sync_client) + _wrong_flavour_clients = (AsyncRedis, AsyncRedisCluster) + _wrong_flavour_needs = "a sync Redis client" + _wrong_flavour_advice = ( + "Pass a sync client here, or use AsyncSearchIndex for async clients." + ) def disconnect(self): """Close the Redis client if this index owns it. @@ -936,6 +962,8 @@ def from_existing( created_redis_client = False if redis_client: + # Ahead of validate_*_redis, whose message is generic. + cls._reject_wrong_client_flavour(redis_client) # Validate client type and set lib name RedisConnectionFactory.validate_sync_redis(redis_client, lib_name) # Mark that client was already validated to avoid duplicate calls @@ -2148,12 +2176,7 @@ def __init__( if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") - # See the note in SearchIndex.__init__ on why this is isinstance. - if isinstance(redis_client, (Redis, RedisCluster)): - raise TypeError( - "AsyncSearchIndex requires an async Redis client; pass a sync " - "client to SearchIndex instead." - ) + self._reject_wrong_client_flavour(redis_client) self.schema = schema self._validate_on_load = validate_on_load @@ -2171,13 +2194,6 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - if "_owns_redis_client" in kwargs: - # Underscore-prefixed kwargs are forwarded verbatim by - # _split_from_existing_kwargs, so this would otherwise be dropped - # in silence and leak the connection it used to control. - raise TypeError( - "_owns_redis_client is no longer accepted; use owns_client instead" - ) # Must be assigned before _register_client_finalizer, which gates on # this flag. self._owns_redis_client = ( @@ -2190,6 +2206,13 @@ def __init__( self._register_client_finalizer(redis_client) _finalizer_close_client = staticmethod(_close_owned_async_client) + _wrong_flavour_clients = (Redis, RedisCluster) + _wrong_flavour_needs = "an async Redis client" + _wrong_flavour_advice = ( + "Pass an async client here, use SearchIndex for sync clients, or " + "convert a non-cluster client with " + "RedisConnectionFactory.sync_to_async_redis()." + ) @classmethod async def from_existing( @@ -2212,16 +2235,21 @@ async def from_existing( the client. Defaults to True when this method created the client from `redis_url`, and False when you supplied one. """ + # Split before the presence check below: redis_kwargs was the async + # alias, so a caller passing it with no client would otherwise get the + # generic ValueError rather than being told the current spelling. + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) + if not redis_url and not redis_client: raise ValueError( "Must provide either a redis_url or redis_client to fetch Redis index info." ) - - init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = cast(str | None, init_kwargs.get("lib_name")) created_redis_client = False if redis_client: + # Ahead of validate_*_redis, whose message is generic. + cls._reject_wrong_client_flavour(redis_client) # Validate client type and set lib name await RedisConnectionFactory.validate_async_redis(redis_client, lib_name) # Mark that client was already validated to avoid duplicate calls diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 1bf80646a..e1763174c 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -30,25 +30,30 @@ logger = get_logger(__name__) -# Connection keywords that were removed rather than renamed. Kept as a table so -# a caller who passes one gets told what to use instead, rather than having the -# name silently ignored or forwarded to redis-py as an unexpected argument. -REMOVED_CONNECTION_KWARGS = { +# Old spellings and their current equivalents. Kept as a table so **kwargs +# cannot swallow one in silence: a caller who passes an old name is told the +# current one, rather than having it ignored or forwarded to redis-py as an +# unexpected argument. +_REMOVED_CONNECTION_KWARGS = { "connection_args": "connection_kwargs", "redis_kwargs": "connection_kwargs", + "_owns_redis_client": "owns_client", } def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: - """Raise if a caller passed a keyword that has been removed. + """Raise if a caller passed a keyword that no longer exists. The message names the keyword and its replacement only. Connection keywords carry passwords, so echoing a rejected value would put a live credential into an exception string and from there into logs. + + It says "not a supported keyword" rather than "no longer supported" + because the callers sharing this table never all accepted every name. """ - for name, replacement in REMOVED_CONNECTION_KWARGS.items(): + for name, replacement in _REMOVED_CONNECTION_KWARGS.items(): if name in kwargs: - raise TypeError(f"{name} is no longer supported; use {replacement} instead") + raise TypeError(f"{name} is not a supported keyword; use {replacement}") def _split_from_existing_kwargs( diff --git a/tests/integration/test_async_search_index.py b/tests/integration/test_async_search_index.py index 863a15ef9..6b0185d12 100644 --- a/tests/integration/test_async_search_index.py +++ b/tests/integration/test_async_search_index.py @@ -206,17 +206,6 @@ async def test_search_index_client(async_client, index_schema): assert async_index.client == async_client -def test_async_search_index_rejects_sync_client(client, index_schema): - """An async index must not accept a sync client. - - The removed set_client() silently converted one via - sync_to_async_redis; the constructor now rejects it outright, so the - mismatch surfaces at construction rather than at the first await. - """ - with pytest.raises(TypeError): - AsyncSearchIndex(schema=index_schema, redis_client=client) - - @pytest.mark.asyncio async def test_search_index_create(async_index): await async_index.create(overwrite=True, drop=True) @@ -477,7 +466,7 @@ async def test_search_index_that_owns_client_disconnect(index_schema, redis_url) async def test_search_index_that_owns_client_disconnect_sync(index_schema, redis_url): async_index = AsyncSearchIndex(schema=index_schema, redis_url=redis_url) await async_index.create(overwrite=True, drop=True) - await async_index.disconnect() + async_index.disconnect_sync() assert async_index._redis_client is None diff --git a/tests/integration/test_search_index.py b/tests/integration/test_search_index.py index 55c43b596..783d328d3 100644 --- a/tests/integration/test_search_index.py +++ b/tests/integration/test_search_index.py @@ -275,16 +275,6 @@ def test_search_index_client(client, index_schema): assert index.client == client -def test_search_index_rejects_async_client(async_client, index_schema): - """A sync index must not accept an async client. - - Previously only reachable through the removed set_client(); the - constructor now rejects it before any connection is attempted. - """ - with pytest.raises(TypeError): - SearchIndex(schema=index_schema, redis_client=async_client) - - def test_search_index_create(index): index.create(overwrite=True, drop=True) assert index.exists() diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index e58cfb1ab..14ded5044 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -3,6 +3,8 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from redis import Redis +from redis.asyncio import Redis as AsyncRedis from redisvl.extensions.cache.embeddings import EmbeddingsCache from redisvl.extensions.router.semantic import SemanticRouter @@ -476,3 +478,39 @@ def test_rejection_message_never_echoes_the_value(): ) assert "s3cr3t-do-not-log" not in str(excinfo.value) + + +@pytest.mark.parametrize( + "index_cls, wrong_client", + [ + (SearchIndex, AsyncRedis.from_url("redis://localhost:6379")), + (AsyncSearchIndex, Redis.from_url("redis://localhost:6379")), + ], + ids=["sync-index-async-client", "async-index-sync-client"], +) +def test_constructors_reject_the_wrong_client_flavour(index_cls, wrong_client): + """A mismatched client fails at construction, not at first use. + + For the sync index this rejection was previously reachable only through + the removed set_client(); for the async index the removed + _validate_client() silently converted the client instead. + """ + with pytest.raises(TypeError) as excinfo: + index_cls(IndexSchema.from_dict(_schema_dict()), redis_client=wrong_client) + + assert "Redis client" in str(excinfo.value) + + +def test_wrong_flavour_guard_catches_specced_mocks(): + """Pins the isinstance choice the guard documents. + + Mock(spec=...) sets __class__, so isinstance catches a mis-flavoured + spec'd mock where issubclass(type(...)) would not. An unspecced mock must + still pass, because much of the suite injects one. + """ + schema = IndexSchema.from_dict(_schema_dict()) + + with pytest.raises(TypeError): + SearchIndex(schema, redis_client=MagicMock(spec=AsyncRedis)) + + assert SearchIndex(schema, redis_client=MagicMock()) is not None diff --git a/tests/unit/test_index_gc_finalizer.py b/tests/unit/test_index_gc_finalizer.py index edc0aed05..5d2a4f439 100644 --- a/tests/unit/test_index_gc_finalizer.py +++ b/tests/unit/test_index_gc_finalizer.py @@ -229,9 +229,9 @@ class TestOwnsClientHandover: """``owns_client`` overrides who closes the client. By default an index closes only a client it created itself. These tests - cover the two explicit overrides at construction, which is where - ownership should be stated: the deprecated ``set_client()`` inherits - whatever ownership the index already had, so it can hand the index a + cover the two explicit overrides at construction, which is now the only + place ownership can be stated: the removed ``set_client()`` inherited + whatever ownership the index already had, and so could be handed a caller's client and then close it. """ From 950afe807ac40e3afbe0526c6b815979c52b8973 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 11:20:15 +0200 Subject: [PATCH 10/13] docs(index): restore the client-injection guidance the removal dropped set_client()'s docstring was the only place in the rendered API reference that explained using a client you configured yourself, and docs/api/searchindex.rst renders both classes with a bare autoclass, so deleting the method took the explanation with it. redis_client= now appears in no example anywhere. Both class docstrings gain one, along with when the index will and will not close such a client. Neither constructor documented the three TypeErrors it can now raise, so the rename of connection_args and redis_kwargs existed only inside an exception string and a private table. Both from_existing docstrings also omitted connection_kwargs, the keyword those exceptions name as the replacement, even though they accept it through **kwargs. _split_from_existing_kwargs gains a docstring: its name promises splitting, it now also rejects, and it consumes the dict it is handed. Replaces the four casts the previous commit added, plus four that predated it, with a TypedDict on BaseCache.redis_kwargs. The casts were all working around one missing annotation on a heterogeneous dict literal, and cast() asserted a type the checker could not verify, so nothing stopped a later edit from making the assertion false. A TypedDict is a plain dict at runtime, so this is annotation-only. --- redisvl/extensions/cache/base.py | 26 +++++++++----- redisvl/extensions/cache/llm/semantic.py | 16 +++------ redisvl/index/index.py | 44 ++++++++++++++++++++++++ redisvl/redis/connection.py | 7 ++++ 4 files changed, 74 insertions(+), 19 deletions(-) diff --git a/redisvl/extensions/cache/base.py b/redisvl/extensions/cache/base.py index 212faec2b..39b5566c1 100644 --- a/redisvl/extensions/cache/base.py +++ b/redisvl/extensions/cache/base.py @@ -6,7 +6,7 @@ import asyncio from collections.abc import Mapping -from typing import Any, cast +from typing import Any, TypedDict from redis import Redis # For backwards compatibility in type checking from redis.cluster import RedisCluster @@ -15,6 +15,18 @@ from redisvl.types import AsyncRedisClient, SyncRedisClient +class _CacheConnectionKwargs(TypedDict): + """The connection parameters a cache keeps for building its clients. + + Annotated so each value keeps its own type. Without it the dict literal + widens to the union of all three, and every read needs a cast. + """ + + redis_client: SyncRedisClient | None + redis_url: str + connection_kwargs: dict[str, Any] + + class BaseCache: """Base abstract cache interface for all RedisVL caches. @@ -51,7 +63,7 @@ def __init__( self._ttl: int | None = None self.set_ttl(ttl) - self.redis_kwargs = { + self.redis_kwargs: _CacheConnectionKwargs = { "redis_client": redis_client, "redis_url": redis_url, "connection_kwargs": connection_kwargs, @@ -121,8 +133,8 @@ def _get_redis_client(self) -> SyncRedisClient: """ if self._redis_client is None: # Create new Redis client - url = cast(str | None, self.redis_kwargs["redis_url"]) - kwargs = cast(dict[str, Any], self.redis_kwargs["connection_kwargs"]) + url = self.redis_kwargs["redis_url"] + kwargs = self.redis_kwargs["connection_kwargs"] self._redis_client = RedisConnectionFactory.get_redis_connection( redis_url=url, **kwargs, @@ -146,10 +158,8 @@ async def _get_async_redis_client(self) -> AsyncRedisClient: if provided and isinstance(provided, (Redis, RedisCluster)): client = RedisConnectionFactory.sync_to_async_redis(provided) else: - url = cast(str | None, self.redis_kwargs["redis_url"]) - kwargs = cast( - dict[str, Any], self.redis_kwargs["connection_kwargs"] - ) + url = self.redis_kwargs["redis_url"] + kwargs = self.redis_kwargs["connection_kwargs"] client = await RedisConnectionFactory._get_aredis_connection( redis_url=url, **kwargs ) diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index cd99e3f64..87383057f 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -1,5 +1,5 @@ import asyncio -from typing import Any, cast +from typing import Any from pydantic import ValidationError from redis import Redis @@ -187,11 +187,8 @@ def __init__( self._index = SearchIndex( schema=schema, redis_client=self._redis_client, - redis_url=cast(str | None, self.redis_kwargs["redis_url"]), - connection_kwargs=cast( - "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] - ) - or None, + redis_url=self.redis_kwargs["redis_url"], + connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, ) self._aindex = None @@ -263,11 +260,8 @@ async def _get_async_index(self) -> AsyncSearchIndex: self._aindex = AsyncSearchIndex( schema=self._index.schema, redis_client=async_client, - redis_url=cast(str | None, self.redis_kwargs["redis_url"]), - connection_kwargs=cast( - "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] - ) - or None, + redis_url=self.redis_kwargs["redis_url"], + connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, ) return self._aindex diff --git a/redisvl/index/index.py b/redisvl/index/index.py index a5112a657..748474cb1 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -848,6 +848,18 @@ class SearchIndex(BaseSearchIndex): # delete index and data index.delete(drop=True) + Pass ``redis_client`` to use a client you configured yourself. The index + leaves such a client open when it is disconnected or garbage collected; + pass ``owns_client=True`` to hand that responsibility over. + + .. code-block:: python + + from redis import Redis + from redisvl.index import SearchIndex + + client = Redis.from_url("redis://localhost:6379", socket_timeout=5) + index = SearchIndex.from_yaml("schemas/schema.yaml", redis_client=client) + """ def __init__( @@ -879,6 +891,12 @@ def __init__( only if it created one itself. Pass True to hand over a client you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. + + Raises: + ValueError: If ``schema`` is not an IndexSchema. + TypeError: If ``redis_client`` is the wrong flavour, or if a + keyword that no longer exists is passed. ``connection_args`` + and ``redis_kwargs`` are both now ``connection_kwargs``. """ _reject_removed_kwargs(kwargs) @@ -953,6 +971,10 @@ def from_existing( owns_client (Optional[bool], optional): Whether the index closes the client. Defaults to True when this method created the client from `redis_url`, and False when you supplied one. + connection_kwargs (Optional[Dict[str, Any]]): Redis client + connection args, used only when this method creates the + client from ``redis_url``. Other keyword arguments are + treated the same way. Raises: ValueError: If redis_url or redis_client is not provided. @@ -2138,6 +2160,18 @@ class AsyncSearchIndex(BaseSearchIndex): # delete index and data await index.delete(drop=True) + Pass ``redis_client`` to use a client you configured yourself. The index + leaves such a client open when it is disconnected or garbage collected; + pass ``owns_client=True`` to hand that responsibility over. + + .. code-block:: python + + from redis.asyncio import Redis + from redisvl.index import AsyncSearchIndex + + client = Redis.from_url("redis://localhost:6379", socket_timeout=5) + index = AsyncSearchIndex.from_yaml("schemas/schema.yaml", redis_client=client) + """ def __init__( @@ -2169,6 +2203,12 @@ def __init__( only if it created one itself. Pass True to hand over a client you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. + + Raises: + ValueError: If ``schema`` is not an IndexSchema. + TypeError: If ``redis_client`` is the wrong flavour, or if a + keyword that no longer exists is passed. ``connection_args`` + and ``redis_kwargs`` are both now ``connection_kwargs``. """ _reject_removed_kwargs(kwargs) @@ -2234,6 +2274,10 @@ async def from_existing( owns_client (Optional[bool], optional): Whether the index closes the client. Defaults to True when this method created the client from `redis_url`, and False when you supplied one. + connection_kwargs (Optional[Dict[str, Any]]): Redis client + connection args, used only when this method creates the + client from ``redis_url``. Other keyword arguments are + treated the same way. """ # Split before the presence check below: redis_kwargs was the async # alias, so a caller passing it with no client would otherwise get the diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index e1763174c..1acacb8c9 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -59,6 +59,13 @@ def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: def _split_from_existing_kwargs( kwargs: dict[str, Any], ) -> tuple[dict[str, Any], dict[str, Any]]: + """Split ``from_existing`` kwargs into constructor and connection halves. + + Consumes ``kwargs`` in place. ``connection_kwargs`` is merged flat into + the connection half, so a caller may either nest or spread it. Removed + keywords are rejected first, so a stale spelling fails here rather than + reaching redis-py as an unexpected argument. + """ _reject_removed_kwargs(kwargs) init_kwargs: dict[str, Any] = {} From d9ad737ff370cfc24ec4a5e94d12f735688f533d Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 11:24:16 +0200 Subject: [PATCH 11/13] test(index): pin what disconnect_sync actually does in a running loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The async ownership tests had two copies of the same assertion: the disconnect_sync case awaited disconnect() instead of calling disconnect_sync(), so the method it was named for was never exercised. Calling it revealed why that mattered — inside a running loop it is a silent no-op, because sync_wrapper reaches loop.run_until_complete on an already-running loop and swallows the RuntimeError. That is the intended design: the method exists for callers outside a loop, such as __del__ and shutdown hooks, and the closing path is covered there by the finalizer unit tests. But an undocumented silent no-op reads as a bug to whoever finds it next, so the test now asserts it and the docstring says it. --- redisvl/index/index.py | 3 +++ tests/integration/test_async_search_index.py | 17 ++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 748474cb1..6effd34f8 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -3373,6 +3373,9 @@ def disconnect_sync(self): For callers outside an event loop, such as ``__del__`` or a shutdown hook. Honours ``owns_client`` exactly as :meth:`disconnect` does. + + Does nothing if a loop is already running, since it cannot await the + close from inside one. Use :meth:`disconnect` there. """ if self._redis_client is None or not self._owns_redis_client: return diff --git a/tests/integration/test_async_search_index.py b/tests/integration/test_async_search_index.py index 6b0185d12..ed2109f2c 100644 --- a/tests/integration/test_async_search_index.py +++ b/tests/integration/test_async_search_index.py @@ -463,10 +463,25 @@ async def test_search_index_that_owns_client_disconnect(index_schema, redis_url) @pytest.mark.asyncio -async def test_search_index_that_owns_client_disconnect_sync(index_schema, redis_url): +async def test_disconnect_sync_is_a_no_op_inside_a_running_loop( + index_schema, redis_url +): + """disconnect_sync() does nothing when a loop is already running. + + It exists for callers outside an event loop, such as __del__ and + shutdown hooks: sync_wrapper calls loop.run_until_complete, which raises + on an already-running loop and is swallowed. Asserting the no-op here + documents behaviour that would otherwise look like a silent bug. The + path that does close the client is covered outside a loop by + tests/unit/test_index_gc_finalizer.py. + """ async_index = AsyncSearchIndex(schema=index_schema, redis_url=redis_url) await async_index.create(overwrite=True, drop=True) + async_index.disconnect_sync() + + assert async_index._redis_client is not None + await async_index.disconnect() assert async_index._redis_client is None From 29c37b1cc4228183a09da3c3ad582954ca20e6ce Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 10 Sep 2026 13:09:11 +0200 Subject: [PATCH 12/13] refactor(migration): drop a cast import left dead by the merge Main replaced the hand-rolled SCAN loop in the validator with scan_iter, which removed the only cast() in the file. `make lint` runs format and mypy but not pylint, so nothing in the standard checks would flag it. --- redisvl/migration/validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redisvl/migration/validation.py b/redisvl/migration/validation.py index c33253817..83a7598c8 100644 --- a/redisvl/migration/validation.py +++ b/redisvl/migration/validation.py @@ -1,7 +1,7 @@ from __future__ import annotations import time -from typing import Any, Dict, List, Optional, cast +from typing import Any, Dict, List, Optional from redis.commands.search.query import Query From 5c2a65845b223441e0a84b334ded43bde591f812 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 23 Sep 2026 13:04:03 +0200 Subject: [PATCH 13/13] fix: keep the cache lock loop-local and treat owns_client=None as unanswered Two review findings on #726, both in code this branch introduced. The lock that serialises lazy async client creation in BaseCache was built in __init__ and held for the cache's lifetime. An asyncio.Lock records the loop that first contends it and raises RuntimeError from any other, so reconnecting after adisconnect() failed whenever it landed on a new loop -- a second asyncio.run, a fresh pytest-asyncio loop, a notebook cell. It now rebuilds when the running loop changes, which is all a lock guarding one object's creation needs. Only the contended acquire reaches the loop check, so the regression test has to race two callers on each loop; a single-caller version passes either way. from_existing claimed a factory-built client with setdefault("owns_client", True), which an explicit owns_client=None satisfies without replacing. __init__ then inferred ownership from redis_client -- already populated by then -- read it as not-owned, and left the client it had just built for nobody to close. A caller forwarding an optional setting lands there. Both index flavours and SemanticRouter now test for None rather than for presence. AsyncSearchIndex._lock carries the same loop-binding flaw on main, untouched by this branch, and is left for its own change. --- redisvl/extensions/cache/base.py | 26 ++++++- redisvl/extensions/router/semantic.py | 6 +- redisvl/index/index.py | 14 +++- tests/unit/test_connection_normalization.py | 75 +++++++++++++++++++++ 4 files changed, 113 insertions(+), 8 deletions(-) diff --git a/redisvl/extensions/cache/base.py b/redisvl/extensions/cache/base.py index 2993c874c..0ed9b34f8 100644 --- a/redisvl/extensions/cache/base.py +++ b/redisvl/extensions/cache/base.py @@ -65,8 +65,10 @@ def __init__( self._async_redis_client = async_redis_client self._redis_client = redis_client # Guards lazy async client creation, which suspends on an await and so - # cannot rely on a bare check-then-set. Mirrors AsyncSearchIndex._lock. - self._async_client_lock = asyncio.Lock() + # cannot rely on a bare check-then-set. Rebuilt whenever the running + # loop changes -- see _get_async_client_lock. + self._async_client_lock: asyncio.Lock | None = None + self._async_client_lock_loop: asyncio.AbstractEventLoop | None = None # Caches never close a caller-supplied client and register no GC # finalizer, so the index's owns_client handover has no cache @@ -133,6 +135,24 @@ def _get_redis_client(self) -> SyncRedisClient: ) return self._redis_client + def _get_async_client_lock(self) -> asyncio.Lock: + """Return the client-creation lock, bound to the running loop. + + An ``asyncio.Lock`` records the loop that first contends it and raises + ``RuntimeError`` if awaited from any other, so one long-lived lock + would break a reconnect that lands on a second loop -- a later + ``asyncio.run``, a fresh pytest-asyncio loop, a notebook cell. The + lock only has to serialise tasks within a single loop, so discarding + it when the loop changes is both sufficient and cheap. + """ + loop = asyncio.get_running_loop() + lock = self._async_client_lock + if lock is None or self._async_client_lock_loop is not loop: + lock = asyncio.Lock() + self._async_client_lock = lock + self._async_client_lock_loop = loop + return lock + async def _get_async_redis_client(self) -> AsyncRedisClient: """Get or create an async Redis client. @@ -141,7 +161,7 @@ async def _get_async_redis_client(self) -> AsyncRedisClient: """ client = getattr(self, "_async_redis_client", None) if client is None: - async with self._async_client_lock: + async with self._get_async_client_lock(): # Double-check: another task may have created the client while # this one waited on the lock or on the factory's round trip. client = getattr(self, "_async_redis_client", None) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 787afd3bf..5bf3b1c5f 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -198,9 +198,9 @@ def from_existing( ) index_kwargs["_client_validated"] = True # index_kwargs wins the merge below, so only claim ownership when - # the caller has not already answered. Matches the setdefault in - # SearchIndex.from_existing, where an explicit value also wins. - if "owns_client" not in init_kwargs: + # the caller has not already answered -- and an explicit None is + # not an answer. Matches SearchIndex.from_existing. + if init_kwargs.get("owns_client") is None: index_kwargs["owns_client"] = True if lib_name is not None: index_kwargs["lib_name"] = lib_name diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 24180352e..b5354ed70 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -1008,7 +1008,12 @@ def from_existing( schema_dict = convert_index_info_to_schema(index_info) schema = IndexSchema.from_dict(schema_dict) if created_redis_client: - init_kwargs.setdefault("owns_client", True) + # An explicit owns_client=None means "infer", and __init__ infers + # from redis_client -- non-None here, so it would read as + # not-owned and leak the client this method just built. Treat an + # unanswered value as the documented default. + if init_kwargs.get("owns_client") is None: + init_kwargs["owns_client"] = True return cls( schema, redis_client=redis_client, @@ -2411,7 +2416,12 @@ async def from_existing( schema_dict = convert_index_info_to_schema(index_info) schema = IndexSchema.from_dict(schema_dict) if created_redis_client: - init_kwargs.setdefault("owns_client", True) + # An explicit owns_client=None means "infer", and __init__ infers + # from redis_client -- non-None here, so it would read as + # not-owned and leak the client this method just built. Treat an + # unanswered value as the documented default. + if init_kwargs.get("owns_client") is None: + init_kwargs["owns_client"] = True return cls( schema, redis_client=redis_client, diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index 28560f79a..7be979cf4 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -142,6 +142,41 @@ def test_search_index_from_existing_honours_explicit_owns_client(): created_client.close.assert_not_called() +def test_search_index_from_existing_owns_client_when_ownership_unanswered(): + """An explicit ``owns_client=None`` must not leak the factory's client. + + ``None`` is the documented "work it out for me" value, and ``__init__`` + works it out from ``redis_client`` -- which ``from_existing`` has already + populated, so the inference reads as not-owned and nothing ever closes + the client this call built. A caller forwarding an optional setting + (``owns_client=cfg.get("owns_client")``) lands here. + """ + created_client = MagicMock() + + with ( + patch( + "redisvl.index.index.RedisConnectionFactory.get_redis_connection", + return_value=created_client, + ), + patch.object(SearchIndex, "_info", return_value={}), + patch( + "redisvl.index.index.convert_index_info_to_schema", + return_value=_schema_dict("search-index"), + ), + ): + index = SearchIndex.from_existing( + "search-index", + redis_url="redis://localhost:6380", + owns_client=None, + ) + + assert index._owns_redis_client is True + + index.disconnect() + + created_client.close.assert_called_once_with() + + @pytest.mark.asyncio async def test_async_search_index_from_existing_prefers_provided_client(): """Use the provided async Redis client instead of constructing a new one.""" @@ -389,6 +424,46 @@ async def factory(*args, **kwargs): assert first is second +def test_base_cache_async_client_reconnects_on_a_second_event_loop(): + """Reconnecting after ``adisconnect`` must work on a fresh event loop. + + An ``asyncio.Lock`` records the loop that first contends it and rejects + every other, so a lock held for the cache's lifetime turned the + documented reconnect into a ``RuntimeError`` whenever it landed on a new + loop -- a second ``asyncio.run``, a fresh pytest-asyncio loop, a notebook + cell. Driven from a sync test because it needs two real loops. + """ + cache = EmbeddingsCache(redis_url="redis://localhost:6379") + created = [] + + async def factory(*args, **kwargs): + await asyncio.sleep(0) # the suspension the real factory introduces + client = AsyncMock(name=f"client{len(created)}") + created.append(client) + return client + + async def race(): + # Two racing callers force the lock's slow path, which is the only + # path that looks at the loop; an uncontended acquire never does, so + # a single-caller version of this test would pass either way. + await asyncio.gather( + cache._get_async_redis_client(), cache._get_async_redis_client() + ) + + async def connect_then_disconnect(): + await race() + await cache.adisconnect() + + with patch( + "redisvl.extensions.cache.base.RedisConnectionFactory._get_aredis_connection", + new=factory, + ): + asyncio.run(connect_then_disconnect()) + asyncio.run(race()) + + assert len(created) == 2, "each loop should get exactly one client" + + def test_sql_query_uses_connection_factory_for_redis_url(): """Build SQL query helper connections through the shared connection factory.""" fake_sql_redis_module = _fake_sql_redis_module()