Fix: concurrent add() no longer strands unreachable nodes (#735) - #772
Open
cpegeric wants to merge 1 commit into
Open
Fix: concurrent add() no longer strands unreachable nodes (#735)#772cpegeric wants to merge 1 commit into
cpegeric wants to merge 1 commit into
Conversation
Contributor
Author
|
see issue #735 |
This was referenced Jun 25, 2026
…#735) Concurrent add() could leave nodes that are stored (contains()==true) yet unreachable via search(): contained-but-orphaned. Root cause is the per-node lock being released between levels while a node builds its links top-down. A node S that had built its level-1 links but not yet its level-0 links was already discoverable (via its level-1 reverse links) while its level-0 neighbor list was still empty. A concurrent inserter greedily descending onto S as its level-0 seed dead-ended the level search with a single candidate, attached by one fragile edge, and was then evicted by the neighbor heuristic into a permanently unreachable node. The old `neighbors_(new_node,level).clear()` also actively discarded reverse links that concurrent inserters had appended during the gap. Fix, in index_gt::add_(): - Split the single per-level loop into two passes: build ALL of the node's forward links (every level) BEFORE adding ANY reverse links. Reverse links are what make a node discoverable, so deferring them guarantees the node is never reachable as a descent seed while one of its lower levels is empty. - form_links_to_closest_ no longer asserts/requires a blank list: it preserves any concurrently-added reverse links and appends the forward links, deduped and bounded by the level capacity, instead of clear()+push_back. - The reverse-link pass snapshots the node's neighbors into a reused per-thread context buffer (buffer_gt with the null-returning candidates_allocator_t), so it adds no per-add heap allocation and reports OOM via result.failed(...) rather than throwing — add_ is noexcept in release builds (NDEBUG), where a throw would std::terminate. Single-threaded behavior is unchanged (the list is empty, so the merge path is a no-op). Adds test_concurrent_add_reachability (cpp/test.cpp): builds an index from many threads and asserts every node is reachable. The pre-existing concurrent test only ran single-threaded, so this gap was uncovered. Validated: issue repro and stress (8000 vec x 16 threads) drop from a handful of orphans per run to zero, in both debug and NDEBUG/noexcept builds; full cpp/test.cpp suite passes; index size and recall unchanged vs single-threaded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mergify Bot
pushed a commit
to matrixorigin/matrixone
that referenced
this pull request
Jun 26, 2026
) patch the fix with usearch PR: unum-cloud/USearch#772 add unit test to test the same bvt case flaky test to make sure no orphan in final index. revert the change build thread = 1 Approved by: @fengttt, @heni02, @XuPeng-SH
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.
Concurrent add() could leave nodes that are stored (contains()==true) yet unreachable via search(): contained-but-orphaned. Root cause is the per-node lock being released between levels while a node builds its links top-down.
A node S that had built its level-1 links but not yet its level-0 links was already discoverable (via its level-1 reverse links) while its level-0 neighbor list was still empty. A concurrent inserter greedily descending onto S as its level-0 seed dead-ended the level search with a single candidate, attached by one fragile edge, and was then evicted by the neighbor heuristic into a permanently unreachable node. The old
neighbors_(new_node,level).clear()also actively discarded reverse links that concurrent inserters had appended to the node during the between-levels gap.Fix, in index_gt::add_():
Single-threaded behavior is unchanged (the list is empty, so the merge path is a no-op). Adds for the reverse-link snapshot buffer.
Adds test_concurrent_add_reachability (cpp/test.cpp): builds an index from many threads and asserts every node is reachable. The pre-existing concurrent test only ran single-threaded, so this gap was uncovered.
Validation
equal recall.
true-orphan (zero-in-edge) count of 1,098/1M concurrent vs 1,123/1M single-threaded, identical recall (~91%). That
residual ~0.11% is present identically in a serial build — it's the neighbor-selection heuristic occasionally evicting
a node's last in-edge at scale, not this race. Concurrent builds were ~5× faster at equal quality.
Notes
This does not touch the update_ path (which intentionally clears and rebuilds), and it does not address the small
algorithmic zero-in-edge fraction at low connectivity / large N, which is independent of threading.