diff --git a/python/semantic_kernel/connectors/redis.py b/python/semantic_kernel/connectors/redis.py index 4accb9fd78b7..8fcb8a705d01 100644 --- a/python/semantic_kernel/connectors/redis.py +++ b/python/semantic_kernel/connectors/redis.py @@ -18,6 +18,7 @@ from redis.commands.search.field import Field as RedisField from redis.commands.search.field import NumericField, TagField, TextField, VectorField from redis.commands.search.index_definition import IndexDefinition, IndexType +from redis.exceptions import ResponseError from redisvl.index import AsyncSearchIndex from redisvl.index.index import process_results from redisvl.query.filter import FilterExpression, Num, Tag, Text @@ -59,6 +60,11 @@ logger = logging.getLogger(__name__) +# Redis Search has no structured "index not found" reply and has worded it differently across +# versions ("Unknown index name", ": no such index", and since 8.8 an error code), so the +# missing-index case is recognised by its text. +_MISSING_INDEX_ERROR_FRAGMENTS = ("unknown index name", "no such index", "search_index_not_found", "index not found") + TKey = TypeVar("TKey", bound=str) TQuery = TypeVar("TQuery", bound=BaseQuery) @@ -315,8 +321,17 @@ async def collection_exists(self, **kwargs) -> bool: try: await self.redis_database.ft(self.collection_name).info() return True - except Exception: - return False + except ResponseError as exc: + # Redis answers FT.INFO for an unknown index with an error reply, which redis-py + # surfaces as a plain ResponseError. Other failures also arrive as ResponseError or a + # subclass of it (NoPermissionError, ClusterDownError), so only a plain ResponseError + # with the missing-index wording means "not there"; everything else propagates, as + # ConnectionError and TimeoutError do. ensure_collection_deleted() gates the drop on + # this answer, so a swallowed failure made it return successfully having deleted nothing. + message = str(exc).lower() + if type(exc) is ResponseError and any(fragment in message for fragment in _MISSING_INDEX_ERROR_FRAGMENTS): + return False + raise @override async def ensure_collection_deleted(self, **kwargs) -> None: diff --git a/python/tests/unit/connectors/memory/test_redis_store.py b/python/tests/unit/connectors/memory/test_redis_store.py index 99083f186961..86afecdb55da 100644 --- a/python/tests/unit/connectors/memory/test_redis_store.py +++ b/python/tests/unit/connectors/memory/test_redis_store.py @@ -5,6 +5,8 @@ import numpy as np from pytest import fixture, mark, raises from redis.asyncio.client import Redis +from redis.exceptions import ClusterDownError, NoPermissionError, ResponseError +from redis.exceptions import ConnectionError as RedisConnectionError import semantic_kernel.connectors.redis as redis_module from semantic_kernel.connectors.redis import ( @@ -281,12 +283,71 @@ async def test_collection_exists(collection_hash, mock_collection_exists): await collection_hash.collection_exists() -async def test_collection_exists_false(collection_hash, mock_collection_exists): - mock_collection_exists.side_effect = Exception +@mark.parametrize( + "message", + [ + "Unknown Index name", + "Unknown index name (or name is an alias itself)", + "test: no such index", + "SEARCH_INDEX_NOT_FOUND", + "Index not found", + ], +) +async def test_collection_exists_false(collection_hash, mock_collection_exists, message): + # ResponseError is what redis-py raises when FT.INFO reports an unknown index, worded + # differently across Redis Search versions. The previous bare Exception did not represent + # that, and matched a timeout just as readily. + mock_collection_exists.side_effect = ResponseError(message) exists = await collection_hash.collection_exists() assert not exists +async def test_collection_exists_propagates_connection_errors(collection_hash, mock_collection_exists): + """A cluster that cannot be reached is not an absent index. + + ensure_collection_deleted() drops the index only when collection_exists() says it is there, so + reporting a connection failure as "not there" made the delete a silent no-op. + """ + mock_collection_exists.side_effect = RedisConnectionError("connection refused") + with raises(RedisConnectionError): + await collection_hash.collection_exists() + + +@mark.parametrize( + "error", + [ + NoPermissionError("User default has no permissions to run the 'ft.info' command"), + ClusterDownError("The cluster is down"), + ResponseError("ERR unexpected reply"), + # The wording match must not apply to a subclass, even when the message happens to + # contain a missing-index phrase (here through the user name). + NoPermissionError("User search_index_not_found has no permissions to run the 'ft.info' command"), + ], + ids=["no_permission", "cluster_down", "other_response_error", "no_permission_with_matching_text"], +) +async def test_collection_exists_propagates_other_response_errors(collection_hash, mock_collection_exists, error): + """Only the missing-index reply means "not there". + + NoPermissionError and ClusterDownError subclass ResponseError, so catching ResponseError alone + still reported a denied or failed FT.INFO as an absent index. + """ + mock_collection_exists.side_effect = error + with raises(type(error)): + await collection_hash.collection_exists() + + +async def test_ensure_collection_deleted_propagates_permission_errors( + collection_hash, mock_collection_exists, mock_ensure_collection_deleted +): + """A denied FT.INFO must fail the delete instead of skipping the drop and returning normally.""" + mock_collection_exists.side_effect = NoPermissionError( + "User default has no permissions to run the 'ft.info' command" + ) + with raises(NoPermissionError): + await collection_hash.ensure_collection_deleted() + mock_ensure_collection_deleted.assert_not_called() + + async def test_ensure_collection_deleted(collection_hash, mock_ensure_collection_deleted): await collection_hash.ensure_collection_deleted() await collection_hash.ensure_collection_deleted()