diff --git a/rust/Cargo.lock b/rust/Cargo.lock index a15433de3..2817c2d79 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -2391,6 +2391,7 @@ dependencies = [ "rand 0.8.5", "sentry-tracing", "supports-color", + "tempfile", "thiserror 1.0.69", "time", "tracing", diff --git a/rust/headless-client/src/clear_logs.rs b/rust/headless-client/src/clear_logs.rs index 5760d657f..9453fdea1 100644 --- a/rust/headless-client/src/clear_logs.rs +++ b/rust/headless-client/src/clear_logs.rs @@ -1,11 +1,7 @@ use anyhow::{Context, Result}; -use std::{ - ffi::OsStr, - io::ErrorKind::NotFound, - path::{Path, PathBuf}, -}; +use std::{io::ErrorKind::NotFound, path::Path}; -/// Deletes all `.log` and `.jsonl` files in `path` except the most recent +/// Deletes all `.log` files in `path`. pub async fn clear_logs(path: &Path) -> Result<()> { let mut dir = match tokio::fs::read_dir(path).await { Ok(x) => x, @@ -18,96 +14,49 @@ pub async fn clear_logs(path: &Path) -> Result<()> { return Err(error.into()); } }; - let mut paths = vec![]; - while let Some(entry) = dir.next_entry().await? { - paths.push(entry.path()); - } + + let mut result = Ok(()); // If we can't delete some files due to permission errors, just keep going // and delete as much as we can, then return the most recent error - let mut result = Ok(()); - let to_delete = choose_logs_to_delete(&paths); - for path in &to_delete { - if let Err(e) = tokio::fs::remove_file(path).await { + while let Some(entry) = dir + .next_entry() + .await + .context("Failed to read next dir entry")? + { + if entry + .file_name() + .to_str() + .is_none_or(|name| !name.ends_with("log")) + { + continue; + } + + if let Err(e) = tokio::fs::remove_file(entry.path()).await { result = Err(e); } } + result.context("Failed to delete at least one file") } -fn choose_logs_to_delete(paths: &[PathBuf]) -> Vec<&Path> { - let mut most_recent_stem = None; - for path in paths { - if path.extension() != Some(OsStr::new("log")) { - continue; - } - let Some(stem) = path.file_stem() else { - continue; - }; - match most_recent_stem { - None => most_recent_stem = Some(stem), - Some(most_recent) if stem > most_recent => most_recent_stem = Some(stem), - Some(_) => {} - } - } - let Some(most_recent_stem) = most_recent_stem else { - tracing::warn!( - "Nothing to delete, should be impossible since both processes always write logs" - ); - return vec![]; - }; - let Some(most_recent_stem) = most_recent_stem.to_str() else { - tracing::warn!("Most recent log file does not have a UTF-8 path"); - return vec![]; - }; - - paths - .iter() - .filter_map(|path| { - // Don't delete files if we can't parse their stems as UTF-8. - let stem = path.file_stem()?.to_str()?; - - (stem < most_recent_stem).then_some(path.as_path()) - }) - .collect() -} - #[cfg(test)] mod tests { - use std::path::Path; + use super::*; + + #[tokio::test] + async fn only_deletes_log_files() { + let dir = tempfile::tempdir().unwrap(); + + std::fs::write(dir.path().join("first.log"), "log file 1").unwrap(); + std::fs::write(dir.path().join("second.log"), "log file 1").unwrap(); + std::fs::write(dir.path().join("not_a_logfile.tmp"), "something important").unwrap(); + + clear_logs(dir.path()).await.unwrap(); - #[test] - fn clear_logs_logic() { - // These are out of order just to make sure it works anyway - let paths: Vec<_> = [ - "connlib.2024-08-05-19-41-46.jsonl", - "connlib.2024-08-05-19-41-46.log", - "connlib.2024-08-07-14-17-56.jsonl", - "connlib.2024-08-07-14-17-56.log", - "connlib.2024-08-06-14-21-13.jsonl", - "connlib.2024-08-06-14-21-13.log", - "connlib.2024-08-06-14-51-19.jsonl", - "connlib.2024-08-06-14-51-19.log", - "crash.2024-07-22-21-16-20.dmp", - "last_crash.dmp", - ] - .into_iter() - .map(|x| Path::new("/bogus").join(x)) - .collect(); - let to_delete = super::choose_logs_to_delete(&paths); assert_eq!( - to_delete, - [ - "/bogus/connlib.2024-08-05-19-41-46.jsonl", - "/bogus/connlib.2024-08-05-19-41-46.log", - "/bogus/connlib.2024-08-06-14-21-13.jsonl", - "/bogus/connlib.2024-08-06-14-21-13.log", - "/bogus/connlib.2024-08-06-14-51-19.jsonl", - "/bogus/connlib.2024-08-06-14-51-19.log", - ] - .into_iter() - .map(Path::new) - .collect::>() + std::fs::read_to_string(dir.path().join("not_a_logfile.tmp")).unwrap(), + "something important" ); } } diff --git a/rust/logging/Cargo.toml b/rust/logging/Cargo.toml index 68c0913ff..c897f5c41 100644 --- a/rust/logging/Cargo.toml +++ b/rust/logging/Cargo.toml @@ -21,6 +21,7 @@ tracing-log = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter"] } [dev-dependencies] +tempfile = { workspace = true } thiserror = { workspace = true } [lints] diff --git a/rust/logging/src/file.rs b/rust/logging/src/file.rs index 5c1aa8ae1..196dc11c2 100644 --- a/rust/logging/src/file.rs +++ b/rust/logging/src/file.rs @@ -119,6 +119,17 @@ impl Appender { ret } + Some((_, filename)) + if !std::fs::exists(self.directory.join(&filename)).unwrap_or_default() => + { + let (mut file, name) = self.create_new_writer()?; + + let ret = cb(&mut file); + + self.current = Some((file, name)); + + ret + } Some((file, _)) => cb(file), } } @@ -197,3 +208,40 @@ impl io::Write for Appender { self.with_current_file(|f| f.flush()) } } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt}; + + use super::*; + + #[test] + fn deleting_log_file_creates_new_one() { + let dir = tempfile::tempdir().unwrap(); + + let (layer, _handle) = layer(dir.path(), "connlib"); + + let _guard = tracing_subscriber::registry() + .with(layer) + .with(tracing_subscriber::EnvFilter::from("info")) + .set_default(); + + tracing::info!("This is a test"); + std::thread::sleep(Duration::from_millis(1000)); // Wait a bit until background thread has flushed the log. + + for dir in std::fs::read_dir(dir.path()).unwrap() { + let dir = dir.unwrap(); + + std::fs::remove_file(dir.path()).unwrap(); + } + + tracing::info!("Write after delete"); + std::thread::sleep(Duration::from_millis(1000)); // Wait a bit until background thread has flushed the log. + + let content = std::fs::read_to_string(dir.path().join("latest")).unwrap(); + + assert!(content.contains("Write after delete")) + } +} diff --git a/website/src/components/Changelog/Android.tsx b/website/src/components/Changelog/Android.tsx index ad16f4a78..65227864e 100644 --- a/website/src/components/Changelog/Android.tsx +++ b/website/src/components/Changelog/Android.tsx @@ -24,6 +24,9 @@ export default function Android() { Improves connection reliability by maintaining the order of IP packets across GSO batches. + + Rolls over to a new log-file as soon as logs are cleared. + diff --git a/website/src/components/Changelog/Apple.tsx b/website/src/components/Changelog/Apple.tsx index 8656a1a71..0fee8b42a 100644 --- a/website/src/components/Changelog/Apple.tsx +++ b/website/src/components/Changelog/Apple.tsx @@ -34,6 +34,9 @@ export default function Apple() { Improves connection reliability by maintaining the order of IP packets across GSO batches. + + Rolls over to a new log-file as soon as logs are cleared. + diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index 71e6c8c61..dc1337284 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -34,6 +34,9 @@ export default function GUI({ os }: { os: OS }) { Improves connection reliability by maintaining the order of IP packets. )} + + Rolls over to a new log-file as soon as logs are cleared. +