diff --git a/apps/rustnzb/config.example.toml b/apps/rustnzb/config.example.toml index aa87782..c4a2691 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 878dd1d..3a169a6 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 8a85cdd..0605dca 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 7b9c6e3..3b45211 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 1b5ae8a..282e625 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 9947640..4ae81d5 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() {