iroh-lan improvements#5
Merged
Merged
Conversation
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModularizes the previous inline authentication into four modules ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Auth as Authenticator
participant Endpoint as iroh::Endpoint
participant Remote as RemotePeer
participant Worker as AuthWorker
rect rgba(200, 230, 255, 0.5)
Note over Client,Auth: Connect-time registration and gated handshake
end
Client->>Auth: before_connect(remote_id, alpn)
Auth->>Auth: register_in_flight(remote_id)
Auth-->>Endpoint: spawn_auth_worker(remote_id) (if endpoint set)
Auth->>Client: provisional accept/reject
Client->>Remote: perform handshake
Remote->>Auth: accept(connection) -> auth_accept(connection)
Auth->>Worker: background auth_open -> SPAKE2 exchange
Worker->>Remote: open stream, exchange derived keys
Worker->>Auth: update auth_state -> Authenticated / Blocked / Unauthenticated
Endpoint->>Auth: after_handshake(conn_info)
Auth->>Auth: get_auth_state(remote_id) / wait watcher (AUTH_TIMEOUT)
Auth-->>Endpoint: accept or reject handshake
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Around line 913-915: The test is configuring an external relay via
RelayMap::try_from_iter(["https://iroh-relay.rustonbsd.com"]) which creates an
external network dependency; remove or replace that by either (a) not calling
relay_mode at all (rely on default/local transports) or (b) using a local/test
relay address, or (c) mark the test as #[ignore] to make it an
integration/stress test; update the configuration around
clear_relay_transports(), relay_mode(...), and hooks(auth_a.clone()) accordingly
so unit tests do not contact the external URL.
- Around line 294-300: The current release_in_flight implementation (using
self.in_flight.lock(), .entry(endpoint_id).or_insert_with(||
WatchableEndpoint::new(None)).set(None)) leaves stale HashMap entries and even
creates new ones; change it to remove the map entry instead of inserting and
setting to None: lock self.in_flight, try to remove the endpoint_id (e.g.,
in_flight.remove(&endpoint_id)), and if a watcher was returned call
watcher.set(None) on it. Replace all similar patterns (the block, timeout paths,
and after_handshake timeout/reject paths that use
.entry(...).or_insert_with(...).set(None)) with this removal helper so no
orphaned entries are left and no new entries are created.
- Around line 907-911: The ALPN loop is wrong: endpoint_a_builder.alpns(...)
replaces the list each call so only the last "/dummy/{i}" is registered; instead
build a Vec of all ALPN byte vectors (e.g., map 0..parallel_count to
format!("/dummy/{}", i).into_bytes()), then call
endpoint_a_builder.alpns(all_alpns) exactly once (look for endpoint_a_builder
and .alpns usage). Also update the connection handling (where results are
currently silenced with .ok()) to collect each connection Result into a Vec,
assert success (e.g., expect or assert_ok) for every entry so failures are not
discarded — adjust the code paths around parallel_count connection spawns to
push results and verify them.
- Around line 798-808: The test currently spawns tasks with tokio::spawn and
waits on a shared atomic `worked`, which can hang forever if a spawned task
panics; instead collect the JoinHandles (or use tokio::task::JoinSet) for the
tasks created in the loop that call `run_auth_parallel_test(secret, secret,
count)`, await them all (checking each handle’s Result to detect panics/errors)
and wrap the entire wait/join phase in a `tokio::time::timeout(...)` so the test
fails fast on panics or non-termination; update the loop that creates tasks (the
block using `tokio::spawn` and `worked`) to push handles into a Vec or JoinSet
and then await/join them under a timeout rather than polling `worked`
indefinitely.
- Around line 562-584: Replace the Option<PublicKey> in the in-flight watcher
with an explicit AuthState enum (e.g., enum AuthState { Pending,
Authenticated(PublicKey) }) and initialize the watcher with AuthState::Pending
instead of WatchableEndpoint::new(None) where the entry is created in the
in_flight map; update any code that previously called
set(Some(pubkey))/set(None) (including release_in_flight) to set
AuthState::Authenticated(public_key) on success and remove the entry (or set a
terminal state) on timeout/reject so notifications occur reliably; finally
update the wait_for_auth async waiter (the block that reads
in_flight_watcher.watch().stream() and checks
in_flight/is_authenticated(&endpoint_id)) to treat AuthState::Authenticated(_)
as completion and AuthState::Pending as waiting.
- Around line 317-331: The final stream drain uses recv.read_to_end(usize::MAX)
which allows an untrusted peer to force unbounded buffering; change the drain to
(1) use a small bounded maximum (e.g., a few KB) instead of usize::MAX and (2)
wrap the await in a timeout using TRANSMISSION_TIMEOUT so the task fails if the
peer keeps sending; preserve the existing error mapping to
AuthenticatorError::OpenFailed/AcceptFailed and keep the warn!("[end_of_auth]
{}", err) path so timeouts or size-limit errors are logged consistently when
recv.read_to_end (or its replacement) returns an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a0cabf5-fe4e-44e2-9142-7aba63a5b79b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlREADME.mdsrc/lib.rs
Comment on lines
+798
to
+808
| for _ in 0..count { | ||
| let worked = worked.clone(); | ||
| tokio::spawn(async move { | ||
| assert!(run_auth_parallel_test(secret, secret, count).await.unwrap()); | ||
| worked.fetch_add(1, std::sync::atomic::Ordering::SeqCst); | ||
| }); | ||
| } | ||
|
|
||
| while worked.load(std::sync::atomic::Ordering::SeqCst) < count { | ||
| tokio::time::sleep(Duration::from_millis(100)).await; | ||
| } |
There was a problem hiding this comment.
Join spawned tasks under a timeout.
A panic inside any spawned task is never observed, and worked will stop incrementing, causing this test to loop forever. Collect the join handles or use JoinSet, then wrap the whole wait in a timeout.
🧪 Proposed direction
- for _ in 0..count {
+ let mut tasks = tokio::task::JoinSet::new();
+ for _ in 0..count {
let worked = worked.clone();
- tokio::spawn(async move {
- assert!(run_auth_parallel_test(secret, secret, count).await.unwrap());
+ tasks.spawn(async move {
+ let result = run_auth_parallel_test(secret, secret, count).await;
+ assert!(result.unwrap());
worked.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
});
}
- while worked.load(std::sync::atomic::Ordering::SeqCst) < count {
- tokio::time::sleep(Duration::from_millis(100)).await;
- }
+ timeout(AUTH_TIMEOUT * 4, async {
+ while let Some(result) = tasks.join_next().await {
+ result.unwrap();
+ }
+ })
+ .await
+ .expect("parallel auth test timed out");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib.rs` around lines 798 - 808, The test currently spawns tasks with
tokio::spawn and waits on a shared atomic `worked`, which can hang forever if a
spawned task panics; instead collect the JoinHandles (or use
tokio::task::JoinSet) for the tasks created in the loop that call
`run_auth_parallel_test(secret, secret, count)`, await them all (checking each
handle’s Result to detect panics/errors) and wrap the entire wait/join phase in
a `tokio::time::timeout(...)` so the test fails fast on panics or
non-termination; update the loop that creates tasks (the block using
`tokio::spawn` and `worked`) to push handles into a Vec or JoinSet and then
await/join them under a timeout rather than polling `worked` indefinitely.
Comment on lines
+907
to
+911
| let mut endpoint_a_builder = iroh::Endpoint::builder(iroh::endpoint::presets::N0); | ||
| for i in 0..parallel_count { | ||
| endpoint_a_builder = | ||
| endpoint_a_builder.alpns(vec![format!("/dummy/{}", i).into_bytes()]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
iroh 0.97.0 Endpoint builder alpns method accumulate or replace
💡 Result:
In iroh 0.97.0, the EndpointBuilder's alpns method sets (replaces) the list of ALPN protocols that the endpoint will accept on incoming connections. It takes a Vec<Vec> and overrides any previously set ALPNs. There is no built-in "accumulate" behavior; calling alpns multiple times will replace the list each time. To accumulate ALPNs, retrieve the current list (if the Builder exposed it publicly, which it does not in the current API), append new ones, and set again—or construct the full list in one call. To accept incoming connections, at least one ALPN must be set. Not setting any allows outbound connections but not inbound. Example usage: let endpoint = Endpoint::builder(presets::N0) .alpns(vec![b"my-protocol".to_vec, b"another".to_vec]) .bind .await?; Source code confirms alpn_protocols: Vec<Vec> is directly replaced in the builder struct.
Citations:
- 1: https://docs.rs/iroh/latest/iroh/endpoint/struct.Builder.html
- 2: https://github.com/n0-computer/iroh/blob/3d6e26b1/iroh/src/endpoint.rs
- 3: n0-computer/iroh@759f14e
- 4: https://docs.rs/iroh-net/latest/iroh_net/endpoint/struct.Builder.html
Fix ALPN loop: builder.alpns() replaces, not accumulates—only the last ALPN is registered.
The current loop repeatedly calls .alpns(), which overwrites the previous ALPN list each iteration. This leaves only the final /dummy/{parallel_count-1} ALPN registered. Parallel connections attempting other ALPNs will fail to negotiate. Build the complete ALPN list once and call .alpns() once. Additionally, collect and assert every connection result instead of discarding with .ok().
🧪 Proposed direction
- let mut endpoint_a_builder = iroh::Endpoint::builder(iroh::endpoint::presets::N0);
- for i in 0..parallel_count {
- endpoint_a_builder =
- endpoint_a_builder.alpns(vec![format!("/dummy/{}", i).into_bytes()]);
- }
+ let alpns: Vec<Vec<u8>> = (0..parallel_count)
+ .map(|i| format!("/dummy/{}", i).into_bytes())
+ .collect();
+ let endpoint_a_builder =
+ iroh::Endpoint::builder(iroh::endpoint::presets::N0).alpns(alpns.clone());- for i in 0..parallel_count {
+ let mut connects = tokio::task::JoinSet::new();
+ for i in 0..parallel_count {
tokio::time::sleep(Duration::from_millis(100)).await;
- spawn({
+ connects.spawn({
let endpoint_a = endpoint_a.clone();
let endpoint_b = endpoint_b.clone();
async move {
+ let alpn = format!("/dummy/{}", i).into_bytes();
endpoint_a
- .connect(
- endpoint_b.addr(),
- format!("/dummy/{}", i).into_bytes().as_slice(),
- )
+ .connect(endpoint_b.addr(), alpn)
.await
- .ok();
}
});
}
+
+ while let Some(result) = connects.join_next().await {
+ result.map_err(|e| e.to_string())?
+ .map_err(|e| e.to_string())?;
+ }Also applies to: 923-926, 954-999
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib.rs` around lines 907 - 911, The ALPN loop is wrong:
endpoint_a_builder.alpns(...) replaces the list each call so only the last
"/dummy/{i}" is registered; instead build a Vec of all ALPN byte vectors (e.g.,
map 0..parallel_count to format!("/dummy/{}", i).into_bytes()), then call
endpoint_a_builder.alpns(all_alpns) exactly once (look for endpoint_a_builder
and .alpns usage). Also update the connection handling (where results are
currently silenced with .ok()) to collect each connection Result into a Vec,
assert success (e.g., expect or assert_ok) for every entry so failures are not
discarded — adjust the code paths around parallel_count connection spawns to
push results and verify them.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
681-702:⚠️ Potential issue | 🟡 MinorBlocked peers should reject with 403, not 401.
When
wait_for_authcompletes because the state transitions toAuthState::Blocked,is_authenticatedreturnsfalseand this branch rejects withVarInt::from_u32(401)+"Authentication failed", even though a few lines above (line 652) the explicitBlockedbranch correctly uses 403. Surface the terminal state to the caller so the reject code is consistent with theBlockedpath earlier in this function.🧹 Proposed direction
- let wait_for_auth = async { - let mut stream = in_flight_watcher.watch().stream(); - while let Some(in_flight) = stream.next().await { - if matches!( - in_flight, - AuthState::Unauthenticated | AuthState::Authenticated | AuthState::Blocked - ) { - return; - } - } - }; - - match timeout(AUTH_TIMEOUT, wait_for_auth).await { - Ok(_) => { - if self.is_authenticated(&endpoint_id) { - AfterHandshakeOutcome::accept() - } else { - AfterHandshakeOutcome::Reject { - error_code: VarInt::from_u32(401), - reason: b"Authentication failed".to_vec(), - } - } - } + let wait_for_auth = async { + let mut stream = in_flight_watcher.watch().stream(); + while let Some(state) = stream.next().await { + if matches!( + state, + AuthState::Unauthenticated | AuthState::Authenticated | AuthState::Blocked + ) { + return Some(state); + } + } + None + }; + + match timeout(AUTH_TIMEOUT, wait_for_auth).await { + Ok(Some(AuthState::Authenticated)) => AfterHandshakeOutcome::accept(), + Ok(Some(AuthState::Blocked)) => AfterHandshakeOutcome::Reject { + error_code: VarInt::from_u32(403), + reason: b"Endpoint is blocked".to_vec(), + }, + Ok(_) => AfterHandshakeOutcome::Reject { + error_code: VarInt::from_u32(401), + reason: b"Authentication failed".to_vec(), + }, Err(_) => { ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 681 - 702, The current timeout branch returns a generic 401 when wait_for_auth finishes but is_authenticated(&endpoint_id) is false; instead, inspect the final authentication state produced by wait_for_auth (or query the same AuthState used earlier) and if that terminal state is AuthState::Blocked return AfterHandshakeOutcome::Reject with VarInt::from_u32(403) and an appropriate "Authentication blocked" reason, otherwise keep the 401/"Authentication failed" behavior; update the logic around timeout(AUTH_TIMEOUT, wait_for_auth).await to use the final AuthState rather than only is_authenticated when choosing the reject code so it matches the explicit Blocked branch above.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Line 141: The auth_state HashMap is never pruning entries, causing unbounded
growth; update register_in_flight/release_in_flight and the failure paths in
ProtocolHandler::accept and EndpointHooks::after_handshake to remove auth_state
entries (the WatchableRemote for an EndpointId) when a peer lands in terminal
Unauthenticated or on timeout, and implement eviction for Blocked entries
(either remove on transition after a TTL or enforce a capped size) so
transient/failed peers do not permanently remain in auth_state; specifically,
locate the auth_state field and calls to register_in_flight/release_in_flight
and ensure release_in_flight removes the map entry for Unauthenticated, add
removal on timeout paths in ProtocolHandler::accept, and add TTL or size-cap
logic when inserting Blocked WatchableRemote entries.
- Around line 203-220: The PartialEq impl for WatchableRemote currently compares
self.state() but Hash uses self.id(), violating the Eq/Hash contract; change the
impl PartialEq for WatchableRemote to compare IDs (use self.id() == other.id())
so equality and hashing both rely on id(), or remove/rename the state-based
equality; ensure the impl PartialEq<PublicKey> remains comparing self.id() to
&PublicKey and keep impl std::hash::Hash hashing self.id() so all equality and
hashing consistently use id() (refer to the impl blocks named PartialEq for
WatchableRemote, PartialEq<PublicKey> for WatchableRemote, the Hash impl, and
the methods id() and state()).
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 681-702: The current timeout branch returns a generic 401 when
wait_for_auth finishes but is_authenticated(&endpoint_id) is false; instead,
inspect the final authentication state produced by wait_for_auth (or query the
same AuthState used earlier) and if that terminal state is AuthState::Blocked
return AfterHandshakeOutcome::Reject with VarInt::from_u32(403) and an
appropriate "Authentication blocked" reason, otherwise keep the
401/"Authentication failed" behavior; update the logic around
timeout(AUTH_TIMEOUT, wait_for_auth).await to use the final AuthState rather
than only is_authenticated when choosing the reject code so it matches the
explicit Blocked branch above.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Owner
Author
|
todo:
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/tests.rs (2)
163-225:⚠️ Potential issue | 🔴 CriticalBuild the full ALPN list once and assert every connection result.
Lines 165 and 178 appear to overwrite the endpoint ALPN list on each loop iteration, leaving only the last
/dummy/{i}route registered. Lines 216-222 then discard connection failures, so the stress test can pass without exercising the intended parallel routes.🧪 Proposed direction
- let mut endpoint_a_builder = iroh::Endpoint::builder(iroh::endpoint::presets::N0); - for i in 0..parallel_count { - endpoint_a_builder = - endpoint_a_builder.alpns(vec![format!("/dummy/{}", i).into_bytes()]); - } + let alpns: Vec<Vec<u8>> = (0..parallel_count) + .map(|i| format!("/dummy/{}", i).into_bytes()) + .collect(); + let endpoint_a_builder = + iroh::Endpoint::builder(iroh::endpoint::presets::N0).alpns(alpns.clone()); @@ - let mut endpoint_b_builder = iroh::Endpoint::builder(iroh::endpoint::presets::N0); - for i in 0..parallel_count { - endpoint_b_builder = - endpoint_b_builder.alpns(vec![format!("/dummy/{}", i).into_bytes()]); - } + let endpoint_b_builder = + iroh::Endpoint::builder(iroh::endpoint::presets::N0).alpns(alpns); @@ - for i in 0..parallel_count { + let mut connects = tokio::task::JoinSet::new(); + for i in 0..parallel_count { tokio::time::sleep(Duration::from_millis(100)).await; - tokio::spawn({ + connects.spawn({ let endpoint_a = endpoint_a.clone(); let endpoint_b = endpoint_b.clone(); async move { + let alpn = format!("/dummy/{}", i).into_bytes(); endpoint_a - .connect( - endpoint_b.addr(), - format!("/dummy/{}", i).into_bytes().as_slice(), - ) + .connect(endpoint_b.addr(), alpn) .await - .ok(); } }); } + + while let Some(result) = connects.join_next().await { + result + .expect("connect task panicked") + .map_err(|e| e.to_string())?; + }Please verify the builder semantics for the exact iroh version:
iroh 0.98 Endpoint builder alpns method replaces or appends ALPN protocols🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests.rs` around lines 163 - 225, The ALPN list is being overwritten in each loop by repeated calls to Endpoint::builder(...).alpns(...) (see endpoint_a_builder and endpoint_b_builder); instead build a single Vec of all ALPN bytes (e.g. collect format!("/dummy/{}", i).into_bytes() for 0..parallel_count) and call .alpns(...) once on the builder before .hooks(...).bind(); also stop discarding connection results in the tokio::spawn block — capture the Result from endpoint_a.connect(...).await and assert or propagate errors (use expect/map_err? or assert!(result.is_ok())) so connection failures to any /dummy/{i} are detected.
62-75:⚠️ Potential issue | 🟠 MajorJoin spawned auth tasks under a timeout.
A panic before
worked.fetch_add(...)leaves this test spinning forever. UseJoinSet/handles and wrap the join phase intimeout(...).🧪 Proposed direction
- let worked = Arc::new(AtomicUsize::new(0)); let count = 10; + let mut tasks = tokio::task::JoinSet::new(); for _ in 0..count { - let worked = worked.clone(); - tokio::spawn(async move { + tasks.spawn(async move { assert!(run_auth_parallel_test(secret, secret, count).await.unwrap()); - worked.fetch_add(1, std::sync::atomic::Ordering::SeqCst); }); } - while worked.load(std::sync::atomic::Ordering::SeqCst) < count { - tokio::time::sleep(Duration::from_millis(100)).await; - } + timeout(AUTH_TIMEOUT * 4, async { + while let Some(result) = tasks.join_next().await { + result.expect("parallel auth task panicked"); + } + }) + .await + .expect("parallel auth test timed out");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests.rs` around lines 62 - 75, The test spawns tasks with tokio::spawn and relies on worked.fetch_add to finish, which can hang if a task panics; replace the detached spawns with a JoinSet or collect JoinHandles for the tasks that call run_auth_parallel_test(secret, secret, count) and then join them, wrapping the join phase in tokio::time::timeout to fail the test instead of spinning forever; reference the existing symbols worked, count, run_auth_parallel_test, and the tokio::spawn usage so you know where to replace spawns with JoinSet/handles and apply timeout around awaiting all joins.src/protocol.rs (1)
279-314:⚠️ Potential issue | 🟠 MajorEvict terminal failure states from
auth_state.
release_in_flight(..., Unauthenticated)leaves a permanent map entry, andBlockedentries are also unbounded. A long-running node can accumulate one entry per failed/transient peer.🧹 Suggested direction
AuthState::InFlight => { - entry.get().set_state(target_state); + entry.get().set_state(target_state.clone()); + if matches!(target_state, AuthState::Unauthenticated) { + entry.remove(); + } Ok(()) }For
Blocked, use a TTL or the addedlrudependency so block retention is bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protocol.rs` around lines 279 - 314, The release_in_flight function currently leaves permanent entries for terminal failure states (e.g., releasing to AuthState::Unauthenticated or adding AuthState::Blocked), causing the in_flight HashMap to grow unbounded; update release_in_flight (and related WatchableRemote usage) to evict entries when setting terminal states (remove the Entry from the guarded in_flight map after setting or instead of inserting) and implement bounded retention for Blocked (either drop entries immediately or integrate an LRU/TTL eviction policy using the added lru dependency or a timestamp+cleanup pass) so failed peers do not remain forever in the map. Ensure you reference and modify release_in_flight, the Branch handling AuthState::Unauthenticated and AuthState::Blocked, and how WatchableRemote::set_state is used so entries are removed or TTL'd appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 25: Replace the bare tokio dependency with an explicit feature list so
the crate gets the APIs it uses: include "rt-multi-thread" (for spawn and
multi-threaded runtime), "macros" (for #[tokio::main]/#[tokio::test]), "sync"
(for tokio::sync::Mutex) and "time" (for tokio::time). Update the tokio entry in
Cargo.toml (the tokio dependency line) to specify these features alongside
version = "1".
In `@src/auth.rs`:
- Around line 209-238: The timeout handler currently only checks for the outer
timeout Err and ignores the inner Result from recv.read_to_end(MAX_READ_SIZE),
so read failures or oversized trailing data are swallowed; update the logic (the
tokio::time::timeout(...).await handling around recv.read_to_end(MAX_READ_SIZE)
in auth.rs) to inspect both outcomes: match the await result, convert a timeout
into AuthenticatorError::OpenFailed/AcceptFailed as you already do, and when the
timeout returns Ok(inner_result) handle inner Err by mapping the read error into
AuthenticatorError::OpenFailed/AcceptFailed (including the error details) and
logging it the same way as the timeout case. Ensure you preserve use of
AUTH_TIMEOUT and the open flag when constructing the error variants.
- Around line 419-424: The loop that retries endpoint.connect is not bounded
because the await can block past AUTH_TIMEOUT; inside the while
(start_time.elapsed() < AUTH_TIMEOUT) in the before_connect logic wrap the
connect future with a timeout computed from the remaining window (AUTH_TIMEOUT -
start_time.elapsed()) so each await respects the overall AUTH_TIMEOUT, handle
the timeout/error path from that timeout wrapper (treat as a failed attempt and
continue or break appropriately) and ensure the code uses the ALPN and remote_id
parameters when calling endpoint.connect as before.
In `@src/error.rs`:
- Around line 25-28: The error variant AuthenticatorError::EndpointNotSet
returns a stale help message referencing authenticator.start(endpoint); update
the message in the Display/formatting arm for AuthenticatorError::EndpointNotSet
to instruct callers to use the new API, e.g. call set_endpoint(...).await
(replace the old "authenticator.start(endpoint)" text with
"set_endpoint(...).await") so the error guidance matches the current
Authenticator API.
In `@src/protocol.rs`:
- Around line 209-251: The code currently accepts the app connection immediately
after registering auth as InFlight or when AlreadyInFlight, allowing a race;
change the logic so that when register_in_flight(...) returns
RegisterResponse::InFlightRegistered you still spawn_auth_worker(remote_id,
endpoint).await but do NOT return BeforeConnectOutcome::Accept—return
BeforeConnectOutcome::Reject (or otherwise block) so the connection waits for
auth to reach a terminal state; likewise, for RegisterResponse::AlreadyInFlight,
if is_authenticated(&remote_id).await is false, return
BeforeConnectOutcome::Reject instead of falling through to accept; keep the
existing behavior for AlreadyAuthenticated and AlreadyBlocked and preserve
logging.
---
Duplicate comments:
In `@src/protocol.rs`:
- Around line 279-314: The release_in_flight function currently leaves permanent
entries for terminal failure states (e.g., releasing to
AuthState::Unauthenticated or adding AuthState::Blocked), causing the in_flight
HashMap to grow unbounded; update release_in_flight (and related WatchableRemote
usage) to evict entries when setting terminal states (remove the Entry from the
guarded in_flight map after setting or instead of inserting) and implement
bounded retention for Blocked (either drop entries immediately or integrate an
LRU/TTL eviction policy using the added lru dependency or a timestamp+cleanup
pass) so failed peers do not remain forever in the map. Ensure you reference and
modify release_in_flight, the Branch handling AuthState::Unauthenticated and
AuthState::Blocked, and how WatchableRemote::set_state is used so entries are
removed or TTL'd appropriately.
In `@src/tests.rs`:
- Around line 163-225: The ALPN list is being overwritten in each loop by
repeated calls to Endpoint::builder(...).alpns(...) (see endpoint_a_builder and
endpoint_b_builder); instead build a single Vec of all ALPN bytes (e.g. collect
format!("/dummy/{}", i).into_bytes() for 0..parallel_count) and call .alpns(...)
once on the builder before .hooks(...).bind(); also stop discarding connection
results in the tokio::spawn block — capture the Result from
endpoint_a.connect(...).await and assert or propagate errors (use
expect/map_err? or assert!(result.is_ok())) so connection failures to any
/dummy/{i} are detected.
- Around line 62-75: The test spawns tasks with tokio::spawn and relies on
worked.fetch_add to finish, which can hang if a task panics; replace the
detached spawns with a JoinSet or collect JoinHandles for the tasks that call
run_auth_parallel_test(secret, secret, count) and then join them, wrapping the
join phase in tokio::time::timeout to fail the test instead of spinning forever;
reference the existing symbols worked, count, run_auth_parallel_test, and the
tokio::spawn usage so you know where to replace spawns with JoinSet/handles and
apply timeout around awaiting all joins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b59a0a31-174a-4add-b50b-978c05a0ca33
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlexamples/basic.rssrc/auth.rssrc/error.rssrc/lib.rssrc/protocol.rssrc/secret.rssrc/tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 39-41: Remove "gossip-example" from the default features in
Cargo.toml so example-only dependencies are not pulled into downstream crates:
change the features table so that gossip-example remains defined but default =
[] (or otherwise excludes "gossip-example"); ensure the feature name
"gossip-example" and the default key "default" in the Cargo.toml features
section are updated accordingly so consumers must opt into the example via
--features gossip-example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 184400fc-2cb1-4351-a173-9080673cd770
📒 Files selected for processing (6)
Cargo.tomlexamples/gossip.rssrc/auth.rssrc/lib.rssrc/protocol.rssrc/tests.rs
…ync and require .await, if not indicated by full version upgrade we run the risk that futures are just never executed in updated projects)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (2)
22-25:⚠️ Potential issue | 🟡 MinorUpdate the installation snippet for the released versions.
Cargo.tomlnow publishesiroh-authas0.2.0and depends oniroh = "0.98", but the README still tells users to installiroh = "0.96"andiroh-auth = "0.1".📚 Proposed docs fix
[dependencies] -iroh = "0.96" -iroh-auth = "0.1" +iroh = "0.98" +iroh-auth = "0.2"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 22 - 25, Update the Cargo.toml example in the README: change the dependency versions from iroh = "0.96" and iroh-auth = "0.1" to the released versions iroh = "0.98" and iroh-auth = "0.2.0" so the example matches the published crate versions (look for the TOML snippet with [dependencies] and the iroh / iroh-auth entries).
47-54:⚠️ Potential issue | 🟡 MinorFix the README example to match the current async setup API.
The example uses
Endpoint::builder()without thepresets::N0argument and callsauth.set_endpoint(&endpoint)without.await. This will cause compile errors; the method is async. The correct code is:Proposed fix
- let endpoint = Endpoint::builder() + let endpoint = Endpoint::builder(iroh::endpoint::presets::N0) .hooks(auth.clone()) .bind() .await.map_err(|e| e.to_string())?; // 3. The authenticator needs a reference to the bound endpoint // to initiate authentication handshakes. - auth.set_endpoint(&endpoint); + auth.set_endpoint(&endpoint).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 47 - 54, Update the README example to match the current async API by calling Endpoint::builder with the required presets argument (e.g., Endpoint::builder(presets::N0)) and await the async authenticator method call (use auth.set_endpoint(&endpoint).await); keep the existing bind().await usage and error mapping but replace the builder() invocation and add .await to set_endpoint to avoid compilation errors.
♻️ Duplicate comments (3)
src/error.rs (1)
25-28:⚠️ Potential issue | 🟡 MinorReplace the stale setup method in the error guidance.
The API is
set_endpoint(&endpoint).await, notstart(endpoint).await, so this message sends users to a non-existent setup path.✏️ Proposed fix
AuthenticatorError::EndpointNotSet => write!( f, - "Authenticator endpoint not set: missing authenticator.start(endpoint).await" + "Authenticator endpoint not set: missing authenticator.set_endpoint(&endpoint).await" ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/error.rs` around lines 25 - 28, Update the error message for AuthenticatorError::EndpointNotSet: change the guidance string in the write! call (the Display/format implementation that constructs the message) to reference the correct API usage set_endpoint(&endpoint).await instead of the stale start(endpoint).await so users are directed to the existing method.src/auth.rs (1)
212-240:⚠️ Potential issue | 🟠 MajorHandle the inner
read_to_enderror instead of dropping it.
timeout(...).awaitreturnsOk(Err(_))for read failures or oversized trailing data, and this branch currently falls through as success. That can hide malformed auth stream shutdowns.🛡️ Proposed fix
const MAX_READ_SIZE: usize = 1024; - if let Err(err) = tokio::time::timeout(AUTH_TIMEOUT, recv.read_to_end(MAX_READ_SIZE)) - .await - .map_err(|_| { - if open { - AuthenticatorError::OpenFailed( - "Failed to wait for stream stopped: timeout".to_string(), - ) - } else { - AuthenticatorError::AcceptFailed( - "Failed to wait for stream stopped: timeout".to_string(), - ) - } - }) - .map_err(|err| { - if open { - AuthenticatorError::OpenFailed(format!( - "Failed to wait for stream stopped: {}", - err - )) - } else { - AuthenticatorError::AcceptFailed(format!( - "Failed to wait for stream stopped: {}", - err - )) - } - }) - { - warn!("[end_of_auth] {}", err); - } + match tokio::time::timeout(AUTH_TIMEOUT, recv.read_to_end(MAX_READ_SIZE)).await { + Ok(Ok(_)) => {} + Ok(Err(err)) => { + let err = if open { + AuthenticatorError::OpenFailed(format!( + "Failed to wait for stream stopped: {}", + err + )) + } else { + AuthenticatorError::AcceptFailed(format!( + "Failed to wait for stream stopped: {}", + err + )) + }; + warn!("[end_of_auth] {}", err); + } + Err(_) => { + let err = if open { + AuthenticatorError::OpenFailed( + "Failed to wait for stream stopped: timeout".to_string(), + ) + } else { + AuthenticatorError::AcceptFailed( + "Failed to wait for stream stopped: timeout".to_string(), + ) + }; + warn!("[end_of_auth] {}", err); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth.rs` around lines 212 - 240, The current timeout(...) call only maps the outer timeout error but ignores the inner read_to_end error (Ok(Err(_))) which lets read failures or oversized trailing data pass as success; change the logic around tokio::time::timeout(AUTH_TIMEOUT, recv.read_to_end(MAX_READ_SIZE)).await so you explicitly match its result and convert both Err(timeout) and Ok(Err(read_err)) into the appropriate AuthenticatorError (use AuthenticatorError::OpenFailed or ::AcceptFailed depending on open), preserving the read_err message in the formatted error; ensure the warn!("[end_of_auth] {}", err) branch is executed for both failure cases instead of letting inner errors fall through.src/protocol.rs (1)
202-243:⚠️ Potential issue | 🟠 Major
before_connectstill accepts app connections before auth reaches a terminal state.The
InFlightRegisteredarm spawns the worker and falls through toAcceptat Line 243; theAlreadyInFlightarm also falls through toAcceptwhenis_authenticatedis false. That lets the application connection race ahead of the SPAKE2 worker — the previous review’safter_handshake-style wait is still the right shape here.Additionally, when
self.endpoint().awaitfails at Lines 211–217,register_in_flighthas already promoted the cache entry toInFlight, but this path returnsRejectwithout callingrelease_in_flight(.., Unauthenticated). The peer is then stuckInFlightuntil LRU eviction, and any futureafter_handshakefor it will payAUTH_TIMEOUTbefore rejecting.🔒 Proposed direction
Ok(RegisterResponse::InFlightRegistered) => { debug!( "[before_connect] registered in-flight auth for {}, performing auth", remote_id ); - let endpoint = match self.endpoint().await { - Ok(ep) => ep, - Err(_) => { - warn!("[before_connect] authenticator endpoint not set"); - return iroh::endpoint::BeforeConnectOutcome::Reject; - } - }; + let endpoint = match self.endpoint().await { + Ok(ep) => ep, + Err(_) => { + warn!("[before_connect] authenticator endpoint not set"); + release_in_flight( + self.auth_state.clone(), + remote_id, + AuthState::Unauthenticated, + ) + .await + .ok(); + return iroh::endpoint::BeforeConnectOutcome::Reject; + } + }; self.spawn_auth_worker(remote_id, endpoint).await; } @@ - iroh::endpoint::BeforeConnectOutcome::Accept + // Wait until the worker marks the peer Authenticated / Blocked / Unauthenticated + // (bounded by AUTH_TIMEOUT) before accepting the app connection. + if timeout(AUTH_TIMEOUT, async { + loop { + if self.is_authenticated(&remote_id).await { return true; } + if let Some(st) = get_auth_state(self.auth_state.clone(), remote_id).await { + if matches!(st.state(), AuthState::Blocked | AuthState::Unauthenticated) { + return false; + } + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + }).await.unwrap_or(false) { + iroh::endpoint::BeforeConnectOutcome::Accept + } else { + iroh::endpoint::BeforeConnectOutcome::Reject + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protocol.rs` around lines 202 - 243, The before_connect branch currently accepts connections while auth is still "InFlight"; change it so that InFlightRegistered and AlreadyInFlight (when is_authenticated(&remote_id) is false) do NOT fall through to Accept but instead defer acceptance to the after_handshake flow (i.e. return Reject here), and ensure any early error paths release the in-flight slot: after register_in_flight returns InFlightRegistered, call spawn_auth_worker(remote_id, endpoint).await but then return BeforeConnectOutcome::Reject (do not Accept); for the AlreadyInFlight arm, if is_authenticated(&remote_id).await is false return Reject; for the endpoint().await failure path call release_in_flight(remote_id, Unauthenticated) before returning Reject; keep the AlreadyAuthenticated and AlreadyBlocked handling as-is. Use the symbols register_in_flight, RegisterResponse::InFlightRegistered/AlreadyInFlight/AlreadyAuthenticated/AlreadyBlocked, spawn_auth_worker, is_authenticated, release_in_flight, and BeforeConnectOutcome to locate and apply these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth.rs`:
- Around line 423-436: The auth_open call currently uses a fresh AUTH_TIMEOUT,
allowing the worker to exceed the intended deadline; recompute the remaining
time after the successful connect and use that remaining duration for the
auth.auth_open(conn) timeout. In the loop that computes t before
endpoint.connect(remote_id, ALPN), after receiving Ok(Ok(conn)) calculate
remaining =
AUTH_TIMEOUT.saturating_sub(Instant::now().saturating_duration_since(start_time))
(or reuse a recomputed variable) and call timeout(remaining,
auth.auth_open(conn)); if remaining is zero treat it as a timeout/failure and
handle accordingly.
In `@src/tests.rs`:
- Around line 126-155: Extract the duplicated wait/timeout/shutdown/assert logic
from run_auth_test and run_auth_parallel_test into two helpers: a
wait_for_auth_terminal(auth_a: &AuthType, auth_b: &AuthType) that runs the
wait_loop (polling list_authenticated/list_blocked with time::sleep) wrapped in
timeout(AUTH_TIMEOUT * 2, ...) and returns a Result<(), String>, and a
shutdown_routers(router_a, router_b) that calls router_a.shutdown().await.ok()
and router_b.shutdown().await.ok(); then replace the duplicated block in both
tests with a call to wait_for_auth_terminal(&auth_a, &auth_b)?/handle Err by
calling shutdown_routers(...) and returning the Err string, then call
shutdown_routers(...) and finally perform the same final
is_authenticated(&endpoint_b.id()).await &&
is_authenticated(&endpoint_a.id()).await check and return it.
- Around line 183-186: Remove the dead commented-out relay configuration
referencing RelayMap in the tests: either delete the commented block containing
.clear_relay_transports(), .relay_mode(iroh::RelayMode::Custom(...)) and the
RelayMap::try_from_iter(...) call, or if you intend to keep it for optional
testing, wrap it behind a feature flag (e.g., cfg(feature = "relay")) and add
the necessary import for RelayMap so the code compiles when the feature is
enabled; update references in the test around clear_relay_transports and
relay_mode accordingly.
- Around line 55-76: The test_auth_parallel function currently detaches
tokio::spawn tasks so panics are lost and the worked counter can hang; change it
to collect the JoinHandles returned by tokio::spawn, await each handle and
propagate errors (use handle.await.unwrap() or propagate JoinError), and wrap
the whole wait in a timeout (tokio::time::timeout) so the test fails
deterministically instead of blocking; ensure you still call
run_auth_parallel_test(secret, secret, count).await.unwrap() inside the spawned
futures but capture the JoinHandle results and assert their success after the
timeout expires, updating or removing the worked counter as needed.
---
Outside diff comments:
In `@README.md`:
- Around line 22-25: Update the Cargo.toml example in the README: change the
dependency versions from iroh = "0.96" and iroh-auth = "0.1" to the released
versions iroh = "0.98" and iroh-auth = "0.2.0" so the example matches the
published crate versions (look for the TOML snippet with [dependencies] and the
iroh / iroh-auth entries).
- Around line 47-54: Update the README example to match the current async API by
calling Endpoint::builder with the required presets argument (e.g.,
Endpoint::builder(presets::N0)) and await the async authenticator method call
(use auth.set_endpoint(&endpoint).await); keep the existing bind().await usage
and error mapping but replace the builder() invocation and add .await to
set_endpoint to avoid compilation errors.
---
Duplicate comments:
In `@src/auth.rs`:
- Around line 212-240: The current timeout(...) call only maps the outer timeout
error but ignores the inner read_to_end error (Ok(Err(_))) which lets read
failures or oversized trailing data pass as success; change the logic around
tokio::time::timeout(AUTH_TIMEOUT, recv.read_to_end(MAX_READ_SIZE)).await so you
explicitly match its result and convert both Err(timeout) and Ok(Err(read_err))
into the appropriate AuthenticatorError (use AuthenticatorError::OpenFailed or
::AcceptFailed depending on open), preserving the read_err message in the
formatted error; ensure the warn!("[end_of_auth] {}", err) branch is executed
for both failure cases instead of letting inner errors fall through.
In `@src/error.rs`:
- Around line 25-28: Update the error message for
AuthenticatorError::EndpointNotSet: change the guidance string in the write!
call (the Display/format implementation that constructs the message) to
reference the correct API usage set_endpoint(&endpoint).await instead of the
stale start(endpoint).await so users are directed to the existing method.
In `@src/protocol.rs`:
- Around line 202-243: The before_connect branch currently accepts connections
while auth is still "InFlight"; change it so that InFlightRegistered and
AlreadyInFlight (when is_authenticated(&remote_id) is false) do NOT fall through
to Accept but instead defer acceptance to the after_handshake flow (i.e. return
Reject here), and ensure any early error paths release the in-flight slot: after
register_in_flight returns InFlightRegistered, call spawn_auth_worker(remote_id,
endpoint).await but then return BeforeConnectOutcome::Reject (do not Accept);
for the AlreadyInFlight arm, if is_authenticated(&remote_id).await is false
return Reject; for the endpoint().await failure path call
release_in_flight(remote_id, Unauthenticated) before returning Reject; keep the
AlreadyAuthenticated and AlreadyBlocked handling as-is. Use the symbols
register_in_flight,
RegisterResponse::InFlightRegistered/AlreadyInFlight/AlreadyAuthenticated/AlreadyBlocked,
spawn_auth_worker, is_authenticated, release_in_flight, and BeforeConnectOutcome
to locate and apply these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ee639c6-345a-43de-a829-1b23f6de5f75
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlREADME.mdexamples/basic.rsexamples/gossip.rssrc/auth.rssrc/error.rssrc/lib.rssrc/protocol.rssrc/secret.rssrc/tests.rs
…s endpoint.online() (this means it only works with relay connected according to iroh docs)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
54-54:⚠️ Potential issue | 🟡 MinorREADME usage snippet is missing
.awaitonset_endpoint.
Authenticator::set_endpointis now async (as reflected in bothexamples/basic.rsandexamples/gossip.rs), but line 54 of this README still calls it synchronously. The snippet will not compile as-is and will mislead new users copy-pasting from the docs.✏️ Proposed fix
- auth.set_endpoint(&endpoint); + auth.set_endpoint(&endpoint).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 54, The README usage snippet calls auth.set_endpoint(&endpoint) synchronously but Authenticator::set_endpoint is async; update the snippet to call auth.set_endpoint(&endpoint).await and ensure the surrounding example is inside an async context (e.g., async fn main or a tokio::main runtime) consistent with examples/basic.rs and examples/gossip.rs so the code compiles when copy-pasted.
♻️ Duplicate comments (5)
src/error.rs (1)
25-28:⚠️ Potential issue | 🟡 MinorError message still references the non-existent
startmethod.The help text points users to
authenticator.start(endpoint).await, but the actual API isset_endpoint(...).await(seesrc/auth.rsand the examples). This is misleading guidance to a caller who hit this error.✏️ Proposed fix
- "Authenticator endpoint not set: missing authenticator.start(endpoint).await" + "Authenticator endpoint not set: missing authenticator.set_endpoint(endpoint).await"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/error.rs` around lines 25 - 28, The error message for AuthenticatorError::EndpointNotSet incorrectly refers to authenticator.start(endpoint).await; update the Display/format string in the AuthenticatorError::EndpointNotSet arm (in src/error.rs) to reference the correct API call authenticator.set_endpoint(...).await (or set_endpoint(endpoint).await) so callers are pointed to the real method (matches src/auth.rs and examples).src/tests.rs (2)
57-75:⚠️ Potential issue | 🟠 Major
test_auth_parallelstill detaches tasks and busy-polls without a timeout.The
tokio::spawnhandles are discarded, so any panic insideassert!/expectis lost; if a worker panics before incrementingworked, thewhileloop at lines 72–74 spins forever and the test only terminates when the CI runner kills it. CollectJoinHandles (or aJoinSet) and wrap the wait intokio::time::timeoutso failures surface deterministically.🔧 Proposed fix
- let secret = b"supersecrettoken1234567890123456"; - let worked = Arc::new(AtomicUsize::new(0)); - let count = 10; - for _ in 0..count { - let worked = worked.clone(); - tokio::spawn(async move { - assert!(run_auth_parallel_test(secret, secret, count) - .await - .expect("auth_parallel test failed")); - worked.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - }); - } - - while worked.load(std::sync::atomic::Ordering::SeqCst) < count { - tokio::time::sleep(Duration::from_millis(100)).await; - } + let secret = b"supersecrettoken1234567890123456"; + let count = 10; + let mut tasks = tokio::task::JoinSet::new(); + for _ in 0..count { + tasks.spawn(async move { + run_auth_parallel_test(secret, secret, count).await + }); + } + timeout(AUTH_TIMEOUT * 4, async { + while let Some(res) = tasks.join_next().await { + assert!(res.expect("task panicked").expect("auth failed")); + } + }) + .await + .expect("test_auth_parallel timed out");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests.rs` around lines 57 - 75, The test_auth_parallel function currently detaches tokio::spawn tasks and busy-waits on the worked AtomicUsize, losing panics and risking an infinite loop; collect the spawned task JoinHandle values (or use a tokio::task::JoinSet) instead of discarding them, await all handles and propagate panics/results from run_auth_parallel_test so failures surface, and wrap the join/wait phase in tokio::time::timeout to bound the wait; update references to the worked Arc, the run_auth_parallel_test calls and the spawn logic to push handles into a Vec/JoinSet and then await them with timeout.
127-155: 🧹 Nitpick | 🔵 TrivialFactor out the duplicated wait/timeout/shutdown block.
run_auth_testandrun_auth_parallel_testrepeat the sametimeout(AUTH_TIMEOUT * 2, wait_loop)→ router shutdown →is_authenticatedcross-check. Extract a small helper (e.g.wait_for_auth_terminal(&auth_a, &auth_b, ...)plusshutdown_routers(router_a, router_b)) so the two helpers do not drift.Also applies to: 230-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests.rs` around lines 127 - 155, The test file duplicates the same wait/timeout/shutdown/is_authenticated logic across run_auth_test and run_auth_parallel_test; extract a helper like wait_for_auth_terminal(auth_a, auth_b, timeout) that encapsulates building the wait_loop and awaiting timeout(AUTH_TIMEOUT * 2, ...), and another helper shutdown_routers(router_a, router_b) to call router.shutdown().await.ok(); then replace the duplicated blocks in run_auth_test and run_auth_parallel_test with calls to wait_for_auth_terminal(&auth_a, &auth_b, AUTH_TIMEOUT * 2) followed by shutdown_routers(router_a, router_b) and finally use auth_a.is_authenticated(&endpoint_b.id()).await && auth_b.is_authenticated(&endpoint_a.id()).await for the cross-check.src/auth.rs (1)
213-241:⚠️ Potential issue | 🟠 MajorInner
read_to_enderror is still silently swallowed.The
Ok(Err(_))branch oftimeout(..., recv.read_to_end(MAX_READ_SIZE)).awaitis still dropped: the first.map_erronly fires on the outerElapsed, and the second only operates on the already-mappedAuthenticatorError, so a failing read or an oversized trailing payload returnsOk(())here. The past review flagged this and it's marked as addressed in commits, but the structural bug remains in the current tree. Please convert this to an explicitmatchoverOk(Ok(_)) / Ok(Err(_)) / Err(_)as previously suggested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth.rs` around lines 213 - 241, The current chained .map_err calls on tokio::time::timeout(AUTH_TIMEOUT, recv.read_to_end(MAX_READ_SIZE)).await silently drop an inner read error; replace the chain with an explicit match on the await result: match tokio::time::timeout(...).await { Ok(Ok(_)) => {}, Ok(Err(inner_err)) => map inner_err to AuthenticatorError::OpenFailed or ::AcceptFailed (using the open boolean) with a message like "Failed to wait for stream stopped: {inner_err}" and warn! it, Err(_) => map the timeout to the same OpenFailed/AcceptFailed timeout message and warn! it }. Use the same symbols AUTH_TIMEOUT, MAX_READ_SIZE, recv, open, and the AuthenticatorError variants OpenFailed/AcceptFailed so the correct error branches are handled instead of being swallowed.src/protocol.rs (1)
224-232:⚠️ Potential issue | 🟠 Major
AlreadyInFlightstill races ahead of the concurrent auth worker.This branch accepts the app connection whether the cached state is
AlreadyInFlightorAlreadyAuthenticated. For the in-flight case, another worker is still runningperform_auth; returningAccepthere lets the application connection proceed before that worker finishes, which is exactly the race the earlier review flagged. Only theInFlightRegisteredarm was fixed (it now blocks onperform_auth); concurrent callers entering viaAlreadyInFlightget no such wait.Additionally, the
debug!on Line 226 ("already authenticated (in flight), accepting connection") only fires whenis_authenticatedis true, i.e. theAlreadyAuthenticatedcase — so in the actually-in-flight case the accept is silent.Consider either awaiting the existing in-flight watcher (mirroring
after_handshake'swait_for_auth) or rejecting untilis_authenticatedbecomes true.🔒 Suggested direction
- Ok(RegisterResponse::AlreadyInFlight) | Ok(RegisterResponse::AlreadyAuthenticated) => { - if self.is_authenticated(&remote_id).await { - debug!( - "[before_connect] already authenticated (in flight), accepting connection to {}", - remote_id - ); - } - iroh::endpoint::BeforeConnectOutcome::Accept - } + Ok(RegisterResponse::AlreadyAuthenticated) => { + debug!("[before_connect] already authenticated, accepting connection to {}", remote_id); + iroh::endpoint::BeforeConnectOutcome::Accept + } + Ok(RegisterResponse::AlreadyInFlight) => { + // Wait for the concurrent worker to reach a terminal state + // instead of letting the app connection race ahead. + match self.wait_for_terminal_state(remote_id).await { + AuthState::Authenticated => iroh::endpoint::BeforeConnectOutcome::Accept, + _ => iroh::endpoint::BeforeConnectOutcome::Reject, + } + }This also directly ties into the PR TODO ("test for an incoming and outgoing auth connection occurring simultaneously from the perspective of one peer") — the current behaviour would likely fail such a test deterministically on the second concurrent caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protocol.rs` around lines 224 - 232, The AlreadyInFlight arm currently returns Accept and can race with the concurrent auth worker; change the logic so that when RegisterResponse::AlreadyInFlight is encountered you do not immediately Accept but instead wait for the existing in-flight authentication to finish (mirror the behavior used for InFlightRegistered by awaiting the same watcher or calling the same wait_for_auth helper used by after_handshake) and only return iroh::endpoint::BeforeConnectOutcome::Accept once self.is_authenticated(&remote_id).await is true; alternatively return Reject until authenticated—ensure the debug log still reflects the actual authenticated state (use RegisterResponse::AlreadyAuthenticated to short-circuit Accept immediately and keep the wait path for AlreadyInFlight).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth.rs`:
- Around line 48-54: The comments on the RegisterResponse enum contain
misspellings of "authenticated" and the function/flag name "is_authenticated";
update all occurrences in the RegisterResponse variant comments
(InFlightRegistered, AlreadyInFlight, AlreadyAuthenticated, AlreadyBlocked) to
use "authenticated" and "is_authenticated" consistently and correct the wording
(e.g., "is_authenticaded" -> "is_authenticated", "authenticaded" ->
"authenticated") so the comments are accurate and typo-free.
- Around line 149-161: Replace the read that uses LruCache::get in
is_authenticated with a non-promoting peek: lock auth_state in is_authenticated
and call .peek(id) instead of .get(id), and simplify the closure to return
w.state() == AuthState::Authenticated (or inline the comparison) so the method
becomes a simple .peek(id).map(|w| w.state() ==
AuthState::Authenticated).unwrap_or(false); this avoids LRU promotion while
preserving the boolean auth check for PublicKey lookups.
- Around line 422-436: The timeout call around endpoint.online() in perform_auth
currently leaks the InFlight state and has a redundant .map_err chain; before
returning the AuthenticatorError on timeout, call release_in_flight(peer_id,
AuthState::Unauthenticated) (use the same peer identifier used when marking
InFlight) so the peer is released, then return the error; also remove the second
.map_err so only the first map_err converting the Elapsed into
AuthenticatorError::OpenFailed remains (apply the same map_err cleanup pattern
in end_of_auth if present).
In `@src/error.rs`:
- Around line 29-34: The two AuthenticatorError match arms for
AcceptFailedAndBlock and OpenFailedAndBlock currently output the same message;
update the Display implementation (the write!(f, ...) calls inside the
AuthenticatorError match) so each variant uses a distinct, variant-specific
prefix (e.g., "Blocked on accept: ..." vs "Blocked on open: ..." or similar) and
preserves the existing msg and id values so logs/callers can distinguish which
side of the handshake triggered the block.
In `@src/protocol.rs`:
- Around line 209-222: Update the stale comment above the before_connect logic:
replace the lines that say it "spawns the authentication worker in a new task"
and "returns directly with BeforeConnectOutcome::Accept" with a short accurate
description that before_connect awaits perform_auth inline using
self.endpoint().await and then returns
iroh::endpoint::BeforeConnectOutcome::Accept or ::Reject based on the result;
reference the before_connect function, the endpoint() call, and perform_auth to
clarify where the behavior now occurs.
In `@src/tests.rs`:
- Around line 116-125: The spawned task silently discards the result of
endpoint_a.connect(...).await.ok(), so connection failures are hidden; change
the tokio::spawn block to capture the Result from
endpoint_a.connect(endpoint_b.addr(), b"/dummy/1").await, and on Err send the
error (or a stringified form) back to the test via a oneshot/mpsc channel (or at
minimum processLogger.error) so the test can fail fast; modify the closure that
currently references endpoint_a and endpoint_b to take a sender, send Err(e) on
failure and Ok(()) on success, and have the test await the receiver to surface
the real connect error instead of timing out during authentication.
- Around line 230-248: The exit condition in wait_loop (inside wait_a and
wait_b) only waits for len() + list_blocked().len() < 1, so a single
authenticated/blocked peer can satisfy the check; update the per-side condition
to require all expected peers by comparing to parallel_count (e.g., replace the
"< 1" check with "< parallel_count" or otherwise require >= parallel_count).
Keep the combined success check using success.load(...) < parallel_count as-is
so the loop only exits when both the per-side auth counts and the overall
success counter reach parallel_count. Ensure the change is applied symmetrically
to both wait_a and wait_b.
---
Outside diff comments:
In `@README.md`:
- Line 54: The README usage snippet calls auth.set_endpoint(&endpoint)
synchronously but Authenticator::set_endpoint is async; update the snippet to
call auth.set_endpoint(&endpoint).await and ensure the surrounding example is
inside an async context (e.g., async fn main or a tokio::main runtime)
consistent with examples/basic.rs and examples/gossip.rs so the code compiles
when copy-pasted.
---
Duplicate comments:
In `@src/auth.rs`:
- Around line 213-241: The current chained .map_err calls on
tokio::time::timeout(AUTH_TIMEOUT, recv.read_to_end(MAX_READ_SIZE)).await
silently drop an inner read error; replace the chain with an explicit match on
the await result: match tokio::time::timeout(...).await { Ok(Ok(_)) => {},
Ok(Err(inner_err)) => map inner_err to AuthenticatorError::OpenFailed or
::AcceptFailed (using the open boolean) with a message like "Failed to wait for
stream stopped: {inner_err}" and warn! it, Err(_) => map the timeout to the same
OpenFailed/AcceptFailed timeout message and warn! it }. Use the same symbols
AUTH_TIMEOUT, MAX_READ_SIZE, recv, open, and the AuthenticatorError variants
OpenFailed/AcceptFailed so the correct error branches are handled instead of
being swallowed.
In `@src/error.rs`:
- Around line 25-28: The error message for AuthenticatorError::EndpointNotSet
incorrectly refers to authenticator.start(endpoint).await; update the
Display/format string in the AuthenticatorError::EndpointNotSet arm (in
src/error.rs) to reference the correct API call
authenticator.set_endpoint(...).await (or set_endpoint(endpoint).await) so
callers are pointed to the real method (matches src/auth.rs and examples).
In `@src/protocol.rs`:
- Around line 224-232: The AlreadyInFlight arm currently returns Accept and can
race with the concurrent auth worker; change the logic so that when
RegisterResponse::AlreadyInFlight is encountered you do not immediately Accept
but instead wait for the existing in-flight authentication to finish (mirror the
behavior used for InFlightRegistered by awaiting the same watcher or calling the
same wait_for_auth helper used by after_handshake) and only return
iroh::endpoint::BeforeConnectOutcome::Accept once
self.is_authenticated(&remote_id).await is true; alternatively return Reject
until authenticated—ensure the debug log still reflects the actual authenticated
state (use RegisterResponse::AlreadyAuthenticated to short-circuit Accept
immediately and keep the wait path for AlreadyInFlight).
In `@src/tests.rs`:
- Around line 57-75: The test_auth_parallel function currently detaches
tokio::spawn tasks and busy-waits on the worked AtomicUsize, losing panics and
risking an infinite loop; collect the spawned task JoinHandle values (or use a
tokio::task::JoinSet) instead of discarding them, await all handles and
propagate panics/results from run_auth_parallel_test so failures surface, and
wrap the join/wait phase in tokio::time::timeout to bound the wait; update
references to the worked Arc, the run_auth_parallel_test calls and the spawn
logic to push handles into a Vec/JoinSet and then await them with timeout.
- Around line 127-155: The test file duplicates the same
wait/timeout/shutdown/is_authenticated logic across run_auth_test and
run_auth_parallel_test; extract a helper like wait_for_auth_terminal(auth_a,
auth_b, timeout) that encapsulates building the wait_loop and awaiting
timeout(AUTH_TIMEOUT * 2, ...), and another helper shutdown_routers(router_a,
router_b) to call router.shutdown().await.ok(); then replace the duplicated
blocks in run_auth_test and run_auth_parallel_test with calls to
wait_for_auth_terminal(&auth_a, &auth_b, AUTH_TIMEOUT * 2) followed by
shutdown_routers(router_a, router_b) and finally use
auth_a.is_authenticated(&endpoint_b.id()).await &&
auth_b.is_authenticated(&endpoint_a.id()).await for the cross-check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d95108f1-0424-4ce3-b57b-4fe66e5ed081
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlREADME.mdexamples/basic.rsexamples/gossip.rssrc/auth.rssrc/error.rssrc/lib.rssrc/protocol.rssrc/secret.rssrc/tests.rs
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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
1 participant
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.
learnings and stability improvements that came out of network stress testing of iroh-lan #PR10
Cargo / versioning
README & examples
Crate layout / large refactor
Public API & traits
Timeouts, constants & caches
Authentication implementation (src/auth.rs)
Protocol integration (src/protocol.rs)
Errors
Tests (new)
Examples adjustments
Lines & churn
Behavioral / migration notes
TODOs / outstanding verification (author notes)