From 95bc3cc11c66f0878633892c6996b17741ac77ef Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Tue, 25 Aug 2026 10:27:16 -0700 Subject: [PATCH 01/10] [feat][node] Add RepairSchedule trait with tokio and manual impls --- Cargo.toml | 5 +- src/node/mod.rs | 3 ++ src/node/repair_schedule.rs | 79 ++++++++++++++++++++++++++++++++ src/node/repair_schedule_test.rs | 63 +++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 src/node/repair_schedule.rs create mode 100644 src/node/repair_schedule_test.rs diff --git a/Cargo.toml b/Cargo.toml index 47d2cfc..90f55f1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,4 +14,7 @@ tracing-subscriber = "0.3" unimock = "0.6" parking_lot = "0.12" tokio = { version = "1.0", features = ["sync", "time", "macros", "rt", "rt-multi-thread"] } -tokio-util = "0.7" \ No newline at end of file +tokio-util = "0.7" + +[dev-dependencies] +tokio = { version = "1.0", features = ["test-util"] } \ No newline at end of file diff --git a/src/node/mod.rs b/src/node/mod.rs index f913028..69bdbf0 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -2,6 +2,9 @@ mod base_node; pub(crate) mod core; #[cfg(test)] mod core_test; +mod repair_schedule; +#[cfg(test)] +mod repair_schedule_test; #[cfg(test)] mod search_by_id_test; #[cfg(test)] diff --git a/src/node/repair_schedule.rs b/src/node/repair_schedule.rs new file mode 100644 index 0000000..3240536 --- /dev/null +++ b/src/node/repair_schedule.rs @@ -0,0 +1,79 @@ +use std::future::Future; +use std::pin::Pin; +use std::time::Duration; +use tokio::sync::Mutex; +#[cfg(test)] +use tokio::sync::Notify; +use tokio::time::Interval; + +/// RepairSchedule abstracts "wait for the next backpointer-repair tick" so the repair task +/// never calls `tokio::time::sleep`/`interval` directly. [`TokioRepairSchedule`] is the +/// production implementation; [`ManualRepairSchedule`] is a test double a test drives +/// explicitly, one tick at a time, with zero wall-clock waiting. +// TODO: remove once RepairSchedule is wired into the repair task. +#[allow(dead_code)] +pub(crate) trait RepairSchedule: Send + Sync { + /// Waits for the next repair tick. + fn tick(&self) -> Pin + Send + '_>>; +} + +/// Production `RepairSchedule`, backed by a real `tokio::time::Interval`. +// TODO: remove once RepairSchedule is wired into the repair task. +#[allow(dead_code)] +pub(crate) struct TokioRepairSchedule { + interval: Mutex, +} + +impl TokioRepairSchedule { + /// Creates a schedule that ticks every `period`. + /// + /// # Args + /// + /// * `period` - the duration between repair ticks. + /// + /// Must be called from within a running Tokio runtime, per + /// `tokio::time::interval`'s own precondition. + #[allow(dead_code)] // TODO: remove once RepairSchedule is wired into the repair task. + pub(crate) fn new(period: Duration) -> Self { + TokioRepairSchedule { + interval: Mutex::new(tokio::time::interval(period)), + } + } +} + +impl RepairSchedule for TokioRepairSchedule { + fn tick(&self) -> Pin + Send + '_>> { + Box::pin(async move { + self.interval.lock().await.tick().await; + }) + } +} + +/// Test `RepairSchedule`, driven by an explicit, manually-fired gate: +/// [`ManualRepairSchedule::fire`] completes exactly one pending or future +/// [`RepairSchedule::tick`] call. +#[cfg(test)] +pub(crate) struct ManualRepairSchedule { + notify: Notify, +} + +#[cfg(test)] +impl ManualRepairSchedule { + pub(crate) fn new() -> Self { + ManualRepairSchedule { + notify: Notify::new(), + } + } + + /// Fires exactly one repair tick, completing one pending or future `tick()` call. + pub(crate) fn fire(&self) { + self.notify.notify_one(); + } +} + +#[cfg(test)] +impl RepairSchedule for ManualRepairSchedule { + fn tick(&self) -> Pin + Send + '_>> { + Box::pin(self.notify.notified()) + } +} diff --git a/src/node/repair_schedule_test.rs b/src/node/repair_schedule_test.rs new file mode 100644 index 0000000..d8a1b6e --- /dev/null +++ b/src/node/repair_schedule_test.rs @@ -0,0 +1,63 @@ +use crate::node::repair_schedule::{ManualRepairSchedule, RepairSchedule, TokioRepairSchedule}; +use std::sync::Arc; +use std::time::Duration; + +/// Verifies `fire` completes a `tick` call that is already pending. +#[tokio::test] +async fn test_manual_repair_schedule_fire_completes_pending_tick() { + let schedule = Arc::new(ManualRepairSchedule::new()); + let waiter = tokio::spawn({ + let schedule = schedule.clone(); + async move { schedule.tick().await } + }); + + // give the spawned task a chance to start polling `tick()` before firing. + tokio::task::yield_now().await; + schedule.fire(); + + tokio::time::timeout(Duration::from_secs(2), waiter) + .await + .expect("tick did not complete before the timeout") + .expect("spawned task panicked"); +} + +/// Verifies a `fire` issued before `tick` is called is not lost: the next `tick` call +/// completes immediately instead of waiting for a subsequent `fire`. +#[tokio::test] +async fn test_manual_repair_schedule_fire_before_tick_is_not_lost() { + let schedule = ManualRepairSchedule::new(); + schedule.fire(); + + tokio::time::timeout(Duration::from_secs(2), schedule.tick()) + .await + .expect("tick did not complete before the timeout"); +} + +/// Verifies a single `fire` unblocks exactly one `tick` call: a second, independent +/// `tick` call still waits for its own `fire`. +#[tokio::test(start_paused = true)] +async fn test_manual_repair_schedule_single_fire_grants_exactly_one_tick() { + let schedule = ManualRepairSchedule::new(); + schedule.fire(); + + tokio::time::timeout(Duration::from_secs(2), schedule.tick()) + .await + .expect("first tick did not complete before the timeout"); + + let second_tick = tokio::time::timeout(Duration::from_millis(50), schedule.tick()).await; + assert!( + second_tick.is_err(), + "second tick completed without its own fire" + ); +} + +/// Verifies `TokioRepairSchedule::tick` actually resolves once its period elapses, +/// confirming the trait is correctly wired to a real `tokio::time::Interval`. +#[tokio::test(start_paused = true)] +async fn test_tokio_repair_schedule_ticks_after_period_elapses() { + let schedule = TokioRepairSchedule::new(Duration::from_millis(100)); + + tokio::time::timeout(Duration::from_secs(2), schedule.tick()) + .await + .expect("tick did not complete before the timeout"); +} From 90ca56dd9aa0a4ff35ecfb945aabceca8f06bb68 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:46:53 -0700 Subject: [PATCH 02/10] [fix][node] Fix doc grammar and assert interval actually waits --- src/node/repair_schedule.rs | 2 +- src/node/repair_schedule_test.rs | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/node/repair_schedule.rs b/src/node/repair_schedule.rs index 3240536..bc02f35 100644 --- a/src/node/repair_schedule.rs +++ b/src/node/repair_schedule.rs @@ -8,7 +8,7 @@ use tokio::time::Interval; /// RepairSchedule abstracts "wait for the next backpointer-repair tick" so the repair task /// never calls `tokio::time::sleep`/`interval` directly. [`TokioRepairSchedule`] is the -/// production implementation; [`ManualRepairSchedule`] is a test double a test drives +/// production implementation; [`ManualRepairSchedule`] is a test double that a test drives /// explicitly, one tick at a time, with zero wall-clock waiting. // TODO: remove once RepairSchedule is wired into the repair task. #[allow(dead_code)] diff --git a/src/node/repair_schedule_test.rs b/src/node/repair_schedule_test.rs index d8a1b6e..546290c 100644 --- a/src/node/repair_schedule_test.rs +++ b/src/node/repair_schedule_test.rs @@ -57,7 +57,17 @@ async fn test_manual_repair_schedule_single_fire_grants_exactly_one_tick() { async fn test_tokio_repair_schedule_ticks_after_period_elapses() { let schedule = TokioRepairSchedule::new(Duration::from_millis(100)); + // `tokio::time::interval`'s own first tick resolves immediately; consume it so the + // assertions below observe a tick that actually waited out the period. + schedule.tick().await; + + let premature = tokio::time::timeout(Duration::from_millis(50), schedule.tick()).await; + assert!( + premature.is_err(), + "tick completed before its period elapsed" + ); + tokio::time::timeout(Duration::from_secs(2), schedule.tick()) .await - .expect("tick did not complete before the timeout"); + .expect("tick did not complete after its period elapsed"); } From 749ad5ea9676941c7803c69176a31d046803af35 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Wed, 26 Aug 2026 10:12:33 -0700 Subject: [PATCH 03/10] [improve][test] Explain premature-tick timeout --- src/node/repair_schedule_test.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/node/repair_schedule_test.rs b/src/node/repair_schedule_test.rs index 546290c..efebe03 100644 --- a/src/node/repair_schedule_test.rs +++ b/src/node/repair_schedule_test.rs @@ -61,6 +61,8 @@ async fn test_tokio_repair_schedule_ticks_after_period_elapses() { // assertions below observe a tick that actually waited out the period. schedule.tick().await; + // the period is 100ms, so a tick can't legitimately arrive within this 50ms window; + // timing out here proves the schedule didn't fire early. let premature = tokio::time::timeout(Duration::from_millis(50), schedule.tick()).await; assert!( premature.is_err(), From 37a8089dd8d81ac17afb62b6f32d8df9581c51fd Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Wed, 26 Aug 2026 14:13:12 -0700 Subject: [PATCH 04/10] [improve][node] Document ManualRepairSchedule fire coalescing --- src/node/repair_schedule.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/node/repair_schedule.rs b/src/node/repair_schedule.rs index bc02f35..5923a56 100644 --- a/src/node/repair_schedule.rs +++ b/src/node/repair_schedule.rs @@ -66,6 +66,9 @@ impl ManualRepairSchedule { } /// Fires exactly one repair tick, completing one pending or future `tick()` call. + /// + /// Permits don't accumulate: firing twice with no intervening `tick()` call still + /// unblocks only one future `tick()` call. pub(crate) fn fire(&self) { self.notify.notify_one(); } From b9ff3ed4b79d02e2da2bae843b38fe126260bf04 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Wed, 26 Aug 2026 16:36:17 -0700 Subject: [PATCH 05/10] [improve][test] Bound first-tick wait with a timeout --- src/node/repair_schedule_test.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node/repair_schedule_test.rs b/src/node/repair_schedule_test.rs index efebe03..f99e0f6 100644 --- a/src/node/repair_schedule_test.rs +++ b/src/node/repair_schedule_test.rs @@ -59,7 +59,9 @@ async fn test_tokio_repair_schedule_ticks_after_period_elapses() { // `tokio::time::interval`'s own first tick resolves immediately; consume it so the // assertions below observe a tick that actually waited out the period. - schedule.tick().await; + tokio::time::timeout(Duration::from_secs(2), schedule.tick()) + .await + .expect("first tick did not resolve immediately"); // the period is 100ms, so a tick can't legitimately arrive within this 50ms window; // timing out here proves the schedule didn't fire early. From df1780b4e2257a75fce1c4b63d86f62d3fc809ca Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:36:02 -0700 Subject: [PATCH 06/10] [cleanup][node] Consolidate dead_code allows --- Cargo.toml | 6 ++++++ src/node/base_node.rs | 6 ------ src/node/core.rs | 5 ----- src/node/repair_schedule.rs | 5 ----- src/node/waiter.rs | 2 -- 5 files changed, 6 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 90f55f1..6f2b3d5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,6 +4,12 @@ version = "0.1.0" edition = "2021" rust-version = "1.89.0" +[lints.rust] +# Several node types (BaseNode, BaseCore, RepairSchedule, Waiter) are staged ahead of the +# production wiring that will call them; allow at the package level instead of scattering +# per-item #[allow(dead_code)] across every not-yet-wired item. Remove once wired in. +dead_code = "allow" + [dependencies] hex = "0.4.3" anyhow = "1.0.86" diff --git a/src/node/base_node.rs b/src/node/base_node.rs index 258113c..d705897 100644 --- a/src/node/base_node.rs +++ b/src/node/base_node.rs @@ -19,8 +19,6 @@ use std::time::Duration; use tokio::sync::oneshot; use tracing::{Instrument, Span}; -// TODO: Remove #[allow(dead_code)] once BaseNode is used in production code. -#[allow(dead_code)] /// `BaseNode` is the network-aware orchestrator for a single skip-graph node. /// /// It composes a `Box` (the pure-local algorithms + lookup table) @@ -75,18 +73,15 @@ impl BaseNode { } /// Returns the node's identifier (delegated to core). - #[allow(dead_code)] pub(crate) fn id(&self) -> Identifier { self.core.id() } /// Returns the node's membership vector (delegated to core). - #[allow(dead_code)] pub(crate) fn mem_vec(&self) -> MembershipVector { self.core.mem_vec() } - #[allow(dead_code)] pub(crate) fn search_by_id(&self, req: IdSearchReq) -> anyhow::Result { let span = tracing::trace_span!("search_by_id", target = ?req.target, level = ?req.level); let _enter = span.enter(); @@ -169,7 +164,6 @@ impl BaseNode { /// seeded level. /// * **RECOVERABLE** — the reply channel is dropped before a reply arrives. /// * **RECOVERABLE** — `timeout` elapses before a reply arrives. - #[allow(dead_code)] // TODO: remove once phase-0 bootstrap is wired into join orchestration. pub(crate) async fn get_max_level( &self, introducer: Identifier, diff --git a/src/node/core.rs b/src/node/core.rs index 9cf504a..f374bfc 100644 --- a/src/node/core.rs +++ b/src/node/core.rs @@ -30,7 +30,6 @@ pub trait Core: Send + Sync { fn search_by_id(&self, req: IdSearchReq) -> anyhow::Result; /// Performs a local search for the given membership vector. - #[allow(dead_code)] fn search_by_mem_vec(&self, req: IdSearchReq) -> anyhow::Result; /// Returns the highest lookup-table level at which this node has any @@ -50,7 +49,6 @@ pub trait Core: Send + Sync { /// **CRITICAL, INTERNAL** — propagated from a failed read of the local /// lookup table: a broken local invariant, not evidence of anything a /// peer sent. - #[allow(dead_code)] // TODO: remove once max_level is wired into join bootstrap. fn max_level(&self) -> anyhow::Result; /// Reports whether this node's membership vector shares a common prefix @@ -60,7 +58,6 @@ pub trait Core: Send + Sync { /// /// * `candidate` - the membership vector to compare against this node's own. /// * `level` - the minimum required common-prefix length, in bits. - #[allow(dead_code)] // TODO: remove once prefix_match is wired into BuddyOp handling. fn prefix_match(&self, candidate: MembershipVector, level: LookupTableLevel) -> bool; /// Shallow-clones this core. Cloned instances share the same underlying @@ -78,8 +75,6 @@ impl Clone for Box { /// `ArrayLookupTable`-style lookup table. It owns the node's identifier, /// membership vector, and lookup table. All state is shallow-cloneable via /// the Arc-backed lookup table; cloned instances share the same LT. -// TODO: Remove #[allow(dead_code)] once BaseCore is used in production code. -#[allow(dead_code)] pub struct BaseCore { id: Identifier, mem_vec: MembershipVector, diff --git a/src/node/repair_schedule.rs b/src/node/repair_schedule.rs index 5923a56..1c3e32f 100644 --- a/src/node/repair_schedule.rs +++ b/src/node/repair_schedule.rs @@ -10,16 +10,12 @@ use tokio::time::Interval; /// never calls `tokio::time::sleep`/`interval` directly. [`TokioRepairSchedule`] is the /// production implementation; [`ManualRepairSchedule`] is a test double that a test drives /// explicitly, one tick at a time, with zero wall-clock waiting. -// TODO: remove once RepairSchedule is wired into the repair task. -#[allow(dead_code)] pub(crate) trait RepairSchedule: Send + Sync { /// Waits for the next repair tick. fn tick(&self) -> Pin + Send + '_>>; } /// Production `RepairSchedule`, backed by a real `tokio::time::Interval`. -// TODO: remove once RepairSchedule is wired into the repair task. -#[allow(dead_code)] pub(crate) struct TokioRepairSchedule { interval: Mutex, } @@ -33,7 +29,6 @@ impl TokioRepairSchedule { /// /// Must be called from within a running Tokio runtime, per /// `tokio::time::interval`'s own precondition. - #[allow(dead_code)] // TODO: remove once RepairSchedule is wired into the repair task. pub(crate) fn new(period: Duration) -> Self { TokioRepairSchedule { interval: Mutex::new(tokio::time::interval(period)), diff --git a/src/node/waiter.rs b/src/node/waiter.rs index 45b85ca..a4df785 100644 --- a/src/node/waiter.rs +++ b/src/node/waiter.rs @@ -12,8 +12,6 @@ use tokio::sync::oneshot; /// differ in concurrency shape: `search_by_id` stays synchronous (blocking `recv`, /// unchanged), while `get_max_level` is `async` (a `tokio::sync::oneshot::Receiver` /// awaited under a timeout). -// TODO: Remove #[allow(dead_code)] once BaseNode is used in production code. -#[allow(dead_code)] pub(super) enum Waiter { /// a pending `search_by_id` call, resolved by a `SearchByIdResponse`. Search(SyncSender), From e53671862671815c765a57701fc98e6447484086 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:01:46 -0700 Subject: [PATCH 07/10] [fix][core] Single-lock max_level computation --- src/core/lookup/array_lookup_table.rs | 13 +++++++ src/core/lookup/array_lookup_table_test.rs | 40 ++++++++++++++++++++++ src/core/lookup/mod.rs | 18 ++++++++++ src/node/core.rs | 16 ++------- src/node/core_test.rs | 4 +-- 5 files changed, 76 insertions(+), 15 deletions(-) diff --git a/src/core/lookup/array_lookup_table.rs b/src/core/lookup/array_lookup_table.rs index e00d165..e484ef7 100644 --- a/src/core/lookup/array_lookup_table.rs +++ b/src/core/lookup/array_lookup_table.rs @@ -314,6 +314,19 @@ impl LookupTable for ArrayLookupTable { Ok(neighbors) } + /// Implements [`LookupTable::max_populated_level`] under a single `inner.read()` guard, so + /// the left and right sides are inspected against the same snapshot of the table. + fn max_populated_level(&self) -> anyhow::Result> { + let inner = self.inner.read(); + + for level in (0..LOOKUP_TABLE_LEVELS).rev() { + if inner.left[level].is_some() || inner.right[level].is_some() { + return Ok(Some(level)); + } + } + Ok(None) + } + fn clone_box(&self) -> Box { Box::new(self.clone()) } diff --git a/src/core/lookup/array_lookup_table_test.rs b/src/core/lookup/array_lookup_table_test.rs index 083f2a6..f3f7d43 100644 --- a/src/core/lookup/array_lookup_table_test.rs +++ b/src/core/lookup/array_lookup_table_test.rs @@ -581,6 +581,46 @@ mod tests { } } + /// `max_populated_level` on an empty table returns `None`, distinguishing "nothing + /// populated" from a populated level 0. + #[test] + fn test_max_populated_level_empty_table() { + let lt = ArrayLookupTable::new(); + assert_eq!(lt.max_populated_level().unwrap(), None); + } + + /// `max_populated_level` returns the populated level when only one side of the table has + /// an entry. + #[test] + fn test_max_populated_level_one_side_populated() { + let lt = ArrayLookupTable::new(); + lt.update_entry(random_identity(), 4, Direction::Left) + .unwrap(); + assert_eq!(lt.max_populated_level().unwrap(), Some(4)); + } + + /// `max_populated_level` returns the higher of the two levels when left and right are + /// populated at different levels. + #[test] + fn test_max_populated_level_both_sides_different_levels() { + let lt = ArrayLookupTable::new(); + lt.update_entry(random_identity(), 2, Direction::Left) + .unwrap(); + lt.update_entry(random_identity(), 6, Direction::Right) + .unwrap(); + assert_eq!(lt.max_populated_level().unwrap(), Some(6)); + } + + /// `max_populated_level` returns `Some(0)`, not `None`, when level 0 is the only populated + /// entry. An empty table and a table populated only at level 0 must not be conflated. + #[test] + fn test_max_populated_level_only_level_zero_populated() { + let lt = ArrayLookupTable::new(); + lt.update_entry(random_identity(), 0, Direction::Right) + .unwrap(); + assert_eq!(lt.max_populated_level().unwrap(), Some(0)); + } + /// Tests that cloning ArrayLookupTable creates a shallow copy. /// Changes made to one instance should be visible in the cloned instance. #[test] diff --git a/src/core/lookup/mod.rs b/src/core/lookup/mod.rs index 58658ea..fd8c325 100644 --- a/src/core/lookup/mod.rs +++ b/src/core/lookup/mod.rs @@ -162,6 +162,24 @@ pub trait LookupTable: Send + Sync { /// Returns the list of right neighbors at the current node as a vector of tuples containing the level and identity. fn right_neighbors(&self) -> anyhow::Result>; + /// Returns the highest level with a populated entry on either side. + /// + /// Unlike calling [`Self::left_neighbors`] and [`Self::right_neighbors`] separately and + /// combining the results, the left and right sides are read atomically with respect to + /// concurrent writes. No concurrent write can land between reading the two sides and leave + /// the combined result reflecting no single consistent state of the table. + /// + /// # Returns + /// + /// `None` if no level has a populated entry on either side (an empty table). `Some(level)` + /// for the highest level with a populated entry, otherwise. + /// + /// # Errors + /// + /// **CRITICAL, INTERNAL**: a failed read of the local table, not evidence of anything a + /// peer sent. + fn max_populated_level(&self) -> anyhow::Result>; + /// Creates a shallow copy of this lookup table. /// /// Implementations should ensure that cloned instances share the same underlying data diff --git a/src/node/core.rs b/src/node/core.rs index f374bfc..2d60c27 100644 --- a/src/node/core.rs +++ b/src/node/core.rs @@ -204,20 +204,10 @@ impl Core for BaseCore { } fn max_level(&self) -> anyhow::Result { - let left = self + Ok(self .lt - .left_neighbors() - .context("failed to read left neighbors from lookup table")?; - let right = self - .lt - .right_neighbors() - .context("failed to read right neighbors from lookup table")?; - - Ok(left - .iter() - .chain(right.iter()) - .map(|(level, _)| *level) - .max() + .max_populated_level() + .context("failed to compute max lookup table level")? .unwrap_or(0)) } diff --git a/src/node/core_test.rs b/src/node/core_test.rs index a92b0a4..b51bb94 100644 --- a/src/node/core_test.rs +++ b/src/node/core_test.rs @@ -409,7 +409,7 @@ fn test_max_level_both_sides_populated_different_levels() { #[test] fn test_max_level_error_propagation() { let lt = Unimock::new( - LookupTableMock::left_neighbors + LookupTableMock::max_populated_level .each_call(matching!()) .answers(&|_| Err(anyhow!("simulated lookup table error"))), ); @@ -423,7 +423,7 @@ fn test_max_level_error_propagation() { ); let error_msg = result.unwrap_err().to_string(); assert!( - error_msg.contains("failed to read left neighbors from lookup table"), + error_msg.contains("failed to compute max lookup table level"), "error message '{error_msg}' doesn't contain expected text" ); } From 7e938846f2c1b7edf3384e1221c105c6954656d3 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Sat, 29 Aug 2026 13:02:28 -0700 Subject: [PATCH 08/10] [cleanup][core] Unwrap infallible lookup result --- src/core/lookup/array_lookup_table.rs | 11 ++++------- src/core/lookup/array_lookup_table_test.rs | 8 ++++---- src/core/lookup/mod.rs | 7 +------ src/node/core.rs | 8 ++------ src/node/core_test.rs | 23 ---------------------- 5 files changed, 11 insertions(+), 46 deletions(-) diff --git a/src/core/lookup/array_lookup_table.rs b/src/core/lookup/array_lookup_table.rs index e484ef7..0a42d80 100644 --- a/src/core/lookup/array_lookup_table.rs +++ b/src/core/lookup/array_lookup_table.rs @@ -316,15 +316,12 @@ impl LookupTable for ArrayLookupTable { /// Implements [`LookupTable::max_populated_level`] under a single `inner.read()` guard, so /// the left and right sides are inspected against the same snapshot of the table. - fn max_populated_level(&self) -> anyhow::Result> { + fn max_populated_level(&self) -> Option { let inner = self.inner.read(); - for level in (0..LOOKUP_TABLE_LEVELS).rev() { - if inner.left[level].is_some() || inner.right[level].is_some() { - return Ok(Some(level)); - } - } - Ok(None) + (0..LOOKUP_TABLE_LEVELS) + .rev() + .find(|&level| inner.left[level].is_some() || inner.right[level].is_some()) } fn clone_box(&self) -> Box { diff --git a/src/core/lookup/array_lookup_table_test.rs b/src/core/lookup/array_lookup_table_test.rs index f3f7d43..317bc74 100644 --- a/src/core/lookup/array_lookup_table_test.rs +++ b/src/core/lookup/array_lookup_table_test.rs @@ -586,7 +586,7 @@ mod tests { #[test] fn test_max_populated_level_empty_table() { let lt = ArrayLookupTable::new(); - assert_eq!(lt.max_populated_level().unwrap(), None); + assert_eq!(lt.max_populated_level(), None); } /// `max_populated_level` returns the populated level when only one side of the table has @@ -596,7 +596,7 @@ mod tests { let lt = ArrayLookupTable::new(); lt.update_entry(random_identity(), 4, Direction::Left) .unwrap(); - assert_eq!(lt.max_populated_level().unwrap(), Some(4)); + assert_eq!(lt.max_populated_level(), Some(4)); } /// `max_populated_level` returns the higher of the two levels when left and right are @@ -608,7 +608,7 @@ mod tests { .unwrap(); lt.update_entry(random_identity(), 6, Direction::Right) .unwrap(); - assert_eq!(lt.max_populated_level().unwrap(), Some(6)); + assert_eq!(lt.max_populated_level(), Some(6)); } /// `max_populated_level` returns `Some(0)`, not `None`, when level 0 is the only populated @@ -618,7 +618,7 @@ mod tests { let lt = ArrayLookupTable::new(); lt.update_entry(random_identity(), 0, Direction::Right) .unwrap(); - assert_eq!(lt.max_populated_level().unwrap(), Some(0)); + assert_eq!(lt.max_populated_level(), Some(0)); } /// Tests that cloning ArrayLookupTable creates a shallow copy. diff --git a/src/core/lookup/mod.rs b/src/core/lookup/mod.rs index fd8c325..2301bbf 100644 --- a/src/core/lookup/mod.rs +++ b/src/core/lookup/mod.rs @@ -173,12 +173,7 @@ pub trait LookupTable: Send + Sync { /// /// `None` if no level has a populated entry on either side (an empty table). `Some(level)` /// for the highest level with a populated entry, otherwise. - /// - /// # Errors - /// - /// **CRITICAL, INTERNAL**: a failed read of the local table, not evidence of anything a - /// peer sent. - fn max_populated_level(&self) -> anyhow::Result>; + fn max_populated_level(&self) -> Option; /// Creates a shallow copy of this lookup table. /// diff --git a/src/node/core.rs b/src/node/core.rs index 2d60c27..9a1dcca 100644 --- a/src/node/core.rs +++ b/src/node/core.rs @@ -2,7 +2,7 @@ use crate::core::model::direction::Direction; use crate::core::{ IdSearchReq, IdSearchRes, Identifier, LookupTable, LookupTableLevel, MembershipVector, }; -use anyhow::{anyhow, Context}; +use anyhow::anyhow; use tracing::Span; /// Core is the pure-local interface for a skip-graph node's algorithms. @@ -204,11 +204,7 @@ impl Core for BaseCore { } fn max_level(&self) -> anyhow::Result { - Ok(self - .lt - .max_populated_level() - .context("failed to compute max lookup table level")? - .unwrap_or(0)) + Ok(self.lt.max_populated_level().unwrap_or(0)) } fn prefix_match(&self, candidate: MembershipVector, level: LookupTableLevel) -> bool { diff --git a/src/node/core_test.rs b/src/node/core_test.rs index b51bb94..99356e9 100644 --- a/src/node/core_test.rs +++ b/src/node/core_test.rs @@ -405,29 +405,6 @@ fn test_max_level_both_sides_populated_different_levels() { assert_eq!(core.max_level().unwrap(), 7); } -/// Verifies `max_level` propagates errors raised by the underlying lookup table. -#[test] -fn test_max_level_error_propagation() { - let lt = Unimock::new( - LookupTableMock::max_populated_level - .each_call(matching!()) - .answers(&|_| Err(anyhow!("simulated lookup table error"))), - ); - - let core = make_core(random_identifier(), Box::new(lt)); - let result = core.max_level(); - - assert!( - result.is_err(), - "expected an error but got a success result" - ); - let error_msg = result.unwrap_err().to_string(); - assert!( - error_msg.contains("failed to compute max lookup table level"), - "error message '{error_msg}' doesn't contain expected text" - ); -} - /// Verifies `prefix_match` matches `common_prefix_bit(candidate) >= level` /// exactly, including at the boundary where they're equal. #[test] From 58a11a973b3ebc883dd56a5e77aafec268129de5 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Sat, 29 Aug 2026 13:23:45 -0700 Subject: [PATCH 09/10] [cleanup][core] Drop dead Result from neighbor lookups --- src/core/lookup/array_lookup_table.rs | 8 ++++---- src/core/lookup/array_lookup_table_test.rs | 4 ++-- src/core/lookup/mod.rs | 4 ++-- src/node/core_test.rs | 4 ---- src/node/search_by_id_test.rs | 1 - 5 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/core/lookup/array_lookup_table.rs b/src/core/lookup/array_lookup_table.rs index 0a42d80..6d76fdd 100644 --- a/src/core/lookup/array_lookup_table.rs +++ b/src/core/lookup/array_lookup_table.rs @@ -289,7 +289,7 @@ impl LookupTable for ArrayLookupTable { } /// Returns the list of left neighbors at the current node as a vector of tuples containing the level and identity. - fn left_neighbors(&self) -> anyhow::Result> { + fn left_neighbors(&self) -> Vec<(usize, Identity)> { let inner = self.inner.read(); let mut neighbors = Vec::new(); @@ -298,11 +298,11 @@ impl LookupTable for ArrayLookupTable { neighbors.push((level, *identity)); } } - Ok(neighbors) + neighbors } /// Returns the list of right neighbors at the current node as a vector of tuples containing the level and identity. - fn right_neighbors(&self) -> anyhow::Result> { + fn right_neighbors(&self) -> Vec<(usize, Identity)> { let inner = self.inner.read(); let mut neighbors = Vec::new(); @@ -311,7 +311,7 @@ impl LookupTable for ArrayLookupTable { neighbors.push((level, *identity)); } } - Ok(neighbors) + neighbors } /// Implements [`LookupTable::max_populated_level`] under a single `inner.read()` guard, so diff --git a/src/core/lookup/array_lookup_table_test.rs b/src/core/lookup/array_lookup_table_test.rs index 317bc74..a8f97d7 100644 --- a/src/core/lookup/array_lookup_table_test.rs +++ b/src/core/lookup/array_lookup_table_test.rs @@ -562,7 +562,7 @@ mod tests { fn test_left_and_right_neighbors() { let lt = random_lookup_table(LOOKUP_TABLE_LEVELS); - let rights = lt.right_neighbors().unwrap(); + let rights = lt.right_neighbors(); assert_eq!(rights.len(), LOOKUP_TABLE_LEVELS); for (level, identity) in rights.iter() { assert_eq!( @@ -571,7 +571,7 @@ mod tests { ); } - let lefts = lt.left_neighbors().unwrap(); + let lefts = lt.left_neighbors(); assert_eq!(lefts.len(), LOOKUP_TABLE_LEVELS); for (level, identity) in lefts.iter() { assert_eq!( diff --git a/src/core/lookup/mod.rs b/src/core/lookup/mod.rs index 2301bbf..857d310 100644 --- a/src/core/lookup/mod.rs +++ b/src/core/lookup/mod.rs @@ -157,10 +157,10 @@ pub trait LookupTable: Send + Sync { ) -> anyhow::Result; /// Returns the list of left neighbors at the current node as a vector of tuples containing the level and identity. - fn left_neighbors(&self) -> anyhow::Result>; + fn left_neighbors(&self) -> Vec<(usize, Identity)>; /// Returns the list of right neighbors at the current node as a vector of tuples containing the level and identity. - fn right_neighbors(&self) -> anyhow::Result>; + fn right_neighbors(&self) -> Vec<(usize, Identity)>; /// Returns the highest level with a populated entry on either side. /// diff --git a/src/node/core_test.rs b/src/node/core_test.rs index 99356e9..333b6b6 100644 --- a/src/node/core_test.rs +++ b/src/node/core_test.rs @@ -75,7 +75,6 @@ fn test_search_by_id_found_left_direction() { let (expected_lvl, expected_identity) = lt .left_neighbors() - .unwrap() .into_iter() .filter(|(l, id)| *l <= req.level && id.id() >= req.target) .min_by_key(|(_, id)| id.id()) @@ -113,7 +112,6 @@ fn test_search_by_id_found_right_direction() { let (expected_lvl, expected_identity) = lt .right_neighbors() - .unwrap() .into_iter() .filter(|(lvl, id)| *lvl <= req.level && id.id() <= req.target) .max_by_key(|(_, id)| id.id()) @@ -253,7 +251,6 @@ fn test_search_by_id_concurrent_found_left_direction() { let expected = lt_clone .left_neighbors() - .unwrap() .into_iter() .filter(|(l, id)| *l <= req.level && id.id() >= req.target) .min_by_key(|(_, id)| id.id()); @@ -308,7 +305,6 @@ fn test_search_by_id_concurrent_right_direction() { let expected = lt_clone .right_neighbors() - .unwrap() .into_iter() .filter(|(l, id)| *l <= req.level && id.id() <= req.target) .max_by_key(|(_, id)| id.id()); diff --git a/src/node/search_by_id_test.rs b/src/node/search_by_id_test.rs index 753a659..4b83b5b 100644 --- a/src/node/search_by_id_test.rs +++ b/src/node/search_by_id_test.rs @@ -41,7 +41,6 @@ fn test_search_by_id_networking_integration_relay() { let (expected_lvl, expected_identity) = lt .left_neighbors() - .unwrap() .into_iter() .filter(|(l, id)| *l <= search_request.level && id.id() >= search_request.target) .min_by_key(|(_, id)| id.id()) From b7136620323e08360054ce7280ff00bcc8e85482 Mon Sep 17 00:00:00 2001 From: yahya <19204398+thep2p@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:28:03 -0700 Subject: [PATCH 10/10] [improve][core] Explain neighbor lookup retention --- src/core/lookup/mod.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/core/lookup/mod.rs b/src/core/lookup/mod.rs index 857d310..ae64699 100644 --- a/src/core/lookup/mod.rs +++ b/src/core/lookup/mod.rs @@ -156,10 +156,16 @@ pub trait LookupTable: Send + Sync { claimant: Identity, ) -> anyhow::Result; - /// Returns the list of left neighbors at the current node as a vector of tuples containing the level and identity. + /// Returns the list of left neighbors at the current node as a vector of tuples containing + /// the level and identity. + /// + /// Exercised only by tests today; retained because a future repair pass needs to enumerate + /// every populated entry here, not just the highest, to probe each neighbor individually. fn left_neighbors(&self) -> Vec<(usize, Identity)>; - /// Returns the list of right neighbors at the current node as a vector of tuples containing the level and identity. + /// Returns the list of right neighbors at the current node as a vector of tuples containing + /// the level and identity. See [`Self::left_neighbors`] for why this is retained despite + /// having no production caller today. fn right_neighbors(&self) -> Vec<(usize, Identity)>; /// Returns the highest level with a populated entry on either side.