set_client() keeps client ownership, so the index closes a caller-provided client
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 427
- Forks
- 101
- Avg merge
- 6d 3h
- Merged PRs (30d)
- 20
Description
Summary
An index created with redis_url owns the client it lazily creates, so _owns_redis_client is True. The deprecated set_client() does not reset that flag, so after a caller swaps in their own client the index still believes it owns it and will close it. __init__(redis_client=...) already gets this right and marks such a client as not owned.
Before 0.25.0 this was latent, because the client finalizer never fired (see #657). Now that the finalizer works, the index closes the caller's client when it is garbage collected.
Affects both SearchIndex and AsyncSearchIndex, on 0.25.0.
Reproduction
import gc
from unittest import mock
from redisvl.index import SearchIndex
SCHEMA = {
"index": {"name": "probe", "prefix": "p", "storage_type": "hash"},
"fields": [{"name": "a", "type": "tag"}],
}
caller_client = mock.MagicMock()
index = SearchIndex.from_dict(SCHEMA, redis_url="redis://localhost:6379")
with mock.patch("redisvl.index.index.RedisConnectionFactory.validate_sync_redis"):
index.set_client(caller_client)
print("owns:", index._owns_redis_client) # True, should be False
del index
gc.collect()
print("caller client closed:", caller_client.close.called) # True, should be False
Observed on 0.25.0:
owns: True
caller client closed: True
The async path behaves the same, awaiting aclose() on the caller's client. Calling disconnect() explicitly closes it too, for the same reason.
Impact
Low severity but real. redis-py clients recover from close() and aclose() by reconnecting on next use, so the practical effect is unexpected connection churn on a client the caller still owns rather than a permanently broken client. Verified: ping() returns True after both close() and aclose() for sync and async clients.
The exposure is further limited because set_client() is deprecated.
Additional problem in the same method
The sync set_client() also abandons the client the index created for itself, without closing it. It overwrites __redis_client and (since 0.25.0) detaches that client's finalizer, so nothing ever closes it. The async set_client() does not have this problem because it awaits disconnect() before swapping.
Suggested fix
set_client() should mark the client as not owned, since a caller-provided client is by definition not the index's to close, and it should release the previously owned client first.
The deprecated async connect() needs care here: it creates its own client and then delegates to set_client(), so a naive ownership flip would leave a client the index created with nobody to close it. That path must keep ownership. Routing both through a small internal helper that takes ownership as a parameter handles this.
Worth covering with tests on all three entry points: constructor injection (already correct), set_client() (must not own), and connect() (must own).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with SearchIndex and AsyncSearchIndex ownership handling in set_client(), connect(), disconnect(), and constructor injection. Trace how sync and async clients are finalized or closed, then add coverage for constructor injection, set_client(), and connect() to verify caller-provided clients remain open while internally created clients are released.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, redis
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 70/100