From e8530bc31b046bf096fe25a1b3484baad30a0121 Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:33:26 +0000 Subject: [PATCH] fix(history): treat history_retention=0 as keep-all (#136) A configured history retention of 0 ran the retention pass with LIMIT 0 right after each history insert. The subquery matched nothing, so the DELETE removed every row silently: the job logged "Moving job to history", no error appeared, and the history table stayed empty. Fold Some(0) into None at every entry point (queue manager setter, startup, both config handlers) via normalize_history_retention(), and make Database::history_enforce_retention(0) a no-op as a last line of defence. Add regression tests at the db, config and queue-manager level; the queue-manager zero-retention test fails on v1.4.7 and passes here. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01MPJczaZMiRkfwK3giJDVM5 --- apps/rustnzb/config.example.toml | 2 +- apps/rustnzb/src/handlers.rs | 12 +++++-- crates/nzb-core/src/config.rs | 20 ++++++++++- crates/nzb-core/src/db.rs | 21 ++++++++++++ crates/nzb-web/src/queue_manager.rs | 51 +++++++++++++++++++++++++++-- crates/nzb-web/src/startup.rs | 6 ++-- 6 files changed, 100 insertions(+), 12 deletions(-) diff --git a/apps/rustnzb/config.example.toml b/apps/rustnzb/config.example.toml index aa87782f..c4a26916 100644 --- a/apps/rustnzb/config.example.toml +++ b/apps/rustnzb/config.example.toml @@ -9,7 +9,7 @@ speed_limit_bps = 0 # 0 = unlimited cache_size = 524288000 # 500 MB log_level = "info" # log_file = "data/rustnzb.log" -# history_retention = 100 # Number of NZBs to keep in history (omit for keep all) +# history_retention = 100 # Number of NZBs to keep in history (omit or 0 = keep all) # Independent post-processing jobs overlap downloads. Separate repair and # extraction gates allow those stages to overlap safely across different jobs. # These limits are applied at process start. diff --git a/apps/rustnzb/src/handlers.rs b/apps/rustnzb/src/handlers.rs index 878dd1dd..3a169a62 100644 --- a/apps/rustnzb/src/handlers.rs +++ b/apps/rustnzb/src/handlers.rs @@ -24,7 +24,9 @@ const MAX_FETCH_BODY_BYTES: usize = 100 * 1024 * 1024; #[cfg(feature = "webdav")] use nzb_web::nzb_core::config::DavConfig; -use nzb_web::nzb_core::config::{CategoryConfig, RssFeedConfig, ServerConfig}; +use nzb_web::nzb_core::config::{ + CategoryConfig, RssFeedConfig, ServerConfig, normalize_history_retention, +}; use nzb_web::nzb_core::models::*; use nzb_web::nzb_core::nzb_parser; use nzb_web::nzb_core::sabnzbd_import; @@ -1111,10 +1113,13 @@ pub async fn h_history_retention_set( State(state): State>, Json(body): Json, ) -> Result, ApiError> { + // 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 = body.retention; + config.general.history_retention = retention; state.update_config(config).map_err(ApiError::from)?; - state.queue_manager.set_history_retention(body.retention); + state.queue_manager.set_history_retention(retention); Ok(Json(SimpleResponse { status: true })) } @@ -1547,6 +1552,7 @@ pub async fn h_general_update( 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; } diff --git a/crates/nzb-core/src/config.rs b/crates/nzb-core/src/config.rs index 8a85cdd2..0605dca2 100644 --- a/crates/nzb-core/src/config.rs +++ b/crates/nzb-core/src/config.rs @@ -54,7 +54,8 @@ pub struct GeneralConfig { pub log_level: String, /// Log file path (None = stdout only) pub log_file: Option, - /// History retention: how many NZBs to keep in history (None = keep all) + /// History retention: how many NZBs to keep in history. + /// `None` or `Some(0)` both mean keep all; see [`normalize_history_retention`]. pub history_retention: Option, /// Max number of NZBs downloading simultaneously (default 1) pub max_active_downloads: usize, @@ -141,6 +142,16 @@ fn default_article_timeout_secs() -> u64 { 30 } +/// Normalize a history retention limit so that `0` means "keep all". +/// +/// SABnzbd users (and this codebase's own `speed_limit_bps`) treat `0` as +/// unlimited. Enforcing a literal limit of zero would delete every history +/// row immediately after each completion (GH #136), so a zero is folded into +/// `None` at every entry point before it reaches the database. +pub fn normalize_history_retention(limit: Option) -> Option { + limit.filter(|max| *max > 0) +} + impl Default for GeneralConfig { fn default() -> Self { Self { @@ -408,6 +419,13 @@ mod tests { assert_eq!(cfg.max_nested_archive_depth, 5); } + #[test] + fn zero_history_retention_normalizes_to_keep_all() { + assert_eq!(normalize_history_retention(Some(0)), None); + assert_eq!(normalize_history_retention(None), None); + assert_eq!(normalize_history_retention(Some(25)), Some(25)); + } + #[test] fn direct_unpack_can_be_explicitly_disabled() { let cfg: GeneralConfig = toml::from_str("direct_unpack = false").unwrap(); diff --git a/crates/nzb-core/src/db.rs b/crates/nzb-core/src/db.rs index 7b9c6e31..3b452111 100644 --- a/crates/nzb-core/src/db.rs +++ b/crates/nzb-core/src/db.rs @@ -602,7 +602,15 @@ impl Database { } /// Enforce history retention limit by deleting oldest entries. + /// + /// A limit of `0` is treated as "keep all" and is a no-op: the + /// `LIMIT 0` subquery would otherwise match nothing and delete every + /// row (GH #136). Callers normalize zero away already; this is the last + /// line of defence. pub fn history_enforce_retention(&self, max_entries: usize) -> Result<(), NzbError> { + if max_entries == 0 { + return Ok(()); + } self.conn.execute( "DELETE FROM history WHERE id NOT IN ( SELECT id FROM history ORDER BY completed_at DESC LIMIT ?1 @@ -1305,6 +1313,19 @@ mod tests { assert_eq!(db.history_count().unwrap(), 3); } + /// GH #136: a retention limit of zero must not wipe history. + #[test] + fn test_history_enforce_retention_zero_keeps_all() { + let db = Database::open_memory().unwrap(); + for i in 0..3 { + db.history_insert(&make_history(&format!("ret0-{i}"), &format!("Job {i}"))) + .unwrap(); + } + + db.history_enforce_retention(0).unwrap(); + assert_eq!(db.history_count().unwrap(), 3); + } + #[test] fn test_history_store_and_get_logs() { let db = Database::open_memory().unwrap(); diff --git a/crates/nzb-web/src/queue_manager.rs b/crates/nzb-web/src/queue_manager.rs index 1b5ae8a0..282e6252 100644 --- a/crates/nzb-web/src/queue_manager.rs +++ b/crates/nzb-web/src/queue_manager.rs @@ -15,7 +15,7 @@ use serde::{Deserialize, Serialize}; use tokio::sync::{broadcast, mpsc}; use tracing::{debug, error, info, warn}; -use crate::nzb_core::config::{CategoryConfig, ServerConfig}; +use crate::nzb_core::config::{CategoryConfig, ServerConfig, normalize_history_retention}; use crate::nzb_core::db::Database; use crate::nzb_core::models::*; use crate::nzb_core::nzb_parser; @@ -876,9 +876,10 @@ impl QueueManager { } } - /// Set history retention limit. + /// Set history retention limit. `Some(0)` is normalized to `None` + /// (keep all) so a zero can never wipe history on completion (GH #136). pub fn set_history_retention(&self, limit: Option) { - *self.history_retention.lock() = limit; + *self.history_retention.lock() = normalize_history_retention(limit); } /// Current generation of the SAB-compatible history view. @@ -3924,6 +3925,50 @@ mod global_pause_tests { assert_eq!(manager.history_update(), 3); } + /// GH #136: a configured retention of 0 used to run `LIMIT 0` retention + /// right after the insert and silently delete the row just persisted. + #[tokio::test] + async fn zero_history_retention_keeps_completed_jobs() { + let (manager, tempdir) = manager(); + manager.set_history_retention(Some(0)); + assert_eq!(manager.get_history_retention(), None); + + insert_job( + &manager, + job("zero-retention", JobStatus::Completed, tempdir.path()), + ); + { + let mut jobs = manager.jobs.lock(); + manager.move_to_history(jobs.get_mut("zero-retention").unwrap(), Vec::new()); + } + + let db = manager.db.lock(); + let entry = db + .history_get("zero-retention") + .unwrap() + .expect("completed job must remain in history with retention 0"); + assert_eq!(entry.status, JobStatus::Completed); + assert_eq!(db.history_count().unwrap(), 1); + } + + /// A positive retention limit still prunes, oldest first. + #[tokio::test] + async fn positive_history_retention_prunes_after_completion() { + let (manager, tempdir) = manager(); + manager.set_history_retention(Some(1)); + assert_eq!(manager.get_history_retention(), Some(1)); + + for id in ["ret-first", "ret-second"] { + insert_job(&manager, job(id, JobStatus::Completed, tempdir.path())); + let mut jobs = manager.jobs.lock(); + manager.move_to_history(jobs.get_mut(id).unwrap(), Vec::new()); + } + + let db = manager.db.lock(); + assert_eq!(db.history_count().unwrap(), 1); + assert!(db.history_get("ret-second").unwrap().is_some()); + } + #[tokio::test] async fn failed_history_cleanup_removes_raw_work_directory_after_persistence() { let (manager, tempdir) = manager(); diff --git a/crates/nzb-web/src/startup.rs b/crates/nzb-web/src/startup.rs index 99476407..4ae81d57 100644 --- a/crates/nzb-web/src/startup.rs +++ b/crates/nzb-web/src/startup.rs @@ -172,10 +172,8 @@ pub async fn initialize( config.general.article_timeout_secs, ); - // Set history retention - if let Some(retention) = config.general.history_retention { - queue_manager.set_history_retention(Some(retention)); - } + // Set history retention (the setter folds 0 into "keep all"). + queue_manager.set_history_retention(config.general.history_retention); // Restore any in-progress jobs from the database if let Err(e) = queue_manager.restore_from_db() {