feat: Implement a blocking start method & auto starting functionality #150
Conversation
…sufficient_peers`
WalkthroughWires optional autostart and sufficient_peers through Engine → EngineActor → TorrentActor, replaces the torrent Start signal with stateful SetState(TorrentState::Downloading), introduces torrent readiness hooks and autostart logic, updates message/request/response enums, and adds Torrent APIs: set_auto_start, set_sufficient_peers, poll_ready. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Engine as Engine
participant EActor as EngineActor
participant TActor as TorrentActor
rect rgb(240,248,255)
note over App,Engine: Engine initialized with optional autostart/sufficient_peers
App->>Engine: Engine::new(..., autostart?, sufficient_peers?)
Engine->>EActor: spawn(EngineActorArgs{..., autostart?, sufficient_peers?})
EActor-->>Engine: ready
end
rect rgb(245,255,240)
note over Engine,TActor: Torrent spawn forwards options
Engine->>TActor: spawn_in_thread_with_mailbox(..., autostart?, sufficient_peers?)
TActor->>TActor: on_start(...)\ninit autostart/sufficient_peers/ready_hook
end
rect rgb(255,250,240)
note over EActor,TActor: State-based start
EActor->>TActor: SetState(Downloading)
alt state == Downloading
TActor->>TActor: start()\ninitialize download
end
end
sequenceDiagram
autonumber
participant App as Application
participant Torrent as Torrent (handle)
participant TActor as TorrentActor
rect rgb(250,250,255)
note over Torrent,TActor: Configure readiness and await
App->>Torrent: set_sufficient_peers(N)
Torrent->>TActor: SetSufficientPeers(N)
App->>Torrent: set_auto_start(true/false)
Torrent->>TActor: SetAutoStart(bool)
App->>Torrent: poll_ready()
Torrent->>TActor: ReadyHook(oneshot_tx)
TActor-->>Torrent: oneshot_tx.send() when ready
Torrent-->>App: Ok(())
end
rect rgb(255,245,250)
note over TActor: Mailbox hook checks before processing
TActor->>TActor: next()\ncheck peers/info & autostart\nif ready and not pending_start: start()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-28T06:33:16.003ZApplied to files:
🧬 Code graph analysis (1)crates/libtortillas/src/torrent/messages.rs (2)
🔇 Additional comments (5)
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
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/libtortillas/src/engine/messages.rs (1)
47-55: Two issues: populate PeerId from handshake, and use.value()onget(...).
- The engine parses the handshake but discards the remote
peer_id. Per prior learnings, when passing aPeerStreamthePeershould already carryid. Not doing so can break per‑peer maps or dedupe logic downstream.DashMap::getreturns a guard; call.value()before.tell(...).Suggested fix (assuming a setter/constructor is available):
- if let PeerMessages::Handshake(handshake) = msg { + if let PeerMessages::Handshake(handshake) = msg { let info_hash = *handshake.info_hash; - let peer = Peer::from_socket_addr(peer_addr); + // Ensure PeerId is set from the handshake + let mut peer = Peer::from_socket_addr(peer_addr); + // e.g., peer.set_id(*handshake.peer_id); // or use a constructor that accepts id if let Some(torrent) = self.torrents.get(&info_hash) { - torrent + torrent.value() .tell(TorrentMessage::IncomingPeer(peer, stream)) .await .expect("Failed to tell torrent about incoming peer");If
Peerlacks an API to set the id, please add one or aPeer::from_handshake(addr, peer_id)constructor. I can draft that change if helpful.crates/libtortillas/src/torrent/actor.rs (1)
237-256: Based on my verification of the codebase, I can now provide the rewritten review comment. The evidence shows that the Engine does indeed handle the initial handshake before passing streams to torrent actors, confirming the learnings from previous interactions.Don't re‑handshake when a PeerStream is provided.
The Engine pre‑handshakes incoming peers in
EngineMessage::IncomingPeer(engine/messages.rs:44-47) and extracts the peer ID before forwarding to torrent actors. Sending a second handshake violates protocol and can disconnect valid peers.Apply:
let stream = match stream { - Some(mut stream) => { - let handshake = Handshake::new(info_hash, our_id); - if let Err(err) = stream.send(PeerMessages::Handshake(handshake)).await { - error!("Failed to send handshake to peer: {}", err); - return; - } - stream - } + Some(stream) => { + // Stream already handshaked by Engine; peer.id should be populated + stream + }
🧹 Nitpick comments (7)
crates/libtortillas/src/engine/messages.rs (1)
60-62: Close unexpected-message connections.We log and return, but the socket stays open until drop; explicitly close to avoid dangling sockets.
} else { error!("Received unexpected message from peer"); + drop(stream); }crates/libtortillas/src/engine/mod.rs (1)
90-127: API surface: prefer concrete types with builder defaults overOption<...>here.Using
boolandusizewith builder-provided defaults simplifies usage and avoids tri-state semantics leaking to callers.Example:
- autostart: Option<bool>, + #[builder(default = true)] + autostart: bool, - sufficient_peers: Option<usize>, + #[builder(default = 6)] + sufficient_peers: usize,Then drop the
Optionwrapping through the engine/actor path and default only once.crates/libtortillas/src/torrent/actor.rs (5)
321-346: Doc/code mismatch about JoinSet.Comment says JoinSet; code uses tokio::spawn. Either update the comment or switch to JoinSet.
461-469: Autostart check placement.Running autostart before recv() can add a tiny latency to message handling. Consider moving the check after one mailbox recv to reduce head‑of‑line blocking.
Possible tweak:
- if !self.pending_start { - self.autostart().await; - } - mailbox_rx.recv().await + let sig = mailbox_rx.recv().await; + if !self.pending_start { + self.autostart().await; + } + sig
505-518: Avoid flaky network dependency in poll_ready test.With no peers added, poll_ready will block unless threshold is 0 and equality is accepted. Make the test deterministic.
Apply:
- let sufficient_peers = 6; + let sufficient_peers = 0; @@ - Some(false), // We don't need to autostart because we're only checking if we have peers + Some(false), // Just checking readiness; autostart not needed Some(sufficient_peers),And ensure the autostart threshold fix (>=) is applied.
Run tests offline to confirm no hangs.
Also applies to: 520-523
626-633: Third test likely flakes or hangs.You poll_ready and then expect on-disk piece files without creating peers. Either:
- set autostart Some(true) and sufficient_peers Some(0) and inject a local mock PeerActor, or
- gate this as a network test with #[ignore] or a feature flag, or
- refactor to unit-test get_piece_path and piece file IO separately.
Example minimal change to make poll_ready deterministic:
- None, - None, + Some(true), + Some(0),But you’ll still need a peer to actually write a piece; otherwise the loop will spin.
218-223: Doc typo: “zeroes” → “ones”.is_full uses count_ones; fix the comment to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/libtortillas/src/engine/actor.rs(4 hunks)crates/libtortillas/src/engine/messages.rs(3 hunks)crates/libtortillas/src/engine/mod.rs(2 hunks)crates/libtortillas/src/torrent/actor.rs(13 hunks)crates/libtortillas/src/torrent/messages.rs(5 hunks)crates/libtortillas/src/torrent/mod.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: artrixdotdev
PR: artrixdotdev/tortillas#125
File: crates/libtortillas/src/torrent/actor.rs:151-171
Timestamp: 2025-08-28T06:33:16.003Z
Learning: In the torrent actor's append_peer function in crates/libtortillas/src/torrent/actor.rs, when a PeerStream is provided, the peer has already been pre-handshaked by the engine. The engine handles the initial handshake validation, info hash verification, and peer ID extraction before passing the stream to the torrent actor. Therefore, the peer.id should already be populated when a stream is provided.
📚 Learning: 2025-08-28T06:33:16.003Z
Learnt from: artrixdotdev
PR: artrixdotdev/tortillas#125
File: crates/libtortillas/src/torrent/actor.rs:151-171
Timestamp: 2025-08-28T06:33:16.003Z
Learning: In the torrent actor's append_peer function in crates/libtortillas/src/torrent/actor.rs, when a PeerStream is provided, the peer has already been pre-handshaked by the engine. The engine handles the initial handshake validation, info hash verification, and peer ID extraction before passing the stream to the torrent actor. Therefore, the peer.id should already be populated when a stream is provided.
Applied to files:
crates/libtortillas/src/torrent/mod.rscrates/libtortillas/src/engine/messages.rscrates/libtortillas/src/torrent/messages.rscrates/libtortillas/src/torrent/actor.rs
📚 Learning: 2025-08-18T18:25:24.831Z
Learnt from: kurealnum
PR: artrixdotdev/tortillas#99
File: crates/libtortillas/src/engine/mod.rs:153-155
Timestamp: 2025-08-18T18:25:24.831Z
Learning: The peer stream handling logic in the Engine's EngineMessage::IncomingPeer handler in crates/libtortillas/src/engine/mod.rs is temporary and planned to be migrated to separate code in the future, so extensive refactoring may not be worthwhile in this location.
Applied to files:
crates/libtortillas/src/engine/messages.rs
🧬 Code graph analysis (4)
crates/libtortillas/src/engine/actor.rs (1)
crates/libtortillas/src/torrent/actor.rs (1)
autostart(171-193)
crates/libtortillas/src/engine/mod.rs (1)
crates/libtortillas/src/torrent/actor.rs (1)
autostart(171-193)
crates/libtortillas/src/torrent/messages.rs (1)
crates/libtortillas/src/torrent/mod.rs (1)
state(264-276)
crates/libtortillas/src/torrent/actor.rs (3)
crates/libtortillas/src/torrent/mod.rs (5)
actor(105-107)start(247-257)info_hash(110-112)new(97-99)poll_ready(336-346)crates/libtortillas/src/engine/mod.rs (2)
actor(145-147)new(90-142)crates/libtortillas/src/engine/actor.rs (1)
on_start(95-135)
🔇 Additional comments (22)
crates/libtortillas/src/engine/messages.rs (2)
67-69: Switch to stateful start — LGTM.
99-110: Args ordering verified — tuple matches spawn call.
TorrentActor::Args is (PeerId, MetaInfo, Arc, UdpServer, Option, PieceStorageStrategy, Option, Option) and the spawn_in_thread_with_mailbox call forwards (self.peer_id, *metainfo, self.utp_socket.clone(), self.udp_server.clone(), None, self.default_piece_storage_strategy.clone(), self.autostart, self.sufficient_peers) in the same order.crates/libtortillas/src/engine/mod.rs (2)
117-126: Doc defaults vs. implementation source of truth — ensure consistency.Docs say autostart defaults to true, sufficient_peers to 6. Here they’re
Options passed through unchanged; defaulting must happen downstream (likely in TorrentActor). Please confirm and align docs at the layer that actually applies defaults.
135-137: Pass-through of new options — LGTM.crates/libtortillas/src/engine/actor.rs (3)
55-59: New config fields carried through — LGTM.
98-108: Destructuring of extended Args tuple — LGTM.
131-134: Mailbox default applied; new options stored verbatim — confirm downstream defaulting.Since
autostart/sufficient_peersremainOptions here, verify TorrentActor resolvesNoneto the documented defaults.crates/libtortillas/src/torrent/mod.rs (3)
248-256: Stateful start call path — LGTM.
265-275: State retrieval via GetState — LGTM.
278-305: New tuning APIs (set_auto_start,set_sufficient_peers) — LGTM.crates/libtortillas/src/torrent/messages.rs (4)
18-20: New imports (ReadyHook, TorrentState) — LGTM.
59-69: Message surface changes — LGTM.SetState/SetAutoStart/SetSufficientPeers/ReadyHook additions look coherent with the new API.
317-321: State transition triggers start — LGTM.Side effect on Downloading is clear and local.
130-138: Engine pre-verification comment vs. peer-id propagation.Docs say engine pre-verifies handshake; ensure the
Peerbuilt in engine/messages.rs carriespeer_idfrom the handshake before sendingIncomingPeer(...). Otherwise peer identity–dependent logic in the actor may misbehave.crates/libtortillas/src/torrent/actor.rs (8)
6-6: LGTM on imports.Instant/oneshot/tracing additions make sense for readiness and timing.
Also applies to: 14-15
67-69: State doc tweak looks good.
84-87: ReadyHook alias: good choice.Helps keep poll_ready wiring clean.
109-120: New actor fields: sane defaults and placement.
381-383: Args plumb‑through and defaults: looks right.Defaults (autostart=true, sufficient_peers=6) match PR intent.
Also applies to: 388-398, 453-458
484-484: Test imports: fine.
558-560: Magnet test spawn: OK.Keeps autostart off; loop waits for info dict only.
169-194: Verified — ReadyHook & SetState wiring present (no action required).poll_ready sends ReadyHook (crates/libtortillas/src/torrent/mod.rs:336–341) and the actor stores/handles it (crates/libtortillas/src/torrent/messages.rs:329–331); start() sends TorrentMessage::SetState(Downloading) (crates/libtortillas/src/torrent/mod.rs:248) and the actor sets the state and calls self.start().await (crates/libtortillas/src/torrent/messages.rs:317–320).
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 (1)
crates/libtortillas/src/torrent/messages.rs (1)
154-157: Critical: BitVec initialized with capacity only; len=0 causes panics/logic errors.with_capacity leaves length at 0. set_aliased(index, true), is_full(), first_zero(), etc. will misbehave; start() may think we’re “full” immediately. Initialize length.
Apply:
- self.bitfield = Arc::new(BitVec::with_capacity(info.piece_count())); + self.bitfield = Arc::new(BitVec::repeat(false, info.piece_count()));
♻️ Duplicate comments (2)
crates/libtortillas/src/torrent/actor.rs (1)
221-227: Off‑by‑one fixed in readiness check (>=).Threshold logic is now correct for sufficient_peers.
crates/libtortillas/src/torrent/messages.rs (1)
329-336: Fix ReadyHook race: fire immediately when already started (or seeding).If autostart ran before the hook was installed, poll_ready can hang. Send immediately when state != Inactive. Keep existing immediate‑send when ready and autostart=false.
Apply:
- TorrentMessage::ReadyHook(hook) => { - let is_ready = self.is_ready_to_start(); - if is_ready && !self.autostart { - let _ = hook.send(()); - } else { - self.ready_hook = Some(hook); - self.autostart().await; - } - } + TorrentMessage::ReadyHook(hook) => { + // If we've already transitioned out of Inactive, we're "ready" from the caller's POV. + if self.state != TorrentState::Inactive { + let _ = hook.send(()); + return; + } + let is_ready = self.is_ready_to_start(); + if is_ready && !self.autostart { + let _ = hook.send(()); + } else { + self.ready_hook = Some(hook); + self.autostart().await; + } + }
🧹 Nitpick comments (4)
crates/libtortillas/src/torrent/actor.rs (3)
192-212: Unconditional ready_hook send after start() is correct; consider also setting start_time when Seeding.Currently start_time is only set when Downloading. If Seeding on start, you may still want a baseline timestamp for metrics.
Apply within the Seeding branch:
if self.is_full() { self.state = TorrentState::Seeding; info!(id = %self.info_hash(), "Torrent is now seeding"); + self.start_time = Some(Instant::now());
465-473: next() ordering triggers the ReadyHook race; fix in ReadyHook handler is safer than reordering here.Keeping autostart() pre‑recv minimizes latency, but requires ReadyHook to short‑circuit when already started. See my messages.rs ReadyHook change.
488-488: Tests: add timeouts around poll_ready to avoid hangs in CI.Trackers/peers are external; without peers, these can block indefinitely.
Apply e.g.:
- assert!(torrent.poll_ready().await.is_ok()); + use tokio::time::{timeout, Duration}; + assert!(timeout(Duration::from_secs(60), torrent.poll_ready()).await.is_ok());Also applies to: 510-512, 520-522, 524-527, 599-599, 630-637
crates/libtortillas/src/torrent/messages.rs (1)
140-167: Reduce potentially huge debug logs for invalid Info dicts.Dumping the entire info bytes can be large. Log hash and size instead.
- debug!(dict = %String::from_utf8_lossy(&bytes), "Received info dict when we already have one"); + debug!(len = bytes.len(), "Received info dict when we already have one");And similarly in the invalid hash branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/libtortillas/src/torrent/actor.rs(13 hunks)crates/libtortillas/src/torrent/messages.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T06:33:16.003Z
Learnt from: artrixdotdev
PR: artrixdotdev/tortillas#125
File: crates/libtortillas/src/torrent/actor.rs:151-171
Timestamp: 2025-08-28T06:33:16.003Z
Learning: In the torrent actor's append_peer function in crates/libtortillas/src/torrent/actor.rs, when a PeerStream is provided, the peer has already been pre-handshaked by the engine. The engine handles the initial handshake validation, info hash verification, and peer ID extraction before passing the stream to the torrent actor. Therefore, the peer.id should already be populated when a stream is provided.
Applied to files:
crates/libtortillas/src/torrent/messages.rscrates/libtortillas/src/torrent/actor.rs
🧬 Code graph analysis (2)
crates/libtortillas/src/torrent/messages.rs (2)
crates/libtortillas/src/torrent/mod.rs (1)
state(264-276)crates/libtortillas/src/torrent/actor.rs (1)
is_ready(221-223)
crates/libtortillas/src/torrent/actor.rs (3)
crates/libtortillas/src/torrent/mod.rs (5)
actor(105-107)start(247-257)info_hash(110-112)new(97-99)poll_ready(336-346)crates/libtortillas/src/engine/mod.rs (2)
actor(145-147)new(90-142)crates/libtortillas/src/engine/actor.rs (1)
on_start(95-135)
🔇 Additional comments (5)
crates/libtortillas/src/torrent/actor.rs (2)
84-87: ReadyHook alias looks good and is appropriately scoped.Clear internal API; good fit for poll_ready.
248-260: Incoming pre‑handshaked peers: ensure capability bits are set.Engine pre‑handshakes and sets peer.id (per prior learnings), but reserved/capability bits may not be present on Peer. In the Some(stream) path you send the handshake but never call determine_supported() nor set reserved. Please confirm engine populates these; otherwise call determine_supported() here.
Also applies to: 267-305
crates/libtortillas/src/torrent/messages.rs (3)
57-69: State/control message split looks good.SetState/SetAutoStart/SetSufficientPeers/ReadyHook additions cleanly separate concerns.
317-322: SetState handler correctly delegates Downloading to start().Good separation; avoids duplicating start logic.
118-120: State request/response rename is consistent.GetState plumbing looks correct.
Also applies to: 375-376
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 (2)
crates/libtortillas/src/torrent/messages.rs (2)
268-283: Fix off‑by‑one: seeding starts one piece earlyAfter incrementing next_piece, you compare against piece_count - 1 which will flip to Seeding one piece too soon (e.g., after completing piece N-2). Compare using cur_piece (the just‑completed index) or next_piece >= piece_count.
Apply:
- if self.next_piece >= piece_count - 1 { + let last_index = piece_count.saturating_sub(1); + if cur_piece >= last_index { // Handle end of torrenting process self.state = TorrentState::Seeding; info!("Torrenting process completed, switching to seeding mode"); } else { self .broadcast_to_peers(PeerTell::NeedPiece(self.next_piece, 0, BLOCK_SIZE)) .await; }
156-161: Initialize bitfield length; current code can panic on set_aliasedBitVec::with_capacity sets capacity, not length. Later set_aliased(index, true) will index out of bounds.
Apply:
- self.bitfield = Arc::new(BitVec::with_capacity(info.piece_count())); + let mut bf = BitVec::<AtomicU8>::with_capacity(info.piece_count()); + bf.resize(info.piece_count(), false); + self.bitfield = Arc::new(bf);
♻️ Duplicate comments (1)
crates/libtortillas/src/torrent/messages.rs (1)
329-343: ReadyHook immediate resolve: looks good and addresses prior feedbackThis implements the “resolve immediately if already ready/non‑Inactive” suggestion from the earlier review. Nice.
Also, please confirm that is_ready_to_start() exists (or is_ready was renamed) and is consistent with the engine’s readiness definition.
#!/bin/bash # Verify readiness helper presence and call sites rg -n -C2 -g '!**/target/**' --type=rust '\bis_ready_to_start\s*\(' ast-grep --pattern $'impl $_ { $$$ fn is_ready_to_start($$$) { $$$ } $$$ }'
🧹 Nitpick comments (3)
crates/libtortillas/src/torrent/messages.rs (3)
71-85: Improve Debug output for new control messagesIncluding the new variants in Debug helps tracing.
Apply:
match self { + TorrentMessage::SetState(state) => write!(f, "SetState({state:?})"), + TorrentMessage::SetAutoStart(auto) => write!(f, "SetAutoStart({auto})"), + TorrentMessage::SetSufficientPeers(n) => write!(f, "SetSufficientPeers({n})"), + TorrentMessage::ReadyHook(_) => write!(f, "ReadyHook(_)"), TorrentMessage::InfoBytes(bytes) => write!(f, "InfoBytes({bytes:?})"), TorrentMessage::KillPeer(peer_id) => write!(f, "KillPeer({peer_id:?})"), TorrentMessage::KillTracker(tracker) => write!(f, "KillTracker({tracker:?})"), TorrentMessage::AddPeer(peer) => write!(f, "AddPeer({peer:?})"), TorrentMessage::IncomingPiece(index, offset, data) => { write!(f, "IncomingPiece({index}, {offset}, {})", data.len()) } TorrentMessage::Announce(peers) => write!(f, "Announce({peers:?})"), _ => write!(f, "TorrentMessage"), // Add more later,
141-147: Avoid logging entire info dict payloadLogging raw info bytes can be large and leak data. Log size/summary instead.
Apply:
- debug!( - dict = %String::from_utf8_lossy(&bytes), - "Received info dict when we already have one" - ); + debug!(len = bytes.len(), "Received info dict when we already have one");- warn!( - dict = %String::from_utf8_lossy(&bytes), - "Received invalid info hash" - ); + warn!(len = bytes.len(), "Received invalid info hash");Also applies to: 162-166
1-1: Remove unnecessary importpanic! macro doesn’t require use core::panic; this import is likely unused.
Apply:
-use core::panic;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/libtortillas/src/torrent/messages.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T06:33:16.003Z
Learnt from: artrixdotdev
PR: artrixdotdev/tortillas#125
File: crates/libtortillas/src/torrent/actor.rs:151-171
Timestamp: 2025-08-28T06:33:16.003Z
Learning: In the torrent actor's append_peer function in crates/libtortillas/src/torrent/actor.rs, when a PeerStream is provided, the peer has already been pre-handshaked by the engine. The engine handles the initial handshake validation, info hash verification, and peer ID extraction before passing the stream to the torrent actor. Therefore, the peer.id should already be populated when a stream is provided.
Applied to files:
crates/libtortillas/src/torrent/messages.rs
🧬 Code graph analysis (1)
crates/libtortillas/src/torrent/messages.rs (2)
crates/libtortillas/src/torrent/mod.rs (1)
state(264-276)crates/libtortillas/src/torrent/actor.rs (1)
is_ready(221-223)
🔇 Additional comments (5)
crates/libtortillas/src/torrent/messages.rs (5)
317-322: State transition before start(): verify start() preconditionsYou set self.state = state before calling start(). If start() expects to transition state itself or requires Inactive, this could skip guards.
If start() is idempotent and tolerant, ignore; otherwise, set state after a successful start() or let start() own the transition.
18-20: Imports update looks correctReadyHook and util are now referenced in this module; import changes are appropriate.
59-69: New control messages: good surfaceSetState/SetAutoStart/SetSufficientPeers/ReadyHook additions align with the PR goals.
118-120: GetState renaming is consistent end‑to‑endRequest/response rename to GetState is coherent with the frontend API.
Also applies to: 382-383
203-207: Handle last‑piece size correctlyPiece math uses info_dict.piece_length for all pieces. The last piece is often shorter; current math will over‑expect blocks and can stall completion.
Suggested approach:
- Compute piece_len for the given index (standard piece_length except for last piece, which is total_length - piece_length*(count-1)).
- Use piece_len to size total_blocks and to compute next_block_len/overflow checks.
If Info exposes a total length helper, wire it like:
- let total_blocks = (info_dict.piece_length as usize).div_ceil(BLOCK_SIZE); + let piece_len = self.piece_len_for(&info_dict, index); // last piece aware + let total_blocks = piece_len.div_ceil(BLOCK_SIZE);- let is_overflowing = offset + BLOCK_SIZE > info_dict.piece_length as usize; - let next_block_len = if is_overflowing { - info_dict.piece_length as usize - offset + let piece_len = self.piece_len_for(&info_dict, index); + let is_overflowing = offset + BLOCK_SIZE > piece_len; + let next_block_len = if is_overflowing { + piece_len.saturating_sub(offset) } else { BLOCK_SIZE };Add outside this file (example):
// crates/libtortillas/src/torrent/actor.rs impl TorrentActor { fn piece_len_for(&self, info: &Info, index: usize) -> usize { let standard = info.piece_length as usize; let count = info.piece_count(); if index + 1 < count { standard } else { // last piece info.total_length() - standard * (count - 1) } } }If Info lacks total_length(), derive it from file list. I can wire this once you confirm Info’s API.
Also applies to: 294-299
Currently, in order to "wait until we're ready to start downloading", we have to run a loop to check if we have the info dict, and we have sufficient peers. This PR aims to create a centralized, and reusable way to "wait" until we are ready to start the downloading process. The features include:
poll_readyfunction for "blocking" until we're ready to downloadStartmessage in favor ofSetStateautostartproperty toTorrentActorto tell the actor if it should start the torrent automatically or not (without having to be polled and explicitly told to start)Torrentderive from themTorrentstruct to interact with all of these things for fine tuning of the engine's defaultsSummary by CodeRabbit
New Features
Refactor