fix: set_client() client ownership, plus paginate and typing fixes for #653

[nkanu17]

Fixes #660

Stacked on #653. The base of this PR is fix/missing-field-payload-result-parsing, so review that one first and merge it first. Two of the three commits here fix defects in #653, which is why this is stacked rather than standalone. #653 was rebased onto main so this stack applies cleanly.

Three commits:

1. fix: do not close a caller-provided client passed to set_client() (the original subject of this PR, set_client() keeps client ownership, so the index closes a caller-provided client #660)
2. fix: keep paginating past a page emptied by dropped documents (fixes a defect in fix: tolerate matched docs with missing field payload (Redis 8.8 expiry race) #653)
3. docs: note that complete does not cover writes that are not yet indexed (fills a gap in fix: tolerate matched docs with missing field payload (Redis 8.8 expiry race) #653's docs)

---

1. set_client() closed a client it did not own (#660)

__init__(redis_client=...) already treats a caller-provided client as not owned and never closes it. The deprecated set_client() did not follow that rule: an index created with redis_url owns the client it lazily creates, so _owns_redis_client stays True, and set_client() never reset it. The index went on believing it owned a client it had never created.

Latent before 0.25.0 because the finalizer never fired (#657). Now that it works, the index closes the caller's client on garbage collection, and disconnect() closes it too. Verified on 0.25.0 before this change, both classes:

_owns_redis_client after __init__(redis_url=...):  True
_owns_redis_client after set_client(caller):       True
caller's client closed on GC (sync):               True
caller's client aclosed on GC (async):             True

Severity is low: redis-py clients recover from close() and aclose() by reconnecting, so the effect is unexpected connection churn rather than a broken client, and set_client() is deprecated. I confirmed the recovery directly, ping() returns True after both.

The trap was the deprecated async connect(), which creates its own client and then delegated to set_client(). A plain ownership flip would have left a client the index created with nobody to close it. Both now route through _swap_client(client, *, owns: bool), so connect() keeps ownership and set_client() gives it up. Also fixed in passing: sync set_client() abandoned the index's own client without closing it.

Ownership is now consistent across all entry points:

Entry point	Who created the client	Index owns it
__init__(redis_client=...)	caller	no (unchanged)
__init__(redis_url=...) then lazy creation	index	yes (unchanged)
connect() (deprecated)	index	yes (unchanged)
set_client() (deprecated)	caller	no (fixed, was yes)

One existing test, test_search_index_set_client, asserted .client is None after disconnect(). That was only reachable through the bug, since disconnect() has always left non-owned clients in place, as it does for constructor-injected clients. It now asserts the corrected semantics, including that the caller's client still answers PING. That last assertion matters here because the test passes a sync client, which _validate_client converts into an async wrapper sharing the caller's pool; closing that wrapper would tear down connections the caller still needs.

2. paginate() silently truncated after a fully-dropped page (defect in #653)

Both paginate() implementations ended on any empty page:

results = self._query(query)
if not results:
    break

With #653, an empty page is ambiguous: either the result set is exhausted, or every match on that page was dropped for a missing field payload. Treating the second case as the end discards every remaining page, which is the same class of silent incompleteness #653 exists to prevent.

Reproduced on #653 before this fix by stubbing _query with one fully-dropped page followed by two healthy ones:

pages yielded: 0
docs seen: []

doc2 and doc3 were never delivered. Pagination now ends only when a page is empty and nothing was dropped. A page emptied entirely by drops is skipped rather than yielded, since callers expect yielded pages to be non-empty and the drop is already logged and counted upstream. The offset still advances, so a run of fully-dropped pages terminates at the first page with no matches instead of looping.

Needs every document on one page to be dropped, so it is not common, but page_size=1 makes it trivial and the short-TTL workloads #653 calls out are where it would show up.

Also in this commit: the query paths are annotated SearchResults instead of list[dict[str, Any]]. Without that, the completeness API #653 adds is invisible to type checkers and its own documented usage fails. mypy on the exact snippet from docs/concepts/queries.md, before:

error: "list[dict[str, Any]]" has no attribute "complete"  [attr-defined]
error: "list[dict[str, Any]]" has no attribute "dropped_count"  [attr-defined]

and after, Success: no issues found. SearchResults subclasses list, so this is purely a typing and discoverability change with no runtime effect.

3. complete does not mean the result set is complete (docs gap in #653)

results.complete reports that nothing was dropped client-side. It is not a guarantee about recent writes. The same Redis 8.8+ worker-pool change also means a freshly written document may not be indexed yet: the server never matches it, nothing is dropped, and a short result set comes back with complete still True.

Measured on redis:latest (8.10.0), 8 concurrent threads each loading 3 documents and querying immediately, 480 iterations:

configuration	shortfalls	docs seen at first query
search-workers 14 (image default)	17/480	0, 1, or 2 instead of 3
search-workers 0	0/480	always 3

In every shortfall all 3 hashes were already in the keyspace and the index caught up within 0.8 to 2.6 ms, so this is indexing lag, not expiry, and no TTLs are involved. The docs now say so, and point at --search-workers 0 or re-querying rather than relying on complete. Tracked separately in #662.

Testing

Test-driven throughout; every test below was confirmed failing first.

• tests/unit/test_index_client_ownership.py, 10 tests. 8 failed before the ownership fix; the 2 that passed were the connect() guards that had to keep passing.
• tests/integration/test_index_client_ownership_integration.py, 7 tests against real Redis: a caller's client still answers PING after collection and after disconnect() for both classes; the index's own client has its pool sockets torn down when replaced; a client created by connect() is closed exactly once on collection; plus the constructor-injection baseline.
• tests/unit/test_paginate_dropped_pages.py, 5 tests. 3 failed before the paginate fix. Covers a fully-dropped page mid-run, termination at a genuinely empty page, and a run where every page is dropped.

Full suite on this stack: 2032 passed, 143 skipped, 2 xfailed, no failures. make format, mypy, and pre-commit all clean.

Note

I had opened #659 for the REDIS_ARGS compose fix before spotting #653, which already contained it. #659 is now closed as a duplicate: the functional change there was byte-identical and the comment in #653 is the better one.

---

Note

Medium Risk
Changes connection lifecycle for deprecated client APIs (wrong close could break shared pools) and pagination behavior under document-drop races; scope is bounded by tests but affects core index query paths.

Overview
Fixes Redis client ownership when using deprecated set_client() / connect(): caller-injected clients are no longer closed on GC or disconnect(), matching __init__(redis_client=...), while index-created clients are still owned and released when swapped. Sync and async paths update _owns_redis_client under a lock (async via _swap_client).

Pagination no longer stops when a page is empty because every match was dropped for missing field payloads; it only ends on a truly empty page (dropped_count == 0), skips non-yieldable empty dropped pages, and still advances the offset.

Query processing APIs are typed to return SearchResults so complete / dropped_count are visible to type checkers. Docs clarify that results.complete reflects client-side drops only, not Redis 8.8+ indexing lag after recent writes.

^{Reviewed by Cursor Bugbot for commit 15fd17a. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

🟡 Not ready to approve

AsyncSearchIndex._swap_client() updates _redis_client under the async lock but flips _owns_redis_client outside it, which can expose an inconsistent state to concurrent coroutines (risking an incorrect close of a caller-provided client).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR fixes a client-lifecycle bug in the deprecated set_client() APIs for SearchIndex / AsyncSearchIndex, aligning their ownership semantics with constructor-injected clients so caller-provided Redis clients are no longer closed by disconnect() or GC finalizers. It also ensures previously index-owned clients are released before swapping in a caller’s client, and adds targeted unit/integration coverage for these ownership cases.

Changes:

• Fix sync SearchIndex.set_client() to release any previously-owned client and mark the new caller-provided client as unowned (no finalizer / no close).
• Refactor async client swapping through _swap_client(client, owns=...) so connect() keeps ownership while set_client() does not.
• Add new unit + integration tests for ownership semantics and update an existing async integration test expectation to match corrected behavior.

File summaries

File	Description
redisvl/index/index.py	Fixes deprecated client swap ownership semantics; introduces _swap_client(..., owns=...) for async and corrects sync set_client() to avoid closing caller clients.
tests/unit/test_index_client_ownership.py	New unit tests covering ownership flips, GC behavior, and releasing previously owned clients (sync + async).
tests/integration/test_index_client_ownership_integration.py	New live-Redis integration tests asserting caller clients remain usable after GC/disconnect while connect()-created clients are closed.
tests/integration/test_async_search_index.py	Updates set_client() test to assert corrected “unowned client remains open and present after disconnect” behavior.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[cursor]

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

^{❌ 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 3ed6684. Configure here.}

[Copilot]

🟢 Ready to approve

The changes align with the stated ownership/pagination semantics and are backed by substantial new unit and integration test coverage.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

redisvl/index/index.py:975

• set_client() unconditionally calls disconnect(). When the index does not own its current client (e.g., constructed with redis_client=), disconnect() logs an INFO message ("Index does not own client, not disconnecting"), which becomes noisy/misleading during a client swap. Consider only disconnecting when the index currently owns its client so swaps of caller-owned clients stay silent.

        # Release the client this index created for itself, if any, before
        # taking on the caller's client.
        self.disconnect()
        self.__redis_client = redis_client

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

There are remaining correctness/contract issues in redisvl/index/index.py (client leak on repeated connect() and CountQuery return-type mismatches) that should be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (5)

redisvl/index/index.py:1888

• SearchIndex._query() and SearchIndex.query() return an int for CountQuery (see CountQuery docs and process_results()), but are annotated as returning SearchResults and the docstring says "List[Result]". This is an API contract/typing mismatch for users and type checkers; update the return type and docstring to include the CountQuery case.

    def query(
        self, query: BaseQuery | AggregationQuery | HybridQuery | SQLQuery
    ) -> SearchResults:
        """Execute a query on the index.

redisvl/index/index.py:1863

• batch_query() can accept CountQuery because it takes Sequence[BaseQuery], and process_results() will return an int for those entries. The return type should reflect that it may contain either SearchResults or int values.

    def batch_query(
        self, queries: Sequence[BaseQuery], batch_size: int = 10
    ) -> list[SearchResults]:
        """Execute a batch of queries and process results."""

redisvl/index/index.py:960

• connect() replaces the current client without closing a previously owned client. Because _register_client_finalizer() detaches the prior finalizer, the old owned client can be leaked (open sockets) if connect() is called when an owned client already exists (e.g., after lazy creation from redis_url or a previous connect()). Capture and close the old client after swapping to avoid leaking resources.

        self.invalidate_sql_schema_cache()
        client = RedisConnectionFactory.get_redis_connection(
            redis_url=redis_url, **kwargs
        )
        with self._lock:
            self.__redis_client = client
            # This index created the client, so it owns and must close it. The
            # index may have been holding a caller-provided client until now
            # (constructor injection or set_client), in which case ownership
            # has to be taken back or nothing would ever close this one.
            self._owns_redis_client = True
            self._register_client_finalizer(client)

redisvl/index/index.py:345

• process_results() returns an integer for CountQuery (results.total), but its return annotation is SearchResults. This makes the internal contract inconsistent and forces incorrect typing through _query()/query(). Update the annotation to a union that matches runtime behavior.

This issue also appears in the following locations of the same file:

• line 1860
• line 1885

def process_results(
    results: "Result", query: BaseQuery, schema: IndexSchema
) -> SearchResults:

docs/concepts/queries.md:396

• Spelling inconsistency in this doc: earlier sections use "behavior", but this note uses British spelling "behaviour". For consistency within the document, use "behavior" here as well.

Measured on Redis 8.10.0 with 8 concurrent writers querying immediately after
loading, the index caught up within about 1 to 3 ms. If you need read-your-writes
behaviour for search, either start Redis with `--search-workers 0` or re-query
rather than relying on `complete`.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.