feat: Send trackers Stopped message when TorrentActor stops
#172
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an async Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TrackerInstance
participant UdpTracker
participant HttpTracker
participant RemoteTracker
Caller->>TrackerInstance: stop()
alt UDP variant
TrackerInstance->>UdpTracker: stop()
rect rgb(240,248,255)
note right of UdpTracker: check Ready state\nset announce_params.event = Stopped
end
UdpTracker->>RemoteTracker: announce(stop)
RemoteTracker-->>UdpTracker: response / timeout
UdpTracker-->>TrackerInstance: Ok(())
else HTTP variant
TrackerInstance->>HttpTracker: stop()
rect rgb(240,248,255)
note right of HttpTracker: set event = Stopped\nbest-effort announce with 3s timeout
end
HttpTracker->>RemoteTracker: announce(stop)
RemoteTracker-->>HttpTracker: response / timeout
HttpTracker-->>TrackerInstance: Ok(())
end
TrackerInstance-->>Caller: Ok(())
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
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/tracker/mod.rs (1)
169-173: Panic risk: unwrap() on UDP announce.This will panic under normal network errors; return the error instead.
Apply this diff:
async fn announce(&self) -> Result<Vec<Peer>> { match self { - TrackerInstance::Udp(tracker) => Ok(tracker.announce().await.unwrap()), + TrackerInstance::Udp(tracker) => tracker.announce().await, TrackerInstance::Http(tracker) => tracker.announce().await, } }
🧹 Nitpick comments (3)
crates/libtortillas/src/tracker/udp.rs (1)
932-951: Make UDP stop best‑effort; don’t error when not Ready and don’t fail shutdown.Currently returns Err if not Ready and propagates announce errors; actor shutdown shouldn’t fail on best‑effort stop.
Apply this diff:
#[instrument(skip(self), fields( tracker_uri = %self.uri, tracker_connection_id = ?self.get_connection_id(), torrent_id = %self.info_hash, tracker_ready_state = ?self.get_ready_state(), ))] async fn stop(&self) -> anyhow::Result<()> { - ensure!( - self.get_ready_state() == ReadyState::Ready, - "Tracker not ready for a stop request" - ); + if self.get_ready_state() != ReadyState::Ready { + trace!("Tracker not ready; skipping stop announce"); + return Ok(()); + } { self.announce_params.write().await.event = Event::Stopped; } - let _ = self.announce().await?; // Discard the response since we don't need it + if let Err(e) = self.announce().await { + debug!(error = %e, "Stop announce failed; ignoring"); + } debug!("Stopped tracker"); Ok(()) }crates/libtortillas/src/tracker/mod.rs (2)
148-150: Clarify stop() semantics as best‑effort.Consider documenting that implementors should bound I/O and avoid propagating errors during shutdown. This aligns actor teardown with resilience goals.
282-292: Don’t propagate stop() errors from on_stop; log and continue.Shutdown should be best‑effort. Propagating errors here can surface spurious failures.
Apply this diff:
async fn on_stop( &mut self, _: WeakActorRef<Self>, reason: ActorStopReason, ) -> Result<(), Self::Error> { match reason { ActorStopReason::Killed | ActorStopReason::Normal => { - self.tracker.stop().await?; - Ok(()) + if let Err(e) = self.tracker.stop().await { + debug!(error = %e, "Tracker stop on actor shutdown failed; ignoring"); + } + Ok(()) } _ => Ok(()), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/libtortillas/src/tracker/http.rs(1 hunks)crates/libtortillas/src/tracker/mod.rs(5 hunks)crates/libtortillas/src/tracker/udp.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/libtortillas/src/tracker/http.rs (2)
crates/libtortillas/src/tracker/mod.rs (3)
uri(121-127)stop(149-149)stop(183-188)crates/libtortillas/src/tracker/udp.rs (1)
stop(938-951)
crates/libtortillas/src/tracker/mod.rs (2)
crates/libtortillas/src/tracker/http.rs (1)
stop(272-280)crates/libtortillas/src/tracker/udp.rs (1)
stop(938-951)
crates/libtortillas/src/tracker/udp.rs (2)
crates/libtortillas/src/tracker/mod.rs (3)
uri(121-127)stop(149-149)stop(183-188)crates/libtortillas/src/tracker/http.rs (1)
stop(272-280)
🔇 Additional comments (1)
crates/libtortillas/src/tracker/mod.rs (1)
183-189: Delegation looks good.TrackerInstance::stop correctly delegates to inner variants.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/libtortillas/src/tracker/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/libtortillas/src/tracker/mod.rs (2)
crates/libtortillas/src/tracker/http.rs (1)
stop(272-280)crates/libtortillas/src/tracker/udp.rs (1)
stop(938-951)
🔇 Additional comments (4)
crates/libtortillas/src/tracker/mod.rs (4)
17-18: LGTM! Necessary imports for theon_stoplifecycle hook.These imports support the new stop functionality added to the actor.
148-149: LGTM! Clean trait method addition.The
stop()method is well-documented and follows the existing trait patterns.
183-188: LGTM! Consistent delegation pattern.The implementation correctly delegates to the underlying tracker implementations and follows the same pattern as other methods in this trait impl.
287-287: Review comment is incorrect—the scenario described is not possible.The review comment assumes initialization could fail while the actor continues running, but this contradicts Kameo's behavior: if Actor::on_start returns an error, the actor will be stopped and will not continue to run/process messages.
In the code path:
TrackerActor::on_startcallsinitialize().await- If initialization fails,
on_startreturnsErr(TrackerActorError::InitializationFailed)and the actor stops- If the actor stops,
on_stopis never invoked, sotracker.stop()is never called- Conversely, when
on_stopexecutes (line 287),on_startmust have succeeded, meaninginitialize()succeeded, which guarantees the tracker was transitioned toReadyState::ReadyTherefore, the UDP tracker's
ReadyState::Readyrequirement instop()is always satisfied when called fromon_stop, and no additional error handling is needed.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/libtortillas/src/tracker/http.rs (1)
267-269: Infinite recursion issue already tracked.This infinite recursion issue was previously identified and marked as out of scope for this PR. It's tracked in issue #173.
🧹 Nitpick comments (1)
crates/libtortillas/src/tracker/mod.rs (1)
282-291: LGTM! Solid best-effort shutdown implementation.The
on_stopimplementation correctly:
- Unconditionally calls
stop()for allActorStopReasonvariants (addressing previous review feedback)- Uses a 5-second timeout to prevent hanging actor shutdown
- Discards errors to ensure shutdown always proceeds
- Returns
Ok(())regardless of outcomeThe timeout duration is appropriate given the HTTP tracker's internal 3-second announce timeout.
Optional: Consider logging all stop errors for consistency.
Currently, only timeout errors are logged via
inspect_err. The HTTP tracker logs its own errors internally, but UDP tracker errors would be silently ignored. For better observability during shutdown, consider:match timeout(Duration::from_secs(5), self.tracker.stop()).await { Ok(Ok(())) => {}, Ok(Err(e)) => warn!(error = %e, "Tracker stop failed; continuing shutdown"), Err(_) => warn!("Tracker stop timed out; continuing shutdown"), }However, the current implementation is acceptable for best-effort shutdown semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/libtortillas/src/tracker/http.rs(2 hunks)crates/libtortillas/src/tracker/mod.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/libtortillas/src/tracker/mod.rs (2)
crates/libtortillas/src/tracker/http.rs (3)
interval(172-174)interval(267-269)stop(275-286)crates/libtortillas/src/tracker/udp.rs (3)
interval(595-597)interval(929-931)stop(938-951)
crates/libtortillas/src/tracker/http.rs (2)
crates/libtortillas/src/tracker/mod.rs (3)
uri(121-127)stop(149-149)stop(183-188)crates/libtortillas/src/tracker/udp.rs (1)
stop(938-951)
🔇 Additional comments (5)
crates/libtortillas/src/tracker/http.rs (2)
18-21: LGTM! Import additions support the new stop() functionality.The addition of
timeoutto the tokio imports is necessary for the bounded stop implementation and is correctly placed.
270-286: LGTM! Excellent best-effort stop implementation.The stop implementation correctly:
- Sets the event to
Stoppedbefore announcing- Uses a bounded 3-second timeout to prevent hanging shutdown
- Logs all outcomes appropriately (success, failure, timeout)
- Always returns
Ok(())to ensure shutdown proceeds regardless of tracker responseThis addresses the previous review feedback and follows best practices for graceful shutdown.
crates/libtortillas/src/tracker/mod.rs (3)
17-18: LGTM! Import additions support the new lifecycle hook.The new imports for
WeakActorRef,ActorStopReason,timeout, andwarnare all necessary for theon_stopimplementation and are correctly organized.Also applies to: 28-29
148-149: LGTM! Trait method addition is well-defined.The
stop()method addition to theTrackerBasetrait is correctly defined with clear documentation about its purpose: stopping the tracker and removing it from the tracker's peer list.
183-188: LGTM! Clean delegation to underlying tracker implementations.The
stop()implementation correctly delegates to both UDP and HTTP tracker variants, following the same pattern as otherTrackerBasemethods inTrackerInstance.
#108
We can't directly use
AsyncDrophere because it's not stable yet. But, we can use theon_stophook for our kameo actor for the same effect (kind of).Summary by CodeRabbit