feat: 自动清理log目录#8
Conversation
There was a problem hiding this comment.
Hey - 我在这里给出了一些整体性反馈:
- 在每次调用
reinit()时都调用cleanup(),如果日志目录很大,可能会引入明显的延迟;可以考虑降低清理频率、对每次清理做上限(例如每次最多处理固定数量的文件),或者异步执行清理。 - 当前的清理逻辑会删除
log_dir_下所有超过 7 天的普通文件,而不区分文件名;如果该目录中可能存在非日志文件,建议按日志文件的命名模式或扩展名进行过滤,会更安全一些。 cleanup()目前只在Logger内部使用,如果没有明确的外部使用场景,你可以考虑把它设为私有方法,以避免将其暴露为公共 API 的一部分。
面向 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- Calling `cleanup()` on every `reinit()` may introduce noticeable latency if the log directory is large; consider making cleanup less frequent, bounded (e.g., a max number of files per call), or running it asynchronously.
- The cleanup routine deletes any regular file under `log_dir_` older than 7 days, regardless of filename; if non-log files can exist in this directory, it would be safer to filter by log file naming pattern or extension.
- Given that `cleanup()` is only used internally by `Logger`, you might want to make it private to avoid exposing it as part of the public API unless there is a specific use case for external callers.帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈来改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- Calling
cleanup()on everyreinit()may introduce noticeable latency if the log directory is large; consider making cleanup less frequent, bounded (e.g., a max number of files per call), or running it asynchronously. - The cleanup routine deletes any regular file under
log_dir_older than 7 days, regardless of filename; if non-log files can exist in this directory, it would be safer to filter by log file naming pattern or extension. - Given that
cleanup()is only used internally byLogger, you might want to make it private to avoid exposing it as part of the public API unless there is a specific use case for external callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Calling `cleanup()` on every `reinit()` may introduce noticeable latency if the log directory is large; consider making cleanup less frequent, bounded (e.g., a max number of files per call), or running it asynchronously.
- The cleanup routine deletes any regular file under `log_dir_` older than 7 days, regardless of filename; if non-log files can exist in this directory, it would be safer to filter by log file naming pattern or extension.
- Given that `cleanup()` is only used internally by `Logger`, you might want to make it private to avoid exposing it as part of the public API unless there is a specific use case for external callers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
为日志系统增加“自动清理 log 目录”的能力,在初始化/重初始化时清理过期文件,减少日志目录长期膨胀。
Changes:
- 在
Logger::reinit()中新增调用清理逻辑 - 新增
Logger::cleanup():遍历日志目录并删除超过 7 天的文件 - 在头文件中补充
cleanup()私有接口声明
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| source/Logger/Logger.cpp | 在 reinit 时执行清理,并实现按最后修改时间删除旧文件的逻辑 |
| include/MaaUtils/Logger.h | 声明新增的 cleanup() 私有方法 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const auto& entry : std::filesystem::recursive_directory_iterator(log_dir_, ec)) { | ||
| if (ec) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Even with the error_code constructor, the range-for over recursive_directory_iterator still advances the iterator via operator++, which can throw std::filesystem::filesystem_error on traversal errors. If the intent is a fully non-throwing cleanup, use an explicit iterator loop with it.increment(ec) (and consider directory_options::skip_permission_denied).
| for (const auto& entry : std::filesystem::recursive_directory_iterator(log_dir_, ec)) { | |
| if (ec) { | |
| break; | |
| } | |
| std::filesystem::recursive_directory_iterator it( | |
| log_dir_, | |
| std::filesystem::directory_options::skip_permission_denied, | |
| ec); | |
| std::filesystem::recursive_directory_iterator end; | |
| for (; it != end && !ec; it.increment(ec)) { | |
| const auto& entry = *it; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void reinit(); | ||
| void cleanup(); | ||
| bool rotate(); | ||
| void open(bool append = true); |
There was a problem hiding this comment.
PR description mentions exposing a Logger::cleanup API for manual triggering, but cleanup() is currently declared in the private section (same as reinit()), so it’s not callable by consumers. Either move cleanup() to the public API (and document expected behavior/threading), or update the PR description if it’s intended to remain internal.
| if (!entry.is_regular_file(ec) || ec) { | ||
| continue; | ||
| } | ||
|
|
||
| auto last_write = entry.last_write_time(ec); | ||
| if (ec) { | ||
| continue; | ||
| } | ||
|
|
||
| auto age = now - last_write; | ||
| if (age > kMaxAge) { | ||
| std::filesystem::remove(entry.path(), ec); | ||
| } |
There was a problem hiding this comment.
The cleanup currently deletes all regular files older than a week under log_dir_ (recursive). PR description says “delete log files”, so this could unintentionally remove unrelated files if the configured log directory is shared. Consider restricting deletion to known log filenames/patterns (e.g., maa*.log) and/or making recursion optional.
| static void remove_old_files(const std::filesystem::path& dir) | ||
| { | ||
| constexpr auto kMaxAge = std::chrono::hours(24 * 7); // 一周 | ||
| const auto now = std::filesystem::file_time_type::clock::now(); | ||
|
|
||
| std::error_code ec; | ||
| for (const auto& entry : std::filesystem::recursive_directory_iterator(dir, ec)) { | ||
| if (ec) { |
There was a problem hiding this comment.
remove_old_files runs on a detached thread, but the for (const auto& entry : std::filesystem::recursive_directory_iterator(...)) range-for uses operator++ without an error_code parameter and may throw on traversal errors (e.g., permission issues). An exception escaping a std::thread entry point will call std::terminate and crash the process. Consider iterating with it.increment(ec) (non-throwing) and/or wrapping the whole function body in a try/catch that prevents exceptions from leaving the thread (optionally logging failures).
Summary by Sourcery
在记录器重新初始化时自动清理旧日志文件。
新功能:
增强内容:
Logger::cleanupAPI,用于手动触发日志目录清理。Original summary in English
Summary by Sourcery
Add automatic cleanup of old log files during logger reinitialization.
New Features:
Enhancements: