cli/node: Improve log rotation

Only the latest log was kept as `node.log.old`. Now, log files are
numbered (`node.log.1`, `node.log.2` and so on). Also `node.log` is now
a hard link to the current log file. `rad node stop` will delete
`node.log` (note that `node.log.x` stays intact) and `rad node start`
will delete `node.log` before it creates `node.log.y` in case the node
crashed.

Also, when running in foreground mode, now a log file is created. It
just contains the hint that the node was started in foreground mode,
just to avoid confusion.

The implementation is split into a sans I/O part as `struct LogRotator`,
while the I/O counterpart is captured by `struct LogRotatorFileSystem`.

crates/radicle-cli/src/commands/node.rs

@@ -340,7 +340,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
             control::status(&node, &profile)?;
         }
         Operation::Stop => {
-            control::stop(node)?;
+            control::stop(node, &profile);
         }
     }
 

crates/radicle-cli/src/commands/node/control.rs

@@ -1,8 +1,11 @@
+mod logs;
+use logs::{LogRotatorFileSystem, Rotated};
+
 use std::collections::HashMap;
 use std::ffi::OsString;
-use std::fs::{File, OpenOptions};
-use std::io::{BufRead, BufReader, Read, Seek, SeekFrom};
-use std::{fs, io, path::Path, process, thread, time};
+use std::fs::File;
+use std::io::{BufRead, BufReader, Read, Seek, SeekFrom, Write};
+use std::{path::Path, process, thread, time};
 
 use anyhow::{anyhow, Context};
 use localtime::LocalTime;
@@ -17,10 +20,6 @@ use crate::terminal::Element as _;
 
 /// How long to wait for the node to start before returning an error.
 pub const NODE_START_TIMEOUT: time::Duration = time::Duration::from_secs(6);
