From 817e33ee7ec037f497a2ff241ff287fa2e7d5bde Mon Sep 17 00:00:00 2001 From: Eric DelOrefice <114540347+ericdelorefice@users.noreply.github.com> Date: Wed, 23 Sep 2026 23:28:31 -0400 Subject: [PATCH 1/3] Python: do not report an unreachable Redis as a missing index RedisCollection.collection_exists caught every exception and returned False. ensure_collection_deleted gates the drop on that answer, so a timeout, an authentication failure or an unreachable node made it log at debug level and return successfully having deleted nothing. The caller is told the collection is gone when it is not. Catch ResponseError, which is what redis-py raises when FT.INFO reports an unknown index. ConnectionError, TimeoutError and AuthenticationError are siblings of ResponseError rather than subclasses, so they now propagate. The ChromaCollection connector already distinguishes these two cases: it handles a genuinely absent collection quietly and raises VectorStoreOperationException for anything else. The existing test used a bare Exception, which is not what Redis raises and matched a timeout just as readily. It now uses ResponseError, and a new test asserts a connection error reaches the caller. Fixes #14473 --- python/semantic_kernel/connectors/redis.py | 9 ++++++++- .../unit/connectors/memory/test_redis_store.py | 18 +++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/python/semantic_kernel/connectors/redis.py b/python/semantic_kernel/connectors/redis.py index 575624895aca..6f26c1599202 100644 --- a/python/semantic_kernel/connectors/redis.py +++ b/python/semantic_kernel/connectors/redis.py @@ -14,6 +14,7 @@ from pydantic import SecretStr, ValidationError from redis.asyncio.client import Redis +from redis.exceptions import ResponseError 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 @@ -287,7 +288,13 @@ async def collection_exists(self, **kwargs) -> bool: try: await self.redis_database.ft(self.collection_name).info() return True - except Exception: + except ResponseError: + # Redis answers FT.INFO for an unknown index with an error reply, which redis-py + # surfaces as ResponseError. ConnectionError, TimeoutError and AuthenticationError are + # siblings of ResponseError rather than subclasses, so they now propagate instead of + # being reported as "the index is not there". ensure_collection_deleted() gates the + # drop on this answer, so a swallowed timeout made it return successfully having + # deleted nothing. return False @override diff --git a/python/tests/unit/connectors/memory/test_redis_store.py b/python/tests/unit/connectors/memory/test_redis_store.py index e779ad945a97..22c05fe4df3a 100644 --- a/python/tests/unit/connectors/memory/test_redis_store.py +++ b/python/tests/unit/connectors/memory/test_redis_store.py @@ -12,6 +12,9 @@ RedisJsonCollection, RedisStore, ) +from redis.exceptions import ConnectionError as RedisConnectionError +from redis.exceptions import ResponseError + from semantic_kernel.exceptions import VectorStoreInitializationException, VectorStoreOperationException BASE_PATH = "redis.asyncio.client.Redis" @@ -281,11 +284,24 @@ async def test_collection_exists(collection_hash, mock_collection_exists): async def test_collection_exists_false(collection_hash, mock_collection_exists): - mock_collection_exists.side_effect = Exception + # ResponseError is what redis-py raises when FT.INFO reports an unknown index. The previous + # bare Exception did not represent that, and matched a timeout just as readily. + mock_collection_exists.side_effect = ResponseError("Unknown Index name") 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() + + async def test_ensure_collection_deleted(collection_hash, mock_ensure_collection_deleted): await collection_hash.ensure_collection_deleted() await collection_hash.ensure_collection_deleted() From 85e37c9af926b80708f8f9caf8973690276a3b71 Mon Sep 17 00:00:00 2001 From: Eric DelOrefice <114540347+ericdelorefice@users.noreply.github.com> Date: Wed, 23 Sep 2026 23:34:46 -0400 Subject: [PATCH 2/3] Fix import ordering flagged in review redis.exceptions sorts after redis.commands, and in the test file the new imports split the third-party block from the semantic_kernel one. Ruff's isort gate is enabled in pyproject.toml, so both would have failed lint. ruff check and ruff format now pass on both files, and the unit tests still pass 27/27. --- python/semantic_kernel/connectors/redis.py | 2 +- python/tests/unit/connectors/memory/test_redis_store.py | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/python/semantic_kernel/connectors/redis.py b/python/semantic_kernel/connectors/redis.py index 6f26c1599202..5933aeba2097 100644 --- a/python/semantic_kernel/connectors/redis.py +++ b/python/semantic_kernel/connectors/redis.py @@ -14,10 +14,10 @@ from pydantic import SecretStr, ValidationError from redis.asyncio.client import Redis -from redis.exceptions import ResponseError 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.index import process_results from redisvl.query.filter import FilterExpression, Num, Tag, Text from redisvl.query.query import BaseQuery, VectorQuery diff --git a/python/tests/unit/connectors/memory/test_redis_store.py b/python/tests/unit/connectors/memory/test_redis_store.py index 22c05fe4df3a..a5a23da1209a 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 ConnectionError as RedisConnectionError +from redis.exceptions import ResponseError from semantic_kernel.connectors.redis import ( RedisCollectionTypes, @@ -12,9 +14,6 @@ RedisJsonCollection, RedisStore, ) -from redis.exceptions import ConnectionError as RedisConnectionError -from redis.exceptions import ResponseError - from semantic_kernel.exceptions import VectorStoreInitializationException, VectorStoreOperationException BASE_PATH = "redis.asyncio.client.Redis" From 52538db5f095d9d22d7f93242a0a06e672591fd5 Mon Sep 17 00:00:00 2001 From: Eric Del Orefice <114540347+ericdelorefice@users.noreply.github.com> Date: Mon, 2 Nov 2026 11:24:25 -0500 Subject: [PATCH 3/3] Python: treat only the missing-index reply as an absent Redis collection NoPermissionError and ClusterDownError subclass ResponseError, so catching ResponseError alone still reported a denied or failed FT.INFO as an absent index, and ensure_collection_deleted() skipped the drop. Only a plain ResponseError with the missing-index wording now returns False; everything else is re-raised. --- python/semantic_kernel/connectors/redis.py | 22 +++++--- .../connectors/memory/test_redis_store.py | 56 +++++++++++++++++-- 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/python/semantic_kernel/connectors/redis.py b/python/semantic_kernel/connectors/redis.py index 5933aeba2097..622f585ad3bc 100644 --- a/python/semantic_kernel/connectors/redis.py +++ b/python/semantic_kernel/connectors/redis.py @@ -58,6 +58,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) @@ -288,14 +293,17 @@ async def collection_exists(self, **kwargs) -> bool: try: await self.redis_database.ft(self.collection_name).info() return True - except ResponseError: + except ResponseError as exc: # Redis answers FT.INFO for an unknown index with an error reply, which redis-py - # surfaces as ResponseError. ConnectionError, TimeoutError and AuthenticationError are - # siblings of ResponseError rather than subclasses, so they now propagate instead of - # being reported as "the index is not there". ensure_collection_deleted() gates the - # drop on this answer, so a swallowed timeout made it return successfully having - # deleted nothing. - return False + # 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 a5a23da1209a..7435750d5d80 100644 --- a/python/tests/unit/connectors/memory/test_redis_store.py +++ b/python/tests/unit/connectors/memory/test_redis_store.py @@ -5,8 +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 -from redis.exceptions import ResponseError from semantic_kernel.connectors.redis import ( RedisCollectionTypes, @@ -282,10 +282,21 @@ 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): - # ResponseError is what redis-py raises when FT.INFO reports an unknown index. The previous - # bare Exception did not represent that, and matched a timeout just as readily. - mock_collection_exists.side_effect = ResponseError("Unknown Index name") +@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 @@ -301,6 +312,41 @@ async def test_collection_exists_propagates_connection_errors(collection_hash, m 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()