diff --git a/breadarrd/src/importer/mod.rs b/breadarrd/src/importer/mod.rs index d2ceded..e5d8774 100644 --- a/breadarrd/src/importer/mod.rs +++ b/breadarrd/src/importer/mod.rs @@ -335,6 +335,25 @@ fn move_or_copy_file(src: &Path, dest: &Path) -> Result<()> { }) } +/// Removes whatever's left of a torrent's download folder once every file +/// breadarr cares about has already been moved out of it — split out from +/// `run_import_cycle` so it's directly testable without a real qBittorrent +/// server. `move_or_copy_file` only ever relocates the one video file it +/// located; a multi-file release's own folder (sample clips, `.nfo`/`.srt`/ +/// `.jpg` sidecars) is otherwise left behind forever, since nothing else +/// ever points at it once the torrent itself is gone from qBittorrent. A +/// bare file (a release with no wrapping folder) needs no cleanup here — +/// `move_or_copy_file` already consumed it. The `!= host_downloads_path` +/// guard is defense in depth against a malformed `content_path` resolving +/// to the downloads root itself; every real torrent lands in its own +/// subdirectory or as a single file, never the root. +fn cleanup_leftover_download_dir(content_path: &Path, host_downloads_path: &str) -> Result<()> { + if content_path.is_dir() && content_path != Path::new(host_downloads_path) { + std::fs::remove_dir_all(content_path)?; + } + Ok(()) +} + /// The copy fallback's actual mechanics, split out so it's directly /// testable without needing a real cross-filesystem boundary to force /// `rename` to fail. @@ -1354,10 +1373,18 @@ pub async fn run_import_cycle( // the torrent itself is just forgotten rather than left sitting around // in a "files missing" error state. Best-effort: a failed delete here // never undoes or blocks the import that already succeeded. - for hash in &imported_hashes { + for (hash, content_path) in &imported_hashes { if let Err(e) = qbit.delete_torrent(hash).await { tracing::warn!(hash, error = %e, "failed to remove completed torrent from qbittorrent"); } + if let Err(e) = cleanup_leftover_download_dir(content_path, host_downloads_path) { + tracing::warn!( + hash, + path = %content_path.display(), + error = %e, + "failed to remove leftover download directory after import" + ); + } } // Best-effort, same reasoning as the `delete_torrent` cleanup just @@ -1394,16 +1421,20 @@ fn process_pending_grabs( container_downloads_path: &str, host_downloads_path: &str, transcode_cfg: Option<&breadarr_shared::config::TranscodeConfig>, -) -> Result<(ImportStats, Vec)> { +) -> Result<(ImportStats, Vec<(String, PathBuf)>)> { let mut stats = ImportStats::default(); - // Torrent hashes whose data has been fully dealt with this cycle — moved - // into the library, or deleted outright because a better file already - // existed (see `ImportOutcome::SkippedAlreadyHaveBetter`, whose only - // producer, `import_one`, deletes the losing download itself). Either - // way nothing remains at the torrent's original location, so the caller - // (`run_import_cycle`, the async I/O boundary this function is - // deliberately kept free of) removes each from qBittorrent afterward. - let mut imported_hashes = Vec::new(); + // Torrents whose data has been fully dealt with this cycle — moved into + // the library, or deleted outright because a better file already existed + // (see `ImportOutcome::SkippedAlreadyHaveBetter`, whose only producer, + // `import_one`, deletes the losing download itself). Either way nothing + // *breadarr still needs* remains at the torrent's original location, so + // the caller (`run_import_cycle`, the async I/O boundary this function is + // deliberately kept free of) removes each from qBittorrent afterward and + // clears out whatever's left of `content_path` — a multi-file release + // only ever has its video moved out by `move_or_copy_file`, so without + // this the surrounding folder (sample clips, .nfo/.srt/.jpg sidecars) + // sits there forever with no torrent left to account for it. + let mut imported_hashes: Vec<(String, PathBuf)> = Vec::new(); for grab in pending { let Some(torrent) = torrents.iter().find(|t| t.hash == grab.torrent_hash()) else { @@ -1467,7 +1498,7 @@ fn process_pending_grabs( stats.imported += outcome.episodes_imported; stats.quality_flagged += outcome.quality_flagged; if outcome.episodes_imported > 0 { - imported_hashes.push(grab.torrent_hash().to_string()); + imported_hashes.push((grab.torrent_hash().to_string(), content_path.clone())); } } Err(e) => { @@ -1505,13 +1536,13 @@ fn process_pending_grabs( if quality_flagged { stats.quality_flagged += 1; } - imported_hashes.push(grab.torrent_hash().to_string()); + imported_hashes.push((grab.torrent_hash().to_string(), content_path.clone())); } Ok(ImportOutcome::SkippedAlreadyHaveBetter) => { // The download's data is already handled — `import_one` // deleted the losing file itself — so there's nothing left // for qBittorrent to track either. - imported_hashes.push(grab.torrent_hash().to_string()); + imported_hashes.push((grab.torrent_hash().to_string(), content_path.clone())); } Err(e) => { let error_count = record_import_error(conn, grab.release_id())?; @@ -3225,6 +3256,57 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn cleanup_leftover_download_dir_removes_a_folder_with_only_sidecars_left() { + let downloads = + std::env::temp_dir().join(format!("breadarr-cleanup-test-{}", std::process::id())); + let release_dir = downloads.join("Some Movie 2016 1080p"); + std::fs::create_dir_all(&release_dir).unwrap(); + // The video was already moved out by `move_or_copy_file`; only the + // sidecar junk qBittorrent downloaded alongside it remains. + std::fs::write(release_dir.join("poster.jpg"), b"jpeg bytes").unwrap(); + std::fs::write(release_dir.join("release.nfo"), b"nfo text").unwrap(); + + cleanup_leftover_download_dir(&release_dir, &downloads.to_string_lossy()).unwrap(); + + assert!(!release_dir.exists(), "the now-empty-of-video release folder should be gone"); + assert!(downloads.exists(), "the shared downloads root itself must survive"); + + std::fs::remove_dir_all(&downloads).unwrap(); + } + + #[test] + fn cleanup_leftover_download_dir_is_a_noop_for_a_bare_file() { + let downloads = + std::env::temp_dir().join(format!("breadarr-cleanup-file-test-{}", std::process::id())); + std::fs::create_dir_all(&downloads).unwrap(); + // A release with no wrapping folder — `move_or_copy_file` already + // consumed it, so `content_path` no longer exists at all. + let bare = downloads.join("Some.Movie.2016.1080p.mp4"); + + cleanup_leftover_download_dir(&bare, &downloads.to_string_lossy()).unwrap(); + + std::fs::remove_dir_all(&downloads).unwrap(); + } + + #[test] + fn cleanup_leftover_download_dir_refuses_to_remove_the_downloads_root_itself() { + let downloads = std::env::temp_dir() + .join(format!("breadarr-cleanup-guard-test-{}", std::process::id())); + std::fs::create_dir_all(&downloads).unwrap(); + std::fs::write(downloads.join("unrelated-other-download.mkv"), b"data").unwrap(); + + // A malformed `content_path` that happens to equal the downloads + // root itself must never be wiped, even though it passes the + // `is_dir()` check. + cleanup_leftover_download_dir(&downloads, &downloads.to_string_lossy()).unwrap(); + + assert!(downloads.exists()); + assert!(downloads.join("unrelated-other-download.mkv").exists()); + + std::fs::remove_dir_all(&downloads).unwrap(); + } + #[test] fn locates_a_single_file_torrent() { let dir = std::env::temp_dir().join(format!("breadarr-test-{}", std::process::id())); diff --git a/breadarrd/src/qbit/mod.rs b/breadarrd/src/qbit/mod.rs index ec9af4b..7510c7f 100644 --- a/breadarrd/src/qbit/mod.rs +++ b/breadarrd/src/qbit/mod.rs @@ -40,6 +40,40 @@ struct AddTorrentResponse { success_count: u32, #[serde(default)] failure_count: u32, + /// Present when adding by URL rather than by a magnet/`.torrent` blob + /// qBittorrent already has in hand — the torrent itself is fetched + /// asynchronously, so the immediate response has neither succeeded nor + /// failed yet, just queued. nyaa's RSS feed always hands over a + /// `.torrent` download URL (never a magnet), so this is the normal + /// shape for every anime grab, not an edge case. Verified live against + /// the deployed qBittorrent: a URL add returns HTTP 202 with + /// `{"pending_count":1,"success_count":0,"failure_count":0}` — treating + /// `success_count == 0` alone as rejection (the previous check) silently + /// dropped every one of these while the torrent downloaded successfully + /// in the background, with no DB record and no log line. + #[serde(default)] + pending_count: u32, +} + +/// Interprets a `torrents/add` response body — split out from `add_magnet` +/// so it's directly testable without a live qBittorrent server. +/// qBittorrent's add-torrent endpoint returns HTTP 200/202 even when it +/// rejects the magnet outright (a dead/malformed hash, one it already knows +/// is unreachable) — the response body is the only signal. Older +/// qBittorrent versions returned plain text ("Ok." vs "Fails."); newer ones +/// return a JSON summary with success_count/failure_count/pending_count +/// instead (verified live against the currently deployed version). Without +/// checking whichever shape is actually in play, a rejected magnet looks +/// identical to a real success: the caller records a `release` row as +/// grabbed and nothing ever downloads, silently and permanently (verified +/// live — this happened for a real release under the old text-only check, +/// and separately for every nyaa URL-add under a since-fixed +/// `success_count == 0` check that didn't account for `pending_count`). +fn add_torrent_response_is_rejected(body: &str) -> bool { + match serde_json::from_str::(body) { + Ok(r) => r.failure_count > 0 || (r.success_count == 0 && r.pending_count == 0), + Err(_) => body.trim() != "Ok.", + } } pub struct QbitClient { @@ -157,23 +191,7 @@ impl QbitClient { if !status.is_success() { bail!("qbit add-torrent failed: status={status} body={body:?}"); } - // qBittorrent's add-torrent endpoint returns HTTP 200 even when - // it rejects the magnet outright (a dead/malformed hash, one it - // already knows is unreachable) — the response body is the only - // signal. Older qBittorrent versions returned plain text ("Ok." - // vs "Fails."); newer ones return a JSON summary with - // success_count/failure_count instead (verified live against - // the currently deployed version). Without checking whichever - // shape is actually in play, a rejected magnet looks identical - // to a real success: the caller records a `release` row as - // grabbed and nothing ever downloads, silently and permanently - // (verified live — this happened for a real release under the - // old text-only check). - let rejected = match serde_json::from_str::(&body) { - Ok(r) => r.failure_count > 0 || r.success_count == 0, - Err(_) => body.trim() != "Ok.", - }; - if rejected { + if add_torrent_response_is_rejected(&body) { return Err(anyhow::Error::new(MagnetRejected { body })); } return Ok(()); @@ -292,4 +310,38 @@ mod tests { None ); } + + #[test] + fn a_pending_url_add_is_not_treated_as_rejected() { + // Exact shape qBittorrent 5.x returns for a URL add (every nyaa + // grab) — the torrent is queued for an async fetch, not yet + // succeeded or failed. This is the shape that used to be + // misread as an outright rejection. + let body = r#"{"added_torrent_ids":[],"failure_count":0,"pending_count":1,"success_count":0}"#; + assert!(!add_torrent_response_is_rejected(body)); + } + + #[test] + fn a_genuine_failure_is_still_rejected() { + let body = r#"{"failure_count":1,"pending_count":0,"success_count":0}"#; + assert!(add_torrent_response_is_rejected(body)); + } + + #[test] + fn an_in_band_success_is_not_rejected() { + // TPB/1337x magnets: qBittorrent already has the info hash, no + // async fetch needed, so success_count is set immediately. + let body = r#"{"failure_count":0,"pending_count":0,"success_count":1}"#; + assert!(!add_torrent_response_is_rejected(body)); + } + + #[test] + fn the_old_plain_text_ok_response_is_not_rejected() { + assert!(!add_torrent_response_is_rejected("Ok.")); + } + + #[test] + fn the_old_plain_text_fails_response_is_rejected() { + assert!(add_torrent_response_is_rejected("Fails.")); + } } diff --git a/breadarrd/src/scheduler.rs b/breadarrd/src/scheduler.rs index 9cd10a7..1af0718 100644 --- a/breadarrd/src/scheduler.rs +++ b/breadarrd/src/scheduler.rs @@ -613,6 +613,33 @@ async fn process_item( let torrent_hash = match grab_and_capture_hash(qbit, &item.link, qbit_category).await { Ok(hash) => hash, Err(e) if e.downcast_ref::().is_some() => { + // Same reasoning as `HashCaptureFailed` just below: recording + // this as `failed` rather than leaving it with no release row + // at all is what makes a genuine rejection visible and frees + // the episode/movie to be re-searched later with a different + // candidate. A silent `Ok(MagnetRejected)` with nothing written + // used to be indistinguishable from a real success once + // `mark_seen` ran — a since-fixed bug in the response-rejection + // check (see `qbit::add_torrent_response_is_rejected`'s doc + // comment) made every nyaa URL-add hit this path even though + // the torrent was actually downloading, so this arm went + // unnoticed for a long time; logging it now that it only fires + // on real rejections. + tracing::warn!(title = %item.title, guid = %item.guid, "qbittorrent rejected this release's magnet/torrent"); + record_grab( + conn, + media_item.id, + episode_id, + season_pack_number, + source_id, + &item.title, + &item.guid, + release_score, + item.size_bytes, + qbit_category, + None, + "failed", + )?; return Ok(ProcessOutcome::MagnetRejected); } Err(e) => return Err(e), @@ -761,6 +788,7 @@ pub struct GrabCycleStats { pub grabbed: usize, pub errors: usize, pub queued_for_review: usize, + pub magnet_rejected: usize, } pub async fn run_grab_cycle( @@ -811,6 +839,7 @@ pub async fn run_grab_cycle( match outcome { ProcessOutcome::Grabbed { .. } => stats.grabbed += 1, ProcessOutcome::QueuedForReview => stats.queued_for_review += 1, + ProcessOutcome::MagnetRejected => stats.magnet_rejected += 1, _ => {} } }