lib/segment/src/index/hnsw_index/hnsw.rs (1)

1402-1478: 💡 Verification agent

🧩 Analysis chain

Consider tolerant vector comparison to reduce false-negative reuse rate

old_vector == new_vector performs an exact floating-point comparison.
Even when a vector has not semantically changed, different storage
back-ends (e.g. serialization, quantization-round-trip, SIMD alignment)
may re-emit the very same numbers with minor binary drift, causing
values that are numerically equal to fail an == test.
As soon as that happens the point is treated as “missing”, forcing the
healing path and potentially discarding a large portion of an otherwise
good graph.

A cheap alternative is to fall back to an approximate check that is
already available in the code base (e.g. cosine/L2 similarity ≥
1-1e-6).

- if old_vector == new_vector {
+ // Treat vectors as equal if the distance is below a tiny epsilon
+ if metric::cosine::distance(&old_vector, &new_vector) <= 1e-6 {

This keeps the incremental-build fast path effective while still
catching truly modified vectors.

---

🏁 Script executed:

#!/bin/bash
# Search for metric module and cosine/distance functions
rg -n "mod metric" .
rg -n "cosine" .
rg -n "distance(" .

# Find definition and usage of get_vector to inspect return type
rg -n "fn get_vector" .
rg -n "get_vector(" .

Length of output: 32005

---

🏁 Script executed:

#!/bin/bash
# List files in the metric module
echo "=== Metric module files ==="
ls lib/segment/src/spaces/metric

# Search for any distance function definitions in metric module
echo -e "\n=== Definitions of fn distance ==="
rg -n "fn distance" lib/segment/src/spaces/metric

# Inspect the top of mod.rs to see submodules
echo -e "\n=== lib/segment/src/spaces/metric/mod.rs (first 200 lines) ==="
sed -n '1,200p' lib/segment/src/spaces/metric/mod.rs

Length of output: 657

---

🏁 Script executed:

#!/bin/bash
# 1. List the contents of the spaces directory to locate the metric module
echo "=== lib/segment/src/spaces directory ==="
ls -R lib/segment/src/spaces

# 2. Search for any definitions of `fn distance` across Rust files
echo -e "\n=== Definitions of `fn distance` ==="
rg -n 'fn distance' -g '*.rs'

# 3. Search for invocations of `distance(` to see how it’s called
echo -e "\n=== Usages of `distance(` ==="
rg -n 'distance\s*\(' -g '*.rs'

Length of output: 14578

---

🏁 Script executed:

#!/bin/bash
# Show the contents of the metric module file to find how distance functions are exposed
echo "=== lib/segment/src/spaces/metric.rs ==="
sed -n '1,200p' lib/segment/src/spaces/metric.rs

# Locate the Distance enum declaration
echo -e "\n=== enum Distance in lib/segment/src/types.rs ==="
rg -n "enum Distance" -g lib/segment/src/types.rs

# Locate the impl block for Distance to see its method signature
echo -e "\n=== impl Distance in lib/segment/src/types.rs ==="
rg -n "impl Distance" -g lib/segment/src/types.rs

Length of output: 1303

---

Use an approximate‐equality check instead of == for floating-point vectors

Exact old_vector == new_vector will drop points whose values only differ by tiny fp noise. To avoid spuriously entering the healing path, replace it with an element-wise epsilon test. For example, in lib/segment/src/index/hnsw_index/hnsw.rs around line 1435:

-    if old_vector == new_vector {
+    // Treat vectors as equal if all components differ by at most EPS
+    const EPS: f32 = 1e-6;
+    if old_vector.len() == new_vector.len()
+        && old_vector
+            .iter()
+            .zip(new_vector.iter())
+            .all(|(a, b)| (a - b).abs() <= EPS)
    {
         old_to_new[old_offset as usize] = Some(new_offset);
         valid_points += 1;

This simple check avoids pulling in non-existent metric::cosine::distance functions and keeps the fast path intact while only rejecting truly changed vectors.

lib/segment/src/index/hnsw_index/hnsw.rs (1)

386-407: Possible data race between concurrent healing tasks

to_heal.into_par_iter().try_for_each(|old_offset| { old_index.migrate_point(old_offset, &graph_layers_builder) })

migrate_point mutates shared state inside graph_layers_builder
(ready_list, link containers, entry-points). Internally the builder
relies on RwLock/Mutex for safety, so the code compiles, but:

• every healed point writes the global ready_list; with par_iter
  there will be heavy cross-thread contention on the same lock,
  significantly degrading throughput for large batches;
• a thread may call search_shortcuts_on_level before another thread
  marks a neighbour as “ready”, therefore exploring a slower “old-graph”
  path and producing sub-optimal shortcuts.

If practical, heal sequentially per level or switch to a two-phase
approach:

1. build a local Vec<(point_id, links)> in parallel;
2. merge results into the builder in a single threaded pass.

That removes lock contention and eliminates the ready-list timing race.

lib/segment/src/index/hnsw_index/graph_layers_builder.rs (1)

497-608: Heavy lock usage & per-call allocations may hurt healing performance

search_shortcuts_on_level is invoked for every healed point and:

1. Acquires ready_list.read() on every neighbour inspection,
   potentially thousands of times per call.
2. Allocates new Vecs (old_offsets, scores_buffer, pending)
   for each call, although their maximum length is bounded by 2 * M.

Under large update batches this becomes a hot path.

Recommended micro-optimisations:

- let mut old_offsets: Vec<PointOffsetType> = Vec::with_capacity(2 * limit);
- let mut scores_buffer = Vec::with_capacity(limit);
+ // Re-use small vec stacks on the caller thread to avoid heap traffic
+ let mut old_offsets = smallvec::SmallVec::<[PointOffsetType; 32]>::new();
+ let mut scores_buffer = smallvec::SmallVec::<[f32; 32]>::new();

- let mut pending = Vec::new();
+ let mut pending = VecDeque::with_capacity(limit);

and cache the ready_list guard once per call:

let ready = self.ready_list.read();
...
if ready[new_offset as usize] { ... }

This removes a large chunk of lock acquisitions and allocations without
changing semantics.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/segment/src/index/hnsw_index/hnsw.rs (1)

1621-1623: ⚠️ Potential issue

unwrap() on internal mapping can panic in inconsistent states

new_scorer assumes that every shortcut returned by search_shortcuts_on_level has a valid back-mapping:

let a_old = self.new_to_old[a as usize].unwrap();

While the surrounding logic should guarantee this, any future change in search_shortcuts_on_level or misuse of LinksContainer could break the invariant and bring the whole build down with an unexpected panic.

A safer pattern is to propagate the error explicitly:

let new_scorer = |a, b| {
-    let a_old = self.new_to_old[a as usize].unwrap();
-    let b_old = self.new_to_old[b as usize].unwrap();
-    old_scorer.score_internal(a_old, b_old)
+    match (self.new_to_old[a as usize], self.new_to_old[b as usize]) {
+        (Some(a_old), Some(b_old)) => old_scorer.score_internal(a_old, b_old),
+        _ => {
+            // This should not happen, but handle it gracefully
+            debug_assert!(false, "Missing mapping in new_scorer");
+            0.0 // Return worst possible score for invalid mappings
+        }
+    }
};

lib/segment/src/index/hnsw_index/hnsw.rs (2)

1496-1506: This section requires a comment

This block identifies points that once existed in the old index but were deleted in its tracker, marking them as missing points for healing purposes.

Add a more detailed comment explaining this loop's purpose:

        // Loop 1: use both id trackers to find matching external ids.
        for item in itertools::merge_join_by(
            id_tracker.iter_from(None),
            old_id_tracker.iter_from(None),
            |(new_external_id, _), (old_external_id, _)| new_external_id.cmp(old_external_id),
        ) {
-        // Loop 2: find points that once were in the old index, but deleted in
-        // its id tracker.
+        // Loop 2: Find points that were deleted in the old index's tracker
+        // but still exist in the graph structure. These points need healing
+        // since they have connections in the graph but no valid vectors.
        for old_offset in 0..old_to_new.len() {
            let old_offset = old_offset as PointOffsetType;
            if old_id_tracker.is_deleted_point(old_offset) && old_graph_has_point(old_offset) {
                missing_points += 1;
                if feature_flags.hnsw_healing {
                    return None;
                }
            }
        }

---

1593-1630: This requires comment

The links_on_level method is complex and would benefit from a clear explanation of how it identifies and fixes links when migrating points from an old index.

Add a detailed comment explaining the method's purpose and approach:

+    /// Reconstructs links for a point on a specific level during migration.
+    /// 
+    /// This method performs two key operations:
+    /// 1. Preserves valid links that point to points still existing in the new index
+    /// 2. For links pointing to deleted/missing points, finds alternative shortcuts
+    ///    using a specialized graph search to maintain connectivity
    fn links_on_level(
        &self,
        src_old: PointOffsetType,
        level: usize,
        builder: &GraphLayersBuilder,
        old_scorer: &dyn RawScorer,
    ) -> Vec<PointOffsetType> {
        let links = &self.index.graph.links;

        let level_m = if level == 0 { self.m0 } else { self.m };
        let mut new_links = Vec::with_capacity(level_m);
        let mut need_fixing = 0;

        for link in links.links(src_old, level).take(level_m) {
            if let Some(new_link) = self.old_to_new[link as usize] {
                new_links.push(new_link);
            } else {
                need_fixing += 1;
            }
        }

        if need_fixing > 0 {
            let shortcuts = builder
                .search_shortcuts_on_level(src_old, level, old_scorer, self)
                .into_iter_sorted();
            let mut container = LinksContainer::with_capacity(level_m);
            let new_scorer = |a, b| {
                let a_old = self.new_to_old[a as usize].unwrap();
                let b_old = self.new_to_old[b as usize].unwrap();
                old_scorer.score_internal(a_old, b_old)
            };
            container.fill_from_sorted_with_heuristic(shortcuts, need_fixing, new_scorer);
            new_links.extend_from_slice(container.links());
        }

        new_links
    }

lib/segment/src/index/hnsw_index/graph_layers_builder.rs (1)

526-612: Comprehensive implementation of shortcut search

The search_shortcuts_on_level method effectively implements a specialized search that can traverse deleted points to find valid shortcuts. The implementation is thorough and covers edge cases well.

There are a few debug assertions that could be improved with more informative error messages to help diagnose issues in development.

Add more descriptive messages to debug assertions:

debug_assert!(
    false,
-    "The new graph points expected to be a subset of the old graph points"
+    "Found a point in new graph that doesn't exist in old graph. Point ID: {new_link}"
);

debug_assert!(
    offset_new_ready.is_none(),
-    "For ready points, every neighbor expected to be in the new graph"
+    "Found a ready point with neighbors missing from new graph. Point ID: {candidate_old.idx}"
);

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro