From 3ac6ca2da6c95d7941a905149de8d087482a0184 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 22 Aug 2026 19:54:20 +0100 Subject: [PATCH] fix(dl): address Transmission review --- Cargo.lock | 181 ++++++++++++++++++++++++++++++++++ crates/arr-dl/Cargo.toml | 3 + crates/arr-dl/src/lib.rs | 205 +++++++++++++++++++++++++++++++++++---- 3 files changed, 368 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7e079ea..93af2eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,15 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "aho-corasick" +version = "1.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c982642fa9e8606056828ee9a8505737230110bb1099153c79efe865c59d12ba" +dependencies = [ + "memchr", +] + [[package]] name = "allocator-api2" version = "0.2.21" @@ -59,6 +68,7 @@ dependencies = [ "thiserror", "tokio", "url", + "wiremock", ] [[package]] @@ -86,6 +96,16 @@ version = "0.1.0" name = "arr-probe" version = "0.1.0" +[[package]] +name = "assert-json-diff" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e4f2b81832e72834d7518d8487a0396a28cc408186a2e8854c0f98011faf12" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "atoi" version = "2.0.0" @@ -269,6 +289,24 @@ dependencies = [ "typenum", ] +[[package]] +name = "deadpool" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0be2b1d1d6ec8d846f05e137292d0b89133caf95ef33695424c09568bdd39b1b" +dependencies = [ + "deadpool-runtime", + "lazy_static", + "num_cpus", + "tokio", +] + +[[package]] +name = "deadpool-runtime" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "092966b41edc516079bdf31ec78a2e0588d1d0c08f78b91d8307215928642b2b" + [[package]] name = "der" version = "0.7.10" @@ -378,6 +416,12 @@ dependencies = [ "spin", ] +[[package]] +name = "fnv" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" + [[package]] name = "foldhash" version = "0.1.5" @@ -393,6 +437,21 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "futures" +version = "0.3.34" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a31d2a3fbaaeb2af2368bbdd904aa8e812d3c04a1ee10d3171f52d556e5d0a3" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + [[package]] name = "futures-channel" version = "0.3.34" @@ -437,6 +496,17 @@ version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "53c0fa8157de1303bfffdaa1cc2a673bfffb60102f76b0ef4441659124373fed" +[[package]] +name = "futures-macro" +version = "0.3.34" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9fb9654ba8355388abeb8dcb4fc62f511300867002afc858860463bdd9fe0c44" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.3", +] + [[package]] name = "futures-sink" version = "0.3.34" @@ -455,8 +525,10 @@ version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0d50a92467f8ba5dd6e3ee5d4bd04d73ab2e4e1c44474a0674821dfce14b79bc" dependencies = [ + "futures-channel", "futures-core", "futures-io", + "futures-macro", "futures-sink", "futures-task", "memchr", @@ -501,6 +573,25 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "h2" +version = "0.4.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "839c0e8a181239723652be9062bb56ca5bf5f64011f73b623f6f4fc59086a228" +dependencies = [ + "atomic-waker", + "bytes", + "fnv", + "futures-core", + "futures-sink", + "http", + "indexmap", + "slab", + "tokio", + "tokio-util", + "tracing", +] + [[package]] name = "hashbrown" version = "0.15.5" @@ -533,6 +624,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hermit-abi" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" + [[package]] name = "hex" version = "0.4.3" @@ -605,6 +702,12 @@ version = "1.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6dbf3de79e51f3d586ab4cb9d5c3e2c14aa28ed23d180cf89b4df0454a69cc87" +[[package]] +name = "httpdate" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9" + [[package]] name = "hyper" version = "1.11.0" @@ -615,9 +718,11 @@ dependencies = [ "bytes", "futures-channel", "futures-core", + "h2", "http", "http-body", "httparse", + "httpdate", "itoa", "pin-project-lite", "smallvec", @@ -974,6 +1079,16 @@ dependencies = [ "libm", ] +[[package]] +name = "num_cpus" +version = "1.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91df4bbde75afed763b708b7eee1e8e7651e02d97f6d5dd763e89367e957b23b" +dependencies = [ + "hermit-abi", + "libc", +] + [[package]] name = "once_cell" version = "1.21.4" @@ -1235,6 +1350,35 @@ dependencies = [ "bitflags", ] +[[package]] +name = "regex" +version = "1.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f020237b6c8eed93db2e2cb53c00c60a8e1bc73da7d073199a1180401450218d" +dependencies = [ + "aho-corasick", + "memchr", + "regex-automata", + "regex-syntax", +] + +[[package]] +name = "regex-automata" +version = "0.4.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ad8553b9b26413251cbf30e620595c7a41b3887f03da04579c0e6b0d6a06b4b2" +dependencies = [ + "aho-corasick", + "memchr", + "regex-syntax", +] + +[[package]] +name = "regex-syntax" +version = "0.8.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" + [[package]] name = "reqwest" version = "0.12.28" @@ -1898,6 +2042,20 @@ dependencies = [ "tokio", ] +[[package]] +name = "tokio-util" +version = "0.7.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "494815d09bf52b5548659851081238f0ca39ff638363907596da739561c62c52" +dependencies = [ + "bytes", + "futures-core", + "futures-sink", + "libc", + "pin-project-lite", + "tokio", +] + [[package]] name = "toml" version = "0.8.23" @@ -2434,6 +2592,29 @@ dependencies = [ "memchr", ] +[[package]] +name = "wiremock" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08db1edfb05d9b3c1542e521aea074442088292f00b5f28e435c714a98f85031" +dependencies = [ + "assert-json-diff", + "base64", + "deadpool", + "futures", + "http", + "http-body-util", + "hyper", + "hyper-util", + "log", + "once_cell", + "regex", + "serde", + "serde_json", + "tokio", + "url", +] + [[package]] name = "writeable" version = "0.6.4" diff --git a/crates/arr-dl/Cargo.toml b/crates/arr-dl/Cargo.toml index 277fc2b..0a00a4c 100644 --- a/crates/arr-dl/Cargo.toml +++ b/crates/arr-dl/Cargo.toml @@ -15,5 +15,8 @@ thiserror = { workspace = true } tokio = { workspace = true } url = { workspace = true } +[dev-dependencies] +wiremock = { workspace = true } + [lints] workspace = true diff --git a/crates/arr-dl/src/lib.rs b/crates/arr-dl/src/lib.rs index 56d2d00..df8edaa 100644 --- a/crates/arr-dl/src/lib.rs +++ b/crates/arr-dl/src/lib.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; use std::sync::Arc; +use std::time::Duration; use base64::Engine as _; use reqwest::{StatusCode, Url}; @@ -11,6 +12,8 @@ use tokio::sync::RwLock; const SESSION_HEADER: &str = "x-transmission-session-id"; const MAX_SESSION_NEGOTIATIONS: usize = 4; +const CONNECT_TIMEOUT: Duration = Duration::from_secs(5); +const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); /// A magnet URI or the bytes of a `.torrent` file. #[derive(Clone, Debug, Eq, PartialEq)] @@ -49,6 +52,7 @@ pub enum TorrentState { Downloading, QueuedToSeed, Seeding, + Unknown(u8), } /// Current torrent state, read directly from Transmission. @@ -68,6 +72,10 @@ pub struct Torrent { pub enum Error { #[error("invalid Transmission RPC URL: {0}")] InvalidUrl(#[from] url::ParseError), + #[error("download directory is not valid UTF-8")] + NonUtf8DownloadDir, + #[error("could not build Transmission HTTP client: {0}")] + BuildClient(reqwest::Error), #[error("Transmission request failed: {0}")] Http(#[from] reqwest::Error), #[error("Transmission did not return a session ID after {0} attempts")] @@ -96,7 +104,11 @@ impl TransmissionClient { pub fn new(endpoint: &str) -> Result { Ok(Self { endpoint: Url::parse(endpoint)?, - http: reqwest::Client::new(), + http: reqwest::Client::builder() + .connect_timeout(CONNECT_TIMEOUT) + .timeout(REQUEST_TIMEOUT) + .build() + .map_err(Error::BuildClient)?, session_id: Arc::new(RwLock::new(None)), }) } @@ -108,12 +120,20 @@ impl TransmissionClient { /// Returns an error for transport failures or rejected/malformed RPC /// responses. pub async fn add_torrent(&self, request: AddTorrent) -> Result { + let download_dir = request + .download_dir + .to_str() + .ok_or(Error::NonUtf8DownloadDir)? + .to_owned(); + let label = request.label; + let seed_ratio_limit = request.seed_ratio_limit; + let seed_idle_limit = request.seed_idle_limit_minutes; let mut arguments = json!({ - "download-dir": request.download_dir, - "labels": [request.label], - "seedRatioLimit": request.seed_ratio_limit, + "download-dir": download_dir, + "labels": [label], + "seedRatioLimit": seed_ratio_limit, "seedRatioMode": 1, - "seedIdleLimit": request.seed_idle_limit_minutes, + "seedIdleLimit": seed_idle_limit, "seedIdleMode": 1, }); @@ -138,6 +158,26 @@ impl TransmissionClient { let torrent: RpcAddedTorrent = serde_json::from_value(torrent) .map_err(|error| Error::InvalidResponse(error.to_string()))?; + if was_duplicate { + self.call( + "torrent-set", + json!({ + "ids": [torrent.id], + "labels": [label], + "seedRatioLimit": seed_ratio_limit, + "seedRatioMode": 1, + "seedIdleLimit": seed_idle_limit, + "seedIdleMode": 1, + }), + ) + .await?; + self.call( + "torrent-set-location", + json!({"ids": [torrent.id], "location": download_dir, "move": false}), + ) + .await?; + } + Ok(AddedTorrent { id: torrent.id, name: torrent.name, @@ -170,11 +210,7 @@ impl TransmissionClient { let response: RpcTorrentList = serde_json::from_value(arguments) .map_err(|error| Error::InvalidResponse(error.to_string()))?; - response - .torrents - .into_iter() - .map(Torrent::try_from) - .collect() + Ok(response.torrents.into_iter().map(Torrent::from).collect()) } /// Remove one torrent, optionally deleting its downloaded data. @@ -265,10 +301,8 @@ struct RpcTorrent { labels: Vec, } -impl TryFrom for Torrent { - type Error = Error; - - fn try_from(value: RpcTorrent) -> Result { +impl From for Torrent { + fn from(value: RpcTorrent) -> Self { let state = match value.status { 0 => TorrentState::Stopped, 1 => TorrentState::QueuedToVerify, @@ -277,14 +311,10 @@ impl TryFrom for Torrent { 4 => TorrentState::Downloading, 5 => TorrentState::QueuedToSeed, 6 => TorrentState::Seeding, - status => { - return Err(Error::InvalidResponse(format!( - "unknown torrent status {status}" - ))); - } + status => TorrentState::Unknown(status), }; - Ok(Self { + Self { id: value.id, name: value.name, hash: value.hash, @@ -292,6 +322,139 @@ impl TryFrom for Torrent { progress: value.progress, download_dir: value.download_dir, labels: value.labels, - }) + } + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use serde_json::json; + use wiremock::matchers::{body_partial_json, method}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + use super::{AddTorrent, Error, TorrentSource, TorrentState, TransmissionClient}; + + #[tokio::test] + async fn retries_the_session_handshake() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with( + ResponseTemplate::new(409).insert_header("x-transmission-session-id", "session"), + ) + .with_priority(1) + .up_to_n_times(1) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(body_partial_json(json!({"method": "torrent-get"}))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "result": "success", + "arguments": {"torrents": []} + }))) + .with_priority(2) + .expect(1) + .mount(&server) + .await; + + let client = TransmissionClient::new(&server.uri()).expect("client"); + assert!(client.list_torrents().await.expect("list").is_empty()); + } + + #[tokio::test] + async fn duplicate_reapplies_label_location_and_seed_limits() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(body_partial_json(json!({"method": "torrent-add"}))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "result": "success", + "arguments": {"torrent-duplicate": { + "id": 7, "name": "duplicate", "hashString": "abc" + }} + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(body_partial_json(json!({ + "method": "torrent-set", + "arguments": { + "ids": [7], "labels": ["movies-main"], + "seedRatioLimit": 1.5, "seedIdleLimit": 60 + } + }))) + .respond_with(success()) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(body_partial_json(json!({ + "method": "torrent-set-location", + "arguments": {"ids": [7], "location": "/downloads", "move": false} + }))) + .respond_with(success()) + .expect(1) + .mount(&server) + .await; + + let client = TransmissionClient::new(&server.uri()).expect("client"); + let added = client + .add_torrent(AddTorrent { + source: TorrentSource::Magnet("magnet:?xt=urn:btih:abc".into()), + label: "movies-main".into(), + download_dir: PathBuf::from("/downloads"), + seed_ratio_limit: 1.5, + seed_idle_limit_minutes: 60, + }) + .await + .expect("duplicate"); + + assert!(added.was_duplicate); + } + + #[tokio::test] + #[cfg(unix)] + async fn rejects_non_utf8_download_directory() { + use std::os::unix::ffi::OsStringExt as _; + + let client = TransmissionClient::new("http://127.0.0.1:1").expect("client"); + let error = client + .add_torrent(AddTorrent { + source: TorrentSource::Magnet("magnet:?xt=urn:btih:abc".into()), + label: "movies-main".into(), + download_dir: PathBuf::from(std::ffi::OsString::from_vec(vec![0xff])), + seed_ratio_limit: 1.5, + seed_idle_limit_minutes: 60, + }) + .await + .expect_err("invalid path"); + + assert!(matches!(error, Error::NonUtf8DownloadDir)); + } + + #[tokio::test] + async fn preserves_unknown_status_without_hiding_the_list() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "result": "success", + "arguments": {"torrents": [{ + "id": 1, "name": "future", "hashString": "abc", "status": 99, + "percentDone": 0.5, "downloadDir": "/downloads", "labels": [] + }]} + }))) + .expect(1) + .mount(&server) + .await; + + let client = TransmissionClient::new(&server.uri()).expect("client"); + let torrents = client.list_torrents().await.expect("list"); + assert_eq!(torrents[0].state, TorrentState::Unknown(99)); + } + + fn success() -> ResponseTemplate { + ResponseTemplate::new(200).set_body_json(json!({"result": "success", "arguments": {}})) } }