diff --git a/breadarr-shared/src/config.rs b/breadarr-shared/src/config.rs index 5d0ee05..5945368 100644 --- a/breadarr-shared/src/config.rs +++ b/breadarr-shared/src/config.rs @@ -297,14 +297,42 @@ pub struct TranscodeConfig { pub poll_interval_secs: u64, #[serde(default = "default_vaapi_device")] pub vaapi_device: String, + /// Applies to both pipelines identically when Jellyfin reports an + /// active transcoding session — deliberately not split per-pipeline; + /// when someone's actually watching something, both the GPU (which + /// they're using) and CPU (contending for the same box) should back off. #[serde(default = "default_parallelism_min")] pub parallelism_min: usize, - /// Ramp-up ceiling for concurrent encode streams during a backfill — - /// tune this against how many simultaneous `av1_vaapi` sessions the - /// target GPU can actually sustain before per-stream throughput starts - /// dropping, not just picked arbitrarily. + /// The total concurrent **live-action** (`av1_vaapi`, GPU-bound) stream + /// budget — not just "the batch job's cap", but the real ceiling the + /// GPU can sustain at all, batch work and live Jellyfin viewers + /// combined. `transcode::live_action_parallelism_for` subtracts however + /// many Jellyfin transcode sessions are actually active from this + /// number to get the batch job's actual parallelism each cycle, so + /// real viewers get exactly the headroom they need rather than the + /// batch job dropping to a flat minimum regardless of how many people + /// are watching. Deliberately separate from `parallelism_max_anime` — + /// the two pipelines contend for genuinely different hardware (GPU + /// encode engine vs CPU threads), so raising one shouldn't raise the + /// other. Empirically calibrated on Hestia's Arc A380: per-stream + /// throughput stays above 1.5x realtime through 7 concurrent streams, + /// crosses below it at 8 (aggregate throughput itself plateaus around + /// ~12x realtime from 5-6 streams on, i.e. the GPU's actual saturation + /// point) — see [[breadarr-av1-transcode]] for the full scaling-test + /// numbers. #[serde(default = "default_parallelism_max")] pub parallelism_max: usize, + /// Ramp-up ceiling for concurrent **anime** (`libsvtav1`, CPU-bound) + /// encode streams — capped separately from `parallelism_max` (see its + /// doc comment) precisely because a single shared cap would let "raise + /// GPU parallelism" accidentally also raise anime concurrency, and + /// anime jobs are CPU-thread-hungry (`anime_svtav1_max_threads` each) + /// in a way live-action jobs aren't. Kept at the original + /// conservative shared-cap default (2) since concurrent-anime-job + /// memory/CPU behavior at higher counts hasn't been load-tested the + /// way the live-action GPU path has. + #[serde(default = "default_parallelism_max_anime")] + pub parallelism_max_anime: usize, /// The "looks fine, no complaints" calibration reference: a real /// bitrate (Mbps, in kbps here) from content already in the library at /// `reference_height` that the user is happy with. New AV1 encodes are @@ -332,6 +360,82 @@ pub struct TranscodeConfig { /// anyway) — revisit once HDR handling is confirmed safe. #[serde(default = "default_exclude_min_height")] pub exclude_min_height: u32, + /// `global_quality` for the live-action `av1_vaapi` `QVBR` encode — the + /// actual quality driver now that rate control is quality-based rather + /// than a flat bitrate target (see `run_ffmpeg_encode_live_action`). + /// `reference_bitrate_kbps`/`av1_efficiency_factor` still compute a + /// `-b:v`/`-maxrate`/`-bufsize` ceiling alongside this, so a source that's + /// already unusually efficient doesn't get inflated up toward the + /// ceiling — QVBR only spends up to it on content that actually needs it. + #[serde(default = "default_quality_live_action")] + pub quality_live_action: u32, + /// Root folder path prefixes (exact string prefix match against + /// `episode_file.path`) routed to the anime encode pipeline + /// (`run_ffmpeg_encode_anime`) instead of the live-action one, regardless + /// of `anime_mapping`/`anime_tmdb_movie` metadata coverage — path is a + /// more reliable signal than TVDB/TMDB anime-list membership, which has + /// real gaps (e.g. Avatar: The Last Airbender and some Dragon Ball movies + /// were missing from those tables and slipped through as "not anime"). + /// Empty by default (a no-op) — set per-deployment to match how the + /// library is actually organized. + #[serde(default)] + pub anime_root_folders: Vec, + /// CRF for the anime pipeline's software `libsvtav1` encode (0-63, lower + /// = higher quality/larger). No hardware AV1 10-bit encode entrypoint + /// exists on Hestia's Arc A380 (`vainfo` only lists `AV1Profile0`, + /// 8-bit) — anime needs true 10-bit output to avoid banding in the flat + /// gradients the art style is full of, so this pipeline trades GPU + /// offload for CPU-based `libsvtav1` specifically to get it. + #[serde(default = "default_quality_anime")] + pub quality_anime: u32, + /// `libsvtav1` preset (0-13, lower = slower/better compression AND more + /// memory-hungry — SVT-AV1's lookahead/reference buffering scales with + /// preset, not just thread count). Raised from an initial guess of 6 to + /// 10 after a real validation run hit a genuine kernel OOM: preset 6 on + /// a single 1080p anime episode grew to 9.3GB resident memory on + /// Hestia's 6-core/12-thread box. This runs as unattended background + /// work, so trading some compression efficiency for a much smaller, + /// safer memory footprint is the right call — see + /// `anime_svtav1_max_threads` for the other half of that fix. + #[serde(default = "default_anime_svtav1_preset")] + pub anime_svtav1_preset: u32, + /// Passed to `libsvtav1` as `-svtav1-params lp=N` — caps how many + /// worker threads it uses, independent of preset. More parallel workers + /// means more concurrently-buffered frames, so this is the other lever + /// (alongside `anime_svtav1_preset`) for bounding the encoder's peak + /// memory to something predictable regardless of how many cores the + /// host actually has. Default is conservative (well under a typical + /// modern host's core count) after the same OOM incident that raised + /// the preset default. + #[serde(default = "default_anime_svtav1_max_threads")] + pub anime_svtav1_max_threads: u32, + /// Hard floor on what counts as "worth keeping": a transcode whose + /// output isn't at least this fraction smaller than the original is + /// discarded (job marked `skipped`, original left untouched) rather than + /// swapped in. Exists because quality-driven rate control can still + /// occasionally produce an output that's the same size as or larger than + /// an already-efficient source — this is the invariant that makes that + /// safe regardless of how good the rate-control tuning is, after a real + /// incident where flat-bitrate VBR targeting silently produced files + /// *larger* than the original on the majority of a backfill. + #[serde(default = "default_min_size_reduction_pct")] + pub min_size_reduction_pct: f64, + /// Skip attempting a transcode at all (no GPU/CPU time spent) when the + /// source's current bitrate is already at or below this fraction of the + /// resolution-scaled ceiling (`target_bitrate_kbps`) — a strong signal + /// there's little room left to save, so it's not worth the encode time + /// to find out (the `min_size_reduction_pct` check above would reject + /// most of these anyway, this just avoids paying for that finding). + #[serde(default = "default_skip_below_ceiling_ratio")] + pub skip_below_ceiling_ratio: f64, + /// Size (seconds) of each of the three start/middle/end windows + /// `ffprobe::verify_decodable_sampled` actually decodes, instead of the + /// whole file — a full decode verification was measured as the actual + /// CPU bottleneck of a transcode cycle (400%+ CPU per job, dwarfing the + /// GPU encode time), not the encode itself. Bounds verification cost to + /// a small constant regardless of source length. + #[serde(default = "default_verify_sample_secs")] + pub verify_sample_secs: f64, } impl Default for TranscodeConfig { @@ -342,11 +446,20 @@ impl Default for TranscodeConfig { vaapi_device: default_vaapi_device(), parallelism_min: default_parallelism_min(), parallelism_max: default_parallelism_max(), + parallelism_max_anime: default_parallelism_max_anime(), reference_bitrate_kbps: default_reference_bitrate_kbps(), reference_height: default_reference_height(), av1_efficiency_factor: default_av1_efficiency_factor(), exclude_hdr: default_exclude_hdr(), exclude_min_height: default_exclude_min_height(), + quality_live_action: default_quality_live_action(), + anime_root_folders: Vec::new(), + quality_anime: default_quality_anime(), + anime_svtav1_preset: default_anime_svtav1_preset(), + anime_svtav1_max_threads: default_anime_svtav1_max_threads(), + min_size_reduction_pct: default_min_size_reduction_pct(), + skip_below_ceiling_ratio: default_skip_below_ceiling_ratio(), + verify_sample_secs: default_verify_sample_secs(), } } } @@ -364,12 +477,25 @@ fn default_parallelism_min() -> usize { } fn default_parallelism_max() -> usize { - // Conservative on purpose: a real incident on a shared 15GB host - // running a dozen+ other containers showed concurrent 1080p/4K - // decode+encode sessions can push memory pressure into swap fast - // enough to trigger the OOM killer well before the GPU itself is the - // bottleneck. Raise this deliberately, per-deployment, once you've - // watched `free -h` under real load at the current setting. + // The measured total GPU budget (not "batch cap plus a static + // reservation") — `live_action_parallelism_for` dynamically subtracts + // real Jellyfin transcode sessions from this each cycle. Raised from + // an initial conservative guess of 2 after an actual concurrent-stream + // scaling test on Hestia's Arc A380 (see `parallelism_max`'s doc + // comment): per-stream throughput stays above 1.5x realtime through 7 + // total concurrent streams, crossing below at 8. + 7 +} + +fn default_parallelism_max_anime() -> usize { + // Kept at the original conservative shared-cap value — unlike + // `parallelism_max`, this hasn't been load-tested at higher counts. + // A real incident already showed a *single* uncapped anime job could + // hit 9.3GB resident memory; multiple concurrent anime jobs (each its + // own `anime_svtav1_max_threads`-sized thread pool) multiply both CPU + // thread contention and memory pressure in a way the GPU path doesn't + // have to worry about. Raise deliberately, per-deployment, only after + // watching `free -h` and CPU load under real concurrent-anime load. 2 } @@ -393,6 +519,43 @@ fn default_exclude_min_height() -> u32 { 2000 } +// Starting point for `av1_vaapi`'s `-global_quality` under `QVBR`, needs the +// same real-hardware calibration pass as the bitrate reference did — this is +// a reasonable guess (roughly x264/x265 "visually near-lossless" territory +// on the encoder's internal QP-like scale), not a measured value. +fn default_quality_live_action() -> u32 { + 26 +} + +// SVT-AV1 CRF starting point for the anime pipeline — slightly lower +// (higher quality) than the live-action guess above since flat-color/ +// gradient-heavy anime content shows banding more readily than live-action +// grain/texture does at the same nominal quality level. Also unvalidated +// against real hardware/content yet. +fn default_quality_anime() -> u32 { + 24 +} + +fn default_anime_svtav1_preset() -> u32 { + 10 +} + +fn default_anime_svtav1_max_threads() -> u32 { + 4 +} + +fn default_min_size_reduction_pct() -> f64 { + 0.10 +} + +fn default_skip_below_ceiling_ratio() -> f64 { + 0.5 +} + +fn default_verify_sample_secs() -> f64 { + 20.0 +} + /// TVDB v4 API key, exchanged for a short-lived JWT at request time. #[derive(Debug, Clone, Default, Deserialize)] pub struct TvdbConfig { @@ -416,9 +579,41 @@ impl Config { let raw = fs::read_to_string(&path)?; let cfg: Config = toml::from_str(&raw)?; + cfg.validate()?; Ok(cfg) } + /// Rejects a handful of `transcode` values that are individually + /// syntactically valid TOML but make the transcode pipeline's math + /// nonsensical — there's no other validation anywhere in this config, + /// so a typo here would otherwise only surface much later, deep inside + /// an encode. + fn validate(&self) -> Result<()> { + // `target_bitrate_kbps` divides by `reference_height` (via + // `reference_pixels`); zero makes that ratio `f64::INFINITY`, which + // saturates to `u32::MAX` on the cast back to `u32` — then + // `run_ffmpeg_encode_live_action`'s `bitrate_ceiling_kbps * 3` + // overflows that `u32::MAX` (panics in a debug build, silently + // wraps to a nonsense small value in release). + anyhow::ensure!( + self.transcode.reference_height > 0, + "transcode.reference_height must be greater than 0" + ); + // `is_beneficial` computes `original_bytes * (1.0 - + // min_size_reduction_pct)` as the max allowed output size — a + // negative value here would raise that ceiling *above* the + // original, letting a transcode that actually grew the file still + // count as "beneficial." That's the exact failure mode + // `min_size_reduction_pct` exists to prevent (see its own doc + // comment: a real incident where flat-bitrate VBR silently produced + // files larger than the original). + anyhow::ensure!( + (0.0..=1.0).contains(&self.transcode.min_size_reduction_pct), + "transcode.min_size_reduction_pct must be between 0.0 and 1.0" + ); + Ok(()) + } + pub fn db_path(&self) -> PathBuf { expand_home(&self.daemon.db_path) } @@ -505,4 +700,37 @@ mod tests { assert_eq!(cfg.daemon.log_level, "debug"); assert_eq!(cfg.daemon.listen_addr, "127.0.0.1:7879"); } + + #[test] + fn default_config_passes_validation() { + Config::default().validate().unwrap(); + } + + // Regression test for a real gap found in review: `reference_height = + // 0` makes `target_bitrate_kbps`'s resolution-scaling ratio divide by + // zero, which eventually overflows a `u32` multiplication deep inside + // the live-action encoder's maxrate calculation — a config typo that + // used to only surface as a panic/garbage value in the middle of an + // encode, not at startup. + #[test] + fn rejects_a_zero_reference_height() { + let cfg: Config = toml::from_str("[transcode]\nreference_height = 0\n").unwrap(); + assert!(cfg.validate().is_err()); + } + + // Regression test for a real gap found in review: a negative + // `min_size_reduction_pct` would let `is_beneficial` accept an encode + // that actually *grew* the file — the exact invariant this field exists + // to guarantee against. + #[test] + fn rejects_a_negative_min_size_reduction_pct() { + let cfg: Config = toml::from_str("[transcode]\nmin_size_reduction_pct = -0.1\n").unwrap(); + assert!(cfg.validate().is_err()); + } + + #[test] + fn rejects_a_min_size_reduction_pct_above_one() { + let cfg: Config = toml::from_str("[transcode]\nmin_size_reduction_pct = 1.5\n").unwrap(); + assert!(cfg.validate().is_err()); + } } diff --git a/breadarrd/src/importer/ffprobe.rs b/breadarrd/src/importer/ffprobe.rs index 5f576e5..835c6ad 100644 --- a/breadarrd/src/importer/ffprobe.rs +++ b/breadarrd/src/importer/ffprobe.rs @@ -14,6 +14,11 @@ pub struct AudioStream { #[derive(Debug, Clone, PartialEq)] pub struct SubtitleStream { pub language: Option, + /// mov_text (mp4's timed-text subtitle codec) isn't valid inside a + /// Matroska container — a transcode pipeline that always outputs `.mkv` + /// needs to know this per-stream to convert rather than blindly stream + /// copy. See `transcode::subtitle_codec_args`. + pub codec: Option, } #[derive(Debug, Clone, Default, PartialEq)] @@ -109,6 +114,16 @@ pub fn probe(path: &Path) -> Result { let json: serde_json::Value = serde_json::from_slice(&output.stdout).context("ffprobe output was not valid JSON")?; + Ok(build_media_probe(&json, raw_json)) +} + +/// The actual JSON-to-`MediaProbe` mapping, split out from `probe` so it can +/// be unit-tested directly against hand-built ffprobe-shaped JSON — real +/// muxers are inconsistent enough about stream ordering/disposition (see +/// `probe_finds_the_real_video_stream_even_when_attached_pic_comes_first`'s +/// doc comment) that constructing every case as an actual file via `ffmpeg` +/// isn't always practical. +fn build_media_probe(json: &serde_json::Value, raw_json: String) -> MediaProbe { let format = &json["format"]; let duration_secs = format["duration"] .as_str() @@ -134,11 +149,18 @@ pub fn probe(path: &Path) -> Result { .as_str() .or_else(|| stream["tags"]["LANGUAGE"].as_str()) .map(str::to_string); + let is_attached_pic = stream["disposition"]["attached_pic"].as_i64() == Some(1); match codec_type { - "video" if probe.video_codec.is_none() => { - // First video stream only — a second "video" stream in a - // real-world file is almost always an embedded cover-art - // thumbnail, not a second picture track. + // First non-attached-pic video stream — a second "video" stream + // in a real-world file is almost always an embedded cover-art + // thumbnail, not a second picture track, and ffmpeg does not + // guarantee it comes *after* the real content stream (some mp4 + // remuxes and mkvmerge outputs put it first). Explicitly + // checking `disposition.attached_pic` rather than relying on + // stream order means a cover-art-first file no longer has its + // actual video codec/height silently replaced by the + // thumbnail's. + "video" if probe.video_codec.is_none() && !is_attached_pic => { probe.video_codec = stream["codec_name"].as_str().map(str::to_string); probe.width = stream["width"].as_i64(); probe.height = stream["height"].as_i64(); @@ -164,13 +186,14 @@ pub fn probe(path: &Path) -> Result { }); } "subtitle" => { - probe.subtitles.push(SubtitleStream { language }); + let codec = stream["codec_name"].as_str().map(str::to_string); + probe.subtitles.push(SubtitleStream { language, codec }); } _ => {} } } - Ok(probe) + probe } /// ffprobe reports frame rate as a "num/den" fraction string (e.g. @@ -216,6 +239,59 @@ pub fn verify_decodable(path: &Path) -> Result { } } +/// Bounded, sampled variant of `verify_decodable` for callers where a full +/// decode's O(duration) cost is the actual bottleneck — the transcode +/// pipeline measured this in practice: two concurrent full-file decode +/// verifications pinned two CPU cores at 400%+ each for the whole +/// verification pass, dwarfing the GPU encode time itself for long files. +/// +/// Decodes only fixed-size windows (`sample_secs` each) near the start, +/// middle, and end of the file, rather than every frame — a deliberate +/// trade of "catches most real corruption cheaply" for "bounded cost +/// regardless of file length", not equivalent thoroughness to a full +/// decode. Truncation specifically doesn't need this: `encode_and_verify`'s +/// separate duration-match check against the original already catches that +/// regardless of what this function samples, since a truncated output's +/// container-reported duration comes up short either way. +/// +/// Falls back to a full `verify_decodable` when `duration_secs` is small +/// enough that sampling wouldn't save meaningful time anyway. +pub fn verify_decodable_sampled( + path: &Path, + duration_secs: f64, + sample_secs: f64, +) -> Result { + if duration_secs <= sample_secs * 3.0 { + return verify_decodable(path); + } + + let windows = [ + 0.0, + (duration_secs / 2.0 - sample_secs / 2.0).max(0.0), + (duration_secs - sample_secs).max(0.0), + ]; + + for start in windows { + let mut cmd = Command::new("ffmpeg"); + cmd.args(["-v", "error", "-xerror"]); + if start > 0.0 { + cmd.args(["-ss", &format!("{start:.2}")]); + } + cmd.arg("-i").arg(path); + cmd.args(["-t", &format!("{sample_secs:.2}"), "-f", "null", "-"]); + let output = cmd + .output() + .context("failed to run ffmpeg for sampled decode verification")?; + + if !(output.status.success() && output.stderr.is_empty()) { + return Ok(DecodeCheck::Corrupt( + String::from_utf8_lossy(&output.stderr).into_owned(), + )); + } + } + Ok(DecodeCheck::Ok) +} + #[cfg(test)] mod tests { use super::*; @@ -297,6 +373,78 @@ mod tests { assert!(result.is_err()); } + fn generate_clip(dir: &Path, name: &str, duration_secs: u32) -> std::path::PathBuf { + let path = dir.join(name); + let status = Command::new("ffmpeg") + .args([ + "-y", + "-f", + "lavfi", + "-i", + &format!("testsrc=size=320x240:duration={duration_secs}:rate=5"), + ]) + .args(["-c:v", "libx264", "-preset", "ultrafast"]) + .arg(&path) + .output() + .expect("failed to run ffmpeg to generate a test clip"); + assert!( + status.status.success(), + "ffmpeg failed to generate a test clip: {}", + String::from_utf8_lossy(&status.stderr) + ); + path + } + + #[test] + fn verify_decodable_sampled_passes_a_genuinely_intact_short_file() { + // Short enough to hit the "falls back to a full check" path. + let dir = std::env::temp_dir().join(format!( + "breadarr-ffprobe-sampled-short-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let clip = generate_clip(&dir, "short.mkv", 2); + + let result = verify_decodable_sampled(&clip, 2.0, 20.0).unwrap(); + assert!(matches!(result, DecodeCheck::Ok)); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn verify_decodable_sampled_passes_a_genuinely_intact_long_file() { + // Long enough (duration > sample_secs * 3) to actually exercise the + // windowed start/middle/end sampling path, not the short-file + // fallback. + let dir = std::env::temp_dir().join(format!( + "breadarr-ffprobe-sampled-long-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let clip = generate_clip(&dir, "long.mkv", 10); + + let result = verify_decodable_sampled(&clip, 10.0, 2.0).unwrap(); + assert!(matches!(result, DecodeCheck::Ok)); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn verify_decodable_sampled_flags_a_file_that_does_not_decode_at_all() { + let dir = std::env::temp_dir().join(format!( + "breadarr-ffprobe-sampled-corrupt-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("corrupt.mkv"); + std::fs::write(&path, b"this is not a real video file").unwrap(); + + let result = verify_decodable_sampled(&path, 10.0, 2.0).unwrap(); + assert!(matches!(result, DecodeCheck::Corrupt(_))); + + std::fs::remove_dir_all(&dir).unwrap(); + } + /// Generates a tiny real video via ffmpeg's `lavfi` synthetic source — /// validates the actual JSON field extraction (width/height/codec/ /// duration/audio language+default) against genuine ffprobe output, @@ -376,4 +524,82 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + + // Regression test for a real gap found in review: `probe`'s "first video + // stream wins" selection used to have no idea about + // `disposition.attached_pic` and just trusted stream order — a + // convention real muxers don't reliably follow (ffmpeg's own mp4 muxer + // was observed reordering an attached-pic stream to the *end* + // regardless of requested `-map` order, which makes constructing a + // genuine cover-art-*first* file via `ffmpeg` impractical — so this + // exercises `build_media_probe` directly against hand-built, + // real-shaped ffprobe JSON instead of a generated file, deterministically + // covering the ordering `ffmpeg`'s own tooling won't produce). Unlike + // `generate_clip_with_attached_pic` in `transcode::mod::tests` (cover + // art second, the already-handled case), this puts it *first* to prove + // the fix is order-independent, not just "skip the second video + // stream". + #[test] + fn probe_finds_the_real_video_stream_even_when_attached_pic_comes_first() { + let json = serde_json::json!({ + "format": { "duration": "10.0", "bit_rate": "5000000", "format_long_name": "Matroska / WebM" }, + "streams": [ + { + "index": 0, + "codec_type": "video", + "codec_name": "png", + "width": 64, + "height": 64, + "disposition": { "attached_pic": 1 } + }, + { + "index": 1, + "codec_type": "video", + "codec_name": "h264", + "width": 640, + "height": 360, + "disposition": { "attached_pic": 0 } + } + ] + }); + + let probe = build_media_probe(&json, "{}".to_string()); + assert_eq!(probe.width, Some(640), "must pick the real content stream's width, not the 64x64 cover art's"); + assert_eq!(probe.height, Some(360), "must pick the real content stream's height, not the 64x64 cover art's"); + assert_eq!(probe.video_codec.as_deref(), Some("h264"), "must pick the real content stream's codec, not the cover art's png"); + } + + // Companion case: attached-pic *second* (the ordering the code + // previously assumed was the only one) must still work exactly as + // before — this fix is additive, not a behavior change for the + // already-handled ordering. + #[test] + fn probe_finds_the_real_video_stream_when_attached_pic_comes_second() { + let json = serde_json::json!({ + "format": { "duration": "10.0", "bit_rate": "5000000", "format_long_name": "Matroska / WebM" }, + "streams": [ + { + "index": 0, + "codec_type": "video", + "codec_name": "h264", + "width": 640, + "height": 360, + "disposition": { "attached_pic": 0 } + }, + { + "index": 1, + "codec_type": "video", + "codec_name": "png", + "width": 64, + "height": 64, + "disposition": { "attached_pic": 1 } + } + ] + }); + + let probe = build_media_probe(&json, "{}".to_string()); + assert_eq!(probe.width, Some(640)); + assert_eq!(probe.height, Some(360)); + assert_eq!(probe.video_codec.as_deref(), Some("h264")); + } } diff --git a/breadarrd/src/importer/mod.rs b/breadarrd/src/importer/mod.rs index 8379fe4..24768e4 100644 --- a/breadarrd/src/importer/mod.rs +++ b/breadarrd/src/importer/mod.rs @@ -289,31 +289,63 @@ fn insufficient_space(dir: &Path, needed_bytes: u64) -> Result { Ok(available < needed_bytes) } -/// Hardlinks into the destination (same filesystem, effectively free) and -/// falls back to a plain copy cross-filesystem — deliberately leaves `src` -/// untouched either way. The previous `rename`-then-delete behavior yanked -/// the file out of qBittorrent's payload directory on every import, leaving -/// qBittorrent holding a torrent whose data had vanished (an errored -/// "missing files" state, seeding stopped instantly). A hardlink costs -/// nothing extra on disk and lets qBittorrent keep seeding after import; -/// even the copy fallback preserves seeding, just at the cost of double -/// disk usage for that one file. +/// `true` if `a` and `b` live on the same filesystem (compares `st_dev`), +/// `false` if they don't *or* either can't be stat'd. Used to skip the +/// free-space check ahead of `move_or_copy_file`: that function tries +/// `rename` first, which needs essentially zero additional space, so +/// checking `dest`'s free space against the *full* source file size is a +/// false positive whenever downloads and the library share a volume (a +/// common setup) — a real production shape found in review: a season-pack +/// file that would `rename` instantly got rejected as "not enough free +/// space", retried and failed identically every cycle +/// (`MAX_IMPORT_ERRORS`), then got released back to the search pool where +/// it was re-grabbed and failed the same way again, forever. Erring toward +/// `false` (i.e. still running the space check) on a stat failure is the +/// safe direction — it only means checking a space guarantee that wasn't +/// strictly needed, never skipping one that was. +fn same_filesystem(a: &Path, b: &Path) -> bool { + use std::os::unix::fs::MetadataExt; + match (std::fs::metadata(a), std::fs::metadata(b)) { + (Ok(a), Ok(b)) => a.dev() == b.dev(), + _ => false, + } +} + +/// Moves `src` into `dest`: a same-filesystem `rename` where possible +/// (instant, atomic, no extra disk usage), falling back to copy-then-delete +/// across a filesystem boundary. Nothing is left behind at `src` either way +/// — there used to be a deliberate hardlink-and-leave-`src`-alone scheme +/// here so qBittorrent could keep seeding the original download after +/// import, but with nothing seeding after download completes anymore, that +/// complexity (a same-filesystem staging relocate before every import, so +/// the hardlink was guaranteed rather than falling back to a permanent +/// second copy) bought nothing but risk. The file just lands directly at +/// its final destination. /// -/// The copy path writes to a `.part` sibling of `dest` and only renames it -/// into place once the copy is complete (a same-filesystem rename, so +/// The copy fallback writes to a `.part` sibling of `dest` and only renames +/// it into place once the copy is complete (a same-filesystem rename, so /// atomic) — a crash mid-copy leaves an orphaned `.part` file rather than a /// truncated file at `dest`, so a retried import can't ever double-count a -/// half-written file as already present. -fn link_or_copy_file(src: &Path, dest: &Path) -> Result<()> { - if std::fs::hard_link(src, dest).is_ok() { +/// half-written file as already present. `src` is only removed once that +/// copy is confirmed in place, so a crash between the copy and the removal +/// leaves both copies on disk (recoverable) rather than neither. +fn move_or_copy_file(src: &Path, dest: &Path) -> Result<()> { + if std::fs::rename(src, dest).is_ok() { return Ok(()); } - copy_via_temp_file(src, dest) + copy_via_temp_file(src, dest)?; + std::fs::remove_file(src).with_context(|| { + format!( + "copied {} to {} but failed to remove the original", + src.display(), + dest.display() + ) + }) } /// The copy fallback's actual mechanics, split out so it's directly /// testable without needing a real cross-filesystem boundary to force -/// `hard_link` to fail. +/// `rename` to fail. fn copy_via_temp_file(src: &Path, dest: &Path) -> Result<()> { let tmp = PathBuf::from(format!("{}.part", dest.display())); std::fs::copy(src, &tmp) @@ -678,6 +710,156 @@ fn find_by_stem(root: &Path, stem: &std::ffi::OsStr) -> Option { None } +/// One TV episode whose file already sits on disk — matching breadarr's own +/// `"S{season:02}E{episode:02}"` naming marker, in exactly the season +/// directory this episode's own metadata says it should be in — despite +/// having no `episode_file` row at all. `has_file = 0` lies about it, so +/// the missing-content search loop treats it as genuinely absent and keeps +/// trying to (re)download something already there. +#[derive(Debug, PartialEq)] +pub struct RelinkCandidate { + pub episode_id: i64, + pub media_item_id: i64, + pub series_title: String, + pub season_number: i64, + pub episode_number: i64, + pub path: PathBuf, +} + +/// Every video file found at any depth under `root` — deliberately +/// unbounded by a fixed season-folder shape, since a real production audit +/// found layouts varying wildly per show: a plain `Season N`/`Season 0N` +/// directly under the show root in most cases, but at least one show (a +/// BluRay box-set release) nests an extra layer — the whole release's own +/// folder name — between the show root and its `Season N` directories. +/// Bounded to one show's own folder (a handful of seasons deep at most), +/// same scope as `find_by_basename`/`find_by_stem`. +fn collect_video_files(root: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(root) else { + return Vec::new(); + }; + let mut found = Vec::new(); + for entry in entries.filter_map(|e| e.ok()) { + let path = entry.path(); + if path.is_dir() { + found.extend(collect_video_files(&path)); + } else if path + .extension() + .and_then(|e| e.to_str()) + .is_some_and(|e| VIDEO_EXTS.contains(&e.to_lowercase().as_str())) + { + found.push(path); + } + } + found +} + +/// Finds every `has_file = 0` TV episode whose show folder contains exactly +/// one video file matching its own `"S{season:02}E{episode:02}"` naming +/// marker (breadarr's own convention, which the vast majority of +/// scene/fansub releases also happen to embed verbatim even under otherwise +/// raw filenames) — found via a real production audit: several shows had +/// `episode`/`media_item` rows (almost certainly rebuilt by an earlier +/// DB-recovery incident) with no matching `episode_file` row, even though +/// the actual video files were never touched and still sat right where they +/// always had, often under a pre-breadarr folder layout (unpadded `Season +/// N`, or an extra nested release-folder level) that a check scoped only to +/// `season_dir`'s exact zero-padded shape would never find. Walks the whole +/// show folder once and reuses that listing for every missing episode in +/// it, rather than re-walking per episode. Purely a finder — see +/// `relink_episode_files` for the (additive-only) part that actually links +/// anything in. +/// +/// Deliberately conservative in both directions: a show folder with *zero* +/// marker matches for an episode is left alone (genuinely missing, nothing +/// to relink here — `reconcile_missing_files`/the normal search loop are the +/// right tools for an actually-absent file), and *more than one* match is +/// left alone too rather than guessed at, returned separately so a human +/// can look instead of silently picking one. A show using pure absolute +/// numbering with no season/episode marker at all in its filenames (a real +/// pattern found on some anime releases) simply never matches either way — +/// the safe failure mode, not a wrong guess. +pub fn find_relinkable_episode_files( + conn: &Connection, +) -> Result<(Vec, Vec)> { + let mut show_stmt = conn.prepare( + "SELECT DISTINCT m.id, m.title, m.root_folder + FROM episode e + JOIN media_item m ON m.id = e.media_item_id + WHERE e.has_file = 0 AND m.kind = 'series'", + )?; + let shows: Vec<(i64, String, String)> = show_stmt + .query_map([], |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)))? + .collect::>()?; + + let mut candidates = Vec::new(); + let mut ambiguous = Vec::new(); + + let mut ep_stmt = conn.prepare( + "SELECT id, season_number, episode_number FROM episode + WHERE media_item_id = ?1 AND has_file = 0", + )?; + for (media_item_id, series_title, root_folder) in shows { + let all_video_files = collect_video_files(Path::new(&root_folder)); + let episodes: Vec<(i64, i64, i64)> = ep_stmt + .query_map(params![media_item_id], |row| { + Ok((row.get(0)?, row.get(1)?, row.get(2)?)) + })? + .collect::>()?; + + for (episode_id, season_number, episode_number) in episodes { + let marker = format!("S{season_number:02}E{episode_number:02}"); + let matches: Vec<&PathBuf> = all_video_files + .iter() + .filter(|p| { + p.file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| n.contains(&marker)) + }) + .collect(); + match matches.len() { + 0 => {} + 1 => candidates.push(RelinkCandidate { + episode_id, + media_item_id, + series_title: series_title.clone(), + season_number, + episode_number, + path: matches[0].clone(), + }), + n => ambiguous.push(format!( + "{series_title} S{season_number:02}E{episode_number:02}: {n} candidate files under {root_folder}" + )), + } + } + } + Ok((candidates, ambiguous)) +} + +/// Links each `RelinkCandidate` into `episode_file` (probing it first, same +/// as a real import) and marks its episode `has_file = 1`. Purely additive +/// — never moves, renames, or deletes anything on disk, since the file was +/// already exactly where a real import would have put it; this only makes +/// breadarr's own bookkeeping admit what's already true. +pub fn relink_episode_files(conn: &Connection, candidates: &[RelinkCandidate]) -> Result { + let mut linked = 0; + for c in candidates { + let size_bytes = std::fs::metadata(&c.path)?.len() as i64; + conn.execute( + "INSERT INTO episode_file (episode_id, media_item_id, path, size_bytes, subtitle_status) VALUES (?1, ?2, ?3, ?4, 'none')", + params![c.episode_id, c.media_item_id, c.path.to_string_lossy(), size_bytes], + )?; + let episode_file_id = conn.last_insert_rowid(); + conn.execute( + "UPDATE episode SET has_file = 1 WHERE id = ?1", + params![c.episode_id], + )?; + ensure_probed(conn, episode_file_id, &c.path)?; + linked += 1; + } + Ok(linked) +} + /// Runs ffprobe on `path` and upserts the result into `media_file_probe` /// against `episode_file_id` — but only if the file's size or mtime has /// actually changed since the last probe, so a routine sweep over a large, @@ -1098,12 +1280,22 @@ fn remux_one_backlog_file(conn: &Connection, episode_file_id: i64, path: &Path) } let tmp = path.with_extension("fixed.mkv"); - mkv::remux_english_default(path, &tmp, &tracks)?; + if let Err(e) = mkv::remux_english_default(path, &tmp, &tracks) { + // Leaked otherwise: a stray `.fixed.mkv` sitting in the library + // directory forever, matched by `find_relinkable_episode_files` on + // the same `SxxExx` substring as the real file and reported as an + // ambiguous, unrelinkable episode. + std::fs::remove_file(&tmp).ok(); + return Err(e); + } // Same atomic-swap shape as `copy_via_temp_file`: rename the freshly // remuxed output over the original on the same filesystem, so a crash // mid-swap can never leave a half-written file at the real path. - std::fs::rename(&tmp, path)?; + if let Err(e) = std::fs::rename(&tmp, path) { + std::fs::remove_file(&tmp).ok(); + return Err(e.into()); + } let size_bytes = std::fs::metadata(path)?.len(); conn.execute( @@ -1138,193 +1330,6 @@ fn remap_path(reported: &str, container_prefix: &str, host_prefix: &str) -> Path } } -/// Marker directory name for the seeding-preserving staging area — checked -/// as a plain substring of a torrent's reported `content_path` to tell -/// whether it's already been relocated there in a previous cycle. -const STAGING_DIR_NAME: &str = ".breadarr-staging"; - -/// How many times to poll qBittorrent for a `setLocation` move to finish -/// before giving up for this cycle (it'll simply be retried next cycle — -/// see `relocate_completed_to_staging`). -const RELOCATE_POLL_ATTEMPTS: u32 = 5; -const RELOCATE_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(2); - -/// Finds the nearest ancestor of `path` that actually exists — `path` -/// itself (e.g. a show's season folder) may not have been created yet. -fn nearest_existing_ancestor(path: &Path) -> Result { - let mut current = path; - loop { - if current.exists() { - return Ok(current.to_path_buf()); - } - current = current - .parent() - .with_context(|| format!("no existing ancestor found for {}", path.display()))?; - } -} - -/// Finds the filesystem mount-point directory containing `path`, by -/// walking up parents until the device id changes. Used to place the -/// seeding-staging directory on the same physical filesystem as the final -/// destination — the whole point of staging is that the later hardlink-out -/// in `import_one` is guaranteed to succeed as a true hardlink rather than -/// silently falling back to a full copy, and a hardlink can never cross a -/// filesystem boundary. -fn find_mount_root(path: &Path) -> Result { - use std::os::unix::fs::MetadataExt; - let start = nearest_existing_ancestor(path)?; - let dev = std::fs::metadata(&start)?.dev(); - let mut current = start; - loop { - let Some(parent) = current.parent() else { - return Ok(current); - }; - let Ok(parent_meta) = std::fs::metadata(parent) else { - return Ok(current); - }; - if parent_meta.dev() != dev { - return Ok(current); - } - current = parent.to_path_buf(); - } -} - -/// The staging directory a given torrent's data should be relocated to — -/// on the same filesystem as `dest_dir`, named by torrent hash so multiple -/// torrents' leftover extras (samples/nfo/screenshots) never collide. -fn staging_dir_for(dest_dir: &Path, torrent_hash: &str) -> Result { - let mount_root = find_mount_root(dest_dir)?; - Ok(mount_root.join(STAGING_DIR_NAME).join(torrent_hash)) -} - -/// For every pending grab whose torrent has finished downloading but whose -/// data isn't already staged, relocates it via qBittorrent's own -/// `setLocation` to a directory on the same filesystem as its eventual -/// destination, then waits (briefly, bounded) for the move to actually -/// finish — `setLocation` returns before the physical move completes. -/// -/// Best-effort and self-healing by design: a grab that isn't staged yet -/// this cycle is simply retried on the next one (nothing here ever fails -/// the whole import cycle), so a slow move or a transient qBit API error -/// never blocks or loses anything — it just costs one extra cycle. -/// -/// Returns whether anything was actually relocated, so the caller knows -/// whether it's worth re-fetching the torrent list before importing (a -/// relocated torrent's `content_path` only reflects its new home after a -/// fresh `list_torrents` call). -async fn relocate_completed_to_staging( - qbit: &QbitClient, - pending: &[PendingGrab], - torrents: &[crate::qbit::TorrentInfo], - category: &str, -) -> bool { - let mut relocated_any = false; - for grab in pending { - let Some(torrent) = torrents.iter().find(|t| t.hash == grab.torrent_hash()) else { - continue; - }; - if torrent.progress < 1.0 { - continue; - } - if torrent.content_path.contains(STAGING_DIR_NAME) { - continue; // already staged in a previous cycle - } - - let staging = match staging_dir_for(Path::new(grab.root_folder()), grab.torrent_hash()) { - Ok(s) => s, - Err(e) => { - tracing::warn!( - error = %e, - hash = grab.torrent_hash(), - "could not determine a staging directory this cycle" - ); - continue; - } - }; - - if let Err(e) = qbit - .set_location(grab.torrent_hash(), &staging.to_string_lossy()) - .await - { - tracing::warn!( - error = %e, - hash = grab.torrent_hash(), - "qbit relocate-to-staging failed, will retry next cycle" - ); - continue; - } - - if wait_for_relocation(qbit, grab.torrent_hash(), category, &staging).await { - relocated_any = true; - } else { - tracing::warn!( - hash = grab.torrent_hash(), - "qbit relocate-to-staging didn't finish in time, will retry next cycle" - ); - } - } - relocated_any -} - -/// Waits for qBittorrent to report the torrent's `content_path` as staged, -/// then verifies the reported path actually landed where expected — -/// qBittorrent (when it's running in its own Docker container) reports *its -/// own* filesystem view, and `staging_dir_for`/`set_location` currently -/// rely on every library mount being set up as an identity mount (container -/// path == host path) for that reported path to be directly meaningful -/// from the host's side. That's a deployment convention, not something the -/// code enforces, so it can be wrong for a given install's mount layout. If -/// it is, treating the reported path as trustworthy without checking could -/// let `import_one`'s later hardlink-out silently fall back to a full -/// cross-device copy (or fail outright) instead of the guaranteed-cheap -/// hardlink staging exists to provide — so this confirms the reported path -/// resolves, from the host, to the *same physical filesystem* as -/// `expected_staging` before trusting it. -async fn wait_for_relocation( - qbit: &QbitClient, - hash: &str, - category: &str, - expected_staging: &Path, -) -> bool { - use std::os::unix::fs::MetadataExt; - - for _ in 0..RELOCATE_POLL_ATTEMPTS { - tokio::time::sleep(RELOCATE_POLL_INTERVAL).await; - let Ok(torrents) = qbit.list_torrents(Some(category)).await else { - continue; - }; - let Some(t) = torrents.iter().find(|t| t.hash == hash) else { - continue; - }; - if !t.content_path.contains(STAGING_DIR_NAME) { - continue; - } - let reported = Path::new(&t.content_path); - match ( - std::fs::metadata(reported), - std::fs::metadata(expected_staging), - ) { - (Ok(reported_meta), Ok(expected_meta)) - if reported_meta.dev() == expected_meta.dev() => - { - return true; - } - _ => { - tracing::error!( - hash, - reported = %t.content_path, - expected = %expected_staging.display(), - "qbit reports the torrent as staged, but its content_path isn't visible \ - on the same host filesystem as expected — the container's mount for this \ - library path may not be an identity mount; refusing to trust this relocation" - ); - return false; - } - } - } - false -} - #[allow(clippy::too_many_arguments)] pub async fn run_import_cycle( conn: &Connection, @@ -1342,20 +1347,7 @@ pub async fn run_import_cycle( let torrents = qbit.list_torrents(Some(category)).await?; - // Relocate completed-but-not-yet-staged torrents onto the same - // filesystem as their destination *before* importing, so the - // hardlink-out below (`link_or_copy_file`, unchanged) is a true - // hardlink instead of a full cross-drive copy — qBittorrent keeps - // seeding indefinitely from the staged location afterward, with no - // permanent second copy of the data anywhere. - let relocated_any = relocate_completed_to_staging(qbit, &pending, &torrents, category).await; - let torrents = if relocated_any { - qbit.list_torrents(Some(category)).await? - } else { - torrents - }; - - let stats = process_pending_grabs( + let (stats, imported_hashes) = process_pending_grabs( conn, &pending, &torrents, @@ -1364,9 +1356,31 @@ pub async fn run_import_cycle( transcode_cfg, )?; + // The file(s) are already gone from qBittorrent's download directory by + // this point — moved into the library, or deleted outright because a + // better file already existed — no seeding to preserve either way, so + // 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 { + if let Err(e) = qbit.delete_torrent(hash).await { + tracing::warn!(hash, error = %e, "failed to remove completed torrent from qbittorrent"); + } + } + + // Best-effort, same reasoning as the `delete_torrent` cleanup just + // above: by this point files have already been moved, DB rows written, + // and torrents removed from qBittorrent — a real import that already + // succeeded. Propagating a Jellyfin 500/timeout here used to discard + // `stats` entirely and report the whole cycle as failed, which also + // skipped `stats.failed`/`stats.quality_flagged` notifications for + // imports that had nothing to do with Jellyfin. The library picks up + // the new files on its own next scheduled scan either way. if stats.imported > 0 { if let Some(jellyfin) = jellyfin { - jellyfin.refresh_library().await?; + if let Err(e) = jellyfin.refresh_library().await { + tracing::warn!(error = %e, "failed to trigger jellyfin library refresh"); + } } } @@ -1388,8 +1402,16 @@ fn process_pending_grabs( container_downloads_path: &str, host_downloads_path: &str, transcode_cfg: Option<&breadarr_shared::config::TranscodeConfig>, -) -> Result { +) -> Result<(ImportStats, Vec)> { 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(); for grab in pending { let Some(torrent) = torrents.iter().find(|t| t.hash == grab.torrent_hash()) else { @@ -1413,11 +1435,11 @@ fn process_pending_grabs( continue; } if torrent.state == "moving" { - // qBittorrent's own `setLocation` relocation (or a manual move) - // is still physically in flight — `content_path` may already - // point at the new location while the actual bytes are still - // being copied there. Importing now risks hardlinking/copying - // a partially-moved (truncated) file into the library and + // A category/save-path change (e.g. a manual move in the + // qBittorrent UI) is still physically in flight — `content_path` + // may already point at the new location while the actual bytes + // are still being copied there. Importing now risks moving a + // partially-relocated (truncated) file into the library and // marking it complete. Simply wait; this is checked every // cycle, so it proceeds as soon as the move finishes. stats.skipped_incomplete += 1; @@ -1452,6 +1474,9 @@ fn process_pending_grabs( Ok(outcome) => { stats.imported += outcome.episodes_imported; stats.quality_flagged += outcome.quality_flagged; + if outcome.episodes_imported > 0 { + imported_hashes.push(grab.torrent_hash().to_string()); + } } Err(e) => { let error_count = record_import_error(conn, *release_id)?; @@ -1488,8 +1513,14 @@ fn process_pending_grabs( if quality_flagged { stats.quality_flagged += 1; } + 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()); } - Ok(ImportOutcome::SkippedAlreadyHaveBetter) => {} Err(e) => { let error_count = record_import_error(conn, grab.release_id())?; if error_count >= MAX_IMPORT_ERRORS { @@ -1512,7 +1543,7 @@ fn process_pending_grabs( } } - Ok(stats) + Ok((stats, imported_hashes)) } /// What actually happened when `import_one` was asked to import a @@ -1529,11 +1560,12 @@ enum ImportOutcome { /// Shared by every place a freshly-imported file becomes eligible for the /// async transcode queue (`import_one`, season-pack import) — reads the -/// probe data `ensure_probed` just wrote, delegates the actual eligibility -/// call to `transcode::should_enqueue` (the one place anime/HDR/height/ -/// already-AV1 rules live), and enqueues only if all of that says yes. -/// Never propagates a failure into the caller's import result — same -/// "never fail an otherwise-successful import" treatment as probing. +/// probe data `ensure_probed` just wrote, delegates the codec/HDR/height +/// eligibility call to `transcode::should_enqueue`, and — if eligible — +/// decides the encode pipeline (`transcode::is_anime_content`, path-prefix +/// first then metadata fallback) before enqueueing. Never propagates a +/// failure into the caller's import result — same "never fail an +/// otherwise-successful import" treatment as probing. fn maybe_enqueue_transcode( conn: &Connection, media_item_id: i64, @@ -1553,12 +1585,42 @@ fn maybe_enqueue_transcode( return Ok(()); }; - if crate::transcode::should_enqueue(conn, media_item_id, video_codec.as_deref(), hdr != 0, height, cfg)? { - crate::transcode::enqueue(conn, episode_file_id, video_codec.as_deref(), size_bytes)?; + if !crate::transcode::should_enqueue(video_codec.as_deref(), hdr != 0, height, cfg) { + return Ok(()); } + + let path: String = conn.query_row( + "SELECT path FROM episode_file WHERE id = ?1", + params![episode_file_id], + |row| row.get(0), + )?; + let is_anime = crate::transcode::is_anime_content(conn, media_item_id, Path::new(&path), cfg)?; + crate::transcode::enqueue(conn, episode_file_id, video_codec.as_deref(), size_bytes, is_anime, false)?; Ok(()) } +/// A tracked file renamed aside to make room for an incoming upgrade — +/// `parked_at` may be empty (never actually renamed anything) when the +/// tracked `episode_file` row's file was already missing from disk; `row_id` +/// is `None` when this represents a stray untracked file at `dest` rather +/// than an actual tracked row to drop. +struct StaleFile { + parked_at: PathBuf, + restore_to: PathBuf, + row_id: Option, +} + +impl StaleFile { + /// Best-effort: puts the parked file back where it came from after a + /// later step (free-space check, the actual move) fails, so a failed + /// import never leaves neither the old file nor the new one behind. + fn restore(&self) { + if self.parked_at.as_os_str().len() > 0 { + std::fs::rename(&self.parked_at, &self.restore_to).ok(); + } + } +} + fn import_one( conn: &Connection, grab: &PendingGrab, @@ -1581,19 +1643,22 @@ fn import_one( !mkv::default_track_is_english_or_unset(&tracks) && mkv::has_english_track(&tracks); if needs_fix { let tmp = source_path.with_extension("fixed.mkv"); - mkv::remux_english_default(&source_path, &tmp, &tracks)?; - // `source_path` is qBittorrent's actual seeding payload for - // this torrent — deleting it here, before the free-space check - // and the import below have even run, defeats the whole - // staging design (whose point is to keep seeding intact) and - // risks real data loss if either subsequent step fails: the - // original would already be gone, `working_path` might not - // have made it into the library either, and qBittorrent can't - // necessarily re-fetch a dead swarm. `link_or_copy_file` - // already leaves its source untouched for exactly this reason - // in the non-remux path (see its own doc comment) — the remux - // scratch output below gets the same treatment: only removed - // once it's been successfully imported. + if let Err(e) = mkv::remux_english_default(&source_path, &tmp, &tracks) { + // A failed remux can still leave a partially written `tmp` + // behind; left uncleaned it sits in the library/download + // directory as a stray `.fixed.mkv`, which + // `find_relinkable_episode_files` can then match on the same + // `SxxExx` substring as the real file and report the episode + // as ambiguous. + std::fs::remove_file(&tmp).ok(); + return Err(e); + } + // Deliberately not deleting `source_path` here, before the + // free-space check and the import below have even run: doing so + // risks real data loss if either subsequent step fails, leaving + // neither the original nor a successfully-placed replacement + // anywhere. `source_path` is only ever removed once the remuxed + // derivative has actually landed in the library (see below). working_path = tmp; remuxed = true; } @@ -1641,41 +1706,65 @@ fn import_one( std::fs::create_dir_all(&dest_dir)?; let dest = dest_dir.join(&filename); - // Set below (to the renamed-aside stale file's path) only when this - // import is an upgrade over an existing, worse-scoring file — see the - // `dest.exists()` branch. Used to restore the original on any failure - // between here and the replacement being confirmed on disk, and to - // gate the deferred cleanup (old row + old file) once it succeeds. - let mut old_sibling: Option = None; + // Looked up by *identity* (episode_id for TV, media_item_id with a NULL + // episode_id for movies) rather than by whether a file happens to sit at + // `dest` right now. A real production bug found the hard way: a movie's + // `root_folder` had drifted (recategorized between library folders) + // after it was already imported, so its tracked `episode_file.path` + // no longer matched the *current* `dest` — `dest.exists()` came back + // false, the whole comparison below was skipped entirely, and a fresh + // grab imported right in alongside the untouched original, a real + // duplicate neither this function nor the score comparison ever knew + // to look for. Keying off identity means the comparison still happens + // even when the tracked file's path and the freshly computed `dest` + // disagree. + // Collects *every* matching row, not just one: nothing in the schema + // enforces a single `episode_file` per episode (the `library_health` + // duplicate-groups report exists precisely because duplicates occur + // here), and `query_row` would silently pick an arbitrary one of them, + // leaving any other duplicate's row and file untouched — orphaned + // pointing at a file this import may have just deleted (via the + // `old_sibling`/`dest` overwrite below), or, worse, read as the *only* + // existing file for the `upgrade_locked` check so a transcoded file's + // lock could be missed if it happened to live in the row `query_row` + // didn't return. + let existing_files: Vec<(i64, String, i64)> = conn + .prepare( + "SELECT id, path, upgrade_locked FROM episode_file + WHERE (episode_id = ?1 AND ?1 IS NOT NULL) OR (media_item_id = ?2 AND ?1 IS NULL)", + )? + .query_map(params![episode_id, grab.media_item_id()], |row| { + Ok((row.get(0)?, row.get(1)?, row.get(2)?)) + })? + .collect::>>()?; - // The deterministic filename means a second release for the same - // episode/movie collides on this exact path. `link_or_copy_file`'s copy - // fallback renames into place, which *silently overwrites* an existing - // file with no comparison at all — a lower-scored duplicate arriving - // second (e.g. a 480p release importing after an already-imported - // 1080p one, entirely possible before resolution was scored, and still - // possible from a race between two grabs of the same episode) would - // quietly replace the better file already in the library. Checked here - // rather than left to the filesystem to decide by import order. - if dest.exists() { + // Trigger the comparison whenever *either* signal says something might + // already be here: a tracked row (regardless of where its file actually + // is), or a physical file already sitting at `dest` (a stray, + // not-yet-reconciled leftover with no tracked row at all — the case the + // original `dest.exists()`-only check covered). Comparing on either + // condition alone would miss the other's failure mode; comparing on + // both is a strict superset of what the original single check did. + let mut old_siblings: Vec = Vec::new(); + if !existing_files.is_empty() || dest.exists() { // Belt-and-suspenders: the missing-content/upgrade search paths // already refuse to re-grab an `upgrade_locked` file (it isn't // "missing" and the upgrade loop skips it), so this shouldn't be // reachable for one today — but a locally AV1-transcoded file // should never be silently overwritten on score alone regardless // of which path produced the incoming grab, same guard as - // `import_season_pack_file`'s matching check. - let upgrade_locked: i64 = conn - .query_row( - "SELECT upgrade_locked FROM episode_file WHERE path = ?1", - params![dest.to_string_lossy()], - |row| row.get(0), - ) - .unwrap_or(0); - if upgrade_locked != 0 { + // `import_season_pack_file`'s matching check. Locked if *any* + // duplicate row is locked, not just whichever one a plain + // `query_row` would have happened to return. + let upgrade_locked = existing_files.iter().any(|(_, _, ul)| *ul != 0); + if upgrade_locked { if remuxed { std::fs::remove_file(&working_path).ok(); } + // Nothing seeds this download anymore and it isn't going + // anywhere — the episode already has a file in place, so + // there's no reason to leave a duplicate orphaned on disk. + std::fs::remove_file(&source_path).ok(); return Ok(ImportOutcome::SkippedAlreadyHaveBetter); } let current_score: f32 = conn @@ -1715,29 +1804,73 @@ fn import_one( if remuxed { std::fs::remove_file(&working_path).ok(); } + // Same reasoning as the upgrade_locked case above — this + // download lost the comparison and nothing seeds it, so it's + // just wasted disk space if left in place. + std::fs::remove_file(&source_path).ok(); return Ok(ImportOutcome::SkippedAlreadyHaveBetter); } // The new file scores strictly better than what's currently there. - // Move the stale file sideways to a `.old` sibling — rather than - // deleting it and its tracking row outright — so the replacement is - // placed and confirmed *before* the original is actually given up. - // A `rename` (not a delete) still frees up `dest` for the primary - // hardlink path (`std::fs::hard_link` fails outright if the - // destination already exists), so an upgrade is still a cheap - // hardlink swap in the common case; it just also means that if the - // free-space check or `link_or_copy_file` below fails (I/O error, - // dest dir vanished, disk full), the original file gets moved back - // into place instead of being gone for good. The DB row is left - // alone until the replacement is confirmed on disk, for the same - // reason. - old_sibling = Some(PathBuf::from(format!("{}.old", dest.display()))); - std::fs::rename(&dest, old_sibling.as_ref().unwrap())?; + // Park aside whatever's actually here — the tracked row's real file + // (not necessarily `dest` — see above) and/or a stray file sitting + // at `dest` with no tracked row — rather than deleting anything + // outright, so the replacement is placed and confirmed *before* + // the original is actually given up. If the free-space check or + // `move_or_copy_file` below then fails (I/O error, dest dir + // vanished, disk full), everything parked gets moved back into + // place instead of being gone for good. The DB row is left alone + // until the replacement is confirmed on disk, for the same reason. + // Park *every* matching row, not just one — a second (duplicate) + // row left untouched here would otherwise keep pointing at a file + // that may be gone once the replacement lands (the incoming file + // gets moved to `dest`, and any duplicate's file sitting elsewhere + // is simply orphaned in the DB with no cleanup). + for (existing_id, existing_path, _) in &existing_files { + let existing_path = PathBuf::from(existing_path); + if existing_path.exists() { + let parked_at = PathBuf::from(format!("{}.old", existing_path.display())); + std::fs::rename(&existing_path, &parked_at)?; + old_siblings.push(StaleFile { + parked_at, + restore_to: existing_path, + row_id: Some(*existing_id), + }); + } else { + // The tracked row survived but its file didn't (e.g. + // deleted out from under breadarr) — nothing to park, but + // the stale row still needs dropping once the new file is + // confirmed in place. + old_siblings.push(StaleFile { + parked_at: PathBuf::new(), + restore_to: PathBuf::new(), + row_id: Some(*existing_id), + }); + } + } + if dest.exists() && old_siblings.iter().all(|s| s.restore_to != dest) { + let parked_at = PathBuf::from(format!("{}.old", dest.display())); + std::fs::rename(&dest, &parked_at)?; + // Doesn't duplicate an existing park (from the tracked-row loop + // above) — only added when none of those already cover `dest`. + old_siblings.push(StaleFile { + parked_at, + restore_to: dest.clone(), + row_id: None, + }); + } } let needed_bytes = std::fs::metadata(&working_path)?.len(); - if insufficient_space(&dest_dir, needed_bytes)? { - if let Some(old_sibling) = &old_sibling { - std::fs::rename(old_sibling, &dest).ok(); + if !same_filesystem(&working_path, &dest_dir) && insufficient_space(&dest_dir, needed_bytes)? { + for stale in &old_siblings { + stale.restore(); + } + // `working_path` is the remux scratch copy (`source_path` is the + // real, still-intact download) — disposable, and left uncleaned it + // leaks into whichever directory it was written in (see the same + // cleanup added above for a failed remux). + if remuxed { + std::fs::remove_file(&working_path).ok(); } anyhow::bail!( "not enough free space at {} for {needed_bytes} bytes (source: {})", @@ -1746,31 +1879,39 @@ fn import_one( ); } - if let Err(err) = link_or_copy_file(&working_path, &dest) { + if let Err(err) = move_or_copy_file(&working_path, &dest) { // Restore the original file rather than leaving the user with // neither the old file nor the new one — this is the exact failure // mode a `DELETE`-then-place ordering used to leave unrecoverable. - if let Some(old_sibling) = &old_sibling { - std::fs::rename(old_sibling, &dest).ok(); + for stale in &old_siblings { + stale.restore(); + } + if remuxed { + std::fs::remove_file(&working_path).ok(); } return Err(err); } if remuxed { - // `working_path` here is the scratch remux output, not qBittorrent's - // original content file (which was already removed above, in the - // remux branch, to make way for it) — nothing else reads it, so - // unlike the plain-import case there's no seeding reason to keep it. - std::fs::remove_file(&working_path).ok(); + // `working_path` (the remux scratch output) was already consumed by + // the move above either way — nothing left to clean up there. What's + // left is `source_path`: qBittorrent's original pre-remux download, + // a genuinely separate file from `working_path`. Nothing seeds it + // anymore and its remuxed derivative is now safely in the library, + // so it's just wasted disk space if left behind. + std::fs::remove_file(&source_path).ok(); } // The replacement is confirmed in place on disk — only now is it safe - // to drop the old tracking row and the renamed-aside original. - if let Some(old_sibling) = old_sibling { - conn.execute( - "DELETE FROM episode_file WHERE path = ?1", - params![dest.to_string_lossy()], - )?; - std::fs::remove_file(&old_sibling).ok(); + // to drop the old tracking row(s) (by id, not by path — a tracked row's + // path may never have matched `dest` in the first place) and the + // renamed-aside original(s). + for stale in old_siblings { + if let Some(row_id) = stale.row_id { + conn.execute("DELETE FROM episode_file WHERE id = ?1", params![row_id])?; + } + if stale.parked_at.as_os_str().len() > 0 { + std::fs::remove_file(&stale.parked_at).ok(); + } } let size_bytes = std::fs::metadata(&dest)?.len(); @@ -2020,7 +2161,7 @@ enum PackFileOutcome { } /// One file's worth of the season-pack import: the same dest-collision -/// scoring, free-space check, hardlink-or-copy, `episode_file` bookkeeping, +/// scoring, free-space check, move-or-copy, `episode_file` bookkeeping, /// and post-import probe that `import_one` does for a single-episode grab, /// scoped to one already-identified `episode_id` within a larger pack. #[allow(clippy::too_many_arguments)] @@ -2058,27 +2199,46 @@ fn import_season_pack_file( std::fs::create_dir_all(&dest_dir)?; let dest = dest_dir.join(&filename); - // Same reasoning as import_one's own dest-collision check: a second - // release for the same episode (here, from a *different* pack or a - // single-episode grab) must not silently overwrite a better file - // already in place. - let mut old_sibling: Option = None; - if dest.exists() { + // Looked up by identity (this episode's id), same reasoning as + // import_one's matching fix: a season pack's incoming file must be + // compared against whatever this episode is *actually* tracked as + // having, not against whatever happens to physically sit at `dest` + // right now — those can disagree if `root_folder` ever drifted after + // the tracked file was imported. + // Every matching row, not just one — same duplicate-row fix as + // `import_one` (see its comment): nothing enforces a single + // `episode_file` per episode, and `query_row` would silently pick an + // arbitrary one, leaving any duplicate's row/file untouched and + // potentially missing its `upgrade_locked` flag. + let mut old_siblings: Vec = Vec::new(); + let existing_files: Vec<(i64, String, i64)> = conn + .prepare("SELECT id, path, upgrade_locked FROM episode_file WHERE episode_id = ?1")? + .query_map(params![episode_id], |row| { + Ok((row.get(0)?, row.get(1)?, row.get(2)?)) + })? + .collect::>>()?; + + // Trigger on either signal, same generalization as import_one's matching + // fix: a tracked row (wherever its file really is) or a stray physical + // file at `dest` with no tracked row at all. + if !existing_files.is_empty() || dest.exists() { // A locally AV1-transcoded file is a deliberate shrink, not // something a season pack (scored against its own, much larger, // original release) should be allowed to silently overwrite just // because its release score happens to be higher — that score was // never computed against a transcoded file's actual size/quality // tradeoff. Checked before the score comparison, same reasoning as - // `movie_eligible_for_upgrade`/`enumerate_upgrade_targets`. - let upgrade_locked: i64 = conn - .query_row( - "SELECT upgrade_locked FROM episode_file WHERE path = ?1", - params![dest.to_string_lossy()], - |row| row.get(0), - ) - .unwrap_or(0); - if upgrade_locked != 0 { + // `movie_eligible_for_upgrade`/`enumerate_upgrade_targets`. Locked + // if *any* duplicate row is locked. + let upgrade_locked = existing_files.iter().any(|(_, _, ul)| *ul != 0); + if upgrade_locked { + // Nothing seeds this download anymore and it isn't going + // anywhere — this one file within the pack loses to what's + // already in place, so there's no reason to leave it orphaned + // on disk (other files in the same pack are handled by their + // own separate calls to this function, so only this one file + // is removed here, not the whole pack directory). + std::fs::remove_file(source_path).ok(); return Ok(PackFileOutcome::SkippedAlreadyHaveBetter); } let existing_best: Option = conn.query_row( @@ -2087,23 +2247,49 @@ fn import_season_pack_file( |row| row.get(0), )?; if existing_best.is_some_and(|best| best >= release_score) { + std::fs::remove_file(source_path).ok(); return Ok(PackFileOutcome::SkippedAlreadyHaveBetter); } - // This episode's file is being upgraded. Move the stale file aside - // to a `.old` sibling rather than deleting it (and its row) outright - // — `dest` is still free for the hardlink fast path, but the - // original survives on disk until the replacement is confirmed in - // place, so a failed free-space check or `link_or_copy_file` below - // can't lose the file. See import_one's matching comment for the - // fuller reasoning; this is the same fix applied there. - old_sibling = Some(PathBuf::from(format!("{}.old", dest.display()))); - std::fs::rename(&dest, old_sibling.as_ref().unwrap())?; + // This episode's file is being upgraded. Park aside whatever's + // actually here — the tracked row's real file and/or a stray file + // at `dest` with no tracked row — rather than deleting anything + // outright, so the replacement is placed and confirmed *before* + // the original is actually given up. See import_one's matching + // comment for the fuller reasoning; this is the same fix applied + // there. + for (existing_id, existing_path, _) in &existing_files { + let existing_path = PathBuf::from(existing_path); + if existing_path.exists() { + let parked_at = PathBuf::from(format!("{}.old", existing_path.display())); + std::fs::rename(&existing_path, &parked_at)?; + old_siblings.push(StaleFile { + parked_at, + restore_to: existing_path, + row_id: Some(*existing_id), + }); + } else { + old_siblings.push(StaleFile { + parked_at: PathBuf::new(), + restore_to: PathBuf::new(), + row_id: Some(*existing_id), + }); + } + } + if dest.exists() && old_siblings.iter().all(|s| s.restore_to != dest) { + let parked_at = PathBuf::from(format!("{}.old", dest.display())); + std::fs::rename(&dest, &parked_at)?; + old_siblings.push(StaleFile { + parked_at, + restore_to: dest.clone(), + row_id: None, + }); + } } let needed_bytes = std::fs::metadata(source_path)?.len(); - if insufficient_space(&dest_dir, needed_bytes)? { - if let Some(old_sibling) = &old_sibling { - std::fs::rename(old_sibling, &dest).ok(); + if !same_filesystem(source_path, &dest_dir) && insufficient_space(&dest_dir, needed_bytes)? { + for stale in &old_siblings { + stale.restore(); } anyhow::bail!( "not enough free space at {} for {needed_bytes} bytes (source: {})", @@ -2112,19 +2298,20 @@ fn import_season_pack_file( ); } - if let Err(err) = link_or_copy_file(source_path, &dest) { - if let Some(old_sibling) = &old_sibling { - std::fs::rename(old_sibling, &dest).ok(); + if let Err(err) = move_or_copy_file(source_path, &dest) { + for stale in &old_siblings { + stale.restore(); } return Err(err); } - if let Some(old_sibling) = old_sibling { - conn.execute( - "DELETE FROM episode_file WHERE path = ?1", - params![dest.to_string_lossy()], - )?; - std::fs::remove_file(&old_sibling).ok(); + for stale in old_siblings { + if let Some(row_id) = stale.row_id { + conn.execute("DELETE FROM episode_file WHERE id = ?1", params![row_id])?; + } + if stale.parked_at.as_os_str().len() > 0 { + std::fs::remove_file(&stale.parked_at).ok(); + } } let size_bytes = std::fs::metadata(&dest)?.len(); @@ -2723,6 +2910,124 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + // Regression coverage for a real production finding: several shows had + // `episode` rows marked `has_file = 0` whose real file was sitting + // exactly where breadarr's own importer would have put it, with no + // `episode_file` row at all — almost certainly a residue of an earlier + // DB-recovery incident. This models that shape directly: an episode row + // with no matching episode_file, and a real file already at the + // expected season directory bearing breadarr's own SxxEyy marker. + #[test] + fn find_relinkable_episode_files_finds_a_file_with_no_tracked_row() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + let dir = std::env::temp_dir().join(format!("breadarr-relink-{}", std::process::id())); + media_item_with_root(&conn, 1, &dir); + conn.execute( + "INSERT INTO episode (id, media_item_id, season_number, episode_number, title, has_file) + VALUES (1, 1, 2, 1, 'Seven Years Later', 0)", + [], + ) + .unwrap(); + + let season_dir = dir.join("Season 02"); + std::fs::create_dir_all(&season_dir).unwrap(); + let real_file = season_dir.join("Some Show - S02E01 - Seven Years Later.mkv"); + std::fs::write(&real_file, b"already on disk, never linked").unwrap(); + + let (candidates, ambiguous) = find_relinkable_episode_files(&conn).unwrap(); + assert!(ambiguous.is_empty()); + assert_eq!(candidates.len(), 1); + assert_eq!(candidates[0].episode_id, 1); + assert_eq!(candidates[0].path, real_file); + + let linked = relink_episode_files(&conn, &candidates).unwrap(); + assert_eq!(linked, 1); + + let has_file: i64 = conn + .query_row("SELECT has_file FROM episode WHERE id = 1", [], |r| r.get(0)) + .unwrap(); + assert_eq!(has_file, 1); + let tracked_path: String = conn + .query_row( + "SELECT path FROM episode_file WHERE episode_id = 1", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(tracked_path, real_file.to_string_lossy()); + // Purely additive — the file itself was never touched. + assert_eq!(std::fs::read(&real_file).unwrap(), b"already on disk, never linked"); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + // Regression coverage for the wider pattern found across ~20 shows on + // real production data: a pre-breadarr library organized entirely under + // the unpadded "Season N" convention, with no zero-padded folder at all + // for that season. `season_dir` alone would never find anything here. + #[test] + fn find_relinkable_episode_files_falls_back_to_the_unpadded_season_folder() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + let dir = std::env::temp_dir().join(format!("breadarr-relink-unpadded-{}", std::process::id())); + media_item_with_root(&conn, 1, &dir); + conn.execute( + "INSERT INTO episode (id, media_item_id, season_number, episode_number, title, has_file) + VALUES (1, 1, 1, 1, 'Pilot', 0)", + [], + ) + .unwrap(); + + // No "Season 01" anywhere — only the unpadded convention. + let season_dir = dir.join("Season 1"); + std::fs::create_dir_all(&season_dir).unwrap(); + let real_file = season_dir.join("Some.Show.S01E01.Pilot.1080p.mkv"); + std::fs::write(&real_file, b"pre-breadarr library content").unwrap(); + + let (candidates, ambiguous) = find_relinkable_episode_files(&conn).unwrap(); + assert!(ambiguous.is_empty()); + assert_eq!(candidates.len(), 1); + assert_eq!(candidates[0].path, real_file); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn find_relinkable_episode_files_skips_an_ambiguous_match_rather_than_guessing() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + let dir = + std::env::temp_dir().join(format!("breadarr-relink-ambiguous-{}", std::process::id())); + media_item_with_root(&conn, 1, &dir); + conn.execute( + "INSERT INTO episode (id, media_item_id, season_number, episode_number, title, has_file) + VALUES (1, 1, 1, 1, 'Pilot', 0)", + [], + ) + .unwrap(); + + let season_dir = dir.join("Season 01"); + std::fs::create_dir_all(&season_dir).unwrap(); + std::fs::write(season_dir.join("Some Show - S01E01 - Pilot.mkv"), b"one").unwrap(); + std::fs::write( + season_dir.join("[Group] Some Show - S01E01 (dual audio).mkv"), + b"two", + ) + .unwrap(); + + let (candidates, ambiguous) = find_relinkable_episode_files(&conn).unwrap(); + assert!(candidates.is_empty(), "an ambiguous match must not be guessed at"); + assert_eq!(ambiguous.len(), 1); + + let tracked_count: i64 = conn + .query_row("SELECT count(*) FROM episode_file", [], |r| r.get(0)) + .unwrap(); + assert_eq!(tracked_count, 0); + + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn a_fresh_grab_is_not_stalled() { let (conn, release_id) = seeded_release_conn(1.0); @@ -2843,45 +3148,6 @@ mod tests { ); } - #[test] - fn nearest_existing_ancestor_returns_the_path_itself_when_it_exists() { - let dir = std::env::temp_dir(); - assert_eq!(nearest_existing_ancestor(&dir).unwrap(), dir); - } - - #[test] - fn nearest_existing_ancestor_walks_up_past_nonexistent_components() { - let dir = std::env::temp_dir().join(format!( - "breadarr-ancestor-test-{}/does/not/exist/yet", - std::process::id() - )); - let expected = - std::env::temp_dir().join(format!("breadarr-ancestor-test-{}", std::process::id())); - std::fs::create_dir_all(&expected).unwrap(); - assert_eq!(nearest_existing_ancestor(&dir).unwrap(), expected); - std::fs::remove_dir_all(&expected).unwrap(); - } - - #[test] - fn staging_dir_for_is_named_by_torrent_hash_under_the_marker_directory() { - let dest = std::env::temp_dir(); - let staging = staging_dir_for(&dest, "deadbeef1234").unwrap(); - assert_eq!( - staging.file_name().unwrap().to_str().unwrap(), - "deadbeef1234" - ); - assert_eq!( - staging - .parent() - .unwrap() - .file_name() - .unwrap() - .to_str() - .unwrap(), - STAGING_DIR_NAME - ); - } - #[test] fn insufficient_space_is_false_for_a_trivially_small_request() { assert!(!insufficient_space(&std::env::temp_dir(), 1).unwrap()); @@ -2893,6 +3159,42 @@ mod tests { assert!(insufficient_space(&std::env::temp_dir(), u64::MAX / 2).unwrap()); } + // Regression test for a real gap found in review: `insufficient_space` + // checks `dest`'s free space against the *full* source file size, but + // `move_or_copy_file` tries a same-filesystem `rename` first, which + // needs essentially none. Two paths under the same temp dir are + // guaranteed to share a device, so this exercises the exact case that + // used to produce false "not enough free space" rejections (and, via + // the grab-fail-search retry loop, a permanent stuck cycle) whenever + // downloads and the library share a volume. + #[test] + fn same_filesystem_is_true_for_two_paths_under_the_same_temp_dir() { + let dir = std::env::temp_dir().join(format!("breadarr-same-fs-test-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let a = dir.join("a.mkv"); + let b = dir.join("subdir"); + std::fs::create_dir_all(&b).unwrap(); + std::fs::write(&a, b"x").unwrap(); + + assert!(same_filesystem(&a, &b), "two paths under the same temp dir must share a device"); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn same_filesystem_is_false_when_either_path_cannot_be_stat_d() { + let dir = std::env::temp_dir().join(format!("breadarr-same-fs-missing-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let missing = dir.join("does-not-exist.mkv"); + + assert!( + !same_filesystem(&dir, &missing), + "a stat failure must default to 'different filesystems' so the free-space check still runs" + ); + + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn copy_via_temp_file_writes_through_a_part_file_and_renames_into_place() { let dir = std::env::temp_dir().join(format!("breadarr-copy-test-{}", std::process::id())); @@ -2915,18 +3217,18 @@ mod tests { } #[test] - fn link_or_copy_file_leaves_the_source_in_place() { - let dir = std::env::temp_dir().join(format!("breadarr-link-test-{}", std::process::id())); + fn move_or_copy_file_moves_the_source_out() { + let dir = std::env::temp_dir().join(format!("breadarr-move-test-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let src = dir.join("source.mkv"); std::fs::write(&src, b"fake video data").unwrap(); let dest = dir.join("dest.mkv"); - link_or_copy_file(&src, &dest).unwrap(); + move_or_copy_file(&src, &dest).unwrap(); - assert!(src.exists(), "source should survive a hardlink-or-copy"); + assert!(!src.exists(), "source should be gone after a move — nothing seeds it anymore"); assert!(dest.exists()); - assert_eq!(std::fs::read(&src).unwrap(), std::fs::read(&dest).unwrap()); + assert_eq!(std::fs::read(&dest).unwrap(), b"fake video data"); std::fs::remove_dir_all(&dir).unwrap(); } @@ -3069,7 +3371,7 @@ mod tests { // absent — simulating torrents qBit no longer knows about. ]; - let stats = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); + let (stats, _) = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); assert_eq!(stats.imported, 1); assert_eq!(stats.skipped_incomplete, 1); @@ -3131,7 +3433,7 @@ mod tests { content_path: "/tmp/somewhere-mid-move".to_string(), }]; - let stats = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); + let (stats, _) = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); assert_eq!(stats.imported, 0); assert_eq!(stats.skipped_incomplete, 1); assert_eq!(stats.errors, 0); @@ -3188,7 +3490,7 @@ mod tests { for i in 1..MAX_IMPORT_ERRORS { let pending = fetch_pending_grabs(&conn).unwrap(); - let stats = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); + let (stats, _) = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); assert_eq!(stats.errors, 1, "iteration {i}"); assert_eq!(stats.failed, 0, "iteration {i}"); let status: String = conn @@ -3199,7 +3501,7 @@ mod tests { // The Nth failure crosses the threshold and gives up. let pending = fetch_pending_grabs(&conn).unwrap(); - let stats = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); + let (stats, _) = process_pending_grabs(&conn, &pending, &torrents, "", "", None).unwrap(); assert_eq!(stats.failed, 1); assert_eq!(stats.errors, 0); let status: String = conn @@ -3262,8 +3564,8 @@ mod tests { let dest = dest_root.join("Some Movie (2016).mp4"); assert!(dest.exists(), "expected {} to exist", dest.display()); assert!( - content.exists(), - "source file should survive import so qBittorrent can keep seeding" + !content.exists(), + "source file should be moved into place, not left behind" ); let status: String = conn @@ -3322,8 +3624,15 @@ mod tests { std::fs::create_dir_all(&dir).unwrap(); let dest_root = dir.join("library"); std::fs::create_dir_all(&dest_root).unwrap(); - let content = dir.join("Some.Movie.2016.1080p.mp4"); - std::fs::write(&content, b"fake movie data").unwrap(); + // A real, probeable h264/1080p clip, not placeholder bytes: since + // `should_enqueue` now requires a successful probe (a `probe_failed` + // NULL-codec/NULL-height row must never enqueue an unclaimable job — + // see `should_enqueue`'s doc comment), a fake file would make + // `ensure_probed` record `probe_failed` and this test would no + // longer exercise the real "should this file be transcoded" path. + let content = dir.join("Some.Movie.2016.1080p.mkv"); + let clip = generate_test_clip(&dir, 1920, 1080); + std::fs::rename(&clip, &content).unwrap(); let grab = PendingGrab::Movie { release_id: 1, @@ -3356,7 +3665,7 @@ mod tests { } #[test] - fn import_one_does_not_enqueue_a_transcode_job_for_anime() { + fn import_one_enqueues_an_anime_tagged_transcode_job_for_anime() { let conn = Connection::open_in_memory().unwrap(); crate::db::init(&conn).unwrap(); conn.execute( @@ -3395,8 +3704,13 @@ mod tests { std::fs::create_dir_all(&dir).unwrap(); let dest_root = dir.join("library"); std::fs::create_dir_all(&dest_root).unwrap(); - let content = dir.join("Some.Anime.S01E01.mp4"); - std::fs::write(&content, b"fake anime data").unwrap(); + // Real, probeable content — see the sibling + // `import_one_enqueues_a_transcode_job_when_transcode_is_enabled` + // test for why a fake file no longer exercises this path now that + // `should_enqueue` requires an actual successful probe. + let content = dir.join("Some.Anime.S01E01.mkv"); + let clip = generate_test_clip(&dir, 1920, 1080); + std::fs::rename(&clip, &content).unwrap(); let grab = PendingGrab::Episode { release_id: 1, @@ -3412,10 +3726,17 @@ mod tests { import_one(&conn, &grab, &content, Some(&breadarr_shared::config::TranscodeConfig::default())).unwrap(); - let job_count: i64 = conn - .query_row("SELECT count(*) FROM transcode_job", [], |r| r.get(0)) + // Anime is no longer excluded from transcoding — it's routed to its + // own pipeline (`is_anime = 1` on the job row), not skipped. + let (job_count, is_anime): (i64, i64) = conn + .query_row( + "SELECT count(*), max(is_anime) FROM transcode_job", + [], + |r| Ok((r.get(0)?, r.get(1)?)), + ) .unwrap(); - assert_eq!(job_count, 0); + assert_eq!(job_count, 1); + assert_eq!(is_anime, 1, "job must be tagged is_anime via anime_mapping"); std::fs::remove_dir_all(&dir).unwrap(); } @@ -3757,6 +4078,9 @@ mod tests { std::fs::read(&dest).unwrap(), b"the better file, already imported" ); + // The losing download is cleaned up rather than left as a dangling + // duplicate — nothing seeds it and it lost the comparison. + assert!(!content.exists(), "the losing download should be deleted, not left behind"); let status: String = conn .query_row("SELECT status FROM release WHERE id = 2", [], |r| r.get(0)) @@ -3767,9 +4091,7 @@ mod tests { } #[test] - fn a_strictly_better_release_replaces_the_existing_file_via_a_real_hardlink_swap() { - use std::os::unix::fs::MetadataExt; - + fn a_strictly_better_release_replaces_the_existing_file_via_a_real_move() { let conn = Connection::open_in_memory().unwrap(); crate::db::init(&conn).unwrap(); conn.execute( @@ -3832,13 +4154,9 @@ mod tests { // The new file's content landed at the shared deterministic path. assert_eq!(std::fs::read(&dest).unwrap(), b"the new, better file"); - // It's a genuine hardlink to the source (same inode), not a copy — - // confirms the old file/row was cleared first so `hard_link` - // itself succeeded instead of falling back to `copy_via_temp_file`. - assert_eq!( - std::fs::metadata(&dest).unwrap().ino(), - std::fs::metadata(&content).unwrap().ino() - ); + // The source is gone — it was moved, not copied or hardlinked + // alongside a surviving original. + assert!(!content.exists(), "source should be moved, not left behind"); // Exactly one episode_file row survives for this movie — the old // one was removed, not left behind as a duplicate alongside the new. @@ -3854,6 +4172,200 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + // Regression test for a real gap found in review: nothing in the schema + // enforces one `episode_file` per episode (the `library_health` + // duplicate-groups report exists precisely because duplicates occur), + // but the old lookup used `query_row`, which silently returns only one + // arbitrary matching row. A second duplicate row was left completely + // untouched — its `upgrade_locked` flag never even consulted, and its + // file never parked-and-cleaned-up alongside the winning replacement. + // This seeds two rows for the same movie and asserts the upgrade sweeps + // both: both old files gone from disk, both old rows gone from the DB, + // exactly one row/file survives. + #[test] + fn an_upgrade_swap_sweeps_every_duplicate_episode_file_row_not_just_one() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + conn.execute( + "INSERT INTO media_item (id, kind, title, year, monitored, quality_profile_id, root_folder) + VALUES (1, 'movie', 'Some Movie', 2016, 1, 1, '/tmp')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO source (id, name, kind, base_url) VALUES (1, 'tpb', 'scrape', 'http://x')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO release (id, media_item_id, episode_id, raw_title, source_id, guid, score, status, torrent_hash, grabbed_at) + VALUES (2, 1, NULL, 'Some Movie 2016 1080p', 1, 'guid-2', 20.0, 'grabbed', 'bbbb', datetime('now'))", + [], + ) + .unwrap(); + + let dir = std::env::temp_dir().join(format!( + "breadarr-duplicate-episode-file-sweep-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let dest_root = dir.join("library"); + std::fs::create_dir_all(&dest_root).unwrap(); + + // Two duplicate rows for the same movie, each with its own real + // file on disk, neither at the deterministic `dest` path (so this + // also exercises the identity-lookup path, not the `dest.exists()` + // fallback). + let stray_a = dir.join("stray-a.mp4"); + std::fs::write(&stray_a, b"duplicate row A's file").unwrap(); + conn.execute( + "INSERT INTO episode_file (id, episode_id, media_item_id, path, size_bytes, subtitle_status) + VALUES (10, NULL, 1, ?1, 20, 'none')", + params![stray_a.to_string_lossy()], + ) + .unwrap(); + let stray_b = dir.join("stray-b.mp4"); + std::fs::write(&stray_b, b"duplicate row B's file").unwrap(); + conn.execute( + "INSERT INTO episode_file (id, episode_id, media_item_id, path, size_bytes, subtitle_status) + VALUES (11, NULL, 1, ?1, 20, 'none')", + params![stray_b.to_string_lossy()], + ) + .unwrap(); + + let content = dir.join("Some.Movie.2016.1080p.mp4"); + std::fs::write(&content, b"the new, better file").unwrap(); + + let grab = PendingGrab::Movie { + release_id: 2, + media_item_id: 1, + torrent_hash: "bbbb".to_string(), + title: "Some Movie".to_string(), + year: Some(2016), + root_folder: dest_root.to_string_lossy().to_string(), + }; + + let outcome = import_one(&conn, &grab, &content, None).unwrap(); + assert!(matches!(outcome, ImportOutcome::Imported { .. })); + + assert!(!stray_a.exists(), "duplicate row A's file must be cleaned up, not orphaned"); + assert!(!stray_b.exists(), "duplicate row B's file must be cleaned up, not orphaned"); + + let remaining: Vec = conn + .prepare("SELECT id FROM episode_file WHERE media_item_id = 1") + .unwrap() + .query_map([], |r| r.get(0)) + .unwrap() + .collect::>>() + .unwrap(); + assert_eq!( + remaining.len(), + 1, + "both duplicate rows must be swept, leaving exactly the new one, got {remaining:?}" + ); + assert!( + !remaining.contains(&10) && !remaining.contains(&11), + "the surviving row must be the newly inserted one, not a stale duplicate" + ); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + // Regression test for a real production bug: a movie's `root_folder` + // drifted (recategorized between library folders — the exact shape of + // a real incident found on "Cars 3", tracked at `.../Kids Movies/...` + // while `root_folder` had since moved to `.../Movies/...`) after it was + // already imported. The old dest-collision check only fired on + // `dest.exists()`, which came back false once the destination path no + // longer matched where the tracked file actually lived — so the score + // comparison was skipped entirely and a fresh grab landed right in + // alongside the untouched original, a real duplicate. This asserts the + // comparison still happens (and the two copies get consolidated into + // one) even when the tracked path and the freshly computed `dest` + // disagree. + #[test] + fn a_drifted_root_folder_does_not_defeat_the_dest_collision_check() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + conn.execute( + "INSERT INTO media_item (id, kind, title, year, monitored, quality_profile_id, root_folder) + VALUES (1, 'movie', 'Cars 3', 2017, 1, 1, '/tmp')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO source (id, name, kind, base_url) VALUES (1, 'tpb', 'scrape', 'http://x')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO release (id, media_item_id, episode_id, raw_title, source_id, guid, score, status, torrent_hash, grabbed_at) + VALUES (1, 1, NULL, 'Cars 3 2017 720p', 1, 'guid-1', 5.0, 'imported', 'aaaa', datetime('now'))", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO release (id, media_item_id, episode_id, raw_title, source_id, guid, score, status, torrent_hash, grabbed_at) + VALUES (2, 1, NULL, 'Cars 3 2017 1080p', 1, 'guid-2', 20.0, 'grabbed', 'bbbb', datetime('now'))", + [], + ) + .unwrap(); + + let dir = std::env::temp_dir().join(format!("breadarr-drift-{}", std::process::id())); + // The tracked file's real home: an old category folder, no longer + // matching the movie's current `root_folder`. + let old_root = dir.join("Kids Movies").join("Cars 3 (2017)"); + std::fs::create_dir_all(&old_root).unwrap(); + let old_path = old_root.join("Cars 3 (2017).mp4"); + std::fs::write(&old_path, b"the old, smaller tracked file").unwrap(); + conn.execute( + "INSERT INTO episode_file (id, episode_id, media_item_id, path, size_bytes, subtitle_status) + VALUES (1, NULL, 1, ?1, 20, 'none')", + params![old_path.to_string_lossy()], + ) + .unwrap(); + + // The movie's root_folder has since drifted to a different folder — + // `dest` will never equal `old_path`. + let new_root = dir.join("Movies").join("Cars 3 (2017)"); + std::fs::create_dir_all(&new_root).unwrap(); + + let content = dir.join("Cars.3.2017.1080p.mp4"); + std::fs::write(&content, b"the new, better file").unwrap(); + + let grab = PendingGrab::Movie { + release_id: 2, + media_item_id: 1, + torrent_hash: "bbbb".to_string(), + title: "Cars 3".to_string(), + year: Some(2017), + root_folder: new_root.to_string_lossy().to_string(), + }; + + let outcome = import_one(&conn, &grab, &content, None).unwrap(); + assert!(matches!(outcome, ImportOutcome::Imported { .. })); + + let dest = new_root.join("Cars 3 (2017).mp4"); + assert_eq!(std::fs::read(&dest).unwrap(), b"the new, better file"); + // The old tracked file, at its own (different) path, is gone — + // consolidated, not left behind as an untracked duplicate. + assert!( + !old_path.exists(), + "the old tracked file should be cleaned up even though its path never matched dest" + ); + + let file_count: i64 = conn + .query_row( + "SELECT count(*) FROM episode_file WHERE media_item_id = 1", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(file_count, 1, "exactly one tracked file should survive, not two"); + + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn locates_the_largest_video_file_in_a_directory() { let dir = std::env::temp_dir().join(format!("breadarr-test-dir-{}", std::process::id())); @@ -4071,6 +4583,10 @@ mod tests { b"locally-transcoded e01", "the locked, locally-transcoded file must survive untouched regardless of score" ); + assert!( + !pack_dir.join("Show.S01E01.mkv").exists(), + "the skipped pack file should be deleted, not left behind in the pack directory" + ); std::fs::remove_dir_all(&dir).unwrap(); } diff --git a/breadarrd/src/transcode/mod.rs b/breadarrd/src/transcode/mod.rs index a7f0e28..a31c2b1 100644 --- a/breadarrd/src/transcode/mod.rs +++ b/breadarrd/src/transcode/mod.rs @@ -24,52 +24,119 @@ pub fn target_bitrate_kbps(width: i64, height: i64, cfg: &TranscodeConfig) -> u3 bitrate.round() as u32 } -/// Runs the actual GPU encode via `av1_vaapi`, decoding through VAAPI too -/// (`-hwaccel_output_format vaapi`) so the whole pipeline stays on-GPU +/// Runs the live-action GPU encode via `av1_vaapi`, decoding through VAAPI +/// too (`-hwaccel_output_format vaapi`) so the whole pipeline stays on-GPU /// rather than round-tripping frames through the CPU. Video-only re-encode /// — every audio/subtitle/data stream is copied verbatim (`-c:a copy -c:s -/// copy -c:d copy`), and 10-bit sources stay 10-bit (AV1 handles this -/// natively; the VAAPI driver preserves the surface format through the -/// pipeline without any extra flags needed). +/// copy -c:d copy`). +/// +/// Rate control is `QVBR` (quality-defined VBR): `-global_quality` is the +/// actual quality target driving output size, `-b:v`/`-maxrate`/`-bufsize` +/// (computed from `target_bitrate_kbps`) are a *ceiling*, not a target — +/// content that's already efficient spends less than the ceiling rather +/// than being inflated up toward it. This replaced a flat `VBR + -b:v` +/// scheme after a real incident: that scheme treated `-b:v` as the target +/// average rather than a cap, so already-efficient sources (some well under +/// the model's own reference bitrate) got re-encoded *larger* than the +/// original on the majority of a real backfill. `encode_and_verify`'s +/// post-encode size check is the actual hard guarantee against that +/// happening again regardless of how this rate-control tuning holds up — +/// this function only has to get it roughly right, not perfectly. /// /// Blocking and slow by design (a real GPU encode, potentially minutes per /// file) — callers must run this inside `tokio::task::spawn_blocking`, never /// directly on an async task, and never while holding the shared DB mutex. /// -/// Known limitation, deliberately not worked around yet: the output -/// container is always Matroska, and `-c:s copy`/`-c:d copy` stream-copy -/// whatever subtitle/data tracks the source has. mp4 sources using -/// `mov_text` subtitles (or certain data streams) aren't valid inside -/// Matroska and will make the whole `ffmpeg` invocation fail — safely -/// (the original file is never touched; the job just lands in `failed` -/// with no space saved for that file), but not silently worked around by -/// re-encoding or dropping the offending stream. Fixing this properly needs -/// probing the source's subtitle/data codecs first and choosing per-stream -/// handling, which wasn't done here rather than risk an unverified fix. -fn run_ffmpeg_encode(input: &Path, output: &Path, bitrate_kbps: u32, vaapi_device: &str) -> Result<()> { - let maxrate = bitrate_kbps * 3 / 2; - let bufsize = bitrate_kbps * 2; +/// The output container is always Matroska, so this maps streams by type +/// rather than the blanket `-map 0` the first version used — two real +/// gaps that caused, both fixed: +/// - `-map 0` pulled in attached-picture streams (embedded cover art, +/// exposed by ffmpeg as a second `video`-type stream, `mjpeg`/`png`) and +/// `-c:v av1_vaapi` then tried to encode *that* too, which the VAAPI +/// filter chain can't handle — failed the whole job on any file with +/// embedded cover art (several real library files hit this). `-map +/// 0:v:0` selects only the first video stream — the actual content, +/// never the attached-pic that (per `ffprobe::probe`'s own convention) +/// always comes after it — so cover art is silently dropped rather than +/// crashing the encode. +/// - mp4 sources using `mov_text` subtitles aren't valid inside Matroska +/// via stream copy and failed the whole job. `subtitle_codec_args` +/// converts just the `mov_text` streams to `srt` (a lossless format +/// change for plain timed text) and copies everything else untouched +/// (so an already-Matroska-compatible `ass`/`srt` track keeps its +/// styling rather than being flattened). +/// +/// Rate control mode is `ICQ`, not `QVBR` — a live validation run found +/// Hestia's iHD driver rejects `QVBR` outright ("Driver does not support +/// QVBR RC mode (supported modes: CQP, CBR, VBR, ICQ)"), despite `ffmpeg -h +/// encoder=av1_vaapi` listing `QVBR` as a libavcodec-level option; the +/// *driver* is the actual authority on what's usable, and libavcodec +/// doesn't validate that ahead of time. `ICQ` is the quality-driven mode +/// this driver actually has, so it's the correct target regardless — the +/// `-b:v`/`-maxrate`/`-bufsize` ceiling stays as a best-effort cap (harmless +/// if this driver ignores it under ICQ; `encode_and_verify`'s post-encode +/// `is_beneficial` check is the actual hard guarantee either way, not this). +/// +/// `-loglevel error` suppresses ffmpeg's default per-frame progress output +/// (`frame=... fps=... bitrate=... speed=...`, one line per progress tick) +/// — for a long encode this otherwise accumulates to megabytes in the +/// `Command::output()`-captured stderr buffer, which is pure noise in a +/// failure's stored `error` text and unnecessary memory held by the parent +/// process for the whole encode's duration. +/// +/// Decode is `-hwaccel vaapi` but deliberately *without* +/// `-hwaccel_output_format vaapi` — a live validation run found that +/// combination fails even on the simplest possible single-stream input +/// ("Impossible to convert between the formats supported by the filter +/// 'Parsed_null_0' and the filter 'auto_scale_0'" / "Function not +/// implemented"), because it makes ffmpeg keep the decoded frame +/// GPU-resident and then implicitly spin up a *second*, separate VAAPI +/// device context to bridge into the encoder — the two contexts don't +/// negotiate a compatible format with each other on this driver, wrong +/// devices or not. Explicitly downloading to a normal system-memory frame +/// (`-hwaccel_output_format` omitted) and re-uploading through the *same* +/// device context via `-vf format=nv12,hwupload` avoids that implicit +/// second context entirely, and is what actually produces a valid, +/// decodable AV1 output in practice — confirmed against real hardware +/// before trusting it further. +fn run_ffmpeg_encode_live_action( + input: &Path, + output: &Path, + bitrate_ceiling_kbps: u32, + quality: u32, + vaapi_device: &str, + subtitles: &[ffprobe::SubtitleStream], +) -> Result<()> { + let maxrate = bitrate_ceiling_kbps * 3 / 2; + let bufsize = bitrate_ceiling_kbps * 2; let result = Command::new("ffmpeg") .arg("-y") + .args(["-loglevel", "error"]) .args(["-hwaccel", "vaapi"]) .args(["-hwaccel_device", vaapi_device]) - .args(["-hwaccel_output_format", "vaapi"]) .arg("-i") .arg(input) - .args(["-map", "0"]) + .args(["-map", "0:v:0"]) + .args(["-map", "0:a?"]) + .args(["-map", "0:s?"]) + .args(["-map", "0:d?"]) + .args(["-vf", "format=nv12,hwupload"]) .args(["-c:v", "av1_vaapi"]) - // Explicit rather than relying on the driver's "auto" inference - // from the bitrate flags present — VBR here spends less on simple - // scenes and more on complex ones within the maxrate/bufsize - // envelope below, rather than a flat per-frame target. - .args(["-rc_mode", "VBR"]) - .args(["-b:v", &format!("{bitrate_kbps}k")]) + .args(["-rc_mode", "ICQ"]) + .args(["-global_quality", &quality.to_string()]) + .args(["-b:v", &format!("{bitrate_ceiling_kbps}k")]) .args(["-maxrate", &format!("{maxrate}k")]) .args(["-bufsize", &format!("{bufsize}k")]) .args(["-c:a", "copy"]) - .args(["-c:s", "copy"]) + .args(subtitle_codec_args(subtitles)) .args(["-c:d", "copy"]) + // Explicit rather than relying on `output`'s extension to imply the + // muxer: `temp_path_for` deliberately gives the scratch file a + // non-video extension so library scans skip it (see its own doc + // comment), which would otherwise leave ffmpeg unable to guess a + // container from the output path at all. + .args(["-f", "matroska"]) .arg(output) .output() .context("failed to run ffmpeg av1_vaapi encode")?; @@ -84,6 +151,105 @@ fn run_ffmpeg_encode(input: &Path, output: &Path, bitrate_kbps: u32, vaapi_devic Ok(()) } +/// Anime pipeline: software `libsvtav1` at 10-bit (`yuv420p10le`), CRF-driven +/// (no bitrate ceiling — this is pure quality-mode, unlike the live-action +/// path). Two reasons this isn't just `run_ffmpeg_encode_live_action` with a +/// different quality number: +/// +/// 1. No hardware AV1 10-bit encode exists on Hestia's Arc A380 — `vainfo` +/// only lists `VAProfileAV1Profile0` (8-bit). Anime art (flat color +/// fields, gradient shading/skies) shows 8-bit banding far more readily +/// than live-action's grain/texture does at an equivalent bitrate, so +/// getting true 10-bit output is worth trading GPU offload for CPU time. +/// 2. `av1_vaapi` exposes no tune/animation-specific options at all (its +/// `AVOption` list is rate-control/GOP-structure knobs only) — a +/// meaningfully different anime pipeline wasn't achievable on the +/// hardware encoder regardless of quality value chosen. +/// +/// Fully software: decode is left default (not `-hwaccel vaapi`) since a +/// software-encode target gains nothing from a hardware-decoded/GPU-resident +/// frame (it would need `hwdownload` back to system memory anyway), and +/// `libsvtav1` is overwhelmingly the throughput bottleneck either way. +/// +/// Same blocking/slow-by-design and stream-mapping/subtitle-codec handling +/// as `run_ffmpeg_encode_live_action` (attached-pic cover art dropped via +/// `-map 0:v:0`, `mov_text` subtitles converted to `srt`) apply here too. +/// +/// `max_threads` is passed through as `-svtav1-params lp=N` (SVT-AV1's own +/// "logical processors" cap) — a real validation run on Hestia's 6c/12t box +/// let `libsvtav1` use every thread at once with no explicit bound, and a +/// single 1080p episode grew to **9.3GB resident memory** before the kernel +/// OOM-killer took it out (no other services lost, but a second real OOM +/// incident is a second time too many). SVT-AV1's memory use scales with +/// both preset and thread count — more parallel workers means more +/// concurrently-buffered lookahead/reference frames — so capping threads +/// bounds peak memory to something predictable regardless of how many +/// cores the host actually has, independent of whatever preset is chosen. +/// +/// `-loglevel error` for the same reason as the live-action encoder — this +/// path runs for tens of minutes per file, and unsuppressed progress output +/// would otherwise dominate the captured stderr buffer for that whole time. +fn run_ffmpeg_encode_anime( + input: &Path, + output: &Path, + crf: u32, + preset: u32, + max_threads: u32, + subtitles: &[ffprobe::SubtitleStream], +) -> Result<()> { + let result = Command::new("ffmpeg") + .arg("-y") + .args(["-loglevel", "error"]) + .arg("-i") + .arg(input) + .args(["-map", "0:v:0"]) + .args(["-map", "0:a?"]) + .args(["-map", "0:s?"]) + .args(["-map", "0:d?"]) + .args(["-c:v", "libsvtav1"]) + .args(["-pix_fmt", "yuv420p10le"]) + .args(["-crf", &crf.to_string()]) + .args(["-preset", &preset.to_string()]) + .args(["-svtav1-params", &format!("lp={max_threads}")]) + .args(["-c:a", "copy"]) + .args(subtitle_codec_args(subtitles)) + .args(["-c:d", "copy"]) + // See the matching comment in `run_ffmpeg_encode_live_action`. + .args(["-f", "matroska"]) + .arg(output) + .output() + .context("failed to run ffmpeg libsvtav1 encode")?; + + if !result.status.success() { + let _ = std::fs::remove_file(output); + bail!( + "ffmpeg libsvtav1 encode failed: {}", + String::from_utf8_lossy(&result.stderr) + ); + } + Ok(()) +} + +/// Per-subtitle-stream `-c:s:{i}` codec args, in the same order `-map +/// 0:s?` presents them in the output. `mov_text` (mp4's timed-text codec) +/// isn't valid inside the Matroska container this pipeline always writes, +/// so it's converted to `srt` — a lossless format change for plain timed +/// text. Everything else (`ass`, `srt`, etc. — already Matroska-compatible) +/// is copied untouched rather than flattened through a lossy re-encode. +/// Pulled out as its own pure function so this exact per-stream decision — +/// the thing that made every mp4-sourced file with subtitles fail outright +/// — has direct unit coverage without needing a real ffmpeg encode. +fn subtitle_codec_args(subtitles: &[ffprobe::SubtitleStream]) -> Vec { + subtitles + .iter() + .enumerate() + .flat_map(|(i, s)| { + let codec = if s.codec.as_deref() == Some("mov_text") { "srt" } else { "copy" }; + [format!("-c:s:{i}"), codec.to_string()] + }) + .collect() +} + /// The temp path a given job's encode writes to — job-id-scoped (not just /// derived from the input filename) so two jobs can never collide on the /// same temp file even if something ends up processing the same @@ -91,32 +257,86 @@ fn run_ffmpeg_encode(input: &Path, output: &Path, bitrate_kbps: u32, vaapi_devic /// `transcode-library` backfill process). Also what `reset_orphaned_ /// running_jobs` uses to find and clean up a crashed job's leftover partial. /// -/// Known cosmetic limitation: `finalize_job` renames this over the -/// original path verbatim, keeping whatever extension the source had — an -/// `.mp4` source ends up as Matroska bytes at an `.mp4` path (Jellyfin -/// content-sniffs, so playback isn't affected, but `episode_file.path`'s -/// extension no longer matches the real container). Not fixed here; would -/// need the rename plus an `episode_file.path` update done together in -/// `finalize_job`'s transaction. +/// Written into the *library* directory (same filesystem as the original — +/// required for the atomic rename-based swap in `finalize_job`, never a +/// separate staging drive) for however long the encode takes (minutes to +/// hours). Deliberately given a non-video final extension (`.tmp`, not +/// `.mkv`) and a leading dot: a real production shape found in review — a +/// growing `....mkv` sitting mid-encode in the library was itself picked up +/// by `collect_video_files`/`walk_files` (both filter on `VIDEO_EXTS`), so +/// `find_relinkable_episode_files` saw two candidate files for the same +/// `SxxExx` and reported the episode "ambiguous," `library_scan` indexed +/// the partial file, and Jellyfin could index/attempt to play a +/// still-encoding file mid-scan. `.tmp` sidesteps every one of those +/// `VIDEO_EXTS`-based scans without needing to special-case any of them +/// individually; `-f matroska` is passed explicitly to both encode +/// functions so ffmpeg doesn't need `output`'s extension to infer the +/// container now that it's no longer `.mkv`-shaped. +/// +/// Known cosmetic limitation, unrelated to the above: `finalize_job` +/// renames this over the *original* path verbatim, keeping whatever +/// extension the source had — an `.mp4` source ends up as Matroska bytes at +/// an `.mp4` path (Jellyfin content-sniffs, so playback isn't affected, but +/// `episode_file.path`'s extension no longer matches the real container). +/// Not fixed here; would need the rename plus an `episode_file.path` update +/// done together in `finalize_job`'s transaction. fn temp_path_for(input: &Path, job_id: i64) -> PathBuf { - input.with_extension(format!("job{job_id}.av1.mkv")) + let file_name = input + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or("transcode"); + input.with_file_name(format!(".{file_name}.job{job_id}.tmp")) +} + +/// What `encode_and_verify` decided to do. Distinct from a hard `Err` (an +/// actual failure — ffmpeg crashed, output was corrupt, duration mismatched) +/// — both `AlreadyAv1` and `NotBeneficial` are successful, deliberate +/// no-ops: nothing went wrong, there's just no transcode worth keeping. +#[derive(Debug)] +enum EncodeOutcome { + /// The input was already AV1 — no encode was even attempted. + AlreadyAv1, + /// An encode ran (or was skipped pre-emptively) but the result wasn't + /// meaningfully smaller than the original, so it was discarded rather + /// than swapped in. + NotBeneficial, + /// A verified, meaningfully-smaller output ready to be swapped in. + Encoded(PathBuf), } /// The blocking half of a transcode: encode to a temp file alongside the /// original (same filesystem, required for the atomic rename-based swap /// later — never a separate staging drive), then verify the result is /// actually good *before* anything touches the original. Leaves the temp -/// file on disk on success (the caller finalizes the swap under the DB -/// lock); cleans it up itself on any failure, so the original is never at -/// risk regardless of what goes wrong here. +/// file on disk only for the `Encoded` outcome (the caller finalizes the +/// swap under the DB lock); cleans it up itself in every other case, so the +/// original is never at risk regardless of what happens here. /// -/// Returns `Ok(None)` (no encode run at all) if the input turns out to +/// Returns `AlreadyAv1` (no encode run at all) if the input turns out to /// already be AV1 — checked fresh against the real file here, not trusted /// from whatever `media_file_probe` said when the job was claimed. This is /// what makes a job that's re-run after a crash between the file-swap /// rename and the DB bookkeeping (a narrow but real window — see /// `finalize_job`) self-correct into a no-op instead of re-encoding an -/// already-AV1 file a second time. +/// already-AV1 file a second time. `force_reencode` (set only by +/// `find_oversized_av1_candidates`'s remediation jobs) skips this check +/// entirely — for a file that's already AV1 but was mis-encoded (e.g. left +/// larger than its original by the rate-control bug this whole module's +/// history is built around), "already AV1" is exactly the case that *does* +/// need a real re-encode, not a no-op. The `is_beneficial` check further +/// down still applies unconditionally either way — forcing a re-encode +/// attempt never bypasses the "never keep a non-improvement" guarantee. +/// +/// Returns `NotBeneficial` in two cases: (1) a pre-check, before spending +/// any GPU/CPU time, when the source's current bitrate is already at or +/// below `skip_below_ceiling_ratio` of the resolution-scaled ceiling — a +/// strong signal there's little room to save; (2) after encoding, if the +/// result isn't at least `min_size_reduction_pct` smaller than the +/// original. (2) is the actual hard guarantee against a real incident where +/// flat-bitrate rate control silently produced outputs *larger* than the +/// original on most of a real backfill — regardless of how well-tuned the +/// rate control is, this is what makes "never keep a non-improvement" true +/// unconditionally. fn encode_and_verify( input: PathBuf, job_id: i64, @@ -124,22 +344,72 @@ fn encode_and_verify( width: i64, height: i64, original_duration: Option, -) -> Result> { - match ffprobe::probe(&input) { - Ok(p) if p.video_codec.as_deref() == Some("av1") => return Ok(None), - // Any other outcome (a different codec, or the probe itself - // failing) falls through to a real encode attempt as normal — - // this check exists only to short-circuit the one case where - // there's provably nothing to do. - _ => {} + is_anime: bool, + force_reencode: bool, +) -> Result { + // Kept (not just checked and discarded) so its `subtitles` can be + // reused for `subtitle_codec_args` below without a second ffprobe call. + let fresh_probe = ffprobe::probe(&input); + if !force_reencode { + if let Ok(p) = &fresh_probe { + if p.video_codec.as_deref() == Some("av1") { + return Ok(EncodeOutcome::AlreadyAv1); + } + } + } + // Any other outcome (a different codec, or the probe itself failing) + // falls through to a real encode attempt as normal — the check above + // exists only to short-circuit the one case where there's provably + // nothing to do. A failed probe means no subtitle info either, so + // `subtitle_codec_args` just gets an empty slice (falls back to + // whatever ffmpeg's own default subtitle handling is for the output + // container) rather than guessing. + let subtitles: &[ffprobe::SubtitleStream] = + fresh_probe.as_ref().map(|p| p.subtitles.as_slice()).unwrap_or(&[]); + + let original_bytes = std::fs::metadata(&input) + .context("failed to stat input file before transcode")? + .len(); + let ceiling_bitrate_kbps = target_bitrate_kbps(width, height, &cfg); + + if let Some(duration) = original_duration { + if duration > 0.0 { + let source_bitrate_kbps = (original_bytes as f64 * 8.0) / duration / 1000.0; + if source_bitrate_kbps <= ceiling_bitrate_kbps as f64 * cfg.skip_below_ceiling_ratio { + return Ok(EncodeOutcome::NotBeneficial); + } + } } let tmp_path = temp_path_for(&input, job_id); - let bitrate = target_bitrate_kbps(width, height, &cfg); - run_ffmpeg_encode(&input, &tmp_path, bitrate, &cfg.vaapi_device)?; + if is_anime { + run_ffmpeg_encode_anime( + &input, + &tmp_path, + cfg.quality_anime, + cfg.anime_svtav1_preset, + cfg.anime_svtav1_max_threads, + subtitles, + )?; + } else { + run_ffmpeg_encode_live_action( + &input, + &tmp_path, + ceiling_bitrate_kbps, + cfg.quality_live_action, + &cfg.vaapi_device, + subtitles, + )?; + } - match ffprobe::verify_decodable(&tmp_path) { + let decode_check = match original_duration { + Some(duration) => ffprobe::verify_decodable_sampled(&tmp_path, duration, cfg.verify_sample_secs), + // Duration unknown — can't pick sample windows sensibly, so fall + // back to the thorough full-file check rather than guess. + None => ffprobe::verify_decodable(&tmp_path), + }; + match decode_check { Ok(ffprobe::DecodeCheck::Ok) => {} Ok(ffprobe::DecodeCheck::Corrupt(detail)) => { let _ = std::fs::remove_file(&tmp_path); @@ -170,14 +440,36 @@ fn encode_and_verify( } } - Ok(Some(tmp_path)) + let new_bytes = std::fs::metadata(&tmp_path) + .context("failed to stat transcoded output")? + .len(); + if !is_beneficial(original_bytes, new_bytes, cfg.min_size_reduction_pct) { + let _ = std::fs::remove_file(&tmp_path); + return Ok(EncodeOutcome::NotBeneficial); + } + + Ok(EncodeOutcome::Encoded(tmp_path)) } -/// Whether `media_item_id` is anime — same two membership checks -/// (`anime_mapping` for TV, `anime_tmdb_movie` for movies) already used -/// elsewhere for scoring/upgrade exclusions. Anime needs its own encode -/// tuning (thin lines, flat color, grain) not designed yet, so it's -/// excluded from both the backfill and the post-grab path for now. +/// The hard invariant that makes "never keep a non-improvement" true +/// regardless of how well any rate-control tuning holds up: an output only +/// counts as worth keeping if it's at least `min_size_reduction_pct` +/// smaller than the original. Pulled out as its own pure function so this +/// exact arithmetic — the thing a real incident got wrong — has direct unit +/// coverage without needing a real ffmpeg encode to exercise it. +fn is_beneficial(original_bytes: u64, new_bytes: u64, min_size_reduction_pct: f64) -> bool { + let max_allowed_bytes = (original_bytes as f64 * (1.0 - min_size_reduction_pct)) as u64; + new_bytes <= max_allowed_bytes +} + +/// Whether `media_item_id` is anime per metadata — same two membership +/// checks (`anime_mapping` for TV, `anime_tmdb_movie` for movies) already +/// used elsewhere for scoring/upgrade exclusions. Known to have real +/// coverage gaps (Avatar: The Last Airbender and some Dragon Ball movies +/// were missing from these tables) — `is_anime_content` below is the +/// combined check that should actually be used for routing; this is kept +/// as its own function since other callers (`scheduler.rs`'s search +/// routing) still want the metadata-only check. pub fn is_anime(conn: &Connection, media_item_id: i64) -> Result { let result: bool = conn.query_row( "SELECT EXISTS( @@ -196,6 +488,33 @@ pub fn is_anime(conn: &Connection, media_item_id: i64) -> Result { Ok(result) } +/// Whether `path` falls under one of `cfg.anime_root_folders` — a plain +/// prefix match. Deliberately independent of any DB metadata: it's how the +/// library is actually organized on disk, and it's what catches the real +/// gaps in `anime_mapping`/`anime_tmdb_movie` coverage (see `is_anime`'s +/// doc comment). +pub fn is_anime_path(path: &Path, cfg: &TranscodeConfig) -> bool { + cfg.anime_root_folders.iter().any(|root| path.starts_with(root)) +} + +/// The combined anime check every enqueue site should use to decide which +/// encode pipeline (`run_ffmpeg_encode_anime` vs `_live_action`) a file +/// gets routed to: path-based first (cheap, no DB access, and the more +/// reliable signal — see `is_anime_path`), falling back to the +/// metadata-based `is_anime` check for anime content filed outside the +/// configured anime root folders. +pub fn is_anime_content( + conn: &Connection, + media_item_id: i64, + path: &Path, + cfg: &TranscodeConfig, +) -> Result { + if is_anime_path(path, cfg) { + return Ok(true); + } + is_anime(conn, media_item_id) +} + /// Resets any `running` job back to `pending` — call once at process /// startup (the daemon itself, and the `transcode-library` backfill), never /// mid-run. `running` only ever means "some still-alive process is @@ -240,16 +559,24 @@ pub fn reset_orphaned_running_jobs(conn: &Connection) -> Result { /// Queues one file for transcoding. `original_codec`/`original_bytes` are /// just recorded for the eventual report — not used for any decision. +/// `is_anime` is decided once here (by the caller, via `is_anime_content`) +/// and carried on the job row so `claim_pending_jobs` can dispatch to the +/// right encode pipeline without re-deriving it at claim time. Every normal +/// enqueue site passes `force_reencode: false`; `true` is only for +/// `find_oversized_av1_candidates`'s remediation jobs — see +/// `encode_and_verify`'s doc comment for why that flag exists. pub fn enqueue( conn: &Connection, episode_file_id: i64, original_codec: Option<&str>, original_bytes: i64, + is_anime: bool, + force_reencode: bool, ) -> Result<()> { conn.execute( - "INSERT INTO transcode_job (episode_file_id, status, original_codec, original_bytes, queued_at) - VALUES (?1, 'pending', ?2, ?3, datetime('now'))", - params![episode_file_id, original_codec, original_bytes], + "INSERT INTO transcode_job (episode_file_id, status, original_codec, original_bytes, is_anime, force_reencode, queued_at) + VALUES (?1, 'pending', ?2, ?3, ?4, ?5, datetime('now'))", + params![episode_file_id, original_codec, original_bytes, is_anime as i64, force_reencode as i64], )?; Ok(()) } @@ -257,44 +584,63 @@ pub fn enqueue( /// The single source of truth for "should this freshly-imported file be /// queued for transcoding at all" — every enqueue site (`import_one`, /// season-pack import) must go through this rather than re-deriving its own -/// subset of the rules, which is exactly how the post-import hook -/// originally ended up checking anime but silently skipping the HDR/height -/// exclusions `find_backlog_candidates` applies to the backfill. Called -/// after `ensure_probed`, once real ffprobe data (codec/hdr/height) exists -/// for the file — the codec/hdr/height parameters come from that probe, not -/// from anything parsed off the release title. +/// subset of the rules. Called after `ensure_probed`, once real ffprobe +/// data (codec/hdr/height) exists for the file — the codec/hdr/height +/// parameters come from that probe, not from anything parsed off the +/// release title. +/// +/// No longer excludes anime — anime now has its own encode pipeline +/// (`run_ffmpeg_encode_anime`, see `is_anime_content`) instead of being +/// skipped outright. A pure function now that anime is out of it (no DB +/// access needed to decide codec/HDR/height eligibility). pub fn should_enqueue( - conn: &Connection, - media_item_id: i64, video_codec: Option<&str>, hdr: bool, height: Option, cfg: &TranscodeConfig, -) -> Result { +) -> bool { + // A `probe_failed` row has NULL codec/height (see `ensure_probed`), not a + // missing row — without this check `should_enqueue` would happily enqueue + // a job for it. `claim_pending_jobs` requires `p.width`/`p.height IS NOT + // NULL` so that job can never be claimed, and the unique partial index on + // (pending/running) then permanently blocks any future, real enqueue for + // this file once probing succeeds. + if video_codec.is_none() || height.is_none() { + return false; + } if video_codec == Some("av1") { - return Ok(false); + return false; } if cfg.exclude_hdr && hdr { - return Ok(false); + return false; } if height.is_some_and(|h| h >= cfg.exclude_min_height as i64) { - return Ok(false); + return false; } - if is_anime(conn, media_item_id)? { - return Ok(false); - } - Ok(true) + true } /// Every existing-library file eligible for the `transcode-library` backfill: -/// not already AV1, not anime, not HDR/2160p+ (per `cfg.exclude_hdr` / +/// not already AV1, not HDR/2160p+ (per `cfg.exclude_hdr` / /// `cfg.exclude_min_height` — the first pass is scoped to SDR 1080p/720p), -/// not already queued or done. Deliberately re-derives the anime exclusion -/// inline (rather than calling `is_anime` per row) so it's one query instead -/// of N+1 against a ~1400-file backlog. +/// not already queued, done, or `skipped` (a completed encode that +/// `finalize_job` rejected as not-beneficial — re-selecting it here would +/// re-run the full encode from scratch every time this backfill runs, only +/// to reject it again; a genuinely `failed` job *is* re-selected, since a +/// transient failure — disk full, transient ffmpeg crash — deserves a retry +/// on the next manual run). Anime is included (routed to its own +/// pipeline, not excluded) — `is_anime` on each candidate is the metadata +/// half of that decision; combined with the path-based check +/// (`is_anime_path`) in Rust after the query, since a dynamic list of path +/// prefixes (`cfg.anime_root_folders`) doesn't fit cleanly into static SQL. pub fn find_backlog_candidates(conn: &Connection, cfg: &TranscodeConfig) -> Result> { let mut stmt = conn.prepare( - "SELECT ef.id, ef.path, ef.size_bytes, p.video_codec, p.width, p.height + "SELECT ef.id, ef.path, ef.size_bytes, p.video_codec, + (m.tvdb_id IS NOT NULL AND m.tvdb_id IN + (SELECT tvdb_id FROM anime_mapping WHERE tvdb_id IS NOT NULL)) + OR + (m.tmdb_id IS NOT NULL AND m.tmdb_id IN (SELECT tmdb_id FROM anime_tmdb_movie)) + AS db_is_anime FROM episode_file ef JOIN media_file_probe p ON p.episode_file_id = ef.id LEFT JOIN episode e ON e.id = ef.episode_id @@ -303,22 +649,73 @@ pub fn find_backlog_candidates(conn: &Connection, cfg: &TranscodeConfig) -> Resu AND (ef.upgrade_locked IS NULL OR ef.upgrade_locked = 0) AND (?1 = 0 OR p.hdr = 0) AND (p.height IS NOT NULL AND p.height < ?2) - AND NOT ( - (m.tvdb_id IS NOT NULL AND m.tvdb_id IN - (SELECT tvdb_id FROM anime_mapping WHERE tvdb_id IS NOT NULL)) - OR - (m.tmdb_id IS NOT NULL AND m.tmdb_id IN (SELECT tmdb_id FROM anime_tmdb_movie)) - ) AND ef.id NOT IN ( - SELECT episode_file_id FROM transcode_job WHERE status IN ('pending','running','done') + SELECT episode_file_id FROM transcode_job WHERE status IN ('pending','running','done','skipped') ) ORDER BY (ef.size_bytes * 8.0 / NULLIF(p.duration_secs, 0)) DESC", )?; let rows = stmt .query_map(params![cfg.exclude_hdr as i64, cfg.exclude_min_height], |row| { + let path: String = row.get(1)?; + let db_is_anime: bool = row.get(4)?; Ok(BacklogCandidate { episode_file_id: row.get(0)?, - path: row.get(1)?, + is_anime: db_is_anime || is_anime_path(Path::new(&path), cfg), + path, + size_bytes: row.get(2)?, + video_codec: row.get(3)?, + }) + })? + .collect::>>()?; + Ok(rows) +} + +/// Every file whose *most recent* transcode attempt succeeded (`done`) but +/// wasn't actually beneficial by the current `min_size_reduction_pct` bar — +/// the 476-file legacy of a real rate-control bug that left files larger +/// than their original, back before `is_beneficial` existed as a hard +/// invariant. These are already AV1 (`find_backlog_candidates` excludes +/// them for exactly that reason), and their pre-transcode originals are +/// long gone — the only way to reclaim the space is to re-transcode the +/// current, oversized AV1 file itself, which needs `enqueue`'s +/// `force_reencode: true` to get past `encode_and_verify`'s normal +/// already-AV1 short-circuit. The `WITH latest_job` CTE picks each file's +/// single most recent job so a file already successfully re-transcoded in +/// a prior run of this same query (its latest job now `done` and properly +/// smaller) doesn't get selected again. +pub fn find_oversized_av1_candidates(conn: &Connection, cfg: &TranscodeConfig) -> Result> { + let mut stmt = conn.prepare( + "WITH latest_job AS ( + SELECT episode_file_id, MAX(id) AS job_id + FROM transcode_job + GROUP BY episode_file_id + ) + SELECT ef.id, ef.path, ef.size_bytes, p.video_codec, + (m.tvdb_id IS NOT NULL AND m.tvdb_id IN + (SELECT tvdb_id FROM anime_mapping WHERE tvdb_id IS NOT NULL)) + OR + (m.tmdb_id IS NOT NULL AND m.tmdb_id IN (SELECT tmdb_id FROM anime_tmdb_movie)) + AS db_is_anime + FROM latest_job lj + JOIN transcode_job tj ON tj.id = lj.job_id + JOIN episode_file ef ON ef.id = lj.episode_file_id + JOIN media_file_probe p ON p.episode_file_id = ef.id + LEFT JOIN episode e ON e.id = ef.episode_id + JOIN media_item m ON m.id = COALESCE(e.media_item_id, ef.media_item_id) + WHERE tj.status = 'done' + AND tj.original_bytes IS NOT NULL AND tj.new_bytes IS NOT NULL AND tj.original_bytes > 0 + AND tj.new_bytes > tj.original_bytes * (1.0 - ?1) + AND p.video_codec = 'av1' + ORDER BY (tj.new_bytes - tj.original_bytes) DESC", + )?; + let rows = stmt + .query_map(params![cfg.min_size_reduction_pct], |row| { + let path: String = row.get(1)?; + let db_is_anime: bool = row.get(4)?; + Ok(BacklogCandidate { + episode_file_id: row.get(0)?, + is_anime: db_is_anime || is_anime_path(Path::new(&path), cfg), + path, size_bytes: row.get(2)?, video_codec: row.get(3)?, }) @@ -332,6 +729,7 @@ pub struct BacklogCandidate { pub path: String, pub size_bytes: i64, pub video_codec: Option, + pub is_anime: bool, } /// A `transcode_job` row claimed for processing this cycle, with the @@ -344,58 +742,74 @@ struct ClaimedJob { width: i64, height: i64, duration_secs: Option, + is_anime: bool, + force_reencode: bool, } -/// Claims up to `limit` `pending` jobs (marking them `running` so a crash -/// mid-cycle doesn't leave them silently re-claimable forever without at -/// least having been attempted once) and returns everything the encode step +/// Claims up to `live_action_limit` live-action jobs and up to +/// `anime_limit` anime jobs (marking them `running` so a crash mid-cycle +/// doesn't leave them silently re-claimable forever without at least +/// having been attempted once) and returns everything the encode step /// needs. Only claims jobs whose file already has probe data — a job /// enqueued moments after import but before `ensure_probed` has run yet /// simply isn't claimed this tick, and picks up naturally on the next one. /// -/// `limit` is treated as a *total* concurrency target, not "claim this many -/// more" — it's first reduced by however many jobs are already `running` -/// (set by anyone: this same daemon's previous still-in-flight cycle, or a -/// separately-invoked `transcode-library` backfill process hitting the same -/// database). Learned the hard way: without this, a long-running cycle -/// (large files easily take longer than `poll_interval_secs`) and a -/// concurrently-run backfill each independently claimed up to their own -/// `parallelism_max` with no awareness of the other, stacking to 20+ +/// The two limits are enforced **independently** — a real gap found after +/// raising the live-action cap for a validated GPU-scaling reason: a single +/// shared cap would let "raise GPU parallelism" silently also raise anime +/// (CPU-bound, thread-hungry) concurrency to something never load-tested. +/// Each limit is treated as a *total* concurrency target for its own +/// pipeline, not "claim this many more" — first reduced by however many +/// jobs of that same pipeline are already `running` (set by anyone: this +/// same daemon's previous still-in-flight cycle, or a separately-invoked +/// `transcode-library`/`retranscode-oversized` process hitting the same +/// database). Learned the hard way (the *original* version of this bug, +/// before the pipelines were even split): without this, a long-running +/// cycle and a concurrently-run backfill each independently claimed up to +/// their own cap with no awareness of the other, stacking to 20+ /// simultaneous GPU encodes and OOMing the host. `status='running'` is /// shared database state, not per-process, so counting it is what makes /// this a real global cap no matter how many processes are hitting this -/// table at once — and wrapping the count+select+update in one -/// `BEGIN IMMEDIATE` transaction (rather than three separate statements) -/// is what stops two processes from both reading the same low count and -/// both claiming past the limit before either one's UPDATE lands. -async fn claim_pending_jobs(conn: &Arc>, limit: usize) -> Result> { +/// table at once — and wrapping each pipeline's count+select+update in one +/// `BEGIN IMMEDIATE` transaction (rather than separate statements, and both +/// pipelines in the *same* transaction) is what stops two processes from +/// both reading the same low count and both claiming past the limit before +/// either one's UPDATE lands. +async fn claim_pending_jobs( + conn: &Arc>, + live_action_limit: usize, + anime_limit: usize, +) -> Result> { let mut conn = conn.lock().await; let tx = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; - let running: i64 = tx.query_row( - "SELECT count(*) FROM transcode_job WHERE status = 'running'", - [], - |row| row.get(0), - )?; - let available = (limit as i64 - running).max(0); + let mut claimed = Vec::new(); + for (is_anime_flag, limit) in [(0i64, live_action_limit), (1i64, anime_limit)] { + let running: i64 = tx.query_row( + "SELECT count(*) FROM transcode_job WHERE status = 'running' AND is_anime = ?1", + params![is_anime_flag], + |row| row.get(0), + )?; + let available = (limit as i64 - running).max(0); + if available == 0 { + continue; + } - let claimed = if available == 0 { - Vec::new() - } else { - // Highest current bitrate first — the worst offenders (REMUX, huge - // season packs) free the most space per file transcoded, so - // they're worth reaching before smaller, already-reasonable files. + // Highest current bitrate first (within this pipeline) — the worst + // offenders (REMUX, huge season packs) free the most space per + // file transcoded, so they're worth reaching before smaller, + // already-reasonable files. let mut stmt = tx.prepare( - "SELECT j.id, j.episode_file_id, ef.path, p.width, p.height, p.duration_secs + "SELECT j.id, j.episode_file_id, ef.path, p.width, p.height, p.duration_secs, j.is_anime, j.force_reencode FROM transcode_job j JOIN episode_file ef ON ef.id = j.episode_file_id JOIN media_file_probe p ON p.episode_file_id = ef.id - WHERE j.status = 'pending' AND p.width IS NOT NULL AND p.height IS NOT NULL + WHERE j.status = 'pending' AND j.is_anime = ?2 AND p.width IS NOT NULL AND p.height IS NOT NULL ORDER BY (ef.size_bytes * 8.0 / NULLIF(p.duration_secs, 0)) DESC LIMIT ?1", )?; - let claimed = stmt - .query_map(params![available], |row| { + let batch = stmt + .query_map(params![available, is_anime_flag], |row| { Ok(ClaimedJob { job_id: row.get(0)?, episode_file_id: row.get(1)?, @@ -403,35 +817,38 @@ async fn claim_pending_jobs(conn: &Arc>, limit: usize) -> Resu width: row.get(3)?, height: row.get(4)?, duration_secs: row.get(5)?, + is_anime: row.get::<_, i64>(6)? != 0, + force_reencode: row.get::<_, i64>(7)? != 0, }) })? .collect::>>()?; - for job in &claimed { + for job in &batch { tx.execute( "UPDATE transcode_job SET status = 'running' WHERE id = ?1", params![job.job_id], )?; } - claimed - }; + claimed.extend(batch); + } tx.commit()?; Ok(claimed) } -/// Finalizes one job under the DB lock: on success, atomically swaps the -/// verified temp file over the original, updates `episode_file` (new size + -/// `upgrade_locked = 1`, the flag that keeps the upgrade cycle from ever -/// trying to replace a file breadarr itself just intentionally shrank), and -/// forces a fresh `ensure_probed` so `media_file_probe` reflects the real -/// AV1 ground truth — same shape as `remux_one_backlog_file`. On failure, -/// the original is left completely untouched; the job is marked `failed` -/// with the error recorded, no automatic retry. +/// Finalizes one job under the DB lock: on a real `Encoded` result, +/// atomically swaps the verified temp file over the original, updates +/// `episode_file` (new size + `upgrade_locked = 1`, the flag that keeps the +/// upgrade cycle from ever trying to replace a file breadarr itself just +/// intentionally shrank), and forces a fresh `ensure_probed` so +/// `media_file_probe` reflects the real AV1 ground truth — same shape as +/// `remux_one_backlog_file`. On failure, the original is left completely +/// untouched; the job is marked `failed` with the error recorded, no +/// automatic retry. /// -/// `Ok(None)` from the encode step means `encode_and_verify` found the -/// input already AV1 and skipped the encode entirely — marked `done` with -/// no byte-count change, not `failed`. +/// `AlreadyAv1`/`NotBeneficial` from the encode step are both marked `done`/ +/// `skipped` respectively with no byte-count change and no swap — neither +/// is a failure, there's just nothing to keep. /// /// The whole swap-and-bookkeeping sequence runs inside a closure so any /// failure partway through (a deleted-out-from-under-it original, a @@ -442,42 +859,78 @@ async fn claim_pending_jobs(conn: &Arc>, limit: usize) -> Resu async fn finalize_job( conn: &Arc>, job: ClaimedJob, - encode_result: Result>, -) -> Result { - let conn = conn.lock().await; + encode_result: Result, +) -> Result { + let mut conn = conn.lock().await; let tmp_path = match &encode_result { - Ok(Some(p)) => Some(p.clone()), + Ok(EncodeOutcome::Encoded(p)) => Some(p.clone()), _ => None, }; - let swap: Result = match encode_result { - Ok(None) => Ok(TranscodeOutcome { original_bytes: 0, new_bytes: 0 }), - Ok(Some(tmp_path)) => (|| { + let swap: Result<(&'static str, TranscodeOutcome)> = match encode_result { + Ok(EncodeOutcome::AlreadyAv1) => { + Ok(("done", TranscodeOutcome { original_bytes: 0, new_bytes: 0 })) + } + Ok(EncodeOutcome::NotBeneficial) => { + Ok(("skipped", TranscodeOutcome { original_bytes: 0, new_bytes: 0 })) + } + Ok(EncodeOutcome::Encoded(tmp_path)) => (|| { let original_bytes = std::fs::metadata(&job.path)?.len(); std::fs::rename(&tmp_path, &job.path)?; let new_bytes = std::fs::metadata(&job.path)?.len(); - conn.execute( + // `size_bytes` and `upgrade_locked` must land together, in one + // transaction: the rename just above is already irreversible in + // practice (the temp file is gone), so if these two writes were + // separate auto-committed statements and the second one failed, + // the on-disk file would already be the smaller AV1 encode while + // `upgrade_locked` stayed 0 — leaving it exactly as eligible for + // the ordinary upgrade cycle to "improve" as any untouched file, + // silently replacing a deliberate, already-paid-for shrink with + // a much larger release. Wrapping both in one transaction means + // either both land or neither does; on failure `swap` returns + // `Err` below and the job is marked `failed` for a human to + // notice, rather than quietly losing the lock. + let tx = conn.transaction()?; + tx.execute( "UPDATE episode_file SET size_bytes = ?1, upgrade_locked = 1 WHERE id = ?2", params![new_bytes as i64, job.episode_file_id], )?; - conn.execute( + tx.execute( "DELETE FROM media_file_probe WHERE episode_file_id = ?1", params![job.episode_file_id], )?; - importer::ensure_probed(&conn, job.episode_file_id, &job.path)?; - Ok(TranscodeOutcome { original_bytes, new_bytes }) + tx.commit()?; + + // Best-effort from here: `ensure_probed` only refreshes + // `media_file_probe`'s descriptive fields (codec/height/etc, fed + // to `find_backlog_candidates` and library-health reporting) — + // it already never propagates an *ffprobe* failure (see its own + // doc comment: "probing must never abort a scan or import"), so + // an `Err` here means a genuine DB-level error, most likely + // transient. The facts that actually matter — the file is now + // AV1, its new size, and `upgrade_locked` — are already durably + // committed above; failing the whole job over a stale probe row + // would misreport a successful transcode as `failed`. + if let Err(e) = importer::ensure_probed(&conn, job.episode_file_id, &job.path) { + tracing::warn!( + episode_file_id = job.episode_file_id, + error = %e, + "post-transcode re-probe failed; media_file_probe left stale until the next scan" + ); + } + Ok(("done", TranscodeOutcome { original_bytes, new_bytes })) })(), Err(e) => Err(e), }; match swap { - Ok(outcome) => { + Ok((status, outcome)) => { conn.execute( - "UPDATE transcode_job SET status = 'done', new_bytes = ?1, finished_at = datetime('now') WHERE id = ?2", - params![outcome.new_bytes as i64, job.job_id], + "UPDATE transcode_job SET status = ?1, new_bytes = ?2, finished_at = datetime('now') WHERE id = ?3", + params![status, outcome.new_bytes as i64, job.job_id], )?; - Ok(outcome) + Ok(FinalizedJob { status, outcome }) } Err(e) => { if let Some(tmp_path) = tmp_path { @@ -497,76 +950,226 @@ pub struct TranscodeOutcome { pub new_bytes: u64, } +struct FinalizedJob { + status: &'static str, + outcome: TranscodeOutcome, +} + #[derive(Debug, Default)] pub struct TranscodeCycleStats { pub attempted: usize, pub succeeded: usize, + pub skipped: usize, pub failed: usize, pub bytes_saved: i64, } -/// Active parallelism for this cycle: full `parallelism_max` when nobody's -/// actively watching a transcoded Jellyfin stream, dropped to -/// `parallelism_min` (1) the moment anyone is — the batch job and a real -/// viewer are contending for the same GPU encode/decode engines, and a -/// stutter during someone's actual show loses every time. -async fn effective_parallelism(jellyfin: Option<&JellyfinClient>, cfg: &TranscodeConfig) -> usize { - let Some(client) = jellyfin else { - return cfg.parallelism_max; - }; - match client.active_transcode_sessions().await { - Ok(0) => cfg.parallelism_max, - Ok(_) => cfg.parallelism_min, - Err(e) => { - tracing::warn!(error = %e, "failed to poll jellyfin sessions; assuming worst case"); - cfg.parallelism_min +impl TranscodeCycleStats { + fn merge(self, other: Self) -> Self { + TranscodeCycleStats { + attempted: self.attempted + other.attempted, + succeeded: self.succeeded + other.succeeded, + skipped: self.skipped + other.skipped, + failed: self.failed + other.failed, + bytes_saved: self.bytes_saved + other.bytes_saved, } } } -/// One transcode-worker pass: claims up to the current (Jellyfin-aware) -/// parallelism worth of pending jobs, encodes them concurrently entirely -/// outside the DB lock (each encode can take minutes — holding the shared -/// mutex for that long would stall every other cycle: import, search, -/// review-queue actions, everything), then finalizes each result under a -/// brief lock. Shared by both the steady-state daemon ticker and the -/// `transcode-library` backfill CLI, so there's exactly one code path that -/// actually runs an encode. +/// The live-action pipeline's parallelism given how many real Jellyfin +/// transcode sessions are active right now: `budget` (`parallelism_max`, +/// the measured total GPU concurrency ceiling — see its doc comment) minus +/// however many streams Jellyfin itself is actually using, floored at 0. +/// Reserves *exactly* the GPU headroom real viewers need instead of +/// dropping to a flat `parallelism_min` regardless of whether 1 or 5 +/// people are watching — the whole point of measuring the real ceiling +/// (rather than guessing) is to spend the difference precisely. Pulled out +/// as its own pure function for direct unit coverage without needing a +/// real (or mocked) `JellyfinClient`. +fn live_action_parallelism_for(budget: usize, active_jellyfin_sessions: usize) -> usize { + budget.saturating_sub(active_jellyfin_sessions) +} + +/// Active parallelism for this cycle, returned as `(live_action, anime)`. +/// Live-action: `live_action_parallelism_for(cfg.parallelism_max, +/// active_sessions)` — dynamically reserves exactly as many GPU streams as +/// Jellyfin is actually using, since Jellyfin transcoding contends for the +/// same encode/decode engines the batch job does. Anime: still a flat +/// `parallelism_max_anime`/`parallelism_min` binary switch on *any* active +/// session — the CPU-bound anime pipeline doesn't contend for the GPU the +/// way live-action does, so it isn't part of this dynamic reservation (per +/// explicit user direction: "for the live action ones"); it still backs +/// off to `parallelism_min` (not eliminated, just conservative) whenever +/// someone's actively watching anything, on the more general theory that a +/// real viewer's experience should never compete with unattended batch +/// work for the box's resources, GPU or not. Only one Jellyfin poll either +/// way, not one per pipeline. +async fn effective_parallelism(jellyfin: Option<&JellyfinClient>, cfg: &TranscodeConfig) -> (usize, usize) { + let Some(client) = jellyfin else { + return (cfg.parallelism_max, cfg.parallelism_max_anime); + }; + match client.active_transcode_sessions().await { + Ok(active_sessions) => { + let live_action = live_action_parallelism_for(cfg.parallelism_max, active_sessions); + let anime = if active_sessions == 0 { cfg.parallelism_max_anime } else { cfg.parallelism_min }; + (live_action, anime) + } + Err(e) => { + tracing::warn!(error = %e, "failed to poll jellyfin sessions; assuming worst case"); + (cfg.parallelism_min, cfg.parallelism_min) + } + } +} + +/// One transcode-worker pass across both pipelines. Runs +/// `run_pipeline_cycle` for live-action and anime *concurrently* via +/// `tokio::join!` and merges their stats — see that function's doc +/// comment for why they must not share one claim-spawn-join batch. +/// +/// Deliberately `join!`, not `try_join!`: `try_join!` cancels (drops) the +/// other branch's future the instant either one returns `Err`, and dropping +/// a `JoinSet` of `spawn_blocking` encode tasks cannot abort work that has +/// already started — the ffmpeg/SVT-AV1 process keeps running to completion +/// with its result simply discarded, `finalize_job` never runs for it, and +/// its `transcode_job` row is stuck `status='running'` (consuming +/// concurrency budget and leaving its multi-GB temp file on disk) until the +/// daemon restarts. `run_pipeline_cycle` itself now treats a +/// `claim_pending_jobs` error as "claim nothing this iteration" rather than +/// propagating it, so in practice this only guards a pipeline that fails for +/// some other reason — but `join!` means even that failure can no longer +/// take the other, healthy pipeline's in-flight work down with it. Each +/// pipeline's error is logged and treated as an empty cycle rather than +/// failing the whole `run_cycle` call. Shared by both the steady-state +/// daemon ticker and the `transcode-library` backfill CLI, so there's +/// exactly one code path that actually runs an encode. pub async fn run_cycle( conn: Arc>, cfg: TranscodeConfig, jellyfin: Option<&JellyfinClient>, ) -> Result { - let parallelism = effective_parallelism(jellyfin, &cfg).await; - let claimed = claim_pending_jobs(&conn, parallelism).await?; + let (live_action_result, anime_result) = tokio::join!( + run_pipeline_cycle(conn.clone(), cfg.clone(), jellyfin, false), + run_pipeline_cycle(conn.clone(), cfg.clone(), jellyfin, true), + ); + let live_action_stats = live_action_result.unwrap_or_else(|e| { + tracing::warn!(error = %e, "live-action transcode pipeline cycle failed"); + TranscodeCycleStats::default() + }); + let anime_stats = anime_result.unwrap_or_else(|e| { + tracing::warn!(error = %e, "anime transcode pipeline cycle failed"); + TranscodeCycleStats::default() + }); + Ok(live_action_stats.merge(anime_stats)) +} +/// One pipeline's worker loop: repeatedly claims whatever's currently +/// available for *this* pipeline only (the other pipeline's limit is passed +/// as 0, so `claim_pending_jobs` naturally claims nothing for it), spawns +/// each newly claimed job, and as soon as any one of them finishes, +/// finalizes it and immediately loops back to claim a replacement — rather +/// than waiting for the whole batch to drain before claiming more. Exits +/// once a claim attempt comes back empty and nothing is left in flight. +/// +/// This replaces an earlier design where one shared claim+spawn+join batch +/// covered both pipelines at once: claimed jobs from both pipelines were +/// joined together, so the *whole* batch — and therefore any new claim for +/// either pipeline — blocked until every job in it finished. A single long +/// anime file (feature-length movies can take over an hour via the +/// CPU-bound SVT-AV1 path) would leave the GPU-bound live-action pipeline +/// sitting completely idle for that entire duration despite having its own +/// independent concurrency budget and a large backlog of its own — +/// discovered in production: 512 pending live-action jobs and a budget of 7, +/// but the GPU sat unused for the better part of an hour waiting on one +/// anime movie to join. Running each pipeline as its own independent +/// replenish-loop, joined concurrently rather than sequentially, means +/// neither pipeline's throughput is ever gated by the other's job durations. +/// +/// Uses `JoinSet::join_next_with_id` rather than awaiting a `Vec` of handles +/// in order: a real validation run surfaced that the naive +/// awaited-in-claim-order approach leaves a fast job's already-finished +/// result (verified output sitting on disk, or even a fast failure) fully +/// idle — not written back to the DB, not freeing its slot for the next +/// claim — for as long as whichever job happens to be *earlier* in claim +/// order (highest-bitrate-first, so often the largest file) keeps running. +/// `join_next_with_id`'s `(Id, T)`/`JoinError::id()` pairing is what lets a +/// panicked task still be matched back to its `ClaimedJob` (a panic doesn't +/// get to return the job alongside its result), which a plain +/// `JoinSet>` with the job moved into the closure +/// wouldn't allow. +async fn run_pipeline_cycle( + conn: Arc>, + cfg: TranscodeConfig, + jellyfin: Option<&JellyfinClient>, + is_anime: bool, +) -> Result { let mut stats = TranscodeCycleStats::default(); - if claimed.is_empty() { - return Ok(stats); - } + let mut set = tokio::task::JoinSet::new(); + let mut jobs_by_task_id: std::collections::HashMap = + std::collections::HashMap::new(); - let mut handles = Vec::with_capacity(claimed.len()); - for job in claimed { - let cfg = cfg.clone(); - let path = job.path.clone(); - let job_id = job.job_id; - let (width, height, duration) = (job.width, job.height, job.duration_secs); - let encode_handle = tokio::task::spawn_blocking(move || { - encode_and_verify(path, job_id, cfg, width, height, duration) - }); - handles.push((job, encode_handle)); - } - - for (job, encode_handle) in handles { - stats.attempted += 1; - let encode_result = match encode_handle.await { - Ok(result) => result, - Err(join_err) => Err(anyhow::anyhow!("encode task panicked: {join_err}")), + loop { + let (live_action_parallelism, anime_parallelism) = effective_parallelism(jellyfin, &cfg).await; + let (live_action_limit, anime_limit) = if is_anime { + (0, anime_parallelism) + } else { + (live_action_parallelism, 0) }; + // A claim failure (e.g. transient SQLITE_BUSY under `busy_timeout` + // when another process holds the write lock) must not propagate out + // of this loop: that would return `Err` from `run_pipeline_cycle`, + // and in `run_cycle` that used to cancel the *other* pipeline's + // future mid-flight via `try_join!`, orphaning its already-running + // encodes (see `run_cycle`'s doc comment). Skip claiming new work + // this iteration instead — whatever's already in `set` keeps + // draining normally, and the next tick retries the claim. + let claimed = match claim_pending_jobs(&conn, live_action_limit, anime_limit).await { + Ok(claimed) => claimed, + Err(e) => { + tracing::warn!(error = %e, is_anime, "failed to claim pending transcode jobs this cycle; will retry next iteration"); + Vec::new() + } + }; + + for job in claimed { + let cfg = cfg.clone(); + let path = job.path.clone(); + let job_id = job.job_id; + let (width, height, duration, job_is_anime, force_reencode) = + (job.width, job.height, job.duration_secs, job.is_anime, job.force_reencode); + let abort_handle = set.spawn_blocking(move || { + encode_and_verify(path, job_id, cfg, width, height, duration, job_is_anime, force_reencode) + }); + jobs_by_task_id.insert(abort_handle.id(), job); + } + + // `None` here means this claim found nothing new *and* nothing from + // an earlier claim is still in flight — this pipeline's queue is + // genuinely empty for now. + let Some(joined) = set.join_next_with_id().await else { + break; + }; + + stats.attempted += 1; + let (task_id, encode_result) = match joined { + Ok((task_id, result)) => (task_id, result), + Err(join_err) => { + let task_id = join_err.id(); + (task_id, Err(anyhow::anyhow!("encode task panicked: {join_err}"))) + } + }; + let job = jobs_by_task_id + .remove(&task_id) + .expect("every spawned task's id was inserted before the task could complete"); + match finalize_job(&conn, job, encode_result).await { - Ok(outcome) => { - stats.succeeded += 1; - stats.bytes_saved += outcome.original_bytes as i64 - outcome.new_bytes as i64; + Ok(finalized) => { + if finalized.status == "skipped" { + stats.skipped += 1; + } else { + stats.succeeded += 1; + } + stats.bytes_saved += + finalized.outcome.original_bytes as i64 - finalized.outcome.new_bytes as i64; } Err(e) => { stats.failed += 1; @@ -589,11 +1192,20 @@ mod tests { vaapi_device: "/dev/dri/renderD128".to_string(), parallelism_min: 1, parallelism_max: 4, + parallelism_max_anime: 2, reference_bitrate_kbps: 5320, reference_height: 1080, av1_efficiency_factor: 0.7, exclude_hdr: true, exclude_min_height: 2000, + quality_live_action: 26, + anime_root_folders: vec!["/mnt/media/Anime".to_string(), "/mnt/media/Anime Movies".to_string()], + quality_anime: 24, + anime_svtav1_preset: 10, + anime_svtav1_max_threads: 2, + min_size_reduction_pct: 0.10, + skip_below_ceiling_ratio: 0.5, + verify_sample_secs: 20.0, } } @@ -647,9 +1259,11 @@ mod tests { } let conn = Arc::new(Mutex::new(conn)); - // Asking for a total of 3 concurrent, with 2 already running — - // should claim exactly 1 more, not 3 more. - let claimed = claim_pending_jobs(&conn, 3).await.unwrap(); + // Asking for a total of 3 concurrent live-action, with 2 already + // running — should claim exactly 1 more, not 3 more. All seeded + // jobs default to is_anime=0 (live-action), so anime_limit=0 here + // is correct, not a placeholder. + let claimed = claim_pending_jobs(&conn, 3, 0).await.unwrap(); assert_eq!(claimed.len(), 1); let conn = conn.lock().await; @@ -659,6 +1273,29 @@ mod tests { assert_eq!(running, 3, "total in-flight jobs must never exceed the requested cap"); } + // Regression test for a real gap found in review: a file whose encode + // completed but was rejected by `is_beneficial` (recorded `skipped` by + // `finalize_job`) was not in `find_backlog_candidates`'s exclusion list, + // so every re-run of the `transcode-library` backfill re-selected it, + // paying the full GPU/CPU encode cost again only to reject it again. + #[test] + fn find_backlog_candidates_excludes_skipped_jobs() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + seed_file(&conn, 1, 1_000_000_000); + conn.execute( + "INSERT INTO transcode_job (episode_file_id, status, queued_at) VALUES (1, 'skipped', datetime('now'))", + [], + ) + .unwrap(); + + let candidates = find_backlog_candidates(&conn, &cfg()).unwrap(); + assert!( + candidates.is_empty(), + "a file with a 'skipped' (not-beneficial) job must not be re-selected for backfill" + ); + } + #[tokio::test] async fn claim_pending_jobs_claims_nothing_when_already_at_the_cap() { let conn = Connection::open_in_memory().unwrap(); @@ -680,10 +1317,50 @@ mod tests { .unwrap(); let conn = Arc::new(Mutex::new(conn)); - let claimed = claim_pending_jobs(&conn, 2).await.unwrap(); + let claimed = claim_pending_jobs(&conn, 2, 0).await.unwrap(); assert!(claimed.is_empty(), "already at the cap — must not claim more"); } + // Regression test for a real gap: raising the live-action cap for a + // validated GPU-scaling reason must not silently also raise anime + // concurrency (CPU-bound, thread-hungry, never load-tested at higher + // counts) — the two caps are enforced completely independently. + #[tokio::test] + async fn claim_pending_jobs_enforces_live_action_and_anime_caps_independently() { + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + for id in 1..=6 { + seed_file(&conn, id, 1_000_000_000); + } + // 3 live-action pending (ids 1-3), 3 anime pending (ids 4-6). + for id in 1..=3 { + conn.execute( + "INSERT INTO transcode_job (episode_file_id, status, is_anime, queued_at) VALUES (?1, 'pending', 0, datetime('now'))", + params![id], + ) + .unwrap(); + } + for id in 4..=6 { + conn.execute( + "INSERT INTO transcode_job (episode_file_id, status, is_anime, queued_at) VALUES (?1, 'pending', 1, datetime('now'))", + params![id], + ) + .unwrap(); + } + + let conn = Arc::new(Mutex::new(conn)); + // A generous live-action cap (6, matching the real raised default) + // alongside a conservative anime cap (2) must claim up to 6 + // live-action jobs (only 3 exist) but no more than 2 anime jobs, + // even though 3 anime jobs are pending and the anime cap is far + // below the live-action one. + let claimed = claim_pending_jobs(&conn, 6, 2).await.unwrap(); + let live_action_claimed = claimed.iter().filter(|j| !j.is_anime).count(); + let anime_claimed = claimed.iter().filter(|j| j.is_anime).count(); + assert_eq!(live_action_claimed, 3, "all 3 pending live-action jobs should be claimed"); + assert_eq!(anime_claimed, 2, "anime claims must stay capped at 2 regardless of the live-action cap"); + } + fn generate_test_clip(dir: &Path, codec: &str) -> PathBuf { let path = dir.join(format!("clip_{codec}.mkv")); let status = std::process::Command::new("ffmpeg") @@ -700,6 +1377,24 @@ mod tests { path } + // Regression test for a real gap found in review: `ensure_probed` + // inserts a `probe_failed` row with NULL codec/height on ffprobe + // failure rather than leaving no row at all, so `maybe_enqueue_transcode`'s + // "no probe row -> skip" guard never fires for it. Without this check, + // `should_enqueue` would return true for that NULL/NULL row, `enqueue` + // would insert a job `claim_pending_jobs` can never select (it requires + // `p.width`/`p.height IS NOT NULL`), and the unique partial index on + // (pending, running) would then permanently block any future, real + // enqueue for the file once probing eventually succeeds. + #[test] + fn should_enqueue_rejects_missing_codec_or_height() { + let c = cfg(); + assert!(!should_enqueue(None, false, None, &c), "no codec, no height -> never enqueue"); + assert!(!should_enqueue(None, false, Some(1080), &c), "missing codec alone must block enqueue"); + assert!(!should_enqueue(Some("hevc"), false, None, &c), "missing height alone must block enqueue"); + assert!(should_enqueue(Some("hevc"), false, Some(1080), &c), "codec+height present -> normal eligibility rules apply"); + } + // Regression test for a real gap found in review: a crash between the // file-swap rename and the DB bookkeeping in `finalize_job` leaves a job // `running` with the file already AV1 but `media_file_probe` still @@ -715,12 +1410,345 @@ mod tests { std::fs::create_dir_all(&dir).unwrap(); let clip = generate_test_clip(&dir, "libsvtav1"); - let result = encode_and_verify(clip.clone(), 999, cfg(), 320, 240, Some(1.0)).unwrap(); - assert!(result.is_none(), "an already-AV1 file must not be re-encoded"); + let result = + encode_and_verify(clip.clone(), 999, cfg(), 320, 240, Some(1.0), false, false).unwrap(); + assert!( + matches!(result, EncodeOutcome::AlreadyAv1), + "an already-AV1 file must not be re-encoded" + ); std::fs::remove_dir_all(&dir).unwrap(); } + // Regression test for a real gap found in review: the temp path used to + // be produced via `input.with_extension("job{id}.av1.mkv")`, so a + // multi-hour in-progress encode sat in the library as a normal-looking + // `.mkv` file — `collect_video_files`/`walk_files` (both filter purely + // on `VIDEO_EXTS`) would pick it up as a second candidate video file for + // the same episode, and `library_scan`/Jellyfin could index or attempt + // to play a still-encoding file. The fix must produce a path whose + // final extension isn't in `VIDEO_EXTS` at all, still under the same + // parent directory (required for the atomic rename-based swap), and + // still job-id-scoped (so two jobs on the same file never collide). + #[test] + fn temp_path_for_is_not_picked_up_by_video_file_scans() { + let input = Path::new("/library/Show/Season 01/Show - S01E01 - Title.mkv"); + let tmp = temp_path_for(input, 42); + + assert_eq!(tmp.parent(), input.parent(), "must stay on the same filesystem/directory as the original"); + let ext = tmp.extension().and_then(|e| e.to_str()).unwrap_or("").to_lowercase(); + assert!( + !crate::importer::VIDEO_EXTS.contains(&ext.as_str()), + "temp path {tmp:?} has a video extension and would be picked up by library scans" + ); + assert!( + tmp.to_string_lossy().contains("job42"), + "must stay job-id-scoped so two jobs on the same file can never collide" + ); + + // Different job ids on the same input must never collide. + let other = temp_path_for(input, 43); + assert_ne!(tmp, other); + } + + // Fast, deterministic regression test for the actual root cause of a + // real incident: flat-bitrate rate control on `av1_vaapi` produced + // outputs *larger* than the original on 476 of 700 backfilled files. + // This is the exact arithmetic that must reject that outcome regardless + // of how any encoder's rate control behaves. + #[test] + fn is_beneficial_rejects_growth_and_marginal_shrinkage() { + // The real incident's shape: ~2.1GB HEVC -> ~4.9GB "AV1". + assert!(!is_beneficial(2_100_000_000, 4_900_000_000, 0.10)); + // Only a 5% reduction — below the 10% floor. + assert!(!is_beneficial(1_000_000_000, 950_000_000, 0.10)); + // A real win: 15% smaller, clears the floor. + assert!(is_beneficial(1_000_000_000, 850_000_000, 0.10)); + // Exactly at the floor — inclusive. + assert!(is_beneficial(1_000_000_000, 900_000_000, 0.10)); + } + + // The dynamic Jellyfin-reservation formula: reserve exactly as many + // GPU streams as Jellyfin is actually using out of the measured total + // budget, rather than dropping to a flat minimum regardless of viewer + // count. + #[test] + fn live_action_parallelism_for_reserves_exactly_what_jellyfin_is_using() { + assert_eq!(live_action_parallelism_for(7, 0), 7, "no active viewers — use the full budget"); + assert_eq!(live_action_parallelism_for(7, 1), 6, "one viewer — reserve exactly one stream"); + assert_eq!(live_action_parallelism_for(7, 3), 4, "three viewers — reserve exactly three streams"); + assert_eq!( + live_action_parallelism_for(7, 9), + 0, + "more active viewers than the whole budget — batch gets nothing, never negative" + ); + } + + // Regression test for the second half of the same incident: several of + // the 199 failures were files (some anime, notably) already efficient + // enough that transcoding them was never going to help — this is the + // pre-check that skips the encode attempt entirely rather than burning + // GPU/CPU time to find that out the slow way. No real ffmpeg encode + // should run here — a huge fabricated duration forces the source + // bitrate calculation near zero regardless of resolution, so this stays + // fast and deterministic. + #[test] + fn encode_and_verify_skips_pre_emptively_when_source_is_already_efficient() { + let dir = std::env::temp_dir().join(format!( + "breadarr-transcode-already-efficient-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let clip = generate_test_clip(&dir, "libx264"); + + let result = + encode_and_verify(clip.clone(), 1, cfg(), 1920, 1080, Some(100_000.0), false, false).unwrap(); + assert!( + matches!(result, EncodeOutcome::NotBeneficial), + "an already-efficient source must be skipped before spending encode time" + ); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + fn generate_lossless_test_clip(dir: &Path) -> PathBuf { + let path = dir.join("clip_lossless.mkv"); + let status = std::process::Command::new("ffmpeg") + .args(["-y", "-f", "lavfi", "-i", "testsrc=size=640x480:duration=2:rate=15"]) + .args(["-c:v", "libx264", "-preset", "ultrafast", "-qp", "0"]) + .arg(&path) + .output() + .expect("failed to run ffmpeg to generate a lossless test clip"); + assert!( + status.status.success(), + "ffmpeg failed to generate a lossless test clip: {}", + String::from_utf8_lossy(&status.stderr) + ); + path + } + + // Reproduces the real "embedded cover art" incident: a second video + // stream with `attached_pic` disposition (a still image, ffmpeg reports + // it as a normal `video`-type stream) alongside the real content stream. + // The old blanket `-map 0` tried to re-encode this too, which the + // VAAPI/libsvtav1 encoders can't handle — failed the whole job on + // several real library files (GoT, Avatar). + fn generate_clip_with_attached_pic(dir: &Path) -> PathBuf { + let path = dir.join("clip_with_cover_art.mkv"); + let status = std::process::Command::new("ffmpeg") + .args(["-y", "-f", "lavfi", "-i", "testsrc=size=640x480:duration=2:rate=15"]) + .args(["-f", "lavfi", "-i", "color=c=red:size=64x64:duration=1"]) + .args(["-map", "0:v", "-map", "1:v"]) + .args(["-c:v:0", "libx264", "-preset", "ultrafast", "-qp", "0"]) + .args(["-c:v:1", "png"]) + .args(["-disposition:v:1", "attached_pic"]) + .arg("-shortest") + .arg(&path) + .output() + .expect("failed to run ffmpeg to generate a test clip with attached cover art"); + assert!( + status.status.success(), + "ffmpeg failed to generate a test clip with attached cover art: {}", + String::from_utf8_lossy(&status.stderr) + ); + path + } + + // Reproduces the real "mov_text subtitle" incident: mp4 sources using + // mov_text (mp4's own timed-text codec) aren't valid inside the + // Matroska container this pipeline always writes, and stream-copying + // them failed the whole job outright. + fn generate_clip_with_mov_text_subtitle(dir: &Path) -> PathBuf { + let srt_path = dir.join("sub.srt"); + std::fs::write(&srt_path, "1\n00:00:00,000 --> 00:00:01,000\nTest subtitle\n").unwrap(); + + let path = dir.join("clip_with_mov_text.mp4"); + let status = std::process::Command::new("ffmpeg") + .args(["-y", "-f", "lavfi", "-i", "testsrc=size=640x480:duration=2:rate=15"]) + .arg("-i") + .arg(&srt_path) + .args(["-c:v", "libx264", "-preset", "ultrafast", "-qp", "0"]) + .args(["-c:s", "mov_text"]) + .args(["-f", "mp4"]) + .arg(&path) + .output() + .expect("failed to run ffmpeg to generate a test clip with a mov_text subtitle"); + assert!( + status.status.success(), + "ffmpeg failed to generate a test clip with a mov_text subtitle: {}", + String::from_utf8_lossy(&status.stderr) + ); + path + } + + #[test] + fn subtitle_codec_args_converts_only_mov_text() { + let subs = vec![ + ffprobe::SubtitleStream { language: Some("eng".to_string()), codec: Some("mov_text".to_string()) }, + ffprobe::SubtitleStream { language: Some("eng".to_string()), codec: Some("ass".to_string()) }, + ffprobe::SubtitleStream { language: None, codec: None }, + ]; + let args = subtitle_codec_args(&subs); + assert_eq!( + args, + vec![ + "-c:s:0".to_string(), "srt".to_string(), + "-c:s:1".to_string(), "copy".to_string(), + "-c:s:2".to_string(), "copy".to_string(), + ] + ); + } + + // End-to-end regression test for the real "embedded cover art" incident + // — an attached-pic stream must no longer crash the whole encode. + // Uses the anime (software) pipeline since it needs no VAAPI hardware. + #[test] + fn encode_and_verify_handles_a_source_with_attached_cover_art() { + let dir = std::env::temp_dir().join(format!( + "breadarr-transcode-attached-pic-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let clip = generate_clip_with_attached_pic(&dir); + + let mut c = cfg(); + c.skip_below_ceiling_ratio = 0.0; // force past the pre-check for this test + let result = encode_and_verify(clip.clone(), 3, c, 640, 480, Some(2.0), true, false).unwrap(); + match result { + EncodeOutcome::Encoded(tmp_path) => { + assert!(tmp_path.exists()); + let _ = std::fs::remove_file(&tmp_path); + } + other => panic!("expected a beneficial encode despite the attached cover art, got {other:?}"), + } + + std::fs::remove_dir_all(&dir).unwrap(); + } + + // End-to-end regression test for the real "mov_text subtitle" incident + // — a mov_text track must be converted rather than crashing the encode. + #[test] + fn encode_and_verify_handles_a_source_with_a_mov_text_subtitle() { + let dir = std::env::temp_dir().join(format!( + "breadarr-transcode-mov-text-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let clip = generate_clip_with_mov_text_subtitle(&dir); + + let mut c = cfg(); + c.skip_below_ceiling_ratio = 0.0; + let result = encode_and_verify(clip.clone(), 4, c, 640, 480, Some(2.0), true, false).unwrap(); + match result { + EncodeOutcome::Encoded(tmp_path) => { + assert!(tmp_path.exists()); + let _ = std::fs::remove_file(&tmp_path); + } + other => panic!("expected a beneficial encode despite the mov_text subtitle, got {other:?}"), + } + + std::fs::remove_dir_all(&dir).unwrap(); + } + + // End-to-end sanity check for the anime pipeline specifically: a real + // `libsvtav1` 10-bit encode of a deliberately bloated (lossless x264) + // source should produce a verified, meaningfully smaller output. Doesn't + // need real VAAPI hardware (unlike the live-action path) since + // `libsvtav1` is software — safe to run in any dev/CI environment that + // has an AV1-capable ffmpeg build. + #[test] + fn encode_and_verify_anime_pipeline_shrinks_a_bloated_source() { + let dir = std::env::temp_dir().join(format!( + "breadarr-transcode-anime-e2e-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let clip = generate_lossless_test_clip(&dir); + + let mut c = cfg(); + c.skip_below_ceiling_ratio = 0.0; // force past the pre-check for this test + let result = encode_and_verify(clip.clone(), 2, c, 640, 480, Some(2.0), true, false).unwrap(); + match result { + EncodeOutcome::Encoded(tmp_path) => { + assert!(tmp_path.exists()); + let _ = std::fs::remove_file(&tmp_path); + } + other => panic!("expected a beneficial encode of a lossless source, got {other:?}"), + } + + std::fs::remove_dir_all(&dir).unwrap(); + } + + // Regression test for the remediation path: `force_reencode: true` must + // bypass the normal "already AV1 -> nothing to do" short-circuit and + // attempt a real re-encode — this is the whole point of + // `find_oversized_av1_candidates`'s jobs, which target files that are + // already AV1 but were left larger than their original by a real + // rate-control bug. Uses a genuinely oversized AV1 source (a lossless + // x264 clip re-encoded to AV1 at a high bitrate, deliberately bigger + // than what a sane target would produce) so the *second* pass — the one + // under test — has real room to shrink it. + #[test] + fn encode_and_verify_force_reencode_bypasses_the_already_av1_short_circuit() { + let dir = std::env::temp_dir().join(format!( + "breadarr-transcode-force-reencode-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let oversized_av1 = dir.join("oversized.mkv"); + let status = std::process::Command::new("ffmpeg") + .args(["-y", "-f", "lavfi", "-i", "testsrc=size=640x480:duration=2:rate=15"]) + .args(["-c:v", "libsvtav1", "-crf", "10", "-preset", "12"]) // deliberately high quality/bitrate + .arg(&oversized_av1) + .output() + .expect("failed to run ffmpeg to generate an oversized AV1 test clip"); + assert!(status.status.success(), "{}", String::from_utf8_lossy(&status.stderr)); + + // Sanity check: without force_reencode, this must still short-circuit. + let unforced = + encode_and_verify(oversized_av1.clone(), 5, cfg(), 640, 480, Some(2.0), true, false).unwrap(); + assert!(matches!(unforced, EncodeOutcome::AlreadyAv1)); + + // is_anime: true — routes through the software libsvtav1 path, same + // as the other end-to-end tests here, since this dev/CI environment + // has no VAAPI hardware for the live-action path to use. + let mut c = cfg(); + c.skip_below_ceiling_ratio = 0.0; + let forced = + encode_and_verify(oversized_av1.clone(), 6, c, 640, 480, Some(2.0), true, true).unwrap(); + match forced { + EncodeOutcome::Encoded(tmp_path) => { + let new_bytes = std::fs::metadata(&tmp_path).unwrap().len(); + let original_bytes = std::fs::metadata(&oversized_av1).unwrap().len(); + assert!( + new_bytes < original_bytes, + "forced re-encode should shrink an oversized AV1 source" + ); + let _ = std::fs::remove_file(&tmp_path); + } + other => panic!("expected force_reencode to produce a real, smaller encode, got {other:?}"), + } + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn is_anime_path_matches_configured_root_folders() { + let c = cfg(); + assert!(is_anime_path( + Path::new("/mnt/media/Anime/Attack on Titan (2013)/Season 02/ep.mkv"), + &c + )); + assert!(is_anime_path( + Path::new("/mnt/media/Anime Movies/Bardock (1990)/movie.mkv"), + &c + )); + assert!(!is_anime_path( + Path::new("/mnt/media/TV Shows/The Expanse (2015)/ep.mkv"), + &c + )); + } + #[test] fn target_bitrate_matches_reference_at_reference_resolution() { let bitrate = target_bitrate_kbps(1920, 1080, &cfg()); @@ -745,4 +1773,121 @@ mod tests { let bitrate_1440 = target_bitrate_kbps(2560, 1440, &cfg()); assert!(bitrate_1440 > bitrate_1080); } + + // Regression test for a real latency bug a live validation run + // surfaced: `run_cycle` used to await claimed jobs' handles in claim + // order (highest-bitrate-first), so a fast job's already-finished + // result sat idle — not written back to the DB — for as long as + // whichever job happened to come *first* in that order kept running. + // The fix (`JoinSet::join_next_with_id`) finalizes whichever job + // actually finishes first, which raises a sharper risk worth its own + // direct test: a job's result must land on *that job's* DB row, not + // get cross-attributed to a different concurrently-running job via a + // wrong task-id lookup. This seeds one job that's claimed first (by + // this test's bitrate ordering) but takes measurably longer, and one + // claimed second that finishes near-instantly, and confirms each + // outcome lands on the correct `episode_file`/`transcode_job` row + // regardless of which one the JoinSet actually resolves first. + #[tokio::test] + async fn run_cycle_attributes_each_result_to_the_correct_job_even_out_of_claim_order() { + let dir = std::env::temp_dir().join(format!( + "breadarr-transcode-run-cycle-attribution-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + + // Claimed first (much higher bitrate) but genuinely slower — a + // real (tiny) SVT-AV1 encode, not a short-circuit. + let slow_clip = generate_lossless_test_clip(&dir); + let slow_id = 101i64; + // Claimed second (lower bitrate) but resolves almost immediately — + // already AV1, hits the `AlreadyAv1` short-circuit with no real + // encode at all. + let fast_clip = generate_test_clip(&dir, "libsvtav1"); + let fast_id = 102i64; + + let conn = Connection::open_in_memory().unwrap(); + crate::db::init(&conn).unwrap(); + for (id, path, codec, size_bytes, duration) in [ + (slow_id, &slow_clip, "h264", 50_000_000i64, 2.0), + (fast_id, &fast_clip, "av1", 1_000_000i64, 1.0), + ] { + conn.execute( + "INSERT INTO episode_file (id, episode_id, media_item_id, path, size_bytes) + VALUES (?1, NULL, NULL, ?2, ?3)", + params![id, path.to_string_lossy().to_string(), size_bytes], + ) + .unwrap(); + conn.execute( + "INSERT INTO media_file_probe (episode_file_id, probed_at, probe_size_bytes, probe_mtime, + duration_secs, video_codec, width, height) + VALUES (?1, datetime('now'), ?2, 0, ?3, ?4, 640, 480)", + params![id, size_bytes, duration, codec], + ) + .unwrap(); + } + // Bitrate-descending claim order puts the slow job first: its size + // is set so `size_bytes*8/duration` comfortably exceeds the fast + // job's, matching the real incident's shape (a big, slow file + // claimed ahead of a small, fast one). + conn.execute( + "INSERT INTO transcode_job (episode_file_id, status, is_anime, queued_at) VALUES (?1, 'pending', 1, datetime('now'))", + params![slow_id], + ) + .unwrap(); + conn.execute( + "INSERT INTO transcode_job (episode_file_id, status, is_anime, queued_at) VALUES (?1, 'pending', 0, datetime('now'))", + params![fast_id], + ) + .unwrap(); + + let conn = Arc::new(Mutex::new(conn)); + let mut c = cfg(); + c.skip_below_ceiling_ratio = 0.0; // don't pre-skip the slow job + let stats = run_cycle(conn.clone(), c, None).await.unwrap(); + assert_eq!(stats.attempted, 2); + assert_eq!(stats.failed, 0); + + let conn = conn.lock().await; + let (slow_status, slow_path): (String, String) = conn + .query_row( + "SELECT tj.status, ef.path FROM transcode_job tj JOIN episode_file ef ON ef.id = tj.episode_file_id WHERE tj.episode_file_id = ?1", + params![slow_id], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .unwrap(); + let (fast_status, fast_new_bytes, fast_path): (String, i64, String) = conn + .query_row( + "SELECT tj.status, tj.new_bytes, ef.path FROM transcode_job tj JOIN episode_file ef ON ef.id = tj.episode_file_id WHERE tj.episode_file_id = ?1", + params![fast_id], + |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)), + ) + .unwrap(); + + assert_eq!(slow_status, "done", "the real encode must land on the slow job's own row"); + assert_eq!(slow_path, slow_clip.to_string_lossy(), "must not cross-attribute the fast job's path"); + assert_eq!(fast_status, "done", "the already-AV1 short-circuit must land on the fast job's own row"); + assert_eq!(fast_new_bytes, 0, "AlreadyAv1 records no byte-count change"); + assert_eq!(fast_path, fast_clip.to_string_lossy(), "must not cross-attribute the slow job's path"); + + // Regression coverage for a real gap found in review: `finalize_job` + // must set `upgrade_locked` in the same transaction as the new + // `size_bytes`, and a best-effort re-probe afterward must still + // leave `media_file_probe` correctly reflecting the swapped-in AV1 + // file — not the stale pre-encode codec, and not silently unset + // just because that refresh is no longer allowed to fail the job. + let (upgrade_locked, probed_codec): (i64, Option) = conn + .query_row( + "SELECT ef.upgrade_locked, p.video_codec FROM episode_file ef + LEFT JOIN media_file_probe p ON p.episode_file_id = ef.id + WHERE ef.id = ?1", + params![slow_id], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .unwrap(); + assert_eq!(upgrade_locked, 1, "a real encode must lock the file against the ordinary upgrade cycle"); + assert_eq!(probed_codec.as_deref(), Some("av1"), "the post-swap re-probe must reflect the new AV1 file, not stale pre-encode data"); + + std::fs::remove_dir_all(&dir).unwrap(); + } }