From 6ccbf82677bd2ff882550c389107f8c22130f7f9 Mon Sep 17 00:00:00 2001 From: OceanLi <122793010+ohdearquant@users.noreply.github.com> Date: Mon, 3 Aug 2026 13:25:59 -0400 Subject: [PATCH 1/2] fix(delta-index): drop the node lock before pruning its neighbour list `DeltaHnsw::connect_node`'s reverse-connection loop held a node's write guard and then took a second lock on the same node: let mut neighbor = self.nodes[neighbor_idx as usize].write(); ... let node_vec = self.nodes[neighbor_idx as usize].read().vector.clone(); self.prune_neighbors(&mut neighbor.neighbors[l], &node_vec, max_conn); `parking_lot::RwLock` is not reentrant, so the `read()` blocks forever the first time a neighbour's adjacency list exceeds `max_conn`. `prune_neighbors` carries the same hazard one level down: it calls `distance`, which takes `nodes[n].read()` for every entry in the list being pruned. `tests::test_insert_and_search` (100 inserts of 128-dim vectors) reaches that branch and parks at 0% CPU indefinitely; sampling the process shows the test thread in `parking_lot_core::parking_lot::park` under `RwLock::read` under `connect_node` under `insert`. The loop now takes the write guard only long enough to push the backlink and, when pruning is required, copy out the adjacency list and the node's own vector. The guard is dropped before `prune_neighbors` runs, so no lock is held while it takes read locks, and the pruned list is stored back under a fresh write guard. `prune_neighbors` and the neighbour-selection logic are unchanged. Adds `test_connect_node_reverse_prune_no_deadlock`, which drives the pruning path with a small `m`/`m0` and bounds itself with a channel timeout so a regression fails the test rather than hanging the runner. With the previous locking restored by hand, that test fails after its timeout and `test_insert_and_search` hangs until the runner kills it; with the fix, all 15 tests in the crate pass in about 0.25s. --- crates/ruvector-delta-index/src/lib.rs | 70 +++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/crates/ruvector-delta-index/src/lib.rs b/crates/ruvector-delta-index/src/lib.rs index 614cf58809..ce03680247 100644 --- a/crates/ruvector-delta-index/src/lib.rs +++ b/crates/ruvector-delta-index/src/lib.rs @@ -443,14 +443,32 @@ impl DeltaHnsw { // Add reverse connections for &neighbor_idx in &selected { - let mut neighbor = self.nodes[neighbor_idx as usize].write(); - if l < neighbor.neighbors.len() { - neighbor.neighbors[l].push(node_idx); - - // Prune if over limit - if neighbor.neighbors[l].len() > max_conn { - let node_vec = self.nodes[neighbor_idx as usize].read().vector.clone(); - self.prune_neighbors(&mut neighbor.neighbors[l], &node_vec, max_conn); + // Take the write guard just long enough to push the new backlink and, if + // pruning is needed, copy out the adjacency list plus the neighbor's own + // vector. The guard is dropped before `prune_neighbors` (and the `distance` + // calls it makes) run, since those take read locks on the same nodes and + // parking_lot's RwLock is not reentrant on a single thread. + let to_prune = { + let mut neighbor = self.nodes[neighbor_idx as usize].write(); + if l < neighbor.neighbors.len() { + neighbor.neighbors[l].push(node_idx); + + if neighbor.neighbors[l].len() > max_conn { + Some((neighbor.neighbors[l].clone(), neighbor.vector.clone())) + } else { + None + } + } else { + None + } + }; + + if let Some((mut list, node_vec)) = to_prune { + self.prune_neighbors(&mut list, &node_vec, max_conn); + + let mut neighbor = self.nodes[neighbor_idx as usize].write(); + if l < neighbor.neighbors.len() { + neighbor.neighbors[l] = list; } } } @@ -777,4 +795,40 @@ mod tests { let results = index.search(&[0.0, 1.0, 0.0, 0.0], 10).unwrap(); assert!(results.iter().all(|r| r.id != "b")); } + + /// Regression test for a self-deadlock in `connect_node`'s reverse-connection loop: + /// pruning a neighbor's adjacency list used to take a second lock (directly, and via + /// `distance` inside `prune_neighbors`) on a node whose write guard was already held + /// on the same thread. `parking_lot::RwLock` is not reentrant, so that hung forever. + /// Uses a small `m0`/`m` so pruning is reached almost immediately, and runs the insert + /// loop on a background thread with a bounded `recv_timeout` so a regression fails the + /// test instead of hanging the test runner. + #[test] + fn test_connect_node_reverse_prune_no_deadlock() { + use std::sync::mpsc; + use std::time::Duration; + + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let config = DeltaHnswConfig { + m: 4, + m0: 4, + ..DeltaHnswConfig::default() + }; + let mut index = DeltaHnsw::new(8, config); + + for i in 0..50 { + let vec = random_vector(8); + index.insert(&format!("v{}", i), vec).unwrap(); + } + + let _ = tx.send(index.len()); + }); + + let len = rx + .recv_timeout(Duration::from_secs(15)) + .expect("connect_node reverse-connection pruning deadlocked (timed out)"); + + assert_eq!(len, 50); + } } From c281fac42f6066f28618b2bc5cf4f10ce4d51532 Mon Sep 17 00:00:00 2001 From: OceanLi <122793010+ohdearquant@users.noreply.github.com> Date: Mon, 3 Aug 2026 13:41:44 -0400 Subject: [PATCH 2/2] test(delta-index): scope the deadlock claim to the read that reproduces it The regression test's comment said the old code took a second lock both directly and through `prune_neighbors`. Only the direct `read()` is demonstrated: `prune_neighbors` reads the entries of the list it prunes, which in a normally constructed graph are other nodes, so it deadlocks only for a list that contains its own owner. Both comments now say which is which, and the test records that `m0 = 4` makes the sixth insert reach the branch. The test also no longer reports a worker panic, a failed insert and a real timeout with the same message, and the bound is raised to 60s so a starved runner is not read as a deadlock. --- crates/ruvector-delta-index/src/lib.rs | 46 ++++++++++++++++---------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/crates/ruvector-delta-index/src/lib.rs b/crates/ruvector-delta-index/src/lib.rs index ce03680247..487d415495 100644 --- a/crates/ruvector-delta-index/src/lib.rs +++ b/crates/ruvector-delta-index/src/lib.rs @@ -445,9 +445,11 @@ impl DeltaHnsw { for &neighbor_idx in &selected { // Take the write guard just long enough to push the new backlink and, if // pruning is needed, copy out the adjacency list plus the neighbor's own - // vector. The guard is dropped before `prune_neighbors` (and the `distance` - // calls it makes) run, since those take read locks on the same nodes and - // parking_lot's RwLock is not reentrant on a single thread. + // vector. Reading that vector through a second `read()` on this same node + // deadlocked: parking_lot's RwLock is not reentrant. `prune_neighbors` is + // also called after the guard is dropped, since the `distance` calls it + // makes read every node in the list it is pruning, which would take the + // same lock again for any list that contains its own owner. let to_prune = { let mut neighbor = self.nodes[neighbor_idx as usize].write(); if l < neighbor.neighbors.len() { @@ -796,13 +798,13 @@ mod tests { assert!(results.iter().all(|r| r.id != "b")); } - /// Regression test for a self-deadlock in `connect_node`'s reverse-connection loop: - /// pruning a neighbor's adjacency list used to take a second lock (directly, and via - /// `distance` inside `prune_neighbors`) on a node whose write guard was already held - /// on the same thread. `parking_lot::RwLock` is not reentrant, so that hung forever. - /// Uses a small `m0`/`m` so pruning is reached almost immediately, and runs the insert - /// loop on a background thread with a bounded `recv_timeout` so a regression fails the - /// test instead of hanging the test runner. + /// Regression test for a self-deadlock in `connect_node`'s reverse-connection loop. + /// Pruning a neighbour's adjacency list used to take a `read()` on the very node whose + /// write guard was still held on the same thread, and `parking_lot::RwLock` is not + /// reentrant, so that blocked forever. `m0 = 4` makes the sixth insert cross the + /// pruning threshold, so the branch is reached deterministically despite the random + /// vectors. The insert loop runs on a background thread behind a bounded receive, so + /// a regression fails the test instead of hanging the runner. #[test] fn test_connect_node_reverse_prune_no_deadlock() { use std::sync::mpsc; @@ -817,17 +819,25 @@ mod tests { }; let mut index = DeltaHnsw::new(8, config); - for i in 0..50 { - let vec = random_vector(8); - index.insert(&format!("v{}", i), vec).unwrap(); - } + let result = (0..50).try_fold((), |(), i| { + index + .insert(&format!("v{}", i), random_vector(8)) + .map_err(|e| e.to_string()) + }); - let _ = tx.send(index.len()); + let _ = tx.send(result.map(|()| index.len())); }); - let len = rx - .recv_timeout(Duration::from_secs(15)) - .expect("connect_node reverse-connection pruning deadlocked (timed out)"); + // A disconnected channel means the worker panicked, which is a different + // failure from the deadlock this test exists to catch. Report them apart. + let len = match rx.recv_timeout(Duration::from_secs(60)) { + Ok(Ok(len)) => len, + Ok(Err(e)) => panic!("insert failed: {}", e), + Err(mpsc::RecvTimeoutError::Timeout) => { + panic!("connect_node reverse-connection pruning did not finish in 60s") + } + Err(mpsc::RecvTimeoutError::Disconnected) => panic!("insert worker panicked"), + }; assert_eq!(len, 50); }