From a313dfea7b814d246284598083fc8dc85182b301 Mon Sep 17 00:00:00 2001 From: Reginald Lips Date: Tue, 23 Sep 2025 15:51:15 -0700 Subject: [PATCH 1/3] Add configuration option to set the number of event logs retained on disk --- app/buck2_client_ctx/src/streaming.rs | 3 +++ app/buck2_client_ctx/src/subscribers/event_log.rs | 2 ++ app/buck2_common/src/init.rs | 11 +++++++++++ app/buck2_event_log/src/file_names.rs | 6 ++---- app/buck2_event_log/src/write.rs | 6 +++++- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/app/buck2_client_ctx/src/streaming.rs b/app/buck2_client_ctx/src/streaming.rs index 37c309bb4a9d..f919811e6fc1 100644 --- a/app/buck2_client_ctx/src/streaming.rs +++ b/app/buck2_client_ctx/src/streaming.rs @@ -293,6 +293,9 @@ fn get_event_log_subscriber( sanitized_argv, T::COMMAND_NAME.to_owned(), log_size_counter_bytes, + ctx.immediate_config.daemon_startup_config().map(|daemon_startup_config| { + daemon_startup_config.retained_event_logs + }).unwrap(), ); Box::new(log) } diff --git a/app/buck2_client_ctx/src/subscribers/event_log.rs b/app/buck2_client_ctx/src/subscribers/event_log.rs index 40d3cc1fe602..cf128e8bf348 100644 --- a/app/buck2_client_ctx/src/subscribers/event_log.rs +++ b/app/buck2_client_ctx/src/subscribers/event_log.rs @@ -37,6 +37,7 @@ impl EventLog { sanitized_argv: SanitizedArgv, command_name: String, log_size_counter_bytes: Option>, + retained_event_logs: usize, ) -> EventLog { Self { writer: WriteEventLog::new( @@ -47,6 +48,7 @@ impl EventLog { sanitized_argv, command_name, log_size_counter_bytes, + retained_event_logs, ), } } diff --git a/app/buck2_common/src/init.rs b/app/buck2_common/src/init.rs index 64b70f1e31fa..2d2886a64d39 100644 --- a/app/buck2_common/src/init.rs +++ b/app/buck2_common/src/init.rs @@ -22,6 +22,8 @@ use serde::Serialize; use crate::legacy_configs::configs::LegacyBuckConfig; use crate::legacy_configs::key::BuckconfigKeyRef; +const DEFAULT_RETAINED_EVENT_LOGS: usize = 12; + /// Helper enum to categorize the kind of timeout we get from the startup config. #[derive(Clone, Debug)] pub enum Timeout { @@ -445,6 +447,7 @@ pub struct DaemonStartupConfig { pub resource_control: ResourceControlConfig, pub log_download_method: LogDownloadMethod, pub health_check_config: HealthCheckConfig, + pub retained_event_logs: usize, } impl DaemonStartupConfig { @@ -520,6 +523,13 @@ impl DaemonStartupConfig { resource_control: ResourceControlConfig::from_config(config)?, log_download_method, health_check_config: HealthCheckConfig::from_config(config)?, + retained_event_logs: config + .get(BuckconfigKeyRef { + section: "buck2", + property: "retained_event_logs", + }) + .and_then(|s| s.parse::().ok()) + .unwrap_or(DEFAULT_RETAINED_EVENT_LOGS), }) } @@ -547,6 +557,7 @@ impl DaemonStartupConfig { LogDownloadMethod::None }, health_check_config: HealthCheckConfig::default(), + retained_event_logs: DEFAULT_RETAINED_EVENT_LOGS, } } } diff --git a/app/buck2_event_log/src/file_names.rs b/app/buck2_event_log/src/file_names.rs index 364a391b312b..e1f5f199bebf 100644 --- a/app/buck2_event_log/src/file_names.rs +++ b/app/buck2_event_log/src/file_names.rs @@ -44,11 +44,9 @@ pub(crate) fn get_logfile_name( )) } -pub(crate) async fn remove_old_logs(logdir: &AbsNormPath) { - const N_LOGS_RETAINED: usize = 12; - +pub(crate) async fn remove_old_logs(logdir: &AbsNormPath, retained_event_logs: usize) { if let Ok(logfiles) = get_files_in_log_dir(logdir) { - futures::stream::iter(logfiles.into_iter().rev().skip(N_LOGS_RETAINED - 1)) + futures::stream::iter(logfiles.into_iter().rev().skip(retained_event_logs)) .then(|file| async move { // The oldest logs might be open from another concurrent build, so suppress error. tokio::fs::remove_file(file).await.ok() diff --git a/app/buck2_event_log/src/write.rs b/app/buck2_event_log/src/write.rs index 4de4eb820119..ea3df6838467 100644 --- a/app/buck2_event_log/src/write.rs +++ b/app/buck2_event_log/src/write.rs @@ -61,6 +61,7 @@ pub struct WriteEventLog { /// Allocation cache. Must be cleaned before use. buf: Vec, log_size_counter_bytes: Option>, + retained_event_logs: usize, } impl WriteEventLog { @@ -72,6 +73,7 @@ impl WriteEventLog { sanitized_argv: SanitizedArgv, command_name: String, log_size_counter_bytes: Option>, + retained_event_logs: usize, ) -> Self { Self { state: LogWriterState::Unopened { @@ -84,6 +86,7 @@ impl WriteEventLog { working_dir, buf: Vec::new(), log_size_counter_bytes, + retained_event_logs, } } @@ -160,7 +163,7 @@ impl WriteEventLog { .with_buck_error_context(|| { format!("Error creating event log directory: `{logdir}`") })?; - remove_old_logs(logdir).await; + remove_old_logs(logdir, self.retained_event_logs).await; let encoding = Encoding::PROTO_ZSTD; let file_name = &get_logfile_name(event, encoding, &self.command_name)?; @@ -477,6 +480,7 @@ mod tests { working_dir: AbsWorkingDir::current_dir()?, buf: Vec::new(), log_size_counter_bytes: None, + retained_event_logs: 5, }) } } From 96f0ae58fc9aa9ee2d017bd795ac3d3ac0c9baa4 Mon Sep 17 00:00:00 2001 From: Reginald Lips Date: Tue, 23 Sep 2025 15:59:54 -0700 Subject: [PATCH 2/3] Formatting --- app/buck2_client_ctx/src/streaming.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/buck2_client_ctx/src/streaming.rs b/app/buck2_client_ctx/src/streaming.rs index f919811e6fc1..93ef81072971 100644 --- a/app/buck2_client_ctx/src/streaming.rs +++ b/app/buck2_client_ctx/src/streaming.rs @@ -293,9 +293,10 @@ fn get_event_log_subscriber( sanitized_argv, T::COMMAND_NAME.to_owned(), log_size_counter_bytes, - ctx.immediate_config.daemon_startup_config().map(|daemon_startup_config| { - daemon_startup_config.retained_event_logs - }).unwrap(), + ctx.immediate_config + .daemon_startup_config() + .map(|daemon_startup_config| daemon_startup_config.retained_event_logs) + .unwrap(), ); Box::new(log) } From d5aa9bac7a7852e7e5fa04825ceb68ef176b305b Mon Sep 17 00:00:00 2001 From: Reginald Lips Date: Fri, 31 Oct 2025 15:29:17 -0700 Subject: [PATCH 3/3] - Fix `remove_old_logs` to only count files - Add a unit test for `remove_old_logs` --- app/buck2_event_log/src/file_names.rs | 73 ++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/app/buck2_event_log/src/file_names.rs b/app/buck2_event_log/src/file_names.rs index e1f5f199bebf..e0b11544ea4c 100644 --- a/app/buck2_event_log/src/file_names.rs +++ b/app/buck2_event_log/src/file_names.rs @@ -46,7 +46,7 @@ pub(crate) fn get_logfile_name( pub(crate) async fn remove_old_logs(logdir: &AbsNormPath, retained_event_logs: usize) { if let Ok(logfiles) = get_files_in_log_dir(logdir) { - futures::stream::iter(logfiles.into_iter().rev().skip(retained_event_logs)) + futures::stream::iter(logfiles.into_iter().rev().skip(retained_event_logs - 1)) .then(|file| async move { // The oldest logs might be open from another concurrent build, so suppress error. tokio::fs::remove_file(file).await.ok() @@ -72,7 +72,10 @@ pub fn get_local_logs(logdir: &AbsNormPath) -> buck2_error::Result Vec { - let mut logfiles = dir.filter_map(Result::ok).collect::>(); + let mut logfiles = dir + .filter_map(Result::ok) + .filter(|entry| entry.file_type().ok().map_or(false, |ft| ft.is_file())) + .collect::>(); logfiles.sort_by_cached_key(|file| { // Return Unix epoch if unable to get creation time. if let Ok(metadata) = file.metadata() { @@ -126,3 +129,69 @@ pub fn retrieve_all_logs(paths: &InvocationPaths) -> buck2_error::Result buck2_error::Result<()> { + let logdir = tempfile::tempdir()?; + let logdir_path = AbsPath::new(logdir.path())?; + let logdir_norm = AbsNormPath::new(logdir_path)?; + + // Create 5 subfolders in logdir, each with a file inside + let mut subdirs = Vec::new(); + for i in 0..5 { + let subdir_path = logdir.path().join(format!("subdir{}", i)); + let subdir = AbsPath::new(&subdir_path)?; + fs_util::create_dir_all(&subdir)?; + let inside_file = subdir.join("inside.txt"); + fs_util::write(&inside_file, format!("content in subdir{}", i))?; + subdirs.push((subdir.to_owned(), inside_file)); + } + + let base_time = SystemTime::now().checked_sub(Duration::from_secs(100)).unwrap(); + + // Create 5 log files directly in logdir with incrementing modification times + let mut log_paths = Vec::new(); + for i in 0..5 { + let log_path = logdir_path.join(format!("buck-log-{}.zst", i)); + fs_util::write(&log_path, format!("log content {}", i))?; + + let file = File::open(&log_path)?; + let mod_time = base_time + Duration::from_secs((i as u64) * 10); + let times = std::fs::FileTimes::new().set_modified(mod_time); + file.set_times(times)?; + + log_paths.push(log_path.clone()); + } + + // Call the function to keep 3 logs (should delete 3 oldest, leave 2 newest) + remove_old_logs(&logdir_norm, 3).await; + + // Check that the 3 oldest logs are removed (indices 0,1,2 - earliest created) + for path in &log_paths[0..3] { + assert!(!path.exists(), "{} should be removed", path.display()); + } + + // Check that the 2 newest logs remain (indices 3,4 - latest created) + for path in &log_paths[3..5] { + assert!(path.exists(), "{} should remain", path.display()); + } + + // Check that all subfolders remain + for (subdir, inside_file) in subdirs { + assert!(subdir.exists(), "{} should remain", subdir.display()); + // Ensure the file inside subdirectory is still there + assert!(inside_file.exists(), "{} should remain", inside_file.display()); + } + + Ok(()) + } +}