From 122d84cfa2b69ff1e0f8b2f5dba73209f17da4dc Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 29 Apr 2025 23:05:02 +1000 Subject: [PATCH] fix(connlib): recreate log file if it got deleted (#8926) Currently, when `connlib`'s log file gets deleted, we write logs into nirvana until the corresponding process gets restarted. This is painful for users to do because they need to restart the IPC service or Network Extension. Instead, we can simply check if the log file exists prior to writing to it and re-create it if it doesn't. Resolves: #6850 Related: #7569 --- rust/Cargo.lock | 1 + rust/headless-client/src/clear_logs.rs | 115 ++++++------------- rust/logging/Cargo.toml | 1 + rust/logging/src/file.rs | 48 ++++++++ website/src/components/Changelog/Android.tsx | 3 + website/src/components/Changelog/Apple.tsx | 3 + website/src/components/Changelog/GUI.tsx | 3 + 7 files changed, 91 insertions(+), 83 deletions(-) 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. +