diff --git a/Cargo.lock b/Cargo.lock index 9c4f9d89..d5cfac32 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1894,13 +1894,14 @@ dependencies = [ [[package]] name = "nzb-web" -version = "0.4.22" +version = "0.4.23" dependencies = [ "anyhow", "arc-swap", "async-trait", "axum", "base64 0.23.1", + "bzip2", "chrono", "feed-rs", "flate2", @@ -1933,6 +1934,7 @@ dependencies = [ "utoipa-swagger-ui", "uuid", "yenc-simd", + "zip 8.6.0", ] [[package]] @@ -2815,7 +2817,6 @@ dependencies = [ "chrono", "clap", "crc32fast", - "flate2", "hex", "http", "libc", @@ -2852,7 +2853,6 @@ dependencies = [ "utoipa", "utoipa-swagger-ui", "uuid", - "zip 8.6.0", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index ae4cbe04..419599e3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,13 +80,15 @@ regex = "1" walkdir = "2" tar = "0.4" flate2 = "1" +zip = { version = "8", default-features = false, features = ["deflate"] } +bzip2 = "0.6" tokio-socks = "0.5" notify = "7" feed-rs = "2" unicode-normalization = "0.1" # Shared NZB crates -nzb-web = { version = "0.4.22", path = "crates/nzb-web", features = ["groups-db"] } +nzb-web = { version = "0.4.23", path = "crates/nzb-web", features = ["groups-db"] } nzb-nntp = { version = "0.2.24", path = "crates/nzb-nntp" } nzb-core = { version = "0.2.18", path = "crates/nzb-core", features = ["groups-db"] } nzb-decode = { version = "0.1.5", path = "crates/nzb-decode" } diff --git a/apps/rustnzb/Cargo.toml b/apps/rustnzb/Cargo.toml index a48273db..cc9412a9 100644 --- a/apps/rustnzb/Cargo.toml +++ b/apps/rustnzb/Cargo.toml @@ -47,8 +47,6 @@ hex = { workspace = true } rust-embed = { version = "8", features = ["debug-embed", "interpolate-folder-path"] } mime_guess = "2" libc = "0.2" -zip = { version = "8", default-features = false, features = ["deflate"] } -flate2 = { workspace = true } parking_lot = { workspace = true } tokio-util = { version = "0.7", features = ["rt"] } diff --git a/apps/rustnzb/src/group_handlers.rs b/apps/rustnzb/src/group_handlers.rs index b70653f1..4edac6dc 100644 --- a/apps/rustnzb/src/group_handlers.rs +++ b/apps/rustnzb/src/group_handlers.rs @@ -3,12 +3,33 @@ use std::sync::Arc; use axum::Json; -use axum::extract::{Path, Query, State}; +use axum::extract::{FromRequestParts, Path, Query, State}; +use axum::http::request::Parts; use serde::Deserialize; use nzb_web::error::ApiError; use nzb_web::state::AppState; +/// `Path` extractor whose rejection is a JSON `ApiError` 400 instead of +/// axum's plain-text parser message (which leaks Rust type names such as +/// "Cannot parse `abc` to a `i64`"). +pub struct IdPath(pub T); + +impl FromRequestParts for IdPath +where + S: Send + Sync, + T: serde::de::DeserializeOwned + Send, +{ + type Rejection = ApiError; + + async fn from_request_parts(parts: &mut Parts, state: &S) -> Result { + Path::::from_request_parts(parts, state) + .await + .map(|Path(value)| IdPath(value)) + .map_err(|_| ApiError::bad_request("Invalid id in request path")) + } +} + #[derive(Deserialize, Default)] pub struct GroupListQuery { pub subscribed: Option, @@ -98,7 +119,7 @@ pub async fn h_group_refresh( /// GET /api/groups/{id} pub async fn h_group_get( State(state): State>, - Path(id): Path, + IdPath(id): IdPath, ) -> Result, ApiError> { let group = state .queue_manager @@ -113,7 +134,7 @@ pub async fn h_group_get( /// GET /api/groups/{id}/status pub async fn h_group_status( State(state): State>, - Path(id): Path, + IdPath(id): IdPath, ) -> Result, ApiError> { let qm = &state.queue_manager; let group = qm @@ -140,7 +161,7 @@ pub async fn h_group_status( /// POST /api/groups/{id}/subscribe pub async fn h_group_subscribe( State(state): State>, - Path(id): Path, + IdPath(id): IdPath, ) -> Result, ApiError> { state .queue_manager @@ -152,7 +173,7 @@ pub async fn h_group_subscribe( /// POST /api/groups/{id}/unsubscribe pub async fn h_group_unsubscribe( State(state): State>, - Path(id): Path, + IdPath(id): IdPath, ) -> Result, ApiError> { state .queue_manager @@ -164,7 +185,7 @@ pub async fn h_group_unsubscribe( /// GET /api/groups/{id}/headers pub async fn h_header_list( State(state): State>, - Path(group_id): Path, + IdPath(group_id): IdPath, Query(q): Query, ) -> Result, ApiError> { let limit = q.limit.unwrap_or(50); @@ -186,7 +207,7 @@ pub async fn h_header_list( /// POST /api/groups/{id}/headers/fetch — Background XOVER fetch. pub async fn h_header_fetch( State(state): State>, - Path(group_id): Path, + IdPath(group_id): IdPath, ) -> Result, ApiError> { use nzb_web::nzb_core::nzb_nntp::connection::NntpConnection; @@ -292,7 +313,7 @@ pub async fn h_header_fetch( /// GET /api/groups/{id}/threads pub async fn h_thread_list( State(state): State>, - Path(group_id): Path, + IdPath(group_id): IdPath, Query(q): Query, ) -> Result, ApiError> { let limit = q.limit.unwrap_or(50); @@ -311,7 +332,7 @@ pub async fn h_thread_list( /// GET /api/groups/{gid}/threads/{root_msg_id} pub async fn h_thread_get( State(state): State>, - Path((group_id, root_msg_id)): Path<(i64, String)>, + IdPath((group_id, root_msg_id)): IdPath<(i64, String)>, ) -> Result, ApiError> { let articles = state .queue_manager @@ -326,7 +347,7 @@ pub async fn h_thread_get( /// POST /api/groups/{id}/headers/mark-read pub async fn h_header_mark_read( State(state): State>, - Path(_group_id): Path, + IdPath(_group_id): IdPath, Json(input): Json, ) -> Result, ApiError> { let count = state @@ -339,7 +360,7 @@ pub async fn h_header_mark_read( /// POST /api/groups/{id}/headers/mark-all-read pub async fn h_header_mark_all_read( State(state): State>, - Path(group_id): Path, + IdPath(group_id): IdPath, ) -> Result, ApiError> { let count = state .queue_manager @@ -372,10 +393,12 @@ pub async fn h_article_get( .await .map_err(|e| ApiError::from(anyhow::anyhow!("Connect failed: {e}")))?; - let response = conn - .fetch_article(&message_id) - .await - .map_err(|e| ApiError::from(anyhow::anyhow!("ARTICLE failed: {e}")))?; + let response = conn.fetch_article(&message_id).await.map_err(|e| match e { + nzb_web::nzb_core::nzb_nntp::error::NntpError::ArticleNotFound(_) => { + ApiError::not_found("Article not found") + } + e => ApiError::from(anyhow::anyhow!("ARTICLE failed: {e}")), + })?; let _ = conn.quit().await; let body = response @@ -392,7 +415,7 @@ pub async fn h_article_get( /// POST /api/groups/{id}/headers/download — Download selected as NZB. pub async fn h_header_download( State(state): State>, - Path(group_id): Path, + IdPath(group_id): IdPath, Json(input): Json, ) -> Result, ApiError> { let group = state diff --git a/apps/rustnzb/src/handlers.rs b/apps/rustnzb/src/handlers.rs index 1e2630a2..37f5c888 100644 --- a/apps/rustnzb/src/handlers.rs +++ b/apps/rustnzb/src/handlers.rs @@ -1,11 +1,9 @@ -use std::io::{Cursor, Read as _}; use std::sync::Arc; use axum::Json; use axum::extract::{Multipart, Path, Query, State}; use axum::http::HeaderMap; use axum::response::IntoResponse; -use flate2::read::GzDecoder; use http::StatusCode; use serde::{Deserialize, Serialize}; @@ -21,8 +19,6 @@ static HTTP_CLIENT: std::sync::LazyLock = std::sync::LazyLock:: .expect("Failed to build shared HTTP client") }); -const MAX_NZB_DECOMPRESSED_BYTES: u64 = 100 * 1024 * 1024; - #[cfg(feature = "webdav")] use nzb_web::nzb_core::config::DavConfig; use nzb_web::nzb_core::config::{ @@ -38,6 +34,7 @@ use nzb_web::fetch_guard::{ read_response_bytes_limited, validate_fetch_url_with, }; use nzb_web::log_buffer::LogEntry; +use nzb_web::nzb_archive::extract_nzbs; use nzb_web::state::AppState; use crate::admissions::{IdempotencyKey, payload_digest}; @@ -289,81 +286,6 @@ pub async fn h_queue_list( })) } -/// Extract NZB files from an uploaded file. If it's an archive (zip, gz), -/// returns all `.nzb` entries found inside. Otherwise returns the file as-is. -fn extract_nzbs(file_name: &str, data: &[u8]) -> Result)>, anyhow::Error> { - let lower = file_name.to_lowercase(); - - // .nzb.gz or .gz containing an nzb - if lower.ends_with(".gz") { - let mut decoder = GzDecoder::new(data); - let mut decompressed = Vec::new(); - decoder - .by_ref() - .take(MAX_NZB_DECOMPRESSED_BYTES + 1) - .read_to_end(&mut decompressed) - .map_err(|e| anyhow::anyhow!("Failed to decompress gzip: {e}"))?; - if decompressed.len() as u64 > MAX_NZB_DECOMPRESSED_BYTES { - anyhow::bail!( - "Decompressed NZB exceeds the {} MB limit", - MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 - ); - } - let inner_name = file_name - .strip_suffix(".gz") - .or_else(|| file_name.strip_suffix(".GZ")) - .unwrap_or(file_name); - return Ok(vec![(inner_name.to_string(), decompressed)]); - } - - // .zip archive — extract all .nzb files inside - if lower.ends_with(".zip") { - let cursor = Cursor::new(data); - let mut archive = zip::ZipArchive::new(cursor) - .map_err(|e| anyhow::anyhow!("Failed to read zip archive: {e}"))?; - let mut nzbs = Vec::new(); - let mut total_uncompressed = 0u64; - for i in 0..archive.len() { - let mut entry = archive - .by_index(i) - .map_err(|e| anyhow::anyhow!("Zip entry error: {e}"))?; - let entry_name = entry.name().to_string(); - if entry_name.to_lowercase().ends_with(".nzb") { - total_uncompressed = total_uncompressed - .checked_add(entry.size()) - .ok_or_else(|| anyhow::anyhow!("Zip archive size overflow"))?; - if total_uncompressed > MAX_NZB_DECOMPRESSED_BYTES { - anyhow::bail!( - "Decompressed NZB exceeds the {} MB limit", - MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 - ); - } - - let mut buf = Vec::new(); - entry - .by_ref() - .take(MAX_NZB_DECOMPRESSED_BYTES + 1) - .read_to_end(&mut buf) - .map_err(|e| anyhow::anyhow!("Failed to read zip entry '{entry_name}': {e}"))?; - if buf.len() as u64 > MAX_NZB_DECOMPRESSED_BYTES { - anyhow::bail!( - "Decompressed NZB exceeds the {} MB limit", - MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 - ); - } - nzbs.push((entry_name, buf)); - } - } - if nzbs.is_empty() { - anyhow::bail!("No .nzb files found in zip archive '{file_name}'"); - } - return Ok(nzbs); - } - - // Plain .nzb or unrecognized — pass through as-is - Ok(vec![(file_name.to_string(), data.to_vec())]) -} - /// Enqueue a single NZB from raw bytes, applying category/priority from query params. async fn next_uploaded_file( multipart: &mut Multipart, @@ -435,7 +357,7 @@ fn enqueue_nzb( } /// POST /api/queue/add -- Add NZB file(s) to the queue. -/// Accepts `.nzb` files directly, or `.zip`/`.gz` archives containing `.nzb` files. +/// Accepts `.nzb` files directly, or `.zip`/`.gz`/`.bz2` archives containing `.nzb` files. /// Multiple files can be uploaded in a single multipart request. pub async fn h_queue_add( State(state): State>, @@ -467,7 +389,7 @@ pub async fn h_queue_add( )?); } else { while let Some((file_name, data)) = next_uploaded_file(&mut multipart).await? { - // Extract NZBs (handles zip/gz archives or plain .nzb) + // Extract NZBs (handles zip/gz/bz2 archives or plain .nzb) for (nzb_name, nzb_data) in extract_nzbs(&file_name, &data).map_err(ApiError::from)? { nzo_ids.push(enqueue_nzb(&state, &q, &nzb_name, nzb_data, None)?); } @@ -491,7 +413,11 @@ pub async fn h_queue_set_priority( ) -> Result, ApiError> { let priority = match body.priority { 0..=3 => priority_from_i32(body.priority), - _ => return Err(ApiError::from(anyhow::anyhow!("Invalid priority value"))), + _ => { + return Err(ApiError::bad_request( + "Invalid priority value (expected 0-3)", + )); + } }; state .queue_manager @@ -743,7 +669,7 @@ pub async fn h_history_retry( .queue_manager .history_get(&id) .map_err(ApiError::from)? - .ok_or_else(|| ApiError::from(anyhow::anyhow!("History entry not found")))?; + .ok_or(ApiError::not_found("History entry not found"))?; // Get the raw NZB data let nzb_data = state @@ -894,13 +820,14 @@ pub async fn h_sab_api_key_get( pub async fn h_sab_api_key_rotate( State(state): State>, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); // UUID v4 is backed by the operating system's cryptographically secure // random source. A simple-form UUID supplies a 32-character token without // separators, matching the common SABnzbd API-key format. let api_key = uuid::Uuid::new_v4().simple().to_string(); - config.general.api_key = Some(api_key.clone()); - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + config.general.api_key = Some(api_key.clone()); + Ok::<_, ApiError>(()) + })?; tracing::info!("SABnzbd API key rotated"); Ok(Json(SabApiKeyResponse { @@ -943,12 +870,14 @@ pub async fn h_server_add( server.password = None; } - let mut config = (*state.config()).clone(); - config.servers.push(server); + state.update_config_with(|config| { + config.servers.push(server); + Ok::<_, ApiError>(()) + })?; + // Apply the latest committed list, so concurrent writers converge. state - .update_config(config.clone()) - .map_err(ApiError::from)?; - state.queue_manager.update_servers(config.servers); + .queue_manager + .update_servers(state.config().servers.clone()); Ok((StatusCode::OK, Json(SimpleResponse { status: true }))) } @@ -980,25 +909,26 @@ pub async fn h_server_update( Path(id): Path, Json(patch): Json, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - - let idx = config - .servers - .iter() - .position(|s| s.id == id) - .ok_or_else(|| ApiError::from(anyhow::anyhow!("Server not found: {id}")))?; - - let mut server = merge_server_update(&config.servers[idx], patch)?; - keep_unchanged_password(&mut server, &config.servers[idx]); - if server.id.is_empty() { - server.id = id; - } - sanitize_server_config(&mut server); - config.servers[idx] = server; + state.update_config_with(|config| { + let idx = config + .servers + .iter() + .position(|s| s.id == id) + .ok_or_else(|| ApiError::from(anyhow::anyhow!("Server not found: {id}")))?; + + let mut server = merge_server_update(&config.servers[idx], patch)?; + keep_unchanged_password(&mut server, &config.servers[idx]); + if server.id.is_empty() { + server.id = id.clone(); + } + sanitize_server_config(&mut server); + config.servers[idx] = server; + Ok::<_, ApiError>(()) + })?; + // Apply the latest committed list, so concurrent writers converge. state - .update_config(config.clone()) - .map_err(ApiError::from)?; - state.queue_manager.update_servers(config.servers); + .queue_manager + .update_servers(state.config().servers.clone()); Ok(Json(SimpleResponse { status: true })) } @@ -1024,18 +954,20 @@ pub async fn h_server_delete( State(state): State>, Path(id): Path, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - let before = config.servers.len(); - config.servers.retain(|s| s.id != id); + state.update_config_with(|config| { + let before = config.servers.len(); + config.servers.retain(|s| s.id != id); - if config.servers.len() == before { - return Err(ApiError::from(anyhow::anyhow!("Server not found: {id}"))); - } + if config.servers.len() == before { + return Err(ApiError::from(anyhow::anyhow!("Server not found: {id}"))); + } + Ok::<_, ApiError>(()) + })?; + // Apply the latest committed list, so concurrent writers converge. state - .update_config(config.clone()) - .map_err(ApiError::from)?; - state.queue_manager.update_servers(config.servers); + .queue_manager + .update_servers(state.config().servers.clone()); Ok(Json(SimpleResponse { status: true })) } @@ -1162,6 +1094,14 @@ pub async fn h_history_logs( State(state): State>, Path(id): Path, ) -> Result, ApiError> { + if state + .queue_manager + .history_get(&id) + .map_err(ApiError::from)? + .is_none() + { + return Err(ApiError::not_found("History entry not found")); + } let logs_json = state .queue_manager .history_get_logs(&id) @@ -1193,18 +1133,19 @@ pub async fn h_category_add( State(state): State>, Json(cat): Json, ) -> Result { - let mut config = (*state.config()).clone(); - if config.categories.iter().any(|c| c.name == cat.name) { - return Err(ApiError::from(anyhow::anyhow!( - "Category '{}' already exists", - cat.name - ))); - } - config.categories.push(cat); - state - .queue_manager - .set_categories(config.categories.clone()); - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + if config.categories.iter().any(|c| c.name == cat.name) { + return Err(ApiError::from(anyhow::anyhow!( + "Category '{}' already exists", + cat.name + ))); + } + config.categories.push(cat); + state + .queue_manager + .set_categories(config.categories.clone()); + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1214,17 +1155,18 @@ pub async fn h_category_update( Path(name): Path, Json(cat): Json, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - let idx = config - .categories - .iter() - .position(|c| c.name == name) - .ok_or_else(|| ApiError::from(anyhow::anyhow!("Category not found")))?; - config.categories[idx] = cat; - state - .queue_manager - .set_categories(config.categories.clone()); - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + let idx = config + .categories + .iter() + .position(|c| c.name == name) + .ok_or_else(|| ApiError::from(anyhow::anyhow!("Category not found")))?; + config.categories[idx] = cat; + state + .queue_manager + .set_categories(config.categories.clone()); + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1233,16 +1175,17 @@ pub async fn h_category_delete( State(state): State>, Path(name): Path, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - let initial_len = config.categories.len(); - config.categories.retain(|c| c.name != name); - if config.categories.len() == initial_len { - return Err(ApiError::from(anyhow::anyhow!("Category not found"))); - } - state - .queue_manager - .set_categories(config.categories.clone()); - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + let initial_len = config.categories.len(); + config.categories.retain(|c| c.name != name); + if config.categories.len() == initial_len { + return Err(ApiError::from(anyhow::anyhow!("Category not found"))); + } + state + .queue_manager + .set_categories(config.categories.clone()); + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1254,9 +1197,10 @@ pub async fn h_history_retention_set( // 0 means "keep all" (GH #136); persist the normalized value so GET // reports what is actually enforced. let retention = normalize_history_retention(body.retention); - let mut config = (*state.config()).clone(); - config.general.history_retention = retention; - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + config.general.history_retention = retention; + Ok::<_, ApiError>(()) + })?; state.queue_manager.set_history_retention(retention); Ok(Json(SimpleResponse { status: true })) } @@ -1276,9 +1220,10 @@ pub async fn h_max_active_downloads_set( State(state): State>, Json(body): Json, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - config.general.max_active_downloads = body.max_active_downloads; - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + config.general.max_active_downloads = body.max_active_downloads; + Ok::<_, ApiError>(()) + })?; state .queue_manager .set_max_active_downloads(body.max_active_downloads); @@ -1325,9 +1270,10 @@ pub async fn h_set_speed_limit( ) -> Result, ApiError> { state.queue_manager.set_speed_limit(body.speed_limit_bps); // Also update config and persist - let mut config = (*state.config()).clone(); - config.general.speed_limit_bps = body.speed_limit_bps; - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + config.general.speed_limit_bps = body.speed_limit_bps; + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1357,13 +1303,14 @@ pub async fn h_disk_guards_set( State(state): State>, Json(body): Json, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - config.general.min_free_space_bytes = body.min_free_space_bytes; - config.general.abort_hopeless = body.abort_hopeless; - state - .queue_manager - .set_min_free_space(body.min_free_space_bytes); - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + config.general.min_free_space_bytes = body.min_free_space_bytes; + config.general.abort_hopeless = body.abort_hopeless; + state + .queue_manager + .set_min_free_space(body.min_free_space_bytes); + Ok::<_, ApiError>(()) + })?; Ok(Json(SimpleResponse { status: true })) } @@ -1385,15 +1332,16 @@ pub async fn h_rss_feed_add( Json(feed): Json, ) -> Result { validate_feed_url(&state, &feed.url).await?; - let mut config = (*state.config()).clone(); - if config.rss_feeds.iter().any(|f| f.name == feed.name) { - return Err(ApiError::from(anyhow::anyhow!( - "Feed '{}' already exists", - feed.name - ))); - } - config.rss_feeds.push(feed); - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + if config.rss_feeds.iter().any(|f| f.name == feed.name) { + return Err(ApiError::from(anyhow::anyhow!( + "Feed '{}' already exists", + feed.name + ))); + } + config.rss_feeds.push(feed); + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1404,14 +1352,15 @@ pub async fn h_rss_feed_update( Json(feed): Json, ) -> Result, ApiError> { validate_feed_url(&state, &feed.url).await?; - let mut config = (*state.config()).clone(); - let idx = config - .rss_feeds - .iter() - .position(|f| f.name == name) - .ok_or_else(|| ApiError::from(anyhow::anyhow!("Feed not found")))?; - config.rss_feeds[idx] = feed; - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + let idx = config + .rss_feeds + .iter() + .position(|f| f.name == name) + .ok_or_else(|| ApiError::from(anyhow::anyhow!("Feed not found")))?; + config.rss_feeds[idx] = feed; + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1420,13 +1369,14 @@ pub async fn h_rss_feed_delete( State(state): State>, Path(name): Path, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - let len = config.rss_feeds.len(); - config.rss_feeds.retain(|f| f.name != name); - if config.rss_feeds.len() == len { - return Err(ApiError::from(anyhow::anyhow!("Feed not found"))); - } - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + let len = config.rss_feeds.len(); + config.rss_feeds.retain(|f| f.name != name); + if config.rss_feeds.len() == len { + return Err(ApiError::from(anyhow::anyhow!("Feed not found"))); + } + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({"status": true}))) } @@ -1549,15 +1499,18 @@ const MAX_RSS_REGEX_LEN: usize = 512; /// pattern cannot consume unbounded memory/CPU at compile time. fn compile_rss_regex(pattern: &str) -> Result { if pattern.len() > MAX_RSS_REGEX_LEN { - return Err(ApiError::from(anyhow::anyhow!( - "Regex too long ({} bytes, max {MAX_RSS_REGEX_LEN})", - pattern.len() + return Err(ApiError::from(( + StatusCode::BAD_REQUEST, + format!( + "Regex too long ({} bytes, max {MAX_RSS_REGEX_LEN})", + pattern.len() + ), ))); } regex::RegexBuilder::new(pattern) .size_limit(1 << 20) // 1 MiB compiled-program cap .build() - .map_err(|e| ApiError::from(anyhow::anyhow!("Invalid regex: {e}"))) + .map_err(|e| ApiError::from((StatusCode::BAD_REQUEST, format!("Invalid regex: {e}")))) } /// GET /api/rss/rules -- List RSS download rules. @@ -1571,11 +1524,43 @@ pub async fn h_rss_rules_list( Ok(Json(rules)) } +/// Names in `feed_names` that match no configured RSS feed. Such a rule is +/// still saved (the feed may be added later) but can never match yet. +fn unknown_rule_feeds(state: &AppState, feed_names: &[String]) -> Vec { + let config = state.config(); + feed_names + .iter() + .filter(|name| !config.rss_feeds.iter().any(|f| &f.name == *name)) + .cloned() + .collect() +} + +/// `{"status": true}`, plus a `warnings` array when a rule references feeds +/// that are not configured. +fn rss_rule_saved_response(state: &AppState, rule: &RssRule) -> serde_json::Value { + let unknown = unknown_rule_feeds(state, &rule.feed_names); + if unknown.is_empty() { + return serde_json::json!({ "status": true }); + } + tracing::warn!( + rule = %rule.name, + feeds = ?unknown, + "RSS rule references feeds that are not configured" + ); + let warnings: Vec = unknown + .iter() + .map(|name| { + format!("feed '{name}' is not configured; the rule will not match until it is added") + }) + .collect(); + serde_json::json!({ "status": true, "warnings": warnings }) +} + /// POST /api/rss/rules -- Add an RSS download rule. pub async fn h_rss_rule_add( State(state): State>, Json(body): Json, -) -> Result, ApiError> { +) -> Result, ApiError> { // Validate and bound the user-supplied regex. compile_rss_regex(&body.match_regex)?; @@ -1592,18 +1577,29 @@ pub async fn h_rss_rule_add( .queue_manager .rss_rule_insert(&rule) .map_err(ApiError::from)?; - Ok(Json(SimpleResponse { status: true })) + Ok(Json(rss_rule_saved_response(&state, &rule))) } -/// PUT /api/rss/rules/{id} -- Update an RSS download rule. +/// PUT /api/rss/rules/{id} -- Update an RSS download rule. 404 if the rule +/// does not exist. pub async fn h_rss_rule_update( State(state): State>, Path(id): Path, Json(body): Json, -) -> Result, ApiError> { +) -> Result, ApiError> { // Validate and bound the user-supplied regex. compile_rss_regex(&body.match_regex)?; + let exists = state + .queue_manager + .rss_rule_list() + .map_err(ApiError::from)? + .iter() + .any(|rule| rule.id == id); + if !exists { + return Err(ApiError::not_found("RSS rule not found")); + } + let rule = RssRule { id, name: body.name, @@ -1617,7 +1613,7 @@ pub async fn h_rss_rule_update( .queue_manager .rss_rule_update(&rule) .map_err(ApiError::from)?; - Ok(Json(SimpleResponse { status: true })) + Ok(Json(rss_rule_saved_response(&state, &rule))) } /// DELETE /api/rss/rules/{id} -- Delete an RSS download rule. @@ -1663,99 +1659,99 @@ pub async fn h_general_update( State(state): State>, Json(body): Json, ) -> Result, ApiError> { - let mut config = (*state.config()).clone(); - - if let Some(dir) = body.incomplete_dir { - config.general.incomplete_dir = dir.into(); - } - if let Some(dir) = body.complete_dir { - config.general.complete_dir = dir.into(); - } - if let Some(dir) = body.data_dir { - config.general.data_dir = dir.into(); - } - // watch_dir: empty string means unset - if let Some(dir) = body.watch_dir { - config.general.watch_dir = if dir.is_empty() { - None - } else { - Some(dir.into()) - }; - } - if let Some(cs) = body.cache_size { - config.general.cache_size = cs; - } - if let Some(mad) = body.max_active_downloads { - state.queue_manager.set_max_active_downloads(mad); - config.general.max_active_downloads = mad; - } - // Resource pools cannot safely discard live permits. Persist updated - // stage limits now and apply them on the next process start. - if let Some(max) = body.max_post_processing_jobs { - config.general.max_post_processing_jobs = max.max(1); - } - if let Some(max) = body.max_repair_workers { - config.general.max_repair_workers = max.max(1); - } - if let Some(max) = body.max_extract_workers { - config.general.max_extract_workers = max.max(1); - } - if let Some(ret) = body.history_retention { - let ret = normalize_history_retention(ret); - state.queue_manager.set_history_retention(ret); - config.general.history_retention = ret; - } - if let Some(rss_limit) = body.rss_history_limit { - config.general.rss_history_limit = rss_limit; - // Prune RSS items if a limit is set - if let Some(limit) = rss_limit { - let _ = state.queue_manager.rss_items_prune(limit); + state.update_config_with(|config| { + if let Some(dir) = body.incomplete_dir { + config.general.incomplete_dir = dir.into(); + } + if let Some(dir) = body.complete_dir { + config.general.complete_dir = dir.into(); + } + if let Some(dir) = body.data_dir { + config.general.data_dir = dir.into(); + } + // watch_dir: empty string means unset + if let Some(dir) = body.watch_dir { + config.general.watch_dir = if dir.is_empty() { + None + } else { + Some(dir.into()) + }; + } + if let Some(cs) = body.cache_size { + config.general.cache_size = cs; + } + if let Some(mad) = body.max_active_downloads { + state.queue_manager.set_max_active_downloads(mad); + config.general.max_active_downloads = mad; + } + // Resource pools cannot safely discard live permits. Persist updated + // stage limits now and apply them on the next process start. + if let Some(max) = body.max_post_processing_jobs { + config.general.max_post_processing_jobs = max.max(1); + } + if let Some(max) = body.max_repair_workers { + config.general.max_repair_workers = max.max(1); + } + if let Some(max) = body.max_extract_workers { + config.general.max_extract_workers = max.max(1); + } + if let Some(ret) = body.history_retention { + let ret = normalize_history_retention(ret); + state.queue_manager.set_history_retention(ret); + config.general.history_retention = ret; + } + if let Some(rss_limit) = body.rss_history_limit { + config.general.rss_history_limit = rss_limit; + // Prune RSS items if a limit is set + if let Some(limit) = rss_limit { + let _ = state.queue_manager.rss_items_prune(limit); + } + } + if let Some(enabled) = body.auto_sort_remaining_pct { + state.queue_manager.set_auto_sort_remaining_pct(enabled); + config.general.auto_sort_remaining_pct = enabled; + } + if let Some(days) = body.rss_downloaded_item_expiry_days { + config.general.rss_downloaded_item_expiry_days = days; + } + if let Some(directory) = body.scripts_dir { + config.general.scripts_dir = if directory.is_empty() { + None + } else { + Some(directory.into()) + }; + } + if let Some(script) = body.script_success { + config.general.script_success = if script.is_empty() { + None + } else { + Some(script.into()) + }; + } + if let Some(script) = body.script_failure { + config.general.script_failure = if script.is_empty() { + None + } else { + Some(script.into()) + }; + } + if let Some(timeout) = body.script_timeout_secs { + config.general.script_timeout_secs = timeout.max(1); + } + if let Some(max_output) = body.script_max_output_bytes { + config.general.script_max_output_bytes = max_output; } - } - if let Some(enabled) = body.auto_sort_remaining_pct { - state.queue_manager.set_auto_sort_remaining_pct(enabled); - config.general.auto_sort_remaining_pct = enabled; - } - if let Some(days) = body.rss_downloaded_item_expiry_days { - config.general.rss_downloaded_item_expiry_days = days; - } - if let Some(directory) = body.scripts_dir { - config.general.scripts_dir = if directory.is_empty() { - None - } else { - Some(directory.into()) - }; - } - if let Some(script) = body.script_success { - config.general.script_success = if script.is_empty() { - None - } else { - Some(script.into()) - }; - } - if let Some(script) = body.script_failure { - config.general.script_failure = if script.is_empty() { - None - } else { - Some(script.into()) - }; - } - if let Some(timeout) = body.script_timeout_secs { - config.general.script_timeout_secs = timeout.max(1); - } - if let Some(max_output) = body.script_max_output_bytes { - config.general.script_max_output_bytes = max_output; - } - state.queue_manager.set_postproc_scripts( - config.general.scripts_dir.clone(), - config.general.script_success.clone(), - config.general.script_failure.clone(), - config.general.script_timeout_secs, - config.general.script_max_output_bytes, - ); + state.queue_manager.set_postproc_scripts( + config.general.scripts_dir.clone(), + config.general.script_success.clone(), + config.general.script_failure.clone(), + config.general.script_timeout_secs, + config.general.script_max_output_bytes, + ); - state.update_config(config).map_err(ApiError::from)?; + Ok::<_, ApiError>(()) + })?; Ok(Json(SimpleResponse { status: true })) } @@ -2179,42 +2175,42 @@ pub async fn h_setup_apply( )); } - let mut config = (*state.config()).clone(); - - // Convert imported servers → ServerConfig with fresh UUIDs - config.servers = preview - .servers - .iter() - .map(|s| s.to_server_config()) - .collect(); + let config = state + .update_config_with(|config| { + // Convert imported servers → ServerConfig with fresh UUIDs + config.servers = preview + .servers + .iter() + .map(|s| s.to_server_config()) + .collect(); - // Replace categories - if !preview.categories.is_empty() { - config.categories = preview.categories; - } + // Replace categories + if !preview.categories.is_empty() { + config.categories = preview.categories; + } - // Apply general settings - if let Some(ref key) = preview.general.api_key { - config.general.api_key = Some(key.clone()); - } - if let Some(ref dir) = preview.general.complete_dir { - config.general.complete_dir = std::path::PathBuf::from(dir); - } - if let Some(ref dir) = preview.general.incomplete_dir { - config.general.incomplete_dir = std::path::PathBuf::from(dir); - } - if preview.general.speed_limit_bps > 0 { - config.general.speed_limit_bps = preview.general.speed_limit_bps; - } + // Apply general settings + if let Some(ref key) = preview.general.api_key { + config.general.api_key = Some(key.clone()); + } + if let Some(ref dir) = preview.general.complete_dir { + config.general.complete_dir = std::path::PathBuf::from(dir); + } + if let Some(ref dir) = preview.general.incomplete_dir { + config.general.incomplete_dir = std::path::PathBuf::from(dir); + } + if preview.general.speed_limit_bps > 0 { + config.general.speed_limit_bps = preview.general.speed_limit_bps; + } - // Apply RSS feeds - if !preview.rss_feeds.is_empty() { - config.rss_feeds = preview.rss_feeds; - } + // Apply RSS feeds + if !preview.rss_feeds.is_empty() { + config.rss_feeds = preview.rss_feeds; + } - // Persist to disk + update in-memory config - state - .update_config(config.clone()) + // Persist to disk + update in-memory config + Ok::<_, ApiError>(config.clone()) + }) .map_err(|e| ApiError::from(anyhow::anyhow!("Failed to save config: {e}")))?; // Update runtime state @@ -2378,36 +2374,18 @@ pub async fn h_dav_config_set( if body.auto_send_all { body.category_rules.clear(); } - let mut config = (*state.config()).clone(); - config.dav = body; - state.update_config(config).map_err(ApiError::from)?; + state.update_config_with(|config| { + config.dav = body; + Ok::<_, ApiError>(()) + })?; Ok(Json(serde_json::json!({ "status": true }))) } #[cfg(test)] mod tests { - use super::{ - MAX_NZB_DECOMPRESSED_BYTES, MAX_RSS_REGEX_LEN, compile_rss_regex, extract_nzbs, - sanitize_server_config, - }; - use std::io::Write; + use super::{MAX_RSS_REGEX_LEN, compile_rss_regex, sanitize_server_config}; use nzb_web::nzb_core::config::ServerConfig; - use zip::CompressionMethod; - use zip::write::SimpleFileOptions; - - fn build_zip(entries: &[(&str, &[u8])]) -> Vec { - let cursor = std::io::Cursor::new(Vec::new()); - let mut writer = zip::ZipWriter::new(cursor); - let options = SimpleFileOptions::default().compression_method(CompressionMethod::Deflated); - - for (name, contents) in entries { - writer.start_file(name, options).unwrap(); - writer.write_all(contents).unwrap(); - } - - writer.finish().unwrap().into_inner() - } #[test] fn compile_rss_regex_accepts_normal_pattern() { @@ -2427,23 +2405,6 @@ mod tests { assert!(err.to_string().contains("too long")); } - #[test] - fn extract_nzbs_rejects_zip_bombs() { - let oversized = vec![b'x'; (MAX_NZB_DECOMPRESSED_BYTES + 1) as usize]; - let zip = build_zip(&[("oversized.nzb", oversized.as_slice())]); - let err = extract_nzbs("oversized.zip", &zip).unwrap_err(); - assert!(err.to_string().contains("100 MB limit")); - } - - #[test] - fn extract_nzbs_accepts_small_zip_nzb() { - let zip = build_zip(&[("sample.nzb", br#""#)]); - let nzbs = extract_nzbs("sample.zip", &zip).unwrap(); - assert_eq!(nzbs.len(), 1); - assert_eq!(nzbs[0].0, "sample.nzb"); - assert_eq!(nzbs[0].1, br#""#); - } - #[test] fn sanitize_server_config_trims_string_fields() { let mut server = ServerConfig::new("srv-1", " news.example.com \n"); diff --git a/apps/rustnzb/tests/api_contracts.rs b/apps/rustnzb/tests/api_contracts.rs index 9b9b647c..c576691b 100644 --- a/apps/rustnzb/tests/api_contracts.rs +++ b/apps/rustnzb/tests/api_contracts.rs @@ -530,6 +530,128 @@ async fn logout_revokes_the_access_token() { ); } +/// Send a request and return (status, parsed JSON body or Null). +async fn call(request: reqwest::RequestBuilder) -> (u16, serde_json::Value) { + let response = request.send().await.unwrap(); + let status = response.status().as_u16(); + let text = response.text().await.unwrap(); + ( + status, + serde_json::from_str(&text).unwrap_or(serde_json::Value::Null), + ) +} + +#[tokio::test] +async fn unknown_ids_return_404_and_invalid_input_returns_400() { + let app = start_app(true).await; + let client = reqwest::Client::new(); + let (access, _) = login(&app, &client).await; + let base = &app.base_url; + + // BUG-51: retry of an unknown history entry. + let (status, body) = call( + client + .post(format!("{base}/api/history/no-such-id/retry")) + .bearer_auth(&access), + ) + .await; + assert_eq!(status, 404, "{body}"); + assert_eq!(body["error_kind"], "not_found"); + + // BUG-52: out-of-range priority is a 400, unknown job a 404. + let (status, body) = call( + client + .put(format!("{base}/api/queue/no-such-id/priority")) + .bearer_auth(&access) + .json(&serde_json::json!({"priority": 999})), + ) + .await; + assert_eq!(status, 400, "{body}"); + assert_eq!(body["error_kind"], "bad_request"); + let (status, body) = call( + client + .put(format!("{base}/api/queue/no-such-id/priority")) + .bearer_auth(&access) + .json(&serde_json::json!({"priority": 2})), + ) + .await; + assert_eq!(status, 404, "{body}"); + assert_eq!(body["error_kind"], "job_not_found"); + + // BUG-55: move of an unknown job, logs of an unknown history entry. + let (status, body) = call( + client + .post(format!("{base}/api/queue/no-such-id/move")) + .bearer_auth(&access) + .json(&serde_json::json!({"position": 0})), + ) + .await; + assert_eq!(status, 404, "{body}"); + let (status, body) = call( + client + .get(format!("{base}/api/history/no-such-id/logs")) + .bearer_auth(&access), + ) + .await; + assert_eq!(status, 404, "{body}"); + + // BUG-55: non-numeric group id is a JSON 400 without parser internals. + let response = client + .get(format!("{base}/api/groups/not-a-number")) + .bearer_auth(&access) + .send() + .await + .unwrap(); + assert_eq!(response.status().as_u16(), 400); + let text = response.text().await.unwrap(); + assert!(!text.contains("i64"), "{text}"); + let body: serde_json::Value = serde_json::from_str(&text).expect("JSON error body"); + assert_eq!(body["error_kind"], "bad_request"); + + // BUG-56: invalid regex is a 400; updating an unknown rule is a 404; + // deleting an unknown rule stays idempotent. + let rule = |regex: &str| serde_json::json!({"name": "r", "feed_names": ["nope"], "match_regex": regex}); + let (status, body) = call( + client + .post(format!("{base}/api/rss/rules")) + .bearer_auth(&access) + .json(&rule("(unclosed")), + ) + .await; + assert_eq!(status, 400, "{body}"); + let (status, body) = call( + client + .put(format!("{base}/api/rss/rules/no-such-rule")) + .bearer_auth(&access) + .json(&rule(".*")), + ) + .await; + assert_eq!(status, 404, "{body}"); + assert!(app.state.queue_manager.rss_rule_list().unwrap().is_empty()); + let (status, _) = call( + client + .delete(format!("{base}/api/rss/rules/no-such-rule")) + .bearer_auth(&access), + ) + .await; + assert_eq!(status, 200); + + // A rule whose feed does not exist is accepted with a warning. + let (status, body) = call( + client + .post(format!("{base}/api/rss/rules")) + .bearer_auth(&access) + .json(&rule(".*")), + ) + .await; + assert_eq!(status, 200, "{body}"); + assert_eq!(body["status"], true); + assert!( + body["warnings"][0].as_str().unwrap().contains("nope"), + "{body}" + ); +} + #[tokio::test] async fn server_update_keeps_password_unless_a_new_one_is_sent() { let app = start_app(false).await; @@ -669,3 +791,96 @@ async fn server_add_and_update_accept_partial_bodies() { .unwrap(); assert_eq!(bad.status(), reqwest::StatusCode::BAD_REQUEST); } + +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn concurrent_config_writes_do_not_lose_updates() { + let app = start_app(true).await; + let client = reqwest::Client::new(); + let tokens = client + .post(format!("{}/api/auth/login", app.base_url)) + .json(&serde_json::json!({"username":"admin","password":"password"})) + .send() + .await + .unwrap() + .json::() + .await + .unwrap(); + let access = tokens["access_token"].as_str().unwrap().to_string(); + + const WRITERS: usize = 24; + let mut tasks = Vec::new(); + for i in 0..WRITERS { + let client = client.clone(); + let access = access.clone(); + let base_url = app.base_url.clone(); + tasks.push(tokio::spawn(async move { + let (path, body) = if i % 2 == 0 { + ( + "categories", + serde_json::json!({"name": format!("cat-{i}"), "post_processing": 3}), + ) + } else { + ( + "rss-feeds", + serde_json::json!({"name": format!("feed-{i}"), "url": "https://feed.invalid/rss"}), + ) + }; + client + .post(format!("{base_url}/api/config/{path}")) + .bearer_auth(&access) + .json(&body) + .send() + .await + .unwrap() + .status() + })); + } + for task in tasks { + assert_eq!(task.await.unwrap(), reqwest::StatusCode::OK); + } + + for config in [ + (*app.state.config()).clone(), + AppConfig::load(&app.config_path).unwrap(), + ] { + for i in 0..WRITERS { + if i % 2 == 0 { + let name = format!("cat-{i}"); + assert!( + config.categories.iter().any(|c| c.name == name), + "lost category {name}" + ); + } else { + let name = format!("feed-{i}"); + assert!( + config.rss_feeds.iter().any(|f| f.name == name), + "lost feed {name}" + ); + } + } + } +} + +#[tokio::test] +async fn missing_article_returns_404() { + use nzb_nntp::testutil::{MockConfig, MockNntpServer, test_config}; + + let server = MockNntpServer::start(MockConfig::default()).await; + let app = start_app(true).await; + app.state + .queue_manager + .update_servers(vec![test_config(server.port())]); + let client = reqwest::Client::new(); + let (access, _) = login(&app, &client).await; + + let (status, body) = call( + client + .get(format!( + "{}/api/articles/missing@example.test", + app.base_url + )) + .bearer_auth(&access), + ) + .await; + assert_eq!(status, 404, "{body}"); +} diff --git a/apps/rustnzb/tests/group_mark_read.rs b/apps/rustnzb/tests/group_mark_read.rs index 42242da2..9ba17bb7 100644 --- a/apps/rustnzb/tests/group_mark_read.rs +++ b/apps/rustnzb/tests/group_mark_read.rs @@ -2,14 +2,14 @@ use std::sync::Arc; use arc_swap::ArcSwap; use axum::Json; -use axum::extract::{Path, State}; +use axum::extract::State; use nzb_web::auth::{CredentialStore, TokenStore}; use nzb_web::nzb_core::config::AppConfig; use nzb_web::nzb_core::db::Database; use nzb_web::nzb_core::models::MarkReadInput; use nzb_web::nzb_core::nzb_nntp::XoverEntry; use nzb_web::{AppState, QueueManager}; -use rustnzb::group_handlers::h_header_mark_read; +use rustnzb::group_handlers::{IdPath, h_header_mark_read}; use tempfile::TempDir; fn build_test_state() -> (Arc, TempDir) { @@ -119,7 +119,7 @@ async fn mark_read_marks_requested_headers_in_one_handler_call() { let Json(payload) = h_header_mark_read( State(state.clone()), - Path(group_id), + IdPath(group_id), Json(MarkReadInput { header_ids: header_ids[..2].to_vec(), }), diff --git a/crates/nzb-core/src/db.rs b/crates/nzb-core/src/db.rs index 0a93ee10..70c1e76f 100644 --- a/crates/nzb-core/src/db.rs +++ b/crates/nzb-core/src/db.rs @@ -384,6 +384,27 @@ impl Database { )?; } + if version < 13 { + info!("Applying database migration v13: per-job post-processing override"); + // Partial schemas (as built by migration tests) may lack the + // queue table; only add the column where the table exists. + let has_queue: i64 = self.conn.query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'queue'", + [], + |row| row.get(0), + )?; + if has_queue > 0 { + self.conn + .execute_batch("ALTER TABLE queue ADD COLUMN pp_override INTEGER;")?; + } + self.conn.execute_batch( + " + DELETE FROM schema_version; + INSERT INTO schema_version (version) VALUES (13); + ", + )?; + } + Ok(()) } @@ -449,9 +470,9 @@ impl Database { "INSERT INTO queue (id, name, category, status, priority, total_bytes, downloaded_bytes, file_count, files_completed, article_count, articles_downloaded, articles_failed, added_at, work_dir, output_dir, password, - nzb_raw) + nzb_raw, pp_override) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, - ?16, ?17)", + ?16, ?17, ?18)", params![ job.id, job.name, @@ -470,6 +491,7 @@ impl Database { job.output_dir.to_string_lossy().to_string(), job.password, nzb_data, + job.pp_override, ], )?; @@ -577,8 +599,10 @@ impl Database { self.conn.execute( "INSERT INTO queue (id, name, category, status, priority, total_bytes, downloaded_bytes, file_count, files_completed, article_count, - articles_downloaded, articles_failed, added_at, work_dir, output_dir, password) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16)", + articles_downloaded, articles_failed, added_at, work_dir, output_dir, password, + pp_override) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16, + ?17)", params![ job.id, job.name, @@ -596,6 +620,7 @@ impl Database { job.work_dir.to_string_lossy().to_string(), job.output_dir.to_string_lossy().to_string(), job.password, + job.pp_override, ], )?; Ok(()) @@ -660,7 +685,7 @@ impl Database { let mut stmt = self.conn.prepare( "SELECT id, name, category, status, priority, total_bytes, downloaded_bytes, file_count, files_completed, article_count, articles_downloaded, articles_failed, - added_at, completed_at, work_dir, output_dir, password, error_message + added_at, completed_at, work_dir, output_dir, password, error_message, pp_override FROM queue ORDER BY priority DESC, added_at ASC", )?; @@ -688,6 +713,7 @@ impl Database { password: row.get(16)?, error_message: row.get(17)?, speed_bps: 0, + pp_override: row.get(18)?, server_stats: Vec::new(), files: Vec::new(), // Loaded separately }) @@ -1340,11 +1366,27 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } } + #[test] + fn queue_round_trips_post_processing_override() { + let db = Database::open_memory().unwrap(); + let mut overridden = make_job("pp-job", "PP Job"); + overridden.pp_override = Some(1); + db.queue_insert(&overridden).unwrap(); + db.queue_insert(&make_job("plain-job", "Plain Job")) + .unwrap(); + + let jobs = db.queue_list().unwrap(); + let pp = |id: &str| jobs.iter().find(|job| job.id == id).unwrap().pp_override; + assert_eq!(pp("pp-job"), Some(1)); + assert_eq!(pp("plain-job"), None); + } + fn make_history(id: &str, name: &str) -> HistoryEntry { HistoryEntry { id: id.into(), diff --git a/crates/nzb-core/src/models.rs b/crates/nzb-core/src/models.rs index 10c8d485..c5661394 100644 --- a/crates/nzb-core/src/models.rs +++ b/crates/nzb-core/src/models.rs @@ -185,6 +185,11 @@ pub struct NzbJob { /// Per-server download statistics #[serde(default)] pub server_stats: Vec, + /// Per-job post-processing level overriding the category's (0=none, + /// 1=repair, 2=unpack, 3=repair+unpack), e.g. SABnzbd's `pp` parameter. + /// `None` uses the category setting. + #[serde(default)] + pub pp_override: Option, /// Files in this job #[serde(skip)] pub files: Vec, diff --git a/crates/nzb-core/src/nzb_parser.rs b/crates/nzb-core/src/nzb_parser.rs index f20ad926..c8f49099 100644 --- a/crates/nzb-core/src/nzb_parser.rs +++ b/crates/nzb-core/src/nzb_parser.rs @@ -283,6 +283,7 @@ pub fn parse_nzb(name: &str, data: &[u8]) -> Result { password: meta_password, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files, }) diff --git a/crates/nzb-dispatch/src/bandwidth.rs b/crates/nzb-dispatch/src/bandwidth.rs index 4ee699a1..49d1088f 100644 --- a/crates/nzb-dispatch/src/bandwidth.rs +++ b/crates/nzb-dispatch/src/bandwidth.rs @@ -6,6 +6,7 @@ use governor::DefaultDirectRateLimiter as RateLimiter; use governor::Quota; use serde::{Deserialize, Serialize}; use std::sync::Arc; +use tokio::sync::Notify; #[derive(Default, Serialize, Deserialize, Clone, Copy, Debug, PartialEq, Eq)] pub struct BandwidthConfig { @@ -23,6 +24,9 @@ struct Bucket { struct Limit { limiter: ArcSwapOption, current_bps: AtomicU32, + /// Signalled on every reconfiguration so parked acquires abandon the + /// limiter they loaded and restart against the current one. + changed: Notify, } impl Limit { @@ -38,16 +42,22 @@ impl Limit { Self { limiter: ArcSwapOption::new(Self::new_inner(bps)), current_bps: AtomicU32::new(bps.map(|v| v.get()).unwrap_or(0)), + changed: Notify::new(), } } async fn acquire(&self, size: NonZeroU32) -> anyhow::Result<()> { let mut remaining = size.get(); while remaining > 0 { - // Reload per chunk: a limit change mid-article takes effect at - // the next chunk instead of after the whole article. - let lim = self.limiter.load().clone(); - let Some(bucket) = lim.as_ref() else { + // Register for change notifications *before* loading the limiter, + // so a reconfiguration between the load and the wait is not lost. + let changed = self.changed.notified(); + tokio::pin!(changed); + changed.as_mut().enable(); + + let lim = self.limiter.load_full(); + let Some(bucket) = lim else { + // Unlimited (including a switch to unlimited mid-acquire). return Ok(()); }; // `Quota::per_second(bps)` gives a burst of `bps` cells, and @@ -55,14 +65,18 @@ impl Limit { // `InsufficientCapacity`. A decoded article (~750 KB) exceeds the // burst for every limit below that, so acquire in burst-sized // chunks rather than in one call. - let burst = bucket.burst.get(); - let chunk = remaining.min(burst); + let chunk = remaining.min(bucket.burst.get()); // `chunk` is non-zero: `remaining > 0` and `burst >= 1`. - bucket - .limiter - .until_n_ready(NonZeroU32::new(chunk).expect("chunk > 0")) - .await?; - remaining -= chunk; + let n = NonZeroU32::new(chunk).expect("chunk > 0"); + tokio::select! { + res = bucket.limiter.until_n_ready(n) => { + res?; + remaining -= chunk; + } + // The limit changed while parked: drop the wait on the old + // limiter and re-evaluate `remaining` against the new one. + () = &mut changed => {} + } } Ok(()) } @@ -72,6 +86,7 @@ impl Limit { self.limiter.swap(new); self.current_bps .store(limit.map(|v| v.get()).unwrap_or(0), Ordering::Relaxed); + self.changed.notify_waiters(); } fn get(&self) -> Option { @@ -135,6 +150,71 @@ mod tests { ); } + /// Start an acquire that, at `bps`, would take hours, then reconfigure + /// the limit and return how long the parked acquire took to complete. + async fn acquire_then_reconfigure( + initial: u32, + changed: Option, + ) -> std::time::Duration { + let limiter = Arc::new(BandwidthLimiter::new(BandwidthConfig { + download_bps: NonZeroU32::new(initial), + })); + let waiter = { + let limiter = Arc::clone(&limiter); + tokio::spawn(async move { + // One decoded article: ~750 KB, i.e. ~10 h at 20 B/s. + limiter + .acquire_download(NonZeroU32::new(750_000).unwrap()) + .await + }) + }; + // Let the acquire park inside the old limiter. + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + assert!(!waiter.is_finished(), "acquire should be throttled"); + + let changed_at = std::time::Instant::now(); + limiter.set_download_bps(changed); + tokio::time::timeout(std::time::Duration::from_secs(5), waiter) + .await + .expect("parked acquire never observed the limit change") + .unwrap() + .unwrap(); + changed_at.elapsed() + } + + /// Regression (BUG-57): a parked acquire kept awaiting the limiter it + /// loaded on entry, so lowering the limit to 20 B/s wedged every worker + /// for hours and setting it back to unlimited did not release them. + #[tokio::test] + async fn switching_to_unlimited_releases_parked_acquire() { + let took = acquire_then_reconfigure(20, None).await; + assert!( + took < std::time::Duration::from_secs(2), + "unlimited took {took:?} to apply" + ); + } + + #[tokio::test] + async fn raising_limit_releases_parked_acquire() { + let took = acquire_then_reconfigure(20, NonZeroU32::new(10_000_000)).await; + assert!( + took < std::time::Duration::from_secs(2), + "raised limit took {took:?} to apply" + ); + } + + #[tokio::test] + async fn unlimited_acquire_returns_immediately() { + let limiter = BandwidthLimiter::new(BandwidthConfig::default()); + tokio::time::timeout( + std::time::Duration::from_millis(100), + limiter.acquire_download(NonZeroU32::new(u32::MAX).unwrap()), + ) + .await + .expect("unlimited acquire must not wait") + .unwrap(); + } + #[tokio::test] async fn limiter_can_be_reconfigured_without_recreation() { let limiter = BandwidthLimiter::new(BandwidthConfig::default()); diff --git a/crates/nzb-dispatch/src/download_engine.rs b/crates/nzb-dispatch/src/download_engine.rs index 1c916f70..fdb2cdf8 100644 --- a/crates/nzb-dispatch/src/download_engine.rs +++ b/crates/nzb-dispatch/src/download_engine.rs @@ -2445,17 +2445,25 @@ async fn run_worker_pipelined( attempt = item.tries_on_current + 1, "Transient pipeline error — re-queuing in-flight work and reconnecting: {e}" ); - let is_auth = matches!( - failure.kind, - crate::article_failure::ArticleFailureKind::AuthFailed - | crate::article_failure::ArticleFailureKind::PermissionDenied - ); - pool.server_health - .lock() - .entry(primary_server.id.clone()) - .or_default() - .record_failure(is_auth, &e.to_string()); - pool.report_provider_outage(); + // Only provider-level failures count against the + // circuit breaker, as in the serial path. Article-level + // answers (protocol errors such as 451, timeouts) are + // capped per server by the outage-try limit below; + // counting them let a few bad articles pause the whole + // server. + if counts_against_circuit_breaker(&e) { + let is_auth = matches!( + failure.kind, + crate::article_failure::ArticleFailureKind::AuthFailed + | crate::article_failure::ArticleFailureKind::PermissionDenied + ); + pool.server_health + .lock() + .entry(primary_server.id.clone()) + .or_default() + .record_failure(is_auth, &e.to_string()); + pool.report_provider_outage(); + } // A server-wide outage (502, 480, 403) says nothing // about the article, so it is re-queued without // spending a try: the circuit breaker decides when the @@ -2827,6 +2835,25 @@ fn all_enabled_providers_definitive( .all(|server| outcomes.contains_key(&server.id)) } +/// Whether a fetch error says the provider itself is unhealthy, and so should +/// count against its circuit breaker: connection/TLS, auth, permission, 502 +/// and timeouts. A server that accepts the connection and then black-holes +/// the read is as unhealthy as one that refuses it. Article-level answers +/// never count. +fn counts_against_circuit_breaker(e: &NntpError) -> bool { + matches!( + e, + NntpError::Connection(_) + | NntpError::Io(_) + | NntpError::Tls(_) + | NntpError::Auth(_) + | NntpError::AuthRequired(_) + | NntpError::PermissionDenied(_) + | NntpError::ServiceUnavailable(_) + | NntpError::Timeout(_) + ) +} + /// Re-queue all in-flight items back to the work queue (on connection loss). fn requeue_all(in_flight: &mut HashMap, work_queue: &Arc) { let items: Vec = in_flight.drain().map(|(_, item)| item).collect(); @@ -3406,6 +3433,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } @@ -4416,4 +4444,28 @@ mod tests { assert_eq!(t.connected_snapshot(), vec![("srv1".into(), 0, 1)]); assert_eq!(t.snapshot(), vec![("srv1".into(), 1, 1)]); } + + #[test] + fn only_provider_level_errors_count_against_circuit_breaker() { + let s = || "x".to_string(); + for e in [ + NntpError::Connection(s()), + NntpError::Tls(s()), + NntpError::Auth(s()), + NntpError::AuthRequired(s()), + NntpError::PermissionDenied(s()), + NntpError::ServiceUnavailable(s()), + NntpError::Timeout(s()), + ] { + assert!(counts_against_circuit_breaker(&e), "{e}"); + } + for e in [ + NntpError::ArticleNotFound(s()), + NntpError::NoSuchGroup(s()), + NntpError::NoArticleSelected(s()), + NntpError::Protocol(s()), + ] { + assert!(!counts_against_circuit_breaker(&e), "{e}"); + } + } } diff --git a/crates/nzb-nntp/src/pipeline.rs b/crates/nzb-nntp/src/pipeline.rs index 76e00a40..d52e4546 100644 --- a/crates/nzb-nntp/src/pipeline.rs +++ b/crates/nzb-nntp/src/pipeline.rs @@ -309,15 +309,24 @@ impl StatPipeline { } else { format!("<{mid}>") }; + // An I/O failure anywhere in the batch leaves the stream + // misaligned with the outstanding STATs, so the connection + // must not be reused. conn.send_command_no_flush(&format!("STAT {normalized}")) - .await?; + .await + .inspect_err(|_| conn.state = ConnectionState::Error)?; trace!(mid = %normalized, "StatPipeline sent STAT"); } - conn.flush().await?; + conn.flush() + .await + .inspect_err(|_| conn.state = ConnectionState::Error)?; // Read responses in order for mid in batch { - let resp = conn.read_response_line().await?; + let resp = conn + .read_response_line() + .await + .inspect_err(|_| conn.state = ConnectionState::Error)?; match resp.code { 223 => { results.push(StatResult { @@ -738,6 +747,31 @@ mod tests { assert!(results[1].exists); } + #[tokio::test] + async fn test_stat_pipeline_io_failure_marks_connection_error() { + // The server answers the connect handshake and the first STAT, then + // drops the socket, so a later read in the batch fails mid-stream. + let mut articles = HashMap::new(); + for i in 0..4 { + articles.insert(format!("a{i}@test"), b"body".to_vec()); + } + let server = MockNntpServer::start(MockConfig { + articles, + close_after_n_commands: Some(2), + ..MockConfig::default() + }) + .await; + let mut conn = NntpConnection::new("test".into()); + conn.connect(&test_config(server.port())).await.unwrap(); + + let mut stat = StatPipeline::new(); + for i in 0..4 { + stat.add(format!("a{i}@test")); + } + assert!(stat.execute(&mut conn).await.is_err()); + assert_eq!(conn.state, ConnectionState::Error); + } + #[tokio::test] async fn test_stat_pipeline_unknown_response_fails_closed() { let mut overrides = HashMap::new(); diff --git a/crates/nzb-web/Cargo.toml b/crates/nzb-web/Cargo.toml index 50579dd6..b71e168f 100644 --- a/crates/nzb-web/Cargo.toml +++ b/crates/nzb-web/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "nzb-web" -version = "0.4.22" +version = "0.4.23" edition = "2024" description = "Usenet download engine: queue management, download orchestration, and background services" license = "MIT" @@ -46,6 +46,8 @@ regex = "1" libc = "0.2" unicode-normalization = "0.1" flate2.workspace = true +zip.workspace = true +bzip2.workspace = true [lints.clippy] all = { level = "warn", priority = -1 } diff --git a/crates/nzb-web/src/dir_watcher.rs b/crates/nzb-web/src/dir_watcher.rs index 30ae3d9a..a27894fe 100644 --- a/crates/nzb-web/src/dir_watcher.rs +++ b/crates/nzb-web/src/dir_watcher.rs @@ -2,14 +2,21 @@ use std::io::Read as _; use std::path::{Path, PathBuf}; use std::sync::Arc; -use flate2::read::GzDecoder; use notify::{Event, EventKind, RecursiveMode, Watcher}; use tokio::sync::mpsc; -use tracing::{error, info, warn}; +use tracing::{debug, error, info, warn}; +use crate::nzb_archive::extract_nzbs; use crate::queue_manager::QueueManager; const MAX_WATCHED_NZB_BYTES: usize = 100 * 1024 * 1024; +/// Interval between the size checks that decide a dropped file is complete. +const SETTLE_INTERVAL: std::time::Duration = std::time::Duration::from_millis(500); +/// Give up waiting for a file that never stops changing and process it anyway. +const MAX_SETTLE_WAIT: std::time::Duration = std::time::Duration::from_secs(600); +/// Suffix of a file the watcher has claimed. Claimed files are never picked +/// up again, so an NZB is enqueued at most once even if the final move fails. +const CLAIM_SUFFIX: &str = ".processing"; pub struct DirWatcher { watch_dir: PathBuf, @@ -61,8 +68,6 @@ impl DirWatcher { EventKind::Create(_) | EventKind::Modify(_) => { for path in &event.paths { if Self::is_nzb_file(path) { - // Small delay to ensure file is fully written - tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; self.process_file(path).await; } } @@ -72,12 +77,16 @@ impl DirWatcher { } } + /// Watched names: `.nzb`, plus `.nzb.gz`, `.nzb.bz2`, and `.zip` archives + /// of NZBs. Matching is case-sensitive, as it always has been here. fn is_nzb_file(path: &Path) -> bool { - path.extension().is_some_and(|ext| ext == "nzb") || Self::is_gz_nzb(path) - } - - fn is_gz_nzb(path: &Path) -> bool { - path.to_str().is_some_and(|s| s.ends_with(".nzb.gz")) + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| { + [".nzb", ".nzb.gz", ".nzb.bz2", ".zip"] + .iter() + .any(|suffix| name.ends_with(suffix)) + }) } async fn process_existing_files(&self) { @@ -93,89 +102,140 @@ impl DirWatcher { let path = entry.path(); if Self::is_nzb_file(&path) { self.process_file(&path).await; + } else if path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.ends_with(CLAIM_SUFFIX)) + { + warn!( + file = %path.display(), + "Found a claimed NZB left by an interrupted run; it may already be queued, so it is not imported again. Rename it to retry." + ); } } } - async fn process_file(&self, path: &Path) { - info!(file = %path.display(), "Processing NZB from watch directory"); - - let raw_data = match Self::read_limited(path) { - Ok(d) => d, - Err(e) => { - warn!(error = %e, file = %path.display(), "Failed to read NZB file"); - return; - } + /// Wait until `path` stops changing: its size and modification time + /// must be identical across two checks `interval` apart. Returns `false` + /// if the file disappeared. + async fn wait_until_stable(path: &Path, interval: std::time::Duration) -> bool { + let snapshot = |path: &Path| { + std::fs::symlink_metadata(path) + .ok() + .map(|m| (m.len(), m.modified().ok())) }; - - let data = if Self::is_gz_nzb(path) { - let decoder = GzDecoder::new(raw_data.as_slice()); - let mut decompressed = Vec::new(); - if let Err(error) = decoder - .take((MAX_WATCHED_NZB_BYTES as u64).saturating_add(1)) - .read_to_end(&mut decompressed) - { - warn!(error = %error, file = %path.display(), "Failed to decompress watched NZB"); - return; + let started = tokio::time::Instant::now(); + let Some(mut previous) = snapshot(path) else { + return false; + }; + loop { + tokio::time::sleep(interval).await; + let Some(current) = snapshot(path) else { + return false; + }; + if current == previous { + return true; } - if decompressed.len() > MAX_WATCHED_NZB_BYTES { - warn!(file = %path.display(), limit = MAX_WATCHED_NZB_BYTES, "Decompressed watched NZB exceeds the input limit"); - return; + if started.elapsed() >= MAX_SETTLE_WAIT { + warn!(file = %path.display(), "Watched file is still changing; processing it anyway"); + return true; } - decompressed - } else { - raw_data - }; - - let name = if Self::is_gz_nzb(path) { - path.file_name() - .and_then(|name| name.to_str()) - .and_then(|name| name.strip_suffix(".nzb.gz")) - .unwrap_or("unknown") - .to_string() - } else { - path.file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("unknown") - .to_string() - }; - - match crate::nzb_core::nzb_parser::parse_nzb(&name, &data) { - Ok(mut job) => { - job.work_dir = self.queue_manager.incomplete_dir().join(&job.id); - job.output_dir = self.queue_manager.complete_dir().join(&job.name); - - if let Err(e) = std::fs::create_dir_all(&job.work_dir) { - error!(error = %e, "Failed to create work directory"); - return; - } + previous = current; + } + } - info!(name = %job.name, id = %job.id, "Auto-enqueuing NZB from watch dir"); + /// Move `from` into `//`. + fn move_into( + &self, + from: &Path, + subdir: &str, + file_name: &std::ffi::OsStr, + ) -> std::io::Result<()> { + let dir = self.watch_dir.join(subdir); + std::fs::create_dir_all(&dir)?; + let dest = dir.join(file_name); + if std::fs::rename(from, &dest).is_err() { + // If rename fails (cross-device), try copy+delete + std::fs::copy(from, &dest).and_then(|_| std::fs::remove_file(from))?; + } + Ok(()) + } - if let Err(e) = self.queue_manager.add_job(job, Some(data)) { - error!(error = %e, "Failed to enqueue NZB"); - return; - } + async fn process_file(&self, path: &Path) { + // Several events can fire for one file while it is being written; a + // later one finds it already claimed and gone. + if !Self::wait_until_stable(path, SETTLE_INTERVAL).await { + debug!(file = %path.display(), "Watched file vanished before it settled"); + return; + } + let Some(file_name) = path.file_name().map(|name| name.to_os_string()) else { + return; + }; - // Move processed file to avoid re-processing - let processed_dir = self.watch_dir.join("processed"); - let _ = std::fs::create_dir_all(&processed_dir); - let dest = processed_dir.join(path.file_name().unwrap_or_default()); - if let Err(_e) = std::fs::rename(path, &dest) { - // If rename fails (cross-device), try copy+delete - if let Err(e2) = - std::fs::copy(path, &dest).and_then(|_| std::fs::remove_file(path)) - { - warn!(error = %e2, "Failed to move processed NZB file"); - } - } + // Claim the file before enqueueing so that, whatever happens next, + // it is never imported a second time. + let mut claimed_name = file_name.clone(); + claimed_name.push(CLAIM_SUFFIX); + let claimed = path.with_file_name(&claimed_name); + if let Err(e) = std::fs::rename(path, &claimed) { + if e.kind() != std::io::ErrorKind::NotFound { + warn!(error = %e, file = %path.display(), "Failed to claim watched NZB"); } + return; + } + + info!(file = %path.display(), "Processing NZB from watch directory"); + let destination = match self.enqueue_claimed(&claimed, &file_name).await { + Ok(()) => "processed", Err(e) => { - warn!(error = %e, file = %path.display(), "Failed to parse NZB from watch dir"); + warn!(error = %e, file = %path.display(), "Failed to import NZB from watch dir; moving it to failed/"); + "failed" } + }; + if let Err(e) = self.move_into(&claimed, destination, &file_name) { + warn!( + error = %e, + file = %claimed.display(), + "Failed to move claimed NZB to {destination}/; it stays claimed and will not be imported again" + ); } } + /// Read, unpack and enqueue the claimed copy. One watched file can hold + /// several NZBs (a zip of them), so every member is enqueued and any + /// failure moves the whole file to failed/. + async fn enqueue_claimed( + &self, + claimed: &Path, + file_name: &std::ffi::OsStr, + ) -> Result<(), String> { + let raw_data = Self::read_limited(claimed).map_err(|e| format!("read failed: {e}"))?; + let file_name_str = file_name.to_str().unwrap_or("unknown.nzb"); + let nzbs = + extract_nzbs(file_name_str, &raw_data).map_err(|e| format!("unpack failed: {e}"))?; + if nzbs.is_empty() { + return Err("archive contains no NZBs".into()); + } + for (nzb_name, data) in nzbs { + // Archive entries may carry directories; the job is named after + // the NZB itself, without its extension. + let base = nzb_name.rsplit(['/', '\\']).next().unwrap_or(&nzb_name); + let name = base.strip_suffix(".nzb").unwrap_or(base); + let name = if name.is_empty() { "unknown" } else { name }; + let mut job = crate::nzb_core::nzb_parser::parse_nzb(name, &data) + .map_err(|e| format!("parse failed for {nzb_name}: {e}"))?; + job.work_dir = self.queue_manager.incomplete_dir().join(&job.id); + job.output_dir = self.queue_manager.complete_dir().join(&job.name); + std::fs::create_dir_all(&job.work_dir) + .map_err(|e| format!("failed to create work directory: {e}"))?; + info!(name = %job.name, id = %job.id, "Auto-enqueuing NZB from watch dir"); + self.queue_manager + .add_job(job, Some(data)) + .map_err(|e| format!("enqueue failed: {e}"))?; + } + Ok(()) + } + fn read_limited(path: &Path) -> std::io::Result> { let metadata = std::fs::symlink_metadata(path)?; if metadata.file_type().is_symlink() { @@ -210,13 +270,57 @@ mod tests { use super::*; #[test] - fn recognizes_plain_and_gzipped_nzb_paths_case_sensitively() { + fn recognizes_plain_and_compressed_nzb_paths_case_sensitively() { assert!(DirWatcher::is_nzb_file(Path::new("release.nzb"))); assert!(DirWatcher::is_nzb_file(Path::new("release.nzb.gz"))); + assert!(DirWatcher::is_nzb_file(Path::new("release.nzb.bz2"))); + assert!(DirWatcher::is_nzb_file(Path::new("bundle.zip"))); + assert!(!DirWatcher::is_nzb_file(Path::new("release.gz"))); assert!(!DirWatcher::is_nzb_file(Path::new("release.NZB"))); assert!(!DirWatcher::is_nzb_file(Path::new("release.txt"))); } + #[tokio::test] + async fn waits_until_a_growing_file_stops_changing() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("growing.nzb"); + std::fs::write(&path, b"a").unwrap(); + + let writer_path = path.clone(); + let writer = tokio::spawn(async move { + for _ in 0..8 { + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + let mut file = std::fs::OpenOptions::new() + .append(true) + .open(&writer_path) + .unwrap(); + std::io::Write::write_all(&mut file, b"a").unwrap(); + } + }); + + let interval = std::time::Duration::from_millis(150); + assert!(DirWatcher::wait_until_stable(&path, interval).await); + writer.await.unwrap(); + assert_eq!(std::fs::metadata(&path).unwrap().len(), 9); + } + + #[tokio::test] + async fn settle_wait_reports_a_vanished_file() { + let temp = tempfile::tempdir().unwrap(); + let interval = std::time::Duration::from_millis(10); + assert!(!DirWatcher::wait_until_stable(&temp.path().join("gone.nzb"), interval).await); + } + + #[test] + fn claimed_files_are_not_watched() { + assert!(!DirWatcher::is_nzb_file(Path::new( + "release.nzb.processing" + ))); + assert!(!DirWatcher::is_nzb_file(Path::new( + "release.nzb.gz.processing" + ))); + } + #[test] fn bounded_file_reader_rejects_oversized_input() { let temp = tempfile::tempdir().unwrap(); diff --git a/crates/nzb-web/src/error.rs b/crates/nzb-web/src/error.rs index ec41ca17..ef077b36 100644 --- a/crates/nzb-web/src/error.rs +++ b/crates/nzb-web/src/error.rs @@ -125,11 +125,25 @@ impl Serialize for ApiError { let serr = SerializedError { error_kind: match &self.kind { - ApiErrorKind::JobNotFound(_) => "job_not_found", - ApiErrorKind::ServerNotFound(_) => "server_not_found", + ApiErrorKind::JobNotFound(_) + | ApiErrorKind::Core(crate::nzb_core::NzbError::JobNotFound(_)) => "job_not_found", + ApiErrorKind::ServerNotFound(_) + | ApiErrorKind::Core(crate::nzb_core::NzbError::ServerNotFound(_)) => { + "server_not_found" + } ApiErrorKind::Unauthorized => "unauthorized", - ApiErrorKind::AdmissionConflict => "admission_conflict", - _ => "internal_error", + ApiErrorKind::AdmissionConflict + | ApiErrorKind::Core(crate::nzb_core::NzbError::AdmissionConflict) => { + "admission_conflict" + } + // Otherwise classify by status so clients can tell a bad + // request or missing resource from a server fault. + _ => match self.status() { + StatusCode::BAD_REQUEST => "bad_request", + StatusCode::NOT_FOUND => "not_found", + StatusCode::CONFLICT => "conflict", + _ => "internal_error", + }, }, human_readable: format!("{:#}", self.kind), status: self.status().as_u16(), @@ -148,9 +162,21 @@ impl From for ApiError { } impl From for ApiError { + /// Map domain errors to the HTTP status that describes them: missing + /// resources are 404, malformed input 400, conflicts 409, and only + /// genuine server-side failures 500. fn from(e: crate::nzb_core::NzbError) -> Self { + use crate::nzb_core::NzbError; + let status = match &e { + NzbError::JobNotFound(_) + | NzbError::ServerNotFound(_) + | NzbError::CategoryNotFound(_) => StatusCode::NOT_FOUND, + NzbError::ParseError(_) | NzbError::InvalidNzb(_) => StatusCode::BAD_REQUEST, + NzbError::AdmissionConflict => StatusCode::CONFLICT, + _ => StatusCode::INTERNAL_SERVER_ERROR, + }; Self { - status: Some(StatusCode::INTERNAL_SERVER_ERROR), + status: Some(status), kind: ApiErrorKind::Core(e), } } @@ -190,3 +216,41 @@ impl IntoResponse for ApiError { response } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::nzb_core::NzbError; + + #[test] + fn domain_errors_map_to_client_statuses() { + let cases = [ + (NzbError::JobNotFound("j".into()), 404, "job_not_found"), + ( + NzbError::ServerNotFound("s".into()), + 404, + "server_not_found", + ), + (NzbError::CategoryNotFound("c".into()), 404, "not_found"), + (NzbError::InvalidNzb("x".into()), 400, "bad_request"), + (NzbError::ParseError("x".into()), 400, "bad_request"), + (NzbError::AdmissionConflict, 409, "admission_conflict"), + (NzbError::Other("boom".into()), 500, "internal_error"), + ]; + for (error, status, kind) in cases { + let api = ApiError::from(error); + assert_eq!(api.status().as_u16(), status); + let json = serde_json::to_value(&api).unwrap(); + assert_eq!(json["error_kind"], kind); + assert_eq!(json["status"], status); + } + } + + #[test] + fn text_errors_are_classified_by_status() { + let json = serde_json::to_value(ApiError::not_found("gone")).unwrap(); + assert_eq!(json["error_kind"], "not_found"); + let json = serde_json::to_value(ApiError::bad_request("bad")).unwrap(); + assert_eq!(json["error_kind"], "bad_request"); + } +} diff --git a/crates/nzb-web/src/lib.rs b/crates/nzb-web/src/lib.rs index f871c24b..e5625d45 100644 --- a/crates/nzb-web/src/lib.rs +++ b/crates/nzb-web/src/lib.rs @@ -10,6 +10,7 @@ pub mod direct_unpack; pub mod error; pub mod fetch_guard; pub mod log_buffer; +pub mod nzb_archive; pub mod queue_manager; pub mod rss_monitor; pub mod sabnzbd_compat; diff --git a/crates/nzb-web/src/nzb_archive.rs b/crates/nzb-web/src/nzb_archive.rs new file mode 100644 index 00000000..31a7c6ce --- /dev/null +++ b/crates/nzb-web/src/nzb_archive.rs @@ -0,0 +1,236 @@ +//! Unpacking of compressed NZB uploads shared by the HTTP API and the watch +//! folder: `.gz`, `.bz2`, and `.zip` archives are expanded into the `.nzb` +//! documents they carry, with a bound on the decompressed size. + +use std::io::{Cursor, Read as _}; + +use bzip2::read::BzDecoder; +use flate2::read::GzDecoder; + +/// Upper bound on the decompressed size of NZB content taken from one archive. +pub const MAX_NZB_DECOMPRESSED_BYTES: u64 = 100 * 1024 * 1024; + +/// Upper bound on the number of `.nzb` entries taken from one zip. A central +/// directory can name far more entries than the decompressed budget could ever +/// hold, and each one costs an allocation. +const MAX_ZIP_NZB_ENTRIES: usize = 10_000; + +/// Extract NZB files from an uploaded file. If it's an archive (zip, gz, bz2), +/// returns all `.nzb` entries found inside. Otherwise returns the file as-is. +pub fn extract_nzbs(file_name: &str, data: &[u8]) -> Result)>, anyhow::Error> { + extract_nzbs_bounded(file_name, data, MAX_NZB_DECOMPRESSED_BYTES) +} + +fn extract_nzbs_bounded( + file_name: &str, + data: &[u8], + max_bytes: u64, +) -> Result)>, anyhow::Error> { + let lower = file_name.to_lowercase(); + + // .nzb.gz or .gz containing an nzb + if lower.ends_with(".gz") { + let decompressed = read_bounded(GzDecoder::new(data), "gzip", max_bytes)?; + let inner_name = &file_name[..file_name.len() - ".gz".len()]; + return Ok(vec![(inner_name.to_string(), decompressed)]); + } + + // .nzb.bz2 or .bz2 containing an nzb + if lower.ends_with(".bz2") { + let decompressed = read_bounded(BzDecoder::new(data), "bzip2", max_bytes)?; + let inner_name = &file_name[..file_name.len() - ".bz2".len()]; + return Ok(vec![(inner_name.to_string(), decompressed)]); + } + + // .zip archive — extract all .nzb files inside + if lower.ends_with(".zip") { + let cursor = Cursor::new(data); + let mut archive = zip::ZipArchive::new(cursor) + .map_err(|e| anyhow::anyhow!("Failed to read zip archive: {e}"))?; + // Overlapping central-directory entries let one compressed span be + // counted once per entry, so the size budget would under-count the + // bytes actually produced. Such archives are rejected outright. + if archive + .has_overlapping_files() + .map_err(|e| anyhow::anyhow!("Failed to read zip archive: {e}"))? + { + anyhow::bail!("Zip archive '{file_name}' contains overlapping files"); + } + let mut nzbs = Vec::new(); + let mut total_uncompressed = 0u64; + for i in 0..archive.len() { + let mut entry = archive + .by_index(i) + .map_err(|e| anyhow::anyhow!("Zip entry error: {e}"))?; + let entry_name = entry.name().to_string(); + if entry_name.to_lowercase().ends_with(".nzb") { + if nzbs.len() >= MAX_ZIP_NZB_ENTRIES { + anyhow::bail!("Zip archive '{file_name}' contains too many NZB files"); + } + // Budget on the bytes actually read, never the declared + // `entry.size()`: the central directory's uncompressed size is + // attacker-controlled and zip does not enforce it. + let room = max_bytes + .saturating_sub(total_uncompressed) + .saturating_add(1); + let mut buf = Vec::new(); + entry + .by_ref() + .take(room) + .read_to_end(&mut buf) + .map_err(|e| anyhow::anyhow!("Failed to read zip entry '{entry_name}': {e}"))?; + total_uncompressed = total_uncompressed + .checked_add(buf.len() as u64) + .ok_or_else(|| anyhow::anyhow!("Zip archive size overflow"))?; + if total_uncompressed > max_bytes { + anyhow::bail!( + "Decompressed NZB exceeds the {} MB limit", + MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 + ); + } + nzbs.push((entry_name, buf)); + } + } + if nzbs.is_empty() { + anyhow::bail!("No .nzb files found in zip archive '{file_name}'"); + } + return Ok(nzbs); + } + + // Plain .nzb or unrecognized — pass through as-is + Ok(vec![(file_name.to_string(), data.to_vec())]) +} + +fn read_bounded( + reader: impl std::io::Read, + format: &str, + max_bytes: u64, +) -> Result, anyhow::Error> { + let mut decompressed = Vec::new(); + reader + .take(max_bytes + 1) + .read_to_end(&mut decompressed) + .map_err(|e| anyhow::anyhow!("Failed to decompress {format}: {e}"))?; + if decompressed.len() as u64 > max_bytes { + anyhow::bail!( + "Decompressed NZB exceeds the {} MB limit", + MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 + ); + } + Ok(decompressed) +} + +#[cfg(test)] +mod tests { + use super::{extract_nzbs, extract_nzbs_bounded}; + use std::io::Write; + + use zip::CompressionMethod; + use zip::write::SimpleFileOptions; + + fn build_zip(entries: &[(&str, &[u8])]) -> Vec { + let cursor = std::io::Cursor::new(Vec::new()); + let mut writer = zip::ZipWriter::new(cursor); + let options = SimpleFileOptions::default().compression_method(CompressionMethod::Deflated); + + for (name, contents) in entries { + writer.start_file(name, options).unwrap(); + writer.write_all(contents).unwrap(); + } + + writer.finish().unwrap().into_inner() + } + + /// An entry whose central-directory header claims a far smaller + /// uncompressed size than the bytes it actually decompresses to. + fn build_zip_underdeclared(name: &str, contents: &[u8], declared: u64) -> Vec { + let mut zip = build_zip(&[(name, contents)]); + let name_bytes = name.as_bytes(); + let mut patched = 0; + let mut i = 0; + while i + 30 < zip.len() { + if zip[i..i + 4] == [0x50, 0x4b, 0x01, 0x02] { + let name_len = u16::from_le_bytes([zip[i + 28], zip[i + 29]]) as usize; + if i + 46 + name_len <= zip.len() && &zip[i + 46..i + 46 + name_len] == name_bytes { + zip[i + 24..i + 28].copy_from_slice(&(declared as u32).to_le_bytes()); + patched += 1; + } + } + i += 1; + } + assert_eq!(patched, 1, "central directory entry not patched"); + zip + } + + #[test] + fn extract_nzbs_rejects_zip_entries_that_underdeclare_their_size() { + // Two entries each declare 1 byte but decompress to just over half the + // cap, so the declared total (2) is inside the budget while the bytes + // actually produced are not. The declared size must not be trusted. + // A small cap stands in for the production 100 MB one: the check is on + // bytes read, so the magnitude does not matter. + let cap: u64 = 64; + let payload = vec![b'x'; (cap / 2 + 1) as usize]; + let zip = build_zip_underdeclared("one.nzb", &payload, 1); + let second = build_zip_underdeclared("two.nzb", &payload, 1); + // A zip is not two concatenated archives; rebuild it with both entries. + let _ = (zip, second); + let cursor = std::io::Cursor::new(Vec::new()); + let mut writer = zip::ZipWriter::new(cursor); + let options = SimpleFileOptions::default().compression_method(CompressionMethod::Deflated); + for name in ["one.nzb", "two.nzb"] { + writer.start_file(name, options).unwrap(); + writer.write_all(&payload).unwrap(); + } + let mut zip = writer.finish().unwrap().into_inner(); + let mut patched = 0; + let mut i = 0; + while i + 30 < zip.len() { + if zip[i..i + 4] == [0x50, 0x4b, 0x01, 0x02] { + zip[i + 24..i + 28].copy_from_slice(&1u32.to_le_bytes()); + patched += 1; + } + i += 1; + } + assert_eq!(patched, 2, "both central directory entries patched"); + + let err = extract_nzbs_bounded("underdeclared.zip", &zip, cap).unwrap_err(); + assert!( + err.to_string().contains("limit"), + "under-declared sizes must still hit the cap, got: {err}" + ); + } + + #[test] + fn extract_nzbs_rejects_zip_bombs() { + let cap: u64 = 64; + let oversized = vec![b'x'; (cap + 1) as usize]; + let zip = build_zip(&[("oversized.nzb", oversized.as_slice())]); + let err = extract_nzbs_bounded("oversized.zip", &zip, cap).unwrap_err(); + assert!(err.to_string().contains("limit")); + } + + #[test] + fn extract_nzbs_accepts_small_zip_nzb() { + let zip = build_zip(&[("sample.nzb", br#""#)]); + let nzbs = extract_nzbs("sample.zip", &zip).unwrap(); + assert_eq!(nzbs.len(), 1); + assert_eq!(nzbs[0].0, "sample.nzb"); + assert_eq!(nzbs[0].1, br#""#); + } + + #[test] + fn extract_nzbs_unpacks_gzip_and_bzip2() { + let body = br#""#; + + let mut gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + gz.write_all(body).unwrap(); + let nzbs = extract_nzbs("sample.nzb.GZ", &gz.finish().unwrap()).unwrap(); + assert_eq!(nzbs, vec![("sample.nzb".to_string(), body.to_vec())]); + + let mut bz = bzip2::write::BzEncoder::new(Vec::new(), bzip2::Compression::default()); + bz.write_all(body).unwrap(); + let nzbs = extract_nzbs("sample.nzb.bz2", &bz.finish().unwrap()).unwrap(); + assert_eq!(nzbs, vec![("sample.nzb".to_string(), body.to_vec())]); + } +} diff --git a/crates/nzb-web/src/queue_manager.rs b/crates/nzb-web/src/queue_manager.rs index 2025f451..00e69747 100644 --- a/crates/nzb-web/src/queue_manager.rs +++ b/crates/nzb-web/src/queue_manager.rs @@ -87,6 +87,158 @@ fn cleanup_terminal_work_dir( } } +/// Upper bound on `.` suffixes tried when the complete directory +/// already holds folders with a job's name. +const MAX_OUTPUT_DIR_SUFFIX: u32 = 9999; + +/// Atomically reserve a complete-directory folder derived from `base`. +/// +/// `base` is tried first, then `.1`, `.2`, ... like SABnzbd. +/// `create_dir` either creates the folder or fails with `AlreadyExists`, so +/// two jobs finishing at the same time can never reserve the same folder. +fn claim_unique_output_dir(base: &std::path::Path) -> std::io::Result { + let (Some(parent), Some(name)) = (base.parent(), base.file_name()) else { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "output directory has no parent or name", + )); + }; + std::fs::create_dir_all(parent)?; + for suffix in 0..=MAX_OUTPUT_DIR_SUFFIX { + let candidate = if suffix == 0 { + base.to_path_buf() + } else { + let mut suffixed = name.to_os_string(); + suffixed.push(format!(".{suffix}")); + parent.join(suffixed) + }; + match std::fs::create_dir(&candidate) { + Ok(()) => return Ok(candidate), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(error), + } + } + Err(std::io::Error::new( + std::io::ErrorKind::AlreadyExists, + "no free output directory name", + )) +} + +/// Reserve a unique complete-directory folder for a job that has not reserved +/// one yet, recording the final path in `output_dir`. +fn claim_output_dir_fields(job_id: &str, output_dir: &mut std::path::PathBuf, claimed: &mut bool) { + if *claimed { + return; + } + match claim_unique_output_dir(output_dir) { + Ok(dir) => { + if dir != *output_dir { + info!( + job_id, + requested = %output_dir.display(), + output_dir = %dir.display(), + "Output folder already exists; using a unique folder name" + ); + } + *output_dir = dir; + *claimed = true; + } + Err(e) => warn!( + job_id, + output_dir = %output_dir.display(), + "Unable to reserve a unique output folder: {e}" + ), + } +} + +/// Accept the output directory an earlier attempt of the same job reserved +/// (an interrupted post-processing run, or a direct unpack before a pause), +/// provided it is still a real directory named `` or `.` +/// beside the job's derived target. +fn restore_claimed_output_dir(job: &mut NzbJob, checkpoint: &JobCheckpoint) -> bool { + let Some(claimed) = checkpoint.output_dir.as_ref() else { + return false; + }; + let same_parent = claimed.parent().is_some() && claimed.parent() == job.output_dir.parent(); + let same_base = match (claimed.file_name(), job.output_dir.file_name()) { + (Some(claimed), Some(base)) => { + let (claimed, base) = (claimed.to_string_lossy(), base.to_string_lossy()); + claimed == base + || claimed + .strip_prefix(&*base) + .and_then(|rest| rest.strip_prefix('.')) + .is_some_and(|n| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit())) + } + _ => false, + }; + let is_dir = std::fs::symlink_metadata(claimed) + .map(|metadata| metadata.file_type().is_dir()) + .unwrap_or(false); + if same_parent && same_base && is_dir { + job.output_dir = claimed.clone(); + true + } else { + false + } +} + +/// Resolve `candidate` to a real directory that is a direct child of the +/// canonical incomplete root. Symlinks, files, the root itself, and anything +/// that resolves outside the root are refused. +fn incomplete_child_dir( + root: &std::path::Path, + candidate: &std::path::Path, +) -> Option { + let metadata = std::fs::symlink_metadata(candidate).ok()?; + if !metadata.file_type().is_dir() { + return None; + } + let resolved = std::fs::canonicalize(candidate).ok()?; + (resolved.parent() == Some(root)).then_some(resolved) +} + +/// Whether `name` has the shape of a job id (a hyphenated UUID). Every job +/// work directory is `incomplete/`, so the startup sweep only +/// considers such names and never touches anything else a user keeps there. +fn is_job_id_dir_name(name: &std::ffi::OsStr) -> bool { + let Some(name) = name.to_str() else { + return false; + }; + name.len() == 36 + && name.char_indices().all(|(index, ch)| match index { + 8 | 13 | 18 | 23 => ch == '-', + _ => ch.is_ascii_hexdigit(), + }) +} + +/// Total size of the regular files under `path`, without following symlinks. +fn tree_size(path: &std::path::Path) -> u64 { + let Ok(entries) = std::fs::read_dir(path) else { + return 0; + }; + entries + .flatten() + .map(|entry| match std::fs::symlink_metadata(entry.path()) { + Ok(metadata) if metadata.is_dir() => tree_size(&entry.path()), + Ok(metadata) if metadata.is_file() => metadata.len(), + _ => 0, + }) + .sum() +} + +/// The work directory recorded in a history row's retry checkpoint, read +/// without materialising the per-article outcomes. +fn retry_checkpoint_work_dir(retry_data: &[u8]) -> Option { + #[derive(Deserialize)] + struct WorkDirOnly { + #[serde(default)] + work_dir: Option, + } + serde_json::from_slice::(retry_data) + .ok()? + .work_dir +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ServerStatsData { pub server_id: String, @@ -329,6 +481,10 @@ struct JobCheckpoint { /// Retained partial work directory for missing-only history retry. #[serde(default)] work_dir: Option, + /// Complete-directory folder this job already reserved, so a resumed + /// attempt keeps writing to its own folder instead of reserving another. + #[serde(default)] + output_dir: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -344,7 +500,7 @@ struct ArticleCheckpoint { tries: u32, } -fn checkpoint_for_job(job: &NzbJob) -> JobCheckpoint { +fn checkpoint_for_job(job: &NzbJob, output_dir_claimed: bool) -> JobCheckpoint { JobCheckpoint { files: job .files @@ -388,6 +544,7 @@ fn checkpoint_for_job(job: &NzbJob) -> JobCheckpoint { }) .collect(), work_dir: Some(job.work_dir.clone()), + output_dir: output_dir_claimed.then(|| job.output_dir.clone()), } } @@ -918,6 +1075,10 @@ struct JobState { /// Typed terminal failure code, tracked in memory until it is persisted to /// the terminal history row. failure_code: Option, + /// Whether `job.output_dir` is a complete-directory folder this job + /// reserved for itself. Reserving happens when output is first written, + /// not at add time, so queued jobs never hold a name. + output_dir_claimed: bool, } /// Notification fired immediately when a job is accepted into the queue. @@ -1119,6 +1280,19 @@ impl QueueManager { } /// Update category configs (e.g. after config reload). + /// Post-processing level for a job: its own override (e.g. SABnzbd's + /// `pp`), else its category's setting, else repair+unpack (3). + pub fn post_processing_level(&self, job: &NzbJob) -> u8 { + job.pp_override.unwrap_or_else(|| { + self.categories + .lock() + .iter() + .find(|c| c.name == job.category) + .map(|c| c.post_processing) + .unwrap_or(3) // default: repair+unpack + }) + } + pub fn set_categories(&self, categories: Vec) { *self.categories.lock() = categories; } @@ -1575,6 +1749,7 @@ impl QueueManager { hopeless_tracker: None, download_time_secs: None, failure_code: None, + output_dir_claimed: false, }; self.jobs.lock().insert(job_id.clone(), state); if !requested_paused { @@ -1597,6 +1772,7 @@ impl QueueManager { hopeless_tracker: None, download_time_secs: None, failure_code: None, + output_dir_claimed: false, }; self.jobs.lock().insert(job_id.clone(), state); self.job_order.lock().push(job_id); @@ -1680,6 +1856,10 @@ impl QueueManager { serde_json::from_slice::(&cp_data) { apply_checkpoint(&mut state.job, &checkpoint); + if !state.output_dir_claimed { + state.output_dir_claimed = + restore_claimed_output_dir(&mut state.job, &checkpoint); + } info!( job_id = %job_id, name = %state.job.name, @@ -1978,6 +2158,15 @@ impl QueueManager { && let Some(vol_info) = parse_rar_volume(&file.filename) { if state.direct_unpacker.is_none() { + // Direct unpack writes into the + // complete folder, so reserve it now. + // (`state.job.files` is borrowed by + // the loop, so pass disjoint fields.) + claim_output_dir_fields( + &state.job.id, + &mut state.job.output_dir, + &mut state.output_dir_claimed, + ); state.direct_unpacker = DirectUnpacker::new( &state.job.work_dir, &state.job.output_dir, @@ -2339,13 +2528,9 @@ impl QueueManager { } let cat = state.job.category.clone(); - let pp = self - .categories - .lock() - .iter() - .find(|c| c.name == cat) - .map(|c| c.post_processing) - .unwrap_or(3); // default: repair+unpack + let pp = self.post_processing_level(&state.job); + // Post-processing writes into the complete folder from here on. + Self::claim_output_dir(state); let du = state.direct_unpacker.take(); let pw = state.job.password.clone(); let content_failed = state @@ -2362,6 +2547,8 @@ impl QueueManager { content_failed, ) }; + // Record the reserved folder so a restart resumes into it. + self.persist_job_progress(job_id); // Repair and extraction can write to both the incomplete and the // category output volumes. Apply the same guard to both paths before @@ -2710,6 +2897,7 @@ impl QueueManager { // Move files from work_dir to output_dir (if not already done by pipeline extract). if final_status == JobStatus::Completed { + Self::claim_output_dir(state); if let Err(e) = std::fs::create_dir_all(&state.job.output_dir) { warn!(job_id = %state.job.id, "Failed to create output dir: {e}"); } @@ -2770,7 +2958,17 @@ impl QueueManager { state.job.status = final_status; // Insert into history with real stage results - let retry_data = serde_json::to_vec(&checkpoint_for_job(&state.job)).ok(); + if final_status == JobStatus::Failed && state.output_dir_claimed { + // A failed job keeps nothing in its reserved folder unless + // extraction already wrote there; release an empty reservation so + // a retry can claim the plain name again. + if std::fs::remove_dir(&state.job.output_dir).is_ok() { + state.output_dir_claimed = false; + } + } + + let retry_data = + serde_json::to_vec(&checkpoint_for_job(&state.job, state.output_dir_claimed)).ok(); let history_entry = HistoryEntry { id: state.job.id.clone(), name: state.job.name.clone(), @@ -2863,6 +3061,25 @@ impl QueueManager { } } + /// Reserve a unique complete-directory folder for this job if it has not + /// reserved one yet, and record the final path on the job. + fn claim_output_dir(state: &mut JobState) { + claim_output_dir_fields( + &state.job.id, + &mut state.job.output_dir, + &mut state.output_dir_claimed, + ); + } + + /// Drop a reservation before the job's target folder changes, removing + /// the reserved folder when nothing was written into it yet. + fn release_output_dir_claim(state: &mut JobState) { + if state.output_dir_claimed { + let _ = std::fs::remove_dir(&state.job.output_dir); + state.output_dir_claimed = false; + } + } + /// Persist current job progress to the database, including article-level /// checkpoint data for resume support. fn persist_job_progress(&self, job_id: &str) { @@ -2881,7 +3098,7 @@ impl QueueManager { } // Build and store checkpoint of downloaded article segments - let checkpoint = checkpoint_for_job(&state.job); + let checkpoint = checkpoint_for_job(&state.job, state.output_dir_claimed); if let Ok(data) = serde_json::to_vec(&checkpoint) && let Err(e) = db.queue_store_job_data(job_id, &data) @@ -3269,8 +3486,10 @@ impl QueueManager { // Remove from order self.job_order.lock().retain(|jid| jid != id); - // Try to clean up work directory - if state.job.work_dir.exists() { + // Try to clean up work directory. A terminal job whose history + // row already exists may have retained its partial download for + // retry; deleting that history entry removes it instead. + if !history_already_persisted && state.job.work_dir.exists() { let _ = std::fs::remove_dir_all(&state.job.work_dir); } @@ -3292,6 +3511,7 @@ impl QueueManager { Some((_, s)) => { let output_dir = self.output_dir_for(&s.job.category, &new_name)?; s.job.name = new_name.clone(); + Self::release_output_dir_claim(s); s.job.output_dir = output_dir; info!(job_id = %id, new_name = %new_name, "Job renamed"); Ok(()) @@ -3321,6 +3541,7 @@ impl QueueManager { Some((_, s)) => { s.job.category = category.to_string(); // Update the output directory to match the new category + Self::release_output_dir_claim(s); s.job.output_dir = output_dir; info!(job_id = %id, category = %category, "Job category changed"); Ok(()) @@ -3934,26 +4155,188 @@ impl QueueManager { /// Remove a history entry. pub fn history_remove(&self, id: &str) -> crate::nzb_core::Result<()> { - let db = self.db.lock(); - let existed = db.history_get(id)?.is_some(); - db.history_remove(id)?; - if existed { - self.history_changed(); + let retained = { + let db = self.db.lock(); + let entry = db.history_get(id)?; + let retained = entry + .as_ref() + .map(|entry| self.history_work_dirs(&db, entry)) + .unwrap_or_default(); + db.history_remove(id)?; + if entry.is_some() { + self.history_changed(); + } + retained + }; + for work_dir in retained { + self.remove_unreferenced_work_dir(&work_dir, "history entry deleted"); } Ok(()) } /// Clear all history. pub fn history_clear(&self) -> crate::nzb_core::Result<()> { - let db = self.db.lock(); - let had_entries = db.history_count()? != 0; - db.history_clear()?; - if had_entries { - self.history_changed(); + let retained = { + let db = self.db.lock(); + let had_entries = db.history_count()? != 0; + let retained: Vec<_> = db + .history_list(i64::MAX as usize)? + .iter() + .flat_map(|entry| self.history_work_dirs(&db, entry)) + .collect(); + db.history_clear()?; + if had_entries { + self.history_changed(); + } + retained + }; + for work_dir in retained { + self.remove_unreferenced_work_dir(&work_dir, "history cleared"); } Ok(()) } + /// Incomplete work directories a history row may have retained: its own + /// `incomplete/` and, for a failed row, the directory its retry + /// checkpoint names (a retry reuses an earlier attempt's directory). + fn history_work_dirs(&self, db: &Database, entry: &HistoryEntry) -> Vec { + let mut dirs = vec![self.incomplete_dir().join(&entry.id)]; + if entry.status == JobStatus::Failed + && let Ok(Some(data)) = db.history_get_retry_data(&entry.id) + && let Some(work_dir) = retry_checkpoint_work_dir(&data) + && !dirs.contains(&work_dir) + { + dirs.push(work_dir); + } + dirs + } + + /// Canonical incomplete directories still referenced by a queue job or a + /// history row. With `only_named`, retry checkpoints are parsed only when + /// they mention that directory name, which keeps a single delete cheap. + /// + /// Fails closed: any error reading the queue, the history, or one row's + /// retry checkpoint is returned rather than treated as "nothing references + /// these directories", because callers use the result to decide deletion. + fn referenced_work_dirs( + &self, + only_named: Option<&std::ffi::OsStr>, + ) -> crate::nzb_core::Result> { + let incomplete = self.incomplete_dir(); + // Terminal jobs linger in the queue view briefly after their history + // row is written; their directory belongs to that row, not the queue. + let mut paths: Vec = self + .jobs + .lock() + .values() + .filter(|state| !matches!(state.job.status, JobStatus::Completed | JobStatus::Failed)) + .map(|state| state.job.work_dir.clone()) + .collect(); + let needle = only_named.map(|name| name.to_string_lossy().into_owned().into_bytes()); + { + let db = self.db.lock(); + paths.extend(db.queue_list()?.into_iter().map(|job| job.work_dir)); + for entry in db.history_list(i64::MAX as usize)? { + paths.push(incomplete.join(&entry.id)); + if entry.status != JobStatus::Failed { + continue; + } + let Some(data) = db.history_get_retry_data(&entry.id)? else { + continue; + }; + let mentioned = needle.as_ref().is_none_or(|needle| { + !needle.is_empty() && data.windows(needle.len()).any(|window| window == needle) + }); + if mentioned && let Some(work_dir) = retry_checkpoint_work_dir(&data) { + paths.push(work_dir); + } + } + } + Ok(paths + .into_iter() + .filter_map(|path| std::fs::canonicalize(path).ok()) + .collect()) + } + + /// Remove one retained incomplete work directory once nothing references + /// it. Removal is confined to direct child directories of the incomplete + /// root and never follows a symlink out of it. + fn remove_unreferenced_work_dir(&self, candidate: &std::path::Path, reason: &str) { + let Ok(root) = std::fs::canonicalize(self.incomplete_dir()) else { + return; + }; + let Some(work_dir) = incomplete_child_dir(&root, candidate) else { + return; + }; + let Ok(referenced) = self.referenced_work_dirs(work_dir.file_name()) else { + warn!( + work_dir = %work_dir.display(), + "Keeping work directory: its references could not be read" + ); + return; + }; + if referenced.contains(&work_dir) { + debug!(work_dir = %work_dir.display(), "Keeping work directory still in use"); + return; + } + let size_bytes = tree_size(&work_dir); + match std::fs::remove_dir_all(&work_dir) { + Ok(()) => info!( + work_dir = %work_dir.display(), + size_bytes, + reason, + "Removed retained work directory" + ), + Err(e) => warn!( + work_dir = %work_dir.display(), + "Failed to remove retained work directory: {e}" + ), + } + } + + /// Remove incomplete work directories that no queue job or history row + /// references, such as partial downloads whose history was deleted while + /// the process was down. Only direct child directories of the incomplete + /// root whose names are job ids are considered; files, symlinks, other + /// directories, and the root itself are kept. Runs at startup, before any + /// job can create a new work directory. + fn sweep_orphaned_work_dirs(&self) { + let incomplete = self.incomplete_dir(); + let Ok(root) = std::fs::canonicalize(&incomplete) else { + return; + }; + let Ok(entries) = std::fs::read_dir(&root) else { + return; + }; + let Ok(referenced) = self.referenced_work_dirs(None) else { + warn!("Skipping orphan sweep: queue or history could not be read"); + return; + }; + for entry in entries.flatten() { + if !is_job_id_dir_name(&entry.file_name()) { + continue; + } + let Some(work_dir) = incomplete_child_dir(&root, &entry.path()) else { + continue; + }; + if referenced.contains(&work_dir) { + continue; + } + let size_bytes = tree_size(&work_dir); + match std::fs::remove_dir_all(&work_dir) { + Ok(()) => info!( + work_dir = %work_dir.display(), + size_bytes, + "Removed orphaned incomplete work directory at startup" + ), + Err(e) => warn!( + work_dir = %work_dir.display(), + "Failed to remove orphaned incomplete work directory: {e}" + ), + } + } + } + /// Get live logs for an active job from the in-memory log buffer. pub fn get_job_logs(&self, job_id: &str, limit: usize) -> Vec { if let Some(ref lb) = self.log_buffer { @@ -4084,6 +4467,10 @@ impl QueueManager { info!("Restored global pause state from database"); } + // Reclaim partial downloads nothing can reach any more before any + // job starts writing into the incomplete directory. + self.sweep_orphaned_work_dirs(); + let jobs = { let db = self.db.lock(); db.queue_list()? @@ -4119,6 +4506,7 @@ impl QueueManager { // is loaded lazily in launch_download() when they reach the front of // the queue. This keeps memory low with large queues (hundreds of jobs). let was_active = job.status == JobStatus::Downloading; + let mut output_dir_claimed = false; let nzb_data = if was_active { let db = self.db.lock(); @@ -4150,6 +4538,7 @@ impl QueueManager { match serde_json::from_slice::(data) { Ok(checkpoint) => { apply_checkpoint(&mut job, &checkpoint); + output_dir_claimed = restore_claimed_output_dir(&mut job, &checkpoint); let remaining = job .article_count @@ -4171,6 +4560,19 @@ impl QueueManager { } } } + } else if was_post_processing { + // Interrupted post-processing resumes into the folder it had + // already reserved rather than reserving a second one. + let checkpoint_data = { + let db = self.db.lock(); + db.queue_load_job_data(&job_id).unwrap_or(None) + }; + if let Some(checkpoint) = checkpoint_data + .as_deref() + .and_then(|data| serde_json::from_slice::(data).ok()) + { + output_dir_claimed = restore_claimed_output_dir(&mut job, &checkpoint); + } } let paused_by_global = was_paused @@ -4193,6 +4595,7 @@ impl QueueManager { hopeless_tracker: None, download_time_secs: None, failure_code: None, + output_dir_claimed, }; self.jobs.lock().insert(job_id.clone(), state); self.job_order.lock().push(job_id); @@ -4505,11 +4908,30 @@ mod global_pause_tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } } + #[tokio::test] + async fn post_processing_level_prefers_the_job_override() { + let (manager, tempdir) = manager(); + manager.set_categories(vec![CategoryConfig { + post_processing: 2, + ..CategoryConfig::default() + }]); + let mut job = job("pp-level", JobStatus::Queued, tempdir.path()); + assert_eq!(manager.post_processing_level(&job), 2); + + job.pp_override = Some(0); + assert_eq!(manager.post_processing_level(&job), 0); + + job.pp_override = None; + job.category = "unconfigured".into(); + assert_eq!(manager.post_processing_level(&job), 3); + } + fn manager() -> (Arc, tempfile::TempDir) { let tempdir = tempfile::tempdir().expect("tempdir"); let db = Database::open_memory().expect("database"); @@ -4546,6 +4968,7 @@ mod global_pause_tests { hopeless_tracker: None, download_time_secs: None, failure_code: None, + output_dir_claimed: false, }, ); manager.job_order.lock().push(id); @@ -4883,6 +5306,30 @@ mod global_pause_tests { ); } + #[tokio::test] + async fn removing_terminal_queue_view_keeps_work_dir_owned_by_history() { + let (manager, tempdir) = manager(); + let mut terminal = job("terminal-retained", JobStatus::Failed, tempdir.path()); + terminal.error_message = Some("articles missing".into()); + let work_dir = terminal.work_dir.clone(); + insert_job(&manager, terminal); + { + let mut jobs = manager.jobs.lock(); + let state = jobs.get_mut("terminal-retained").unwrap(); + manager.move_to_history(state, Vec::new()); + } + // Stand-in for a partial download retained for history retry. + std::fs::create_dir_all(&work_dir).unwrap(); + std::fs::write(work_dir.join("partial.bin"), b"partial").unwrap(); + + manager.remove_job("terminal-retained").unwrap(); + + assert!( + work_dir.join("partial.bin").exists(), + "the history row owns a retained work dir once it is persisted" + ); + } + #[tokio::test] async fn repeated_terminal_persistence_keeps_one_history_row() { let (manager, tempdir) = manager(); @@ -5027,10 +5474,11 @@ mod global_pause_tests { let completed = job("completed-retain", JobStatus::Completed, tempdir.path()); std::fs::create_dir_all(&completed.work_dir).unwrap(); std::fs::write(completed.work_dir.join("unmoved.bin"), b"payload").unwrap(); - // A regular file at output_dir makes both rename and copy fail, so the - // work directory must be retained instead of silently losing data. - std::fs::create_dir_all(completed.output_dir.parent().unwrap()).unwrap(); - std::fs::write(&completed.output_dir, b"not a directory").unwrap(); + // A regular file in place of the complete root makes reserving the + // output folder, rename and copy all fail, so the work directory must + // be retained instead of silently losing data. (A file at output_dir + // itself is now a name collision and resolves to `.1`.) + std::fs::write(completed.output_dir.parent().unwrap(), b"not a directory").unwrap(); let work_dir = completed.work_dir.clone(); insert_job(&manager, completed); @@ -5198,6 +5646,80 @@ mod global_pause_tests { assert_eq!(history.status, JobStatus::Completed); assert!(interrupted.output_dir.join("payload.mkv").exists()); } + + #[tokio::test] + async fn interrupted_post_processing_resumes_into_its_reserved_output_dir() { + let (manager, tempdir) = manager(); + let mut interrupted = job("SameName", JobStatus::PostProcessing, tempdir.path()); + interrupted.id = "restart-claimed".to_string(); + std::fs::create_dir_all(&interrupted.work_dir).unwrap(); + std::fs::write(interrupted.work_dir.join("second.mkv"), b"second").unwrap(); + // Another job already completed into the plain name; this job had + // reserved `SameName.1` (and extracted into it) before the restart. + let foreign = interrupted.output_dir.clone(); + std::fs::create_dir_all(&foreign).unwrap(); + std::fs::write(foreign.join("first.mkv"), b"first").unwrap(); + let reserved = foreign.with_file_name("SameName.1"); + std::fs::create_dir_all(&reserved).unwrap(); + std::fs::write(reserved.join("extracted.mkv"), b"extracted").unwrap(); + interrupted.output_dir = reserved.clone(); + { + let db = manager.db.lock(); + db.queue_insert(&interrupted).unwrap(); + let checkpoint = checkpoint_for_job(&interrupted, true); + db.queue_store_job_data(&interrupted.id, &serde_json::to_vec(&checkpoint).unwrap()) + .unwrap(); + } + + manager.restore_from_db().unwrap(); + + tokio::time::timeout(Duration::from_secs(2), async { + while manager + .db + .lock() + .history_get("restart-claimed") + .unwrap() + .is_none() + { + tokio::task::yield_now().await; + } + }) + .await + .expect("recovered post-processing should reach terminal history"); + + let history = manager + .db + .lock() + .history_get("restart-claimed") + .unwrap() + .unwrap(); + assert_eq!(history.status, JobStatus::Completed); + assert_eq!(history.output_dir, reserved); + assert!(reserved.join("extracted.mkv").exists()); + assert!(reserved.join("second.mkv").exists()); + assert!(!foreign.join("second.mkv").exists()); + assert!(!foreign.with_file_name("SameName.2").exists()); + } + + #[test] + fn stored_output_claim_outside_the_job_target_is_ignored() { + let tempdir = tempfile::tempdir().unwrap(); + let mut target = job("Name", JobStatus::PostProcessing, tempdir.path()); + let elsewhere = tempdir.path().join("elsewhere").join("Name.1"); + let unrelated = target.output_dir.with_file_name("Other"); + std::fs::create_dir_all(&elsewhere).unwrap(); + std::fs::create_dir_all(&unrelated).unwrap(); + for claimed in [elsewhere, unrelated] { + let mut stored = target.clone(); + stored.output_dir = claimed; + let checkpoint = checkpoint_for_job(&stored, true); + assert!(!restore_claimed_output_dir(&mut target, &checkpoint)); + assert_eq!( + target.output_dir, + tempdir.path().join("complete").join("Name") + ); + } + } } #[cfg(test)] diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 6ca0e351..13b2562e 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -49,6 +49,12 @@ pub struct SabApiRequest { pub last_history_update: Option, pub password: Option, pub del_files: Option, + /// Job name override for `addfile`/`addurl` (SABnzbd `nzbname`). + pub nzbname: Option, + /// Post-processing override for `addfile`/`addurl` (SABnzbd `pp`, 0-3). + /// SABnzbd's `script` parameter is accepted and ignored (unknown query + /// fields are not rejected); RustNZB has no per-job scripts. + pub pp: Option, } /// Validate API key. Returns Err with JSON response on failure. @@ -124,14 +130,17 @@ pub async fn h_sabnzbd_api_get( // GET rather than a multipart POST. Route it to the same URL-fetching // logic the POST handler uses so `cat`/`priority` are honored here too. if mode == "addurl" { + // `name` (or `value`) is the URL; the job name comes from `nzbname` + // or is derived from the fetched NZB, never from the URL string. let url = req.name.clone().or_else(|| req.value.clone()); return handle_addurl( &state, url, - req.name.clone(), + req.nzbname.clone(), req.cat.clone(), req.priority.clone(), req.password.clone(), + req.pp.clone(), ) .await; } @@ -141,14 +150,16 @@ pub async fn h_sabnzbd_api_get( } /// Fetch an NZB from a URL and enqueue it, applying category/priority/password -/// overrides. Shared by the GET and POST `addurl` entry points. +/// overrides. Shared by the GET and POST `addurl` entry points. `nzbname` +/// is the optional job-name override. async fn handle_addurl( state: &AppState, url: Option, - name: Option, + nzbname: Option, cat: Option, priority: Option, password: Option, + pp: Option, ) -> Result, ApiError> { let url = url.unwrap_or_default(); @@ -196,25 +207,71 @@ async fn handle_addurl( }))); } + let content_disposition = response + .headers() + .get(reqwest::header::CONTENT_DISPOSITION) + .and_then(|value| value.to_str().ok()) + .map(str::to_string); + // Cap the fetched body to avoid unbounded memory from a hostile URL. let data = crate::fetch_guard::read_response_bytes_limited(response, MAX_ADDURL_BODY_BYTES).await?; - // Derive job name from URL filename if not provided - let job_name = name.unwrap_or_else(|| { - url.rsplit('/') - .next() - .and_then(|s| s.split('?').next()) - .unwrap_or("unknown") - .strip_suffix(".nzb") - .unwrap_or( - url.rsplit('/') - .next() - .and_then(|s| s.split('?').next()) - .unwrap_or("unknown"), - ) - .to_string() - }); + // Unpack compressed NZBs (.nzb.gz, .nzb.bz2, .zip) the way uploads are. + // A multi-NZB zip is handed to addfile, which enqueues each NZB. + let url_file_name = url + .rsplit('/') + .next() + .and_then(|s| s.split('?').next()) + .unwrap_or("unknown") + .to_string(); + let mut nzbs = match crate::nzb_archive::extract_nzbs(&url_file_name, &data) { + Ok(nzbs) => nzbs, + Err(error) => { + return Ok(Json(serde_json::json!({ + "status": false, + "error": error.to_string() + }))); + } + }; + if nzbs.len() > 1 { + return Box::pin(dispatch_post( + state, + "addfile".into(), + None, + cat, + priority, + Some((url_file_name, data)), + None, + password, + SabApiRequest { + nzbname, + pp, + ..SabApiRequest::default() + }, + )) + .await; + } + let (nzb_file_name, data) = nzbs.pop().expect("extract_nzbs returns at least one NZB"); + + // Adapted: job naming keeps our SABnzbd order (nzbname, then + // Content-Disposition, then the URL path), but when neither override nor + // disposition names the job the decompressed member is used, so a fetched + // .nzb.gz is named after the inner NZB rather than the archive. + let job_name = if nzbname.as_deref().and_then(clean_nzb_name).is_none() + && content_disposition + .as_deref() + .and_then(content_disposition_filename) + .is_none() + { + clean_nzb_name(&nzb_file_name).unwrap_or_else(|| "unknown".to_string()) + } else { + addurl_job_name( + nzbname.as_deref(), + content_disposition.as_deref(), + &fetch_plan.url, + ) + }; match nzb_parser::parse_nzb(&job_name, &data) { Ok(mut job) => { @@ -231,6 +288,7 @@ async fn handle_addurl( if let Some(ref pw) = password { job.password = Some(pw.clone()); } + job.pp_override = sab_pp_override(pp.as_deref()); let qm = &state.queue_manager; job.work_dir = qm.incomplete_dir().join(&job.id); @@ -285,6 +343,94 @@ async fn handle_addurl( } } +/// Job name for an `addurl` fetch, in SABnzbd's order of preference: the +/// `nzbname` parameter, else the `Content-Disposition` filename, else the +/// last path segment of the URL (query string excluded). The `.nzb` +/// extension is dropped. The URL itself is never used: it is not a valid +/// single path component, so every such job failed to enqueue. +fn addurl_job_name( + nzbname: Option<&str>, + content_disposition: Option<&str>, + url: &reqwest::Url, +) -> String { + nzbname + .and_then(clean_nzb_name) + .or_else(|| { + content_disposition + .and_then(content_disposition_filename) + .as_deref() + .and_then(clean_nzb_name) + }) + .or_else(|| { + url.path_segments() + .and_then(|mut segments| segments.next_back()) + .map(percent_decode_lossy) + .as_deref() + .and_then(clean_nzb_name) + }) + .unwrap_or_else(|| "unknown".to_string()) +} + +/// Reduce a client- or server-supplied NZB name to a bare job name: last +/// path component, without a `.nzb` extension. `None` if nothing is left. +fn clean_nzb_name(raw: &str) -> Option { + let base = raw.rsplit(['/', '\\']).next().unwrap_or(raw).trim(); + let name = if base.len() > 4 + && base + .get(base.len() - 4..) + .is_some_and(|suffix| suffix.eq_ignore_ascii_case(".nzb")) + { + &base[..base.len() - 4] + } else { + base + }; + let name = name.trim(); + (!name.is_empty() && name != "." && name != "..").then(|| name.to_string()) +} + +/// The filename of a `Content-Disposition` header, preferring the RFC 5987 +/// `filename*` form over plain `filename`. +fn content_disposition_filename(header: &str) -> Option { + let mut plain = None; + for part in header.split(';') { + let Some((key, value)) = part.split_once('=') else { + continue; + }; + let value = value.trim().trim_matches('"'); + match key.trim().to_ascii_lowercase().as_str() { + "filename*" => { + // charset'language'percent-encoded-name + let encoded = value.splitn(3, '\'').nth(2).unwrap_or(value); + return Some(percent_decode_lossy(encoded)); + } + "filename" => plain = Some(value.to_string()), + _ => {} + } + } + plain +} + +/// Decode `%XX` escapes, replacing invalid UTF-8 with U+FFFD. +fn percent_decode_lossy(value: &str) -> String { + let bytes = value.as_bytes(); + let mut decoded = Vec::with_capacity(bytes.len()); + let mut index = 0; + while index < bytes.len() { + if bytes[index] == b'%' + && let Some(byte) = value + .get(index + 1..index + 3) + .and_then(|hex| u8::from_str_radix(hex, 16).ok()) + { + decoded.push(byte); + index += 3; + } else { + decoded.push(bytes[index]); + index += 1; + } + } + String::from_utf8_lossy(&decoded).into_owned() +} + /// Body encodings a SABnzbd client may use for a POST request. enum SabPostBody { /// `multipart/form-data` -- the only encoding that can carry an NZB file. @@ -323,7 +469,7 @@ fn classify_post_body(request: &Request) -> SabPostBody { /// request actually says it is one. pub async fn h_sabnzbd_api_post( State(state): State>, - Query(query_req): Query, + Query(mut query_req): Query, request: Request, ) -> Result { // Query-string parameters are the baseline; body fields override them. @@ -365,6 +511,12 @@ pub async fn h_sabnzbd_api_post( if let Some(pw) = form.password.filter(|pw| !pw.is_empty()) { password = Some(pw); } + if form.nzbname.is_some() { + query_req.nzbname = form.nzbname; + } + if form.pp.is_some() { + query_req.pp = form.pp; + } } SabPostBody::Multipart => { let mut multipart = Multipart::from_request(request, &()).await.map_err(|e| { @@ -380,6 +532,8 @@ pub async fn h_sabnzbd_api_post( &mut nzb_data, &mut nzb_url, &mut password, + &mut query_req.nzbname, + &mut query_req.pp, ) .await?; } @@ -412,6 +566,8 @@ async fn read_multipart_fields( nzb_data: &mut Option<(String, Vec)>, nzb_url: &mut Option, password: &mut Option, + nzbname: &mut Option, + pp: &mut Option, ) -> Result<(), ApiError> { while let Some(field) = multipart .next_field() @@ -483,6 +639,16 @@ async fn read_multipart_fields( *password = Some(text); } } + "nzbname" => { + if let Ok(text) = field.text().await { + *nzbname = Some(text); + } + } + "pp" => { + if let Ok(text) = field.text().await { + *pp = Some(text); + } + } _ => { let _ = field.bytes().await; } @@ -492,7 +658,9 @@ async fn read_multipart_fields( } /// Dispatch a POST request once its parameters have been assembled from the -/// query string and (optional) body. +/// query string and (optional) body. `query_req.nzbname` carries the merged +/// `nzbname` job-name override and `query_req.pp` the merged `pp` +/// post-processing override. #[allow(clippy::too_many_arguments)] async fn dispatch_post( state: &AppState, @@ -517,12 +685,43 @@ async fn dispatch_post( } }; - let job_name = name.clone().unwrap_or_else(|| { - file_name - .strip_suffix(".nzb") - .unwrap_or(&file_name) - .to_string() - }); + // Unpack compressed uploads (.nzb.gz, .nzb.bz2, .zip) with the + // same code and limits as the native add endpoint. + let mut nzbs = match crate::nzb_archive::extract_nzbs(&file_name, &data) { + Ok(nzbs) => nzbs, + Err(error) => { + return Ok(Json(serde_json::json!({ + "status": false, + "error": error.to_string() + }))); + } + }; + if nzbs.len() > 1 { + return add_each_nzb( + state, + nzbs, + cat, + priority, + password, + query_req.nzbname.clone(), + query_req.pp.clone(), + ) + .await; + } + let (entry_name, data) = nzbs.pop().expect("extract_nzbs returns at least one NZB"); + let file_name = nzb_entry_file_name(&entry_name); + + let job_name = query_req + .nzbname + .as_deref() + .and_then(clean_nzb_name) + .or_else(|| name.clone()) + .unwrap_or_else(|| { + file_name + .strip_suffix(".nzb") + .unwrap_or(&file_name) + .to_string() + }); match nzb_parser::parse_nzb(&job_name, &data) { Ok(mut job) => { @@ -539,6 +738,7 @@ async fn dispatch_post( if let Some(ref pw) = password { job.password = Some(pw.clone()); } + job.pp_override = sab_pp_override(query_req.pp.as_deref()); let qm = &state.queue_manager; job.work_dir = qm.incomplete_dir().join(&job.id); @@ -599,8 +799,18 @@ async fn dispatch_post( } "addurl" => { + // `value`/`url` (or `name`) is the URL to fetch, not a job name. let url = nzb_url.or_else(|| name.clone()); - handle_addurl(state, url, name, cat, priority, password).await + handle_addurl( + state, + url, + query_req.nzbname, + cat, + priority, + password, + query_req.pp, + ) + .await } _ => { @@ -628,6 +838,8 @@ async fn dispatch_post( last_history_update: query_req.last_history_update, password, del_files: query_req.del_files, + nzbname: query_req.nzbname, + pp: query_req.pp, }; Ok(dispatch_mode( state, @@ -638,6 +850,72 @@ async fn dispatch_post( } } +/// Zip entries may sit in folders; a job is named after the file alone. +fn nzb_entry_file_name(entry_name: &str) -> String { + entry_name + .rsplit(['/', '\\']) + .next() + .unwrap_or(entry_name) + .to_string() +} + +/// Enqueue every NZB of a multi-NZB archive as its own job (SABnzbd adds +/// each NZB in a zip separately), reporting all resulting nzo_ids. +async fn add_each_nzb( + state: &AppState, + nzbs: Vec<(String, Vec)>, + cat: Option, + priority: Option, + password: Option, + nzbname: Option, + pp: Option, +) -> Result, ApiError> { + let mut nzo_ids = Vec::new(); + let mut errors = Vec::new(); + for (entry_name, data) in nzbs { + let file_name = nzb_entry_file_name(&entry_name); + let response = Box::pin(dispatch_post( + state, + "addfile".into(), + None, + cat.clone(), + priority.clone(), + Some((file_name, data)), + None, + password.clone(), + SabApiRequest { + // Only the first member keeps the explicit nzbname; the rest + // are named after their own files. + nzbname: if nzo_ids.is_empty() { + nzbname.clone() + } else { + None + }, + pp: pp.clone(), + ..SabApiRequest::default() + }, + )) + .await? + .0; + match response["nzo_ids"].as_array() { + Some(ids) => nzo_ids.extend(ids.iter().cloned()), + None => errors.push(format!( + "{entry_name}: {}", + response["error"].as_str().unwrap_or("failed to add") + )), + } + } + if nzo_ids.is_empty() { + return Ok(Json(serde_json::json!({ + "status": false, + "error": errors.join("; ") + }))); + } + Ok(Json( + serde_json::json!({ "status": true, "nzo_ids": nzo_ids }), + )) +} + /// Dispatch an API mode to the appropriate handler. fn dispatch_mode(state: &AppState, mode: &str, req: &SabApiRequest) -> Json { match mode { @@ -674,7 +952,9 @@ fn dispatch_mode(state: &AppState, mode: &str, req: &SabApiRequest) -> Json handle_priority(state, req), - "fullstatus" | "server_stats" => handle_fullstatus(state), + "fullstatus" => handle_fullstatus(state), + + "server_stats" => handle_server_stats(state), "pause" => handle_pause(state, req), @@ -684,6 +964,15 @@ fn dispatch_mode(state: &AppState, mode: &str, req: &SabApiRequest) -> Json handle_retry(state, req), + // SABnzbd `_api_warnings`. RustNZB does not keep a SABnzbd-style + // warnings list (queue/fullstatus report `have_warnings: "0"` and + // `warnings: []`), so this is the same empty list; `name=clear` + // is accepted. + "warnings" => match req.name.as_deref() { + Some("clear") => Json(serde_json::json!({ "status": true })), + _ => Json(serde_json::json!({ "warnings": Vec::::new() })), + }, + _ => Json(serde_json::json!({ "status": false, "error": format!("Unknown mode: {mode}") @@ -796,6 +1085,55 @@ fn handle_fullstatus(state: &AppState) -> Json { })) } +/// `mode=server_stats`, in SABnzbd's shape (`api.py::_api_server_stats`): +/// downloaded-byte totals overall and per server, keyed by server name. +/// The data is the same as the native `GET /api/config/servers/stats`. +/// RustNZB keeps rolling 1/7/30-day windows rather than calendar ones, and +/// no per-day timeline, so `daily` is empty. +fn handle_server_stats(state: &AppState) -> Json { + let config = state.config(); + let stats = state.queue_manager.server_stats_get_all(&config.servers); + + let mut servers = serde_json::Map::new(); + let (mut total, mut month, mut week, mut day) = (0_u64, 0_u64, 0_u64, 0_u64); + for server in &stats { + total = total.saturating_add(server.total_bytes); + month = month.saturating_add(server.month_bytes); + week = week.saturating_add(server.week_bytes); + day = day.saturating_add(server.today_bytes); + + let mut key = if server.server_name.is_empty() { + server.server_id.clone() + } else { + server.server_name.clone() + }; + if servers.contains_key(&key) { + // Two servers share a display name; keep both, keyed apart. + key = format!("{key} ({})", server.server_id); + } + servers.insert( + key, + serde_json::json!({ + "total": server.total_bytes, + "month": server.month_bytes, + "week": server.week_bytes, + "day": server.today_bytes, + "daily": serde_json::Map::new(), + "articles_tried": server.total_ok.saturating_add(server.total_fail), + "articles_success": server.total_ok, + }), + ); + } + + Json(serde_json::json!({ + "total": total, + "month": month, + "week": week, + "day": day, + "servers": servers, + })) +} + // --------------------------------------------------------------------------- // Mode handlers // --------------------------------------------------------------------------- @@ -2023,6 +2361,21 @@ fn parse_sab_units(value: &str) -> Option { .map(|amount| amount * multiplier) } +/// Map SABnzbd's `pp` add parameter onto RustNZB's post-processing level. +/// SABnzbd's scale is cumulative (0=download only, 1=+repair, +/// 2=+repair/unpack, 3=+repair/unpack/delete); RustNZB's is 0=none, +/// 1=repair, 2=unpack only, 3=repair+unpack, with source cleanup governed +/// separately. So SABnzbd 2 and 3 both map to 3. Anything else (absent, +/// `-1`/default, out of range) leaves the category's setting in force. +fn sab_pp_override(pp: Option<&str>) -> Option { + match pp?.trim().parse::().ok()? { + 0 => Some(0), + 1 => Some(1), + 2 | 3 => Some(3), + _ => None, + } +} + /// Convert arr-protocol priority string to our Priority enum. fn sab_priority_to_priority(s: &str) -> Priority { match s.trim() { @@ -2396,6 +2749,21 @@ mod tests { const HISTORY_GOLDEN: &str = include_str!("../tests/fixtures/sabnzbd-5.0.4/history.json"); const FULLSTATUS_GOLDEN: &str = include_str!("../tests/fixtures/sabnzbd-5.0.4/fullstatus.json"); + #[test] + fn clean_nzb_name_does_not_panic_on_non_ascii() { + // The last four bytes of each fall inside the final multibyte + // character, so a byte slice at `len - 4` panics. + for name in ["Amélie", "Pokémon", "Amélie.nzb", "Pokémon.nzb"] { + let cleaned = clean_nzb_name(name); + assert!(cleaned.is_some(), "{name}"); + assert!( + !cleaned.unwrap().to_ascii_lowercase().ends_with(".nzb"), + "{name}" + ); + } + assert_eq!(clean_nzb_name("Show.nzb").as_deref(), Some("Show")); + } + struct TestState { state: AppState, _tempdir: tempfile::TempDir, @@ -2466,6 +2834,7 @@ mod tests { password: Some("secret".into()), error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } @@ -2526,6 +2895,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } @@ -2950,6 +3320,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), }; @@ -3078,6 +3449,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), }; @@ -3109,6 +3481,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), }; @@ -3559,6 +3932,152 @@ mod tests { .category } + fn gzip(data: &[u8]) -> Vec { + use std::io::Write as _; + let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + encoder.write_all(data).unwrap(); + encoder.finish().unwrap() + } + + fn bzip2(data: &[u8]) -> Vec { + use std::io::Write as _; + let mut encoder = bzip2::write::BzEncoder::new(Vec::new(), bzip2::Compression::default()); + encoder.write_all(data).unwrap(); + encoder.finish().unwrap() + } + + fn zip_of(entries: &[(&str, &[u8])]) -> Vec { + use std::io::Write as _; + let mut writer = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); + for (name, data) in entries { + writer + .start_file(*name, zip::write::SimpleFileOptions::default()) + .unwrap(); + writer.write_all(data).unwrap(); + } + writer.finish().unwrap().into_inner() + } + + async fn addfile_upload( + test_state: &TestState, + file_name: &str, + data: Vec, + ) -> serde_json::Value { + dispatch_post( + &test_state.state, + "addfile".into(), + None, + None, + None, + Some((file_name.into(), data)), + None, + None, + SabApiRequest::default(), + ) + .await + .expect("addfile response") + .0 + } + + fn queued_names(test_state: &TestState) -> Vec { + let mut names: Vec = test_state + .state + .queue_manager + .get_jobs() + .into_iter() + .map(|job| job.name) + .collect(); + names.sort(); + names + } + + /// `addfile` accepts the same compressed uploads as the native add + /// endpoint: `.nzb.gz`, `.nzb.bz2` and `.zip` (one job per NZB inside). + #[tokio::test] + async fn addfile_accepts_compressed_nzbs() { + let test_state = test_state(); + let gz = addfile_upload(&test_state, "Gz.Show.nzb.gz", gzip(SAMPLE_NZB.as_bytes())).await; + assert_eq!(gz["status"], serde_json::json!(true), "gz: {gz}"); + let bz = addfile_upload(&test_state, "Bz.Show.nzb.bz2", bzip2(SAMPLE_NZB.as_bytes())).await; + assert_eq!(bz["status"], serde_json::json!(true), "bz2: {bz}"); + assert_eq!(queued_names(&test_state), vec!["Bz.Show", "Gz.Show"]); + + let test_state = self::test_state(); + let archive = zip_of(&[ + ("First.nzb", SAMPLE_NZB.as_bytes()), + ("folder/Second.nzb", SAMPLE_NZB.as_bytes()), + ("readme.txt", b"not an nzb"), + ]); + let zip = addfile_upload(&test_state, "pack.zip", archive).await; + assert_eq!(zip["status"], serde_json::json!(true), "zip: {zip}"); + assert_eq!(zip["nzo_ids"].as_array().unwrap().len(), 2); + assert_eq!(queued_names(&test_state), vec!["First", "Second"]); + + let broken = + addfile_upload(&self::test_state(), "broken.nzb.gz", b"not gzip".to_vec()).await; + assert_eq!(broken["status"], serde_json::json!(false)); + assert!(broken["error"].is_string()); + } + + /// Serves `body` once on a loopback port at `path`. + async fn spawn_bytes_server(body: Vec, path: &str) -> String { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral test server"); + let addr = listener.local_addr().expect("test server local addr"); + tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.expect("accept test connection"); + let mut buf = [0u8; 1024]; + let _ = socket.read(&mut buf).await; + let mut response = format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ) + .into_bytes(); + response.extend_from_slice(&body); + let _ = socket.write_all(&response).await; + let _ = socket.shutdown().await; + }); + format!("http://{addr}{path}") + } + + /// `addurl` unpacks a fetched `.nzb.gz` the same way. + /// + /// Adapted: loopback is admitted through the test state's + /// `fetch_allowed_hosts` rather than the upstream cfg(test) task-local + /// seam, which this tree does not have. + #[tokio::test] + async fn addurl_accepts_gzipped_nzb() { + let test_state = test_state(); + let mut config = (*test_state.state.config()).clone(); + config.general.fetch_allowed_hosts = vec!["127.0.0.1".into()]; + test_state.state.config.store(Arc::new(config)); + + let url = spawn_bytes_server(gzip(SAMPLE_NZB.as_bytes()), "/Url.Show.nzb.gz").await; + let response = dispatch_post( + &test_state.state, + "addurl".into(), + None, + None, + None, + None, + Some(url), + None, + SabApiRequest::default(), + ) + .await + .expect("addurl response") + .0; + assert_eq!( + response["status"], + serde_json::json!(true), + "resp={response}" + ); + assert_eq!(queued_names(&test_state), vec!["Url.Show"]); + } + /// SABnzbd falls back to the default category when `addfile`/`addurl` /// name a category that is not configured, and matches configured /// category names case-insensitively. @@ -3768,6 +4287,137 @@ mod tests { assert_eq!(format_sab_age(2 * 86_400 + 5), "2d"); } + /// Run a multipart `mode=addfile` with `query` as the query string and + /// `fields` as extra text fields, returning the queued job. + async fn addfile_multipart(query: SabApiRequest, fields: &[(&str, &str)]) -> NzbJob { + let TestState { state, _tempdir } = test_state(); + let state = Arc::new(state); + let boundary = "sabboundary"; + let mut body = format!( + "--{boundary}\r\nContent-Disposition: form-data; name=\"mode\"\r\n\r\naddfile\r\n" + ); + for (name, value) in fields { + body.push_str(&format!( + "--{boundary}\r\nContent-Disposition: form-data; name=\"{name}\"\r\n\r\n{value}\r\n" + )); + } + body.push_str(&format!( + "--{boundary}\r\nContent-Disposition: form-data; name=\"name\"; filename=\"upload.nzb\"\r\nContent-Type: application/x-nzb\r\n\r\n{SAMPLE_NZB}\r\n--{boundary}--\r\n" + )); + let query = SabApiRequest { + apikey: Some("contract-api-key".into()), + ..query + }; + let request = Request::builder() + .method("POST") + .uri("/sabnzbd/api") + .header( + CONTENT_TYPE, + format!("multipart/form-data; boundary={boundary}"), + ) + .body(axum::body::Body::from(body)) + .expect("build request"); + let response = h_sabnzbd_api_post(State(state.clone()), Query(query), request) + .await + .expect("addfile over multipart") + .into_response(); + let value = json_body(response).await; + assert_eq!(value["status"], serde_json::json!(true), "resp={value}"); + let mut jobs = state.queue_manager.get_jobs(); + assert_eq!(jobs.len(), 1); + jobs.remove(0) + } + + /// SABnzbd's `pp` (0-3) overrides the category's post-processing for + /// the added job; `script` is accepted and ignored. + #[tokio::test] + async fn addfile_applies_pp_override_and_ignores_script() { + let from_query = addfile_multipart( + SabApiRequest { + pp: Some("1".into()), + ..SabApiRequest::default() + }, + &[("script", "Notify.py")], + ) + .await; + assert_eq!(from_query.pp_override, Some(1)); + + let from_field = addfile_multipart(SabApiRequest::default(), &[("pp", "0")]).await; + assert_eq!(from_field.pp_override, Some(0)); + + // SABnzbd's cumulative 2 (+repair/unpack) is RustNZB's 3. + let repair_unpack = addfile_multipart(SabApiRequest::default(), &[("pp", "2")]).await; + assert_eq!(repair_unpack.pp_override, Some(3)); + + let invalid = addfile_multipart(SabApiRequest::default(), &[("pp", "7")]).await; + assert_eq!(invalid.pp_override, None); + + let absent = addfile_multipart(SabApiRequest::default(), &[]).await; + assert_eq!(absent.pp_override, None); + } + + /// `mode=server_stats` has its own shape in SABnzbd + /// (`api.py::_api_server_stats`): byte totals plus a per-server map, not + /// the fullstatus envelope. + #[tokio::test] + async fn server_stats_uses_sabnzbd_shape() { + let test_state = test_state(); + let mut config = (*test_state.state.config()).clone(); + let mut server = crate::nzb_core::config::ServerConfig::new("srv-1", "news.example.com"); + server.name = "Primary".into(); + config.servers = vec![server]; + test_state.state.config.store(Arc::new(config)); + + let mut entry = history_entry("stats-job", "Stats Job", "tv", JobStatus::Completed, 60); + entry.server_stats = vec![ServerArticleStats { + server_id: "srv-1".into(), + server_name: "Primary".into(), + articles_downloaded: 9, + articles_failed: 1, + bytes_downloaded: 4096, + }]; + test_state + .state + .queue_manager + .with_db(|database| database.history_insert(&entry).expect("insert history")); + + let stats = dispatch_mode(&test_state.state, "server_stats", &SabApiRequest::default()).0; + assert!( + stats.get("status").is_none(), + "not the fullstatus envelope: {stats}" + ); + for field in ["total", "month", "week", "day"] { + assert_eq!(stats[field], 4096, "{field}"); + } + let primary = &stats["servers"]["Primary"]; + for field in ["total", "month", "week", "day"] { + assert_eq!(primary[field], 4096, "Primary.{field}"); + } + assert!(primary["daily"].is_object()); + assert_eq!(primary["articles_tried"], 10); + assert_eq!(primary["articles_success"], 9); + } + + /// `mode=warnings` is a real SABnzbd mode (`api.py::_api_warnings`); + /// it must answer with a `warnings` list, consistent with fullstatus, + /// and `name=clear` must succeed. + #[tokio::test] + async fn warnings_mode_matches_fullstatus_warnings() { + let test_state = test_state(); + let warnings = dispatch_mode(&test_state.state, "warnings", &SabApiRequest::default()).0; + let fullstatus = + dispatch_mode(&test_state.state, "fullstatus", &SabApiRequest::default()).0; + assert!(warnings["warnings"].is_array(), "resp={warnings}"); + assert_eq!(warnings["warnings"], fullstatus["status"]["warnings"]); + + let clear = SabApiRequest { + name: Some("clear".into()), + ..SabApiRequest::default() + }; + let cleared = dispatch_mode(&test_state.state, "warnings", &clear).0; + assert_eq!(cleared, serde_json::json!({ "status": true })); + } + /// SABnzbd's real `_api_queue_delete` accepts a comma-separated `value` /// list, removing every matching job in one call. #[tokio::test] @@ -3844,6 +4494,33 @@ mod tests { assert!(!output_dir.exists()); } + /// A failed job keeps its partial download in `incomplete/` for retry. + /// Deleting its history entry, with or without `del_files`, must free it. + #[tokio::test] + async fn history_delete_removes_retained_incomplete_work_dir() { + let test_state = test_state(); + let incomplete = test_state.state.queue_manager.incomplete_dir(); + for (id, del_files) in [("sab-failed-keep", None), ("sab-failed-del", Some("1"))] { + insert_history_status(&test_state, id, JobStatus::Failed, 10); + let work_dir = incomplete.join(id); + std::fs::create_dir_all(&work_dir).expect("create retained work dir"); + std::fs::write(work_dir.join("partial.rar"), b"partial").expect("write partial"); + + let req = SabApiRequest { + value: Some(id.into()), + del_files: del_files.map(Into::into), + ..SabApiRequest::default() + }; + let response = handle_history_delete(&test_state.state, &req).0; + assert_eq!(response["status"], serde_json::json!(true)); + assert!( + !work_dir.exists(), + "{id}: retained work dir must be removed" + ); + } + assert!(incomplete.is_dir(), "the incomplete root itself is kept"); + } + fn insert_history_status(test_state: &TestState, id: &str, status: JobStatus, age_secs: i64) { let mut entry = history_entry(id, id, "tv", status, age_secs); entry.output_dir = test_state.state.config().general.complete_dir.join(id); @@ -4029,6 +4706,15 @@ mod tests { /// Serves `body` once over a raw TCP listener bound to an ephemeral /// port, returning the URL to fetch it from. async fn spawn_nzb_server(body: &'static str) -> String { + spawn_nzb_server_with_headers(body, "").await + } + + /// Like [`spawn_nzb_server`], adding `extra_headers` (each line ending + /// in `\r\n`) to the response. + async fn spawn_nzb_server_with_headers( + body: &'static str, + extra_headers: &'static str, + ) -> String { use tokio::io::{AsyncReadExt, AsyncWriteExt}; let listener = tokio::net::TcpListener::bind("127.0.0.1:0") @@ -4041,7 +4727,7 @@ mod tests { let mut buf = [0u8; 1024]; let _ = socket.read(&mut buf).await; let response = format!( - "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nContent-Type: application/x-nzb\r\nConnection: close\r\n\r\n{}", + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nContent-Type: application/x-nzb\r\n{extra_headers}Connection: close\r\n\r\n{}", body.len(), body ); @@ -4205,6 +4891,160 @@ mod tests { assert_eq!(value["version"], serde_json::json!(SABNZBD_COMPAT_VERSION)); } + /// Run a GET `mode=addurl` against a loopback fixture (admitted by the + /// test state's explicit `fetch_allowed_hosts` entry) and return the + /// response and the names of the queued jobs. + async fn addurl_over_get( + url: String, + nzbname: Option<&str>, + ) -> (serde_json::Value, Vec) { + let TestState { state, _tempdir } = test_state(); + // Loopback is not a private LAN address; admit the fixture only here, + // through an explicit entry, instead of relaxing the guard globally. + { + let mut config = (*state.config()).clone(); + config.general.fetch_allowed_hosts = vec!["127.0.0.1".into()]; + state.config.store(std::sync::Arc::new(config)); + } + let state = Arc::new(state); + let req = SabApiRequest { + mode: Some("addurl".into()), + name: Some(url), + nzbname: nzbname.map(str::to_string), + apikey: Some("contract-api-key".into()), + ..SabApiRequest::default() + }; + let response = h_sabnzbd_api_get(State(state.clone()), Query(req)) + .await + .expect("addurl over GET") + .into_response(); + let value = json_body(response).await; + let names = state + .queue_manager + .get_jobs() + .into_iter() + .map(|job| job.name) + .collect(); + (value, names) + } + + /// `mode=addurl&name=` used the whole URL as the job name, which + /// `output_dir_for` rejects, so every successful fetch failed to + /// enqueue. The name comes from the URL's last path segment instead, + /// without `.nzb` or the query string. + #[tokio::test] + async fn addurl_names_job_from_url_path_not_the_url() { + let url = spawn_nzb_server(SAMPLE_NZB).await; + let (response, names) = addurl_over_get(format!("{url}?apikey=x&t=get"), None).await; + assert_eq!( + response["status"], + serde_json::json!(true), + "resp={response}" + ); + assert_eq!(names, vec!["test".to_string()]); + } + + #[tokio::test] + async fn addurl_honours_nzbname() { + let url = spawn_nzb_server(SAMPLE_NZB).await; + let (response, names) = addurl_over_get(url, Some("My.Show.S01E01")).await; + assert_eq!( + response["status"], + serde_json::json!(true), + "resp={response}" + ); + assert_eq!(names, vec!["My.Show.S01E01".to_string()]); + } + + #[tokio::test] + async fn addurl_uses_content_disposition_filename() { + let url = spawn_nzb_server_with_headers( + SAMPLE_NZB, + "Content-Disposition: attachment; filename=\"Some.Release.nzb\"\r\n", + ) + .await; + let (response, names) = addurl_over_get(url, None).await; + assert_eq!( + response["status"], + serde_json::json!(true), + "resp={response}" + ); + assert_eq!(names, vec!["Some.Release".to_string()]); + } + + #[test] + fn addurl_job_name_sources_in_sabnzbd_order() { + let url = reqwest::Url::parse("https://indexer.example/get/My%20File.nzb?id=1").unwrap(); + assert_eq!(addurl_job_name(None, None, &url), "My File"); + assert_eq!( + addurl_job_name( + None, + Some("attachment; filename*=UTF-8''Caf%C3%A9.nzb"), + &url + ), + "Café" + ); + assert_eq!( + addurl_job_name(Some("Chosen.nzb"), Some("attachment; filename=x.nzb"), &url), + "Chosen" + ); + let bare = reqwest::Url::parse("https://indexer.example/").unwrap(); + assert!(!addurl_job_name(None, None, &bare).contains('/')); + } + + /// `nzbname` also overrides the job name for uploads, from the query + /// string, a urlencoded body or a multipart field. + #[tokio::test] + async fn addfile_honours_nzbname_from_query_and_multipart() { + for (query_name, field_name, expected) in [ + (Some("From.Query"), None, "From.Query"), + (None, Some("From.Field"), "From.Field"), + ] { + let TestState { state, _tempdir } = test_state(); + let state = Arc::new(state); + let boundary = "sabboundary"; + let mut body = String::new(); + body.push_str(&format!( + "--{boundary}\r\nContent-Disposition: form-data; name=\"mode\"\r\n\r\naddfile\r\n" + )); + if let Some(field) = field_name { + body.push_str(&format!( + "--{boundary}\r\nContent-Disposition: form-data; name=\"nzbname\"\r\n\r\n{field}\r\n" + )); + } + body.push_str(&format!( + "--{boundary}\r\nContent-Disposition: form-data; name=\"name\"; filename=\"upload.nzb\"\r\nContent-Type: application/x-nzb\r\n\r\n{SAMPLE_NZB}\r\n--{boundary}--\r\n" + )); + let req = SabApiRequest { + apikey: Some("contract-api-key".into()), + nzbname: query_name.map(str::to_string), + ..SabApiRequest::default() + }; + let request = Request::builder() + .method("POST") + .uri("/sabnzbd/api") + .header( + CONTENT_TYPE, + format!("multipart/form-data; boundary={boundary}"), + ) + .body(axum::body::Body::from(body)) + .expect("build request"); + let response = h_sabnzbd_api_post(State(state.clone()), Query(req), request) + .await + .expect("addfile over multipart") + .into_response(); + let value = json_body(response).await; + assert_eq!(value["status"], serde_json::json!(true), "resp={value}"); + let names: Vec = state + .queue_manager + .get_jobs() + .into_iter() + .map(|job| job.name) + .collect(); + assert_eq!(names, vec![expected.to_string()]); + } + } + /// Non-upload modes must also work over a bare POST. #[tokio::test] async fn version_over_bare_post_dispatches() { diff --git a/crates/nzb-web/src/state.rs b/crates/nzb-web/src/state.rs index 6344cce2..02a5dee2 100644 --- a/crates/nzb-web/src/state.rs +++ b/crates/nzb-web/src/state.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use crate::nzb_core::config::AppConfig; use arc_swap::ArcSwap; +use parking_lot::Mutex; use crate::auth::{CredentialStore, TokenStore}; use crate::log_buffer::LogBuffer; @@ -19,6 +20,9 @@ pub struct AppState { /// When the application state was created at startup; the basis for the /// SABnzbd-compatible `uptime`. pub started_at: std::time::Instant, + /// Serialises config writers so a read-modify-write cycle (including + /// persisting the TOML) cannot interleave with another one. + config_write: Mutex<()>, } impl AppState { @@ -38,6 +42,7 @@ impl AppState { token_store, credential_store, started_at: std::time::Instant::now(), + config_write: Mutex::new(()), } } @@ -46,8 +51,37 @@ impl AppState { self.config.load_full() } - /// Update config in memory and save to file. + /// Replace the whole config in memory and on disk. + /// + /// This is a blind write: anything another writer changed since `config` + /// was read is lost. Handlers that modify part of the config must use + /// [`AppState::update_config_with`] instead. pub fn update_config(&self, config: AppConfig) -> anyhow::Result<()> { + let _guard = self.config_write.lock(); + self.commit_config(config) + } + + /// Atomically read, modify and persist the config. + /// + /// `f` runs on a copy of the latest config while holding the config write + /// lock, which stays held until the TOML is saved and the new config is + /// published, so concurrent updates cannot overwrite each other. If `f` + /// returns an error nothing is written. + pub fn update_config_with( + &self, + f: impl FnOnce(&mut AppConfig) -> Result, + ) -> Result + where + E: From, + { + let _guard = self.config_write.lock(); + let mut config = (*self.config.load_full()).clone(); + let result = f(&mut config)?; + self.commit_config(config)?; + Ok(result) + } + + fn commit_config(&self, config: AppConfig) -> anyhow::Result<()> { config.save(&self.config_path)?; self.config.store(Arc::new(config)); Ok(()) diff --git a/crates/nzb-web/tests/harness_article_errors.rs b/crates/nzb-web/tests/harness_article_errors.rs index 12362bef..6eda8880 100644 --- a/crates/nzb-web/tests/harness_article_errors.rs +++ b/crates/nzb-web/tests/harness_article_errors.rs @@ -78,3 +78,86 @@ async fn article_errors_fail_over_serial() { async fn article_errors_fail_over_pipelined() { run_with_pipelining("article-errors-pipe", 4).await; } + +/// Pipelined article-level errors (here an unexpected 451) must not count +/// against the server's circuit breaker. When several workers each hit one +/// before any of them reconnects, three such answers used to trip the breaker +/// and pause the only server for 30 s, stalling every other article. +#[tokio::test] +async fn pipelined_article_errors_do_not_trip_circuit_breaker() { + const CONNECTIONS: u16 = 3; + const PIPELINING: u8 = 2; + // Enough rejected articles at the front of the queue that every worker's + // first pipelined batch is all 451s, so all workers report an error + // before any of their reconnects completes. + const REJECTED_ARTICLES: usize = CONNECTIONS as usize * PIPELINING as usize; + const OK_ARTICLES: usize = 12; + let name = "article-errors-breaker"; + let rejected_ids = (0..REJECTED_ARTICLES) + .map(|i| format!("{name}-451-{i}")) + .collect::>(); + let ok_ids = (0..OK_ARTICLES) + .map(|i| format!("{name}-ok-{i}")) + .collect::>(); + let mut segments: Vec<(&str, &[u8])> = rejected_ids + .iter() + .map(|id| (id.as_str(), b"rejected".as_slice())) + .collect(); + segments.extend(ok_ids.iter().map(|id| (id.as_str(), b"ok-body".as_slice()))); + let fixture = NzbFixture::new(name) + .add_file("payload.bin", &segments) + .build(); + let triples = fixture + .articles + .iter() + .map(|(id, bytes, file)| (*id, *bytes, file.as_str())) + .collect::>(); + let overrides = rejected_ids + .iter() + .map(|id| (id.clone(), 451)) + .collect::>(); + let mut server = ServerProfile::start( + name, + MockConfig { + articles: yenc_articles(&triples), + article_response_overrides: overrides, + // A reconnect (banner + CAPABILITIES) takes several delayed + // writes, longer than the worker start-up stagger, so the workers' + // first errors land before any reconnect resets the failure count. + response_delay: Some(Duration::from_millis(80)), + ..MockConfig::default() + }, + CONNECTIONS, + ) + .await; + server.config.pipelining = PIPELINING; + let engine = HarnessBuilder::new() + .with_server(server) + .article_timeout(120) + .abort_hopeless(false) + .early_failure_check(false) + .build(); + let job_id = engine.submit_nzb_xml(name, fixture.xml).expect("submit"); + + // Under the 30 s transient circuit-breaker cooldown, so a tripped breaker + // fails the test. + assert!( + engine + .wait_for(Duration::from_secs(25), |snapshot| { + snapshot.job(&job_id).is_none_or(|job| { + job.articles_downloaded == OK_ARTICLES + && job.articles_failed == REJECTED_ARTICLES + }) + }) + .await, + "pipelined 451s paused the server: {:?}", + engine.job(&job_id) + ); + match engine.job(&job_id) { + Some(job) => { + assert_eq!(job.articles_downloaded, OK_ARTICLES); + assert_eq!(job.articles_failed, REJECTED_ARTICLES); + } + None => assert!(engine.history_status(&job_id).is_some()), + } +} diff --git a/crates/nzb-web/tests/harness_incomplete_cleanup.rs b/crates/nzb-web/tests/harness_incomplete_cleanup.rs new file mode 100644 index 00000000..f5b92f61 --- /dev/null +++ b/crates/nzb-web/tests/harness_incomplete_cleanup.rs @@ -0,0 +1,301 @@ +//! Retained incomplete work directories must not leak forever. +//! +//! A failed job keeps its partial download in `incomplete/` so a history +//! retry can reuse it. Deleting that history entry, removing the queue job, +//! or restarting with directories nothing references must reclaim the space, +//! without ever reaching outside the incomplete root. + +mod harness; + +use std::path::{Path, PathBuf}; +use std::time::Duration; + +use harness::nzb_fixture::NzbFixture; +use harness::{HarnessBuilder, ServerProfile, TestEngine, yenc_articles}; +use nzb_nntp::testutil::MockConfig; +use nzb_web::nzb_core::db::Database; +use nzb_web::nzb_core::models::{HistoryEntry, JobFailureCode, JobStatus, NzbJob}; +use nzb_web::nzb_core::nzb_parser; + +/// Start an engine whose provider serves only the first of two files, so a +/// submitted job fails after writing part of its payload. +async fn failing_engine(tag: &str) -> (TestEngine, Vec) { + let present = format!("{tag}-present@test"); + let missing = format!("{tag}-missing@test"); + let fixture = NzbFixture::new(tag) + .add_file("present.bin", &[(present.as_str(), b"partial payload")]) + .add_file("missing.bin", &[(missing.as_str(), b"never served")]) + .build(); + let triples: Vec<(&str, &[u8], &str)> = fixture + .articles + .iter() + .filter(|(m, _, _)| *m == present.as_str()) + .map(|(m, b, f)| (*m, *b, f.as_str())) + .collect(); + let server = ServerProfile::start( + tag, + MockConfig { + articles: yenc_articles(&triples), + ..Default::default() + }, + 2, + ) + .await; + let engine = HarnessBuilder::new() + .with_server(server) + .article_timeout(5) + .build(); + (engine, fixture.xml) +} + +async fn submit_and_fail(engine: &TestEngine, xml: Vec) -> (String, PathBuf) { + let id = engine.submit_nzb_xml("partial", xml).expect("submit"); + assert!( + engine + .wait_for_status(&id, Duration::from_secs(20), &[JobStatus::Failed]) + .await, + "job did not fail" + ); + let work_dir = engine.incomplete_dir.join(&id); + assert!( + work_dir.join("present.bin").exists(), + "a failed job keeps its partial download for retry" + ); + (id, work_dir) +} + +#[tokio::test] +async fn deleting_failed_history_entry_removes_retained_work_dir() { + let (engine, xml) = failing_engine("hist-del").await; + let (id, work_dir) = submit_and_fail(&engine, xml).await; + + engine.queue_manager.history_remove(&id).unwrap(); + + assert!( + !work_dir.exists(), + "deleting the history entry must remove its retained work dir" + ); + assert!(engine.incomplete_dir.is_dir(), "incomplete root survives"); +} + +#[tokio::test] +async fn clearing_history_removes_retained_work_dirs() { + let (engine, xml) = failing_engine("hist-clear").await; + let (_id, work_dir) = submit_and_fail(&engine, xml).await; + + engine.queue_manager.history_clear().unwrap(); + + assert!(!work_dir.exists()); + assert!(engine.incomplete_dir.is_dir()); +} + +#[tokio::test] +async fn history_delete_keeps_work_dir_a_queued_retry_is_using() { + let (engine, xml) = failing_engine("hist-retry").await; + let (id, work_dir) = submit_and_fail(&engine, xml.clone()).await; + + // Retry reuses the retained partial directory. + let qm = &engine.queue_manager; + let entry = qm.history_get(&id).unwrap().unwrap(); + let retry_data = qm.history_get_retry_data(&id).unwrap(); + let retry = qm + .prepare_retry_job(&entry, &xml, retry_data.as_deref()) + .unwrap(); + assert_eq!( + retry.work_dir, work_dir, + "retry reuses the partial download" + ); + qm.pause_all(); + qm.add_job(retry, Some(xml)).unwrap(); + + qm.history_remove(&id).unwrap(); + + assert!( + work_dir.join("present.bin").exists(), + "a work dir still used by a queued retry must survive history delete" + ); +} + +#[tokio::test] +async fn deleting_a_queued_job_removes_its_work_dir() { + let (engine, xml) = failing_engine("queue-del").await; + engine.queue_manager.pause_all(); + let id = engine.submit_nzb_xml("queued", xml).unwrap(); + let work_dir = engine.incomplete_dir.join(&id); + assert!(work_dir.is_dir()); + + engine.queue_manager.remove_job(&id).unwrap(); + + assert!(!work_dir.exists()); +} + +fn history_entry(id: &str, complete: &Path) -> HistoryEntry { + HistoryEntry { + id: id.to_string(), + name: id.to_string(), + category: "Default".to_string(), + status: JobStatus::Failed, + total_bytes: 1, + downloaded_bytes: 0, + added_at: chrono::Utc::now(), + completed_at: chrono::Utc::now(), + download_time_secs: None, + output_dir: complete.join(id), + stages: Vec::new(), + error_message: Some("failed".to_string()), + failure_code: Some(JobFailureCode::DownloadFailed), + server_stats: Vec::new(), + nzb_data: None, + retry_data: None, + } +} + +fn dir_with_file(path: &Path) { + std::fs::create_dir_all(path).unwrap(); + std::fs::write(path.join("data.bin"), b"data").unwrap(); +} + +#[tokio::test] +async fn startup_sweeps_unreferenced_incomplete_dirs_only() { + let state = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let database_path = state.path().join("rustnzb.db"); + let incomplete = state.path().join("incomplete"); + let complete = state.path().join("complete"); + // Work directories are named after job ids (UUIDs). + let kept_history = "1a2b3c4d-5e6f-4a7b-8c9d-0e1f2a3b4c5d"; + let retried_history = "3c4d5e6f-7a8b-4c9d-8e0f-1a2b3c4d5e6f"; + let reused_partial = incomplete.join("2b3c4d5e-6f7a-4b8c-9d0e-1f2a3b4c5d6e"); + let orphan_one = incomplete.join("0b7c7f0e-3a51-4c4e-9a43-1f6f0c2b8d11"); + let orphan_two = incomplete.join("5d2a9c1e-7b3f-4e8a-b6c0-2e4f8a1d9c33"); + let job_named_file = incomplete.join("7f6e5d4c-3b2a-4190-8f7e-6d5c4b3a2918"); + let link = incomplete.join("9e8d7c6b-5a49-4382-a1b0-c9d8e7f6a5b4"); + let user_folder = incomplete.join("user-folder"); + + let fixture = NzbFixture::new("queued") + .add_file("queued.bin", &[("sweep-queued@test", b"queued")]) + .build(); + let mut queued: NzbJob = nzb_parser::parse_nzb("queued", &fixture.xml).unwrap(); + queued.work_dir = incomplete.join(&queued.id); + queued.output_dir = complete.join("queued"); + queued.status = JobStatus::Paused; + { + let db = Database::open(&database_path).unwrap(); + db.queue_insert(&queued).unwrap(); + db.queue_store_nzb_data(&queued.id, &fixture.xml).unwrap(); + db.history_insert(&history_entry(kept_history, &complete)) + .unwrap(); + // A retry reused another job's partial dir; its checkpoint names it. + let mut retried = history_entry(retried_history, &complete); + retried.retry_data = Some( + serde_json::to_vec(&serde_json::json!({ + "files": {}, + "downloaded_bytes": 0, + "articles_downloaded": 0, + "articles_failed": 0, + "files_completed": 0, + "work_dir": reused_partial, + })) + .unwrap(), + ); + db.history_insert(&retried).unwrap(); + } + + dir_with_file(&queued.work_dir); + dir_with_file(&incomplete.join(kept_history)); + dir_with_file(&reused_partial); + dir_with_file(&orphan_one); + dir_with_file(&orphan_two.join("nested")); + dir_with_file(&user_folder); + std::fs::write(&job_named_file, b"a file, not a dir").unwrap(); + dir_with_file(&outside.path().join("target")); + #[cfg(unix)] + std::os::unix::fs::symlink(outside.path().join("target"), &link).unwrap(); + + let server = ServerProfile::start("sweep", MockConfig::default(), 1).await; + let engine = HarnessBuilder::new() + .with_server(server) + .with_database_path(database_path) + .with_state_dir(state.path().to_path_buf()) + .build(); + engine.queue_manager.restore_from_db().unwrap(); + + assert!(!orphan_one.exists(), "unreferenced job dir is removed"); + assert!(!orphan_two.exists(), "nested content goes with it"); + assert!( + queued.work_dir.join("data.bin").exists(), + "queue job dir kept" + ); + assert!(incomplete.join(kept_history).join("data.bin").exists()); + assert!(reused_partial.join("data.bin").exists()); + assert!( + user_folder.join("data.bin").exists(), + "only job-id named directories are swept" + ); + assert!(job_named_file.is_file(), "files are left alone"); + assert!(incomplete.is_dir(), "the incomplete root itself is kept"); + assert!( + outside.path().join("target").join("data.bin").exists(), + "the sweep must never follow a symlink out of the incomplete root" + ); + #[cfg(unix)] + assert!( + std::fs::symlink_metadata(&link).is_ok(), + "symlinks are not directories and are left alone" + ); +} + +#[tokio::test] +async fn startup_sweep_keeps_everything_when_a_queue_row_is_undecodable() { + let state = tempfile::tempdir().unwrap(); + let database_path = state.path().join("rustnzb.db"); + let incomplete = state.path().join("incomplete"); + let complete = state.path().join("complete"); + let orphan = incomplete.join("0b7c7f0e-3a51-4c4e-9a43-1f6f0c2b8d11"); + + let fixture = NzbFixture::new("queued") + .add_file("queued.bin", &[("undecodable-queued@test", b"queued")]) + .build(); + let mut queued: NzbJob = nzb_parser::parse_nzb("queued", &fixture.xml).unwrap(); + queued.work_dir = incomplete.join(&queued.id); + queued.output_dir = complete.join("queued"); + queued.status = JobStatus::Paused; + { + let db = Database::open(&database_path).unwrap(); + db.queue_insert(&queued).unwrap(); + db.queue_store_nzb_data(&queued.id, &fixture.xml).unwrap(); + drop(db); + // One undecodable column makes queue_list() fail for the whole table. + let status = std::process::Command::new("sqlite3") + .arg(&database_path) + .arg(format!( + "UPDATE queue SET total_bytes = 'x' WHERE id = '{}'", + queued.id + )) + .status() + .expect("sqlite3"); + assert!(status.success(), "corrupting the queue row failed"); + } + + dir_with_file(&queued.work_dir); + dir_with_file(&orphan); + + let server = ServerProfile::start("sweep-bad-row", MockConfig::default(), 1).await; + let engine = HarnessBuilder::new() + .with_server(server) + .with_database_path(database_path) + .with_state_dir(state.path().to_path_buf()) + .build(); + // Restoration fails on the same undecodable row. The sweep must not have + // deleted anything first: a read error fails closed. + assert!(engine.queue_manager.restore_from_db().is_err()); + + assert!( + queued.work_dir.join("data.bin").exists(), + "the queued job's work dir survives an unreadable queue" + ); + assert!( + orphan.join("data.bin").exists(), + "even an orphan survives when the reference set cannot be read" + ); +} diff --git a/crates/nzb-web/tests/harness_speed_limit.rs b/crates/nzb-web/tests/harness_speed_limit.rs new file mode 100644 index 00000000..cbad2ccd --- /dev/null +++ b/crates/nzb-web/tests/harness_speed_limit.rs @@ -0,0 +1,165 @@ +//! BUG-57 — speed-limit changes must reach in-flight and newly added jobs. +//! +//! These drive a real `QueueManager` + worker pool against a mock NNTP +//! server and change the limit through `QueueManager::set_speed_limit`, the +//! same call `PUT /api/config/speed-limit` makes. A tiny limit must throttle +//! the download, and raising it — or clearing it to 0 (unlimited) — must +//! release workers parked on the old limiter within about a second. + +mod harness; + +use std::time::{Duration, Instant}; + +use harness::nzb_fixture::NzbFixture; +use harness::{HarnessBuilder, ServerProfile, TestEngine, yenc_articles}; +use nzb_nntp::testutil::MockConfig; +use nzb_web::nzb_core::models::JobStatus; + +const SEGMENT_BYTES: usize = 64 * 1024; + +/// Build an NZB of `segments` 64 KiB articles served by a mock with +/// `connections` connections, and an engine with `speed_limit_bps` set at +/// construction (the startup path that reads `general.speed_limit_bps`). +async fn engine_with_job( + name: &str, + segments: usize, + connections: u16, + speed_limit_bps: u64, +) -> (TestEngine, String, Vec) { + let ids: Vec = (0..segments).map(|i| format!("{name}-{i}@test")).collect(); + let bodies: Vec> = (0..segments) + .map(|i| vec![(i % 251) as u8; SEGMENT_BYTES]) + .collect(); + let segs: Vec<(&str, &[u8])> = ids + .iter() + .zip(&bodies) + .map(|(id, body)| (id.as_str(), body.as_slice())) + .collect(); + let fixture = NzbFixture::new(name).add_file("data.bin", &segs).build(); + let triples: Vec<(&str, &[u8], &str)> = fixture + .articles + .iter() + .map(|(m, b, f)| (*m, *b, f.as_str())) + .collect(); + let server = ServerProfile::start( + name, + MockConfig { + articles: yenc_articles(&triples), + ..Default::default() + }, + connections, + ) + .await; + let engine = HarnessBuilder::new() + .with_server(server) + .article_timeout(30) + .speed_limit_bps(speed_limit_bps) + .build(); + (engine, name.to_string(), fixture.xml) +} + +fn downloaded(engine: &TestEngine, job_id: &str) -> usize { + engine + .job(job_id) + .map(|j| j.articles_downloaded) + .unwrap_or(usize::MAX) // left the queue => finished +} + +async fn assert_completes_within(engine: &TestEngine, job_id: &str, limit: Duration) { + let start = Instant::now(); + let done = engine + .wait_for_status( + job_id, + limit, + &[ + JobStatus::Completed, + JobStatus::PostProcessing, + JobStatus::Verifying, + JobStatus::Repairing, + JobStatus::Extracting, + ], + ) + .await + || engine + .wait_for(Duration::ZERO, |s| { + s.job(job_id) + .is_some_and(|j| j.articles_downloaded + j.articles_failed >= j.article_count) + }) + .await; + assert!( + done, + "job did not finish downloading within {limit:?} of the limit change (downloaded {})", + downloaded(engine, job_id) + ); + eprintln!("finished {:?} after the limit change", start.elapsed()); +} + +/// A tiny limit configured at startup throttles the first job; clearing it +/// to unlimited releases the parked workers promptly. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn startup_limit_throttles_and_unlimited_releases() { + let (engine, name, xml) = engine_with_job("bw-startup", 6, 2, 20).await; + let job_id = engine.submit_nzb_xml(&name, xml).expect("submit"); + + tokio::time::sleep(Duration::from_millis(1_500)).await; + assert_eq!( + downloaded(&engine, &job_id), + 0, + "a 20 B/s limit must hold back 64 KiB articles" + ); + + engine.queue_manager.set_speed_limit(0); + assert_completes_within(&engine, &job_id, Duration::from_secs(3)).await; +} + +/// The live repro: the limit is set (as by the PUT handler) while idle, then +/// a job is added. It must be throttled, and raising the limit must release +/// it promptly rather than after the old 20 B/s waits drain. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn limit_set_before_job_throttles_and_raise_releases() { + let (engine, name, xml) = engine_with_job("bw-preset", 6, 2, 0).await; + engine.queue_manager.set_speed_limit(20); + let job_id = engine.submit_nzb_xml(&name, xml).expect("submit"); + + tokio::time::sleep(Duration::from_millis(1_500)).await; + assert_eq!( + downloaded(&engine, &job_id), + 0, + "a limit set before the job was added must throttle it" + ); + + engine.queue_manager.set_speed_limit(10_000_000); + assert_completes_within(&engine, &job_id, Duration::from_secs(3)).await; +} + +/// Lowering the limit mid-download throttles in-flight work, and clearing it +/// unthrottles again. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn mid_download_changes_throttle_and_unthrottle() { + // 40 x 64 KiB at 256 KiB/s on one connection is ~10 s unthrottled-ish. + let (engine, name, xml) = engine_with_job("bw-mid", 40, 1, 256 * 1024).await; + let job_id = engine.submit_nzb_xml(&name, xml).expect("submit"); + + assert!( + engine + .wait_for(Duration::from_secs(5), |s| s + .job(&job_id) + .is_some_and(|j| j.articles_downloaded >= 2)) + .await, + "download should make progress under a 256 KiB/s limit" + ); + + engine.queue_manager.set_speed_limit(20); + // Articles already past the limiter (pipelined) may still land. + tokio::time::sleep(Duration::from_millis(500)).await; + let after_throttle = downloaded(&engine, &job_id); + tokio::time::sleep(Duration::from_millis(1_500)).await; + assert_eq!( + downloaded(&engine, &job_id), + after_throttle, + "lowering the limit to 20 B/s must stop progress" + ); + + engine.queue_manager.set_speed_limit(0); + assert_completes_within(&engine, &job_id, Duration::from_secs(3)).await; +} diff --git a/crates/nzb-web/tests/harness_unique_output_dir.rs b/crates/nzb-web/tests/harness_unique_output_dir.rs new file mode 100644 index 00000000..b28f97b1 --- /dev/null +++ b/crates/nzb-web/tests/harness_unique_output_dir.rs @@ -0,0 +1,184 @@ +//! Two jobs that share a name must not complete into the same directory. +//! +//! SABnzbd resolves a collision in the complete directory by appending `.1`, +//! `.2`, ... to the later job's folder. Without that, the second job's files +//! overwrite or merge into the first job's completed release. + +mod harness; + +use std::collections::HashMap; +use std::time::Duration; + +use harness::nzb_fixture::NzbFixture; +use harness::{HarnessBuilder, ServerProfile, yenc_articles}; +use nzb_nntp::testutil::MockConfig; +use nzb_web::nzb_core::models::{JobStatus, NzbJob}; +use nzb_web::nzb_core::nzb_parser; + +fn submit_named(engine: &harness::TestEngine, name: &str, xml: Vec) -> String { + let mut job: NzbJob = nzb_parser::parse_nzb(name, &xml).expect("parse nzb"); + job.work_dir = engine.incomplete_dir.join(&job.id); + // Production derives the output folder from the job name, so two jobs + // with the same name start with the same target directory. + job.output_dir = engine.complete_dir.join(name); + let id = job.id.clone(); + engine + .queue_manager + .add_job(job, Some(xml)) + .expect("add job"); + id +} + +#[tokio::test] +async fn same_named_jobs_complete_into_distinct_directories() { + let first = NzbFixture::new("SameName") + .add_file("release.bin", &[("same-name-1@test", b"first release")]) + .build(); + let second = NzbFixture::new("SameName") + .add_file("release.bin", &[("same-name-2@test", b"second release")]) + .build(); + // Both releases carry a `release.bin`, so encode each NZB on its own: + // `yenc_articles` treats same-named triples as segments of one file. + let mut articles = HashMap::new(); + for fixture in [&first, &second] { + let triples: Vec<(&str, &[u8], &str)> = fixture + .articles + .iter() + .map(|(m, b, f)| (*m, *b, f.as_str())) + .collect(); + articles.extend(yenc_articles(&triples)); + } + let server = ServerProfile::start( + "same-name", + MockConfig { + articles, + ..Default::default() + }, + 4, + ) + .await; + let engine = HarnessBuilder::new() + .with_server(server) + .max_active_downloads(1) + .build(); + + let first_id = submit_named(&engine, "SameName", first.xml); + assert!( + engine + .wait_for_status(&first_id, Duration::from_secs(20), &[JobStatus::Completed]) + .await, + "first job did not complete" + ); + let second_id = submit_named(&engine, "SameName", second.xml); + assert!( + engine + .wait_for_status(&second_id, Duration::from_secs(20), &[JobStatus::Completed]) + .await, + "second job did not complete" + ); + + let first_entry = engine + .queue_manager + .history_get(&first_id) + .unwrap() + .expect("first history row"); + let second_entry = engine + .queue_manager + .history_get(&second_id) + .unwrap() + .expect("second history row"); + + assert_eq!(first_entry.output_dir, engine.complete_dir.join("SameName")); + assert_eq!( + second_entry.output_dir, + engine.complete_dir.join("SameName.1"), + "the second same-named job must complete into a suffixed directory" + ); + assert_eq!( + std::fs::read(first_entry.output_dir.join("release.bin")).unwrap(), + b"first release", + "the first job's completed files must not be overwritten" + ); + assert_eq!( + std::fs::read(second_entry.output_dir.join("release.bin")).unwrap(), + b"second release" + ); +} + +#[tokio::test] +async fn concurrent_same_named_jobs_never_share_a_directory() { + let names = ["a", "b", "c"]; + let bodies: [&[u8]; 3] = [b"body a", b"body b", b"body c"]; + let msg_ids = ["race-a@test", "race-b@test", "race-c@test"]; + let fixtures: Vec<_> = (0..3) + .map(|i| { + NzbFixture::new("Race") + .add_file( + &format!("release-{}.bin", names[i]), + &[(msg_ids[i], bodies[i])], + ) + .build() + }) + .collect(); + let triples: Vec<(&str, &[u8], &str)> = fixtures + .iter() + .flat_map(|f| f.articles.iter()) + .map(|(m, b, f)| (*m, *b, f.as_str())) + .collect(); + let server = ServerProfile::start( + "race", + MockConfig { + articles: yenc_articles(&triples), + ..Default::default() + }, + 6, + ) + .await; + let engine = HarnessBuilder::new() + .with_server(server) + .max_active_downloads(3) + .build(); + + let ids: Vec = fixtures + .into_iter() + .map(|fixture| submit_named(&engine, "Race", fixture.xml)) + .collect(); + for id in &ids { + assert!( + engine + .wait_for_status(id, Duration::from_secs(20), &[JobStatus::Completed]) + .await, + "job {id} did not complete" + ); + } + + let mut dirs: Vec<_> = ids + .iter() + .map(|id| { + engine + .queue_manager + .history_get(id) + .unwrap() + .expect("history row") + .output_dir + }) + .collect(); + dirs.sort(); + assert_eq!( + dirs, + vec![ + engine.complete_dir.join("Race"), + engine.complete_dir.join("Race.1"), + engine.complete_dir.join("Race.2"), + ] + ); + for dir in &dirs { + let entries = std::fs::read_dir(dir).unwrap().count(); + assert_eq!( + entries, + 1, + "{} must hold exactly one job's files", + dir.display() + ); + } +} diff --git a/crates/nzb-web/tests/workflow_fixtures.rs b/crates/nzb-web/tests/workflow_fixtures.rs index a2e28a42..d57ca104 100644 --- a/crates/nzb-web/tests/workflow_fixtures.rs +++ b/crates/nzb-web/tests/workflow_fixtures.rs @@ -81,3 +81,177 @@ async fn existing_gzip_nzb_is_imported_once_and_moved_to_processed() { assert!(!input.exists()); assert_eq!(queue.get_jobs()[0].status, JobStatus::Downloading); } + +fn watch_queue(temp: &Path) -> std::sync::Arc { + QueueManager::new( + Vec::new(), + Database::open_memory().unwrap(), + temp.join("incomplete"), + temp.join("complete"), + LogBuffer::default(), + 1, + Vec::new(), + 0, + 0, + false, + 5, + true, + true, + 100.0, + 2, + ) +} + +fn fixture_nzb(subject: &str) -> Vec { + format!( + r#"alt.test{subject}-1@test"# + ) + .into_bytes() +} + +async fn wait_until(timeout: Duration, mut done: impl FnMut() -> bool) -> bool { + tokio::time::timeout(timeout, async { + while !done() { + tokio::time::sleep(Duration::from_millis(20)).await; + } + }) + .await + .is_ok() +} + +#[tokio::test] +async fn existing_zip_of_nzbs_is_imported_and_moved_to_processed() { + use zip::write::SimpleFileOptions; + + let temp = tempfile::tempdir().unwrap(); + let watch_dir = temp.path().join("watch"); + std::fs::create_dir_all(&watch_dir).unwrap(); + + let mut writer = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); + let options = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + for (name, subject) in [("first.nzb", "first.bin"), ("second.nzb", "second.bin")] { + writer.start_file(name, options).unwrap(); + writer.write_all(&fixture_nzb(subject)).unwrap(); + } + writer.start_file("readme.txt", options).unwrap(); + writer.write_all(b"not an nzb").unwrap(); + let archive = writer.finish().unwrap().into_inner(); + let input = watch_dir.join("bundle.zip"); + std::fs::write(&input, archive).unwrap(); + + let queue = watch_queue(temp.path()); + let watcher_task = tokio::spawn(DirWatcher::new(watch_dir.clone(), queue.clone()).run()); + let imported = wait_until(Duration::from_secs(2), || queue.queue_size() == 2).await; + watcher_task.abort(); + + assert!( + imported, + "watch folder did not enqueue both NZBs from the zip" + ); + let mut names: Vec = queue.get_jobs().into_iter().map(|job| job.name).collect(); + names.sort(); + assert_eq!(names, ["first", "second"]); + assert!(watch_dir.join("processed/bundle.zip").exists()); + assert!(!input.exists()); +} + +#[tokio::test] +async fn existing_bzip2_nzb_is_imported_and_moved_to_processed() { + let temp = tempfile::tempdir().unwrap(); + let watch_dir = temp.path().join("watch"); + std::fs::create_dir_all(&watch_dir).unwrap(); + + let mut encoder = bzip2::write::BzEncoder::new(Vec::new(), bzip2::Compression::default()); + encoder.write_all(&fixture_nzb("bz.bin")).unwrap(); + let input = watch_dir.join("packed.nzb.bz2"); + std::fs::write(&input, encoder.finish().unwrap()).unwrap(); + + let queue = watch_queue(temp.path()); + let watcher_task = tokio::spawn(DirWatcher::new(watch_dir.clone(), queue.clone()).run()); + let imported = wait_until(Duration::from_secs(2), || queue.queue_size() == 1).await; + watcher_task.abort(); + + assert!(imported, "watch folder did not enqueue the bzip2 NZB"); + assert_eq!(queue.get_jobs()[0].name, "packed"); + assert!(watch_dir.join("processed/packed.nzb.bz2").exists()); + assert!(!input.exists()); +} + +#[tokio::test] +async fn unparseable_nzb_is_moved_to_failed() { + let temp = tempfile::tempdir().unwrap(); + let watch_dir = temp.path().join("watch"); + std::fs::create_dir_all(&watch_dir).unwrap(); + let input = watch_dir.join("broken.nzb"); + std::fs::write(&input, b"this is not xml").unwrap(); + + let queue = watch_queue(temp.path()); + let watcher_task = tokio::spawn(DirWatcher::new(watch_dir.clone(), queue.clone()).run()); + let moved = wait_until(Duration::from_secs(5), || { + watch_dir.join("failed/broken.nzb").exists() + }) + .await; + watcher_task.abort(); + + assert!(moved, "unparseable NZB was not moved to failed/"); + assert!(!input.exists()); + assert_eq!(queue.queue_size(), 0); +} + +#[tokio::test] +async fn enqueued_nzb_is_not_enqueued_again_after_restart_when_move_fails() { + let temp = tempfile::tempdir().unwrap(); + let watch_dir = temp.path().join("watch"); + std::fs::create_dir_all(&watch_dir).unwrap(); + // A regular file where processed/ should be makes the post-enqueue move fail. + std::fs::write(watch_dir.join("processed"), b"").unwrap(); + std::fs::write(watch_dir.join("once.nzb"), fixture_nzb("once.bin")).unwrap(); + + let first = watch_queue(&temp.path().join("first")); + let task = tokio::spawn(DirWatcher::new(watch_dir.clone(), first.clone()).run()); + let imported = wait_until(Duration::from_secs(5), || first.queue_size() == 1).await; + // Let the watcher finish its post-enqueue bookkeeping. + tokio::time::sleep(Duration::from_millis(200)).await; + task.abort(); + assert!(imported, "first run did not enqueue the NZB"); + + // Simulated restart: a fresh watcher over the same folder must not pick + // the already-enqueued NZB up again. + let second = watch_queue(&temp.path().join("second")); + let task = tokio::spawn(DirWatcher::new(watch_dir.clone(), second.clone()).run()); + let duplicated = wait_until(Duration::from_secs(2), || second.queue_size() > 0).await; + task.abort(); + assert!( + !duplicated, + "restart re-enqueued an NZB that was already enqueued" + ); +} + +#[tokio::test] +async fn slowly_written_nzb_is_parsed_only_once_complete() { + let temp = tempfile::tempdir().unwrap(); + let watch_dir = temp.path().join("watch"); + std::fs::create_dir_all(&watch_dir).unwrap(); + + let queue = watch_queue(temp.path()); + let watcher_task = tokio::spawn(DirWatcher::new(watch_dir.clone(), queue.clone()).run()); + // Give the watcher time to start watching. + tokio::time::sleep(Duration::from_millis(300)).await; + + let body = fixture_nzb("slow.bin"); + let path = watch_dir.join("slow.nzb"); + let mut file = std::fs::File::create(&path).unwrap(); + for chunk in body.chunks(body.len() / 12 + 1) { + file.write_all(chunk).unwrap(); + file.flush().unwrap(); + tokio::time::sleep(Duration::from_millis(100)).await; + } + drop(file); + + let imported = wait_until(Duration::from_secs(5), || queue.queue_size() == 1).await; + watcher_task.abort(); + + assert!(imported, "slowly written NZB was not enqueued"); + assert!(!watch_dir.join("failed/slow.nzb").exists()); + assert!(watch_dir.join("processed/slow.nzb").exists()); +} diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index 12c4c071..31063d98 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -1142,7 +1142,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -1343,7 +1343,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2973,7 +2973,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -3109,13 +3109,14 @@ dependencies = [ [[package]] name = "nzb-web" -version = "0.4.22" +version = "0.4.23" dependencies = [ "anyhow", "arc-swap", "async-trait", "axum", "base64 0.23.1", + "bzip2", "chrono", "feed-rs", "flate2", @@ -3146,6 +3147,7 @@ dependencies = [ "utoipa", "utoipa-swagger-ui", "uuid", + "zip 8.6.0", ] [[package]] @@ -3507,7 +3509,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7d8fae84b431384b68627d0f9b3b1245fcf9f46f6c0e3dc902e9dce64edd1967" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.45.0", ] [[package]] @@ -3949,7 +3951,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4312,7 +4314,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4362,7 +4364,6 @@ dependencies = [ "base64 0.23.1", "chrono", "clap", - "flate2", "hex", "http", "libc", @@ -4390,7 +4391,6 @@ dependencies = [ "utoipa", "utoipa-swagger-ui", "uuid", - "zip 8.6.0", ] [[package]] @@ -5385,7 +5385,7 @@ dependencies = [ "serde_with", "swift-rs", "thiserror 2.0.18", - "toml 1.1.2+spec-1.1.0", + "toml 0.9.12+spec-1.1.0", "url", "urlpattern", "uuid", @@ -5424,7 +5424,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -6475,7 +6475,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]]