[Core] Add try_link: atomic accept-or-forward slot decision

[TheP2P (thep2p)]

Closes #85: single-write-lock try_link/LinkOutcome on LookupTable + ArrayLookupTable, per docs/protocol/concurrent-insert.md §4.

[Copilot]

Pull request overview

Adds the try_link primitive to the core lookup-table abstraction to support the concurrent insert/join protocol (#85) with an atomic “accept-or-forward” decision per (level, direction) slot.

Changes:

• Introduces LinkOutcome and adds try_link to the LookupTable trait.
• Implements try_link in ArrayLookupTable using a single inner.write() critical section for compare+decision+conditional write.
• Adds unit tests covering direct-link, overwrite, forward-without-modification, and out-of-bounds error cases.

Reviewed changes

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

Show a summary per file

File	Description
src/node/core_test.rs	Updates test mock LookupTable impl to satisfy the new trait method signature.
src/core/model/identity.rs	Adds Eq to support LinkOutcome: Eq derivation.
src/core/model/address.rs	Adds Eq to support Identity: Eq derivation.
src/core/mod.rs	Re-exports LinkOutcome from core.
src/core/lookup/mod.rs	Adds LinkOutcome and the try_link API + rustdoc.
src/core/lookup/array_lookup_table.rs	Implements atomic try_link under a single write lock.
src/core/lookup/array_lookup_table_test.rs	Adds tests for both try_link outcomes and bounds error behavior.

Comments suppressed due to low confidence (1)

src/core/lookup/mod.rs:57

• Doc comment has a grammatical omission and inconsistent sentence casing: "Direction::Right this node's own right slot" is missing "is", and the next sentence starts with lowercase "an".

    /// `direction` is receiver-owned, never re-interpreted hop-to-hop: it always names this
    /// node's own slot (`Direction::Right` this node's own right slot, holding neighbors with
    /// larger identifiers; `Direction::Left` its own left slot), never something relative to a
    /// caller or hop. an existing entry sits strictly between this node and `candidate` when,

---

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

[TheP2P (thep2p)]

Claude (@claude) review

[claude]

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

---

Review: PR #99 — try_link atomic slot decision

• Read changed files
• Analyze concurrency correctness
• Analyze API design & tests
• Post review feedback

---

Overall this is a clean, well-scoped implementation that faithfully matches docs/protocol/concurrent-insert.md §4.3. The atomicity requirement — compare-then-decide-then-write inside a single inner.write() guard — is correctly satisfied, and the tests cover all four decision branches on both sides. Nice work.

✅ Correctness — the atomicity guarantee holds

array_lookup_table.rs:194-219: the entire compare-and-branch runs under one self.inner.write() acquisition (:194), never composing separately-locked get_entry/update_entry calls. This is exactly what §4.2 requires and closes the lost-update race described there. The cmp logic matches the spec:

• Right + existing.id() < candidate.id() → Forward (:206)
• Left + existing.id() > candidate.id() → Forward (:209)
• else (empty, or existing on far side) → insert + LinkedDirectly (:212-218)

Bounds check (:187) matches the other accessors. Tests confirm all four branches + OOB.

🟡 Minor: doc comment grammar/casing (already flagged by Copilot)

mod.rs:54-59 still has the issues Copilot noted:

• :55 — `Direction::Right` this node's own right slot is missing "is" → should read `Direction::Right` **is** this node's own right slot.
• :57 — sentence starts lowercase: "an existing entry sits..." → "An existing entry...".
• The rustdoc paragraphs at :46, :50 also start lowercase ("atomically decides...", "the decision is atomic..."), inconsistent with the surrounding capitalized rustdoc (:26, :34, etc.). The LinkOutcome variant docs (:13, :17) are likewise lowercase.

Consistency-only; not blocking. Since commit 1edb313 already touched doc capitalization, worth a follow-up pass here too.

🟡 Minor: test-fixture edge case (non-blocking, pre-existing pattern)

array_lookup_table_test.rs:125-128 (and the three sibling tests) do:

let candidate_id = random_identifier();
let existing_id = random_identifier_greater_than(&candidate_id);

random_identifier_greater_than panics if candidate_id == MAX, and random_identifier_less_than panics if candidate_id == ZERO (see fixtures.rs). With a 32-byte random id the probability is negligible, so this is not a practical flake — just noting the theoretical edge. No change required unless you want hard determinism.

💡 Suggestions (optional)

1. Concurrency test for try_link itself. The existing test_randomized_concurrent_operations_with_validation covers update/get/remove but not try_link. Since try_link's whole reason for existing is atomicity under contention, a test spawning N threads racing try_link on the same (level, direction) slot and asserting exactly one LinkedDirectly winner (others Forward) would directly exercise the guarantee. The current tests are all single-threaded. Fine to defer, but it's the highest-value follow-up.
2. Eq derivations on Identity/Address (identity.rs, address.rs) are correct and minimal to support LinkOutcome: Eq. Good — just confirm no float/f* fields exist on those types (they don't, from the diff).

Verification note

I was unable to run cargo build/cargo test (the cargo command isn't in my allowed tools for this run), so correctness above is by inspection only. If you'd like me to compile/test, please add Bash(cargo build:*) / Bash(cargo test:*) to --allowedTools.

--- • branch thep2p/85-try-link

[TheP2P (thep2p)]

Fixed the doc grammar/casing issues (dropped "is" at mod.rs:55, remaining lowercase paragraph starts) in 80a1ec2. Test-fixture panic edge case and Eq derivations: agreed, no action needed. Concurrency test for try_link itself: intentionally deferred to #97.

[S T (staheri14)]

LGTM, added a non-blocking comment.

[S T (staheri14)]

This accept-vs-forward decision compares only existing.id() vs candidate.id(); it can't check candidate against this node's id, because the lookup table doesn't store it. So it silently trusts the caller to pass a candidate on the correct side of this node for direction (Right ⇒ candidate.id() > self.id(), Left ⇒ <).

If a caller violates that (e.g. a candidate.id() < self.id() passed into a Right slot), this comparison is meaningless and an out-of-order neighbor is installed silently — table corruption rather than an error.

Suggest a /// precondition line on try_link, and optionally a debug_assert! at the call site (where self's id is known) so misuse fails loudly in tests instead of silently corrupting the table. Non-blocking.

[TheP2P (thep2p)]

Good catch, thanks. Confirmed against docs/protocol/concurrent-insert.md §4.1/§4.3: the spec's own change_neighbor pseudocode has the same shape — it only ever compares the existing entry against the candidate, never the candidate against self. So try_link is deliberately a low-level, trusted primitive; the caller (the not-yet-written GetLinkOp/BuddyOp handler) owns the "candidate is on the correct side" guarantee.

Added a # Preconditions doc section on the trait in 731a077 spelling this out. Held off on the debug_assert! — there's no call site yet (this PR only adds the primitive, nothing wires it in), and it can't live inside try_link itself since ArrayLookupTable has no notion of its own owner's id by design. Makes sense to add the assert at the actual call site once the handler lands.