fix: prevent SearchIndex/AsyncSearchIndex finalizer from leaking every instance

[nkanu17]

Background

An external user reported that every SearchIndex and AsyncSearchIndex instance is retained for the lifetime of the process, and attributed unbounded memory growth in a document-ingestion service (which constructs an index per upload request) to it.

Their production telemetry, quoted here as reported and not independently verified: about 33.5 KB leaked per request, container RSS climbing 3.82 GB to 5.72 GB over 42 hours at roughly 21k requests/hour, and pods OOM-killed about every 4 days.

What was verified locally against origin/main (v0.24.0):

• 300 of 300 constructed indexes of each class survive del plus a full gc.collect().
• Retained memory over 3000 construct-and-drop cycles, measured as RSS delta after gc.collect(): about 5.1 KB per construction with a 3-field schema, and about 29 KB with a 25-field schema (the instance drags its IndexSchema, every field object, its lock, and its Redis client along with it). With this fix both fall to noise (roughly 0.1 KB) and zero instances are retained.
• Explicit disconnect() does not mitigate the leak, and indexes given a caller-provided client never leaked. Both corollaries follow from the mechanism below.

The bug was introduced by #280 and first shipped in 0.4.0, so every release from 0.4.0 through 0.24.0 is affected (wider than the 0.18.2+ range in the original report).

Description

SearchIndex.__init__ registered a finalizer whose callback is a bound method of the instance being finalized:

if self._owns_redis_client:
    weakref.finalize(self, self.disconnect)

A bound method holds a strong reference to its instance, and weakref.finalize stores its callback in a module-level registry that is a genuine GC root until the finalizer fires. The finalizer can only fire once the instance is unreachable, but the instance can never become unreachable while the registry holds the callback that references it. The finalizer therefore never fires, disconnect() never runs, and the index (schema, field objects, locks, and Redis client) is immortal.

AsyncSearchIndex had the same defect one level removed: sync_wrapper(self.disconnect) returns a closure over the bound method.

This is the documented constraint of weakref.finalize: the callback must not hold a reference to the object being finalized.

Reproduction (no Redis needed): construct and drop 300 indexes, then gc.collect(). Before this change, all 300 remain alive for both classes; after, zero. This also explains the two corollaries above: the registry entry retains the instance whether or not disconnect() is ever called, and no finalizer is registered at all on the caller-provided-client path.

Fix

Bind the finalizer to the client, never to self. Binding at __init__ alone is not enough: in the default from_dict/from_yaml + redis_url path the client does not exist yet at construction and is created lazily, so an __init__-bound finalizer would capture None and silently stop closing real connections.

Instead, the finalizer is registered at every site where an owned client is created or replaced, and detached whenever that client is closed or swapped out:

• Module-level close callbacks (_close_owned_sync_client, _close_owned_async_client) that take the client as an argument and cannot reference the index. The async callback reuses the existing shutdown-hardened sync_wrapper.
• _register_client_finalizer / _detach_client_finalizer helpers on BaseSearchIndex. Registration detaches any previous finalizer first, so a replaced client is never closed later by a stale finalizer.
• Registration sites: __init__ (covers the from_existing constructors, which pass an owned client), lazy creation (sync _redis_client property, async _get_client), and the deprecated connect() / set_client() paths.
• Both disconnect() implementations detach the finalizer before closing, so an explicitly disconnected index is not double-closed at collection time.

Behavior is unchanged for indexes that do not own their client: no finalizer is registered and the caller's client is never touched.

Testing

Test-driven: all tests were written first and confirmed failing against unmodified origin/main, then the fix was applied.

