Repository navigation
Python: do not report an unreachable Redis as a missing index - #14474
Eric Del Orefice (ericdelorefice) wants to merge 6 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
GitHub Actions
Failure
GitHub Actions
Failure
|
@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.
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 |
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Inactive
semantic-kernel-automation
Bot
added
the
python
GitHub Actions
Inactive
| await self.redis_database.ft(self.collection_name).info() | ||
| return True | ||
| except Exception: | ||
| except ResponseError: |
There was a problem hiding this comment.
ResponseError is not specific to a missing index. In the supported redis-py 6.x range, NoPermissionError and ClusterDownError also inherit from it. An FT.INFO permission denial or cluster-down response therefore still returns False, and ensure_collection_deleted() reports success without deleting anything—the failure mode in #14473 remains for these errors. I verified the catch with the method isolated from SDK imports and checked the redis-py exception hierarchy. Please handle only the specific unknown-index response quietly and propagate other response errors; add permission/cluster-error regressions alongside the connection-error test.
There was a problem hiding this comment.
Thanks for catching the NoPermissionError and ClusterDownError subclasses. I had only checked the connection-side classes, and both inherit from ResponseError in 7.x as well.
Pushed 52538db. collection_exists() now returns False only for a plain ResponseError whose text contains, ignoring case, one of the missing-index wordings: "Unknown index name", ": no such index", SEARCH_INDEX_NOT_FOUND, or "Index not found". Everything else is re-raised. The plain-ResponseError check is there so a permission error can't slip through just because a user or index name contains one of those phrases. I took the wordings from the RediSearch source and redisvl's list.
New tests next to the connection-error one: a permission error, a cluster-down error, a generic ResponseError, and a permission error whose text contains a missing-index phrase all propagate from collection_exists(). ensure_collection_deleted() raises on a denied FT.INFO without calling dropindex, and each missing-index wording still returns False. The propagation tests fail on 44d07ac and pass now (36 passed on redis-py 6.0.0 and 7.4.1), and ruff check and format are clean. I did not run this against a live Redis Search server.
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.
Head branch was pushed to by a user without write access
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
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, an unreachable node, a permission denial or a cluster-down reply 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 returnsFalseonly for a plainResponseErrorwhose text contains, ignoring case, one of the missing-index wordings: "Unknown index name", ": no such index",SEARCH_INDEX_NOT_FOUND, or "Index not found" (taken from the RediSearch source and redisvl's list). Everything else is re-raised:ConnectionError,TimeoutErrorandAuthenticationError, which are siblings ofResponseError, and alsoNoPermissionErrorandClusterDownError, which are subclasses of it (review feedback from Eduard van Valkenburg (@eavanvalkenburg)).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 raisesResponseErrorwith each missing-index wording. New tests assert that a connection error, a permission error, a cluster-down error and otherResponseErrors reach the caller, and thatensure_collection_deletedraises on a deniedFT.INFOwithout callingdropindex.I checked that the new propagation tests fail without the source change, so they test what they claim to.
Contribution Checklist
pytest tests/unit/connectors/memory/test_redis_store.pypasses 36/36 on redis-py 6.0.0 and 7.4.1