From e56311739709570ea38f2d260a3d2f5411e5dd23 Mon Sep 17 00:00:00 2001
From: Lorenz Leutgeb <lorenz.leutgeb@radicle.xyz>
Date: Fri, 19 Sep 2025 14:56:36 +0200
Subject: [PATCH] node: Refactor logging initialization

Logging initialization was infallible previously, i.e., a logger would
always have to be chosen, falling back to our own logger
implementation in case of failure with setting up journald submission.

This lead to complexity like attempting to log errors from parsing
arguments, as there is a circular dependency.

Change this to make logging initialization fallible. If it fails, exit
non-zero instead of falling back to another logger. This makes the
initialization code slightly simpler.

At the same time, make choosing the logger possible via the command
line and deprecate `--log`.

Note that we still default to journald if detected.
---
 crates/radicle-node/src/main.rs       | 172 ++++++++++++++++----------
 crates/radicle-systemd/src/journal.rs |  14 +--
 2 files changed, 111 insertions(+), 75 deletions(-)

diff --git a/crates/radicle-node/src/main.rs b/crates/radicle-node/src/main.rs
index a9f71392..3cb8fcac 100644
--- a/crates/radicle-node/src/main.rs
+++ b/crates/radicle-node/src/main.rs
@@ -2,6 +2,7 @@ use std::io;
 use std::net::SocketAddr;
 use std::path::PathBuf;
 use std::process::exit;
+use std::str::FromStr;
 
 use crossbeam_channel as chan;
 use thiserror::Error;
@@ -14,17 +15,7 @@ use radicle_node::{Runtime, VERSION};
 #[cfg(unix)]
 use radicle_signals as signals;
 
-/// The log level to use before reading any other value
-/// from configuration.
-///
-/// Note that this is different from the default value
-/// of the command line argument `--log`, as it is valid
-/// *even before that argument is parsed*.
-/// It ensures that we log the errors parsing the
-/// command line arguments, such as `--log`.
-const LOG_LEVEL_DEFAULT: &log::Level = &log::Level::Warn;
-
-pub const HELP_MSG: &str = r#"
+const HELP_MSG: &str = r#"
 Usage
 
    radicle-node [<option>...]
@@ -34,18 +25,59 @@ Usage
 
 Options
 
-    --config             <path>         Config file to use (default ~/.radicle/config.json)
-    --force                             Force start even if an existing control socket is found
-    --listen             <address>      Address to listen on
-    --log                <level>        Set log level (default: info)
-    --version                           Print program version
-    --help                              Print help
+    --config      <path>                            Config file to use
+                  (default: ~/.radicle/config.json)
+    --force                                         Force start even if an existing control socket
+                                                      is found
+    --listen      <address>                         Address to listen on
+    --log-level   <level>                           Set log level
+                  (default: info)
+    --log-logger  (radicle | systemd)               Set logger implementation
+                  (default: radicle)
+    --version                                       Print program version
+    --help                                          Print help
 "#;
 
+#[derive(Debug, Clone)]
+enum Logger {
+    Radicle,
+    #[cfg(all(feature = "systemd", target_os = "linux"))]
+    Systemd,
+}
+
+impl Default for Logger {
+    fn default() -> Self {
+        #[cfg(all(feature = "systemd", target_os = "linux"))]
+        if radicle_systemd::journal::connected() {
+            return Logger::Systemd;
+        }
+
+        Logger::Radicle
+    }
+}
+
+impl FromStr for Logger {
+    type Err = &'static str;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "radicle" => Ok(Logger::Radicle),
+            #[cfg(all(feature = "systemd", target_os = "linux"))]
+            "systemd" => Ok(Logger::Systemd),
+            _ => Err("unknown logger"),
+        }
+    }
+}
+
+struct LogOptions {
+    level: Option<log::Level>,
+    logger: Logger,
+}
+
 struct Options {
     config: Option<PathBuf>,
     listen: Vec<SocketAddr>,
-    log: Option<log::Level>,
+    log: LogOptions,
     force: bool,
 }
 
