diff --git a/src/core/lookup/array_lookup_table.rs b/src/core/lookup/array_lookup_table.rs index d04664d..a53f260 100644 --- a/src/core/lookup/array_lookup_table.rs +++ b/src/core/lookup/array_lookup_table.rs @@ -1,4 +1,4 @@ -use crate::core::lookup::{LinkOutcome, LookupTable, LookupTableLevel}; +use crate::core::lookup::{LinkOutcome, LookupTable, LookupTableLevel, RelinkOutcome}; use crate::core::model; use crate::core::model::direction::Direction; use crate::core::model::identity::Identity; @@ -230,6 +230,64 @@ impl LookupTable for ArrayLookupTable { Ok(outcome) } + /// Implements [`LookupTable::try_relink`] — see that doc for what `claimant` means and what + /// each [`RelinkOutcome`] variant represents. Runs entirely under a single `inner.write()` + /// guard, for the same reason `try_link` does: composing separately-locked + /// `get_entry`/`update_entry` calls would reopen a race between two concurrent repair probes + /// for the same slot, each reading the same stale entry and clobbering the other's write + /// without ever forwarding against the true post-write state. + fn try_relink( + &self, + level: LookupTableLevel, + direction: Direction, + claimant: Identity, + ) -> anyhow::Result { + if level >= LOOKUP_TABLE_LEVELS { + return Err(anyhow!( + "position is larger than the max lookup table entry number: {}", + level + )); + } + + let mut inner = self.inner.write(); + + let existing = match direction { + Direction::Left => inner.left[level], + Direction::Right => inner.right[level], + }; + + // three-way decision against the single read of `existing` above, all inside this one + // write-lock critical section: already-equal is a no-op; strictly-between (same + // per-direction comparison as try_link) forwards; anything else relinks and evicts. + let outcome = match (existing, direction) { + (Some(existing), _) if existing == claimant => RelinkOutcome::AlreadyConsistent, + (Some(existing), Direction::Right) if existing.id() < claimant.id() => { + RelinkOutcome::Forward(existing) + } + (Some(existing), Direction::Left) if existing.id() > claimant.id() => { + RelinkOutcome::Forward(existing) + } + (evicted, _) => { + match direction { + Direction::Left => inner.left[level] = Some(claimant), + Direction::Right => inner.right[level] = Some(claimant), + } + RelinkOutcome::Relinked { evicted } + } + }; + + // Log the try_relink decision + tracing::trace!( + "try_relink decision at level {} in direction {}: claimant {}, outcome {:?}", + level, + direction, + claimant.id(), + outcome + ); + + Ok(outcome) + } + /// Dynamically compares the lookup table with another for equality. /// This is a deep comparison of the entries in the table. /// Returns true if the entries are equal, false otherwise. diff --git a/src/core/lookup/array_lookup_table_test.rs b/src/core/lookup/array_lookup_table_test.rs index c15431b..098b9a6 100644 --- a/src/core/lookup/array_lookup_table_test.rs +++ b/src/core/lookup/array_lookup_table_test.rs @@ -3,7 +3,9 @@ mod tests { use crate::core::model::direction::Direction; use crate::core::model::identity::Identity; use crate::core::testutil::fixtures::*; - use crate::core::{model, ArrayLookupTable, LinkOutcome, LookupTable, LOOKUP_TABLE_LEVELS}; + use crate::core::{ + model, ArrayLookupTable, LinkOutcome, LookupTable, RelinkOutcome, LOOKUP_TABLE_LEVELS, + }; use std::collections::HashMap; #[test] @@ -202,6 +204,126 @@ mod tests { assert!(result.is_err()); } + /// (a) try_relink is a no-op returning AlreadyConsistent when the entry already equals the + /// claimant, on both sides. + #[test] + fn test_try_relink_already_consistent() { + let lt = ArrayLookupTable::new(); + let existing = random_identity(); + lt.update_entry(existing, 0, Direction::Right).unwrap(); + lt.update_entry(existing, 0, Direction::Left).unwrap(); + let outcome = lt.try_relink(0, Direction::Right, existing).unwrap(); + assert_eq!(outcome, RelinkOutcome::AlreadyConsistent); + assert_eq!(lt.get_entry(0, Direction::Right).unwrap(), Some(existing)); + let outcome = lt.try_relink(0, Direction::Left, existing).unwrap(); + assert_eq!(outcome, RelinkOutcome::AlreadyConsistent); + assert_eq!(lt.get_entry(0, Direction::Left).unwrap(), Some(existing)); + } + + /// (b, Right) an existing right neighbor strictly between self and the claimant + /// (existing.id() < claimant.id()) forwards instead of relinking; table unchanged. + #[test] + fn test_try_relink_existing_between_forwards_right() { + let lt = ArrayLookupTable::new(); + let claimant_id = random_identifier(); + let claimant = Identity::new(claimant_id, random_membership_vector(), random_address()); + let existing_id = random_identifier_less_than(&claimant_id); + let existing = Identity::new(existing_id, random_membership_vector(), random_address()); + lt.update_entry(existing, 0, Direction::Right).unwrap(); + let outcome = lt.try_relink(0, Direction::Right, claimant).unwrap(); + assert_eq!(outcome, RelinkOutcome::Forward(existing)); + assert_eq!(lt.get_entry(0, Direction::Right).unwrap(), Some(existing)); + } + + /// (b, Left) an existing left neighbor strictly between self and the claimant + /// (existing.id() > claimant.id()) forwards instead of relinking; table unchanged. + #[test] + fn test_try_relink_existing_between_forwards_left() { + let lt = ArrayLookupTable::new(); + let claimant_id = random_identifier(); + let claimant = Identity::new(claimant_id, random_membership_vector(), random_address()); + let existing_id = random_identifier_greater_than(&claimant_id); + let existing = Identity::new(existing_id, random_membership_vector(), random_address()); + lt.update_entry(existing, 0, Direction::Left).unwrap(); + let outcome = lt.try_relink(0, Direction::Left, claimant).unwrap(); + assert_eq!(outcome, RelinkOutcome::Forward(existing)); + assert_eq!(lt.get_entry(0, Direction::Left).unwrap(), Some(existing)); + } + + /// (c) try_relink into an empty slot relinks with no eviction, on both sides. + #[test] + fn test_try_relink_empty_slot_relinks_with_no_eviction() { + let lt = ArrayLookupTable::new(); + let right_claimant = random_identity(); + let left_claimant = random_identity(); + let outcome = lt.try_relink(0, Direction::Right, right_claimant).unwrap(); + assert_eq!(outcome, RelinkOutcome::Relinked { evicted: None }); + assert_eq!( + lt.get_entry(0, Direction::Right).unwrap(), + Some(right_claimant) + ); + let outcome = lt.try_relink(0, Direction::Left, left_claimant).unwrap(); + assert_eq!(outcome, RelinkOutcome::Relinked { evicted: None }); + assert_eq!( + lt.get_entry(0, Direction::Left).unwrap(), + Some(left_claimant) + ); + } + + /// (d, Right) an existing right neighbor NOT strictly between self and the claimant + /// (existing.id() > claimant.id()) is evicted; get_entry afterward reflects the claimant. + #[test] + fn test_try_relink_existing_not_between_evicts_right() { + let lt = ArrayLookupTable::new(); + let claimant_id = random_identifier(); + let claimant = Identity::new(claimant_id, random_membership_vector(), random_address()); + let existing_id = random_identifier_greater_than(&claimant_id); + let existing = Identity::new(existing_id, random_membership_vector(), random_address()); + lt.update_entry(existing, 0, Direction::Right).unwrap(); + let outcome = lt.try_relink(0, Direction::Right, claimant).unwrap(); + assert_eq!( + outcome, + RelinkOutcome::Relinked { + evicted: Some(existing) + } + ); + assert_eq!(lt.get_entry(0, Direction::Right).unwrap(), Some(claimant)); + } + + /// (d, Left) an existing left neighbor NOT strictly between self and the claimant + /// (existing.id() < claimant.id()) is evicted; get_entry afterward reflects the claimant. + #[test] + fn test_try_relink_existing_not_between_evicts_left() { + let lt = ArrayLookupTable::new(); + let claimant_id = random_identifier(); + let claimant = Identity::new(claimant_id, random_membership_vector(), random_address()); + let existing_id = random_identifier_less_than(&claimant_id); + let existing = Identity::new(existing_id, random_membership_vector(), random_address()); + lt.update_entry(existing, 0, Direction::Left).unwrap(); + let outcome = lt.try_relink(0, Direction::Left, claimant).unwrap(); + assert_eq!( + outcome, + RelinkOutcome::Relinked { + evicted: Some(existing) + } + ); + assert_eq!(lt.get_entry(0, Direction::Left).unwrap(), Some(claimant)); + } + + /// (e) try_relink at an out-of-range level returns an error, matching the other + /// lookup-table accessors' bounds-checking behavior. + #[test] + fn test_try_relink_out_of_bound_level_errors() { + let lt = ArrayLookupTable::new(); + let claimant = random_identity(); + assert!(lt + .try_relink(LOOKUP_TABLE_LEVELS, Direction::Right, claimant) + .is_err()); + assert!(lt + .try_relink(LOOKUP_TABLE_LEVELS, Direction::Left, claimant) + .is_err()); + } + #[test] /// Test equality of lookup tables. /// The test will create two identical lookup tables and check if they are equal. diff --git a/src/core/lookup/mod.rs b/src/core/lookup/mod.rs index 87bb259..a7522d4 100644 --- a/src/core/lookup/mod.rs +++ b/src/core/lookup/mod.rs @@ -21,6 +21,27 @@ pub enum LinkOutcome { Forward(Identity), } +/// Outcome of a [`LookupTable::try_relink`] compare-then-act decision — the repair counterpart +/// of [`LinkOutcome`], returned when a node (the "claimant") checks or re-asserts a link it +/// believes it should already hold, rather than requesting a brand-new one. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum RelinkOutcome { + /// The slot already points to the claimant, so the pointer was never actually wrong: + /// nothing is written and no correction is needed. `try_link` has no equivalent outcome, + /// since a first-time candidate is never already installed. + AlreadyConsistent, + /// The table was left untouched: the carried identity is the existing entry at + /// `(level, direction)`, and it sits strictly between this node and the claimant on that + /// side — it is closer to the claimant's true position, so it is not this node's call to + /// make, and the repair check should be retried against that neighbor instead. + Forward(Identity), + /// The slot was empty, or its occupant was neither the claimant nor strictly between this + /// node and the claimant (the same comparison `try_link` uses): the claimant is installed + /// as the new entry, evicting whatever occupied the slot before (`evicted` is `None` if it + /// was empty). + Relinked { evicted: Option }, +} + /// LookupTable is the core view of Skip Graph node towards the network. pub trait LookupTable: Send + Sync { /// Update the entry at the given level and direction. @@ -83,6 +104,57 @@ pub trait LookupTable: Send + Sync { candidate: Identity, ) -> anyhow::Result; + /// Atomically decides whether `claimant` should become — or already is — the neighbor at + /// `(level, direction)`. + /// + /// This is the repair counterpart of [`Self::try_link`], and the two differ in what the + /// caller is asking: + /// + /// - `try_link`'s `candidate` is a node requesting a link **for the first time** (e.g. + /// joining) — it is never already installed, so the only choices are to accept it or + /// forward the request onward. + /// - `try_relink`'s `claimant` is a node **checking or re-asserting a link it believes it + /// should already hold**, run periodically by a background repair sweep to catch and fix + /// pointers that drifted out of sync — e.g. silently overwritten by a concurrent, otherwise + /// individually-correct `try_link` call landing on the same slot. Because the claimant may + /// already be correctly linked, there is a third possible outcome `try_link` has no use for. + /// + /// The decision is atomic and `direction` is receiver-owned, exactly as for `try_link` (see + /// its docs for those general rules and for the "strictly between" comparison, which is + /// identical here). Given the current entry at `(level, direction)`: + /// + /// - **it already equals `claimant`** — the pointer was never actually wrong: nothing is + /// written, [`RelinkOutcome::AlreadyConsistent`] is returned. This is what makes a repair + /// sweep over an already-healthy graph produce zero writes and zero further messages. + /// - **it sits strictly between this node and `claimant`** — that neighbor is closer to + /// `claimant`'s true position, so it is not this node's call to make: the table is left + /// untouched and [`RelinkOutcome::Forward`] carries that neighbor, for the caller to retry + /// the check against. + /// - **otherwise** (the slot is empty, or its occupant is neither `claimant` nor strictly + /// between this node and `claimant`) — `claimant` is installed as the new entry, and + /// [`RelinkOutcome::Relinked`] reports whatever was evicted (`None` if the slot was empty). + /// This method does not act on that eviction itself — it only reports it. The intent is + /// forward-looking: a future caller-side repair handler is expected to take the evicted + /// identity and issue a fresh check against *it*, so a fix can cascade outward and heal a + /// whole chain of stale pointers from a single triggering probe, not just the one slot + /// checked here. That handler does not yet exist in this codebase. + /// + /// # Preconditions + /// + /// Same as `try_link`: the lookup table has no notion of this node's own identifier, so + /// callers must ensure `claimant` actually belongs on the `direction` side before calling — + /// a violated precondition installs an out-of-order neighbor silently rather than erroring. + /// + /// # Errors + /// + /// returns an error when `level` is out of bounds. + fn try_relink( + &self, + level: LookupTableLevel, + direction: Direction, + claimant: Identity, + ) -> anyhow::Result; + /// Dynamically compares the lookup table with another for equality. fn equal(&self, other: &dyn LookupTable) -> bool; diff --git a/src/core/mod.rs b/src/core/mod.rs index 97800e0..d8177de 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -10,6 +10,7 @@ pub use crate::core::lookup::array_lookup_table::LOOKUP_TABLE_LEVELS; pub use crate::core::lookup::LinkOutcome; pub use crate::core::lookup::LookupTable; pub use crate::core::lookup::LookupTableLevel; +pub use crate::core::lookup::RelinkOutcome; pub use crate::core::model::address::Address; pub use crate::core::model::direction::Direction; pub use crate::core::model::identifier::Identifier; diff --git a/src/node/core_test.rs b/src/node/core_test.rs index 6b87c69..eab9e4e 100644 --- a/src/node/core_test.rs +++ b/src/node/core_test.rs @@ -8,7 +8,7 @@ use crate::core::testutil::fixtures::{ }; use crate::core::{ ArrayLookupTable, IdSearchReq, Identifier, LinkOutcome, LookupTable, LookupTableLevel, - LOOKUP_TABLE_LEVELS, + RelinkOutcome, LOOKUP_TABLE_LEVELS, }; use crate::node::core::{BaseCore, Core}; use anyhow::anyhow; @@ -369,6 +369,15 @@ fn test_search_by_id_error_propagation() { todo!() } + fn try_relink( + &self, + _: LookupTableLevel, + _: Direction, + _: Identity, + ) -> anyhow::Result { + todo!() + } + fn equal(&self, _: &dyn LookupTable) -> bool { todo!() }