From bd382f89be835a6724db69f833d0ae3ef79d50d3 Mon Sep 17 00:00:00 2001 From: Daniele Pallastrelli <5451767+daniele77@users.noreply.github.com> Date: Tue, 14 Apr 2026 09:42:28 +0200 Subject: [PATCH 1/4] 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: - 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 --- include/spdlog/sinks/daily_file_sink.h | 184 +++++++++++++++++++++---- tests/test_daily_logger.cpp | 43 ++++++ 2 files changed, 197 insertions(+), 30 deletions(-) diff --git a/include/spdlog/sinks/daily_file_sink.h b/include/spdlog/sinks/daily_file_sink.h index fa2582de76..420405132e 100644 --- a/include/spdlog/sinks/daily_file_sink.h +++ b/include/spdlog/sinks/daily_file_sink.h @@ -4,7 +4,6 @@ #pragma once #include -#include #include #include #include @@ -13,12 +12,21 @@ #include #include +#if defined(_WIN32) +#include +#else +#include +#include +#endif + +#include #include #include #include #include #include #include +#include namespace spdlog { namespace sinks { @@ -62,8 +70,8 @@ struct daily_filename_format_calculator { * Rotating file sink based on date. * If truncate != false , the created file will be truncated. * If max_files > 0, retain only the last max_files and delete previous. - * Note that old log files from previous executions will not be deleted by this class, - * rotation and deletion is only applied while the program is running. + * Cleanup scans the target directory and removes old files matching this sink naming pattern. + * Rotation and deletion is applied while the program is running. */ template class daily_file_sink final : public base_sink { @@ -80,8 +88,7 @@ class daily_file_sink final : public base_sink { rotation_m_(rotation_minute), file_helper_{event_handlers}, truncate_(truncate), - max_files_(max_files), - filenames_q_() { + max_files_(max_files) { if (rotation_hour < 0 || rotation_hour > 23 || rotation_minute < 0 || rotation_minute > 59) { throw_spdlog_ex("daily_file_sink: Invalid rotation time in ctor"); @@ -91,10 +98,6 @@ class daily_file_sink final : public base_sink { const auto new_filename = FileNameCalc::calc_filename(base_filename_, now_tm(now)); file_helper_.open(new_filename, truncate_); rotation_tp_ = next_rotation_tp_(); - - if (max_files_ > 0) { - init_filenames_q_(); - } } filename_t filename() { @@ -124,23 +127,130 @@ class daily_file_sink final : public base_sink { void flush_() override { file_helper_.flush(); } private: - void init_filenames_q_() { - using details::os::path_exists; + static bool starts_with_(const filename_t &value, const filename_t &prefix) { + return value.size() >= prefix.size() && + std::equal(prefix.begin(), prefix.end(), value.begin()); + } - filenames_q_ = details::circular_q(static_cast(max_files_)); - std::vector filenames; - auto now = log_clock::now(); - while (filenames.size() < max_files_) { - const auto new_filename = FileNameCalc::calc_filename(base_filename_, now_tm(now)); - if (!path_exists(new_filename)) { - break; + static bool date_like_suffix_(const filename_t &suffix) { + if (suffix.empty()) { + return false; + } + + bool has_digit = false; + const auto c0 = static_cast('0'); + const auto c9 = static_cast('9'); + const auto dash = static_cast('-'); + const auto underscore = static_cast('_'); + for (auto ch : suffix) { + if (ch >= c0 && ch <= c9) { + has_digit = true; + continue; } - filenames.emplace_back(new_filename); - now -= std::chrono::hours(24); + if (ch == dash || ch == underscore) { + continue; + } + return false; } - for (auto iter = filenames.rbegin(); iter != filenames.rend(); ++iter) { - filenames_q_.push_back(std::move(*iter)); + + return has_digit; + } + + static filename_t join_path_(const filename_t &dir, const filename_t &basename) { + if (dir.empty()) { + return basename; } + + filename_t result = dir; + if (result.back() != details::os::folder_seps_filename[0]) { + result.push_back(details::os::folder_seps_filename[0]); + } + result += basename; + return result; + } + + bool is_matching_daily_file_(const filename_t &filename) const { + filename_t base_name_no_ext; + filename_t base_ext; + std::tie(base_name_no_ext, base_ext) = details::file_helper::split_by_extension(base_filename_); + + filename_t candidate_no_ext; + 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)) { + return false; + } + + return date_like_suffix_(candidate_no_ext.substr(base_name_no_ext.size())); + } + + std::vector scan_matching_filenames_() const { + std::vector matching_filenames; + const filename_t dir_path = details::os::dir_name(base_filename_); + +#if defined(_WIN32) + filename_t search_pattern = dir_path.empty() ? SPDLOG_FILENAME_T("*") + : join_path_(dir_path, SPDLOG_FILENAME_T("*")); + +#if defined(SPDLOG_WCHAR_FILENAMES) + WIN32_FIND_DATAW find_data; + HANDLE find_handle = ::FindFirstFileW(search_pattern.c_str(), &find_data); +#else + WIN32_FIND_DATAA find_data; + HANDLE find_handle = ::FindFirstFileA(search_pattern.c_str(), &find_data); +#endif + if (find_handle == INVALID_HANDLE_VALUE) { + return matching_filenames; + } + + do { + if ((find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0) { + continue; + } + + filename_t filename = find_data.cFileName; + const filename_t full_path = join_path_(dir_path, filename); + if (is_matching_daily_file_(full_path)) { + matching_filenames.push_back(std::move(full_path)); + } + } while ( +#if defined(SPDLOG_WCHAR_FILENAMES) + ::FindNextFileW(find_handle, &find_data) +#else + ::FindNextFileA(find_handle, &find_data) +#endif + ); + ::FindClose(find_handle); +#else + const filename_t scan_path = dir_path.empty() ? SPDLOG_FILENAME_T(".") : dir_path; + DIR *dir = ::opendir(scan_path.c_str()); + if (dir == nullptr) { + return matching_filenames; + } + + struct dirent *entry = nullptr; + while ((entry = ::readdir(dir)) != nullptr) { + if (entry->d_name[0] == '.') { + continue; + } + + const filename_t filename = entry->d_name; + const filename_t full_path = join_path_(dir_path, filename); + + struct stat st; + if (::stat(full_path.c_str(), &st) != 0 || !S_ISREG(st.st_mode)) { + continue; + } + + if (is_matching_daily_file_(full_path)) { + matching_filenames.push_back(full_path); + } + } + (void)::closedir(dir); +#endif + + return matching_filenames; } tm now_tm(log_clock::time_point tp) { @@ -167,18 +277,33 @@ class daily_file_sink final : public base_sink { using details::os::filename_to_str; using details::os::remove_if_exists; - filename_t current_file = file_helper_.filename(); - if (filenames_q_.full()) { - auto old_filename = std::move(filenames_q_.front()); - filenames_q_.pop_front(); - bool ok = remove_if_exists(old_filename) == 0; + auto matching_filenames = scan_matching_filenames_(); + const filename_t current_file = file_helper_.filename(); + + matching_filenames.erase( + std::remove(matching_filenames.begin(), matching_filenames.end(), current_file), + matching_filenames.end()); + + const size_t files_to_keep_excluding_current = + max_files_ > 0 ? static_cast(max_files_ - 1) : 0; + + if (matching_filenames.size() <= files_to_keep_excluding_current) { + return; + } + + std::sort(matching_filenames.begin(), matching_filenames.end()); + + const size_t files_to_delete = + matching_filenames.size() - files_to_keep_excluding_current; + + for (size_t i = 0; i < files_to_delete; ++i) { + const filename_t &old_filename = matching_filenames[i]; + const bool ok = remove_if_exists(old_filename) == 0; if (!ok) { - filenames_q_.push_back(std::move(current_file)); throw_spdlog_ex("Failed removing daily file " + filename_to_str(old_filename), errno); } } - filenames_q_.push_back(std::move(current_file)); } filename_t base_filename_; @@ -188,7 +313,6 @@ class daily_file_sink final : public base_sink { details::file_helper file_helper_; bool truncate_; uint16_t max_files_; - details::circular_q filenames_q_; }; using daily_file_sink_mt = daily_file_sink; diff --git a/tests/test_daily_logger.cpp b/tests/test_daily_logger.cpp index 4a691a3c54..6fe34d8031 100644 --- a/tests/test_daily_logger.cpp +++ b/tests/test_daily_logger.cpp @@ -4,6 +4,8 @@ */ #include "includes.h" +#include + #ifdef SPDLOG_USE_STD_FORMAT using filename_memory_buf_t = std::basic_string; #else @@ -131,6 +133,23 @@ static spdlog::details::log_msg create_msg(std::chrono::seconds offset) { return msg; } +static void touch_file(const spdlog::filename_t &filename) { + using spdlog::details::os::create_dir; + using spdlog::details::os::dir_name; + + // Create directory if needed + const auto dir_path = dir_name(filename); + if (!dir_path.empty()) { + create_dir(dir_path); + } + + FILE *fd = nullptr; + if (spdlog::details::os::fopen_s(&fd, filename, SPDLOG_FILENAME_T("ab"))) { + throw std::runtime_error("Failed creating file " + spdlog::details::os::filename_to_str(filename)); + } + std::fclose(fd); +} + static void test_rotate(int days_to_run, uint16_t max_days, uint16_t expected_n_files) { using spdlog::log_clock; using spdlog::details::log_msg; @@ -167,3 +186,27 @@ TEST_CASE("daily_logger rotate", "[daily_file_sink]") { test_rotate(days_to_run, 11, 10); test_rotate(days_to_run, 20, 10); } +TEST_CASE("daily_logger rotate with missing day", "[daily_file_sink]") { + using spdlog::log_clock; + using spdlog::sinks::daily_file_sink_st; + using spdlog::sinks::daily_filename_calculator; + + prepare_logdir(); + + const spdlog::filename_t basename = SPDLOG_FILENAME_T("test_logs/daily_gap.txt"); + const uint16_t max_files = 3; + + const auto now = log_clock::now(); + const std::vector existing_days_ago = {0, 1, 2, 4, 5}; + for (int days_ago : existing_days_ago) { + const auto day_tp = now - std::chrono::hours(24 * days_ago); + const auto filename = daily_filename_calculator::calc_filename( + basename, spdlog::details::os::localtime(log_clock::to_time_t(day_tp))); + touch_file(filename); + } + + daily_file_sink_st sink{basename, 2, 30, true, max_files}; + sink.log(create_msg(std::chrono::hours(24))); + + REQUIRE(count_files("test_logs") == static_cast(max_files)); +} \ No newline at end of file From bd91e89daa623f6465dbc9dc932ca1319516b5cf Mon Sep 17 00:00:00 2001 From: Daniele Pallastrelli <5451767+daniele77@users.noreply.github.com> Date: Wed, 22 Apr 2026 16:45:26 +0200 Subject: [PATCH 2/4] Enhance daily_file_sink suffix validation and add test for false-positive cleanup --- include/spdlog/sinks/daily_file_sink.h | 54 +++++++++++++++++++++----- tests/test_daily_logger.cpp | 43 ++++++++++++++++++++ 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/include/spdlog/sinks/daily_file_sink.h b/include/spdlog/sinks/daily_file_sink.h index 420405132e..3c75cf257f 100644 --- a/include/spdlog/sinks/daily_file_sink.h +++ b/include/spdlog/sinks/daily_file_sink.h @@ -133,27 +133,63 @@ class daily_file_sink final : public base_sink { } static bool date_like_suffix_(const filename_t &suffix) { - if (suffix.empty()) { + // Validate that suffix matches the daily_filename_calculator pattern: _YYYY-MM-DD + // Expected format: _4digits-2digits-2digits (case insensitive for hexadecimal won't apply here) + // Examples valid: _2026-04-22 + // Examples invalid: _123, _v1_2, _latest, _12-34-5, etc. + + if (suffix.size() < 11) { // Minimum: _YYYY-MM-DD = 11 chars return false; } - bool has_digit = false; const auto c0 = static_cast('0'); const auto c9 = static_cast('9'); const auto dash = static_cast('-'); const auto underscore = static_cast('_'); - for (auto ch : suffix) { - if (ch >= c0 && ch <= c9) { - has_digit = true; - continue; + + // Must start with underscore or hyphen (depending on calculator variant) + size_t pos = 0; + if (suffix[0] != underscore && suffix[0] != dash) { + return false; + } + ++pos; + + // Expect exactly 4 digits (YYYY) + for (int i = 0; i < 4; ++i, ++pos) { + if (pos >= suffix.size() || suffix[pos] < c0 || suffix[pos] > c9) { + return false; } - if (ch == dash || ch == underscore) { - continue; + } + + // Expect dash + if (pos >= suffix.size() || suffix[pos] != dash) { + return false; + } + ++pos; + + // Expect exactly 2 digits (MM) + for (int i = 0; i < 2; ++i, ++pos) { + if (pos >= suffix.size() || suffix[pos] < c0 || suffix[pos] > c9) { + return false; } + } + + // Expect dash + if (pos >= suffix.size() || suffix[pos] != dash) { return false; } + ++pos; + + // Expect exactly 2 digits (DD) + for (int i = 0; i < 2; ++i, ++pos) { + if (pos >= suffix.size() || suffix[pos] < c0 || suffix[pos] > c9) { + return false; + } + } - return has_digit; + // All characters consumed should be the date pattern + // Allow trailing characters (for format calculators that append more) + return true; } static filename_t join_path_(const filename_t &dir, const filename_t &basename) { diff --git a/tests/test_daily_logger.cpp b/tests/test_daily_logger.cpp index 6fe34d8031..6ec4d73189 100644 --- a/tests/test_daily_logger.cpp +++ b/tests/test_daily_logger.cpp @@ -209,4 +209,47 @@ TEST_CASE("daily_logger rotate with missing day", "[daily_file_sink]") { sink.log(create_msg(std::chrono::hours(24))); REQUIRE(count_files("test_logs") == static_cast(max_files)); +} + +TEST_CASE("daily_logger rotate avoids false-positive cleanup", "[daily_file_sink]") { + using spdlog::log_clock; + using spdlog::sinks::daily_file_sink_st; + using spdlog::sinks::daily_filename_calculator; + + prepare_logdir(); + + const spdlog::filename_t basename = SPDLOG_FILENAME_T("test_logs/daily_falsepos.txt"); + const uint16_t max_files = 2; + + const auto now = log_clock::now(); + + // Create legitimate daily log files + const std::vector existing_days_ago = {0, 1}; + for (int days_ago : existing_days_ago) { + const auto day_tp = now - std::chrono::hours(24 * days_ago); + const auto filename = daily_filename_calculator::calc_filename( + basename, spdlog::details::os::localtime(log_clock::to_time_t(day_tp))); + touch_file(filename); + } + + // Create ambiguous files with same prefix/extension but non-date-like suffix + // These should NOT be deleted even if they match the prefix/extension pattern + const std::vector ambiguous_files = { + SPDLOG_FILENAME_T("test_logs/daily_falsepos_123.txt"), // digits only + SPDLOG_FILENAME_T("test_logs/daily_falsepos_v1_2.txt"), // digits + underscore + SPDLOG_FILENAME_T("test_logs/daily_falsepos_latest.txt"), // no digits + }; + for (const auto &file : ambiguous_files) { + touch_file(file); + } + + // Trigger cleanup by rotating to a new day + daily_file_sink_st sink{basename, 2, 30, true, max_files}; + sink.log(create_msg(std::chrono::hours(48))); + + // Verify that only legitimate daily log files remain (or fewer if some are deleted) + // but ambiguous files should still exist + for (const auto &file : ambiguous_files) { + REQUIRE(spdlog::details::os::path_exists(file) == true); + } } \ No newline at end of file From 457a545bfa79d3b07af34569e2df4459fc3d016e Mon Sep 17 00:00:00 2001 From: Daniele Pallastrelli <5451767+daniele77@users.noreply.github.com> Date: Wed, 22 Apr 2026 16:47:03 +0200 Subject: [PATCH 3/4] Improve directory entry filtering in daily_file_sink to skip only '.' and '..' entries --- include/spdlog/sinks/daily_file_sink.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/spdlog/sinks/daily_file_sink.h b/include/spdlog/sinks/daily_file_sink.h index 3c75cf257f..e62b6c6a69 100644 --- a/include/spdlog/sinks/daily_file_sink.h +++ b/include/spdlog/sinks/daily_file_sink.h @@ -267,7 +267,8 @@ class daily_file_sink final : public base_sink { struct dirent *entry = nullptr; while ((entry = ::readdir(dir)) != nullptr) { - if (entry->d_name[0] == '.') { + if (entry->d_name[0] == '.' && + (entry->d_name[1] == '\0' || (entry->d_name[1] == '.' && entry->d_name[2] == '\0'))) { continue; } From 986fd4f955f4b57bf9e08841db0d78617869586c Mon Sep 17 00:00:00 2001 From: Daniele Pallastrelli <5451767+daniele77@users.noreply.github.com> Date: Wed, 22 Apr 2026 17:00:48 +0200 Subject: [PATCH 4/4] Normalize path separators in daily_file_sink for consistent file handling across platforms --- include/spdlog/sinks/daily_file_sink.h | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/include/spdlog/sinks/daily_file_sink.h b/include/spdlog/sinks/daily_file_sink.h index e62b6c6a69..84c77c2087 100644 --- a/include/spdlog/sinks/daily_file_sink.h +++ b/include/spdlog/sinks/daily_file_sink.h @@ -132,6 +132,17 @@ class daily_file_sink final : public base_sink { std::equal(prefix.begin(), prefix.end(), value.begin()); } + // Normalize path separators to '/' so comparisons are reliable regardless of + // whether the caller used '\' or '/' (both are valid on Windows). + static filename_t normalize_seps_(filename_t path) { +#if defined(_WIN32) + std::replace(path.begin(), path.end(), + static_cast('\\'), + static_cast('/')); +#endif + return path; + } + static bool date_like_suffix_(const filename_t &suffix) { // Validate that suffix matches the daily_filename_calculator pattern: _YYYY-MM-DD // Expected format: _4digits-2digits-2digits (case insensitive for hexadecimal won't apply here) @@ -198,8 +209,11 @@ class daily_file_sink final : public base_sink { } filename_t result = dir; - if (result.back() != details::os::folder_seps_filename[0]) { - result.push_back(details::os::folder_seps_filename[0]); + // Check against ALL valid separators (on Windows both '\' and '/' are valid). + // folder_seps_filename contains all valid separator characters for the platform. + const filename_t seps(details::os::folder_seps_filename); + if (seps.find(result.back()) == filename_t::npos) { + result.push_back(seps[0]); } result += basename; return result; @@ -209,10 +223,12 @@ class daily_file_sink final : public base_sink { filename_t base_name_no_ext; filename_t base_ext; std::tie(base_name_no_ext, base_ext) = details::file_helper::split_by_extension(base_filename_); + base_name_no_ext = normalize_seps_(base_name_no_ext); filename_t candidate_no_ext; filename_t candidate_ext; std::tie(candidate_no_ext, candidate_ext) = details::file_helper::split_by_extension(filename); + candidate_no_ext = normalize_seps_(candidate_no_ext); if (candidate_ext != base_ext || !starts_with_(candidate_no_ext, base_name_no_ext)) { return false; @@ -248,7 +264,7 @@ class daily_file_sink final : public base_sink { filename_t filename = find_data.cFileName; const filename_t full_path = join_path_(dir_path, filename); if (is_matching_daily_file_(full_path)) { - matching_filenames.push_back(std::move(full_path)); + matching_filenames.push_back(normalize_seps_(full_path)); } } while ( #if defined(SPDLOG_WCHAR_FILENAMES) @@ -281,7 +297,7 @@ class daily_file_sink final : public base_sink { } if (is_matching_daily_file_(full_path)) { - matching_filenames.push_back(full_path); + matching_filenames.push_back(normalize_seps_(full_path)); } } (void)::closedir(dir); @@ -315,7 +331,7 @@ class daily_file_sink final : public base_sink { using details::os::remove_if_exists; auto matching_filenames = scan_matching_filenames_(); - const filename_t current_file = file_helper_.filename(); + const filename_t current_file = normalize_seps_(file_helper_.filename()); matching_filenames.erase( std::remove(matching_filenames.begin(), matching_filenames.end(), current_file),