-/// Node log file name.
-pub const NODE_LOG: &str = "node.log";
-/// Node log old file name, after rotation.
-pub const NODE_LOG_OLD: &str = "node.log.old";
 
 pub fn start(
     node: Node,
@@ -55,14 +54,19 @@ pub fn start(
     if !options.contains(&OsString::from("--force")) {
         options.push(OsString::from("--force"));
     }
+
+    let Rotated {
+        path: log_path,
+        log: log_file,
+    } = LogRotatorFileSystem::from_profile(profile).rotate()?;
+
     if daemon {
-        let log = log_rotate(profile)?;
         let child = process::Command::new(cmd)
             .args(options)
             .envs(envs)
             .stdin(process::Stdio::null())
-            .stdout(process::Stdio::from(log.try_clone()?))
-            .stderr(process::Stdio::from(log))
+            .stdout(process::Stdio::from(log_file.try_clone()?))
+            .stderr(process::Stdio::from(log_file))
             .spawn()
             .map_err(|e| anyhow!("failed to start node process {cmd:?}: {e}"))?;
         let pid = term::format::parens(term::format::dim(child.id()));
@@ -98,6 +102,10 @@ pub fn start(
             }
         }
     } else {
+        // Write a hint to the log file, but swallow any errors.
+        let mut log_file = log_file;
+        let _ = log_file.write_all(format!("radicle-node started in foreground, no futher log messages are written to '{}' (this file).\n", log_path.display()).as_bytes());
+
         let mut child = process::Command::new(cmd)
             .args(options)
             .envs(envs)
@@ -110,7 +118,7 @@ pub fn start(
     Ok(())
 }
 
-pub fn stop(node: Node) -> anyhow::Result<()> {
+pub fn stop(node: Node, profile: &Profile) {
     let mut spinner = term::spinner("Stopping node...");
     if node.shutdown().is_err() {
         spinner.error("node is not running");
@@ -118,7 +126,8 @@ pub fn stop(node: Node) -> anyhow::Result<()> {
         spinner.message("Node stopped");
         spinner.finish();
     }
-    Ok(())
+    let rotator = LogRotatorFileSystem::from_profile(profile);
+    rotator.remove().ok();
 }
 
 pub fn debug(node: &mut Node) -> anyhow::Result<()> {
@@ -394,22 +403,6 @@ pub fn config(node: &Node) -> anyhow::Result<()> {
     Ok(())
 }
 
-fn log_rotate(profile: &Profile) -> io::Result<File> {
-    let base = profile.home.node();
-    if base.join(NODE_LOG).exists() {
-        // Let this fail, eg. if the file doesn't exist.
-        fs::remove_file(base.join(NODE_LOG_OLD)).ok();
-        fs::rename(base.join(NODE_LOG), base.join(NODE_LOG_OLD))?;
-    }
-
-    let log = OpenOptions::new()
-        .write(true)
-        .create_new(true)
-        .open(base.join(NODE_LOG))?;
-
-    Ok(log)
-}
-
 fn state_label() -> term::Paint<String> {
     term::Paint::from("?".to_string())
 }

crates/radicle-cli/src/commands/node/logs.rs

@@ -0,0 +1,174 @@
+use std::collections::BinaryHeap;
+use std::fs::{File, OpenOptions};
+use std::path::PathBuf;
+use std::{fs, io};
+
+use radicle::Profile;
+
+/// [`LogRotator`] manages the rotation of the log files when the Radicle node
+/// is run in the background, without being managed by a service manager.
+pub struct LogRotator {
+    /// The base path where the logs should live.
+    base: PathBuf,
+    /// All existing logs, identified by a suffix.
+    existing_logs: BinaryHeap<u16>,
+}
+
+impl LogRotator {
+    /// Node log file name.
+    pub const NODE_LOG: &str = "node.log";
+    /// Node log old file name, after rotation.
+    pub const NODE_LOG_OLD_PREFIX: &str = "node.log.";
+
+    /// Construct a new [`LogRotator`] with the give `base` path.
+    pub fn new(base: PathBuf) -> Self {
+        Self {
+            base,
+            existing_logs: BinaryHeap::new(),
+        }
+    }
+
+    /// Add a set of existing suffixes to known logs.
+    pub fn found_logs(&mut self, suffixes: impl Iterator<Item = u16>) {
+        self.existing_logs.extend(suffixes);
+    }
+
+    /// Specify that the [`LogRotator::current`] log should be removed.
+    ///
+    /// Returns `None` if the file does not exist.
+    pub fn remove_current(&self) -> Option<Remove> {
+        let current = self.current();
+        current.exists().then_some(Remove { current })
+    }
+
+    /// Specify that the logs should be rotated.
+    pub fn rotate(&self) -> Rotate {
+        let next = self.next_log();
+        let remove = self.remove_current();
+        Rotate {
+            next,
+            link: self.current(),
+            remove,
+        }
+    }
+
+    /// The current log file that should be logged to.
+    pub fn current(&self) -> PathBuf {
+        self.base.join(Self::NODE_LOG)
+    }
+
+    fn next_log(&self) -> PathBuf {
+        let suffix = self
+            .existing_logs
+            .peek()
+            .copied()
+            .unwrap_or(0)
+            .saturating_add(1);
+        self.base
+            .join(Self::NODE_LOG_OLD_PREFIX.to_owned() + suffix.to_string().as_str())
+    }
+}
+
+/// A [`LogRotator`] that implements the removal and rotation by accessing the
+/// filesystem.
+pub struct LogRotatorFileSystem {
+    rotator: LogRotator,
+}
+
+impl LogRotatorFileSystem {
+    /// Create a new [`LogRotatorFileSystem`] from a [`Profile`].
+    ///
+    /// The [`LogRotator`]'s base path will be the node path.
+    pub fn from_profile(profile: &Profile) -> Self {
+        Self {
+            rotator: LogRotator::new(profile.home.node()),
+        }
+    }
+
+    /// Rotate the log files, returning [`Rotated`].
+    pub fn rotate(mut self) -> io::Result<Rotated> {
+        self.rotator.found_logs(self.existing_logs().into_iter());
+        self.rotator.rotate().execute()
+    }
+
+    /// Remove the current log file, returning `true` if the file existed and
+    /// was removed.
+    pub fn remove(self) -> io::Result<bool> {
+        self.rotator
+            .remove_current()
+            .map(|remove| remove.execute())
+            .transpose()
+            .map(|res| res.is_some())
+    }
+
+    fn parse_suffix(filename: String) -> Option<u16> {
+        filename
+            .strip_prefix(LogRotator::NODE_LOG_OLD_PREFIX)
+            .and_then(|suffix| suffix.parse::<u16>().ok())
+    }
+
+    fn existing_logs(&self) -> BinaryHeap<u16> {
+        self.rotator
+            .base
+            .read_dir()
+            .ok()
+            .map(|dir| {
+                dir.filter_map(Result::ok)
+                    .filter_map(|entry| entry.file_name().into_string().ok())
+                    .filter_map(Self::parse_suffix)
+                    .collect()
+            })
+            .unwrap_or_default()
+    }
+}
+
+/// Remove the path identified by [`Remove::current`].
+pub struct Remove {
+    current: PathBuf,
+}
+
+impl Remove {
+    /// Use [`fs::remove_file`] to remove the file.
+    pub fn execute(self) -> io::Result<()> {
+        fs::remove_file(self.current)
+    }
+}
+
+/// Rotate the logs to the next log.
+pub struct Rotate {
+    /// The next log that needs to be created
+    next: PathBuf,
+    /// The path to create a hard link to.
+    link: PathBuf,
+    /// If the current log exists, then we need to remove it
+    remove: Option<Remove>,
+}
+
+impl Rotate {
+    /// Remove the existing file, if it exists. Then create the next log, and
+    /// create a hard link to it.
+    pub fn execute(self) -> io::Result<Rotated> {
+        if let Some(to_remove) = self.remove {
+            if let Err(err) = to_remove.execute() {
+                log::warn!(target: "cli", "Failed to remove current log file: {err}");
+            }
+        }
+        let log = OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(&self.next)?;
+        fs::hard_link(&self.next, &self.link)?;
+        Ok(Rotated {
+            path: self.next,
+            log,
+        })
+    }
+}
+
+/// The result of rotating the logs.
+pub struct Rotated {
+    /// The [`PathBuf`] to the new log file.
+    pub path: PathBuf,
+    /// The [`File`] handle for the log file.
+    pub log: File,
+}