Repository navigation
Python: do not report an unreachable Redis as a missing index - #14474
Eric Del Orefice (ericdelorefice) wants to merge 3 commits into
Conversation
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 microsoft#14473
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Redis imports are incorrectly ordered in both reviewed files and will fail configured Ruff checks.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This pull request ensures Redis connection failures are not reported as missing indexes.
Changes:
- Handles
ResponseErrorfor missing indexes. - Adds tests for connection-error propagation.
| File | Summary |
|---|---|
python/semantic_kernel/connectors/redis.py |
Updates Redis index existence error handling. |
python/tests/unit/connectors/memory/test_redis_store.py |
Adds targeted exception tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 ConnectionError as RedisConnectionError | ||
| from redis.exceptions import ResponseError |
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.
|
Good catch on both, and you are right. Fixed and pushed.
One correction on the report itself: the second instance is at line 20 of the test file, not line 291 of the connector. Line 291 is the |

Motivation and Context
RedisCollection.collection_existscaught every exception and returnedFalse, andensure_collection_deletedgates the drop on that answer:collection_exists()reaches Redis to answer, so a timeout, an authentication failure or an unreachable node all returnFalse.ensure_collection_deletedthen logs at debug level and returns normally, and the caller is told the collection is gone when it is still there.The
ChromaCollectionconnector already draws this distinction - it handles a genuinely absent collection quietly and raisesVectorStoreOperationExceptionfor anything else. This brings the Redis connector in line with it.Fixes #14473
Description
collection_existsnow catchesResponseError, which is what redis-py raises whenFT.INFOreports an unknown index.ConnectionError,TimeoutErrorandAuthenticationErrorare siblings ofResponseErrorrather than subclasses, so they propagate instead of being reported as an absent index.On the test change:
test_collection_exists_falseused a bareExceptionas the side effect. Redis does not raise that, and it matched a timeout as readily as a missing index, so the test could not tell the two cases apart. It now raisesResponseError, which is the real behaviour, and a new test asserts that a connection error reaches the caller.I checked that the new test fails without the source change, so it is testing what it claims to.
Contribution Checklist
pytest tests/unit/connectors/memory/test_redis_store.pypasses 27/27