refactor: runtime defaults into settings#219
Conversation
|
Warning Review limit reached
Next review available in: 30 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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThis PR introduces a centralized ChangesSettings-driven configuration refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Engine
participant EngineActor
participant TorrentActor
participant TrackerActor
Caller->>Engine: new(settings: Option<Settings>)
Engine->>Engine: settings.unwrap_or_default()
Engine->>EngineActor: spawn(EngineActorArgs { settings })
EngineActor->>EngineActor: bind sockets, UDP server from settings
EngineActor->>TorrentActor: spawn(TorrentActorArgs { settings })
TorrentActor->>TrackerActor: spawn(TrackerActorArgs { settings: settings.tracker })
TrackerActor->>TrackerActor: schedule_next_announce using settings
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
artrixdotdev
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
enhancement
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/libtortillas/src/peer/actor.rs (1)
443-460: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a timer path for idle-peer keepalive/disconnect.
These checks run only before the actor blocks on mailbox or stream receive. If a peer becomes fully silent, nothing wakes this loop at
keepalive_timeoutordisconnect_timeout, so stale peer actors can remain open indefinitely. Add a timeout/sleep branch in the receive loop or schedule a self-message for the next deadline.🤖 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/libtortillas/src/peer/actor.rs` around lines 443 - 460, The idle-peer keepalive/disconnect checks in actor receive flow are only evaluated when the loop wakes for mailbox or stream activity, so silent peers can never trigger them. Update the peer actor logic in actor.rs around the receive loop and the last_message/keepalive_timeout/disconnect_timeout handling to add an explicit timer path, either by sleeping until the next deadline or by scheduling a self-message, so the actor wakes up even with no inbound traffic and can send KeepAlive or stop the peer.crates/libtortillas/src/torrent/choking_flow.rs (1)
53-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClamp
peer_stats_concurrencyto at least 1.
buffer_unordered(0)stops this stream from making progress, so a customSettingsvalue of0can hangpeer_stats()and stallrechoke_peers(). Clamp it before passing it intobuffer_unordered.Proposed fix
- .buffer_unordered(self.settings.torrent.peer_stats_concurrency) + .buffer_unordered(self.settings.torrent.peer_stats_concurrency.max(1))🤖 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/libtortillas/src/torrent/choking_flow.rs` around lines 53 - 84, The peer stats collection in peer_stats() can hang if torrent.peer_stats_concurrency is set to 0 because buffer_unordered(0) makes the stream stop progressing. Clamp the configured concurrency to at least 1 before passing it into buffer_unordered, using the peer_stats() flow in choking_flow.rs so rechoke_peers() always completes.crates/libtortillas/src/tracker/udp.rs (1)
691-730: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTransaction registered in
response_channelscan leak on future cancellation.
register_transactionis paired withunregister_transactionvia sequential.awaitcalls, with no cleanup guard. If the future drivingsend_and_recv_retryis dropped mid-flight — which happens whenever an enclosingtokio::time::timeoutfires while a request is in-flight (e.g.connect()'stimeout(connect_timeout, self.send_and_wait(request))below, or the actor'stimeout(self.settings.stop_timeout, self.tracker.stop())intracker/actor.rs) — execution never reaches theunregister_transactioncall afterrecv_registered_response. The entry stays inUdpServer::response_channels(aDashMap) indefinitely, since nothing else ever removes it. Over time (repeated timeouts under degraded network conditions), this accumulates unbounded entries.Now that
udp_connect_timeout/stop_timeoutare independently user-configurable viaTrackerSettings(rather than tuned, fixed constants), a short timeout relative to the retry/backoff budget makes this cancellation path far more likely to be hit in practice.🛡️ Suggested fix — use a cleanup guard so unregistration always runs, even on cancellation
async fn send_and_recv_retry( &self, message: &TrackerRequest, transaction_id: &TransactionId, ) -> anyhow::Result<TrackerResponse, RetryError<TrackerActorError>> { let message_bytes = message.to_bytes(); let (response_tx, mut response_rx) = mpsc::unbounded_channel(); self .server .register_transaction(*transaction_id, response_tx) .await; + struct UnregisterGuard<'a> { + server: &'a UdpServer, + transaction_id: TransactionId, + } + impl Drop for UnregisterGuard<'_> { + fn drop(&mut self) { + let server = self.server.clone(); + let transaction_id = self.transaction_id; + tokio::spawn(async move { + server.unregister_transaction(&transaction_id).await; + }); + } + } + let _guard = UnregisterGuard { server: &self.server, transaction_id: *transaction_id }; // Send the message through the shared message receiver let send_result = self.server.send_message(&message_bytes, self.addr).await; if let Err(err) = send_result { - self.server.unregister_transaction(transaction_id).await; return Err(RetryError::Permanent(err)); } ... - let response = self - .recv_registered_response(transaction_id, &mut response_rx) - .await; - self.server.unregister_transaction(transaction_id).await; - response + self + .recv_registered_response(transaction_id, &mut response_rx) + .await }🤖 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/libtortillas/src/tracker/udp.rs` around lines 691 - 730, `send_and_recv_retry` leaves a registered transaction in `UdpServer::response_channels` if the future is cancelled before the final `unregister_transaction` runs. Add a cleanup guard in `send_and_recv_retry` (or a small RAII helper scoped around `register_transaction`/`recv_registered_response`) so `unregister_transaction(transaction_id)` is always executed even on timeout/cancellation, while preserving the existing early-unregister path on `send_message` failure.crates/libtortillas/src/tracker/actor.rs (1)
146-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnclamped delay when tracker interval is still at its sentinel default.
Duration::from_secs(self.tracker.interval() as u64).max(self.settings.minimum_announce_interval)only enforces a lower bound.HttpTracker/UdpTrackerinitializeintervaltousize::MAX/u32::MAX(seecrates/libtortillas/src/tracker/http.rsinterval: Arc::new(usize::MAX.into())andcrates/libtortillas/src/tracker/udp.rsinterval: Arc::new(AtomicU32::new(u32::MAX))), andannounce()only updates the interval on a successful response. If the very first announce fails,schedule_next_announcecomputes a delay on the order ofu32::MAX/usize::MAXseconds, effectively disabling all further announces/re-tries for that tracker for the lifetime of the actor.🐛 Suggested fix
- let delay = Duration::from_secs(self.tracker.interval() as u64) - .max(self.settings.minimum_announce_interval); + let interval_secs = self.tracker.interval(); + let delay = if interval_secs == u32::MAX as usize || interval_secs == usize::MAX { + self.settings.minimum_announce_interval + } else { + Duration::from_secs(interval_secs as u64).max(self.settings.minimum_announce_interval) + };🤖 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/libtortillas/src/tracker/actor.rs` around lines 146 - 161, In TrackerActor::schedule_next_announce, the delay only has a lower bound today, so the sentinel default from HttpTracker/UdpTracker can produce an absurdly large timeout on the first failed announce. Update the delay calculation to also clamp or replace sentinel/default interval values from self.tracker.interval() before calling SetTimeout::new, using a sane maximum (or a fallback announce interval) so retries continue normally. Keep the existing minimum_announce_interval behavior and preserve the abort-and-reschedule flow in schedule_next_announce.
🧹 Nitpick comments (3)
crates/libtortillas/src/settings.rs (2)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRepeated restart-policy and backoff shape could be extracted.
(limit: u32, period: Duration)is duplicated four times (EngineSettings.torrent_restart_*, andTorrentSettings.scheduler_restart_*,tracker_restart_*,piece_store_restart_*), and the UDP backoff fields inTrackerSettings(udp_retry_initial_delay/factor/max_delay/attempts) form another repeated shape. A small reusable type (e.g.RestartPolicySettings { limit: u32, period: Duration }) would reduce duplication, though this would ripple into the actor files that already destructure these fields directly.Also applies to: 42-61, 106-124
🤖 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/libtortillas/src/settings.rs` around lines 17 - 25, Extract the repeated restart-policy shape into a reusable settings type, since EngineSettings, TorrentSettings, and related actor destructuring all duplicate the same limit/period pair. Introduce a shared RestartPolicySettings (or equivalent) and replace the torrent_restart_*, scheduler_restart_*, tracker_restart_*, and piece_store_restart_* fields with it, then update the corresponding constructors and destructuring sites in the settings and actor code to use the new type. If you also address the TrackerSettings retry/backoff fields, consider a second reusable backoff config type for the udp_retry_* group.
17-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePublic settings fields lack per-field documentation.
All four settings structs expose public fields with no doc comments explaining units/semantics (e.g. is
stop_timeoutvshttp_stop_timeouta fallback vs override? what unit isudp_announce_key?). Since this is a new public API surface, brief doc comments would help consumers.🤖 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/libtortillas/src/settings.rs` around lines 17 - 148, The public fields on EngineSettings, TorrentSettings, PeerSettings, and TrackerSettings are exposed without per-field documentation, making the new API hard to use correctly. Add brief doc comments to each public field in these structs, especially clarifying units, defaults, and semantics for ambiguous values like stop_timeout versus http_stop_timeout and raw tracker/UDP fields such as udp_announce_key and udp_announce_ip_address. Keep the documentation close to the field definitions so the meaning is clear when reading EngineSettings, TorrentSettings, PeerSettings, and TrackerSettings.crates/libtortillas/src/engine/mod.rs (1)
111-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify precedence between
settingsand legacy builder options.These docs still say
Noneuses fixed defaults, but nowNonepreserves values from the suppliedSettings; onlySome(...)overrides them. Please document that precedence to avoid surprising builder users.🤖 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/libtortillas/src/engine/mod.rs` around lines 111 - 134, Update the documentation for the builder options in engine/mod.rs to reflect the actual precedence between `Settings` and the legacy fields: `mailbox_size`, `autostart`, and `sufficient_peers` should be described as overriding the supplied `Settings` only when set to `Some(...)`, while `None` preserves the corresponding value from `Settings` instead of falling back to a fixed default. Keep the wording aligned with the existing field docs so users understand that builder values take precedence only when explicitly provided.
🤖 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/libtortillas/src/engine/actor.rs`:
- Around line 143-147: The UDP startup path in actor initialization currently
panics because UdpServer::new_with_receive_buffer_size unwraps UdpSocket::bind
instead of returning an error. Update UdpServer::new_with_receive_buffer_size to
return a Result and propagate bind failures, then handle that Result in the
actor startup code alongside the TCP/uTP setup by mapping it to
EngineError::NetworkSetupFailed.
---
Outside diff comments:
In `@crates/libtortillas/src/peer/actor.rs`:
- Around line 443-460: The idle-peer keepalive/disconnect checks in actor
receive flow are only evaluated when the loop wakes for mailbox or stream
activity, so silent peers can never trigger them. Update the peer actor logic in
actor.rs around the receive loop and the
last_message/keepalive_timeout/disconnect_timeout handling to add an explicit
timer path, either by sleeping until the next deadline or by scheduling a
self-message, so the actor wakes up even with no inbound traffic and can send
KeepAlive or stop the peer.
In `@crates/libtortillas/src/torrent/choking_flow.rs`:
- Around line 53-84: The peer stats collection in peer_stats() can hang if
torrent.peer_stats_concurrency is set to 0 because buffer_unordered(0) makes the
stream stop progressing. Clamp the configured concurrency to at least 1 before
passing it into buffer_unordered, using the peer_stats() flow in choking_flow.rs
so rechoke_peers() always completes.
In `@crates/libtortillas/src/tracker/actor.rs`:
- Around line 146-161: In TrackerActor::schedule_next_announce, the delay only
has a lower bound today, so the sentinel default from HttpTracker/UdpTracker can
produce an absurdly large timeout on the first failed announce. Update the delay
calculation to also clamp or replace sentinel/default interval values from
self.tracker.interval() before calling SetTimeout::new, using a sane maximum (or
a fallback announce interval) so retries continue normally. Keep the existing
minimum_announce_interval behavior and preserve the abort-and-reschedule flow in
schedule_next_announce.
In `@crates/libtortillas/src/tracker/udp.rs`:
- Around line 691-730: `send_and_recv_retry` leaves a registered transaction in
`UdpServer::response_channels` if the future is cancelled before the final
`unregister_transaction` runs. Add a cleanup guard in `send_and_recv_retry` (or
a small RAII helper scoped around
`register_transaction`/`recv_registered_response`) so
`unregister_transaction(transaction_id)` is always executed even on
timeout/cancellation, while preserving the existing early-unregister path on
`send_message` failure.
---
Nitpick comments:
In `@crates/libtortillas/src/engine/mod.rs`:
- Around line 111-134: Update the documentation for the builder options in
engine/mod.rs to reflect the actual precedence between `Settings` and the legacy
fields: `mailbox_size`, `autostart`, and `sufficient_peers` should be described
as overriding the supplied `Settings` only when set to `Some(...)`, while `None`
preserves the corresponding value from `Settings` instead of falling back to a
fixed default. Keep the wording aligned with the existing field docs so users
understand that builder values take precedence only when explicitly provided.
In `@crates/libtortillas/src/settings.rs`:
- Around line 17-25: Extract the repeated restart-policy shape into a reusable
settings type, since EngineSettings, TorrentSettings, and related actor
destructuring all duplicate the same limit/period pair. Introduce a shared
RestartPolicySettings (or equivalent) and replace the torrent_restart_*,
scheduler_restart_*, tracker_restart_*, and piece_store_restart_* fields with
it, then update the corresponding constructors and destructuring sites in the
settings and actor code to use the new type. If you also address the
TrackerSettings retry/backoff fields, consider a second reusable backoff config
type for the udp_retry_* group.
- Around line 17-148: The public fields on EngineSettings, TorrentSettings,
PeerSettings, and TrackerSettings are exposed without per-field documentation,
making the new API hard to use correctly. Add brief doc comments to each public
field in these structs, especially clarifying units, defaults, and semantics for
ambiguous values like stop_timeout versus http_stop_timeout and raw tracker/UDP
fields such as udp_announce_key and udp_announce_ip_address. Keep the
documentation close to the field definitions so the meaning is clear when
reading EngineSettings, TorrentSettings, PeerSettings, and TrackerSettings.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d470123c-e032-4c08-ba27-79bed9b0b760
📒 Files selected for processing (17)
crates/libtortillas/src/engine/actor.rscrates/libtortillas/src/engine/messages.rscrates/libtortillas/src/engine/mod.rscrates/libtortillas/src/lib.rscrates/libtortillas/src/peer/actor.rscrates/libtortillas/src/settings.rscrates/libtortillas/src/torrent/actor.rscrates/libtortillas/src/torrent/choking.rscrates/libtortillas/src/torrent/choking_flow.rscrates/libtortillas/src/torrent/messages.rscrates/libtortillas/src/torrent/piece_flow.rscrates/libtortillas/src/torrent/swarm.rscrates/libtortillas/src/tracker/actor.rscrates/libtortillas/src/tracker/http.rscrates/libtortillas/src/tracker/mod.rscrates/libtortillas/src/tracker/model.rscrates/libtortillas/src/tracker/udp.rs
1 participant
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Refactor