feat(index): owns_client for explicit Redis client ownership handover - #726
Merged
vishal-bala merged 12 commits intoOct 1, 2026
Merged
vishal-bala merged 12 commits into
vishal-bala merged 12 commits into
Conversation
BaseCache._get_async_redis_client called get_async_redis_connection, which warns unconditionally, so anyone who built a cache from a redis_url and awaited a cache method saw a DeprecationWarning for an API they never called. A suite-wide filter in pyproject.toml hid it. Point the method at _get_aredis_connection, the async form already used everywhere else, and drop the filter. Cache clients now also report their library name via CLIENT SETINFO like every other RedisVL client, and a connection failure surfaces when the client is created rather than at the first command. The regression guard lives in tests/unit/test_connection_normalization.py and lands with the next commit, which is the first to touch that file.
An index closed only a client it created itself, and callers who needed to override that wrote to the private _owns_redis_client keyword or poked the attribute afterwards. The MCP server did the latter, which never worked as intended: _register_client_finalizer gates on the flag, so a post-construction flip lands after registration has already declined and no finalizer is ever created. owns_client states ownership once, at construction, before the finalizer is registered. It replaces the private keyword rather than sitting alongside it, so there is one spelling and no precedence question. An explicit value also wins over the ownership from_existing would otherwise assume for a client it created, which is why the assignment there uses setdefault. Also documents the accessor asymmetry between the two classes: _redis_client lazily creates on SearchIndex but is a plain nullable attribute on AsyncSearchIndex, whose lazy getter is _get_client.
The four migration modules read index.client, which is None until the client is lazily created, and each handled that differently: one raised, two recorded an error, one dereferenced unguarded, and the planner degraded to an empty key sample that then made the key-sample check vacuously true. All four build their index through from_existing, which always yields a client, so none of this was reachable in practice. Reading through _redis_client and _get_client removes the disagreement and the dead guards with it. The test doubles are renamed to match the real accessors so they still stand in for an index.
The deprecation decorators told users each deprecated argument, function and class would be removed "in the next major release". Every release so far has been 0.x and breaking changes ship on minor bumps per project convention, so that promise has never been accurate and each removal would otherwise need a release note explaining the mismatch. Say "in a future release" instead. Warning text only; no behaviour change.
Moving this method onto _get_aredis_connection introduced an await between its "is the client None" check and the assignment, because the async factory issues a CLIENT SETINFO round trip. BaseCache has no lock, so two concurrent callers each built a client and the first was left unreachable: adisconnect only closes the client currently on the instance, so the orphan's connection pool was never released. Wrap the lazy path in a double-checked lock, the same shape AsyncSearchIndex._get_client already uses over the same factory. The regression test fails without the lock and passes with it.
SemanticRouter.from_existing merges {**init_kwargs, **index_kwargs}
with index_kwargs second, and set owns_client there unconditionally on
the branch where it creates the client. A caller's owns_client=False
was therefore discarded in silence, while SearchIndex.from_existing
honoured the same argument via setdefault. Same keyword, same verb,
opposite answer.
Claim ownership only when the caller has not already answered, so all
three public from_existing entry points agree.
Review of the owns_client work turned up several statements that were wrong or missing rather than merely terse. The owns_client entry said the index owns a client it created "from redis_url", but get_redis_connection falls back to REDIS_URL, so an index built with no connection arguments at all still creates and owns one. It also left the caller's obligation unstated: declining ownership of a client the index created means closing it yourself. disconnect was documented as "Disconnect from the Redis database" on the base and sync classes and not at all on the async one, which now misleads: it is a no-op for an unowned client, and with owns_client public that is a state callers choose. Its log line claimed the index did not own the client even when the index had created it. A test docstring asserted set_client() was already gone. It is not, until the next branch removes it, and pointing readers away from it hides the ownership footgun owns_client exists to fix. Also: coerce owns_client with bool(), since the finalizer gate tests truthiness while disconnect tested "is False", so a falsy non-bool made the two paths disagree; reject the retired private _owns_redis_client keyword loudly, because underscore-prefixed keywords are forwarded verbatim and it would otherwise be dropped in silence; hoist a lazily created client out of a per-key loop; drop the last dead client-is-None guard in the migration package; and stop one more warning promising removal in the next major release.
…internal-callers
Two conflicts, both where main had touched the same lines this branch did.
redisvl/extensions/cache/base.py: main removed the now-unused Mapping
import while this branch added `import asyncio` for the lazy-client lock.
Kept both decisions — asyncio is still used, Mapping is used nowhere.
redisvl/migration/planner.py: main replaced the hand-rolled SCAN loop in
_sample_keys with scan_iter, because a cluster client replies with a
{node_name: cursor} mapping that cannot be fed back as a cursor. Took
main's body wholesale; keeping this branch's version would have
reintroduced that bug. The cast() this branch added existed only to type
the loop it replaced, so it and its import are gone. The SyncRedisClient
annotation on the client parameter survives and still applies.
One semantic break that merged cleanly but did not work: main's new
test_migration_cluster_scan.py stands an _Index double in for the
validators, exposing .client, while this branch moved the validators onto
_redis_client and _get_client(). The double now mirrors the real
accessors, matching the two doubles this branch already updated.
Main replaced the hand-rolled SCAN loop in the validator with scan_iter, which removed the only cast() in the file. `make lint` runs format and mypy but not pylint, so nothing in the standard checks would flag it.
vishal-bala
marked this pull request as ready for review
September 10, 2026 12:04
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ 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 29c37b1. Configure here.
…lient/01-owns-client-and-internal-callers
…nswered Two review findings on #726, both in code this branch introduced. The lock that serialises lazy async client creation in BaseCache was built in __init__ and held for the cache's lifetime. An asyncio.Lock records the loop that first contends it and raises RuntimeError from any other, so reconnecting after adisconnect() failed whenever it landed on a new loop -- a second asyncio.run, a fresh pytest-asyncio loop, a notebook cell. It now rebuilds when the running loop changes, which is all a lock guarding one object's creation needs. Only the contended acquire reaches the loop check, so the regression test has to race two callers on each loop; a single-caller version passes either way. from_existing claimed a factory-built client with setdefault("owns_client", True), which an explicit owns_client=None satisfies without replacing. __init__ then inferred ownership from redis_client -- already populated by then -- read it as not-owned, and left the client it had just built for nobody to close. A caller forwarding an optional setting lands there. Both index flavours and SemanticRouter now test for None rather than for presence. AsyncSearchIndex._lock carries the same loop-binding flaw on main, untouched by this branch, and is left for its own change.
…lient/01-owns-client-and-internal-callers
limjoobin
approved these changes
Oct 1, 2026
limjoobin
left a comment
Contributor
There was a problem hiding this comment.
LGTM, this PR cleans up the migration module and introduces a new public API with the owns_client argument for explicit ownership.
vishal-bala
added a commit
that referenced
this pull request
Oct 2, 2026
One conflict, in redisvl/extensions/cache/base.py, resolved by taking main's version of the file. main (#726, #727) independently typed BaseCache.redis_kwargs as a TypedDict, _CacheConnectionKwargs, with the same three fields, and removed the same four casts. This branch's RedisConnectionKwargs was the only change it made to that file, so keeping both would have left two identical types. utils.py merged cleanly: main reworded the three deprecation messages ("a future release" rather than "the next major release"), and the ParamSpec retyping of the same functions is untouched.
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.

