From 00d589ca161022cceeb02c42ca3e93e11a21b270 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 28 Aug 2026 09:24:45 +0200 Subject: [PATCH 1/4] fix(index): bound clear() with a CountQuery instead of FT.INFO SearchIndex.clear() read FT.INFO's num_docs to size a runaway backstop for its delete loop. FT.INFO carries only the @search ACL category, so that one call denied the whole method to a `+@read +@write` credential -- even though the FT.SEARCH and DEL the sweep is actually made of are both granted. Verified on Redis 8.4.5: FT.INFO is denied to `+@read +@write -@dangerous`, FT.SEARCH is not. The backstop now comes from a CountQuery, which is what drop_by_filter has used all along and is also FT.SEARCH. Measured under that credential: clear() removed 1500 of 1500 documents and the index survived. A page whose keys cannot all be deleted now advances the paging offset instead of re-reading the same head, so a blockage no longer hides the documents behind it. Measured with 600 of 2000 keys undeletable: the sweep removes exactly the other 1400 and returns. Termination is also verified against a writer inserting 1.04M keys mid-sweep, against every delete failing, and against a second concurrent clear(). Two stubs in test_error_handling.py patched SearchIndex.info for a call clear() no longer makes, so they passed while asserting nothing. They now raise, which makes re-introducing FT.INFO into either twin fail loudly. The new test drives the cluster branch of _delete_batch, the only path that reaches the backstop, and carries its own query cap because pytest-timeout is not installed and a regression would otherwise hang the suite rather than fail it. --- redisvl/index/index.py | 158 ++++++++++++++++++++++++++---- tests/unit/test_error_handling.py | 82 +++++++++++++++- 2 files changed, 215 insertions(+), 25 deletions(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 3daa69d3e..2872b9e4f 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -1151,28 +1151,102 @@ 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. + Normal termination is an empty page. A ``CountQuery`` taken up front + sizes a runaway backstop, so a writer inserting as fast as this deletes + cannot keep the sweep running forever. The count used to come from + ``FT.INFO``'s ``num_docs``, which is ``@search`` only and so denied this + entire method to a ``+@read +@write`` credential; ``CountQuery`` is + ``FT.SEARCH``, which the query loop below already requires. + + Note: + This enumerates *through the index*, so it can only delete what the + index currently returns. On an index still running its background + scan the sweep drains the documents indexed so far, sees an empty + page while unindexed keys remain, and stops -- reporting what it + deleted as if it were done. Measured on Redis 8.4.6 against 20 000 + hashes indexed immediately after loading, two runs cleared 125 and + 57; against a fully indexed copy of the same data, both cleared all + 20 000. Wait for ``FT.INFO``'s ``indexing`` to reach ``0`` if you + need the sweep to be complete, or clear by key prefix instead. This + is long-standing behaviour, not a consequence of the bound below: + the ``num_docs`` bound this replaced cleared 57 on the same setup. + + Note: + A page whose keys cannot be deleted at all -- most plausibly a + permission denial swallowed by :meth:`_delete_batch`'s cluster + branch -- does not abort the sweep: the offset advances past it so + the documents behind it are still reached. If nothing can be + deleted, the backstop ends the sweep and a warning is logged. + + Note: + Keys under the index's prefix that failed to index are never + returned by the query, so they survive. + + Warning: + The return value counts deletions, so ``0`` does not distinguish an + already-empty index from a sweep that could delete nothing. Callers + that must know check the log, or re-run: the operation is + idempotent. + 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 -- 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 = 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 the blockage instead of re-reading the same head + # forever; the documents behind it 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 +2559,72 @@ 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 full semantics, which are + identical here: why the backstop comes from a ``CountQuery`` rather than + ``FT.INFO``, that a sweep racing a background index scan can stop while + unindexed keys remain, that a page which cannot be deleted is paged past + rather than aborting, and that a ``0`` return does not distinguish an + empty index from a sweep that deleted nothing. + 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 the blockage instead of re-reading the same head + # forever; the documents behind it 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/unit/test_error_handling.py b/tests/unit/test_error_handling.py index 4dbc47d7c..df9626f98 100644 --- a/tests/unit/test_error_handling.py +++ b/tests/unit/test_error_handling.py @@ -452,12 +452,21 @@ def test_clear_individual_key_deletion_errors(self, mock_validate): 1, # Third succeeds ] - # Mock the .info() and ._query() methods to return test data + # clear() sizes its runaway backstop with a CountQuery, and must not + # read FT.INFO: that command is @search only, so a single call for a + # loop bound would deny the whole method to a +@read +@write + # credential. info() is stubbed to raise so re-introducing it fails + # here rather than silently in production. 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 +489,67 @@ 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 the cluster branch of `_delete_batch`, which catches + per-key `RedisError` -- and `NoPermissionError` is one -- logs it, and + returns 0 while the page stays non-empty. `clear()` has no `FT.INFO` + bound any more, so the offset advance plus the runaway backstop are the + only things standing between that and an infinite loop. Cluster is never + exercised in CI, so this is asserted hermetically. + + This proves control flow given `_delete_batch`'s documented cluster + return contract. It proves nothing about CROSSSLOT behaviour, node + targeting, or whether FT.SEARCH enumerates every shard. + """ + 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"}] + # A runaway guard for the test itself: pytest-timeout is not installed, + # so a regression that removes the bound would hang the suite instead of + # failing 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 +575,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 +589,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 From cbf9042620c4ed70b8d70b9884863e7b09f2d7fe Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 28 Aug 2026 09:25:17 +0200 Subject: [PATCH 2/4] fix(extensions): allow clear() when the index is externally managed create_index=False refused both delete() and clear(), justified as protecting an externally managed index from being destroyed through an attach-only instance. That holds for delete(), which calls _index.delete(drop=True). It does not hold for clear(), which removes entries and leaves the index standing, so a caller attached to a platform-provisioned index had no public way to invalidate it. The guard was not a boundary either. drop(keys=...) is public and unguarded under the same flag, so the whole cache was always reachable through public API by enumerating keys first -- which is what downstream callers did, taking RedisVL's key-layout knowledge with them. clear() and aclear() are now unguarded on all four extensions. delete() and adelete() still raise. EXTERNAL_INDEX_LIFECYCLE_CONFLICT becomes EXTERNAL_INDEX_DROP_CONFLICT, since it now guards exactly one operation. No alias is kept: the message text changed too, so the old name preserved nothing for anyone matching on it. Neither clear() shape verifies the prefix against the live index under this flag, and they fail silently in opposite directions -- prefix-based clearing reaches keys the index never covered, index-based clearing reaches documents this instance never wrote. Both are documented on the methods. remove_route() gains the stored-config caveat add_route() already carried, because clear()'s new docstring points readers at it. Assertions compare against the constant rather than a loose regex on its message, which is what let the rename above pass silently at first. The new MessageHistory integration test proves the FT.INFO removal end to end against a real restricted credential in about two seconds and without a vectorizer; the pre-existing ACL coverage exercises SemanticCache, whose prefix-based clear() never needed it. --- redisvl/extensions/cache/llm/semantic.py | 60 +++++++++++++--- redisvl/extensions/constants.py | 12 ++-- .../message_history/message_history.py | 30 ++++++-- .../message_history/semantic_history.py | 30 ++++++-- redisvl/extensions/router/semantic.py | 44 ++++++++++-- tests/integration/test_llmcache.py | 64 +++++++++++++++++ tests/integration/test_message_history.py | 61 +++++++++++++++- .../unit/test_extension_create_index_flag.py | 69 ++++++++++++++++--- 8 files changed, 325 insertions(+), 45 deletions(-) diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index 87383057f..7246788e5 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,66 @@ 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 this instance was constructed with + ``create_index=False``. Dropping an index RedisVL did not + create is not this instance's to do; 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 this instance was constructed with + ``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. + + Not blocked by ``create_index=False``: this walks the keyspace with + ``SCAN`` and ``DEL`` over the prefix this cache declares, issuing no + index command at all, so a ``+@read +@write`` credential can run it. + Dropping the index itself is :meth:`delete`, which stays guarded. + + Warning: + The prefix, not the index, is the unit of clearing, and it is the + prefix this instance *declares* -- always ``{name}:``. With + ``create_index=False`` nothing verifies that against the live + index, so both directions can go wrong silently: + + - **Too little.** If the live index covers a different prefix, or + is an alias onto one, this 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 it was called to + invalidate. + - **Too much.** ``SCAN``/``DEL`` is blind to both the index and the + key type, so it removes *every* key under ``{name}:`` -- another + writer's cache entries, and any unrelated application data that + happens to live under the same namespace root. + + Neither is detectable from an attach-only instance, because + diagnosing it needs the ``FT.INFO`` such a credential is denied. + """ 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. + + Not blocked by ``create_index=False``; see :meth:`clear` for the + permissions this needs and for the prefix 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 c31bb7ef6..2271be35b 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 03204389a..2037d749e 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,33 @@ 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. + + Not blocked by ``create_index=False``: that flag guards index lifecycle + operations -- :meth:`delete` -- not entry removal. + + Note: + This enumerates entries through the index (``FT.SEARCH``, which a + ``+@read +@write`` credential is granted) rather than by keyspace + prefix. So it deletes exactly the documents the *live* index + covers, which under ``create_index=False`` is unverified: against + an index whose prefix differs from this one's, it removes + documents this instance never wrote and leaves this instance's own + entries in place. + """ 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 this instance was constructed with + ``create_index=False``. Dropping an index RedisVL did not + create is not this instance's to do; 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 22ae5daa3..1197b72f8 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,33 @@ 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. + + Not blocked by ``create_index=False``: that flag guards index lifecycle + operations -- :meth:`delete` -- not entry removal. + + Note: + This enumerates entries through the index (``FT.SEARCH``, which a + ``+@read +@write`` credential is granted) rather than by keyspace + prefix. So it deletes exactly the documents the *live* index + covers, which under ``create_index=False`` is unverified: against + an index whose prefix differs from this one's, it removes + documents this instance never wrote and leaves this instance's own + entries in place. + """ 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 this instance was constructed with + ``create_index=False``. Dropping an index RedisVL did not + create is not this instance's to do; 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 16472615b..ecde4ae7d 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,13 @@ 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. + Note that, like :meth:`add_route`, this replaces the router's stored + config with this instance's route list -- including under + ``create_index=False``, where that list was never reconciled against + Redis. Removing one route from a router this instance holds only a + subset of will drop the rest from the config :meth:`from_existing` + reads. + Args: route_name (str): Name of the route to remove. """ @@ -671,18 +678,43 @@ 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 this instance was constructed with + ``create_index=False``. Dropping an index RedisVL did not + create is not this instance's to do; 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. + + Not blocked by ``create_index=False``: that flag guards index lifecycle + operations -- :meth:`delete` -- not entry removal. + + Note: + This enumerates entries through the index (``FT.SEARCH``, which a + ``+@read +@write`` credential is granted) rather than by keyspace + prefix, so under ``create_index=False`` it deletes whatever the + unverified live index covers. + + Warning: + The stored ``route_config`` is left as it was, on this path and the + default one alike -- only :meth:`delete` removes it. So after + clearing, a separate process calling + :meth:`SemanticRouter.from_existing` rebuilds its route list from + that blob and reports routes whose reference vectors are gone, + matching nothing. :meth:`remove_route` keeps the config in step, but + rewrites it from this instance's route list -- see its own note + before using it on a router you attached to. + """ self._index.clear() self.routes = [] diff --git a/tests/integration/test_llmcache.py b/tests/integration/test_llmcache.py index 8bd4c69a0..24a9281db 100644 --- a/tests/integration/test_llmcache.py +++ b/tests/integration/test_llmcache.py @@ -1212,3 +1212,67 @@ 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 +): + """Invalidating the cache is not an index lifecycle operation. + + `clear()` is a `SCAN`/`DEL` walk of the cache's own key prefix, so the + `@read`/`@write` credential that `create_index=False` exists to serve can + run it -- and it must, because that credential cannot reprovision the index + and so has no other way to invalidate a stale cache. `delete()` drops the + index, so it stays refused. + + Only a live server shows the parts that matter: that a real restricted + credential is admitted, that the index survives, and that the async path + carries the same credential as the sync one. Both are 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 from the same credentials, + # so it needs its own exercise rather than trusting the sync one. + 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: + # This cache built its own clients from the credentials, so the + # fixture does not track them -- and the user is about to go away. + await restricted_cache.adisconnect() + restricted_cache.disconnect() + + # The index itself is untouched: exists() proves the definition survived, + # which an FT._LIST membership check would not. + assert cache.index.exists() + # And it still works, which exists() alone would not prove. + 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 1c3276bc5..f23a52a17 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,62 @@ 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()` goes through `SearchIndex.clear()`, which used to + read `FT.INFO` to size its loop bound. `FT.INFO` is `@search` only, so that + single call denied the whole method to the `+@read +@write` credential + `create_index=False` exists to serve -- even though the `FT.SEARCH` and + `DEL` the sweep is actually made of are both granted. + + This is the extension the fix was for, and it needs no vectorizer, so it is + the cheap place to prove it 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 the index metadata + # that clear() used to depend on. + with pytest.raises(NoPermissionError): + user.connect().execute_command("FT.INFO", name) + + restricted.clear() + + # Read back through the owner, so the assertion does not depend on the + # restricted instance outliving its ACL user. + 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_extension_create_index_flag.py b/tests/unit/test_extension_create_index_flag.py index c77b18384..446a3c6cc 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,17 @@ 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 in place, so it stays + available: guarding it too left a caller attached to a platform-provisioned + index with no public way to invalidate the cache, which pushed downstream + code into reimplementing the keyspace walk against private APIs. + """ + @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 +167,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 +182,52 @@ 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): + # The reason this one is safe to allow: the cache clears by keyspace + # prefix, so it never names the index. Asserted as "ft() was never + # reached" rather than on the SCAN call shape, which belongs to + # BaseCache and is being reworked for cluster correctness. + 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. MessageHistory's body is currently + # byte-identical to SemanticMessageHistory's, but parametrizing it + # anyway is what keeps that from being load-bearing: re-adding a guard + # to either one has to fail a test. + 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): From 3853b22dd4c16b4b9e5aa745dbdf651b3734dc01 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 28 Aug 2026 09:25:34 +0200 Subject: [PATCH 3/4] docs(installation): describe entry removal under create_index=False The ACL guide said an attach-only extension refuses index-wide delete() and clear() alike. clear() is now permitted, and the operation table listed index.clear() under FT.INFO, which it no longer calls. The replacement section leads with what the flag permits rather than what it refuses, and states the failure mode each clear() shape has: prefix-based clearing reaches keys the index never covered and misses served entries when the live prefix differs, while index-based clearing reaches documents this instance never wrote. Diagnosing either needs the FT.INFO such a credential lacks, so the guidance is to get the index's prefixes from whoever provisions it rather than infer them from a successful query. --- docs/user_guide/installation.md | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index ce70fe8a5..c7f20400a 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 From 0858b26a83c923e68d10f5bfd6ddd375f126dc09 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 28 Aug 2026 10:47:31 +0200 Subject: [PATCH 4/4] docs: cut the docstring and comment volume added by this branch The three commits before this added roughly two and a half lines of prose per line of code, which is bloat that has to be maintained. This keeps the split deliberate: a docstring carries only what a caller needs to call the method correctly, the reasoning lives in the commit message, and the detailed hazard discussion lives once in the ACL guide with the docstrings pointing at it. Cut, specifically: the rationale for choosing CountQuery over FT.INFO, which belongs in e1f2f06's message and not on every reader's screen; measured trial counts, which pin a Redis patch version and a race outcome and will age; the 26-line warning on SemanticCache.clear() that restated the guide; five near-identical Raises blocks; and the duplicated hazard note across the two message-history classes. No executable code changes. Verified by comparing the AST of every touched module against its parent with docstrings stripped: all nine identical. --- redisvl/extensions/cache/llm/semantic.py | 38 +++-------- .../message_history/message_history.py | 23 +++---- .../message_history/semantic_history.py | 23 +++---- redisvl/extensions/router/semantic.py | 34 +++------- redisvl/index/index.py | 65 ++++--------------- tests/integration/test_llmcache.py | 29 +++------ tests/integration/test_message_history.py | 19 ++---- tests/unit/test_error_handling.py | 27 +++----- .../unit/test_extension_create_index_flag.py | 16 ++--- 9 files changed, 80 insertions(+), 194 deletions(-) diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index 7246788e5..439423cc2 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -312,9 +312,7 @@ def delete(self) -> None: """Delete the cache and its index entirely. Raises: - ValueError: If this instance was constructed with - ``create_index=False``. Dropping an index RedisVL did not - create is not this instance's to do; use :meth:`clear` to + ValueError: If ``create_index=False``. Use :meth:`clear` to empty the cache and leave the index standing. """ if not self._create_index: @@ -325,8 +323,7 @@ async def adelete(self) -> None: """Async delete the cache and its index entirely. Raises: - ValueError: If this instance was constructed with - ``create_index=False``. See :meth:`delete`. + ValueError: If ``create_index=False``. See :meth:`delete`. """ if not self._create_index: raise ValueError(EXTERNAL_INDEX_DROP_CONFLICT) @@ -336,38 +333,21 @@ async def adelete(self) -> None: def clear(self) -> None: """Delete every cache entry, leaving the index in place. - Not blocked by ``create_index=False``: this walks the keyspace with - ``SCAN`` and ``DEL`` over the prefix this cache declares, issuing no - index command at all, so a ``+@read +@write`` credential can run it. - Dropping the index itself is :meth:`delete`, which stays guarded. + 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: - The prefix, not the index, is the unit of clearing, and it is the - prefix this instance *declares* -- always ``{name}:``. With - ``create_index=False`` nothing verifies that against the live - index, so both directions can go wrong silently: - - - **Too little.** If the live index covers a different prefix, or - is an alias onto one, this 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 it was called to - invalidate. - - **Too much.** ``SCAN``/``DEL`` is blind to both the index and the - key type, so it removes *every* key under ``{name}:`` -- another - writer's cache entries, and any unrelated application data that - happens to live under the same namespace root. - - Neither is detectable from an attach-only instance, because - diagnosing it needs the ``FT.INFO`` such a credential is denied. + 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 delete every cache entry, leaving the index in place. - Not blocked by ``create_index=False``; see :meth:`clear` for the - permissions this needs and for the prefix caveats, which apply - identically here. + See :meth:`clear` for the caveats, which apply identically here. """ await super().aclear() diff --git a/redisvl/extensions/message_history/message_history.py b/redisvl/extensions/message_history/message_history.py index 2037d749e..ca8f5e074 100644 --- a/redisvl/extensions/message_history/message_history.py +++ b/redisvl/extensions/message_history/message_history.py @@ -97,17 +97,14 @@ def __repr__(self) -> str: def clear(self) -> None: """Delete every message, leaving the index in place. - Not blocked by ``create_index=False``: that flag guards index lifecycle - operations -- :meth:`delete` -- not entry removal. - - Note: - This enumerates entries through the index (``FT.SEARCH``, which a - ``+@read +@write`` credential is granted) rather than by keyspace - prefix. So it deletes exactly the documents the *live* index - covers, which under ``create_index=False`` is unverified: against - an index whose prefix differs from this one's, it removes - documents this instance never wrote and leaves this instance's own - entries 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() @@ -115,9 +112,7 @@ def delete(self) -> None: """Remove every message and drop the search index. Raises: - ValueError: If this instance was constructed with - ``create_index=False``. Dropping an index RedisVL did not - create is not this instance's to do; use :meth:`clear` to + ValueError: If ``create_index=False``. Use :meth:`clear` to remove the messages and leave the index standing. """ if not self._create_index: diff --git a/redisvl/extensions/message_history/semantic_history.py b/redisvl/extensions/message_history/semantic_history.py index 1197b72f8..e8ba8417d 100644 --- a/redisvl/extensions/message_history/semantic_history.py +++ b/redisvl/extensions/message_history/semantic_history.py @@ -158,17 +158,14 @@ def __repr__(self) -> str: def clear(self) -> None: """Delete every message, leaving the index in place. - Not blocked by ``create_index=False``: that flag guards index lifecycle - operations -- :meth:`delete` -- not entry removal. - - Note: - This enumerates entries through the index (``FT.SEARCH``, which a - ``+@read +@write`` credential is granted) rather than by keyspace - prefix. So it deletes exactly the documents the *live* index - covers, which under ``create_index=False`` is unverified: against - an index whose prefix differs from this one's, it removes - documents this instance never wrote and leaves this instance's own - entries 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() @@ -176,9 +173,7 @@ def delete(self) -> None: """Remove every message and drop the search index. Raises: - ValueError: If this instance was constructed with - ``create_index=False``. Dropping an index RedisVL did not - create is not this instance's to do; use :meth:`clear` to + ValueError: If ``create_index=False``. Use :meth:`clear` to remove the messages and leave the index standing. """ if not self._create_index: diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index ecde4ae7d..5958c1bf2 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -654,11 +654,9 @@ 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. - Note that, like :meth:`add_route`, this replaces the router's stored - config with this instance's route list -- including under - ``create_index=False``, where that list was never reconciled against - Redis. Removing one route from a router this instance holds only a - subset of will drop the rest from the config :meth:`from_existing` + 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: @@ -681,9 +679,7 @@ def delete(self) -> None: """Delete the semantic router index and its persisted route config. Raises: - ValueError: If this instance was constructed with - ``create_index=False``. Dropping an index RedisVL did not - create is not this instance's to do; use :meth:`clear` to + ValueError: If ``create_index=False``. Use :meth:`clear` to remove the route references and leave the index standing. """ if not self._create_index: @@ -696,24 +692,14 @@ def delete(self) -> None: def clear(self) -> None: """Delete every route reference, leaving the index in place. - Not blocked by ``create_index=False``: that flag guards index lifecycle - operations -- :meth:`delete` -- not entry removal. - - Note: - This enumerates entries through the index (``FT.SEARCH``, which a - ``+@read +@write`` credential is granted) rather than by keyspace - prefix, so under ``create_index=False`` it deletes whatever the - unverified live index covers. + 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, on this path and the - default one alike -- only :meth:`delete` removes it. So after - clearing, a separate process calling - :meth:`SemanticRouter.from_existing` rebuilds its route list from - that blob and reports routes whose reference vectors are gone, - matching nothing. :meth:`remove_route` keeps the config in step, but - rewrites it from this instance's route list -- see its own note - before using it on a router you attached to. + 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 2872b9e4f..e70175914 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -1151,42 +1151,12 @@ 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. - Normal termination is an empty page. A ``CountQuery`` taken up front - sizes a runaway backstop, so a writer inserting as fast as this deletes - cannot keep the sweep running forever. The count used to come from - ``FT.INFO``'s ``num_docs``, which is ``@search`` only and so denied this - entire method to a ``+@read +@write`` credential; ``CountQuery`` is - ``FT.SEARCH``, which the query loop below already requires. - - Note: - This enumerates *through the index*, so it can only delete what the - index currently returns. On an index still running its background - scan the sweep drains the documents indexed so far, sees an empty - page while unindexed keys remain, and stops -- reporting what it - deleted as if it were done. Measured on Redis 8.4.6 against 20 000 - hashes indexed immediately after loading, two runs cleared 125 and - 57; against a fully indexed copy of the same data, both cleared all - 20 000. Wait for ``FT.INFO``'s ``indexing`` to reach ``0`` if you - need the sweep to be complete, or clear by key prefix instead. This - is long-standing behaviour, not a consequence of the bound below: - the ``num_docs`` bound this replaced cleared 57 on the same setup. - - Note: - A page whose keys cannot be deleted at all -- most plausibly a - permission denial swallowed by :meth:`_delete_batch`'s cluster - branch -- does not abort the sweep: the offset advances past it so - the documents behind it are still reached. If nothing can be - deleted, the backstop ends the sweep and a warning is logged. - Note: - Keys under the index's prefix that failed to index are never - returned by the query, so they survive. - - Warning: - The return value counts deletions, so ``0`` does not distinguish an - already-empty index from a sweep that could delete nothing. Callers - that must know check the log, or re-run: the operation is - idempotent. + 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. @@ -1195,10 +1165,9 @@ def clear(self) -> int: matched = cast(int, 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. + # 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 @@ -1231,10 +1200,9 @@ def clear(self) -> int: # survivors is at offset 0 again. offset = 0 else: - # Nothing in this page could be deleted -- most plausibly a + # Nothing in this page could be deleted, most plausibly a # permission denial swallowed by _delete_batch's cluster branch. - # Page past the blockage instead of re-reading the same head - # forever; the documents behind it may still be deletable. + # Page past it: the documents behind may still be deletable. offset += batch_size if offset > max_records: logger.warning( @@ -2559,12 +2527,8 @@ 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 full semantics, which are - identical here: why the backstop comes from a ``CountQuery`` rather than - ``FT.INFO``, that a sweep racing a background index scan can stop while - unindexed keys remain, that a page which cannot be deleted is paged past - rather than aborting, and that a ``0`` return does not distinguish an - empty index from a sweep that deleted nothing. + See :meth:`SearchIndex.clear` for the sweep's caveats, which apply + identically here. Returns: int: Count of records deleted from Redis. @@ -2609,10 +2573,9 @@ async def clear(self) -> int: # survivors is at offset 0 again. offset = 0 else: - # Nothing in this page could be deleted -- most plausibly a + # Nothing in this page could be deleted, most plausibly a # permission denial swallowed by _delete_batch's cluster branch. - # Page past the blockage instead of re-reading the same head - # forever; the documents behind it may still be deletable. + # Page past it: the documents behind may still be deletable. offset += batch_size if offset > max_records: logger.warning( diff --git a/tests/integration/test_llmcache.py b/tests/integration/test_llmcache.py index 24a9281db..4f5c59184 100644 --- a/tests/integration/test_llmcache.py +++ b/tests/integration/test_llmcache.py @@ -1218,18 +1218,12 @@ def test_create_index_false_works_under_a_read_write_acl( async def test_create_index_false_can_invalidate_but_not_drop( cache, vectorizer, redis_url, acl_user ): - """Invalidating the cache is not an index lifecycle operation. - - `clear()` is a `SCAN`/`DEL` walk of the cache's own key prefix, so the - `@read`/`@write` credential that `create_index=False` exists to serve can - run it -- and it must, because that credential cannot reprovision the index - and so has no other way to invalidate a stale cache. `delete()` drops the - index, so it stays refused. - - Only a live server shows the parts that matter: that a real restricted - credential is admitted, that the index survives, and that the async path - carries the same credential as the sync one. Both are folded into one test - because the ACL user and the pre-created index are the expensive fixtures. + """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") @@ -1252,8 +1246,7 @@ async def test_create_index_false_can_invalidate_but_not_drop( # not depend on the restricted one still working. assert cache.check("What is the capital of France?") == [] - # The async path builds its own client from the same credentials, - # so it needs its own exercise rather than trusting the sync one. + # 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?") == [] @@ -1264,15 +1257,13 @@ async def test_create_index_false_can_invalidate_but_not_drop( with pytest.raises(ValueError, match="does not manage.*lifecycle"): await restricted_cache.adelete() finally: - # This cache built its own clients from the credentials, so the - # fixture does not track them -- and the user is about to go away. + # Not fixture-tracked, and the ACL user is about to go away. await restricted_cache.adisconnect() restricted_cache.disconnect() - # The index itself is untouched: exists() proves the definition survived, - # which an FT._LIST membership check would not. + # exists() proves the definition survived; the round-trip below proves it + # is still usable, which exists() alone would not. assert cache.index.exists() - # And it still works, which exists() alone would not prove. 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 f23a52a17..bbebe00e5 100644 --- a/tests/integration/test_message_history.py +++ b/tests/integration/test_message_history.py @@ -833,15 +833,10 @@ def test_create_index_false_clear_works_under_a_read_write_acl( ): """The `FT.INFO` removal from `SearchIndex.clear()`, end to end. - `MessageHistory.clear()` goes through `SearchIndex.clear()`, which used to - read `FT.INFO` to size its loop bound. `FT.INFO` is `@search` only, so that - single call denied the whole method to the `+@read +@write` credential - `create_index=False` exists to serve -- even though the `FT.SEARCH` and - `DEL` the sweep is actually made of are both granted. - - This is the extension the fix was for, and it needs no vectorizer, so it is - the cheap place to prove it against a real restricted credential rather than - a mock. + `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 @@ -869,15 +864,13 @@ def test_create_index_false_clear_works_under_a_read_write_acl( create_index=False, ) - # Pin the premise: this credential cannot read the index metadata - # that clear() used to depend on. + # 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, so the assertion does not depend on the - # restricted instance outliving its ACL user. + # 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() diff --git a/tests/unit/test_error_handling.py b/tests/unit/test_error_handling.py index df9626f98..e55683e79 100644 --- a/tests/unit/test_error_handling.py +++ b/tests/unit/test_error_handling.py @@ -452,11 +452,9 @@ def test_clear_individual_key_deletion_errors(self, mock_validate): 1, # Third succeeds ] - # clear() sizes its runaway backstop with a CountQuery, and must not - # read FT.INFO: that command is @search only, so a single call for a - # loop bound would deny the whole method to a +@read +@write - # credential. info() is stubbed to raise so re-introducing it fails - # here rather than silently in production. + # 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, @@ -492,16 +490,10 @@ def test_clear_individual_key_deletion_errors(self, mock_validate): def test_clear_terminates_when_no_key_can_be_deleted(self): """clear() must not spin when every delete in a page fails. - Reachable through the cluster branch of `_delete_batch`, which catches - per-key `RedisError` -- and `NoPermissionError` is one -- logs it, and - returns 0 while the page stays non-empty. `clear()` has no `FT.INFO` - bound any more, so the offset advance plus the runaway backstop are the - only things standing between that and an infinite loop. Cluster is never - exercised in CI, so this is asserted hermetically. - - This proves control flow given `_delete_batch`'s documented cluster - return contract. It proves nothing about CROSSSLOT behaviour, node - targeting, or whether FT.SEARCH enumerates every shard. + 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 @@ -519,9 +511,8 @@ def test_clear_terminates_when_no_key_can_be_deleted(self): ) page = [{"id": "test:key1"}, {"id": "test:key2"}] - # A runaway guard for the test itself: pytest-timeout is not installed, - # so a regression that removes the bound would hang the suite instead of - # failing it. + # 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} diff --git a/tests/unit/test_extension_create_index_flag.py b/tests/unit/test_extension_create_index_flag.py index 446a3c6cc..65f58df14 100644 --- a/tests/unit/test_extension_create_index_flag.py +++ b/tests/unit/test_extension_create_index_flag.py @@ -150,10 +150,7 @@ 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 in place, so it stays - available: guarding it too left a caller attached to a platform-provisioned - index with no public way to invalidate the cache, which pushed downstream - code into reimplementing the keyspace walk against private APIs. + `clear()` removes entries and leaves the index standing, so it does not. """ @pytest.mark.parametrize("kind", ALL_KINDS) @@ -188,10 +185,8 @@ async def test_async_cache_dropping_the_index_is_rejected(self, vectorizer): assert client.mock_calls == [] def test_cache_clear_issues_no_index_command(self, vectorizer): - # The reason this one is safe to allow: the cache clears by keyspace - # prefix, so it never names the index. Asserted as "ft() was never - # reached" rather than on the SCAN call shape, which belongs to - # BaseCache and is being reworked for cluster correctness. + # 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"]) @@ -210,10 +205,7 @@ def test_cache_clear_issues_no_index_command(self, vectorizer): @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. MessageHistory's body is currently - # byte-identical to SemanticMessageHistory's, but parametrizing it - # anyway is what keeps that from being load-bearing: re-adding a guard - # to either one has to fail a test. + # that control reaches it at all. cleared = Mock(return_value=0) monkeypatch.setattr(SearchIndex, "clear", cleared) extension = _build(