feat: add typed torrent sources#250
Open
artrixdotdev wants to merge 4 commits into
Open
Conversation
Contributor
WalkthroughThis PR introduces a typed ChangesTyped torrent source
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Engine
participant TorrentSource
participant reqwest
Caller->>Engine: add_torrent(TorrentSource::...)
Engine->>TorrentSource: into_metainfo()
alt Remote URL
TorrentSource->>TorrentSource: validate http/https scheme
TorrentSource->>reqwest: get(url)
reqwest-->>TorrentSource: bytes
end
TorrentSource-->>Engine: MetaInfo or EngineError
Engine-->>Caller: Torrent or EngineError
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/libtortillas/src/engine/mod.rs (2)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the stable facade export explicit.
pub use source::*makes future public items insource.rspart of the engine facade unintentionally. Re-export only the intended stable API item.Proposed change
-pub use source::*; +pub use source::TorrentSource;🤖 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` at line 48, The engine facade is re-exporting everything from source with pub use source::*, which can accidentally expose future internals; update mod.rs to re-export only the intended stable API item from the source module, using the relevant source symbol explicitly so the public surface stays controlled.
295-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a facade-level magnet success test.
The engine facade tests cover file-path success and typed remote mismatch, but not the documented magnet path through
Engine::add_torrent.Proposed test addition
use crate::{ engine::{Engine, TorrentSource}, errors::EngineError, - testing::{BIG_BUCK_BUNNY_INFO_HASH, BIG_BUCK_BUNNY_TORRENT_FILE, torrent_fixture_path}, + testing::{ + BIG_BUCK_BUNNY_INFO_HASH, BIG_BUCK_BUNNY_MAGNET, BIG_BUCK_BUNNY_TORRENT_FILE, + torrent_fixture_path, + }, }; @@ async fn engine_when_torrent_source_is_file_path_then_adds_torrent() { let engine = Engine::builder().autostart(false).build(); let source = TorrentSource::torrent_file_path(torrent_fixture_path(BIG_BUCK_BUNNY_TORRENT_FILE)); @@ assert_eq!(export.torrents.len(), 1); } + + #[tokio::test] + async fn engine_when_torrent_source_is_magnet_uri_then_adds_torrent() { + let engine = Engine::builder().autostart(false).build(); + let source = TorrentSource::magnet(BIG_BUCK_BUNNY_MAGNET); + + let torrent = engine.add_torrent(source).await.unwrap(); + let export = engine.export().await.unwrap(); + + assert_eq!(torrent.info_hash().to_hex(), BIG_BUCK_BUNNY_INFO_HASH); + assert_eq!(export.torrents.len(), 1); + } #[tokio::test] async fn engine_when_torrent_source_type_does_not_match_then_returns_typed_error() {🤖 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 295 - 332, Add a facade-level success test for the magnet path in the tests module under Engine. The current Engine::add_torrent coverage only verifies file-path success and a remote-source type mismatch, so add a new async tokio test that builds Engine with autostart(false), creates a TorrentSource via the magnet constructor, calls add_torrent, and asserts the returned torrent has the expected info hash and appears in Engine::export. Use the existing Engine, TorrentSource, and BIG_BUCK_BUNNY_INFO_HASH symbols to keep the new test aligned with the current facade-level coverage.
🤖 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/mod.rs`:
- Line 248: The remote metainfo fetch path used by add_torrent is unbounded
because source.into_metainfo() can reach metainfo_from_remote_url, which still
relies on an un-timed reqwest::get call. Update the metainfo fetch flow in the
engine/mod.rs path so the request is executed with the existing client timeout
configuration, and make metainfo_from_remote_url use a timeout-aware client or
request builder instead of bare reqwest::get. Keep the fix localized around
source.into_metainfo(), metainfo_from_remote_url, and the add_torrent call chain
so stalled endpoints fail promptly instead of hanging indefinitely.
In `@crates/libtortillas/src/engine/source.rs`:
- Around line 95-98: The MagnetUri parsing in source handling is still mapping
parse failures to MetaInfoDeserializeError, which loses the typed magnet error
path. Update the error mapping in the MagnetUri::parse branch inside the source
parsing flow to return the dedicated typed magnet error variant instead of
MetaInfoDeserializeError, while keeping the existing logging of the malformed
URI and parse error. Make sure the fix is applied in the code path around
MagnetUri::parse and the EngineError handling used by the source parser.
- Around line 201-214: The current test only covers the invalid-scheme failure
path for TorrentSource::remote_torrent_url and does not verify the happy path.
Add a new async test alongside
torrent_source_when_remote_url_is_not_http_then_returns_typed_error that spins
up a local HTTP/HTTPS server serving a real .torrent file, constructs a
remote_torrent_url from that URL, and asserts into_metainfo() returns success.
Keep the existing invalid-scheme test intact and reuse the TorrentSource and
into_metainfo symbols so the new coverage clearly validates the remote download
path.
- Around line 118-123: The remote torrent fetch in `Source::download` currently
uses `reqwest::get(...).bytes()`, which buffers the entire response without
enforcing status checks, timeouts, or a maximum size. Update this path to use a
`reqwest::Client`-based request with a timeout, verify the response is
successful before reading, and stream/read the body with a hard size cap before
converting it into `torrent_file_bytes`. Keep the fix localized around the
existing `reqwest::get`, `EngineError::MetaInfoFetchError`, and
`torrent_file_bytes` handling.
---
Nitpick comments:
In `@crates/libtortillas/src/engine/mod.rs`:
- Line 48: The engine facade is re-exporting everything from source with pub use
source::*, which can accidentally expose future internals; update mod.rs to
re-export only the intended stable API item from the source module, using the
relevant source symbol explicitly so the public surface stays controlled.
- Around line 295-332: Add a facade-level success test for the magnet path in
the tests module under Engine. The current Engine::add_torrent coverage only
verifies file-path success and a remote-source type mismatch, so add a new async
tokio test that builds Engine with autostart(false), creates a TorrentSource via
the magnet constructor, calls add_torrent, and asserts the returned torrent has
the expected info hash and appears in Engine::export. Use the existing Engine,
TorrentSource, and BIG_BUCK_BUNNY_INFO_HASH symbols to keep the new test aligned
with the current facade-level coverage.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 691e3510-a7c0-4055-8536-1b42f76565e7
📒 Files selected for processing (3)
crates/libtortillas/src/engine/mod.rscrates/libtortillas/src/engine/source.rscrates/libtortillas/src/errors.rs
| }; | ||
|
|
||
| pub async fn add_torrent(&self, source: TorrentSource) -> Result<Torrent, EngineError> { | ||
| let metainfo = source.into_metainfo().await?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remote torrent loading is bounded by an explicit reqwest timeout.
rg -n -C4 'reqwest::get|Client::builder|timeout\(' crates/libtortillas/src/engine/source.rsRepository: artrixdotdev/tortillas
Length of output: 456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the loader path and the remote fetch implementation around the reported lines.
sed -n '220,270p' crates/libtortillas/src/engine/mod.rs
printf '\n---\n'
sed -n '100,150p' crates/libtortillas/src/engine/source.rs
printf '\n---\n'
# Look for any timeout/cancellation applied elsewhere in the engine path.
rg -n 'timeout\(|reqwest::Client::builder|reqwest::get|into_metainfo\(' crates/libtortillas/src/engineRepository: artrixdotdev/tortillas
Length of output: 4305
Bound remote torrent fetches with a client timeout. source.into_metainfo() can reach metainfo_from_remote_url, which still uses bare reqwest::get without any timeout. A stalled endpoint can leave add_torrent pending indefinitely.
🤖 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` at line 248, The remote metainfo fetch
path used by add_torrent is unbounded because source.into_metainfo() can reach
metainfo_from_remote_url, which still relies on an un-timed reqwest::get call.
Update the metainfo fetch flow in the engine/mod.rs path so the request is
executed with the existing client timeout configuration, and make
metainfo_from_remote_url use a timeout-aware client or request builder instead
of bare reqwest::get. Keep the fix localized around source.into_metainfo(),
metainfo_from_remote_url, and the add_torrent call chain so stalled endpoints
fail promptly instead of hanging indefinitely.
Comment on lines
+95
to
+98
| MagnetUri::parse(uri.clone()).map_err(|error| { | ||
| error!(magnet_uri = uri, %error); | ||
| EngineError::MetaInfoDeserializeError | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep malformed magnet URIs on the typed error path.
After the magnet: prefix check passes, parse failures are still invalid Magnet sources; returning MetaInfoDeserializeError drops the new typed error details.
Proposed fix
MagnetUri::parse(uri.clone()).map_err(|error| {
error!(magnet_uri = uri, %error);
- EngineError::MetaInfoDeserializeError
+ EngineError::InvalidTorrentSource {
+ source_type: "magnet URI",
+ reason: error.to_string(),
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| MagnetUri::parse(uri.clone()).map_err(|error| { | |
| error!(magnet_uri = uri, %error); | |
| EngineError::MetaInfoDeserializeError | |
| }) | |
| MagnetUri::parse(uri.clone()).map_err(|error| { | |
| error!(magnet_uri = uri, %error); | |
| EngineError::InvalidTorrentSource { | |
| source_type: "magnet URI", | |
| reason: error.to_string(), | |
| } | |
| }) |
🤖 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/source.rs` around lines 95 - 98, The MagnetUri
parsing in source handling is still mapping parse failures to
MetaInfoDeserializeError, which loses the typed magnet error path. Update the
error mapping in the MagnetUri::parse branch inside the source parsing flow to
return the dedicated typed magnet error variant instead of
MetaInfoDeserializeError, while keeping the existing logging of the malformed
URI and parse error. Make sure the fix is applied in the code path around
MagnetUri::parse and the EngineError handling used by the source parser.
Comment on lines
+118
to
+123
| let torrent_file_bytes = reqwest::get(&url) | ||
| .await | ||
| .map_err(EngineError::MetaInfoFetchError)? | ||
| .bytes() | ||
| .await | ||
| .map_err(EngineError::MetaInfoFetchError)?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the crate already has a shared reqwest client, timeout, or body-size limit pattern to reuse.
rg -n -C3 --type rust '\b(reqwest::Client|ClientBuilder|\.timeout\(|error_for_status\(|content_length\(|bytes_stream\(|\.chunk\()' crates/libtortillas || trueRepository: artrixdotdev/tortillas
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and search for other reqwest usage or size/timeout helpers.
set -o pipefail
echo '--- source.rs outline ---'
ast-grep outline crates/libtortillas/src/engine/source.rs --view expanded || true
echo '--- source.rs lines 1-220 ---'
nl -ba crates/libtortillas/src/engine/source.rs | sed -n '1,220p'
echo '--- workspace reqwest-related search ---'
rg -n -C3 --type rust '\b(reqwest::Client|ClientBuilder|\.timeout\(|error_for_status\(|content_length\(|bytes_stream\(|\.chunk\(|reqwest::get\()' . || trueRepository: artrixdotdev/tortillas
Length of output: 1642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- crates/libtortillas/src/engine/source.rs (110-140) ---'
sed -n '110,140p' crates/libtortillas/src/engine/source.rs
echo '--- reqwest usage in workspace ---'
rg -n -C2 --type rust 'reqwest::get\(|reqwest::Client|ClientBuilder|error_for_status\(|content_length\(|bytes_stream\(|\.chunk\(' .
echo '--- torrent/url validation helpers ---'
rg -n -C2 --type rust 'validate_remote_torrent_url|RemoteTorrentUrl|MetaInfoFetchError|MetaInfoDeserializeError' crates/libtortillas/srcRepository: artrixdotdev/tortillas
Length of output: 7147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- crates/libtortillas/src/tracker/http.rs (225-270) ---'
sed -n '225,270p' crates/libtortillas/src/tracker/http.rs
echo '--- reqwest and body-limit helpers in workspace ---'
rg -n -C2 --type rust 'MAX_.*(BODY|SIZE|LEN)|body.*limit|size.*limit|timeout|error_for_status|bytes_stream|content_length|reqwest::get\(' crates/libtortillas/src
echo '--- Cargo.toml reqwest configuration ---'
rg -n -C2 'reqwest' Cargo.toml crates/*/Cargo.tomlRepository: artrixdotdev/tortillas
Length of output: 24609
Bound remote torrent downloads before buffering. reqwest::get(...).bytes() loads the full body from an arbitrary URL into memory, and this path doesn’t apply a timeout or reject non-2xx responses first. Stream the response with a size cap instead.
🤖 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/source.rs` around lines 118 - 123, The remote
torrent fetch in `Source::download` currently uses `reqwest::get(...).bytes()`,
which buffers the entire response without enforcing status checks, timeouts, or
a maximum size. Update this path to use a `reqwest::Client`-based request with a
timeout, verify the response is successful before reading, and stream/read the
body with a hard size cap before converting it into `torrent_file_bytes`. Keep
the fix localized around the existing `reqwest::get`,
`EngineError::MetaInfoFetchError`, and `torrent_file_bytes` handling.
Comment on lines
+201
to
+214
| #[tokio::test] | ||
| async fn torrent_source_when_remote_url_is_not_http_then_returns_typed_error() { | ||
| let source = TorrentSource::remote_torrent_url("file:///tmp/example.torrent"); | ||
|
|
||
| let error = source.into_metainfo().await.unwrap_err(); | ||
|
|
||
| assert!(matches!( | ||
| error, | ||
| EngineError::InvalidTorrentSource { | ||
| source_type: "remote URL", | ||
| .. | ||
| } | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for valid remote torrent success coverage or an HTTP mock fixture.
rg -n -C4 --type rust 'remote_torrent_url|RemoteTorrentUrl|httpmock|mockito|wiremock|TcpListener' crates/libtortillas || trueRepository: artrixdotdev/tortillas
Length of output: 11644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tests in crates/libtortillas/src/engine/source.rs and search the repo for
# any success-path coverage for remote torrent URLs or HTTP test servers/fixtures.
sed -n '145,260p' crates/libtortillas/src/engine/source.rs
printf '\n--- SEARCH: remote URL success coverage ---\n'
rg -n -C3 --type rust 'remote_torrent_url\(|RemoteTorrentUrl|into_metainfo\(\).*await.*unwrap|reqwest::get\(|MetaInfoFetchError|TorrentSource::remote_torrent_url' .
printf '\n--- SEARCH: test HTTP server helpers ---\n'
rg -n -C3 --type rust 'TcpListener|hyper::|axum::|warp::|tiny_http|mockito|httpmock|wiremock|httptest|reqwest::Client|listen\(' crates/libtortillasRepository: artrixdotdev/tortillas
Length of output: 15892
Add a success-path test for remote_torrent_url
crates/libtortillas/src/engine/source.rs only covers the invalid-scheme case here; add a local HTTP/HTTPS test that serves a real .torrent and asserts into_metainfo() succeeds.
🤖 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/source.rs` around lines 201 - 214, The current
test only covers the invalid-scheme failure path for
TorrentSource::remote_torrent_url and does not verify the happy path. Add a new
async test alongside
torrent_source_when_remote_url_is_not_http_then_returns_typed_error that spins
up a local HTTP/HTTPS server serving a real .torrent file, constructs a
remote_torrent_url from that URL, and asserts into_metainfo() returns success.
Keep the existing invalid-scheme test intact and reuse the TorrentSource and
into_metainfo symbols so the new coverage clearly validates the remote download
path.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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.
Summary
TorrentSourcevariants for magnet URIs, local torrent paths, loaded torrent bytes, and remote torrent URLs.Engine::add_torrentto accept typed sources instead of guessing from arbitrary strings.Closes #224
Part of #221
Testing
cargo nextest run -p libtortillas torrent_sourcecargo nextest run -p libtortillas engine_when_torrent_sourcecargo nextest run --workspacecargo test --doc -p libtortillascargo clippy --workspace --all-targets -- -D warningsIgnored network tests were listed with
cargo nextest list --workspace --run-ignored ignored-only; they were not run because this PR only changes typed source validation and local metainfo parsing, while the live tracker/peer behavior remains delegated to existing network code.Summary by CodeRabbit
New Features
Bug Fixes
Documentation