Reclaim abandoned JVector vector indices (thread + graph) (internal#104) - #764
Merged
Conversation
A GigaMap VectorIndex with any background feature enabled (eventual indexing, background optimization, background persistence) owns a daemon ScheduledExecutorService thread through a BackgroundTaskManager. The manager held the index via a strong reference and the executor's scheduled tasks are callbacks on it, so an index abandoned without an explicit close() was pinned forever: dropping the map/index references never made it collectable, and every abandon-and-reopen cycle leaked one scheduler thread plus the whole in-memory HNSW graph and vector store. BackgroundTaskManager now holds the index through a WeakReference and runs a lightweight liveness watchdog that self-terminates the executor once the index has been garbage-collected, so a dropped index becomes fully reclaimable (thread and graph) even with no storage in play. Self-termination runs on the executor thread and therefore does not await termination. Mirrors the self-cancelling weak-reference pattern already used by the cache module's EvictionManager. As additional hardening, EmbeddedStorageManager.shutdown() now closes the index groups of every loaded GigaMap so background threads stop and auxiliary resources are freed even when the caller forgets to close indices explicitly. Discovery enumerates the object registry (already-loaded instances only, so no Lazy subgraph is force-loaded) and invokes the opt-in PersistenceShutdownReleasable contract; GigaMap implements it and delegates to GigaIndices.closeAllGroups(), which closes Closeable groups without mutating the persistent graph. The three mechanisms compose as layered defenses: explicit close()/removeIndex, storage shutdown, and GC-driven self-termination. Also fixes a sibling resource leak in DiskIndexManager.writeIndexWithFusedPQ, where a throw between obtaining index.getView() and the trailing view.close() leaked the heap-only view; the view is now acquired in try-with-resources. Adds VectorIndexAbandonmentLeakTest covering the abandonment cases (index collectable, background threads released) and the storage-shutdown close path; verified to fail against the unpatched code.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a resource-leak scenario where background-enabled JVector VectorIndex instances could become permanently non-collectable when abandoned without an explicit close(), leaking both a scheduler thread and the in-memory HNSW graph/vector store. It introduces GC-driven executor self-termination via weak-referenced callbacks, adds a storage-shutdown release hook to defensively close index groups, and fixes a view-close leak on an exception path in disk writing.
Changes:
- Switch
BackgroundTaskManagerto hold the index callback viaWeakReferenceand add a liveness watchdog that self-terminates the executor when the index is GC’d. - Invoke
PersistenceShutdownReleasable.releaseOnShutdown()for already-materialized instances duringEmbeddedStorageManager.shutdown(), withGigaMapclosing allCloseableindex groups as part of shutdown participation. - Wrap
DiskIndexManager.writeIndexWithFusedPQ’sindex.getView()usage in try-with-resources to avoid leaking the heap-only view on exceptions; add regression tests for abandonment/thread cleanup and storage shutdown.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| storage/embedded/src/main/java/org/eclipse/store/storage/embedded/types/EmbeddedStorageManager.java | Enumerates the object registry on shutdown and invokes releaseOnShutdown() on opt-in instances. |
| gigamap/jvector/src/main/java/org/eclipse/store/gigamap/jvector/BackgroundTaskManager.java | Uses weak callback reference + periodic watchdog to self-terminate executor after index GC; cancels watchdog during shutdown. |
| gigamap/jvector/src/main/java/org/eclipse/store/gigamap/jvector/DiskIndexManager.java | Ensures index.getView() is always closed via try-with-resources around the writer path. |
| gigamap/jvector/src/test/java/org/eclipse/store/gigamap/jvector/VectorIndexAbandonmentLeakTest.java | Adds regression coverage for index GC collectability, background thread reclamation, and shutdown-triggered group closing. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java | Implements shutdown participation by closing all index groups on storage shutdown. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaIndices.java | Adds closeAllGroups() to close Closeable groups without mutating persistent state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…down deadlock Closing a vector index acquires the builder write-lock, while a background persist holds that write-lock and then waits for the GigaMap parentMap monitor (doPersistToDisk: builderLock then synchronized(parentMap)). The shutdown/close path did the opposite — it held the parentMap monitor across index.close() — so an on-disk background index with unsaved changes and the default persistOnShutdown=true dead-locked permanently on storage.shutdown(): the shutdown thread held the monitor and waited for the write-lock while the executor thread held the write-lock and waited for the monitor. The fix collects the closeables under the monitor and closes them outside of it, so no application lock is held across close(). Applied to releaseShutdownParticipants (release participants after the object-registry iteration), GigaIndices.closeAllGroups and VectorIndices.close, and to the pre-existing sibling VectorIndices.removeIndex, which had the same monitor-across-close pattern. The original PR regression test only used an in-memory index, where doPersistToDisk no-ops on !onDisk() and the deadlock cannot occur. Adds an on-disk, background-persistence regression test that reproduces the deadlock; it uses assertTimeoutPreemptively because a hard monitor deadlock cannot be interrupted, so a plain @timeout would hang instead of failing. Verified: the test hangs against the unfixed code and passes with the fix; the existing lifecycle (removeIndex, group removal) and on-disk suites stay green.
zdenek-jonas
approved these changes
Jul 20, 2026
…l, and make removeIndex atomic Extends the shutdown deadlock fix to the two remaining close-under-monitor paths flagged in review. GigaIndices.internalRemoveGroup closed the group while holding the parentMap monitor, so removing a vector index group (index().remove(category)) could dead-lock against a background persist exactly like storage shutdown did. It now finds and validates the group under the monitor, closes it outside, and removes it from the registry under the monitor only after close() succeeds. VectorIndices.removeIndex previously dropped the index from the registry before closing it, so a throwing close() left the index detached but not reliably closed. It now closes outside the monitor first and removes (and marks the state change) only on success, keeping the registry consistent on failure; close() is idempotent, so a still-registered index is closed again on the next shutdown or removal. Adds on-disk, background-persistence regression tests for removeIndex and whole-group removal, mirroring the shutdown deadlock test (assertTimeoutPreemptively, since a hard monitor deadlock cannot be interrupted). Also drops the private issue-tracker references from the comments and test messages of this change.
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.
Summary
A GigaMap
VectorIndexwith any background feature enabled (eventual indexing, background optimization, background persistence) owns a daemonScheduledExecutorServicethread through aBackgroundTaskManager. Two problems followed from that:close()was pinned forever — every abandon-and-reopen cycle leaked one scheduler thread plus the whole in-memory HNSW graph and vector store.parentMapmonitor acrossindex.close(), which acquires the builder write-lock, while a background persist holds that write-lock and then waits for theparentMapmonitor (doPersistToDisk). For an on-disk background index with unsaved changes and the defaultpersistOnShutdown=true,storage.shutdown()(and group/index removal) could dead-lock permanently.Changes
Reclaim abandoned indices (weak callback + self-termination).
BackgroundTaskManagernow holds the index through aWeakReference, and a lightweight liveness watchdog self-terminates the executor once the index has been garbage-collected, so a dropped index becomes fully reclaimable (thread + graph) even with no storage in play. Self-termination runs on the executor thread and therefore does not await termination. This mirrors the self-cancelling weak-reference pattern already used by the cache module'sEvictionManager.Close index groups on storage shutdown (defense in depth).
EmbeddedStorageManager.shutdown()closes the index groups of every loaded GigaMap, so background threads stop and auxiliary resources are freed even when the caller forgets to close indices explicitly. Discovery enumerates the object registry (already-loaded instances only, so noLazysubgraph is force-loaded) and invokes the opt-inPersistenceShutdownReleasablecontract;GigaMapimplements it and delegates toGigaIndices.closeAllGroups(), which closesCloseablegroups without mutating the persistent graph. The three mechanisms compose as layered defenses: explicitclose()/removeIndex, storage shutdown, and GC-driven self-termination.Never close under the parentMap monitor (deadlock fix). The shutdown release pass,
GigaIndices.closeAllGroups,GigaIndices.internalRemoveGroup(whole-group removal) andVectorIndices.close/removeIndexnow collect the closeables under the monitor and close them outside of it, so no application lock is held acrossclose().removeIndexandinternalRemoveGroupadditionally close first and drop the entry from the registry only afterclose()succeeds, keeping the registry consistent if a close throws (close()is idempotent, so a still-registered index is closed again on the next shutdown/removal).Sibling view leak.
DiskIndexManager.writeIndexWithFusedPQacquiredindex.getView()and closed it with a trailing bareview.close(); a throw in between leaked the heap-only view. It is now acquired in try-with-resources.Tests
VectorIndexAbandonmentLeakTestcovers: index GC-collectability (with a no-background control), background-thread reclamation, storage-shutdown group closing, and — on an on-disk background index with unsaved changes — thatstorage.shutdown(),removeIndexand whole-group removal do not dead-lock. The deadlock cases useassertTimeoutPreemptively, since a hard monitor deadlock cannot be interrupted and a plain@Timeoutwould hang. Each case was verified to fail (or hang) against the unpatched code and pass with the fix; the existing lifecycle and on-disk suites stay green.Dependency
Consumes the
PersistenceShutdownReleasableinterface added in eclipse-serializer (already merged).