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
…deleted too little (microsoft#14510) ### Motivation and Context `ChromaCollection` has two handlers for the same condition, and the error type they were written against changed underneath them. `pyproject.toml` allows `chromadb >= 0.5, < 1.6`. Across that range the error for a missing collection is not the same: ``` chromadb 0.5.0 errors.py defines NotFoundError: False chromadb 1.5.9 client.get_collection("no_such") -> chromadb.errors.NotFoundError isinstance(e, ValueError) -> False ``` Both symptoms follow from that one fact, and they point in opposite directions. **`collection_exists` catches too much.** `except Exception: return False`, so an unreachable server, a rejected request or a timeout all report that the collection is not there. Three places in this repo already say that is not the rule. `_get_collection`, nine lines above, raises `RuntimeError` from the identical `client.get_collection(...)` call. `ensure_collection_deleted`, in the same class, raises `VectorStoreOperationException` for anything that is not a missing collection. And `VectorStore.collection_exists` in `data/vector.py` wraps this method and narrows to `except VectorStoreOperationException`, so the layer above already assumes only a recognised failure becomes `False`. **`ensure_collection_deleted` catches too little.** Its `except ValueError` is the 0.5-era signal. On chromadb 1.x a genuinely absent collection raises `NotFoundError`, which is not a `ValueError`, so it falls through to `except Exception` and is re-raised as `VectorStoreOperationException`. The "could not be deleted because it doesn't exist" path is unreachable on any chromadb 1.x, which is most of the supported range. ### Description One module-level tuple resolves the version difference once: ```python try: # chromadb >= 1.0 raises NotFoundError for a missing collection from chromadb.errors import NotFoundError as _ChromaNotFoundError COLLECTION_NOT_FOUND: tuple[type[Exception], ...] = (ValueError, _ChromaNotFoundError) except ImportError: # chromadb < 1.0 raised ValueError COLLECTION_NOT_FOUND = (ValueError,) ``` `collection_exists` narrows from `Exception` to that tuple. `ensure_collection_deleted` widens from `ValueError` to the same tuple. Nothing else changes, and the guarded import keeps `chromadb 0.5` working, where `NotFoundError` does not exist. ### Contribution Checklist Four unit tests, no Chroma server. They resolve the not-found error themselves rather than importing the new constant, so the same file runs against the code before and after this change. Against unmodified `main`, two fail - one per symptom - and two pass: ``` FAILED test_collection_exists_raises_when_the_server_cannot_be_reached FAILED test_ensure_collection_deleted_tolerates_an_absent_collection ``` The two that pass on `main` pin what must not move: a not-found still returns `False`, and a real failure during delete still raises. ``` 17 passed # tests/unit/connectors/memory/test_chroma.py All checks passed! # ruff check 2 files already formatted # ruff format --check ``` ### Note This is the connector I named as the correct one in microsoft#14473, and it is correct in `ensure_collection_deleted`, which was the method that issue was about. `collection_exists` underneath it is not, which I only saw when I went back to check the claim. `RedisCollection.collection_exists` has the broad-catch half of this and is already covered by microsoft#14474. `CosmosNoSqlCollection.collection_exists` catches `CosmosHttpResponseError`, which includes 401, 403 and 5xx, so it may want the same narrowing - left out here because I have not read it closely enough to say.
GitHub Actions
Active
| 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
Active
GitHub Actions
Active
GitHub Actions
Active
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