feat(daemon): add daily cleanup timer service for large log files#161
feat(daemon): add daily cleanup timer service for large log files#161
Conversation
Add a configurable cleanup timer service that runs every 24 hours to automatically remove log files exceeding the configured threshold. Changes: - Add LogCleanup config struct with enabled (default: true) and thresholdMB (default: 100) options - Add daemon log path helpers for macOS daemon logs - Create shared log cleanup module in model package - Refactor gc command to use shared cleanup logic - Create CleanupTimerService following HeartbeatResyncService pattern - Platform-aware: only cleans daemon logs on macOS (Linux uses journal) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust and configurable system for automatically managing and cleaning up large log files generated by the daemon and CLI. By implementing a daily cleanup timer service and centralizing log management logic, it aims to prevent excessive disk space consumption due to growing log files. The changes ensure that log cleanup is efficient, configurable, and intelligently handles platform-specific logging practices. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a daily cleanup service for large log files, which is a great addition for long-running daemons. The implementation is mostly solid, following existing service patterns. I've identified a few areas for improvement, mainly around code duplication, API correctness, and consistency. My main suggestions involve refactoring the log cleaning logic into a shared helper to reduce duplication and correcting function signatures that incorrectly promise an error return. I've also pointed out a redundant check and the use of magic numbers that could be replaced by constants for better readability. Additionally, a reminder has been added regarding best practices for platform-independent path construction.
| } | ||
|
|
||
| // Start cleanup timer service if enabled (enabled by default) | ||
| if cfg.LogCleanup != nil && cfg.LogCleanup.Enabled != nil && *cfg.LogCleanup.Enabled { |
There was a problem hiding this comment.
The ReadConfigFile function ensures that cfg.LogCleanup and cfg.LogCleanup.Enabled are initialized with default values and are never nil. Therefore, this check can be simplified to just check the boolean value.
| if cfg.LogCleanup != nil && cfg.LogCleanup.Enabled != nil && *cfg.LogCleanup.Enabled { | |
| if *cfg.LogCleanup.Enabled { |
| slog.Warn("failed to read config, using default threshold", slog.Any("err", err)) | ||
| cfg.LogCleanup = &model.LogCleanup{ThresholdMB: 100} | ||
| } | ||
| thresholdBytes := cfg.LogCleanup.ThresholdMB * 1024 * 1024 |
There was a problem hiding this comment.
The magic number multiplication * 1024 * 1024 to convert megabytes to bytes is also used in daemon/cleanup_timer.go. To improve readability and avoid duplication, consider defining a shared constant for this conversion, for example const BytesPerMB = 1024 * 1024 in the model package, and using it in both places.
| freedBytes, err := model.CleanLargeLogFiles(thresholdBytes, forceCleanLogs) | ||
| if err != nil { | ||
| slog.Warn("error during log cleanup", slog.Any("err", err)) | ||
| } |
There was a problem hiding this comment.
Following the proposed change in model/log_cleanup.go to have CleanLargeLogFiles no longer return an error, this error handling block becomes redundant and can be removed.
| freedBytes, err := model.CleanLargeLogFiles(thresholdBytes, forceCleanLogs) | |
| if err != nil { | |
| slog.Warn("error during log cleanup", slog.Any("err", err)) | |
| } | |
| freedBytes, _ := model.CleanLargeLogFiles(thresholdBytes, forceCleanLogs) |
|
|
||
| // cleanup performs the log cleanup | ||
| func (s *CleanupTimerService) cleanup(ctx context.Context) { | ||
| thresholdBytes := s.config.LogCleanup.ThresholdMB * 1024 * 1024 |
There was a problem hiding this comment.
The magic number multiplication * 1024 * 1024 to convert megabytes to bytes is also used in commands/gc.go. To improve readability and avoid duplication, consider defining a shared constant for this conversion, for example const BytesPerMB = 1024 * 1024 in the model package, and using it in both places.
| freedCLI, err := model.CleanLargeLogFiles(thresholdBytes, false) | ||
| if err != nil { | ||
| slog.Warn("error during CLI log cleanup", slog.Any("err", err)) | ||
| } |
There was a problem hiding this comment.
Following the proposed change in model/log_cleanup.go to have CleanLargeLogFiles no longer return an error, this error handling block becomes redundant and can be removed.
| freedCLI, err := model.CleanLargeLogFiles(thresholdBytes, false) | |
| if err != nil { | |
| slog.Warn("error during CLI log cleanup", slog.Any("err", err)) | |
| } | |
| freedCLI, _ := model.CleanLargeLogFiles(thresholdBytes, false) |
| freedDaemon, err := model.CleanDaemonLogFiles(thresholdBytes, false) | ||
| if err != nil { | ||
| slog.Warn("error during daemon log cleanup", slog.Any("err", err)) | ||
| } |
There was a problem hiding this comment.
Following the proposed change in model/log_cleanup.go to have CleanDaemonLogFiles no longer return an error, this error handling block becomes redundant and can be removed.
| freedDaemon, err := model.CleanDaemonLogFiles(thresholdBytes, false) | |
| if err != nil { | |
| slog.Warn("error during daemon log cleanup", slog.Any("err", err)) | |
| } | |
| freedDaemon, _ := model.CleanDaemonLogFiles(thresholdBytes, false) |
| // CleanLargeLogFiles checks CLI log files and removes those exceeding the size threshold. | ||
| // thresholdBytes: size threshold in bytes | ||
| // If force is true, removes all log files regardless of size. | ||
| func CleanLargeLogFiles(thresholdBytes int64, force bool) (int64, error) { | ||
| logFiles := []string{ | ||
| GetLogFilePath(), | ||
| GetHeartbeatLogFilePath(), | ||
| GetSyncPendingFilePath(), | ||
| } | ||
|
|
||
| var totalFreed int64 | ||
| for _, filePath := range logFiles { | ||
| freed, err := CleanLogFile(filePath, thresholdBytes, force) | ||
| if err != nil { | ||
| slog.Warn("failed to clean log file", slog.String("file", filePath), slog.Any("err", err)) | ||
| continue | ||
| } | ||
| totalFreed += freed | ||
| } | ||
|
|
||
| return totalFreed, nil | ||
| } | ||
|
|
||
| // CleanDaemonLogFiles cleans daemon-specific log files. | ||
| // On macOS, daemon logs go to ~/.shelltime/logs/shelltime-daemon.{log,err} | ||
| // On Linux, daemon logs go to systemd journal and can't be cleaned from here. | ||
| // thresholdBytes: size threshold in bytes | ||
| // If force is true, removes all log files regardless of size. | ||
| func CleanDaemonLogFiles(thresholdBytes int64, force bool) (int64, error) { | ||
| // Only clean daemon logs on macOS (darwin) | ||
| // On Linux, daemon uses systemd journal which is managed by journald | ||
| if runtime.GOOS != "darwin" { | ||
| return 0, nil | ||
| } | ||
|
|
||
| logFiles := []string{ | ||
| GetDaemonLogFilePath(), | ||
| GetDaemonErrFilePath(), | ||
| } | ||
|
|
||
| var totalFreed int64 | ||
| for _, filePath := range logFiles { | ||
| freed, err := CleanLogFile(filePath, thresholdBytes, force) | ||
| if err != nil { | ||
| slog.Warn("failed to clean daemon log file", slog.String("file", filePath), slog.Any("err", err)) | ||
| continue | ||
| } | ||
| totalFreed += freed | ||
| } | ||
|
|
||
| return totalFreed, nil | ||
| } |
There was a problem hiding this comment.
The functions CleanLargeLogFiles and CleanDaemonLogFiles contain duplicate logic for iterating over a list of files and cleaning them. This can be extracted into a private helper function to improve maintainability.
Additionally, both functions are declared to return an error, but they always return nil, making the error check at call sites redundant. The function signatures should be changed to only return (int64, error) to reflect this. The callers in commands/gc.go and daemon/cleanup_timer.go will need to be updated to no longer check for an error.
When constructing file paths, such as those returned by GetLogFilePath() and similar functions, ensure that filepath.Join is used for combining segments and os.UserHomeDir() for the home directory to maintain platform independence.
func cleanFiles(filePaths []string, thresholdBytes int64, force bool, warnMsg string) int64 {
var totalFreed int64
for _, filePath := range filePaths {
freed, err := CleanLogFile(filePath, thresholdBytes, force)
if err != nil {
slog.Warn(warnMsg, slog.String("file", filePath), slog.Any("err", err))
continue
}
totalFreed += freed
}
return totalFreed
}
// CleanLargeLogFiles checks CLI log files and removes those exceeding the size threshold.
// thresholdBytes: size threshold in bytes
// If force is true, removes all log files regardless of size.
func CleanLargeLogFiles(thresholdBytes int64, force bool) (int64, error) {
logFiles := []string{
GetLogFilePath(),
GetHeartbeatLogFilePath(),
GetSyncPendingFilePath(),
}
return cleanFiles(logFiles, thresholdBytes, force, "failed to clean log file"), nil
}
// CleanDaemonLogFiles cleans daemon-specific log files.
// On macOS, daemon logs go to ~/.shelltime/logs/shelltime-daemon.{log,err}
// On Linux, daemon logs go to systemd journal and can't be cleaned from here.
// thresholdBytes: size threshold in bytes
// If force is true, removes all log files regardless of size.
func CleanDaemonLogFiles(thresholdBytes int64, force bool) (int64, error) {
// Only clean daemon logs on macOS (darwin)
// On Linux, daemon uses systemd journal which is managed by journald
if runtime.GOOS != "darwin" {
return 0, nil
}
logFiles := []string{
GetDaemonLogFilePath(),
GetDaemonErrFilePath(),
}
return cleanFiles(logFiles, thresholdBytes, force, "failed to clean daemon log file"), nil
}References
- For platform-independent paths, use
filepath.Jointo combine segments andos.UserHomeDir()to get the home directory, rather than hardcoding path separators or environment variables like$HOME.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive tests for: - LogCleanup defaults (enabled=true, thresholdMB=100) - LogCleanup custom values - LogCleanup partial config (only enabled or only threshold) - LogCleanup merge from local config - CodeTracking merge from local config 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The gc command now requires LogCleanup config to be present. Update the mock to include proper LogCleanup defaults. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Summary
LogCleanupconfig struct withenabled(default: true) andthresholdMB(default: 100) optionsCleanupTimerServicefollowingHeartbeatResyncServicepatternConfig Example
Test plan
shelltime gccommand works with configurable thresholdenabled = falsethresholdMB🤖 Generated with Claude Code