feat: concurrent originator searches via per-request waiter map#69
Open
thep2p wants to merge 4 commits into
Open
feat: concurrent originator searches via per-request waiter map#69thep2p wants to merge 4 commits into
thep2p wants to merge 4 commits into
Conversation
Introduce a RequestId newtype (random u128) carried by every IdSearchReq. The originator mints a fresh id; relay hops forward it unchanged so a returning response can later be matched to the exact parked caller that started the search. This is the groundwork for replacing BaseNode's single-slot waiter with a per-request map. No behavioral change yet: the id rides along but is not consumed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace BaseNode's single-slot waiter (Option<SyncSender>) with a HashMap<RequestId, SyncSender<IdSearchRes>>, letting a node originate multiple concurrent searches without one clobbering another. The originator parks its sender keyed by the search's RequestId; the returning IdSearchResponse looks the waiter up by the id it carries back and removes it before delivering. IdSearchRes now carries the RequestId (copied from the request by the terminating node) so responses can be matched to their parked caller. Bound the end-to-end integration test with join_with_timeout so a future regression fails loudly instead of deadlocking the test runner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fire eight simultaneous search_by_id calls from a single node (eight shallow clones sharing one waiter map) for eight distinct targets, and assert each thread receives its own correct result. This exercises the per-request waiter map under contention — the old single-slot waiter would race here, delivering a response to the wrong parked caller. Bounded by join_all_with_timeout so a regression fails instead of hanging the runner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
If send_event fails after the originator has parked its sender, remove the now-orphaned map entry before returning the error. Without this, every failed relay would leave a permanent HashMap entry (an unbounded leak, vs. the old single-slot design's bounded-at-one stale sender). Also document the implemented/next roadmap in the README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the “single in-flight search” limitation for distributed search_by_id by introducing a per-request RequestId that is carried on IdSearchReq/IdSearchRes, and by replacing BaseNode’s single waiter slot with a HashMap<RequestId, SyncSender<...>> so multiple concurrent originator searches can be awaited safely.
Changes:
- Added
RequestIdand threaded it throughIdSearchReq/IdSearchResso responses can be matched to the correct originator call. - Replaced
BaseNode’s single waiter slot with arequest_id_mapkeyed byRequestId, with cleanup on relay send failure. - Updated/added tests to generate per-request IDs and validate concurrent search behavior; added roadmap documentation and trimmed unused deps.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/node/base_node.rs | Replaces the single waiter slot with a HashMap<RequestId, SyncSender<IdSearchRes>> and matches responses by request id. |
| src/core/model/search.rs | Introduces RequestId and adds request-id fields/accessors to search request/response types. |
| src/node/core.rs | Ensures IdSearchRes includes the originating RequestId when produced by the core search. |
| src/node/core_test.rs | Updates unit tests to include RequestId on all constructed IdSearchReqs. |
| src/node/search_by_id_test.rs | Updates networking integration tests to include RequestId on requests. |
| src/node/skip_graph_integration_test.rs | Adds a concurrent-originator test and joins threads with timeouts to avoid hangs. |
| README.md | Adds a roadmap section reflecting implemented work and next steps. |
| Cargo.toml | Removes unused dependencies. |
Comments suppressed due to low confidence (1)
src/core/model/search.rs:71
- The
IdSearchResdoc comment describes only(target, termination_level, result), but the struct now also includes arequest_id. This makes the docs inaccurate for readers and API users.
/// A struct representing the result of an identifier search within lookup table of current node.
///
/// The `IdentifierSearchResult` struct is composed of three key components:
/// - The `target` identifier that was searched for in the lookup table of the current node.
/// - The `termination_level` of the lookup table where the identifier search was terminated at the current node.
/// - The `result` identifier that was found during the search process at the current node.
///
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
99
to
102
| { | ||
| let mut slot = self.ch.lock().expect("mutex was poisoned by a previous panic"); | ||
| *slot = Some(tx); | ||
| let mut request_id_map = self.request_id_map.lock().expect("mutex was poisoned by a previous panic"); | ||
| request_id_map.insert(req.req_id(), tx); | ||
| } |
Comment on lines
94
to
98
| /// | ||
| /// Returns a new `IdentifierSearchResult` instance populated with the provided `target`, `level`, | ||
| /// and `result` parameters. | ||
| pub fn new(target: Identifier, level: LookupTableLevel, result: Identifier) -> Self { | ||
| pub fn new(request_id: RequestId, target: Identifier, level: LookupTableLevel, result: Identifier) -> Self { | ||
| IdSearchRes { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the single-in-flight-search limitation from #68.
Each
IdSearchReq/IdSearchResnow carries a randomRequestId.BaseNodeparks waiters in aHashMap<RequestId, SyncSender>instead of oneOptionslot, so a node can originate many searches at once without one clobbering another. Responses are matched back to the correct caller by the id they carry home.RequestIdsend_eventfails (no unbounded leak)join_with_timeoutguards so a regression fails loudly instead of hanging🤖 Generated with Claude Code