Combination of the RotatingFileSink and RotationNamingScheme::DateAndTime

[fahrudinhalilovic]

One kinda strange behaviour which I spotted when working with RotatingFileSink and using RotationNamingScheme::DateAndTime for rotation naming scheme is that the log files which have been generated in the previous application/service runs are not cleaned up i.e. taken into account when doing the recovery of the files and performing rotation later on in time. Our problem is that if for any reason let's say any of our services crashed and we used the above mentioned combination with maximum 10 files with 100MB per each logging file then with the next automatic (or even manual) service restart the previously created files will not be taken into account and our service will start accumulating them inside the destination directory where the log files are stored. Is this really intended behaviour? I found this section of the code:

  void _clean_and_recover_files(fs::path const& filename, std::string const& open_mode, uint64_t today_timestamp_ns)
  {
    if ((_config.rotation_naming_scheme() != RotatingFileSinkConfig::RotationNamingScheme::Index) &&
        (_config.rotation_naming_scheme() != RotatingFileSinkConfig::RotationNamingScheme::Date))
    {
      // clean and recover is only supported for index and date naming scheme, when using
      // DateAndTime there are no collisions in the filenames
      return;
    }

I mean having unique filenames is one thing but not taking them into account when restarting app/service is kinda another thing meaning that they are "left" behind. Is it also meant that this behaviour remains like this or any changes here are gonna happen in the future?

I have this utility function which performs clean up of the files (kinda duplicates the work of the library but this will take into account case when there is a new start of the app/service).

std::shared_ptr<quill::Sink> CreateRotatingFileSink(CustomFileSinkConfig file_sink_cfg)
{
	auto clean_up_old_files = [fsc = file_sink_cfg](const std::filesystem::path& file_path) {
		std::deque<std::filesystem::path> matching_files;

		for (const auto& entry : std::filesystem::directory_iterator(fsc.destination)) {
			if (!entry.is_regular_file()) {
				continue;
			}

			if (const auto& filename = entry.path().filename().string(); filename.starts_with(fsc.file_name_prefix)) {
				matching_files.emplace_back(entry.path());
			}
		}

		std::sort(std::begin(matching_files), std::end(matching_files), [](const auto& a, const auto& b) {
			return std::filesystem::last_write_time(a) < std::filesystem::last_write_time(b);
		});

		while (matching_files.size() >= fsc.max_backup_files()) {
			const auto& oldest_file = matching_files.front();
			if (std::filesystem::is_regular_file(oldest_file)) {
				std::error_code ec{ };
				std::filesystem::remove(oldest_file, ec);
				if (ec) {
					throw std::runtime_error{ std::format("Can not delete oldest log file {}!", oldest_file.string()) };
				}
				else {
					// removing completed, get rid of that file
					matching_files.pop_front();
				}
			}
		}
	};

	quill::RotatingFileSinkConfig cfg = file_sink_cfg;
	auto destination_path{ file_sink_cfg.destination / file_sink_cfg.file_name_prefix };
	quill::FileEventNotifier file_event_notifier{ .before_open = std::move(clean_up_old_files) };
	auto rotating_file_sink = quill::Frontend::create_or_get_sink<quill::RotatingFileSink>(destination_path.string(), cfg, std::move(file_event_notifier));
	if (file_sink_cfg.prevent_duplicate_messages) {
		rotating_file_sink->add_filter(std::make_unique<DuplicateLogFilter>(CreateUniqueFilterName()));
	}
	return rotating_file_sink;
}

[AdyaTech]

Yes, this appears to be intentional.

The code explicitly skips recovery/cleanup for RotationNamingScheme::DateAndTime because the generated filenames are unique and cannot collide. As a result, existing rotated log files from previous application runs are not considered when enforcing max_backup_files.

That said, I agree this can be surprising. While unique filenames prevent collisions, they don't prevent old log files from accumulating across restarts. In practice, max_backup_files is only enforced for files created during the current process lifetime.

Your cleanup workaround seems like a reasonable solution for now. It effectively enforces retention across restarts, which the current DateAndTime implementation does not do.

[fahrudinhalilovic]

Can documentation at least be enhanced in the section where enum RotationNamingScheme are mentioned so that this behaviour is documented and other devs do not get "surprised" by this 😬 I think this would be useful... There are no any plans in the future to change this behaviour?

[HyunjinX917]

#931

[odygrd]

Thanks for reporting this! The library lets you handle custom behaviours like this by leeting you create your own custom Sink. I think this one is fine to support directly in the built-in RotatingFileSink.