• tests/unit/test_index_gc_finalizer.py (11 tests, no Redis required): instances of both classes are collectable after del + gc.collect(); repeated construct/drop cycles do not accumulate; a lazily created owned client is closed exactly once on collection; explicit disconnect() closes once and detaches; injected clients are never closed. Before the fix 8 of 11 failed (the 3 passing were the unowned-client cases, consistent with the diagnosis).
• tests/integration/test_index_gc_finalizer_integration.py (6 tests, real Redis via testcontainers): an owned client that performed a real round trip is closed exactly once on index collection and its pool sockets are torn down; same for the async client through sync_wrapper; injected sync and async clients survive collection and still answer PING; the index-per-request pattern leaves no live instances. Before the fix 4 of 6 failed.
• Full non-API suite locally (pytest -n auto, redis:8.4): 1976 passed. Six test_sql_redis_hybrid.py tests failed locally at the time, and were verified to fail identically on unmodified origin/main, so they were unrelated to this change. Correction added after merge: their cause was a stale local virtualenv holding sql-redis 0.6.0 while pyproject.toml requires >=0.7.1. After uv sync --all-extras all six pass locally. They were never broken in the repository or in CI.
• make format, make check-types (mypy), and pre-commit all pass.

On the redis:latest CI job

One matrix job (Python 3.13 - redis-py 7.x [redis:latest]) fails with two test_hybrid.py assertions returning zero rows. This looks unrelated to this change:

• tests/docker-compose.yml already documents this failure mode: Redis 8.8 changed the RediSearch worker-pool default to a multithreaded background executor, "which surfaces a race where FT.SEARCH can return nil fields for docs that expire mid-query -- the source of flaky redis:latest CI runs".
• Nightly runs on main fail the same way on redis:latest with a rotating cast of tests (test_unf_noindex_integration on Jul 31, test_query_cosine_distance_un_normalized on Jul 29), all with the same signature of fewer documents than expected.
• Both failing tests pass locally against redis:latest (8.10.0), repeatedly, under -n auto.
• Structurally, this change only closes a client the index itself created. RedisConnectionFactory.get_redis_connection builds a fresh pool per call via Redis.from_url with no caching, so a finalizer firing on one index cannot affect any other client's connections or document visibility.

Two separate observations for whoever picks up the redis:latest flakiness, neither of which blocks this PR:

1. The compose mitigation is inert. The official redis image does not read REDIS_ARGS (that is a redis-stack convention), so --search-workers 0 is never applied. Confirmed locally: a container started with REDIS_ARGS=--maxmemory 123mb reports maxmemory 0, and one started with the compose settings reports search-workers 14. The flags belong in a compose command: entry.
2. That said, the root cause of the redis:latest failures is still unattributed. Loading documents and querying immediately, 150 iterations against redis:latest (8.10.0) with search-workers 14 and again with search-workers 0, produced no shortfall in either configuration (vector and filter queries both saw all documents every time). So the worker-pool race described in the compose comment was not reproducible here, and fixing item 1 should not be assumed to fix the flake.

Notes for users on affected versions (0.4.0 through 0.24.0)

• Prefer one long-lived index object per logical index rather than one per operation; this avoids the leak entirely and is also cheaper (no connection churn).
• Calling disconnect() does not mitigate the leak on affected versions; only instance reuse does.

[nkanu17]

@codex review

[Copilot]

Pull request overview

Fixes a process-lifetime memory leak where SearchIndex / AsyncSearchIndex instances that own their Redis client could never be garbage collected due to a weakref.finalize(...) callback that (indirectly) held a strong reference back to the index instance. The new design registers finalizers that close the owned client (not the index), and detaches/re-registers them when the owned client is created, replaced, or explicitly disconnected.

Changes:

• Replace instance-bound finalizers with module-level close callbacks that only capture the owned Redis client (sync + async).
• Add BaseSearchIndex helpers to register/detach client finalizers, and wire them into owned-client creation/replacement sites and both disconnect() paths.
• Add unit + integration regression tests verifying collectability and correct owned/unowned client closing behavior (including lazy client creation).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
redisvl/index/index.py	Reworks finalizer registration to avoid retaining index instances; ensures owned clients are closed on GC and finalizers are detached on disconnect/replacement.
tests/unit/test_index_gc_finalizer.py	Unit regression tests (mocked clients) for GC collectability and owned/unowned client close semantics.
tests/integration/test_index_gc_finalizer_integration.py	Integration tests (real Redis) verifying owned client sockets are torn down and index-per-request usage does not accumulate live instances.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[abrookins]

Makes sense to me! 👍 Good find.

[applied-ai-release-bot]

🚀 PR was released in v0.25.0 🚀