@@ -57,7 +89,8 @@ fn parse_options() -> Result<Options, lexopt::Error> {
     let mut listen = Vec::new();
     let mut config = None;
     let mut force = false;
-    let mut log = None;
+    let mut log_level = None;
+    let mut log_logger = Logger::default();
 
     while let Some(arg) = parser.next()? {
         match arg {
@@ -71,8 +104,21 @@ fn parse_options() -> Result<Options, lexopt::Error> {
                 let addr = parser.value()?.parse_with(SocketAddr::from_str)?;
                 listen.push(addr);
             }
-            Long("log") => {
-                log = Some(parser.value()?.parse_with(log::Level::from_str)?);
+            Long("log") | Long("log-level") => {
+                if matches!(arg, Long("log")) {
+                    eprintln!("Warning: The option `--log` is deprecated and will be removed. Please use `--log-level` instead.");
+                }
+                log_level = Some(parser.value()?.parse_with(log::Level::from_str)?);
+            }
+            Long("log-logger") => {
+                let parsed = parser.value()?.parse_with(Logger::from_str)?;
+                if matches!(parsed, Logger::Radicle) {
+                    return Err(lexopt::Error::Custom(
+                        "explicitly choosing this logger is forbidden, because it is deprecated"
+                            .into(),
+                    ));
+                }
+                log_logger = parsed;
             }
             Long("help") | Short('h') => {
                 println!("{HELP_MSG}");
@@ -91,8 +137,11 @@ fn parse_options() -> Result<Options, lexopt::Error> {
     Ok(Options {
         force,
         listen,
-        log,
         config,
+        log: LogOptions {
+            level: log_level,
+            logger: log_logger,
+        },
     })
 }
 
@@ -115,14 +164,14 @@ fn execute(options: Options) -> Result<(), ExecutionError> {
     // The first thing we do after reading command line options is
     // to set the log level, as this influences logging during
     // configuration loading.
-    if let Some(level) = options.log {
+    if let Some(level) = options.log.level {
         log::set_max_level(level.to_level_filter());
     }
 
     let config = options.config.unwrap_or_else(|| home.config());
     let mut config = profile::Config::load(&config)?;
 
-    if options.log.is_none() {
+    if options.log.level.is_none() {
         log::set_max_level(log::Level::from(config.node.log).to_level_filter());
     } else {
         // It might seem counter-intuitive at first, as there
@@ -178,51 +227,38 @@ fn execute(options: Options) -> Result<(), ExecutionError> {
     Ok(())
 }
 
-fn initialize_logging() {
-    let level = *LOG_LEVEL_DEFAULT;
+fn initialize_logging(options: &LogOptions) -> Result<(), Box<dyn std::error::Error>> {
+    let level = options.level.unwrap_or(log::Level::Info);
 
-    //  - We are compiling conditionally, so cannot depend
-    //    on the concrete type of the logger(s).
-    //  - We are dealing with `Option`, so we need `Box: Sized`.
-    //  - We want to provide a `Box` to `log::set_boxed_logger`.
-    //  - We also want to keep around any errors along the way.
-    type Logger = Box<dyn log::Log>;
-    type Error = Box<dyn std::error::Error>;
+    let logger: Box<dyn log::Log> = {
+        match options.logger {
+            #[cfg(all(feature = "systemd", target_os = "linux"))]
+            Logger::Systemd => {
+                use radicle_systemd::journal::*;
+                use thiserror::Error;
 
-    let journal: Result<Option<Logger>, Error> = {
-        #[cfg(all(feature = "systemd", target_os = "linux"))]
-        {
-            use thiserror::Error;
+                #[derive(Error, Debug)]
+                enum JournalError {
+                    #[error("journald not connected")]
+                    NotConnected,
+                    #[error("journald i/o: {0}")]
+                    Io(#[from] io::Error),
+                }
 
-            #[derive(Error, Debug)]
-            #[error("Error connecting to systemd journal: {0}")]
-            struct JournalError(io::Error);
+                if !connected() {
+                    return Err(Box::new(JournalError::NotConnected));
+                }
 
-            radicle_systemd::journal::logger::<&str, &str, _>("radicle-node".to_string(), [])
-                .map_err(|err| Box::new(JournalError(err)) as Error)
+                logger::<&str, &str, _>("radicle-node".to_string(), []).map_err(Box::new)?
+            }
+            Logger::Radicle => Box::new(radicle::logger::Logger::new(level)),
         }
-        #[cfg(not(all(feature = "systemd", target_os = "linux")))]
-        {
-            // This is constant, and `rustc` will hopefully use it to
-            // optimize away the `match` below.
-            Ok(None)
-        }
-    };
-
-    let (logger, err) = match journal {
-        Ok(Some(logger)) => (logger, None),
-        otherwise => (
-            Box::new(radicle::logger::Logger::new(level)) as Logger,
-            otherwise.err(),
-        ),
     };
 
     log::set_boxed_logger(logger).expect("no other logger should have been set already");
     log::set_max_level(level.to_level_filter());
 
-    if let Some(err) = err {
-        log::warn!(target: "node", "Error initializing logger (fell back to default): {err}");
-    }
+    Ok(())
 }
 
 fn main() {
@@ -234,16 +270,16 @@ fn main() {
         std::env::set_var(RUST_BACKTRACE, "1");
     }
 
-    initialize_logging();
+    let options = parse_options().unwrap_or_else(|err| {
+        // The lexopt errors read nicely with a comma.
+        eprintln!("Failed to parse options, {err:#}");
+        exit(2);
+    });
 
-    let options = match parse_options() {
-        Ok(options) => options,
-        Err(err) => {
-            // The lexopt errors read nicely with a comma.
-            log::error!(target: "node", "Failed to parse options, {err:#}");
-            exit(2);
-        }
-    };
+    initialize_logging(&options.log).unwrap_or_else(|err| {
+        eprintln!("Failed to initialize logging: {err:#}");
+        exit(3);
+    });
 
     if let Err(err) = execute(options) {
         log::error!(target: "node", "{err:#}");
diff --git a/crates/radicle-systemd/src/journal.rs b/crates/radicle-systemd/src/journal.rs
index 46d841d4..ffea4105 100644
--- a/crates/radicle-systemd/src/journal.rs
+++ b/crates/radicle-systemd/src/journal.rs
@@ -5,19 +5,19 @@ use systemd_journal_logger::{connected_to_journal, current_exe_identifier, Journ
 pub fn logger<K, V, I>(
     default_identifier: String,
     extra_fields: I,
-) -> std::io::Result<Option<Box<dyn log::Log>>>
+) -> std::io::Result<Box<dyn log::Log>>
 where
     I: IntoIterator<Item = (K, V)>,
     K: AsRef<str>,
     V: AsRef<[u8]>,
 {
-    if !connected_to_journal() {
-        return Ok(None);
-    }
-
-    Ok(Some(Box::new(
+    Ok(Box::new(
         JournalLog::new()?
             .with_syslog_identifier(current_exe_identifier().unwrap_or(default_identifier))
             .with_extra_fields(extra_fields),
-    )))
+    ))
+}
+
+pub fn connected() -> bool {
+    connected_to_journal()
 }