Skip to content

feat: let callers opt out of index creation with create_index=False - #687

Open
vishal-bala wants to merge 2 commits into
fix/acl-drop-echo-identification-fallbackfrom
feat/extension-create-index-flag
Open

feat: let callers opt out of index creation with create_index=False#687
vishal-bala wants to merge 2 commits into
fix/acl-drop-echo-identification-fallbackfrom
feat/extension-create-index-flag

Conversation

@vishal-bala

@vishal-bala vishal-bala commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #685, which fixes the connection-time failure this PR's credential also hits. Review that one first.

Every extension checks whether its index exists while being constructed, and that check is FT.INFO. A credential assembled from +@read +@write is denied FT.INFO and FT.CREATE together — neither command is in either category, identically on Redis 8.0.6 through 8.8.1 — so such a role cannot construct SemanticCache, MessageHistory, SemanticMessageHistory or SemanticRouter at all, even against an index it can query perfectly well:

RedisSearchError: Error while fetching llmcache index info:
User <name> has no permissions to run the 'FT.INFO' command

There was no way to ask for less. overwrite=False is the reason the check runs — create() calls exists() first and consults overwrite only afterwards — drop is not a constructor parameter at all, and overwrite=True is strictly worse, since it proceeds to FT.DROPINDEX. SearchIndex exposes no lifecycle seam either, and each constructor calls create() inline with no hook to subclass around.

create_index=False lets the caller state what RedisVL cannot ask: the index exists. It skips the existence check, the schema comparison against the live index, and creation, so the constructor issues no index command at all. It is rejected together with overwrite=True, which asks for the opposite.

Why not a runtime probe

FT.SEARCH is permitted where FT.INFO is not, so probing with it looks attractive. Its reply is identical for an index and for an alias pointing at one, though, so a probe would reinstate the create(overwrite=True, drop=True)FT.DROPINDEX <alias> DD data-loss path that #672 closed. And a credential that cannot ask whether the index exists cannot create one either, so there is nothing to work out at runtime — exists() keeps failing loudly instead.

The invariant this had to fix first

Constructor-time create() was the de-facto eager connect. SearchIndex.client returns the raw client and is None until the lazy _redis_client property runs, so skipping create() left ten self._index.client sites in redisvl/extensions/ dereferencing None — starting with the router's route_config write, which runs immediately after index setup. All ten now use _redis_client. Two of the # type: ignore comments they carried turned out to cover a real scan_by_pattern signature mismatch rather than the Optional, and are kept with explicit codes.

Router semantics

create_index=False means the index exists, is already seeded, and is not ours to rewrite, so the router also skips writing route references and the stored route_config. Rewriting that blob from an unverified local route list would truncate a shared router's routes, and JSON.SET is @write, so a restricted credential can do it. _update_router_state() stays armed — add_route() and remove_route() are the caller acting deliberately — but that consequence is now stated in add_route()'s own docstring as well as the guide, since the docstring is the reference for anyone who never opens the guide.

SemanticRouter.from_existing() threads the flag through. It reads the stored config with JSON.GET and reaches FT.INFO only via the constructor, so with create_index=False it issues no index command and becomes the way to attach to a router under a restricted credential. The flag is popped before _split_from_existing_kwargs, which retains only SearchIndex init kwargs and would otherwise pass it to the Redis client constructor — where SearchIndex.__init__ discards unknown kwargs silently, making the mistake invisible.

Separately, routes=[] now raises a useful error when matching instead of max() arg is an empty sequence. That is unconditional: emptiness is legal on either path, and a flag about index ownership should not decide it.

Tests

tests/unit/test_extension_create_index_flag.py — 17 cases. The contract is "no index command at all", so they assert on the client: a MagicMock records every call, and ft() is the gate every FT.* command passes through. All four constructors reach zero recorded calls.

Two cases pin the lazy-connect invariant above by driving drop(id=...) and get_route_references() as the first operation — reverting any of the ten conversions otherwise breaks no test. Others pin that the flag survives as instance state, that it never reaches the router's stored config, and that from_existing() still verifies by default.

The default construction path is not re-tested here; the existing 189 integration tests already fail if create() is dropped.