Motivation
#660 reports that
SearchIndex.set_client()keeps client ownership, so an index closes a client the caller still owns. Two earlier pull requests tried to repair the ownership logic insideset_client()and both were closed: the method has carried aDeprecationWarningsince v0.4.0 and the project has shipped 23 further minor versions without removing it, so patching it adds machinery to a code path nobody should be on. The agreed fix is removal.This is the first of three stacked pull requests doing that. It removes nothing. It puts in place the one thing callers lose when
set_client()goes — a supported way to hand an index a client it should close — and clears the internal callers that would otherwise complicate the removal diffs. The two branches above it delete the deprecated index API and the deprecated connection-factory API.Changes
A public
owns_client, replacing a private keywordAn index closed only a client it created itself, and callers needing to override that passed a private
_owns_redis_clientkeyword or wrote the attribute afterwards. The MCP server did the latter, which never worked as intended:_register_client_finalizergates on the flag, so a post-construction flip lands after registration has already declined and no finalizer is ever created. That server was relying entirely on its explicitdisconnect(), with no safety net if a binding runtime were ever dropped.owns_clientstates ownership once, at construction, before the finalizer is registered. It replaces the private keyword rather than sitting beside it, so there is one spelling and no precedence question.The default is unchanged: an index owns a client it created and never closes one you passed. Note the default follows from who created the client, not from which argument you used —
get_redis_connectionfalls back toREDIS_URL, so an index built with no connection arguments at all also creates and owns one.Async cache clients no longer warn, and no longer race
BaseCache._get_async_redis_clientcalledget_async_redis_connection, which warns unconditionally, so anyone who built a cache from aredis_urland awaited a cache method saw aDeprecationWarningfor an API they never called. A suite-widefilterwarningsentry inpyproject.tomlhad been hiding it. The method now uses_get_aredis_connectionand the filter is gone.That move introduced an
awaitinside the lazy-init guard, which made check-then-set non-atomic and let two concurrent callers each build a client, orphaning the first with its pool never released. The lazy path is now wrapped in the same double-checked lockAsyncSearchIndex._get_clientalready uses over the same factory. The regression test fails without the lock.Migration modules read the client that actually exists
The four modules under
redisvl/migration/readindex.client, which isNoneuntil the client is lazily created, and each handled that differently: one raised, two recorded an error, one degraded to an empty key sample that then made the key-sample check vacuously true. All four build their index throughfrom_existing, which always yields a client, so none of it was reachable. They now read through the lazy accessors and the dead guards are gone.Smaller changes
disconnect()was documented as "Disconnect from the Redis database" on two classes and not at all on the async one, and its log line claimed the index did not own a client it had in fact created. Both corrected, andowns_clientis documented onfrom_existingtoo.owns_clientis coerced withbool(): the finalizer gate tests truthiness whiledisconnecttestedis False, so a falsy non-boolmade the two paths disagree._owns_redis_clientkeyword is now rejected with aTypeError. Underscore-prefixed keywords are forwarded verbatim by_split_from_existing_kwargs, so it would otherwise be dropped in silence and leak the connection it used to control.Release Notes
SearchIndexandAsyncSearchIndexaccept a newowns_clientargument controlling whether the index closes the Redis client when it is disconnected or garbage collected. By default an index closes only a client it created itself and never closes one passed asredis_client, which is unchanged. Passowns_client=Trueto hand over a client you created, orowns_client=Falseto keep one the index would otherwise close.A cache built from a
redis_urlnow raisesConnectionErrorwhen its async client is created rather than at the first command, so an unreachable server surfaces earlier than before.Notes
SemanticRouter.from_existingmerged{**init_kwargs, **index_kwargs}withindex_kwargssecond, so it discarded an explicitowns_client=FalsewhileSearchIndex.from_existinghonoured it. All three publicfrom_existingentry points now agree, and the rule is that an explicit value beats the ownership the library would otherwise infer. The consequence worth knowing:from_existing(name, redis_url=..., owns_client=False)produces a client nobody closes. The caller keeps a handle via.client, so it is an escape hatch rather than a leak, but there is no good reason to ask for it.Cache clients now issue
CLIENT SETINFOwhen they are created, so they finally report their library name like every other RedisVL client. The same change means an unreachable server surfacesConnectionErrorat client creation rather than at the first command. AResponseErrorthere is still swallowed, so a restricted ACL that forbidsCLIENT SETINFOis unaffected.Handing the same client to two indexes with
owns_client=Trueregisters two finalizers on it, so collecting the first closes the pool under the second. Do not do this. The redis-py documentation is explicit that closing one client which shares aConnectionPoolsilently invalidates the connections every other client using it holds, so the consequence is silent failure rather than reconnection. Ownership belongs to exactly one holder.The migration modules now reach into another package's private accessors. That is deliberate staging: the root cause is
.clientbeingOptional, which cannot be fixed untilconnect()andset_client()are gone. Branch 03 makes.clientnon-optional and reverts these call sites to the public property.BaseCachestill derives its own ownership flag with no keyword override and registers no finalizer, so there is no cache equivalent of this handover. That asymmetry is intentional here and recorded in a comment; nothing in the library needs to hand a cache a client it should close.Note
Medium Risk
Changes Redis client ownership and lazy async creation paths used by indexes, caches, migration, and MCP; misconfigured
owns_clientor shared clients could leak or close pools unexpectedly, though defaults stay the same and coverage is expanded.Overview
Introduces a public
owns_clientargument onSearchIndexandAsyncSearchIndex(and wires it throughfrom_existing,SemanticRouter, and the MCP server) so callers can declare who closes the Redis client at construction time, replacing the private_owns_redis_clientkwarg—which now raisesTypeErrorif passed.BaseCache._get_async_redis_clientno longer calls the warning-emittingget_async_redis_connection; itawaits_get_aredis_connectionbehind a double-checkedasyncio.Lockthat is rebound when the event loop changes, fixing concurrent duplicate clients and reconnect-after-adisconnecton a new loop. The pytestfilterwarningssuppression for that deprecation is removed.Migration planner/validator/executor code now obtains clients via lazy accessors (
_redis_client/await _get_client()) instead ofindex.client, dropping deadNoneguards. Deprecation copy shifts from “next major release” to “in a future release,” anddisconnect()docs/logging align with ownership behavior.Reviewed by Cursor Bugbot for commit b620272. Bugbot is set up for automated code reviews on this repo. Configure here.