[fix][core] Single-lock max_level computation by thep2p · Pull Request #129 · mathcrypt-research/skipgraph-rust

TheP2P (thep2p) · 2026-08-27T23:02:25Z

Closes #127.

Adds LookupTable::max_populated_level, a single-lock query, and switches BaseCore::max_level to use it instead of composing left_neighbors/right_neighbors under two separate read locks.

Two deliberate deviations from the issue's sketch, both narrowing rather than widening the API surface:

max_populated_level returns Option<LookupTableLevel>, not anyhow::Result<Option<LookupTableLevel>> as sketched in the issue. It takes no level argument to bounds-check, and parking_lot::RwLock::read() doesn't poison, so it structurally can't fail, the Result wrapper would have been dead code.
left_neighbors/right_neighbors got the same treatment for the same reason. Since max_level no longer calls them, they now have zero production callers left (test fixtures only).

Core::max_level keeps its existing anyhow::Result signature, that's a broader trait contract predating this PR and out of scope here. BaseCore's implementation of it just no longer has a way to fail, so the prior test_max_level_error_propagation test was removed, there's no failure path left to exercise.

…k-max-level # Conflicts: # src/node/repair_schedule.rs

Copilot

Pull request overview

This PR addresses a concurrency/consistency gap in BaseCore::max_level by introducing a single-lock lookup-table query (LookupTable::max_populated_level) and switching max_level to use it, avoiding separate reads of left/right neighbor state.

Changes:

Add LookupTable::max_populated_level and implement it for ArrayLookupTable using a single inner.read() snapshot.
Update BaseCore::max_level to use max_populated_level rather than combining left_neighbors/right_neighbors.
Adjust left_neighbors/right_neighbors APIs (and update affected tests) to remove Result wrapping.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/node/search_by_id_test.rs	Updates tests to reflect `left_neighbors()` now returning a `Vec` directly.
src/node/core.rs	Switches `max_level()` implementation to use `max_populated_level()`.
src/node/core_test.rs	Updates neighbor-based expectations and removes the prior `max_level` error-propagation test.
src/core/lookup/mod.rs	Changes `LookupTable` neighbor APIs and adds `max_populated_level()` to the trait.
src/core/lookup/array_lookup_table.rs	Implements `max_populated_level()` under a single read lock and adjusts neighbor methods to be infallible.
src/core/lookup/array_lookup_table_test.rs	Updates neighbor tests and adds coverage for `max_populated_level()` behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

TheP2P (thep2p) · 2026-08-29T22:08:21Z

    /// 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<Vec<(usize, Identity)>>;
+    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<Vec<(usize, Identity)>>;
+    fn right_neighbors(&self) -> Vec<(usize, Identity)>;


Fair point, this is technically a breaking change for the pub trait. Given it, the crate isn't published (no license/repository metadata in Cargo.toml) and is still 0.1.0, I'm treating pre-1.0 API churn as acceptable rather than preserving a Result no implementation currently needs. Happy to revisit if this crate is headed for publication with external implementors in mind.

TheP2P (thep2p) · 2026-08-29T22:08:28Z

+    ///
+    /// `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.
+    fn max_populated_level(&self) -> Option<LookupTableLevel>;


Intentional deviation, not an oversight. Result was dead weight here (no level arg to bounds-check, RwLock::read() doesn't poison), so I dropped it rather than keep a wrapper that could never be Err. Called this out explicitly in the PR description now.

TheP2P (thep2p) · 2026-08-29T22:08:34Z

    fn max_level(&self) -> anyhow::Result<LookupTableLevel> {
-        let left = 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()
-            .unwrap_or(0))
+        Ok(self.lt.max_populated_level().unwrap_or(0))
    }


Core::max_level's # Errors section documents the trait's general contract (a future Core impl could genuinely fail reading its own storage), not a guarantee this specific implementation exercises it, that signature predates this PR and is out of scope here. BaseCore's own impl just has no failure path left, so the old error-propagation test was removed since there's nothing left for it to exercise. No doc change needed on the trait itself.

TheP2P (thep2p) · 2026-08-29T22:16:18Z

Claude (@claude) review

claude · 2026-08-29T22:16:30Z

Claude finished TheP2P (@thep2p)'s task —— View job

Code Review: Single-lock `max_level` computation

Read changed files
Analyze concurrency fix correctness
Review API changes and test coverage
Post review feedback

Reviewed all 6 changed files. This is a clean, well-motivated fix. The core change is correct and the reasoning in the PR description and review replies is sound.

Correctness ✅

max_populated_level (array_lookup_table.rs:319-325) is the heart of the fix and it's right:

fn max_populated_level(&self) -> Option<LookupTableLevel> {
    let inner = self.inner.read();
    (0..LOOKUP_TABLE_LEVELS)
        .rev()
        .find(|&level| inner.left[level].is_some() || inner.right[level].is_some())
}

Single inner.read() guard means both sides are inspected against one consistent snapshot — this genuinely closes the two-lock inconsistency window that the old left_neighbors/right_neighbors composition had.
.rev().find(...) short-circuits at the highest populated level; O(levels) worst case but stops early, strictly better than the old approach that allocated two Vecs and iterated both fully.
None vs Some(0) distinction is preserved and its ambiguity with a level-0-only table is documented at core.rs:41-45. The unwrap_or(0) at core.rs:207 correctly collapses None → 0 for the join-bootstrap contract.

