Fix #2553: daily_file_sink cleanup now scans directory instead of relying on filenames_q_#3598
Fix #2553: daily_file_sink cleanup now scans directory instead of relying on filenames_q_#3598daniele77 wants to merge 4 commits into
Conversation
…of relying on filenames_q_ Daily rotation does not remove files when one day is missing. The issue was caused by init_filenames_q_() breaking on the first missing file. Changes: - Replace queue-based (filenames_q_) retention with directory scan - Cleanup now enumerates matching files, sorts by name, deletes oldest if count exceeds max_files - Handles gap days, long restarts, manual deletes robustly - Works cross-platform (Windows/Unix) Tests: - Added regression test for gap-day scenario - All existing daily logger tests still pass
There was a problem hiding this comment.
Pull request overview
Fixes daily file sink retention so cleanup no longer stops at the first missing day by scanning the directory for matching daily log files and deleting the oldest beyond max_files.
Changes:
- Replaced queue-based retention (
filenames_q_) with a directory scan + sort + delete approach indaily_file_sink. - Added cross-platform directory enumeration (Win32
FindFirstFile*/ POSIXopendir+stat) to find candidate files for cleanup. - Added a regression test covering a “gap day” scenario to ensure retention still deletes the oldest files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
include/spdlog/sinks/daily_file_sink.h |
Implements directory-scan-based retention cleanup and removes the old circular-queue mechanism. |
tests/test_daily_logger.cpp |
Adds a regression test that pre-creates a gapped set of daily files and verifies cleanup reduces to max_files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ch == dash || ch == underscore) { | ||
| continue; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
date_like_suffix_ accepts any mix of digits, '-' and '_' (with at least one digit). That can match and delete unrelated files like "_123.txt" even though the built-in daily_filename_calculator always produces the stricter "_YYYY-MM-DD" suffix. Tightening this to the exact expected pattern (or making the predicate depend on FileNameCalc) would reduce the risk of deleting non-log files that share the same prefix/extension.
|
|
||
| struct dirent *entry = nullptr; | ||
| while ((entry = ::readdir(dir)) != nullptr) { | ||
| if (entry->d_name[0] == '.') { |
There was a problem hiding this comment.
POSIX directory scan skips any entry whose name starts with '.' (entry->d_name[0] == '.'). This excludes legitimate hidden log files (e.g. base filename ".myapp.log" -> ".myapp_YYYY-MM-DD.log"), so retention cleanup will never see/delete them. It should only skip the special "." and ".." entries (or skip dotfiles only when the base filename itself is not hidden).
| if (entry->d_name[0] == '.') { | |
| if (entry->d_name[0] == '.' && | |
| (entry->d_name[1] == '\0' || (entry->d_name[1] == '.' && entry->d_name[2] == '\0'))) { |
| filename_t result = dir; | ||
| if (result.back() != details::os::folder_seps_filename[0]) { | ||
| result.push_back(details::os::folder_seps_filename[0]); | ||
| } |
There was a problem hiding this comment.
On Windows, join_path_ always appends details::os::folder_seps_filename[0] ("\"), which can produce mixed-separator paths when the user passes base_filename_ with forward slashes (common and used in tests). That breaks is_matching_daily_file_ prefix checks and also makes the exact current_file erase in delete_old_ unreliable. Consider appending a separator only if dir.back() is not any folder separator, and preserve/normalize separators consistently between base_filename_, scan results, and file_helper_.filename().
| filename_t candidate_ext; | ||
| std::tie(candidate_no_ext, candidate_ext) = details::file_helper::split_by_extension(filename); | ||
|
|
||
| if (candidate_ext != base_ext || !starts_with_(candidate_no_ext, base_name_no_ext)) { |
There was a problem hiding this comment.
The starts_with_ check uses base_filename_ (split_by_extension) as a literal prefix. For daily_file_format_sink_* the base filename is a strftime format string (e.g. "example-%Y-%m-%d.log"), so no rotated file ("example-2026-04-18.log") will ever match and retention cleanup becomes a no-op. Matching needs to be based on the expanded naming scheme (e.g., parse candidate with std::get_time against the format string and verify round-trip), or implement a different retention strategy for daily_filename_format_calculator/custom calculators.
There was a problem hiding this comment.
why? it search the prefix with starts_with, ignores thee date
… and '..' entries
…ling across platforms
| return result; | ||
| } | ||
|
|
||
| bool is_matching_daily_file_(const filename_t &filename) const { |
There was a problem hiding this comment.
@gabime / @daniele77, I was interested in #2553, and found my way to this PR, and thought I'd provide my 2p.
I don't think the is_matching_daily_file_ function should belong to daily_file_sink. I think it would better suited as a static function on the FileNameCalc class (as something like is_matching_file) and adding the base_filename_ as an argument to said function, just like FileNameCalc::calc_filename, and by extension moving this specific implementation to daily_filename_calculator. That way, client-code that provides a custom FileNameCalc to daily_file_sink can ensure the scan_matching_filenames_ works for their custom filenames too. However, this would obviously be a breaking change, as it would be a new requirement on FileNameCalc, so I understand if you want to avoid that.
Fix #2553: daily_file_sink cleanup now scans directory instead of relying on filenames_q_
Daily rotation does not remove files when one day is missing. The issue was caused by init_filenames_q_() breaking on the first missing file.
Changes:
Tests: