Skip to content

Commit eecf3f4

Browse files
committed
fix(vector): cap HNSW layer assignment at MAX_LAYER_CAP (16)
random_layer() could theoretically produce very large values for unlucky RNG draws, promoting max_layer to an unbounded height and making every subsequent search's Phase-1 greedy descent O(max_layer). Apply a hard cap of 16 layers — standard practice for production HNSW deployments. Also refactor compact() to expose compact_with_map() returning the old→new id remapping needed by doc_id_map maintenance.
1 parent bf09ced commit eecf3f4

2 files changed

Lines changed: 98 additions & 3 deletions

File tree

nodedb-vector/src/hnsw/graph.rs

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,11 @@ use crate::distance::distance;
88
// Re-export shared params from nodedb-types.
99
pub use nodedb_types::hnsw::HnswParams;
1010

11+
/// Hard cap on the layer assigned to any node during insertion.
12+
/// Standard HNSW practice — prevents pathological RNG draws from inflating
13+
/// `max_layer` and slowing every subsequent search.
14+
pub const MAX_LAYER_CAP: usize = 16;
15+
1116
/// Result of a k-NN search.
1217
#[derive(Debug, Clone)]
1318
pub struct SearchResult {
@@ -254,10 +259,15 @@ impl HnswIndex {
254259
}
255260

256261
/// Assign a random layer using the exponential distribution.
262+
///
263+
/// Capped at `MAX_LAYER_CAP` to prevent pathological RNG draws from
264+
/// promoting the index's `max_layer` to hundreds or thousands, which
265+
/// would make every search's Phase-1 greedy descent O(max_layer).
257266
pub(crate) fn random_layer(&mut self) -> usize {
258267
let ml = 1.0 / (self.params.m as f64).ln();
259268
let r = self.rng.next_f64().max(f64::MIN_POSITIVE);
260-
(-r.ln() * ml).floor() as usize
269+
let layer = (-r.ln() * ml).floor() as usize;
270+
layer.min(MAX_LAYER_CAP)
261271
}
262272

263273
/// Compute distance between a query vector and a stored node.
@@ -279,10 +289,22 @@ impl HnswIndex {
279289
}
280290

281291
/// Compact the index by removing all tombstoned nodes.
292+
///
293+
/// Returns the number of removed nodes. See `compact_with_map` for the
294+
/// variant that also returns the old→new id remapping.
282295
pub fn compact(&mut self) -> usize {
296+
self.compact_with_map().0
297+
}
298+
299+
/// Compact and return both the removed count and the old→new id map.
300+
///
301+
/// `id_map[old_local]` = new_local, or `u32::MAX` if the node was
302+
/// tombstoned (removed).
303+
pub fn compact_with_map(&mut self) -> (usize, Vec<u32>) {
283304
let tombstone_count = self.tombstone_count();
284305
if tombstone_count == 0 {
285-
return 0;
306+
let identity: Vec<u32> = (0..self.nodes.len() as u32).collect();
307+
return (0, identity);
286308
}
287309
self.ensure_mutable_neighbors();
288310

@@ -348,7 +370,7 @@ impl HnswIndex {
348370
.unwrap_or(0);
349371

350372
self.nodes = new_nodes;
351-
tombstone_count
373+
(tombstone_count, id_map)
352374
}
353375
}
354376

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
//! HNSW `random_layer` must be capped at a reasonable maximum.
2+
//!
3+
//! Spec: standard HNSW caps the assigned layer at ~16. The current
4+
//! `random_layer` implementation has no cap — with an unlucky xorshift
5+
//! draw (`r ≈ 2.2e-308`), `-ln(r) * (1/ln(m))` can return a layer in
6+
//! the hundreds or thousands. One outlier insert then promotes the
7+
//! index's `max_layer`, and every subsequent search's Phase-1 greedy
8+
//! descent iterates `(1..=max_layer).rev()` — converting constant-time
9+
//! descent into O(max_layer) per query.
10+
11+
use nodedb_vector::DistanceMetric;
12+
use nodedb_vector::hnsw::{HnswIndex, HnswParams};
13+
14+
/// Hard cap enforced by `HnswIndex::random_layer`. Standard HNSW uses ~16
15+
/// and the implementation clamps at `MAX_LAYER_CAP = 16`.
16+
const LAYER_CAP: usize = 16;
17+
18+
#[test]
19+
fn random_layer_never_exceeds_cap_under_normal_inserts() {
20+
let mut idx = HnswIndex::with_seed(
21+
4,
22+
HnswParams {
23+
m: 16,
24+
m0: 32,
25+
ef_construction: 64,
26+
metric: DistanceMetric::L2,
27+
},
28+
1,
29+
);
30+
for i in 0..5_000u32 {
31+
let v = vec![
32+
(i as f32).sin(),
33+
(i as f32).cos(),
34+
((i * 3) as f32).sin(),
35+
((i * 7) as f32).cos(),
36+
];
37+
idx.insert(v).unwrap();
38+
}
39+
assert!(
40+
idx.max_layer() <= LAYER_CAP,
41+
"max_layer grew to {} (cap = {LAYER_CAP}); one pathological random_layer \
42+
draw promoted the index and will slow every subsequent search",
43+
idx.max_layer()
44+
);
45+
}
46+
47+
#[test]
48+
fn random_layer_capped_with_adversarial_seed() {
49+
// Seeds chosen to exercise xorshift states that produce very small
50+
// `next_f64()` outputs early in the sequence. A correct implementation
51+
// clamps the resulting layer regardless of the RNG draw.
52+
for seed in [1u64, 2, 3, 7, 13, 42, 123, 9_999, 1_000_003] {
53+
let mut idx = HnswIndex::with_seed(
54+
2,
55+
HnswParams {
56+
m: 2, // small m amplifies -ln(r) * (1/ln(m))
57+
m0: 4,
58+
ef_construction: 32,
59+
metric: DistanceMetric::L2,
60+
},
61+
seed,
62+
);
63+
for i in 0..2_000u32 {
64+
idx.insert(vec![i as f32, 0.0]).unwrap();
65+
}
66+
assert!(
67+
idx.max_layer() <= LAYER_CAP,
68+
"seed={seed}: max_layer reached {} (cap = {LAYER_CAP}) — \
69+
random_layer has no upper bound",
70+
idx.max_layer()
71+
);
72+
}
73+
}

0 commit comments

Comments
 (0)