API changes — I agree with your calls

Dropping Result on max_populated_level: Correct. No level arg to bounds-check and parking_lot::RwLock::read() doesn't poison, so Result would be structurally uninhabitable on the Err side. Keeping it would be dead code.
Dropping Result on left_neighbors/right_neighbors: Same reasoning holds. Your pre-1.0 / unpublished-crate justification for the breaking trait change is reasonable. Copilot's concern is valid in principle but out of scope for a 0.1.0 internal crate.
Core::max_level keeping anyhow::Result: Agreed this is the right boundary — it's a trait-level contract for future Core impls with fallible storage, and BaseCore's specific impl simply having no failure path doesn't change the trait's general contract. The # Errors docs at core.rs:47-51 remain accurate as a trait contract.

Test coverage ✅

Good coverage on the new method in array_lookup_table_test.rs:587-622:

empty → None
one side populated
both sides at different levels → higher wins
level-0-only → Some(0) (guards the ambiguity boundary)

And core_test.rs retains test_max_level_empty_table / _one_side_populated / _both_sides_populated_different_levels. Removing test_max_level_error_propagation is consistent — there's no failure path left in BaseCore::max_level to exercise, and search_by_id's error-propagation test (core_test.rs:348-363) still covers the mock-error path for the trait that does have one.

Minor observations (non-blocking)

left_neighbors/right_neighbors now have zero production callers (test fixtures only), as you noted. docs/protocol/concurrent-insert.md:402-408 still references them for a future Stage-2 range-linking use case, so keeping them is justified rather than dead code to delete. Worth a one-line doc/comment noting they're retained for that planned use, but not required.
The two methods now duplicate identical iteration logic differing only in inner.left vs inner.right. Trivial, and not worth a shared helper given they may diverge — just noting it.
Consider whether max_populated_level deserves a concurrency-focused test (a writer racing the reader asserting no torn snapshot), mirroring the existing test_randomized_concurrent_operations_with_validation. The single-lock invariant is the whole point of this PR, and it's currently only covered by single-threaded tests. Not blocking — the lock guarantees it structurally — but it would lock in the contract against future refactors.

Verdict: LGTM. The fix is correct, minimal, and the API narrowing is well-justified for a pre-1.0 internal crate. Only the concurrency-test suggestion (#3) is worth considering before merge.
• Branch: thep2p/127-single-lock-max-level

S T (staheri14)

👍🏻

TheP2P (thep2p) added 11 commits August 25, 2026 10:27

[feat][node] Add RepairSchedule trait with tokio and manual impls

95bc3cc

[fix][node] Fix doc grammar and assert interval actually waits

90ca56d

[improve][test] Explain premature-tick timeout

749ad5e

[improve][node] Document ManualRepairSchedule fire coalescing

37a8089

Merge branch 'main' into thep2p/87-repair-schedule

fc3e9a8

[improve][test] Bound first-tick wait with a timeout

b9ff3ed

[cleanup][node] Consolidate dead_code allows

df1780b

[fix][core] Single-lock max_level computation

e536718

Merge remote-tracking branch 'origin/main' into thep2p/127-single-loc…

b0fc637

…k-max-level # Conflicts: # src/node/repair_schedule.rs

[cleanup][core] Unwrap infallible lookup result

7e93884

[cleanup][core] Drop dead Result from neighbor lookups

58a11a9

TheP2P (thep2p) requested a lite review from Copilot August 29, 2026 20:27

Copilot started reviewing on behalf of TheP2P (thep2p) August 29, 2026 20:28 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

TheP2P (thep2p) requested a review from S T (staheri14) August 29, 2026 22:56

[improve][core] Explain neighbor lookup retention

b713662

S T (staheri14) approved these changes Aug 30, 2026

View reviewed changes

TheP2P (thep2p) merged commit a4185ae into main Sep 1, 2026
6 checks passed

TheP2P (thep2p) deleted the thep2p/127-single-lock-max-level branch September 1, 2026 00:50

Navigation Menu

Uh oh!

[fix][core] Single-lock max_level computation - #129

[fix][core] Single-lock max_level computation#129
TheP2P (thep2p) merged 12 commits into
mainfrom
thep2p/127-single-lock-max-level

TheP2P (thep2p) commented Aug 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

TheP2P (thep2p) Aug 29, 2026

Uh oh!

TheP2P (thep2p) Aug 29, 2026

Uh oh!

TheP2P (thep2p) Aug 29, 2026

Uh oh!

TheP2P (thep2p) commented Aug 29, 2026

Uh oh!

claude Bot commented Aug 29, 2026 •

edited

Loading

Uh oh!

S T (staheri14) left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheP2P (thep2p) commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

TheP2P (thep2p) Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

TheP2P (thep2p) Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

TheP2P (thep2p) Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

TheP2P (thep2p) commented Aug 29, 2026

Uh oh!

claude Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: Single-lock max_level computation

Correctness ✅

API changes — I agree with your calls

Test coverage ✅

Minor observations (non-blocking)

Uh oh!

S T (staheri14) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheP2P (thep2p) commented Aug 27, 2026 •

edited

Loading

claude Bot commented Aug 29, 2026 •

edited

Loading

Code Review: Single-lock `max_level` computation