fix: check index existence with FT.INFO instead of listing every index - #672
Merged
Conversation
…work exists() asked a question about one index but answered it with FT._LIST, which returns every index name in the database and which Redis tags @admin. ACL rules apply left to right, so a credential built the way least-privilege guidance recommends -- grant what is needed, then take back administrative commands -- lost the command. Because create() checks exists() first, index creation and the construction of SemanticCache, SemanticMessageHistory, MessageHistory and SemanticRouter failed outright rather than degrading. exists() now runs FT.INFO, which Redis tags @search only, through the existing _info() helper so it inherits that helper's cluster routing. It returns False only for a recognised missing-index reply and re-raises anything else, so a permission or connection failure is never reported as an absent index. FT.INFO answers for an alias's target, so the resolved index_name is compared against the schema name: reporting an alias as existing would let create(overwrite=True) drop the index it points at. listall() still uses FT._LIST, since enumerating the database is what it is for. The missing-index wordings, which differ across Redis versions, now live in a single shared predicate in redisvl/exceptions.py, replacing a duplicated copy in the MCP server. Two behaviour changes: - exists() raises RedisSearchError where it previously let raw redis-py exceptions escape. This makes create() uniform, since it already wrapped every other Redis failure that way; the existence check was the one path that leaked. - A credential whose key patterns do not cover the index prefix now gets a permission error instead of True, because FT.INFO is key-scoped and FT._LIST is not. Redis applies the same rule to FT.SEARCH, so such a credential could not have queried the index anyway.
vishal-bala
marked this pull request as ready for review
August 7, 2026 13:25
tylerhutcherson
approved these changes
Aug 7, 2026
|
🚀 PR was released in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
exists()asks a question about one index. It answered that question withFT._LIST, which returns the name of every index in the database, and then checked whether ours was in the list. That works — but it answers a narrow question with a broad command, and Redis prices the two differently.FT._LISTis tagged@admin, alongside@searchand@slow.FT.INFO, which reports on a single named index, is tagged@searchonly. ACL rules apply left to right, so a credential built the way least-privilege guidance recommends — grant what's needed, then take back administrative commands — losesFT._LISTwhile keepingFT.INFO:create()checksexists()before it does anything else, andSemanticCache,SemanticMessageHistory,MessageHistory, andSemanticRouterall callcreate()while being constructed. So this wasn't a degraded mode users could route around — it was a hard failure on the first Redis call:This change points
exists()atFT.INFO, reusing the existing_info()helper so it inherits that helper's cluster routing.listall(), which genuinely does need to enumerate the database, still usesFT._LIST.How narrow is this? Narrower than it may look, and worth stating plainly so the fix isn't over-read.
FT._LISTdoes not require@admin— granting+@searchalone permits it. Only rules that explicitly subtract@adminafter granting search were affected.Aliases, and why there's a name comparison.
FT.INFOaccepts an index alias and answers for the index it points at. Left alone, that would makeexists()returnTruefor an alias — and becausecreate(overwrite=True, drop=True)acts on that answer by issuingFT.DROPINDEX <name> DD, it would delete the aliased index and its documents.exists()therefore compares theindex_namethatFT.INFOreturns against the schema's own name.delete()has the same hazard independently of this change and now carries a docstring warning; guarding it would cost a round-trip on every delete, so it's left as a follow-up.Classifying "missing". Redis Search reports an absent index as an ordinary error reply, with wording that has changed between versions (
Unknown index name,<name>: no such index, andSEARCH_INDEX_NOT_FOUND Index not found:as of 8.8). All known wordings now live in one shared predicate inredisvl/exceptions.py, replacing a duplicated copy in the MCP server. It reads__cause__rather than the wrappingRedisSearchError, whose message interpolates the index name — otherwise an index named after one of the wordings would make an unrelated failure look like an absence. Anything not recognised as a missing index is re-raised rather than reported as absence.Behaviour changes
exists()raisesRedisSearchErrorwhere it previously let rawredis-pyexceptions escape. This makescreate()uniform: it already wrapped every other Redis failure that way, and the existence check was the one path that leaked. Callers catchingredis.exceptions.ResponseErroraroundcreate()or an extension constructor should catchRedisSearchErrorand read__cause__.True, becauseFT.INFOis key-scoped andFT._LISTis not. Redis applies the same rule toFT.SEARCH, so such a credential could not have queried the index anyway.Tests
tests/unit/test_index_exists.py— 13 cases covering the three branches for both twins, plus clustertarget_nodesrouting. They drive the real_info()rather than stubbing it, so the not-found match is exercised through that helper's message rewrapping. Mutation-checked: narrowing the predicate to one wording, replacing theexceptwith a barereturn False, deleting theindex_namecomparison, bypassing_info(), narrowing_info()'sexcept, and reading the wrapper instead of__cause__each fail specific cases.+@all -@adminand pins the premise withpytest.raises(NoPermissionError)onFT._LIST, so it can't silently go vacuous if Redis ever recategorises the command — it fails against the previous implementation with the original error. The other covers alias resolution. Verified on 8.2.7, 8.4.4, and 8.8.0.FT._LISTcall tolistall(), which had no direct coverage before.test_no_proactive_module_checks.pyupdated for the exception-type change above; it now also asserts theredis-pyexception is chained.Docs
New "Redis permissions (ACLs)" section in
docs/user_guide/installation.mdwith an operation-to-command table, a subsection on how Redis Cloud and Redis Software differ (both manage ACLs through their own control plane rather thanACL SETUSER), and a note that RedisVL'sCLIENT SETINFO/ECHOidentification step also needs permission.docs/api/exceptions.rstgains a section on telling a missing index apart from other failures.docs/api/cli.rstnotes which commandsFT._LISTgates.Not in scope
Sync
listall()still lacks the cluster branch its async twin has.delete()'s alias hazard is documented but not guarded.migration/utils.pyandmigration/async_executor.pystill have bareexcept Exception: return Falseblocks that this predicate would improve. Separately,RedisConnectionFactory.get_redis_connectioncannot open a connection under an ACL that denies bothCLIENT SETINFOandECHO, because the fallback atconnection.py:550is itself unguarded andNoPermissionErrorsubclassesResponseError— pre-existing, and filed separately.Note
Medium Risk
Changes core index lifecycle (
exists()drivescreate()and several constructors) with new error semantics and alias handling; behavior shifts for restricted ACLs and key-scopedFT.INFO, but scope is focused and well tested.Overview
exists()on sync and async search indexes no longer callsFT._LIST(membership in the full index list). It usesFT.INFOvia_info(), so least-privilege ACLs such as+@search -@admincan runcreate()and extension constructors that depend onexists().Missing-index detection is centralized in
_is_missing_index_error()inredisvl/exceptions.py(version-specific Redis error text, matched on__cause__).exists()returnsFalseonly for those replies; permission and connection failures raiseRedisSearchError.FT.INFOon an alias is treated as non-existent by comparingindex_namefrom the response to the schema name, avoidingcreate(overwrite=True)dropping the aliased index.listall()still usesFT._LIST. MCP reuses the shared missing-index helper. Docs add Redis ACL guidance, exception handling notes, and CLI notes forlistall/ migration discovery. Tests cover unit branches, ACL integration, and alias behavior.Reviewed by Cursor Bugbot for commit fb00bb1. Bugbot is set up for automated code reviews on this repo. Configure here.