diff --git a/breadarrd/src/importer/mod.rs b/breadarrd/src/importer/mod.rs index e5d8774..d2ceded 100644 --- a/breadarrd/src/importer/mod.rs +++ b/breadarrd/src/importer/mod.rs @@ -335,25 +335,6 @@ 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. @@ -1373,18 +1354,10 @@ 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, content_path) in &imported_hashes { + for hash 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 @@ -1421,20 +1394,16 @@ fn process_pending_grabs( container_downloads_path: &str, host_downloads_path: &str, transcode_cfg: Option<&breadarr_shared::config::TranscodeConfig>, -) -> Result<(ImportStats, Vec<(String, PathBuf)>)> { +) -> Result<(ImportStats, Vec)> { let mut stats = ImportStats::default(); - // 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(); + // 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(); for grab in pending { let Some(torrent) = torrents.iter().find(|t| t.hash == grab.torrent_hash()) else { @@ -1498,7 +1467,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(), content_path.clone())); + imported_hashes.push(grab.torrent_hash().to_string()); } } Err(e) => { @@ -1536,13 +1505,13 @@ fn process_pending_grabs( if quality_flagged { stats.quality_flagged += 1; } - imported_hashes.push((grab.torrent_hash().to_string(), content_path.clone())); + imported_hashes.push(grab.torrent_hash().to_string()); } 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(), content_path.clone())); + imported_hashes.push(grab.torrent_hash().to_string()); } Err(e) => { let error_count = record_import_error(conn, grab.release_id())?; @@ -3256,57 +3225,6 @@ 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 7510c7f..ec9af4b 100644 --- a/breadarrd/src/qbit/mod.rs +++ b/breadarrd/src/qbit/mod.rs @@ -40,40 +40,6 @@ 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 { @@ -191,7 +157,23 @@ impl QbitClient { if !status.is_success() { bail!("qbit add-torrent failed: status={status} body={body:?}"); } - if add_torrent_response_is_rejected(&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 { return Err(anyhow::Error::new(MagnetRejected { body })); } return Ok(()); @@ -310,38 +292,4 @@ 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 1af0718..9cd10a7 100644 --- a/breadarrd/src/scheduler.rs +++ b/breadarrd/src/scheduler.rs @@ -613,33 +613,6 @@ 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), @@ -788,7 +761,6 @@ pub struct GrabCycleStats { pub grabbed: usize, pub errors: usize, pub queued_for_review: usize, - pub magnet_rejected: usize, } pub async fn run_grab_cycle( @@ -839,7 +811,6 @@ 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, _ => {} } }