One integration test round-trips store()/check() through a cache built with create_index=False under a real +@read +@write -@dangerous ACL user — the customer's rule, so the destructive commands it denies stay denied — with the premise pinned (FT.INFO must raise NoPermissionError for that user) and the negative alongside it: without the flag the same credential raises RedisSearchError naming ft.info, with NoPermissionError chained.

Docs

The ACL section of docs/user_guide/installation.md is restructured. Four statements were falsified by this change or were already wrong: that a credential needs @search at all, that all four extensions always call create(), that enumeration is the only thing an -@admin rule breaks, and the advice to grant FT.CREATE. The operation table gains a +@read +@write column, the command-to-category mapping is labelled as measured rather than documented, and the wrapped error text appears verbatim under its own heading — an H3, so it has an anchor to link to.

The pre-existing key-permission material became its own section rather than being dropped, and was corrected while there: partial key-pattern overlap is denied exactly like no overlap, not filtered down to the readable subset, and FT.CREATE is not key-checked at all, so a credential can create an index it cannot query.

A new subsection covers what the flag gives up. An absent index fails loudly, and a vector dimension mismatch does too — but only once the index holds a document, which a freshly provisioned index will not. A wrong prefix, an ON JSON index written as hashes, and a differing datatype or distance metric are silent. The tell is FT.INFO's key_type, prefixes and attributes, not hash_indexing_failures, which stays 0 because those keys were never indexing candidates. Router provisioning gets its own subsection, since preparing one for this mode needs embedded reference vectors rather than a hand-written FT.CREATE.

Two corrections worth calling out for reviewers who know this area: clear() is not uniform — only SemanticCache.clear() avoids FT.INFO, while the other three delegate to SearchIndex.clear(), which calls info() first — and Redis Cloud's predefined Read-Write rule reads as @read/@write-shaped from its published description, so it is a candidate for this problem rather than immune to it.

Not in scope

create_index=False restrains construction only; delete() and clear() stay armed. Coupling ownership to the flag is the coherent next step, and the flag is stored as instance state so it can be added without another parameter.

adk_redis needs a matching field on RedisVLCacheProviderConfig before this reaches callers who construct through that provider. Until then the interim for the escalation remains the exists() monkeypatch already shared in the thread.


Note

Medium Risk
Behavior changes constructor and lifecycle paths for four public extension APIs and SemanticRouter provisioning; misused create_index=False can yield silent empty queries, though defaults preserve existing behavior.

Overview
Adds create_index=False on SemanticCache, MessageHistory, SemanticMessageHistory, and SemanticRouter so constructors skip FT.INFO, schema checks, and FT.CREATE—needed when +@read +@write (or similar) roles cannot run index-metadata commands. overwrite=True with create_index=False is rejected via shared constants; index-wide delete() / clear() (and async cache equivalents) raise when the index is externally managed.

SemanticRouter with the flag does not seed reference vectors or write route_config; from_existing(..., create_index=False) threads the flag and stays on JSON.GET only. Empty routes now fails matching with a clear ValueError instead of max() on an empty sequence.

Extension code switches self._index.client to _redis_client so skipping constructor create() no longer leaves lazy connections as None on first use.

Docs expand the installation ACL guide (operation matrix, create_index=False workflow, router provisioning, silent schema mismatches) and point FT.INFO permission errors at the new flag in exceptions.rst. New unit tests assert zero Redis calls at construction; integration tests exercise a real read/write ACL user.

Reviewed by Cursor Bugbot for commit 46459d6. Bugbot is set up for automated code reviews on this repo. Configure here.

@vishal-bala vishal-bala added the auto:minor Increment the minor version when merged label Aug 14, 2026
@nkanu17
nkanu17 force-pushed the feat/extension-create-index-flag branch from efdf94e to 3097a50 Compare August 14, 2026 14:53
@vishal-bala
vishal-bala force-pushed the feat/extension-create-index-flag branch from 3097a50 to ff8feb1 Compare August 14, 2026 14:58
@vishal-bala
vishal-bala marked this pull request as ready for review August 14, 2026 14:58

