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

cpp/test.cpp

@@ -709,6 +709,74 @@ void test_punned_concurrent_updates(index_at& index, typename index_at::vector_k
     expect_eq(index.size(), vectors.size());
 }
 
+/**
+ * Regression test for USearch #735: concurrent `add()` must never leave a node
+ * stored-but-unreachable. The bug: while a node was building links top-down, its
+ * per-level lock was released between levels, so a concurrent inserter could
+ * descend onto it before its lower level had any links, dead-end the level search,
+ * and end up attached by a single fragile edge that was then evicted — leaving a
+ * node for which `contains()` is true but `search()` can never reach it.
+ *
+ * This builds an index from many threads, then asserts every inserted vector finds
+ * itself in an exhaustive search. A single orphaned node fails the test. Before the
+ * fix this reproduced a handful of unreachable nodes per run at a few thousand
+ * vectors; with serialized (single-thread) builds it always passed — so the test is
+ * only meaningful with `threads_count > 1`.
+ *
+ * @param dimensions Number of dimensions per vector.
+ * @param vectors_per_thread Vectors inserted by each worker thread.
+ * @param threads_count Number of concurrent insertion threads (must be > 1 to bite).
+ * @param connectivity Graph connectivity (M) for the index.
+ */
+template <typename key_at, typename slot_at>
+void test_concurrent_add_reachability(std::size_t dimensions, std::size_t vectors_per_thread,
+                                      std::size_t threads_count, std::size_t connectivity) {
+
+    using index_t = index_dense_gt<key_at, slot_at>;
+    std::size_t const total = vectors_per_thread * threads_count;
+
+    // Deterministic pseudo-random vectors, so a failure is reproducible.
+    std::vector<std::vector<float>> vectors(total);
+    std::default_random_engine generator(42);
+    std::uniform_real_distribution<float> distribution(-1.0f, 1.0f);
+    for (auto& vector : vectors) {
+        vector.resize(dimensions);
+        for (auto& component : vector)
+            component = distribution(generator);
+    }
+
+    metric_punned_t metric(dimensions, metric_kind_t::cos_k, scalar_kind_t::f32_k);
+    index_dense_config_t config(connectivity);
+    typename index_t::state_result_t make_result = index_t::make(metric, config);
+    expect(make_result);
+    index_t& index = make_result.index;
+
+    // Concurrent build — the condition that triggers #735.
+    executor_default_t executor(threads_count);
+    expect(index.try_reserve({total, executor.size()}));
+    executor.fixed(total, [&](std::size_t thread, std::size_t task) {
+        typename index_t::add_result_t result = index.add(static_cast<key_at>(task), vectors[task].data(), thread);
+        expect(result);
+    });
+    expect_eq(index.size(), total);
+
+    // Every stored vector must be reachable: searching for it (requesting every
+    // result) must return itself. An orphaned node is contained but unreachable.
+    std::size_t unreachable = 0;
+    std::vector<key_at> keys(total);
+    std::vector<distance_punned_t> distances(total);
+    for (std::size_t i = 0; i != total; ++i) {
+        typename index_t::search_result_t search_result = index.search(vectors[i].data(), total);
+        expect(search_result);
+        std::size_t found = search_result.dump_to(keys.data(), distances.data());
+        bool found_self = false;
+        for (std::size_t r = 0; r != found && !found_self; ++r)
+            found_self = keys[r] == static_cast<key_at>(i);
+        unreachable += !found_self;
+    }
+    expect_eq(unreachable, static_cast<std::size_t>(0));
+}
+
 /**
  * Overloaded function to test cosine similarity index functionality using specific scalar, key, and slot types.
  *
@@ -1453,5 +1521,17 @@ int main(int, char**) {
     test_filtered_search();
     test_isolate();
     test_load_after_metric_make();
+
+    // Regression for USearch #735: concurrent builds must not strand nodes as
+    // contained-but-unreachable. Only meaningful with more than one thread, and at
+    // a realistic connectivity — at a degenerate connectivity (<= 3) HNSW orphans
+    // nodes even single-threaded, which is an algorithmic property, not this race.
+    std::printf("Testing concurrent-add reachability (#735)\n");
+    if (std::thread::hardware_concurrency() > 1)
+        for (std::size_t threads : {static_cast<std::size_t>(4),
+                                    static_cast<std::size_t>(std::thread::hardware_concurrency())}) {
+            test_concurrent_add_reachability<std::int64_t, slot32_t>(64, 250, threads, 16);
+            test_concurrent_add_reachability<std::int64_t, uint40_t>(64, 250, threads, 16);
+        }
     return 0;
 }

include/usearch/index.hpp

@@ -2460,6 +2460,9 @@ class index_gt {
         top_candidates_t top_for_refine{};
         next_candidates_t next_candidates{};
         visits_hash_set_t visits{};
+        /// @brief Reused snapshot of a new node's forward neighbors for the
+        /// reverse-link pass in `add_` (#735), so it needs no per-add allocation.
+        buffer_gt<candidate_t, candidates_allocator_t> reverse_neighbors{};
         std::default_random_engine level_generator{};
         std::size_t iteration_cycles{};
         std::size_t computed_distances{};
@@ -3228,18 +3231,55 @@ class index_gt {
             value, metric, prefetch,                      //
             entry_slot_copy, max_level_copy, new_target_level, context);
 
-        // From `new_target_level` down - perform proper extensive search
-        for (level_t level = (std::min)(new_target_level, max_level_copy); level >= 0; --level) {
+        // #735: Form ALL of this node's forward links (every level) BEFORE adding
+        // any reverse links. Reverse links are what make a node discoverable by
+        // concurrent searches; deferring them to a second pass guarantees this node
+        // is never reachable as a greedy-descent seed while one of its lower levels
+        // still has an empty neighbor list. Otherwise a concurrent inserter could
+        // descend onto this half-linked node, dead-end its level search with a single
+        // candidate, attach by one fragile edge, and be evicted into a permanently
+        // unreachable node — the root of #735.
+        level_t const top_level = (std::min)(new_target_level, max_level_copy);
+
+        // Pass 1: forward links, top-down (each level seeds the next from its closest).
+        for (level_t level = top_level; level >= 0; --level) {
             // TODO: Handle out of memory conditions
             search_to_insert_(value, metric, prefetch, closest_slot, level, config.expansion, context);
-            candidates_view_t closest_view;
+            node_lock_t new_lock = node_lock_(new_slot);
+            // Do NOT clear() the list — concurrent inserters may already have appended
+            // valid reverse links here; form_links_to_closest_ preserves and extends them.
+            candidates_view_t closest_view = form_links_to_closest_(metric, new_slot, level, context);
+            if (closest_view.size())
+                closest_slot = closest_view[0].slot;
+        }
+
+        // Pass 2: reverse links, for every level. Snapshot this node's forward
+        // neighbors under its lock, release, then link each neighbor back to it.
+        // (form_reverse_links_ locks the *neighbor* nodes, so we must not hold this
+        // node's lock across it — see the consistent lock ordering in CLAUDE.md.)
+        //
+        // The snapshot lives in a per-thread context buffer (reused across adds, not
+        // a per-add heap allocation), and is sized via the null-returning allocator
+        // so OOM yields `result.failed("Out of memory!")` instead of throwing — add_
+        // is noexcept in release builds (NDEBUG), where a throw would std::terminate.
+        std::size_t const max_neighbors = (std::max)(config_.connectivity, config_.connectivity_base);
+        if (context.reverse_neighbors.size() < max_neighbors) {
+            buffer_gt<candidate_t, candidates_allocator_t> grown(max_neighbors);
+            if (!grown)
+                return result.failed("Out of memory!");
+            context.reverse_neighbors = std::move(grown);
+        }
+        for (level_t level = top_level; level >= 0; --level) {
+            std::size_t n = 0;
             {
                 node_lock_t new_lock = node_lock_(new_slot);
-                neighbors_(new_node, level).clear();
-                closest_view = form_links_to_closest_(metric, new_slot, level, context);
-                closest_slot = closest_view[0].slot;
+                neighbors_ref_t new_neighbors = neighbors_(new_node, level);
+                n = new_neighbors.size(); // <= per-level capacity <= max_neighbors
+                for (std::size_t i = 0; i != n; ++i)
+                    context.reverse_neighbors[i] = candidate_t{distance_t{}, new_neighbors[i]};
             }
-            form_reverse_links_(metric, new_slot, closest_view, value, level, context);
+            form_reverse_links_(metric, new_slot, candidates_view_t{context.reverse_neighbors.data(), n}, value, level,
+                                context);
         }
 
         // Normalize stats
@@ -4269,13 +4309,32 @@ class index_gt {
             refine_(metric, config_.connectivity, top, context, context.computed_distances_in_refines);
         usearch_assert_m(top_view.size() || !require_non_empty_ak, "This would lead to isolated nodes");
 
-        // Outgoing links from `new_slot`:
+        // Outgoing links from `new_slot`.
+        //
+        // #735: the list is NOT necessarily blank here. Between forming this node's
+        // upper-level links and this level's links, the node's per-level lock is
+        // released (the level loop re-acquires `node_lock_(new_slot)` each level).
+        // A concurrent inserter that picked this node as its greedy-descent seed
+        // (`search_for_one_` returns it from an upper level) can append reverse
+        // links into this still-empty lower-level list. Clearing it — as the old
+        // code did at the call site — would silently drop those edges and strand
+        // that inserter as a permanently unreachable node. So we PRESERVE whatever
+        // is already here and append our forward links, deduped, bounded by the
+        // level's capacity. Single-threaded the list is empty and this is identical
+        // to the previous behavior.
         neighbors_ref_t new_neighbors = neighbors_(new_node, level);
-        usearch_assert_m(!new_neighbors.size(), "The newly inserted element should have blank link list");
+        std::size_t const connectivity_max = level ? config_.connectivity : config_.connectivity_base;
         for (std::size_t idx = 0; idx != top_view.size(); idx++) {
-            usearch_assert_m(!new_neighbors[idx], "Possible memory corruption");
-            usearch_assert_m(level <= node_at_(top_view[idx].slot).level(), "Linking to missing level");
-            new_neighbors.push_back(top_view[idx].slot);
+            compressed_slot_t forward_slot = top_view[idx].slot;
+            usearch_assert_m(level <= node_at_(forward_slot).level(), "Linking to missing level");
+            if (new_neighbors.size() >= connectivity_max)
+                break;
+            // Skip a forward candidate already present as a concurrently-added reverse link.
+            if (std::find_if(new_neighbors.begin(), new_neighbors.end(), [forward_slot](compressed_slot_t slot) {
+                    return slot == forward_slot;
+                }) != new_neighbors.end())
+                continue;
+            new_neighbors.push_back(forward_slot);
         }
 
         return top_view;