[feat][core] Add try_relink repair primitive

[TheP2P (thep2p)]

Adds try_relink/RelinkOutcome to LookupTable, the repair counterpart to try_link (#93). Single write-lock critical section; closes #93.

[Copilot]

Pull request overview

This PR adds a repair-oriented atomic primitive, try_relink, to the LookupTable API (with RelinkOutcome) and implements it for ArrayLookupTable using a single write-lock critical section, mirroring the existing try_link atomicity guarantees. It also adds targeted unit tests that cover all acceptance-criteria outcomes and updates re-exports / test mocks to compile with the expanded trait.

Changes:

• Add RelinkOutcome and LookupTable::try_relink (repair counterpart to try_link) to the core lookup API.
• Implement try_relink in ArrayLookupTable using a single inner.write() guard and return the evicted neighbor when replacing.
• Add unit tests covering AlreadyConsistent, Forward, Relinked (with/without eviction), and out-of-range errors.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/node/core_test.rs	Updates a test mock LookupTable impl to include the new try_relink method.
src/core/mod.rs	Re-exports RelinkOutcome from the core module.
src/core/lookup/mod.rs	Adds RelinkOutcome and extends the LookupTable trait with try_relink + documentation.
src/core/lookup/array_lookup_table.rs	Implements try_relink under a single write lock and logs the decision.
src/core/lookup/array_lookup_table_test.rs	Adds tests for all try_relink outcomes and bounds-check errors.

Comments suppressed due to low confidence (1)

src/core/lookup/mod.rs:134

• The try_relink docs describe the "otherwise" case as an occupant being "farther from claimant than this node is", but the implementation (and try_link docs) are based on the strictly-between comparison (existing.id() < claimant.id() for Right, > for Left). Rewording this avoids implying a distance metric that isn’t enforced.

    /// - **otherwise** (the slot is empty, or its occupant sits on the wrong side, farther from
    ///   `claimant` than this node is) — `claimant` is installed as the new entry, and
    ///   [`RelinkOutcome::Relinked`] reports whatever was evicted (`None` if the slot was empty).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[TheP2P (thep2p)]

Good catch, fixed in 84c3158 — reworded both spots (this one and line 132's trait doc bullet) to describe the actual ordinal condition ("neither the claimant nor strictly between this node and the claimant, per the same comparison try_link uses") instead of the inaccurate distance claim.

[TheP2P (thep2p)]

Claude (@claude) review

[claude]

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

---

Code Review — PR #102 try_relink repair primitive

• Read repo guidelines (no CLAUDE.md present)
• Review array_lookup_table.rs implementation
• Review mod.rs trait + RelinkOutcome
• Review tests
• Review re-exports & mock updates
• Post review feedback

---

Overall this is a clean, well-scoped PR. try_relink faithfully mirrors try_link's single-write-lock atomicity, the three-way decision is correct, the docs are excellent, and the tests cover every branch on both sides. Approve-in-spirit — nothing blocking. A few notes below.

Correctness ✅

The match arm ordering in array_lookup_table.rs:262-277 is right and matters:

1. existing == claimant → AlreadyConsistent (must come first, so an already-linked claimant is never treated as "not strictly between" and needlessly re-written).
2. strictly-between → Forward (same per-direction comparison as try_link).
3. catch-all (evicted, _) → Relinked { evicted }, where evicted is the Option read at line 254-257, so empty-slot yields evicted: None naturally. Neat.

The single inner.write() guard (line 252) preserves the compare-decide-write atomicity, matching the rationale documented for try_link. Good.

Observations / minor suggestions (non-blocking)

1. Ordering-tie edge case in docs. The Forward arms use strict < / >, so an occupant whose id() equals the claimant's id() but is a different Identity (different membership vector / address) falls through to the catch-all and gets evicted/relinked, not forwarded. AlreadyConsistent requires full Identity equality (existing == claimant), so this is internally consistent — but it's a subtle case (equal identifiers, unequal identities). Worth a one-line doc note if identifier collisions are considered possible in this system; ignore if identifiers are guaranteed unique.

2. Duplicated decision logic. try_link (lines 205-219) and try_relink (lines 262-277) now share the identical strictly-between comparison, copy-pasted. Not worth refactoring for two call sites, but if a third primitive appears, consider extracting a small fn strictly_between(existing, claimant, direction) -> bool helper to keep the comparison defined once.

3. Doc wording fix from Copilot review already applied in 84c3158 — mod.rs:38-42 and 133-135 now describe the actual ordinal condition ("neither claimant nor strictly between…") rather than a distance metric. Confirmed correct.

4. Mock in core_test.rs:372-379 correctly adds try_relink returning todo!(), consistent with the existing try_link mock. Fine — that path is never exercised by these tests.

Test coverage ✅

array_lookup_table_test.rs:207-325 covers all five outcomes on both directions: AlreadyConsistent, Forward (L/R), Relinked empty (no eviction), Relinked with eviction (L/R), and out-of-bounds error. Post-conditions (get_entry reflects the write / non-write) are asserted throughout. Thorough and mirrors the try_link test structure.

One optional gap: no test asserts the equal-identifier-but-different-identity → eviction case from note (1). If that scenario is reachable, a test would lock in the intended behavior.

Note on verification

I was unable to run cargo build / cargo test in this session (the cargo command isn't in my allowed tools). The review above is from static analysis only. If you'd like me to actually compile and run the try_relink tests, please add Bash(cargo test:*) / Bash(cargo build:*) to --allowedTools.
• Branch: thep2p/93-try-relink

[TheP2P (thep2p)]

Re the identifier-collision edge case (note 1): identifiers are guaranteed unique in this system, so that path is unreachable — no doc or test change needed. Note 2 (extracting the shared comparison) — agreed, not worth it for two call sites.

[S T (staheri14)]

Looks good!