diff --git a/include/spdlog/sinks/daily_file_sink.h b/include/spdlog/sinks/daily_file_sink.h index fa2582de76..84c77c2087 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,183 @@ 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; + // 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) + // 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; + } + + const auto c0 = static_cast('0'); + const auto c9 = static_cast('9'); + const auto dash = static_cast('-'); + const auto underscore = static_cast('_'); + + // 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; + } + } + + // 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; + } + } + + // 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) { + if (dir.empty()) { + return basename; + } + + filename_t result = dir; + // 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; + } + + 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_); + 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; + } + + 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; } - filenames.emplace_back(new_filename); - now -= std::chrono::hours(24); + + 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(normalize_seps_(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; } - for (auto iter = filenames.rbegin(); iter != filenames.rend(); ++iter) { - filenames_q_.push_back(std::move(*iter)); + + struct dirent *entry = nullptr; + while ((entry = ::readdir(dir)) != nullptr) { + if (entry->d_name[0] == '.' && + (entry->d_name[1] == '\0' || (entry->d_name[1] == '.' && entry->d_name[2] == '\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(normalize_seps_(full_path)); + } } + (void)::closedir(dir); +#endif + + return matching_filenames; } tm now_tm(log_clock::time_point tp) { @@ -167,18 +330,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 = normalize_seps_(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 +366,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..6ec4d73189 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,70 @@ 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)); +} + +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