diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index ce70fe8a..c7f20400 100644 --- a/docs/user_guide/installation.md +++ b/docs/user_guide/installation.md @@ -196,7 +196,8 @@ The command-to-category mapping below was measured against live servers rather t |---|---|---|---| | `index.query()`, `index.search()`, `index.aggregate()` | `FT.SEARCH`, `FT.AGGREGATE` | Yes | Yes | | `index.load()` | `HSET` or `JSON.SET` (needs key access) | Yes | Yes | -| `index.exists()`, `index.info()`, `index.clear()`, `SearchIndex.from_existing()`, `rvl index info`, `rvl stats` | `FT.INFO` | Yes | **No** | +| `index.clear()` | `FT.SEARCH`, then `DEL` per batch | Yes | Yes | +| `index.exists()`, `index.info()`, `SearchIndex.from_existing()`, `rvl index info`, `rvl stats` | `FT.INFO` | Yes | **No** | | `index.create()` | `FT.CREATE` | Yes | **No** | | `index.delete()`, `rvl index delete`, `rvl index destroy` | `FT.DROPINDEX` | Yes | Yes | | Enumerating indexes (see below) | `FT._LIST` | **No** | **No** | @@ -240,7 +241,7 @@ cache = SemanticCache( `create_index=False` is available on `SemanticCache`, `MessageHistory`, `SemanticMessageHistory` and `SemanticRouter`. It skips the existence check, the comparison of your schema against the live index, and index creation — the constructor issues no index command at all. Pass it when the index is managed externally, or when the credential cannot run `FT.INFO`. It cannot be combined with `overwrite=True`, which asks for the opposite. -A `SearchIndex` used directly needs nothing special: build it with `from_dict()` or `from_yaml()`, then load and query. Two of its methods stay unavailable, because both read index metadata: `from_existing()`, which reconstructs a schema out of Redis, and `clear()`, which starts by calling `info()`. +A `SearchIndex` used directly needs nothing special: build it with `from_dict()` or `from_yaml()`, then load and query. The methods that stay unavailable are the ones that read index metadata — `exists()`, `info()`, and `from_existing()`, which reconstructs a schema out of Redis. `clear()` is not among them: it enumerates with `FT.SEARCH` and deletes in batches, so it needs no more than querying does. The flag also skips the SVS-VAMANA capability probe described above, since that runs inside `create()`. @@ -264,7 +265,27 @@ With `create_index=False` nothing verifies that the live index matches the schem For the silent cases the tell is `FT.INFO`'s `key_type`, `prefixes` and `attributes` — not `hash_indexing_failures`, which stays `0` because those keys were never indexing candidates. Diagnosing it therefore needs a credential that can run `FT.INFO`. -An extension constructed with `create_index=False` refuses index-wide `delete()` and `clear()` operations (and their async cache equivalents). This protects an externally managed index — including an index reached through an alias — from being destroyed through an attach-only instance. Targeted operations such as dropping a specific cache entry or message remain available. Perform lifecycle-wide destructive operations through the privileged provisioning path that owns the index. +### What an attach-only instance may still do + +Removing *entries* is available on every path, and is how a caller invalidates an externally managed cache without holding the provisioning credential: `clear()` (plus `SemanticCache.aclear()`), and targeted removal of a specific cache entry, message or route. None of it removes the index, and all of it runs under `+@read +@write`. + +What `create_index=False` refuses is `delete()` (and `SemanticCache.adelete()`), because that drops the index. Refusing it protects an externally managed index — including one reached through an alias — from being destroyed through an attach-only instance. Drop the index through the privileged provisioning path that owns it. + +The two kinds of `clear()` decide *which keys go* differently, and neither choice is verified against the live index under this flag: + +| Method | Deletes | Chooses keys by | +|---|---|---| +| `SemanticCache.clear()`, `aclear()` | every key under `{name}:` | `SCAN`/`DEL` on the prefix this instance declares — no index command at all | +| `MessageHistory.clear()`, `SemanticMessageHistory.clear()`, `SemanticRouter.clear()` | every document the live index covers | `FT.SEARCH` paging via `SearchIndex.clear()` | + +`FT.SEARCH` is in `@read` as well as `@search`, so a `+@read +@write` credential is granted it — unlike `FT.INFO`, which is in neither and is what made these three unavailable before. Note that `FT.SEARCH` additionally requires the credential's key patterns to be a superset of the index prefixes, the same rule described under [Key permissions](#key-permissions). + +Because the two enumerate differently, they fail differently, and the section above is what decides which failure you get. Both are silent: + +- **Prefix-based clearing deletes too much, or nothing.** `SCAN`/`DEL` is blind to the index and to the key type, so it removes every key under `{name}:` — another writer's entries, and unrelated application data sharing that namespace root. And if the live index covers a *different* prefix, or is an alias onto one, `clear()` deletes only what this instance itself wrote and leaves every served entry in place: it reports success and the cache still returns the stale hits you called it to invalidate. +- **Index-based clearing deletes documents you never wrote.** `SearchIndex.clear()` deletes what the live index covers, so against an index on a different prefix — or a multi-`PREFIX` index, or an alias — it removes another application's documents while leaving this instance's own unindexed entries behind. + +Diagnosing either needs `FT.INFO`, which is the command an attach-only credential does not have. If the index is provisioned for you, get its `prefixes` and `key_type` from whoever provisions it and make your extension's name match, rather than inferring it from a successful query. ### Key permissions diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index 87383057..439423cc 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -14,7 +14,7 @@ CACHE_VECTOR_FIELD_NAME, CREATE_INDEX_OVERWRITE_CONFLICT, ENTRY_ID_FIELD_NAME, - EXTERNAL_INDEX_LIFECYCLE_CONFLICT, + EXTERNAL_INDEX_DROP_CONFLICT, INSERTED_AT_FIELD_NAME, METADATA_FIELD_NAME, PROMPT_FIELD_NAME, @@ -309,28 +309,46 @@ def set_threshold(self, distance_threshold: float) -> None: self._distance_threshold = float(distance_threshold) def delete(self) -> None: - """Delete the cache and its index entirely.""" + """Delete the cache and its index entirely. + + Raises: + ValueError: If ``create_index=False``. Use :meth:`clear` to + empty the cache and leave the index standing. + """ if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + raise ValueError(EXTERNAL_INDEX_DROP_CONFLICT) self._index.delete(drop=True) async def adelete(self) -> None: - """Async delete the cache and its index entirely.""" + """Async delete the cache and its index entirely. + + Raises: + ValueError: If ``create_index=False``. See :meth:`delete`. + """ if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + raise ValueError(EXTERNAL_INDEX_DROP_CONFLICT) aindex = await self._get_async_index() await aindex.delete(drop=True) def clear(self) -> None: - """Clear all cache keys when RedisVL manages the index lifecycle.""" - if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + """Delete every cache entry, leaving the index in place. + + Clears by key prefix, not by index membership, so it removes every key + under ``{name}:`` and nothing outside it. Available under + ``create_index=False``; dropping the index is :meth:`delete`. + + Warning: + Under ``create_index=False`` the prefix is unverified, so this can + delete keys the index never covered and miss entries it does. See + :doc:`/user_guide/installation`. + """ super().clear() async def aclear(self) -> None: - """Async clear all cache keys when RedisVL manages the index lifecycle.""" - if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + """Async delete every cache entry, leaving the index in place. + + See :meth:`clear` for the caveats, which apply identically here. + """ await super().aclear() def drop(self, ids: list[str] | None = None, keys: list[str] | None = None) -> None: diff --git a/redisvl/extensions/constants.py b/redisvl/extensions/constants.py index c31bb7ef..2271be35 100644 --- a/redisvl/extensions/constants.py +++ b/redisvl/extensions/constants.py @@ -42,9 +42,11 @@ ) # Raised when an extension attached to an externally managed index is asked to -# perform an index-wide destructive operation. -EXTERNAL_INDEX_LIFECYCLE_CONFLICT: str = ( - "Cannot delete or clear an index when create_index=False because RedisVL " - "does not manage that index's lifecycle. Use the externally managed " - "provisioning path to perform index-wide destructive operations." +# drop that index. Removing entries is deliberately not covered: `clear()` +# leaves the index in place, so it is not a lifecycle operation. +EXTERNAL_INDEX_DROP_CONFLICT: str = ( + "Cannot delete the index when create_index=False because RedisVL does not " + "manage that index's lifecycle. Use the externally managed provisioning " + "path to drop it. To remove every entry while leaving the index in place, " + "use clear()." ) diff --git a/redisvl/extensions/message_history/message_history.py b/redisvl/extensions/message_history/message_history.py index 03204389..ca8f5e07 100644 --- a/redisvl/extensions/message_history/message_history.py +++ b/redisvl/extensions/message_history/message_history.py @@ -4,7 +4,7 @@ from redisvl.extensions.constants import ( CONTENT_FIELD_NAME, - EXTERNAL_INDEX_LIFECYCLE_CONFLICT, + EXTERNAL_INDEX_DROP_CONFLICT, ID_FIELD_NAME, METADATA_FIELD_NAME, ROLE_FIELD_NAME, @@ -95,15 +95,28 @@ def __repr__(self) -> str: return f"MessageHistory(name={self._name!r}, session_tag={self._session_tag!r})" def clear(self) -> None: - """Clears the conversation message history.""" - if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + """Delete every message, leaving the index in place. + + Clears by index membership, so it removes the documents the live index + covers. Available under ``create_index=False``; dropping the index is + :meth:`delete`. + + Warning: + Under ``create_index=False`` the live index is unverified, so if its + prefix differs from this instance's it removes documents this + instance never wrote. See :doc:`/user_guide/installation`. + """ self._index.clear() def delete(self) -> None: - """Clear all conversation keys and remove the search index.""" + """Remove every message and drop the search index. + + Raises: + ValueError: If ``create_index=False``. Use :meth:`clear` to + remove the messages and leave the index standing. + """ if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + raise ValueError(EXTERNAL_INDEX_DROP_CONFLICT) self._index.delete(drop=True) def drop(self, id: str | None = None) -> None: diff --git a/redisvl/extensions/message_history/semantic_history.py b/redisvl/extensions/message_history/semantic_history.py index 22ae5daa..e8ba8417 100644 --- a/redisvl/extensions/message_history/semantic_history.py +++ b/redisvl/extensions/message_history/semantic_history.py @@ -5,7 +5,7 @@ from redisvl.extensions.constants import ( CONTENT_FIELD_NAME, CREATE_INDEX_OVERWRITE_CONFLICT, - EXTERNAL_INDEX_LIFECYCLE_CONFLICT, + EXTERNAL_INDEX_DROP_CONFLICT, ID_FIELD_NAME, MESSAGE_VECTOR_FIELD_NAME, METADATA_FIELD_NAME, @@ -156,15 +156,28 @@ def __repr__(self) -> str: ) def clear(self) -> None: - """Clears the message history.""" - if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + """Delete every message, leaving the index in place. + + Clears by index membership, so it removes the documents the live index + covers. Available under ``create_index=False``; dropping the index is + :meth:`delete`. + + Warning: + Under ``create_index=False`` the live index is unverified, so if its + prefix differs from this instance's it removes documents this + instance never wrote. See :doc:`/user_guide/installation`. + """ self._index.clear() def delete(self) -> None: - """Clear all message keys and remove the search index.""" + """Remove every message and drop the search index. + + Raises: + ValueError: If ``create_index=False``. Use :meth:`clear` to + remove the messages and leave the index standing. + """ if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + raise ValueError(EXTERNAL_INDEX_DROP_CONFLICT) self._index.delete(drop=True) def drop(self, id: str | None = None) -> None: diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 16472615..5958c1bf 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -9,7 +9,7 @@ from redisvl.extensions.constants import ( CREATE_INDEX_OVERWRITE_CONFLICT, - EXTERNAL_INDEX_LIFECYCLE_CONFLICT, + EXTERNAL_INDEX_DROP_CONFLICT, ROUTE_VECTOR_FIELD_NAME, ) from redisvl.extensions.router.schema import ( @@ -654,6 +654,11 @@ def add_route(self, route: Route) -> str: def remove_route(self, route_name: str) -> None: """Remove a route and all references from the semantic router. + Like :meth:`add_route`, this replaces the router's stored config with + this instance's route list, so removing one route from a router holding + only a subset drops the rest from the config :meth:`from_existing` + reads. + Args: route_name (str): Name of the route to remove. """ @@ -671,18 +676,31 @@ def remove_route(self, route_name: str) -> None: self._update_router_state() def delete(self) -> None: - """Delete the semantic router index and its persisted route config.""" + """Delete the semantic router index and its persisted route config. + + Raises: + ValueError: If ``create_index=False``. Use :meth:`clear` to + remove the route references and leave the index standing. + """ if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + raise ValueError(EXTERNAL_INDEX_DROP_CONFLICT) self._index.delete(drop=True) # The route config is stored as a standalone JSON key that is not # tracked by the search index, so it must be removed explicitly. self._index._redis_client.delete(f"{self.name}:route_config") def clear(self) -> None: - """Flush all routes from the semantic router index.""" - if not self._create_index: - raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + """Delete every route reference, leaving the index in place. + + Clears by index membership. Available under ``create_index=False``; + dropping the index is :meth:`delete`. + + Warning: + The stored ``route_config`` is left as it was, here and on the + default path. A separate process calling :meth:`from_existing` + afterwards will report routes whose reference vectors are gone. + :meth:`remove_route` keeps the two in step. + """ self._index.clear() self.routes = [] diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 3daa69d3..e7017591 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -1151,28 +1151,70 @@ def clear(self) -> int: here, we can't easily give control of the keys we're clearing to the user so they can separate them based on hash tag. + Note: + The sweep enumerates through the index, so it removes only what the + index currently returns, and it can stop early -- against an index + still being backfilled, or when a page's keys cannot be deleted. The + returned count is the only signal, and ``0`` does not distinguish an + empty index from a sweep that deleted nothing. Re-running is safe. + Returns: int: Count of records deleted from Redis. """ batch_size = 500 - max_ratio = 1.01 - info = self.info() - max_records_deleted = ceil( - info["num_docs"] * max_ratio - ) # Allow to remove some additional concurrent inserts + matched = cast(int, self.query(CountQuery(FilterExpression("*")))) + # Runaway backstop sized to the matched count plus slack for concurrent + # inserts, as in drop_by_filter. Deliberately not FT.INFO's num_docs: + # that command is @search only, so reading it for a loop bound denied + # this whole method to a `+@read +@write` credential. + max_records = ceil(matched * 1.5) + batch_size + total_records_deleted: int = 0 + offset = 0 query = FilterQuery(FilterExpression("*"), return_fields=["id"]) - query.paging(0, batch_size) while True: + if total_records_deleted > max_records: + logger.warning( + "clear() of index %s hit its runaway backstop (%d) with " + "documents possibly still indexed; %d records were deleted. " + "Re-run to continue.", + self.schema.index.name, + max_records, + total_records_deleted, + ) + break + + query.paging(offset, batch_size) batch = self._query(query) - if batch and total_records_deleted <= max_records_deleted: - batch_keys = [record["id"] for record in batch] - total_records_deleted += self._delete_batch(batch_keys) - else: + if not batch: break + batch_keys = [record["id"] for record in batch] + records_deleted = self._delete_batch(batch_keys) + total_records_deleted += records_deleted + + if records_deleted: + # Deleted documents leave the index, so the next page of + # survivors is at offset 0 again. + offset = 0 + else: + # Nothing in this page could be deleted, most plausibly a + # permission denial swallowed by _delete_batch's cluster branch. + # Page past it: the documents behind may still be deletable. + offset += batch_size + if offset > max_records: + logger.warning( + "clear() of index %s paged past its runaway backstop " + "(%d) without being able to delete; %d records were " + "deleted. Documents remain.", + self.schema.index.name, + max_records, + total_records_deleted, + ) + break + self.invalidate_sql_schema_cache() return total_records_deleted @@ -2485,28 +2527,67 @@ async def clear(self) -> int: we can't easily give control of the keys we're clearing to the user so they can separate them based on hash tag. + See :meth:`SearchIndex.clear` for the sweep's caveats, which apply + identically here. + Returns: int: Count of records deleted from Redis. """ batch_size = 500 - max_ratio = 1.01 - info = await self.info() - max_records_deleted = ceil( - info["num_docs"] * max_ratio - ) # Allow to remove some additional concurrent inserts + matched = cast(int, await self.query(CountQuery(FilterExpression("*")))) + # Runaway backstop sized to the matched count plus slack for concurrent + # inserts -- the same shape as drop_by_filter. CountQuery is FT.SEARCH, + # which the query loop below already needs and which `+@read +@write` + # grants; the FT.INFO this once read for the same purpose is `@search` + # only, so a single call for a loop bound denied the whole method. + max_records = ceil(matched * 1.5) + batch_size + total_records_deleted: int = 0 + offset = 0 query = FilterQuery(FilterExpression("*"), return_fields=["id"]) - query.paging(0, batch_size) while True: + if total_records_deleted > max_records: + logger.warning( + "clear() of index %s hit its runaway backstop (%d) with " + "documents possibly still indexed; %d records were deleted. " + "Re-run to continue.", + self.schema.index.name, + max_records, + total_records_deleted, + ) + break + + query.paging(offset, batch_size) batch = await self._query(query) - if batch and total_records_deleted <= max_records_deleted: - batch_keys = [record["id"] for record in batch] - total_records_deleted += await self._delete_batch(batch_keys) - else: + if not batch: break + batch_keys = [record["id"] for record in batch] + records_deleted = await self._delete_batch(batch_keys) + total_records_deleted += records_deleted + + if records_deleted: + # Deleted documents leave the index, so the next page of + # survivors is at offset 0 again. + offset = 0 + else: + # Nothing in this page could be deleted, most plausibly a + # permission denial swallowed by _delete_batch's cluster branch. + # Page past it: the documents behind may still be deletable. + offset += batch_size + if offset > max_records: + logger.warning( + "clear() of index %s paged past its runaway backstop " + "(%d) without being able to delete; %d records were " + "deleted. Documents remain.", + self.schema.index.name, + max_records, + total_records_deleted, + ) + break + self.invalidate_sql_schema_cache() return total_records_deleted diff --git a/tests/integration/test_llmcache.py b/tests/integration/test_llmcache.py index 8bd4c69a..4f5c5918 100644 --- a/tests/integration/test_llmcache.py +++ b/tests/integration/test_llmcache.py @@ -1212,3 +1212,58 @@ def test_create_index_false_works_under_a_read_write_acl( ) assert "ft.info" in str(excinfo.value).lower() assert isinstance(excinfo.value.__cause__, NoPermissionError) + + +@pytest.mark.asyncio +async def test_create_index_false_can_invalidate_but_not_drop( + cache, vectorizer, redis_url, acl_user +): + """A restricted credential can clear its entries but not drop the index. + + Needs a live server for the parts that matter: that a real `+@read +@write` + user is admitted, that the index survives, and that the async path carries + the same credential. Folded into one test because the ACL user and the + pre-created index are the expensive fixtures. + """ + cache.store("What is the capital of France?", "Paris") + + with acl_user( + "~*", "&*", "+@read", "+@write", "-@dangerous", name="acl_clear_user" + ) as user: + credentials = {"username": user.username, "password": user.password} + restricted_cache = SemanticCache( + name=cache.index.name, + vectorizer=vectorizer, + distance_threshold=0.2, + redis_url=redis_url, + connection_kwargs=credentials, + create_index=False, + ) + + try: + restricted_cache.clear() + # Read back through the privileged instance, so the assertion does + # not depend on the restricted one still working. + assert cache.check("What is the capital of France?") == [] + + # The async path builds its own client, so it needs its own run. + cache.store("Who wrote Hamlet?", "Shakespeare") + await restricted_cache.aclear() + assert cache.check("Who wrote Hamlet?") == [] + + # Dropping the index remains refused on both paths. + with pytest.raises(ValueError, match="does not manage.*lifecycle"): + restricted_cache.delete() + with pytest.raises(ValueError, match="does not manage.*lifecycle"): + await restricted_cache.adelete() + finally: + # Not fixture-tracked, and the ACL user is about to go away. + await restricted_cache.adisconnect() + restricted_cache.disconnect() + + # exists() proves the definition survived; the round-trip below proves it + # is still usable, which exists() alone would not. + assert cache.index.exists() + cache.store("Who wrote Hamlet?", "Shakespeare") + hits = cache.check("Who wrote Hamlet?") + assert hits and hits[0]["response"] == "Shakespeare" diff --git a/tests/integration/test_message_history.py b/tests/integration/test_message_history.py index 1c3276bc..bbebe00e 100644 --- a/tests/integration/test_message_history.py +++ b/tests/integration/test_message_history.py @@ -2,7 +2,7 @@ from contextlib import suppress import pytest -from redis.exceptions import ConnectionError +from redis.exceptions import ConnectionError, NoPermissionError from redisvl.extensions.constants import ID_FIELD_NAME from redisvl.extensions.message_history import MessageHistory, SemanticMessageHistory @@ -826,3 +826,55 @@ def test_deprecated_dtype_argument(client, redis_url, redis_test_name): history.clear() with suppress(Exception): history.delete() + + +def test_create_index_false_clear_works_under_a_read_write_acl( + app_name, client, redis_url, acl_user +): + """The `FT.INFO` removal from `SearchIndex.clear()`, end to end. + + `MessageHistory.clear()` delegates to `SearchIndex.clear()`, which used to + read `FT.INFO` -- `@search` only, and so denied to the credential this flag + serves. This extension needs no vectorizer, making it the cheap place to + prove the fix against a real restricted credential rather than a mock. + """ + skip_if_no_redis_search(client) + name = app_name + + owner = MessageHistory(name=name, redis_url=redis_url) + try: + owner.add_messages( + [ + {"role": "user", "content": "hello"}, + {"role": "llm", "content": "hi there"}, + ] + ) + assert len(owner.get_recent(top_k=10)) == 2 + + with acl_user( + "~*", "&*", "+@read", "+@write", "-@dangerous", name="acl_history_user" + ) as user: + restricted = MessageHistory( + name=name, + redis_url=redis_url, + connection_kwargs={ + "username": user.username, + "password": user.password, + }, + create_index=False, + ) + + # Pin the premise: this credential cannot read FT.INFO. + with pytest.raises(NoPermissionError): + user.connect().execute_command("FT.INFO", name) + + restricted.clear() + + # Read back through the owner: the restricted instance is gone. + assert owner.get_recent(top_k=10) == [] + # And clearing left the index standing, so the history is still usable. + assert owner._index.exists() + owner.add_messages([{"role": "user", "content": "again"}]) + assert len(owner.get_recent(top_k=10)) == 1 + finally: + owner.delete() diff --git a/tests/unit/test_error_handling.py b/tests/unit/test_error_handling.py index 4dbc47d7..e55683e7 100644 --- a/tests/unit/test_error_handling.py +++ b/tests/unit/test_error_handling.py @@ -452,12 +452,19 @@ def test_clear_individual_key_deletion_errors(self, mock_validate): 1, # Third succeeds ] - # Mock the .info() and ._query() methods to return test data + # info() is stubbed to raise, not to return: clear() must never call + # it, because FT.INFO is @search only and would deny the whole method + # to a +@read +@write credential. with ( - patch.object(SearchIndex, "info") as mock_info, + patch.object( + SearchIndex, + "info", + side_effect=AssertionError("clear() must not call info()"), + ), + patch.object(SearchIndex, "query") as mock_count, patch.object(SearchIndex, "_query") as mock_query, ): - mock_info.return_value = {"num_docs": 3} + mock_count.return_value = 3 mock_query.side_effect = [ [{"id": "test:key1"}, {"id": "test:key2"}, {"id": "test:key3"}], [], @@ -480,6 +487,60 @@ def test_clear_individual_key_deletion_errors(self, mock_validate): # Should return count of successfully deleted keys (2 out of 3) assert result == 2 + def test_clear_terminates_when_no_key_can_be_deleted(self): + """clear() must not spin when every delete in a page fails. + + Reachable through `_delete_batch`'s cluster branch, which swallows + per-key `RedisError` and returns 0 while the page stays non-empty. + Asserted hermetically because cluster never runs in CI, so this covers + control flow only -- not CROSSSLOT behaviour or node targeting. + """ + from redisvl.index import SearchIndex + from redisvl.schema import IndexSchema + + schema = Mock(spec=IndexSchema) + schema.index = Mock() + schema.index.name = "stalled" + schema.index.prefix = "test" + schema.index.key_separator = ":" + schema.index.storage_type = StorageType.HASH + + mock_cluster_client = Mock(spec=RedisCluster) + mock_cluster_client.delete.side_effect = redis.exceptions.NoPermissionError( + "this user has no permissions to run the 'del' command" + ) + + page = [{"id": "test:key1"}, {"id": "test:key2"}] + # pytest-timeout is not installed, so without this cap a regression + # would hang the suite rather than fail it. + max_calls = 200 + calls = {"n": 0} + + def always_a_full_page(*args, **kwargs): + calls["n"] += 1 + if calls["n"] > max_calls: + raise AssertionError( + f"clear() did not terminate within {max_calls} queries" + ) + return page + + with ( + patch.object(SearchIndex, "query", return_value=2), + patch.object(SearchIndex, "_query", side_effect=always_a_full_page), + ): + index = SearchIndex(schema) + index._SearchIndex__redis_client = mock_cluster_client + + with patch("redisvl.index.index.logger") as mock_logger: + result = index.clear() + + assert result == 0 + # It gave up by paging past the backstop rather than by deleting. + assert any( + "paged past its runaway backstop" in str(call) + for call in mock_logger.warning.call_args_list + ) + @patch("redisvl.redis.connection.RedisConnectionFactory.validate_async_redis") @pytest.mark.asyncio async def test_async_clear_individual_key_deletion_errors(self, mock_validate): @@ -505,10 +566,11 @@ async def test_async_clear_individual_key_deletion_errors(self, mock_validate): ] ) - # Mock the .info() and ._query() methods to return test data + # See the sync twin: info() must never be reached from clear(). async def mock_info(*args, **kwargs): - return {"num_docs": 3} + raise AssertionError("clear() must not call info()") + mock_count = AsyncMock(return_value=3) mock_query = AsyncMock( side_effect=[ [{"id": "test:key1"}, {"id": "test:key2"}, {"id": "test:key3"}], @@ -518,6 +580,7 @@ async def mock_info(*args, **kwargs): with ( patch.object(AsyncSearchIndex, "info", mock_info), + patch.object(AsyncSearchIndex, "query", mock_count), patch.object(AsyncSearchIndex, "_query", mock_query), ): # Create index with mocked client diff --git a/tests/unit/test_extension_create_index_flag.py b/tests/unit/test_extension_create_index_flag.py index c77b1838..65f58df1 100644 --- a/tests/unit/test_extension_create_index_flag.py +++ b/tests/unit/test_extension_create_index_flag.py @@ -12,6 +12,7 @@ gate every `FT.*` command passes through. """ +import re from unittest.mock import MagicMock, Mock import pytest @@ -20,9 +21,11 @@ from redisvl.exceptions import RedisSearchError from redisvl.extensions.cache.llm import SemanticCache +from redisvl.extensions.constants import EXTERNAL_INDEX_DROP_CONFLICT from redisvl.extensions.message_history import MessageHistory, SemanticMessageHistory from redisvl.extensions.router import SemanticRouter from redisvl.extensions.router.schema import Route +from redisvl.index import SearchIndex from redisvl.redis.connection import RedisConnectionFactory from redisvl.utils.vectorize import CustomVectorizer @@ -144,9 +147,14 @@ def test_overwrite_with_create_index_false_is_rejected(self, kind, vectorizer): class TestExternalIndexLifecycle: + """The flag guards the index's lifecycle, not its contents. + + `delete()` drops the index, so an attach-only instance must refuse it. + `clear()` removes entries and leaves the index standing, so it does not. + """ + @pytest.mark.parametrize("kind", ALL_KINDS) - @pytest.mark.parametrize("method", ["clear", "delete"]) - def test_index_wide_mutation_is_rejected(self, kind, method, vectorizer): + def test_dropping_the_index_is_rejected(self, kind, vectorizer): client = _client() extension = _build( kind, @@ -156,16 +164,13 @@ def test_index_wide_mutation_is_rejected(self, kind, method, vectorizer): name="production_alias", ) - with pytest.raises(ValueError, match="does not manage.*lifecycle"): - getattr(extension, method)() + with pytest.raises(ValueError, match=re.escape(EXTERNAL_INDEX_DROP_CONFLICT)): + extension.delete() assert client.mock_calls == [] @pytest.mark.asyncio - @pytest.mark.parametrize("method", ["aclear", "adelete"]) - async def test_async_cache_index_wide_mutation_is_rejected( - self, method, vectorizer - ): + async def test_async_cache_dropping_the_index_is_rejected(self, vectorizer): client = _client() cache = SemanticCache( name="production_alias", @@ -174,11 +179,47 @@ async def test_async_cache_index_wide_mutation_is_rejected( create_index=False, ) - with pytest.raises(ValueError, match="does not manage.*lifecycle"): - await getattr(cache, method)() + with pytest.raises(ValueError, match=re.escape(EXTERNAL_INDEX_DROP_CONFLICT)): + await cache.adelete() assert client.mock_calls == [] + def test_cache_clear_issues_no_index_command(self, vectorizer): + # Asserted as "ft() was never reached" rather than on the SCAN call + # shape, which belongs to BaseCache and is being reworked separately. + client = _client() + client.scan.return_value = (0, ["llmcache:abc"]) + client.scan_iter.return_value = iter(["llmcache:abc"]) + cache = SemanticCache( + name="llmcache", + vectorizer=vectorizer, + redis_client=client, + create_index=False, + ) + + cache.clear() + + client.ft.assert_not_called() + client.delete.assert_called_once_with("llmcache:abc") + + @pytest.mark.parametrize("kind", ["history", "semantic_history", "router"]) + def test_index_backed_clear_is_not_refused(self, kind, vectorizer, monkeypatch): + # These delegate to SearchIndex.clear(), so the minimal assertion is + # that control reaches it at all. + cleared = Mock(return_value=0) + monkeypatch.setattr(SearchIndex, "clear", cleared) + extension = _build( + kind, + _client(), + vectorizer, + create_index=False, + name="production_alias", + ) + + extension.clear() + + cleared.assert_called_once() + class TestRouterWithoutRoutes: def test_empty_routes_raises_a_useful_error_when_matching(self, vectorizer):