From 30523ab8bb6621b14ac11609f701d85d85420b7d Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:29:56 -0500 Subject: [PATCH 01/17] fix(sab): honour nzbname and stop naming addurl jobs after the URL mode=addurl&name= passed name both as the URL and as the job name, so the job was named after the whole URL, which output_dir_for rejects as not a single safe path component: every successful addurl fetch failed at enqueue. The nzbname override was also ignored on addfile and addurl over GET, form and multipart. Read nzbname from the query string, a urlencoded body or a multipart field. For addurl the job name is nzbname, else the Content-Disposition filename, else the URL's last path segment (percent-decoded, query excluded), each without .nzb; never the URL itself. addfile prefers nzbname over the uploaded file name. Unit tests reach the loopback fixture server through a cfg(test)-only task-local seam in fetch_guard (ALLOW_LOOPBACK_FOR_TESTS), scoped to the test's own future; the SSRF guard is unchanged in every other build. Fixes #69 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 9ed65989e1af63e8d98da44cd1a234cbfb3ddb29) Adapted: tests admit the loopback fixture through an explicit fetch_allowed_hosts entry on the test state instead of the upstream cfg(test) ALLOW_LOOPBACK_FOR_TESTS seam, which our fetch guard (private-LAN allowlist plus explicit hosts) does not have. --- crates/nzb-web/src/sabnzbd_compat.rs | 322 ++++++++++++++++++++++++--- 1 file changed, 294 insertions(+), 28 deletions(-) diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 6ca0e351..f5de39e0 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -49,6 +49,8 @@ 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, } /// Validate API key. Returns Err with JSON response on failure. @@ -124,11 +126,13 @@ 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(), @@ -141,11 +145,12 @@ 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, @@ -196,25 +201,21 @@ 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() - }); + let job_name = addurl_job_name( + nzbname.as_deref(), + content_disposition.as_deref(), + &fetch_plan.url, + ); match nzb_parser::parse_nzb(&job_name, &data) { Ok(mut job) => { @@ -285,6 +286,90 @@ 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[base.len() - 4..].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 +408,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 +450,9 @@ 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; + } } SabPostBody::Multipart => { let mut multipart = Multipart::from_request(request, &()).await.map_err(|e| { @@ -380,6 +468,7 @@ pub async fn h_sabnzbd_api_post( &mut nzb_data, &mut nzb_url, &mut password, + &mut query_req.nzbname, ) .await?; } @@ -412,6 +501,7 @@ async fn read_multipart_fields( nzb_data: &mut Option<(String, Vec)>, nzb_url: &mut Option, password: &mut Option, + nzbname: &mut Option, ) -> Result<(), ApiError> { while let Some(field) = multipart .next_field() @@ -483,6 +573,11 @@ async fn read_multipart_fields( *password = Some(text); } } + "nzbname" => { + if let Ok(text) = field.text().await { + *nzbname = Some(text); + } + } _ => { let _ = field.bytes().await; } @@ -492,7 +587,8 @@ 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. #[allow(clippy::too_many_arguments)] async fn dispatch_post( state: &AppState, @@ -517,12 +613,17 @@ async fn dispatch_post( } }; - let job_name = name.clone().unwrap_or_else(|| { - file_name - .strip_suffix(".nzb") - .unwrap_or(&file_name) - .to_string() - }); + 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) => { @@ -599,8 +700,9 @@ 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).await } _ => { @@ -628,6 +730,7 @@ async fn dispatch_post( last_history_update: query_req.last_history_update, password, del_files: query_req.del_files, + nzbname: query_req.nzbname, }; Ok(dispatch_mode( state, @@ -4029,6 +4132,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 +4153,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 +4317,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() { From 102a3abf2e6da654335525abbf26baf618939421 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:34:26 -0500 Subject: [PATCH 02/17] fix(web): serialise config read-modify-write to stop lost updates Every /api/config/* handler cloned the current config, mutated the copy and stored it with an unguarded ArcSwap::store. Two concurrent writers started from the same snapshot, and the later store silently dropped the earlier writer's change, in memory and in config.toml. A burst of 24 concurrent category/feed adds lost entries on every run. Add AppState::update_config_with(|cfg| ...), which runs the closure on the latest config and saves the TOML and publishes the result while holding a config write mutex. An error from the closure writes nothing. update_config takes the same lock and is documented as a blind replacement. All 17 mutating handlers (SAB API key rotation, servers, categories, history retention, max active downloads, speed limit, disk guards, RSS feeds, general settings, SABnzbd import apply, DAV) now go through it. Server handlers push the latest committed server list to the queue manager, so concurrent writers converge on the same state. Bumps nzb-web to 0.4.23. Fixes #71 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit aec138d1df5f20a7c06219be1bdcf01d3ffa442b) Adapted: update_config_with keeps our partial server merge, password masking and RSS feed URL validation inside the new write lock; started_at stays alongside config_write. --- Cargo.lock | 2 +- Cargo.toml | 2 +- apps/rustnzb/src/handlers.rs | 483 ++++++++++++++-------------- apps/rustnzb/tests/api_contracts.rs | 69 ++++ crates/nzb-web/Cargo.toml | 2 +- crates/nzb-web/src/state.rs | 36 ++- 6 files changed, 357 insertions(+), 237 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9c4f9d89..eca3cdf0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1894,7 +1894,7 @@ dependencies = [ [[package]] name = "nzb-web" -version = "0.4.22" +version = "0.4.23" dependencies = [ "anyhow", "arc-swap", diff --git a/Cargo.toml b/Cargo.toml index ae4cbe04..60039973 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -86,7 +86,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/src/handlers.rs b/apps/rustnzb/src/handlers.rs index 1e2630a2..f0d1ff15 100644 --- a/apps/rustnzb/src/handlers.rs +++ b/apps/rustnzb/src/handlers.rs @@ -894,13 +894,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 +944,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 +983,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 +1028,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 })) } @@ -1193,18 +1199,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 +1221,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 +1241,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 +1263,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 +1286,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 +1336,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 +1369,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 +1398,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 +1418,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 +1435,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}))) } @@ -1663,99 +1679,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 +2195,42 @@ pub async fn h_setup_apply( )); } - let mut config = (*state.config()).clone(); + 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(); - // 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,9 +2394,10 @@ 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 }))) } diff --git a/apps/rustnzb/tests/api_contracts.rs b/apps/rustnzb/tests/api_contracts.rs index 9b9b647c..59f938e9 100644 --- a/apps/rustnzb/tests/api_contracts.rs +++ b/apps/rustnzb/tests/api_contracts.rs @@ -669,3 +669,72 @@ 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}" + ); + } + } + } +} diff --git a/crates/nzb-web/Cargo.toml b/crates/nzb-web/Cargo.toml index 50579dd6..d9f943f8 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" 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(()) From a5937720f3dff77f226b41b052ea3cd28ea78fdd Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:35:59 -0500 Subject: [PATCH 03/17] fix(watch): import .zip and .nzb.bz2 archives from the watch folder The watch folder only matched .nzb and .nzb.gz, so a .zip of NZBs (or a .nzb.bz2) dropped there was skipped without a log line. The upload API already unpacked .gz and .zip, but through extract_nzbs in the binary crate, which the watcher in nzb-web cannot reach. Move extract_nzbs (unchanged, with its size bounds and tests) into nzb_web::nzb_archive and have both the upload handler and the watcher call it. The helper also learns .bz2, using the bzip2 crate that was already in the dependency tree via zip. The watcher now enqueues each .nzb inside a zip, names jobs after the NZB without any archive directory, and moves the archive to processed/ once at least one NZB was enqueued. zip and flate2 drop out of the binary crate, which no longer uses them directly. Fixes #73 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 67a70705f912262757239eb714f7215e1be33c28) --- Cargo.lock | 4 +- Cargo.toml | 2 + apps/rustnzb/Cargo.toml | 2 - apps/rustnzb/src/handlers.rs | 118 +---------------- crates/nzb-web/Cargo.toml | 2 + crates/nzb-web/src/dir_watcher.rs | 121 +++++++++--------- crates/nzb-web/src/lib.rs | 1 + crates/nzb-web/src/nzb_archive.rs | 147 ++++++++++++++++++++++ crates/nzb-web/tests/workflow_fixtures.rs | 98 +++++++++++++++ 9 files changed, 313 insertions(+), 182 deletions(-) create mode 100644 crates/nzb-web/src/nzb_archive.rs diff --git a/Cargo.lock b/Cargo.lock index eca3cdf0..d5cfac32 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1901,6 +1901,7 @@ dependencies = [ "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 60039973..419599e3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,6 +80,8 @@ 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" 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/handlers.rs b/apps/rustnzb/src/handlers.rs index f0d1ff15..6567a513 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,7 @@ 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; - +use nzb_web::nzb_archive::extract_nzbs; #[cfg(feature = "webdav")] use nzb_web::nzb_core::config::DavConfig; use nzb_web::nzb_core::config::{ @@ -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, @@ -2403,28 +2325,9 @@ pub async fn h_dav_config_set( #[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() { @@ -2444,23 +2347,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/crates/nzb-web/Cargo.toml b/crates/nzb-web/Cargo.toml index d9f943f8..b71e168f 100644 --- a/crates/nzb-web/Cargo.toml +++ b/crates/nzb-web/Cargo.toml @@ -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..5d079d2f 100644 --- a/crates/nzb-web/src/dir_watcher.rs +++ b/crates/nzb-web/src/dir_watcher.rs @@ -2,11 +2,11 @@ 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 crate::nzb_archive::extract_nzbs; use crate::queue_manager::QueueManager; const MAX_WATCHED_NZB_BYTES: usize = 100 * 1024 * 1024; @@ -72,12 +72,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) { @@ -108,70 +112,60 @@ impl DirWatcher { } }; - 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; - } - if decompressed.len() > MAX_WATCHED_NZB_BYTES { - warn!(file = %path.display(), limit = MAX_WATCHED_NZB_BYTES, "Decompressed watched NZB exceeds the input limit"); + let file_name = path + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("unknown.nzb"); + let nzbs = match extract_nzbs(file_name, &raw_data) { + Ok(nzbs) => nzbs, + Err(e) => { + warn!(error = %e, file = %path.display(), "Failed to unpack watched NZB"); return; } - 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; - } - - info!(name = %job.name, id = %job.id, "Auto-enqueuing NZB from watch dir"); + let mut enqueued = 0usize; + 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 }; + 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"); + continue; + } - if let Err(e) = self.queue_manager.add_job(job, Some(data)) { - error!(error = %e, "Failed to enqueue NZB"); - return; - } + info!(name = %job.name, id = %job.id, "Auto-enqueuing NZB from watch dir"); - // 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"); + if let Err(e) = self.queue_manager.add_job(job, Some(data)) { + error!(error = %e, "Failed to enqueue NZB"); + continue; } + enqueued += 1; + } + Err(e) => { + warn!(error = %e, file = %path.display(), entry = %nzb_name, "Failed to parse NZB from watch dir"); } } - Err(e) => { - warn!(error = %e, file = %path.display(), "Failed to parse NZB from watch dir"); + } + if enqueued == 0 { + 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"); } } } @@ -210,9 +204,12 @@ 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"))); } 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..c6dbd5a9 --- /dev/null +++ b/crates/nzb-web/src/nzb_archive.rs @@ -0,0 +1,147 @@ +//! 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; + +/// 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> { + 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")?; + 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")?; + 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}"))?; + 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())]) +} + +fn read_bounded(reader: impl std::io::Read, format: &str) -> Result, anyhow::Error> { + let mut decompressed = Vec::new(); + reader + .take(MAX_NZB_DECOMPRESSED_BYTES + 1) + .read_to_end(&mut decompressed) + .map_err(|e| anyhow::anyhow!("Failed to decompress {format}: {e}"))?; + if decompressed.len() as u64 > MAX_NZB_DECOMPRESSED_BYTES { + anyhow::bail!( + "Decompressed NZB exceeds the {} MB limit", + MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 + ); + } + Ok(decompressed) +} + +#[cfg(test)] +mod tests { + use super::{MAX_NZB_DECOMPRESSED_BYTES, extract_nzbs}; + 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() + } + + #[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 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/tests/workflow_fixtures.rs b/crates/nzb-web/tests/workflow_fixtures.rs index a2e28a42..535bd867 100644 --- a/crates/nzb-web/tests/workflow_fixtures.rs +++ b/crates/nzb-web/tests/workflow_fixtures.rs @@ -81,3 +81,101 @@ 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_for_queue_size(queue: &QueueManager, expected: usize) -> bool { + tokio::time::timeout(Duration::from_secs(2), async { + loop { + if queue.queue_size() == expected { + break; + } + tokio::time::sleep(Duration::from_millis(10)).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_for_queue_size(&queue, 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_for_queue_size(&queue, 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()); +} From 6d05f7aff4183d3ed0fe6cac194abfc019846d3e Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:38:24 -0500 Subject: [PATCH 04/17] fix(nntp): mark connection Error on STAT pipeline I/O failures StatPipeline::execute set the connection Busy for each batch and then propagated send, flush and read failures with a bare `?`, leaving the connection Busy. After such a failure the stream is misaligned with the outstanding STATs, so the connection must not be reused. All three now set ConnectionState::Error first, matching fetch_article and the ARTICLE pipeline. Fixes #75 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit e20357ddd9a771264f7d08f76636925f8859e64d) --- crates/nzb-nntp/src/pipeline.rs | 40 ++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) 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(); From bd2b41e36fccb5d76592e21fc867e4d7efbcebb2 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:39:30 -0500 Subject: [PATCH 05/17] fix(sab): honour the pp post-processing override on addfile/addurl SABnzbd's pp parameter selects post-processing for a single job, but the compat layer dropped it, so every job used its category's level. Add NzbJob::pp_override (serde default; persisted in a new nullable queue.pp_override column via schema migration v13, which tolerates partial schemas without a queue table) and QueueManager::post_processing_level, which prefers the override over the category setting where post-processing chooses its level. addfile/addurl read pp from the query string, a urlencoded body or a multipart field, mapping SABnzbd's cumulative scale onto RustNZB's (2 and 3 -> 3; -1/absent/out of range -> category default). script is accepted and ignored, as before (unknown parameters are not rejected). Fixes #77 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit a3c7cd7490e75ce3c215062db2072e118866600b) Adapted: kept both the earlier nzbname plumbing and the pp override in the shared form/multipart parsers and in handle_addurl; existing SAB config/priority/diskspace tests kept alongside the new pp tests. --- crates/nzb-core/src/db.rs | 52 ++++++++- crates/nzb-core/src/models.rs | 5 + crates/nzb-core/src/nzb_parser.rs | 1 + crates/nzb-dispatch/src/download_engine.rs | 1 + crates/nzb-web/src/queue_manager.rs | 40 +++++-- crates/nzb-web/src/sabnzbd_compat.rs | 122 ++++++++++++++++++++- 6 files changed, 207 insertions(+), 14 deletions(-) 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/download_engine.rs b/crates/nzb-dispatch/src/download_engine.rs index 1c916f70..85a8ea2d 100644 --- a/crates/nzb-dispatch/src/download_engine.rs +++ b/crates/nzb-dispatch/src/download_engine.rs @@ -3406,6 +3406,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } diff --git a/crates/nzb-web/src/queue_manager.rs b/crates/nzb-web/src/queue_manager.rs index 2025f451..a97349fe 100644 --- a/crates/nzb-web/src/queue_manager.rs +++ b/crates/nzb-web/src/queue_manager.rs @@ -1119,6 +1119,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; } @@ -2339,13 +2352,7 @@ 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); let du = state.direct_unpacker.take(); let pw = state.job.password.clone(); let content_failed = state @@ -4505,11 +4512,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"); diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index f5de39e0..7b8fa9ec 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -51,6 +51,10 @@ pub struct SabApiRequest { 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. @@ -136,6 +140,7 @@ pub async fn h_sabnzbd_api_get( req.cat.clone(), req.priority.clone(), req.password.clone(), + req.pp.clone(), ) .await; } @@ -154,6 +159,7 @@ async fn handle_addurl( cat: Option, priority: Option, password: Option, + pp: Option, ) -> Result, ApiError> { let url = url.unwrap_or_default(); @@ -232,6 +238,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); @@ -453,6 +460,9 @@ pub async fn h_sabnzbd_api_post( 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| { @@ -469,6 +479,7 @@ pub async fn h_sabnzbd_api_post( &mut nzb_url, &mut password, &mut query_req.nzbname, + &mut query_req.pp, ) .await?; } @@ -502,6 +513,7 @@ async fn read_multipart_fields( nzb_url: &mut Option, password: &mut Option, nzbname: &mut Option, + pp: &mut Option, ) -> Result<(), ApiError> { while let Some(field) = multipart .next_field() @@ -578,6 +590,11 @@ async fn read_multipart_fields( *nzbname = Some(text); } } + "pp" => { + if let Ok(text) = field.text().await { + *pp = Some(text); + } + } _ => { let _ = field.bytes().await; } @@ -588,7 +605,8 @@ async fn read_multipart_fields( /// Dispatch a POST request once its parameters have been assembled from the /// query string and (optional) body. `query_req.nzbname` carries the merged -/// `nzbname` job-name override. +/// `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, @@ -640,6 +658,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); @@ -702,7 +721,16 @@ 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, query_req.nzbname, cat, priority, password).await + handle_addurl( + state, + url, + query_req.nzbname, + cat, + priority, + password, + query_req.pp, + ) + .await } _ => { @@ -731,6 +759,7 @@ async fn dispatch_post( password, del_files: query_req.del_files, nzbname: query_req.nzbname, + pp: query_req.pp, }; Ok(dispatch_mode( state, @@ -2126,6 +2155,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() { @@ -2569,6 +2613,7 @@ mod tests { password: Some("secret".into()), error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } @@ -2629,6 +2674,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), } @@ -3053,6 +3099,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), }; @@ -3181,6 +3228,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), }; @@ -3212,6 +3260,7 @@ mod tests { password: None, error_message: None, speed_bps: 0, + pp_override: None, server_stats: Vec::new(), files: Vec::new(), }; @@ -3871,6 +3920,75 @@ 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); + } + /// SABnzbd's real `_api_queue_delete` accepts a comma-separated `value` /// list, removing every matching job in one call. #[tokio::test] From 8a639b264d3e4947ad6fa2f62c5bab364a51c16f Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:40:49 -0500 Subject: [PATCH 06/17] fix(api): return 404/400 instead of 500 for unknown ids and invalid input Client errors on the native API surfaced as 500 internal_error because From for ApiError always used 500, and several handlers wrapped "not found" and validation failures in anyhow!, which carries no status. Central mapping (nzb-web error.rs): - NzbError::JobNotFound/ServerNotFound/CategoryNotFound -> 404, ParseError/InvalidNzb -> 400, AdmissionConflict -> 409, rest 500. - error_kind recognises the wrapped NzbError variants and otherwise classifies by status (bad_request / not_found / conflict / internal_error) instead of labelling every non-typed error internal_error. Handlers: - history retry and history logs: unknown id -> 404 (logs previously returned 200 with an empty list). - queue priority: out-of-range value -> 400; unknown job -> 404 via the central mapping, as is queue move. - GET /api/articles/{id}: NNTP 430 (ArticleNotFound) -> 404. - Group routes extract ids with IdPath, which rejects a non-numeric id with a JSON 400 "Invalid id in request path" instead of axum's parser text that leaked Rust type names. - RSS rules: invalid or overlong regex -> 400; PUT of an unknown rule -> 404 (DELETE stays idempotent); a rule naming feeds that are not configured is still saved, logged as a warning, and the response carries a "warnings" array. Bumps nzb-web to 0.4.23. Fixes #79 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 1e09a67caaa01d0ffdd96291f4367f830a588a4b) Adapted: the new contract tests authenticate with the existing login() helper (this tree's first-run setup is already covered elsewhere) and sit alongside the existing contract tests. --- apps/rustnzb/src/group_handlers.rs | 55 +++++++--- apps/rustnzb/src/handlers.rs | 80 ++++++++++++-- apps/rustnzb/tests/api_contracts.rs | 146 ++++++++++++++++++++++++++ apps/rustnzb/tests/group_mark_read.rs | 6 +- crates/nzb-web/src/error.rs | 74 ++++++++++++- 5 files changed, 326 insertions(+), 35 deletions(-) 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 6567a513..ef1e897a 100644 --- a/apps/rustnzb/src/handlers.rs +++ b/apps/rustnzb/src/handlers.rs @@ -413,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 @@ -665,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 @@ -1090,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) @@ -1487,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. @@ -1509,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)?; @@ -1530,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, @@ -1555,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. diff --git a/apps/rustnzb/tests/api_contracts.rs b/apps/rustnzb/tests/api_contracts.rs index 59f938e9..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; @@ -738,3 +860,27 @@ async fn concurrent_config_writes_do_not_lose_updates() { } } } + +#[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-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"); + } +} From 56dee53ff84013028d85f5588e31bd2f976f4727 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:42:13 -0500 Subject: [PATCH 07/17] fix(watch): claim watched NZBs before enqueueing and wait for writes to settle The watch folder could import one NZB twice, retry a bad one forever, or read one mid-copy: - A parse failure only logged a warning and left the file in place, so every restart retried it. Unreadable, unparseable or unenqueueable files now move to failed/. - A fixed 500 ms sleep was the only guard against reading a file still being written. The watcher now waits until the size and modification time are unchanged across two checks 500 ms apart (capped at 10 minutes), for both new events and files present at startup. - The move to processed/ after add_job was best-effort, so if it failed the next restart enqueued a duplicate. The watcher now renames the file to .processing before enqueueing. Claimed files are never imported again, and the claimed file then moves to processed/ or failed/. If that move fails, the file stays claimed, and startup logs a warning for leftover claimed files instead of re-importing them. Fixes #81 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 5b8c1380d3e4920dd1ae816aad80295e026e01fc) Adapted: the claim-before-enqueue and write-settle logic wraps our archive-aware watcher (zip/bz2/gz), so enqueue_claimed unpacks every NZB in the claimed file and any failure sends the whole file to failed/; both sets of watcher tests are kept. --- crates/nzb-web/src/dir_watcher.rs | 217 ++++++++++++++++------ crates/nzb-web/tests/workflow_fixtures.rs | 94 +++++++++- 2 files changed, 247 insertions(+), 64 deletions(-) diff --git a/crates/nzb-web/src/dir_watcher.rs b/crates/nzb-web/src/dir_watcher.rs index 5d079d2f..a27894fe 100644 --- a/crates/nzb-web/src/dir_watcher.rs +++ b/crates/nzb-web/src/dir_watcher.rs @@ -4,12 +4,19 @@ use std::sync::Arc; 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; } } @@ -97,77 +102,138 @@ 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." + ); } } } + /// 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 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 started.elapsed() >= MAX_SETTLE_WAIT { + warn!(file = %path.display(), "Watched file is still changing; processing it anyway"); + return true; + } + previous = current; + } + } + + /// 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(()) + } + async fn process_file(&self, path: &Path) { - info!(file = %path.display(), "Processing NZB from watch directory"); + // 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; + }; - 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; + // 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; + } - let file_name = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("unknown.nzb"); - let nzbs = match extract_nzbs(file_name, &raw_data) { - Ok(nzbs) => nzbs, + 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 unpack watched NZB"); - return; + 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" + ); + } + } - let mut enqueued = 0usize; + /// 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 }; - 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"); - continue; - } - - info!(name = %job.name, id = %job.id, "Auto-enqueuing NZB from watch dir"); - - if let Err(e) = self.queue_manager.add_job(job, Some(data)) { - error!(error = %e, "Failed to enqueue NZB"); - continue; - } - enqueued += 1; - } - Err(e) => { - warn!(error = %e, file = %path.display(), entry = %nzb_name, "Failed to parse NZB from watch dir"); - } - } - } - if enqueued == 0 { - 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"); - } + 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> { @@ -214,6 +280,47 @@ mod tests { 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/tests/workflow_fixtures.rs b/crates/nzb-web/tests/workflow_fixtures.rs index 535bd867..d57ca104 100644 --- a/crates/nzb-web/tests/workflow_fixtures.rs +++ b/crates/nzb-web/tests/workflow_fixtures.rs @@ -109,13 +109,10 @@ fn fixture_nzb(subject: &str) -> Vec { .into_bytes() } -async fn wait_for_queue_size(queue: &QueueManager, expected: usize) -> bool { - tokio::time::timeout(Duration::from_secs(2), async { - loop { - if queue.queue_size() == expected { - break; - } - tokio::time::sleep(Duration::from_millis(10)).await; +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 @@ -144,7 +141,7 @@ async fn existing_zip_of_nzbs_is_imported_and_moved_to_processed() { let queue = watch_queue(temp.path()); let watcher_task = tokio::spawn(DirWatcher::new(watch_dir.clone(), queue.clone()).run()); - let imported = wait_for_queue_size(&queue, 2).await; + let imported = wait_until(Duration::from_secs(2), || queue.queue_size() == 2).await; watcher_task.abort(); assert!( @@ -171,7 +168,7 @@ async fn existing_bzip2_nzb_is_imported_and_moved_to_processed() { let queue = watch_queue(temp.path()); let watcher_task = tokio::spawn(DirWatcher::new(watch_dir.clone(), queue.clone()).run()); - let imported = wait_for_queue_size(&queue, 1).await; + 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"); @@ -179,3 +176,82 @@ async fn existing_bzip2_nzb_is_imported_and_moved_to_processed() { 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()); +} From 87635fa1ce6fa6e7b54c53b85f6456b9cb2205a6 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:45:27 -0500 Subject: [PATCH 08/17] fix(sab): accept .nzb.gz, .nzb.bz2 and .zip on addfile/addurl SAB addfile/addurl passed compressed NZBs straight to the XML parser, which rejects them, while the native add endpoint already unpacked .gz and .zip. Move that extraction (and its decompression-bomb limits) from the app crate into a shared nzb_web::nzb_archive module, used by both the native handlers and the SAB layer, and add .bz2 support there (bzip2 0.6 was already in the dependency tree via zip's default features; no new crates are locked). addfile unpacks the upload; a multi-NZB zip enqueues each NZB as its own job and reports every nzo_id. addurl unpacks the fetched body, naming the job after the inner NZB, and hands multi-NZB zips to addfile. The addurl test reaches its loopback fixture through the same cfg(test)-only fetch_guard seam (ALLOW_LOOPBACK_FOR_TESTS) introduced on the nzbname branch; the hunk is identical. Fixes #83 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 066b77f5baff610092b31a5d0bec19528b188b6c) --- apps/rustnzb/src/handlers.rs | 7 +- crates/nzb-web/src/sabnzbd_compat.rs | 302 ++++++++++++++++++++++++++- 2 files changed, 301 insertions(+), 8 deletions(-) diff --git a/apps/rustnzb/src/handlers.rs b/apps/rustnzb/src/handlers.rs index ef1e897a..ad712e63 100644 --- a/apps/rustnzb/src/handlers.rs +++ b/apps/rustnzb/src/handlers.rs @@ -19,7 +19,6 @@ static HTTP_CLIENT: std::sync::LazyLock = std::sync::LazyLock:: .expect("Failed to build shared HTTP client") }); -use nzb_web::nzb_archive::extract_nzbs; #[cfg(feature = "webdav")] use nzb_web::nzb_core::config::DavConfig; use nzb_web::nzb_core::config::{ @@ -35,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}; @@ -357,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>, @@ -389,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)?); } @@ -2385,6 +2385,7 @@ pub async fn h_dav_config_set( mod tests { use super::{MAX_RSS_REGEX_LEN, compile_rss_regex, sanitize_server_config}; + use nzb_web::nzb_archive::MAX_NZB_DECOMPRESSED_BYTES; use nzb_web::nzb_core::config::ServerConfig; #[test] diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 7b8fa9ec..52c36e5e 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -217,11 +217,62 @@ async fn handle_addurl( let data = crate::fetch_guard::read_response_bytes_limited(response, MAX_ADDURL_BODY_BYTES).await?; - let job_name = addurl_job_name( - nzbname.as_deref(), - content_disposition.as_deref(), - &fetch_plan.url, - ); + // 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) => { @@ -631,6 +682,32 @@ async fn dispatch_post( } }; + // 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() @@ -643,6 +720,7 @@ async fn dispatch_post( .to_string() }); + match nzb_parser::parse_nzb(&job_name, &data) { Ok(mut job) => { if let Some(ref c) = cat @@ -770,6 +848,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 { @@ -3621,6 +3765,7 @@ mod tests { assert!(malformed["error"].is_string()); } + /// SABnzbd's PAUSED_PRIORITY (-2) adds the job paused rather than at a /// queue priority; the slot keeps reporting a normal priority. #[tokio::test] @@ -3711,6 +3856,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. @@ -3732,6 +4023,7 @@ mod tests { assert_eq!(addfile_with_category(&test_state, "*").await, "Default"); } + fn config_request(name: &str, value: &str) -> SabApiRequest { SabApiRequest { mode: Some("config".into()), From 5d505b2bc44c3c2f9a9b28a4b7fa1b987a52b420 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:47:52 -0500 Subject: [PATCH 09/17] fix(sab): give mode=server_stats SABnzbd's response shape mode=server_stats was aliased to fullstatus, so clients got the {"status": {...}} dashboard document instead of SABnzbd's server_stats shape. Add a dedicated handler that remaps the native per-server statistics (QueueManager::server_stats_get_all, as served by GET /api/config/servers/stats) into SABnzbd's layout: overall total/month/week/day bytes and a per-server map keyed by server name with total/month/week/day, daily, articles_tried and articles_success. Fixes #85 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 205e70d341142b4d6e11bf9f24ccb5e89daf06c4) --- crates/nzb-web/src/sabnzbd_compat.rs | 95 +++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 1 deletion(-) diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 52c36e5e..43b587b0 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -950,7 +950,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), @@ -1072,6 +1074,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 // --------------------------------------------------------------------------- @@ -4281,6 +4332,48 @@ mod tests { 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); + } + /// SABnzbd's real `_api_queue_delete` accepts a comma-separated `value` /// list, removing every matching job in one call. #[tokio::test] From ebfb91c3cb33a24ec241d5e94f624000163b1318 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:49:48 -0500 Subject: [PATCH 10/17] fix(sab): answer mode=warnings instead of "Unknown mode" mode=warnings is a real SABnzbd mode that clients poll, but RustNZB rejected it as unknown. Answer with SABnzbd's {"warnings": [...]} shape. RustNZB keeps no SABnzbd-style warnings list (fullstatus already reports warnings: [] and have_warnings "0"), so the list is empty and consistent with fullstatus; name=clear returns {"status": true}. Fixes #87 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 83938f6f072075f46ea577058a7cb75e4322f4fe) --- crates/nzb-web/src/sabnzbd_compat.rs | 29 ++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 43b587b0..bc8595d2 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -962,6 +962,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}") @@ -4374,6 +4383,26 @@ mod tests { 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] From 098bf57a620bf6feeee916c1cd3277a7f1a2ef9d Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 21:00:48 -0500 Subject: [PATCH 11/17] fix(dispatch): keep pipelined article errors off the circuit breaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit faebca6) Adapted: our pipeline error path already caps per-article outage retries (server-wide vs article-outage vs fail-over). Only the circuit-breaker accounting (record_failure / report_provider_outage) is gated by counts_against_circuit_breaker; the outage-try cap is unchanged. Connect-time failures still always count — they are provider-level. --- crates/nzb-dispatch/src/download_engine.rs | 71 +++++++++++++--- .../nzb-web/tests/harness_article_errors.rs | 83 +++++++++++++++++++ 2 files changed, 143 insertions(+), 11 deletions(-) diff --git a/crates/nzb-dispatch/src/download_engine.rs b/crates/nzb-dispatch/src/download_engine.rs index 85a8ea2d..34a2a987 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,22 @@ 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 and +/// 502. Article-level answers never do. +fn counts_against_circuit_breaker(e: &NntpError) -> bool { + matches!( + e, + NntpError::Connection(_) + | NntpError::Io(_) + | NntpError::Tls(_) + | NntpError::Auth(_) + | NntpError::AuthRequired(_) + | NntpError::PermissionDenied(_) + | NntpError::ServiceUnavailable(_) + ) +} + /// 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(); @@ -4417,4 +4441,29 @@ 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()), + ] { + assert!(counts_against_circuit_breaker(&e), "{e}"); + } + for e in [ + NntpError::ArticleNotFound(s()), + NntpError::NoSuchGroup(s()), + NntpError::NoArticleSelected(s()), + NntpError::Protocol(s()), + NntpError::Timeout(s()), + ] { + assert!(!counts_against_circuit_breaker(&e), "{e}"); + } + } + } 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()), + } +} From 03a1f855f493a90461284d09bba365d42d742126 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 21:45:35 -0500 Subject: [PATCH 12/17] fix(dispatch): apply speed-limit changes to parked bandwidth waiters acquire_download loaded the limiter once and awaited it for the whole article in burst-sized chunks. At a very low limit (20 B/s) one ~750 KB article parks a worker for hours, and set_download_bps only swapped the ArcSwap: parked acquires kept waiting on the old limiter, so raising the limit or clearing it to 0 had no effect. Because every worker connection stayed parked, new jobs stalled too, and only a restart recovered. The limiter now signals a Notify on every reconfiguration. Each chunk wait races the limiter against that notification, which is registered before the limiter is loaded so a change cannot be missed. On a change the acquire reloads the current limiter and continues with the bytes still owed. With no limit configured it returns immediately. No lock is held across an await. Tests: unit tests in bandwidth.rs (20 B/s to unlimited and to 10 MB/s release a parked acquire within 2 s; unlimited never waits), plus harness_speed_limit.rs, which drives a real QueueManager against the mock NNTP server: a limit set at startup, a limit set before a job is added, and throttle/unthrottle mid-download. Fixes #91 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 1050af77e764eea34aff381d06939d7450c75f76) --- crates/nzb-dispatch/src/bandwidth.rs | 102 ++++++++++-- crates/nzb-web/tests/harness_speed_limit.rs | 165 ++++++++++++++++++++ 2 files changed, 256 insertions(+), 11 deletions(-) create mode 100644 crates/nzb-web/tests/harness_speed_limit.rs 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-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; +} From b1536a23ca60b2e84eb281adeb37b6989e3d69be Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 22:28:00 -0500 Subject: [PATCH 13/17] fix(queue): give same-named jobs unique complete folders Two jobs with the same name derived the same output folder at add time and both completed into it, so the second release overwrote or merged into the first and both history rows reported the same storage path. Reserve the complete folder when a job first writes output (direct unpack, post-processing, or the final move) rather than at add time, so queued jobs never hold a name. The reservation uses create_dir and retries with `.1`, `.2`, ... on AlreadyExists, as SABnzbd does, so two jobs finishing together cannot pick the same folder. The chosen folder is recorded on the job and therefore in history, and in the job checkpoint so an interrupted post-processing run resumes into its own folder instead of reserving another. A failed job releases an empty reservation, and a rename or category change drops it. Fixes #93 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 2ef62075bfaa32604e3379c357c25f9d02aa8083) Adapted: kept our post_processing_level() helper (job override wins over the category default) and our sanitize_job_name rename path; the output dir claim and release calls from upstream are applied on top of both. --- crates/nzb-web/src/queue_manager.rs | 261 +++++++++++++++++- .../tests/harness_unique_output_dir.rs | 184 ++++++++++++ 2 files changed, 438 insertions(+), 7 deletions(-) create mode 100644 crates/nzb-web/tests/harness_unique_output_dir.rs diff --git a/crates/nzb-web/src/queue_manager.rs b/crates/nzb-web/src/queue_manager.rs index a97349fe..b4fa5a8a 100644 --- a/crates/nzb-web/src/queue_manager.rs +++ b/crates/nzb-web/src/queue_manager.rs @@ -87,6 +87,101 @@ 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 + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ServerStatsData { pub server_id: String, @@ -329,6 +424,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 +443,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 +487,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 +1018,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. @@ -1588,6 +1692,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 { @@ -1610,6 +1715,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); @@ -1693,6 +1799,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, @@ -1991,6 +2101,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, @@ -2353,6 +2472,8 @@ impl QueueManager { let cat = state.job.category.clone(); 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 @@ -2369,6 +2490,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 @@ -2717,6 +2840,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}"); } @@ -2777,7 +2901,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(), @@ -2870,6 +3004,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) { @@ -2888,7 +3041,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) @@ -3299,6 +3452,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(()) @@ -3328,6 +3482,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(()) @@ -4126,6 +4281,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(); @@ -4157,6 +4313,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 @@ -4178,6 +4335,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 @@ -4200,6 +4370,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); @@ -4572,6 +4743,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); @@ -5053,10 +5225,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); @@ -5224,6 +5397,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/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() + ); + } +} From f8dac41fc2a17d52469789dbfd7d0bd39ef70fd6 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 8 Oct 2026 22:43:39 -0500 Subject: [PATCH 14/17] fix(queue): reclaim retained incomplete work dirs (cherry picked from commit 60eeb1e) Adapted: the new incomplete-dir sweep helpers were merged alongside our existing claim_unique_output_dir / restore_claimed_output_dir helpers; neither replaced the other. --- crates/nzb-web/src/queue_manager.rs | 271 +++++++++++++++++- crates/nzb-web/src/sabnzbd_compat.rs | 27 ++ .../tests/harness_incomplete_cleanup.rs | 246 ++++++++++++++++ 3 files changed, 532 insertions(+), 12 deletions(-) create mode 100644 crates/nzb-web/tests/harness_incomplete_cleanup.rs diff --git a/crates/nzb-web/src/queue_manager.rs b/crates/nzb-web/src/queue_manager.rs index b4fa5a8a..a1a138dd 100644 --- a/crates/nzb-web/src/queue_manager.rs +++ b/crates/nzb-web/src/queue_manager.rs @@ -182,6 +182,63 @@ fn restore_claimed_output_dir(job: &mut NzbJob, checkpoint: &JobCheckpoint) -> b } } +/// 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, @@ -3429,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); } @@ -4096,26 +4155,186 @@ 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. + fn referenced_work_dirs( + &self, + only_named: Option<&std::ffi::OsStr>, + ) -> HashSet { + 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(); + match db.queue_list() { + Ok(jobs) => paths.extend(jobs.into_iter().map(|job| job.work_dir)), + Err(e) => warn!("Unable to list queue while checking work directories: {e}"), + } + match db.history_list(i64::MAX as usize) { + Ok(entries) => { + for entry in entries { + paths.push(incomplete.join(&entry.id)); + if entry.status != JobStatus::Failed { + continue; + } + let Ok(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); + } + } + } + Err(e) => warn!("Unable to list history while checking work directories: {e}"), + } + } + 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; + }; + if self + .referenced_work_dirs(work_dir.file_name()) + .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 referenced = self.referenced_work_dirs(None); + 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 { @@ -4246,6 +4465,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()? @@ -5081,6 +5304,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(); diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index bc8595d2..61d7d4bd 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -4479,6 +4479,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); 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..7eba4504 --- /dev/null +++ b/crates/nzb-web/tests/harness_incomplete_cleanup.rs @@ -0,0 +1,246 @@ +//! 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" + ); +} From 13932cbbaf10dd46c0993f35334430eb42bcfccf Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 05:21:28 +0000 Subject: [PATCH 15/17] style: cargo fmt after the xbmc batch 2 cherry-picks --- apps/rustnzb/src/handlers.rs | 1 - crates/nzb-dispatch/src/download_engine.rs | 1 - crates/nzb-web/src/sabnzbd_compat.rs | 20 ++++++++------------ 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/apps/rustnzb/src/handlers.rs b/apps/rustnzb/src/handlers.rs index ad712e63..37f5c888 100644 --- a/apps/rustnzb/src/handlers.rs +++ b/apps/rustnzb/src/handlers.rs @@ -2385,7 +2385,6 @@ pub async fn h_dav_config_set( mod tests { use super::{MAX_RSS_REGEX_LEN, compile_rss_regex, sanitize_server_config}; - use nzb_web::nzb_archive::MAX_NZB_DECOMPRESSED_BYTES; use nzb_web::nzb_core::config::ServerConfig; #[test] diff --git a/crates/nzb-dispatch/src/download_engine.rs b/crates/nzb-dispatch/src/download_engine.rs index 34a2a987..a6a606cb 100644 --- a/crates/nzb-dispatch/src/download_engine.rs +++ b/crates/nzb-dispatch/src/download_engine.rs @@ -4465,5 +4465,4 @@ mod tests { assert!(!counts_against_circuit_breaker(&e), "{e}"); } } - } diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 61d7d4bd..4369c0d5 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -273,7 +273,6 @@ async fn handle_addurl( ) }; - match nzb_parser::parse_nzb(&job_name, &data) { Ok(mut job) => { if let Some(ref c) = cat @@ -720,7 +719,6 @@ async fn dispatch_post( .to_string() }); - match nzb_parser::parse_nzb(&job_name, &data) { Ok(mut job) => { if let Some(ref c) = cat @@ -3825,7 +3823,6 @@ mod tests { assert!(malformed["error"].is_string()); } - /// SABnzbd's PAUSED_PRIORITY (-2) adds the job paused rather than at a /// queue priority; the slot keeps reporting a normal priority. #[tokio::test] @@ -3918,16 +3915,14 @@ mod tests { fn gzip(data: &[u8]) -> Vec { use std::io::Write as _; - let mut encoder = - flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + 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()); + let mut encoder = bzip2::write::BzEncoder::new(Vec::new(), bzip2::Compression::default()); encoder.write_all(data).unwrap(); encoder.finish().unwrap() } @@ -3984,8 +3979,7 @@ mod tests { 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; + 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"]); @@ -4057,11 +4051,14 @@ mod tests { .await .expect("addurl response") .0; - assert_eq!(response["status"], serde_json::json!(true), "resp={response}"); + 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. @@ -4083,7 +4080,6 @@ mod tests { assert_eq!(addfile_with_category(&test_state, "*").await, "Default"); } - fn config_request(name: &str, value: &str) -> SabApiRequest { SabApiRequest { mode: Some("config".into()), From 1b6009e9fa3c14db8cf9e650527bc2a51dad2eca Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 05:51:43 +0000 Subject: [PATCH 16/17] chore(desktop): refresh src-tauri lockfile for nzb-web 0.4.23 The batch 2 port bumped nzb-web to 0.4.23 and added bzip2 and zip to its dependency list, which left desktop/src-tauri/Cargo.lock stale under `cargo build --locked`. Only the desktop lockfile changes; no dependency versions were upgraded. --- desktop/src-tauri/Cargo.lock | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) 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]] From 54e49e0856f11f88aaac0a43fdd72c19392450f6 Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 06:41:11 +0000 Subject: [PATCH 17/17] fix(web): close the three security blockers from the batch 2 review - Budget zip extraction on bytes actually read instead of the declared entry size, cap the number of NZB entries, and reject archives whose central-directory entries overlap. - Make the incomplete-dir reclaim fail closed: an error from queue_list, history_list or a per-row history_get_retry_data skips every removal in both the startup sweep and single-directory removal. - Slice the ".nzb" suffix in clean_nzb_name on a char boundary, so non-ASCII names no longer panic. Each has a regression test. Timeouts now also count against the circuit breaker: a server that accepts the connection and then black-holes the read is as unhealthy as one that refuses it. --- crates/nzb-dispatch/src/download_engine.rs | 9 +- crates/nzb-web/src/nzb_archive.rs | 129 +++++++++++++++--- crates/nzb-web/src/queue_manager.rs | 64 ++++----- crates/nzb-web/src/sabnzbd_compat.rs | 21 ++- .../tests/harness_incomplete_cleanup.rs | 55 ++++++++ 5 files changed, 223 insertions(+), 55 deletions(-) diff --git a/crates/nzb-dispatch/src/download_engine.rs b/crates/nzb-dispatch/src/download_engine.rs index a6a606cb..fdb2cdf8 100644 --- a/crates/nzb-dispatch/src/download_engine.rs +++ b/crates/nzb-dispatch/src/download_engine.rs @@ -2836,8 +2836,10 @@ fn all_enabled_providers_definitive( } /// Whether a fetch error says the provider itself is unhealthy, and so should -/// count against its circuit breaker: connection/TLS, auth, permission and -/// 502. Article-level answers never do. +/// 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, @@ -2848,6 +2850,7 @@ fn counts_against_circuit_breaker(e: &NntpError) -> bool { | NntpError::AuthRequired(_) | NntpError::PermissionDenied(_) | NntpError::ServiceUnavailable(_) + | NntpError::Timeout(_) ) } @@ -4452,6 +4455,7 @@ mod tests { NntpError::AuthRequired(s()), NntpError::PermissionDenied(s()), NntpError::ServiceUnavailable(s()), + NntpError::Timeout(s()), ] { assert!(counts_against_circuit_breaker(&e), "{e}"); } @@ -4460,7 +4464,6 @@ mod tests { NntpError::NoSuchGroup(s()), NntpError::NoArticleSelected(s()), NntpError::Protocol(s()), - NntpError::Timeout(s()), ] { assert!(!counts_against_circuit_breaker(&e), "{e}"); } diff --git a/crates/nzb-web/src/nzb_archive.rs b/crates/nzb-web/src/nzb_archive.rs index c6dbd5a9..31a7c6ce 100644 --- a/crates/nzb-web/src/nzb_archive.rs +++ b/crates/nzb-web/src/nzb_archive.rs @@ -10,21 +10,34 @@ 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")?; + 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")?; + 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)]); } @@ -34,6 +47,15 @@ pub fn extract_nzbs(file_name: &str, data: &[u8]) -> Result 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() { @@ -42,23 +64,25 @@ pub fn extract_nzbs(file_name: &str, data: &[u8]) -> Result .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 - ); + 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(MAX_NZB_DECOMPRESSED_BYTES + 1) + .take(room) .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 { + 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 @@ -77,13 +101,17 @@ pub fn extract_nzbs(file_name: &str, data: &[u8]) -> Result Ok(vec![(file_name.to_string(), data.to_vec())]) } -fn read_bounded(reader: impl std::io::Read, format: &str) -> Result, anyhow::Error> { +fn read_bounded( + reader: impl std::io::Read, + format: &str, + max_bytes: u64, +) -> Result, anyhow::Error> { let mut decompressed = Vec::new(); reader - .take(MAX_NZB_DECOMPRESSED_BYTES + 1) + .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_NZB_DECOMPRESSED_BYTES { + if decompressed.len() as u64 > max_bytes { anyhow::bail!( "Decompressed NZB exceeds the {} MB limit", MAX_NZB_DECOMPRESSED_BYTES / 1024 / 1024 @@ -94,7 +122,7 @@ fn read_bounded(reader: impl std::io::Read, format: &str) -> Result, any #[cfg(test)] mod tests { - use super::{MAX_NZB_DECOMPRESSED_BYTES, extract_nzbs}; + use super::{extract_nzbs, extract_nzbs_bounded}; use std::io::Write; use zip::CompressionMethod; @@ -113,12 +141,73 @@ mod tests { 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 oversized = vec![b'x'; (MAX_NZB_DECOMPRESSED_BYTES + 1) as usize]; + 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("oversized.zip", &zip).unwrap_err(); - assert!(err.to_string().contains("100 MB limit")); + let err = extract_nzbs_bounded("oversized.zip", &zip, cap).unwrap_err(); + assert!(err.to_string().contains("limit")); } #[test] diff --git a/crates/nzb-web/src/queue_manager.rs b/crates/nzb-web/src/queue_manager.rs index a1a138dd..00e69747 100644 --- a/crates/nzb-web/src/queue_manager.rs +++ b/crates/nzb-web/src/queue_manager.rs @@ -4214,10 +4214,14 @@ impl QueueManager { /// 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>, - ) -> HashSet { + ) -> 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. @@ -4231,36 +4235,27 @@ impl QueueManager { let needle = only_named.map(|name| name.to_string_lossy().into_owned().into_bytes()); { let db = self.db.lock(); - match db.queue_list() { - Ok(jobs) => paths.extend(jobs.into_iter().map(|job| job.work_dir)), - Err(e) => warn!("Unable to list queue while checking work directories: {e}"), - } - match db.history_list(i64::MAX as usize) { - Ok(entries) => { - for entry in entries { - paths.push(incomplete.join(&entry.id)); - if entry.status != JobStatus::Failed { - continue; - } - let Ok(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); - } - } + 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); } - Err(e) => warn!("Unable to list history while checking work directories: {e}"), } } - paths + Ok(paths .into_iter() .filter_map(|path| std::fs::canonicalize(path).ok()) - .collect() + .collect()) } /// Remove one retained incomplete work directory once nothing references @@ -4273,10 +4268,14 @@ impl QueueManager { let Some(work_dir) = incomplete_child_dir(&root, candidate) else { return; }; - if self - .referenced_work_dirs(work_dir.file_name()) - .contains(&work_dir) - { + 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; } @@ -4309,7 +4308,10 @@ impl QueueManager { let Ok(entries) = std::fs::read_dir(&root) else { return; }; - let referenced = self.referenced_work_dirs(None); + 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; diff --git a/crates/nzb-web/src/sabnzbd_compat.rs b/crates/nzb-web/src/sabnzbd_compat.rs index 4369c0d5..13b2562e 100644 --- a/crates/nzb-web/src/sabnzbd_compat.rs +++ b/crates/nzb-web/src/sabnzbd_compat.rs @@ -375,7 +375,11 @@ fn addurl_job_name( /// 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[base.len() - 4..].eq_ignore_ascii_case(".nzb") { + 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 @@ -2745,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, diff --git a/crates/nzb-web/tests/harness_incomplete_cleanup.rs b/crates/nzb-web/tests/harness_incomplete_cleanup.rs index 7eba4504..f5b92f61 100644 --- a/crates/nzb-web/tests/harness_incomplete_cleanup.rs +++ b/crates/nzb-web/tests/harness_incomplete_cleanup.rs @@ -244,3 +244,58 @@ async fn startup_sweeps_unreferenced_incomplete_dirs_only() { "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" + ); +}