Skip to content

Fix: concurrent add() no longer strands unreachable nodes (#735) - #772

Open
cpegeric wants to merge 1 commit into
unum-cloud:mainfrom
cpegeric:concurrent_add
Open

Fix: concurrent add() no longer strands unreachable nodes (#735)#772
cpegeric wants to merge 1 commit into
unum-cloud:mainfrom
cpegeric:concurrent_add

Conversation

@cpegeric

Copy link
Copy Markdown
Contributor

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_():

  • 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. Removing the destructive clear() also stops dropping valid concurrent edges.

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

  • Issue repro + stress (8,000 vectors × 16 threads × 8 iterations): orphans drop from a handful per run to zero.
  • Full cpp/test.cpp suite passes, no regressions.
  • Index quality unchanged. Same data, single-threaded vs concurrent build: equal size (+0.01%), equal average degree,
    equal recall.
  • 1M scale (wiki_all, 768-dim, M=16): a concurrent build is now statistically indistinguishable from a serial one —
    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.

@cpegeric

Copy link
Copy Markdown
Contributor Author

see issue #735

…#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant