fix: never panic when the log directory is unwritable; disable file logging instead [#67]

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:?}");
 }
 
-  log_path.push("managarr.log");
+fn resolve_log_dir(preferred: PathBuf) -> PathBuf {
-  log_path
+  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()
+      );
+    });
+  }
+
+  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()
+  let roller = match FixedWindowRoller::builder().build(&archive_pattern, 3) {
-    .build(&archive_pattern, 3)
+    Ok(roller) => roller,
-    .expect("Failed to build log 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))
+    .build(&log_path, Box::new(policy))
-    .expect("Failed to build rolling file appender");
+  {
+    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<timestamp>\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3})\s+<(?P<opid>[^\s>]+)>\s+\[(?P<level>[A-Z]+)]\s+(?P<logger>[^:]+):(?P<line>\d+)\s+-\s+(?P<message>.*)$").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

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();
+  }
 }

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())

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}"
+  );
+}