From 0f230a1a4f293ec5e9ac6874ebf28de7857d80c4 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Mon, 12 Oct 2026 11:16:11 -0600 Subject: [PATCH] fix: never panic when the log directory is unwritable; disable file logging instead [#67] --- src/utils.rs | 85 ++++++-- src/utils_tests.rs | 78 +++++++- tests/servarr_name_cli_integration_tests.rs | 22 ++- ...nwritable_log_dir_cli_integration_tests.rs | 184 ++++++++++++++++++ 4 files changed, 341 insertions(+), 28 deletions(-) create mode 100644 tests/unwritable_log_dir_cli_integration_tests.rs diff --git a/src/utils.rs b/src/utils.rs index 43519f65..52f08d43 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -2,7 +2,7 @@ use std::fs::{self, File}; use std::io::{BufRead, BufReader, Seek, SeekFrom}; use std::path::PathBuf; use std::process; -use std::sync::Arc; +use std::sync::{Arc, Once}; use std::time::Duration; use anyhow::Result; @@ -34,52 +34,92 @@ use crate::ui::theme::ThemeDefinitionsWrapper; mod utils_tests; pub fn get_log_path() -> PathBuf { - let mut log_path = if cfg!(target_os = "linux") { + let preferred = if cfg!(target_os = "linux") { dirs_next::cache_dir().unwrap_or_else(|| PathBuf::from("~/.cache")) } else if cfg!(target_os = "macos") { - dirs_next::home_dir().unwrap().join("Library/Logs") + dirs_next::home_dir() + .map(|h| h.join("Library/Logs")) + .unwrap_or_else(std::env::temp_dir) } else { dirs_next::data_local_dir().unwrap_or_else(|| PathBuf::from("C:\\Logs")) - }; + } + .join("managarr"); - log_path.push("managarr"); + resolve_log_dir(preferred).join("managarr.log") +} - if let Err(e) = fs::create_dir_all(&log_path) { - eprintln!("Failed to create log directory: {e:?}"); +fn resolve_log_dir(preferred: PathBuf) -> PathBuf { + static WARN_ONCE: Once = Once::new(); + + if let Err(e) = fs::create_dir_all(&preferred) { + WARN_ONCE.call_once(|| { + eprintln!( + "Failed to create log directory {}: {e}; file logging will be disabled", + preferred.display() + ); + }); } - log_path.push("managarr.log"); - log_path + preferred } pub fn init_logging_config() -> log4rs::Config { - let log_path = get_log_path(); + build_logging_config(get_log_path()) +} + +fn build_logging_config(log_path: PathBuf) -> log4rs::Config { let archive_pattern = log_path .with_file_name("managarr.{}.log") .to_string_lossy() .into_owned(); let trigger = SizeTrigger::new(10 * 1024 * 1024); - let roller = FixedWindowRoller::builder() - .build(&archive_pattern, 3) - .expect("Failed to build log roller"); + let roller = match FixedWindowRoller::builder().build(&archive_pattern, 3) { + Ok(roller) => roller, + Err(e) => { + eprintln!( + "Failed to build log roller for {archive_pattern}: {e}; file logging will be disabled" + ); + return disabled_logging_config(); + } + }; let policy = CompoundPolicy::new(Box::new(trigger), Box::new(roller)); - let logfile = RollingFileAppender::builder() + let logfile = match RollingFileAppender::builder() .encoder(Box::new(PatternEncoder::new( "{d(%Y-%m-%d %H:%M:%S%.3f)(utc)} <{i}> [{l}] {f}:{L} - {m}{n}", ))) - .build(log_path, Box::new(policy)) - .expect("Failed to build rolling file appender"); + .build(&log_path, Box::new(policy)) + { + Ok(logfile) => logfile, + Err(e) => { + eprintln!( + "Failed to build rolling file appender for {}: {e}; file logging will be disabled", + log_path.display() + ); + return disabled_logging_config(); + } + }; - log4rs::Config::builder() + match log4rs::Config::builder() .appender(Appender::builder().build("logfile", Box::new(logfile))) .build( Root::builder() .appender("logfile") .build(LevelFilter::Debug), - ) - .unwrap() + ) { + Ok(config) => config, + Err(e) => { + eprintln!("Failed to build logging config: {e}; file logging will be disabled"); + disabled_logging_config() + } + } +} + +fn disabled_logging_config() -> log4rs::Config { + log4rs::Config::builder() + .build(Root::builder().build(LevelFilter::Debug)) + .expect("appender-free logging config is infallibly valid") } pub fn format_size(bytes: i64, precision: usize) -> String { @@ -102,7 +142,12 @@ pub fn convert_runtime(runtime: i64) -> (i64, i64) { pub async fn tail_logs(no_color: bool) -> Result<()> { let re = Regex::new(r"^(?P\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3})\s+<(?P[^\s>]+)>\s+\[(?P[A-Z]+)]\s+(?P[^:]+):(?P\d+)\s+-\s+(?P.*)$").unwrap(); let file_path = get_log_path(); - let file = File::open(&file_path).expect("Cannot open file"); + let file = File::open(&file_path).with_context(|| { + format!( + "Unable to open log file {}; file logging may be disabled", + file_path.display() + ) + })?; let mut reader = BufReader::new(file); reader diff --git a/src/utils_tests.rs b/src/utils_tests.rs index 07336545..fbd20a24 100644 --- a/src/utils_tests.rs +++ b/src/utils_tests.rs @@ -15,7 +15,8 @@ mod tests { app::{App, AppConfig, ServarrConfig}, network::NetworkError, utils::{ - convert_runtime, format_cli_error, format_size, select_cli_configuration, was_log_rotated, + build_logging_config, convert_runtime, format_cli_error, format_size, resolve_log_dir, + select_cli_configuration, was_log_rotated, }, }; @@ -236,4 +237,79 @@ mod tests { fs::remove_file(&path).unwrap(); } + + #[test] + fn test_resolve_log_dir_returns_preferred_dir_when_creatable() { + let preferred = std::env::temp_dir().join(format!( + "managarr_test_preferred_log_dir_{}", + std::process::id() + )); + let _ = fs::remove_dir_all(&preferred); + + let resolved = resolve_log_dir(preferred.clone()); + + assert_eq!(resolved, preferred); + assert!(preferred.is_dir()); + + fs::remove_dir_all(&preferred).unwrap(); + } + + #[test] + fn test_resolve_log_dir_keeps_preferred_dir_and_disables_file_logging_when_uncreatable() { + let blocker = std::env::temp_dir().join(format!( + "managarr_test_log_dir_blocker_{}", + std::process::id() + )); + let _ = fs::remove_file(&blocker); + fs::write(&blocker, "not a directory").unwrap(); + let preferred = blocker.join("logs"); + + let resolved = resolve_log_dir(preferred.clone()); + + assert_eq!(resolved, preferred); + assert!(!resolved.exists()); + + let config = build_logging_config(resolved.join("managarr.log")); + + assert!(config.appenders().is_empty()); + assert!(config.root().appenders().is_empty()); + + fs::remove_file(&blocker).unwrap(); + } + + #[test] + fn test_build_logging_config_uses_logfile_appender_when_log_path_is_writable() { + let log_dir = std::env::temp_dir().join(format!( + "managarr_test_logging_config_{}", + std::process::id() + )); + let _ = fs::remove_dir_all(&log_dir); + fs::create_dir_all(&log_dir).unwrap(); + + let config = build_logging_config(log_dir.join("managarr.log")); + + assert_eq!(config.appenders().len(), 1); + assert_str_eq!(config.appenders()[0].name(), "logfile"); + assert_eq!(config.root().appenders(), ["logfile"]); + + drop(config); + fs::remove_dir_all(&log_dir).unwrap(); + } + + #[test] + fn test_build_logging_config_disables_logging_when_appender_is_unbuildable() { + let blocker = std::env::temp_dir().join(format!( + "managarr_test_appender_blocker_{}", + std::process::id() + )); + let _ = fs::remove_file(&blocker); + fs::write(&blocker, "not a directory").unwrap(); + + let config = build_logging_config(blocker.join("logs").join("managarr.log")); + + assert!(config.appenders().is_empty()); + assert!(config.root().appenders().is_empty()); + + fs::remove_file(&blocker).unwrap(); + } } diff --git a/tests/servarr_name_cli_integration_tests.rs b/tests/servarr_name_cli_integration_tests.rs index 99e4ec98..5f9d0194 100644 --- a/tests/servarr_name_cli_integration_tests.rs +++ b/tests/servarr_name_cli_integration_tests.rs @@ -5,7 +5,7 @@ use std::{ sync::atomic::{AtomicU64, Ordering}, }; -use assert_cmd::cargo::cargo_bin_cmd; +use assert_cmd::{Command, cargo::cargo_bin_cmd}; use mockito::Server; static NEXT_CONFIG_ID: AtomicU64 = AtomicU64::new(0); @@ -36,6 +36,14 @@ impl Drop for TemporaryConfig { } } +fn managarr_command() -> Command { + let home = std::env::temp_dir().join(format!("managarr-servarr-name-home-{}", process::id())); + fs::create_dir_all(&home).unwrap(); + let mut command = cargo_bin_cmd!("managarr"); + command.env("HOME", &home).env("XDG_CACHE_HOME", &home); + command +} + #[test] fn named_instance_of_different_servarr_type_rejects_command() { let mut radarr_server = Server::new(); @@ -62,7 +70,7 @@ fn named_instance_of_different_servarr_type_rejects_command() { sonarr_server.url() )); - let mut command = cargo_bin_cmd!("managarr"); + let mut command = managarr_command(); let assertion = command .arg("--config-file") .arg(config.path()) @@ -112,7 +120,7 @@ fn named_readarr_instance_selected_for_readarr_command() { readarr_server.url() )); - let mut command = cargo_bin_cmd!("managarr"); + let mut command = managarr_command(); let assertion = command .arg("--config-file") .arg(config.path()) @@ -163,7 +171,7 @@ fn same_named_servarrs_select_command_matching_instance() { sonarr_server.url() )); - let mut command = cargo_bin_cmd!("managarr"); + let mut command = managarr_command(); let assertion = command .arg("--config-file") .arg(config.path()) @@ -204,7 +212,7 @@ fn unknown_servarr_name_fails() { "radarr:\n - name: Movies\n uri: {}\n api_token: test-token\n", radarr_server.url() )); - let mut command = cargo_bin_cmd!("managarr"); + let mut command = managarr_command(); let assertion = command .arg("--config-file") .arg(config.path()) @@ -243,7 +251,7 @@ fn whitespace_servarr_name_trimmed() { "radarr:\n - name: Movies\n uri: {}\n api_token: test-token\n", radarr_server.url() )); - let mut command = cargo_bin_cmd!("managarr"); + let mut command = managarr_command(); let assertion = command .arg("--config-file") .arg(config.path()) @@ -293,7 +301,7 @@ fn no_servarr_name_uses_first_configured_instance() { radarr_server2.url() )); - let mut command = cargo_bin_cmd!("managarr"); + let mut command = managarr_command(); let assertion = command .arg("--config-file") .arg(config.path()) diff --git a/tests/unwritable_log_dir_cli_integration_tests.rs b/tests/unwritable_log_dir_cli_integration_tests.rs new file mode 100644 index 00000000..17ffcad4 --- /dev/null +++ b/tests/unwritable_log_dir_cli_integration_tests.rs @@ -0,0 +1,184 @@ +#![cfg(unix)] + +use std::{fs, path::PathBuf, process, str}; + +#[cfg(target_os = "linux")] +use assert_cmd::Command; +use assert_cmd::cargo::cargo_bin_cmd; + +struct ProbeEnv { + base: PathBuf, +} + +impl ProbeEnv { + fn new(test_name: &str) -> Self { + let base = std::env::temp_dir().join(format!( + "managarr_unwritable_log_dir_{test_name}_{}", + process::id() + )); + let _ = fs::remove_dir_all(&base); + fs::create_dir_all(base.join("home")).unwrap(); + fs::create_dir_all(base.join("fresh_tmp")).unwrap(); + fs::write(base.join("blockfile"), "not a directory").unwrap(); + fs::write( + base.join("config.yml"), + "radarr:\n - name: Movies\n host: 127.0.0.1\n port: 7878\n api_token: test-token\n", + ) + .unwrap(); + + Self { base } + } + + #[cfg(target_os = "linux")] + fn blocked_cache_dir(&self) -> PathBuf { + self.base.join("blockfile").join("cache") + } + + #[cfg(target_os = "linux")] + fn command(&self) -> Command { + let mut command = cargo_bin_cmd!("managarr"); + command + .env_clear() + .env("PATH", std::env::var_os("PATH").unwrap()) + .env("HOME", self.base.join("home")) + .env("XDG_CACHE_HOME", self.blocked_cache_dir()) + .env("TMPDIR", self.base.join("fresh_tmp")) + .arg("--config-file") + .arg(self.base.join("config.yml")); + command + } +} + +impl Drop for ProbeEnv { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.base); + } +} + +#[cfg(target_os = "linux")] +#[test] +fn unwritable_log_dir_warns_and_cli_command_still_succeeds() { + let env = ProbeEnv::new("completions"); + let expected_log_dir = env.blocked_cache_dir().join("managarr"); + + let mut command = env.command(); + let assertion = command.args(["completions", "bash"]).assert().success(); + let output = assertion.get_output(); + let stderr = str::from_utf8(&output.stderr).unwrap(); + + assert!( + !output.stdout.is_empty(), + "completions produced no stdout despite exiting successfully" + ); + assert!( + stderr.contains(&format!( + "Failed to create log directory {}", + expected_log_dir.display() + )), + "stderr did not warn about the uncreatable log directory: {stderr}" + ); + assert!( + stderr.contains("file logging will be disabled"), + "stderr did not announce that file logging is disabled: {stderr}" + ); + assert!( + !stderr.contains("panicked"), + "binary panicked instead of degrading gracefully: {stderr}" + ); + assert!( + fs::read_dir(env.base.join("fresh_tmp")).unwrap().count() == 0, + "log output fell back to a temp directory despite the no-fallback ruling" + ); +} + +#[cfg(target_os = "linux")] +#[test] +fn tail_logs_with_unopenable_log_file_returns_error_without_panicking() { + let env = ProbeEnv::new("tail_logs"); + let expected_log_file = env + .blocked_cache_dir() + .join("managarr") + .join("managarr.log"); + + let mut command = env.command(); + let assertion = command + .arg("tail-logs") + .timeout(std::time::Duration::from_secs(30)) + .assert() + .failure(); + let stderr = str::from_utf8(&assertion.get_output().stderr).unwrap(); + + assert!( + stderr.contains(&format!( + "Unable to open log file {}", + expected_log_file.display() + )), + "tail-logs error did not name the log file: {stderr}" + ); + assert_eq!( + stderr.matches("Failed to create log directory").count(), + 1, + "the create-dir warning must print exactly once per process: {stderr}" + ); + assert!( + stderr.contains("file logging may be disabled"), + "tail-logs error did not mention that file logging may be disabled: {stderr}" + ); + assert!( + !stderr.contains("panicked"), + "tail-logs panicked instead of returning an error: {stderr}" + ); +} + +#[cfg(target_os = "linux")] +#[test] +fn writable_cache_dir_creates_log_file_without_warnings() { + let env = ProbeEnv::new("happy_path"); + let cache_dir = env.base.join("home").join("cache"); + fs::create_dir_all(&cache_dir).unwrap(); + + let mut command = env.command(); + let assertion = command + .env("XDG_CACHE_HOME", &cache_dir) + .args(["completions", "bash"]) + .assert() + .success(); + let stderr = str::from_utf8(&assertion.get_output().stderr).unwrap(); + + assert!( + stderr.is_empty(), + "happy path emitted unexpected stderr: {stderr}" + ); + assert!( + cache_dir.join("managarr").join("managarr.log").is_file(), + "logfile appender did not create managarr.log in the writable cache dir" + ); +} + +#[test] +fn unset_home_does_not_panic_on_startup() { + let env = ProbeEnv::new("unset_home"); + + let mut command = cargo_bin_cmd!("managarr"); + let assertion = command + .env_clear() + .env("PATH", std::env::var_os("PATH").unwrap()) + .env("TMPDIR", env.base.join("fresh_tmp")) + .current_dir(&env.base) + .arg("--config-file") + .arg(env.base.join("config.yml")) + .args(["completions", "bash"]) + .assert() + .success(); + let output = assertion.get_output(); + let stderr = str::from_utf8(&output.stderr).unwrap(); + + assert!( + !output.stdout.is_empty(), + "completions produced no stdout despite exiting successfully" + ); + assert!( + !stderr.contains("panicked"), + "binary panicked instead of degrading gracefully: {stderr}" + ); +}