@nkanu17 nkanu17 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the alias-target deletion path introduced by create_index=False. Normal construction uses SearchIndex.exists() to reject aliases, but this mode skips that identity guard while leaving extension-wide delete operations armed. Please fail closed for lifecycle-wide destructive operations when the index is externally managed, or require a separate explicit ownership/force opt-in, and add alias regression coverage across all four extensions plus async cache deletion. The remaining inline comments are non-blocking correctness and documentation fixes.

self._index.create(overwrite=self.overwrite, drop=False)
# Create the search index in Redis
self._index.create(overwrite=self.overwrite, drop=False)
else:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — prevent cross-alias target deletion. This branch deliberately skips the existence/identity check, so it can now construct an extension whose name is an index alias. All four extension delete() methods (and cache adelete()) later call FT.DROPINDEX <name> DD; Redis resolves an alias and drops the separately named target index plus its documents. Normal construction does not expose this path because SearchIndex.exists() reports aliases as absent and creation then fails. Please reject lifecycle-wide delete/adelete when _create_index is false unless ownership is separately and explicitly enabled; clear() should be considered under the same ownership rule. Add an alias regression for every extension and the async cache path.

"""
# Pulled out before the split below, which retains only SearchIndex init
# kwargs and would otherwise hand this to the Redis client constructor.
create_index = kwargs.pop("create_index", True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — handle overwrite before splitting connection kwargs. create_index is popped here, but overwrite is not. from_existing(..., create_index=False, overwrite=True) therefore bypasses the constructor conflict check: with a supplied client the value is silently ignored, while URL-based construction forwards it to redis-py and raises TypeError: AbstractConnection.__init__() got an unexpected keyword argument overwrite. Pop and reject or explicitly forward overwrite, and cover both connection forms.

Comment thread docs/user_guide/installation.md Outdated
| `index.delete()`, `rvl index delete`, `rvl index destroy` | `FT.DROPINDEX` | Yes | Yes |
| Enumerating indexes (see below) | `FT._LIST` | **No** | **No** |

Every `Yes` above assumes key patterns that cover the index prefix — see [Key permissions](#key-permissions) — and `-@dangerous` layered on the second column additionally denies `FT.DROPINDEX`, so `index.delete()` becomes `No`. An SVS-VAMANA schema needs more than `FT.CREATE`: `index.create()` first probes capabilities with `INFO` (`@slow @dangerous`) and `MODULE LIST` (`@admin @slow @dangerous`), so both `-@admin` and `-@dangerous` policies break creation for those schemas.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — this overstates key-pattern requirements. The table includes index.create() as a Yes, but the Key permissions section below correctly says FT.CREATE is not checked against index-prefix key patterns. Please carve out FT.CREATE here so the guide does not imply that Redis prevents creating an index the caller cannot subsequently query.

Comment thread docs/user_guide/installation.md Outdated

`SemanticRouter` with `create_index=False` writes nothing at all: not the reference vectors for its routes, and not the stored route config that `SemanticRouter.from_existing()` reads. Preparing a router for this mode therefore means constructing it once with a privileged credential — a hand-written `FT.CREATE` is not enough, because the reference vectors have to be embedded and written too. Without them the router matches nothing, which looks like a distance-threshold problem rather than an empty index.

Afterwards, `SemanticRouter.from_existing(name, create_index=False)` is the way to attach to it: it recovers the routes and thresholds with `JSON.GET` and needs no `FT.INFO`. Pass the full route set. Each route's distance threshold is applied from the local list, so a partial set silently narrows matching — and `add_route()` and `remove_route()` rewrite the stored config from that same list, so attaching with a subset and then adding a route drops the rest from the config every other client reads.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — this instruction cannot be followed with this API. SemanticRouter.from_existing() does not accept a route set; it obtains routes from the stored route_config. Please say that the stored config must contain the complete route set, or move this warning to direct SemanticRouter(..., routes=..., create_index=False) construction.

Comment thread docs/user_guide/installation.md Outdated
### When the schema diverges

`SemanticCache`, `SemanticMessageHistory`, `MessageHistory`, and `SemanticRouter` each call `index.create()` while being constructed, so they are covered by the first row.
With `create_index=False` nothing verifies that the live index matches the schema you described. Some mismatches are loud on first use, and two are silent:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — the count is incorrect. The table identifies three silent mismatch classes: prefix, storage type, and datatype/distance metric. Use “three” or “several.”

Comment thread docs/user_guide/installation.md Outdated

### Roles built from `@read` and `@write`

`FT.INFO` is in neither `@read` nor `@write` — its only category is `@search` — and `FT.CREATE` is the same. So an application role assembled from `+@read +@write` — a natural least-privilege shape — can query and load, but cannot ask whether an index exists and cannot create one. Note that subtracting `@dangerous` is not what causes this: `+@all -@dangerous` permits both. The commands are simply never granted.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — avoid calling the bare role least privilege. This same guide shows that +@read +@write grants FT.DROPINDEX, so users can copy this “natural least-privilege” shape while retaining a destructive command. Recommend +@read +@write -@dangerous for the runtime example, with explicit grants for anything additionally required.

Every extension checks whether its index exists while being constructed, and
that check is `FT.INFO`. A credential assembled from `+@READ +@write` is denied
`FT.INFO` and `FT.CREATE` together -- neither command is in either category, and
measured on Redis 8.0.6 through 8.8.1 the mapping is identical -- so such a role
cannot construct `SemanticCache`, `MessageHistory`, `SemanticMessageHistory` or
`SemanticRouter` at all, even against an index it can query perfectly well:

    RedisSearchError: Error while fetching llmcache index info:
    User <name> has no permissions to run the 'FT.INFO' command

There was no way to ask for less. `overwrite=False` is the *reason* the check
runs -- `create()` calls `exists()` first and consults `overwrite` only
afterwards -- `drop` is not a constructor parameter at all, and `overwrite=True`
is strictly worse, since it proceeds to `FT.DROPINDEX`. `SearchIndex` exposes no
lifecycle seam either, and each constructor calls `create()` inline with no hook
to subclass around.

`create_index=False` lets the caller state what RedisVL cannot ask: the index
exists. It skips the existence check, the schema comparison against the live
index, and creation, so the constructor issues no index command at all. It is
rejected together with `overwrite=True`, which asks for the opposite.

Deliberately not a runtime probe. `FT.SEARCH` would be permitted where `FT.INFO`
is not, but its reply is identical for an index and for an alias pointing at one,
so a probe would reinstate the `create(overwrite=True, drop=True)` ->
`FT.DROPINDEX <alias> DD` data-loss path that #672 closed. A credential that
cannot ask whether the index exists cannot create one either, so there is
nothing to work out at runtime -- `exists()` keeps failing loudly instead.

## The invariant this had to fix first

Constructor-time `create()` was the de-facto eager connect. `SearchIndex.client`
returns the raw client and is `None` until the lazy `_redis_client` property
runs, so skipping `create()` left ten `self._index.client` sites in
`redisvl/extensions/` dereferencing `None` -- starting with the router's
`route_config` write, which runs immediately after index setup. All ten now use
`_redis_client`. Two of the `# type: ignore` comments they carried turned out to
cover a real `scan_by_pattern` signature mismatch rather than the Optional, and
are kept with explicit codes.

## Router semantics

`create_index=False` means the index exists, is already seeded, and is not ours
to rewrite, so the router also skips writing route references and the stored
`route_config`. Rewriting that blob from an unverified local route list would
truncate a shared router's routes, and `JSON.SET` is `@write`, so a restricted
credential can do it. `_update_router_state()` stays armed: `add_route()` and
`remove_route()` are the caller acting deliberately -- but that consequence is
now stated in `add_route()`'s own docstring and in the constructor's, not only in
the guide, since the docstring is the reference for anyone who never opens it.

`SemanticRouter.from_existing()` now threads the flag through. It reads the
stored config with `JSON.GET` and reaches `FT.INFO` only via the constructor, so
with `create_index=False` it issues no index command and becomes the way to
attach to a router under a restricted credential. The flag is popped before
`_split_from_existing_kwargs`, which retains only `SearchIndex` init kwargs and
would otherwise pass it to the Redis client constructor -- where
`SearchIndex.__init__` discards unknown kwargs silently, making the mistake
invisible.

Separately, `routes=[]` now raises a useful error when matching instead of
`max() arg is an empty sequence`. That is unconditional: emptiness is legal on
either path, and a flag about index ownership should not decide it.

## Tests

- `tests/unit/test_extension_create_index_flag.py` -- 17 cases. The contract is
  "no index command at all", so they assert on the client: a `MagicMock` records
  every call, and `ft()` is the gate every `FT.*` command passes through. All
  four constructors reach zero recorded calls. Two cases pin the lazy-connect
  invariant above by driving `drop(id=...)` and `get_route_references()` as the
  first operation -- reverting any of the ten conversions otherwise breaks no
  test. Others pin that the flag survives as instance state, that it never
  reaches the router's stored config, and that `from_existing()` still verifies
  by default. The default construction path is not re-tested here; the existing
  189 integration tests already fail if `create()` is dropped.
- One integration test round-trips `store()`/`check()` through a cache built with
  `create_index=False` under a real `+@READ +@Write -@dangerous` ACL user -- the
  customer's rule, so the destructive commands it denies stay denied -- with the
  premise pinned (`FT.INFO` must raise `NoPermissionError` for that user) and the
  negative alongside it: without the flag the same credential raises
  `RedisSearchError` naming `ft.info`, with `NoPermissionError` chained.

## Docs

`docs/user_guide/installation.md`'s ACL section is restructured. Four statements
were falsified by this change or were already wrong: that a credential needs
`@search` at all, that all four extensions always call `create()`, that
enumeration is the only thing an `-@admin` rule breaks, and the advice to grant
`FT.CREATE`. The operation table gains a `+@READ +@write` column, the
command-to-category mapping is labelled as measured rather than documented, and
the wrapped error text appears verbatim under its own heading -- an H3, so it has
an anchor to link to.

The pre-existing key-permission material was rewritten as its own section rather
than dropped, and corrected while there: partial key-pattern overlap is denied
exactly like no overlap, not filtered down to the readable subset, and
`FT.CREATE` is not key-checked at all, so a credential can create an index it
cannot query.

A new subsection covers what the flag gives up. An absent index fails loudly, and
a vector dimension mismatch does too -- but only once the index holds a document,
which a freshly provisioned index will not. A wrong prefix, an `ON JSON` index
written as hashes, and a differing datatype or distance metric are silent. The
tell is `FT.INFO`'s `key_type`, `prefixes` and `attributes`, not
`hash_indexing_failures`, which stays `0` because those keys were never indexing
candidates. Router provisioning gets its own subsection, since preparing one for
this mode needs embedded reference vectors rather than a hand-written
`FT.CREATE`.

Two corrections worth calling out: `clear()` is not uniform -- only
`SemanticCache.clear()` avoids `FT.INFO`, while the other three delegate to
`SearchIndex.clear()`, which calls `info()` first -- and Redis Cloud's predefined
Read-Write rule reads as `@read`/`@write`-shaped from its published description,
so it is a candidate for this problem rather than immune to it.

`docs/api/exceptions.rst` gains one cross-reference: when the credential
genuinely cannot run `FT.INFO`, the permission error is not something to handle.

## Not in scope

`create_index=False` restrains construction only; `delete()` and `clear()` stay
armed. Coupling ownership to the flag is the coherent next step, and the flag is
stored as instance state so it can be added without another parameter.

`adk_redis` needs a matching field on `RedisVLCacheProviderConfig` before this
reaches callers who construct through that provider.
@vishal-bala
vishal-bala force-pushed the feat/extension-create-index-flag branch from ff8feb1 to 3e40a2e Compare August 14, 2026 15:18
Comment thread redisvl/extensions/cache/llm/semantic.py
Comment thread redisvl/extensions/router/semantic.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 46459d6. Configure here.

"""Async clear all cache keys when RedisVL manages the index lifecycle."""
if not self._create_index:
raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT)
await super().aclear()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alias drop via public index

High Severity

create_index=False skips the alias-aware exists() check so a cache can attach when name is an alias, and the new delete/clear/adelete/aclear guards block that on the extension. The public index and aindex properties still return a raw SearchIndex whose delete runs FT.DROPINDEX on the alias and drops the target index plus its documents. The guide claims attach-only instances protect aliases from that destruction.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 46459d6. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto:minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants