Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 59 additions & 1 deletion src/core/lookup/array_lookup_table.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<RelinkOutcome> {
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.
Expand Down
124 changes: 123 additions & 1 deletion src/core/lookup/array_lookup_table_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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.
Expand Down
72 changes: 72 additions & 0 deletions src/core/lookup/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Identity> },
}

/// 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.
Expand Down Expand Up @@ -83,6 +104,57 @@ pub trait LookupTable: Send + Sync {
candidate: Identity,
) -> anyhow::Result<LinkOutcome>;

/// 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<RelinkOutcome>;

/// Dynamically compares the lookup table with another for equality.
fn equal(&self, other: &dyn LookupTable) -> bool;

Expand Down
1 change: 1 addition & 0 deletions src/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 10 additions & 1 deletion src/node/core_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -369,6 +369,15 @@ fn test_search_by_id_error_propagation() {
todo!()
}

fn try_relink(
&self,
_: LookupTableLevel,
_: Direction,
_: Identity,
) -> anyhow::Result<RelinkOutcome> {
todo!()
}

fn equal(&self, _: &dyn LookupTable) -> bool {
todo!()
}
Expand Down
Loading