From 4934473b86b3a014afca8e5317b85c0c78cc58d3 Mon Sep 17 00:00:00 2001 From: Lorenz Leutgeb Date: Thu, 17 Jul 2025 15:23:19 +0200 Subject: [PATCH] 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 | 2 +- .../radicle-cli/src/commands/node/control.rs | 49 +++-- crates/radicle-cli/src/commands/node/logs.rs | 174 ++++++++++++++++++ 3 files changed, 196 insertions(+), 29 deletions(-) create mode 100644 crates/radicle-cli/src/commands/node/logs.rs diff --git a/crates/radicle-cli/src/commands/node.rs b/crates/radicle-cli/src/commands/node.rs index db1be3fe..1b9a7da4 100644 --- a/crates/radicle-cli/src/commands/node.rs +++ b/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); } } diff --git a/crates/radicle-cli/src/commands/node/control.rs b/crates/radicle-cli/src/commands/node/control.rs index 49f46c07..bf36367d 100644 --- a/crates/radicle-cli/src/commands/node/control.rs +++ b/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 { - 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 { term::Paint::from("?".to_string()) } diff --git a/crates/radicle-cli/src/commands/node/logs.rs b/crates/radicle-cli/src/commands/node/logs.rs new file mode 100644 index 00000000..e37cd4e8 --- /dev/null +++ b/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, +} + +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) { + 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 { + 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 { + 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 { + self.rotator + .remove_current() + .map(|remove| remove.execute()) + .transpose() + .map(|res| res.is_some()) + } + + fn parse_suffix(filename: String) -> Option { + filename + .strip_prefix(LogRotator::NODE_LOG_OLD_PREFIX) + .and_then(|suffix| suffix.parse::().ok()) + } + + fn existing_logs(&self) -> BinaryHeap { + 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, +} + +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 { + 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, +}