Experimental P2P fix for Linux#1087
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe iroh peer ID helper now normalizes endpoint-style IDs, and the p2p adapter uses that normalization to detect existing connections before dialing. Existing peers now short-circuit connect and replicator setup, with regression tests covering the new behavior. ChangesPeer ID canonicalization and adapter connection checks
Estimated code review effort: 3 (Moderate) | ~25 minutes 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/p2p-adapter/src/iroh.rs`:
- Around line 276-281: The `connected_peers()` call in `IrohAdapter` is
swallowing transport failures by falling back to false, which makes real errors
look like “not connected.” Update the `already_connected` lookup to handle the
`Result` explicitly: propagate or map the error to `P2PError::transport` the
same way other `connected_peers()` call sites do, and only compute
`contains(&peer_id)` on the successful path. Use the `IrohAdapter` logic around
`connected_peers()` and the existing `P2PError::transport` patterns as the
reference points.
🪄 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: CHILL
Plan: Pro
Run ID: aef9c443-ef64-4aba-b61f-06d0e6108dcb
📒 Files selected for processing (1)
crates/p2p-adapter/src/iroh.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: WASM Build Check
- GitHub Check: Build & Test
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use self-documenting code with minimal comments - only comment non-obvious WHY, safety invariants, and public API docs with///. Do not include what the code does, TODO/FIXME comments, commented-out code, or change history
Name files and modules using snake_case (e.g.,lww.rs)
Name types using PascalCase (e.g.,LwwDelta)
Name functions using snake_case (e.g.,encode_priority())
Name constants using SCREAMING_SNAKE_CASE (e.g.,MAX_PRIORITY)
Organize code into one concept per file, preferring small files over large files. Keep files under 200 lines; check consistency if 200-400 lines; consider splitting if over 400 lines
Do not include commented-out code in the codebase
Use issues instead of TODO or FIXME comments - do not include TODO/FIXME comments in code
Files:
crates/p2p-adapter/src/iroh.rs
🔇 Additional comments (2)
crates/p2p-adapter/src/iroh.rs (2)
282-287: 🩺 Stability & AvailabilityEarly-return path skips topic re-subscription.
The full connect path calls
resubscribe_tracked_document_topics()(Line 309) after connecting, but the idempotent early-return path does not. If the peer is technically still inconnected_peers()but topic subscriptions had lapsed (e.g. after a partial network blip), this early return would leave them unrepaired. Worth confirming whether "already connected" truly implies "topics still subscribed" for this transport.
276-287: 🎯 Functional CorrectnessNo issue: keep the fast path
connected_peers()is the source of truth here, andpeer_addressesonly rewrites the display address for peers already reported as connected. A later disconnect is handled by the transport state, so this does not create a lasting false-positive.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/p2p-adapter/src/iroh.rs (1)
49-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSilently swallowed transport errors on the liveness check.
.unwrap_or(false)discards any error fromconnected_peers()without logging, unlike the otherconnected_peers()call sites in this file (e.g. Lines 288-292, 871-873) which propagate the error viaP2PError::transport. The doc comment explains the fallback is intentional (preserve pre-existing dial-on-uncertainty behavior), but a transport failure here is still indistinguishable from "not connected" in the logs, which can hide a real underlying issue during debugging.🩺 Suggested fix: log the error before degrading to `false`
async fn is_transport_connected(&self, peer_id: &p2p::transport::PeerId) -> bool { let canonical_id = canonical_peer_id(peer_id); self.transport .connected_peers() .await .map(|peers| { peers .iter() .any(|peer| canonical_peer_id(peer) == canonical_id) }) - .unwrap_or(false) + .unwrap_or_else(|error| { + tracing::warn!(peer_id = %peer_id, error = %error, "failed to read connected peers; falling back to dial"); + false + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/p2p-adapter/src/iroh.rs` around lines 49 - 67, The liveness check in is_transport_connected currently hides transport failures by turning any connected_peers() error into false. Update this path to log the error before falling back to false, using the same transport logging style as the other connected_peers() call sites in iroh.rs, so a real transport issue is visible while preserving the existing dial-on-uncertainty behavior.
🧹 Nitpick comments (1)
crates/p2p-adapter/src/iroh.rs (1)
998-1191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest module addition pushes
iroh.rswell past the file-size guideline.The new
#[cfg(test)] mod testsblock is well-targeted (covers idempotent reconnect, canonical-ID matching, and a negative-control redial-failure case), but it adds ~190 lines to a file already exceeding the repository's file-size guideline. Consider moving tests into a separate test module/file to keepiroh.rscloser to the recommended size.As per coding guidelines, "Organize code into one concept per file, preferring small files over large files. Keep files under 200 lines; check consistency if 200-400 lines; consider splitting if over 400 lines."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/p2p-adapter/src/iroh.rs` around lines 998 - 1191, The new #[cfg(test)] mod tests block in iroh.rs pushes the file beyond the size guideline; move these tests into a separate test module/file so iroh.rs stays focused on the transport implementation. Keep the existing test coverage intact by relocating the helper symbols and cases such as NoopBlockstore, test_endpoint_config, dialable_ticket, base32_spelling, connect_peer_is_noop_when_already_connected, connect_peer_still_dials_when_not_connected, and add_replicator_skips_dial_when_already_connected.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/p2p-adapter/src/iroh.rs`:
- Around line 49-67: The liveness check in is_transport_connected currently
hides transport failures by turning any connected_peers() error into false.
Update this path to log the error before falling back to false, using the same
transport logging style as the other connected_peers() call sites in iroh.rs, so
a real transport issue is visible while preserving the existing
dial-on-uncertainty behavior.
---
Nitpick comments:
In `@crates/p2p-adapter/src/iroh.rs`:
- Around line 998-1191: The new #[cfg(test)] mod tests block in iroh.rs pushes
the file beyond the size guideline; move these tests into a separate test
module/file so iroh.rs stays focused on the transport implementation. Keep the
existing test coverage intact by relocating the helper symbols and cases such as
NoopBlockstore, test_endpoint_config, dialable_ticket, base32_spelling,
connect_peer_is_noop_when_already_connected,
connect_peer_still_dials_when_not_connected, and
add_replicator_skips_dial_when_already_connected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfbc3584-5168-4861-b1f3-d4c73cfe37af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/p2p-adapter/Cargo.tomlcrates/p2p-adapter/src/iroh.rscrates/p2p/src/iroh/addr.rscrates/p2p/src/iroh/mod.rs
✅ Files skipped from review due to trivial changes (1)
- crates/p2p/src/iroh/mod.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build & Test
- GitHub Check: P2P Integration (Linux)
🧰 Additional context used
📓 Path-based instructions (2)
**/Cargo.toml
📄 CodeRabbit inference engine (CLAUDE.md)
Name crates using lowercase with hyphens (e.g.,
defra-core)
Files:
crates/p2p-adapter/Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use self-documenting code with minimal comments - only comment non-obvious WHY, safety invariants, and public API docs with///. Do not include what the code does, TODO/FIXME comments, commented-out code, or change history
Name files and modules using snake_case (e.g.,lww.rs)
Name types using PascalCase (e.g.,LwwDelta)
Name functions using snake_case (e.g.,encode_priority())
Name constants using SCREAMING_SNAKE_CASE (e.g.,MAX_PRIORITY)
Organize code into one concept per file, preferring small files over large files. Keep files under 200 lines; check consistency if 200-400 lines; consider splitting if over 400 lines
Do not include commented-out code in the codebase
Use issues instead of TODO or FIXME comments - do not include TODO/FIXME comments in code
Files:
crates/p2p/src/iroh/addr.rscrates/p2p-adapter/src/iroh.rs
🔇 Additional comments (8)
crates/p2p/src/iroh/addr.rs (3)
6-6: LGTM!
73-88: LGTM!
223-251: LGTM!crates/p2p-adapter/Cargo.toml (1)
43-45: LGTM!crates/p2p-adapter/src/iroh.rs (4)
16-17: LGTM!
189-206: LGTM!
314-325: LGTM!
496-508: LGTM!
iverc
force-pushed
the
experimental-linux-p2p-fix
branch
from
3d2d078 to
87a53e3
Compare
2 participants
This PR is to show something I have worked on, which seems to get the demo running on Linux.