From df99cd1062be9d9e1c09a56317df5abbaa8e92c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:32:54 +0000 Subject: [PATCH] fix(daemon): honor max_rotated_files == 0 in rotate_log_files `rotate_log_files` deletes the oldest rotated file (`log_..jsonl`), shifts `.N -> .N+1` for `1..max`, then renames the current file to `.1`. When `max_files == 0` the shift range `(1..0)` is empty and the delete-oldest step targets `log_.0.jsonl` (never created), yet the current file is still renamed to `log_.1.jsonl`. That rotated file is then never pruned, so a node configured with `max_rotated_files: 0` keeps a rotated log forever instead of retaining none. `max_rotated_files` is user-configurable via the dataflow descriptor and reaches this function directly (`spawn/prepared.rs`), so `0` is a reachable input to this public library function. Fix by special-casing `max_files == 0`: discard the current log and prune any rotated files a previous (non-zero) configuration may have left behind, so the "retain no rotated copies" contract holds regardless of history. Adds regression tests for a single/repeated rotation and for pruning pre-existing rotated files. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SD4dBVimzepwKSh2b9F8aG --- binaries/daemon/src/log.rs | 72 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/binaries/daemon/src/log.rs b/binaries/daemon/src/log.rs index f63eeb0220..dda612b232 100644 --- a/binaries/daemon/src/log.rs +++ b/binaries/daemon/src/log.rs @@ -46,6 +46,30 @@ pub fn rotate_log_files( node_id: &NodeId, max_files: u32, ) -> std::io::Result<()> { + // With no rotated copies requested, discard the current log instead of + // renaming it to `.1`. Otherwise the shift-then-rename logic below would + // move `current` to `log_.1.jsonl` while the delete-oldest step + // targets the never-created `log_.0.jsonl`, leaving one rotated file + // behind forever — contradicting `max_files == 0`. Also prune any rotated + // files a previous (non-zero) configuration may have left behind, so the + // "retain no rotated copies" contract holds regardless of history. + if max_files == 0 { + let current = log_path(working_dir, dataflow_id, node_id); + if current.exists() { + std::fs::remove_file(¤t)?; + } + let mut index = 1u32; + loop { + let rotated = log_path_rotated(working_dir, dataflow_id, node_id, index); + if !rotated.exists() { + break; + } + std::fs::remove_file(&rotated)?; + index += 1; + } + return Ok(()); + } + // Delete the oldest if it exists let oldest = log_path_rotated(working_dir, dataflow_id, node_id, max_files); if oldest.exists() { @@ -689,6 +713,54 @@ mod tests { ); } + #[test] + fn rotate_with_zero_max_files_discards_current() { + let tmp = tempfile::tempdir().unwrap(); + let uuid = Uuid::nil(); + let node = NodeId::from("n".to_string()); + + let dataflow_dir = tmp.path().join("out").join(uuid.to_string()); + std::fs::create_dir_all(&dataflow_dir).unwrap(); + let current = log_path(tmp.path(), &uuid, &node); + std::fs::write(¤t, "current\n").unwrap(); + + rotate_log_files(tmp.path(), &uuid, &node, 0).unwrap(); + + // With max_files == 0 no rotated copy is retained: the current file is + // discarded and no `.1` file is left behind. + assert!(!current.exists()); + assert!(!log_path_rotated(tmp.path(), &uuid, &node, 1).exists()); + + // A second rotation is still a no-op and does not accumulate files. + std::fs::write(¤t, "again\n").unwrap(); + rotate_log_files(tmp.path(), &uuid, &node, 0).unwrap(); + assert!(!current.exists()); + assert!(!log_path_rotated(tmp.path(), &uuid, &node, 1).exists()); + } + + #[test] + fn rotate_with_zero_max_files_prunes_preexisting_rotated_files() { + let tmp = tempfile::tempdir().unwrap(); + let uuid = Uuid::nil(); + let node = NodeId::from("n".to_string()); + + let dataflow_dir = tmp.path().join("out").join(uuid.to_string()); + std::fs::create_dir_all(&dataflow_dir).unwrap(); + + // Simulate rotated files left behind by an earlier non-zero config. + let current = log_path(tmp.path(), &uuid, &node); + std::fs::write(¤t, "current\n").unwrap(); + std::fs::write(log_path_rotated(tmp.path(), &uuid, &node, 1), "old1\n").unwrap(); + std::fs::write(log_path_rotated(tmp.path(), &uuid, &node, 2), "old2\n").unwrap(); + + rotate_log_files(tmp.path(), &uuid, &node, 0).unwrap(); + + // "retain no rotated copies" must hold even against pre-existing files. + assert!(!current.exists()); + assert!(!log_path_rotated(tmp.path(), &uuid, &node, 1).exists()); + assert!(!log_path_rotated(tmp.path(), &uuid, &node, 2).exists()); + } + #[test] fn rotate_noop_when_no_files() { let tmp = tempfile::tempdir().unwrap();