Refactor Logger for Dated Log Files and Implement Rotation#534
Merged
reachraza merged 7 commits intoRunMaestro:0.16.0-RCfrom Mar 13, 2026
Merged
Refactor Logger for Dated Log Files and Implement Rotation#534reachraza merged 7 commits intoRunMaestro:0.16.0-RCfrom
reachraza merged 7 commits intoRunMaestro:0.16.0-RCfrom
Conversation
…Dir() Extract getLogsDir() helper from getLogFilePath() and change log filename from static maestro-debug.log to dated maestro-debug-YYYY-MM-DD.log using local date. This is the foundation for daily log rotation with 7-day cleanup. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Adds currentLogDate and rotationTimer fields, extracts getTodayDateString() helper, and implements rotateIfNeeded() which detects date changes and rotates to a new log file. Includes cleanOldLogs() stub for next phase. Rotation check is called from addLog() when file logging is enabled. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Implements the cleanOldLogs() private method on the Logger class that: - Reads the logs directory and filters for maestro-debug-YYYY-MM-DD.log files - Parses dates from filenames and calculates age in days - Deletes files older than 7 days with per-file error handling - Uses console.log for cleanup messages to avoid circular logging Co-Authored-By: Claude Opus 4.6 <[email protected]>
On startup, if a legacy maestro-debug.log exists, rename it to
maestro-debug-{mtime-date}.log using the file's modification time.
Skips rename if the target dated file already exists. Wrapped in
try/catch to avoid blocking startup on migration errors.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…Logging() enableFileLogging() now sets currentLogDate/logFilePath to today's values, calls cleanOldLogs() after opening the stream, and starts a 10-minute rotation timer with unref(). disableFileLogging() clears the timer. Legacy migration tests updated to use recent dates to avoid cleanOldLogs deleting test fixtures. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
📝 WalkthroughWalkthroughThe logger module now implements platform-aware file logging with daily, date-based log rotation, legacy maestro-debug.log migration, automatic cleanup of logs older than 7 days, and timer-based rotation checks; tests add comprehensive coverage for lifecycle, rotation, migration, cleanup, and edge cases. Changes
Sequence DiagramssequenceDiagram
participant App as Application
participant Logger as Logger Module
participant FS as File System
participant Timer as Timer Manager
App->>Logger: enableFileLogging()
Logger->>FS: Check for legacy `maestro-debug.log`
FS-->>Logger: Exists / Not exists
alt Legacy exists
Logger->>FS: Read mtime of legacy file
Logger->>FS: Rename -> `maestro-debug-YYYY-MM-DD.log`
end
Logger->>FS: Ensure logs directory exists
Logger->>FS: Open/Create today's dated log file
Logger->>FS: Write startup marker
Logger->>Logger: cleanOldLogs()
Logger->>FS: List files in logs directory
FS-->>Logger: File list
Logger->>FS: Delete files older than 7 days
Logger->>Timer: setInterval(rotateIfNeeded)
Timer-->>Logger: Timer active
sequenceDiagram
participant App as Application
participant Logger as Logger Module
participant FS as File System
participant Timer as Timer Manager
loop On write or periodic interval
Logger->>Logger: getTodayDateString()
Logger->>Logger: Compare to currentLogDate
alt Date changed
Logger->>FS: Close current stream
Logger->>FS: Open new dated log file
Logger->>FS: Write rotation marker
Logger->>Logger: cleanOldLogs()
else Same day
Logger->>FS: Write message to current stream
end
end
App->>Logger: disableFileLogging()
Logger->>Timer: clearInterval(timer)
Logger->>FS: Close log stream
Logger->>Logger: Drop timer reference
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR refactors the Key findings:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant Logger
participant FS
App->>Logger: enableFileLogging()
Logger->>FS: mkdirSync(logsDir)
Logger->>FS: existsSync(legacyPath)
alt legacy file exists && target doesn't
Logger->>FS: renameSync(legacy → dated)
end
Logger->>FS: createWriteStream(maestro-debug-YYYY-MM-DD.log)
Logger->>Logger: cleanOldLogs()
Logger->>Logger: setInterval(rotateIfNeeded, 10min).unref()
loop every addLog() call
App->>Logger: info/debug/warn/error(msg)
Logger->>Logger: rotateIfNeeded()
alt date changed
Logger->>FS: stream.end() [old file]
Logger->>FS: createWriteStream(new dated file)
Logger->>Logger: cleanOldLogs()
end
Logger->>FS: stream.write(message)
end
App->>Logger: disableFileLogging()
Logger->>Logger: clearInterval(rotationTimer)
Logger->>FS: stream.end()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+169
to
+201
| private rotateIfNeeded(): void { | ||
| try { | ||
| const todayDate = getTodayDateString(); | ||
| if (todayDate === this.currentLogDate) return; | ||
|
|
||
| // Close old stream if it exists | ||
| if (this.logFileStream) { | ||
| this.logFileStream.end(); | ||
| this.logFileStream = null; | ||
| } | ||
|
|
||
| // Update to today's log file | ||
| this.logFilePath = getLogFilePath(); | ||
| this.currentLogDate = todayDate; | ||
|
|
||
| // Ensure the logs directory exists | ||
| const logsDir = getLogsDir(); | ||
| if (!fs.existsSync(logsDir)) { | ||
| fs.mkdirSync(logsDir, { recursive: true }); | ||
| } | ||
|
|
||
| // Open new log file in append mode | ||
| this.logFileStream = fs.createWriteStream(this.logFilePath, { flags: 'a' }); | ||
|
|
||
| // Write startup marker | ||
| const startupMsg = `\n${'='.repeat(80)}\n[${new Date().toISOString()}] Maestro log rotated - new log file\nPlatform: ${process.platform}, Node: ${process.version}\nLog file: ${this.logFilePath}\n${'='.repeat(80)}\n`; | ||
| this.logFileStream.write(startupMsg); | ||
|
|
||
| // Clean up old log files | ||
| this.cleanOldLogs(); | ||
| } catch (error) { | ||
| console.error('[Logger] Failed to rotate log file:', error); | ||
| } |
There was a problem hiding this comment.
Silent log loss after rotation failure
When an error occurs inside rotateIfNeeded() after the old stream has been closed (line 176-178) but before the new stream is successfully opened, the catch block logs the error but leaves this.fileLogEnabled = true and this.logFileStream = null. Every subsequent call to addLog() will pass the if (this.fileLogEnabled) guard, call rotateIfNeeded() again (which will keep retrying and failing), and then be dropped silently by the if (this.fileLogEnabled && this.logFileStream) write guard. The result is that all logs are permanently silently discarded without any further indication to the user.
The fix should set this.fileLogEnabled = false in the catch block so the caller is aware that file logging is no longer operational:
} catch (error) {
console.error('[Logger] Failed to rotate log file:', error);
// Disable file logging so callers know logs are no longer being written to disk
this.fileLogEnabled = false;
this.logFileStream = null;
}
Comment on lines
+296
to
+299
| // Check if we need to rotate to a new day's log file | ||
| if (this.fileLogEnabled) { | ||
| this.rotateIfNeeded(); | ||
| } |
There was a problem hiding this comment.
Per-write rotation check makes the timer redundant
rotateIfNeeded() is called on every single call to addLog(). Inside it, getTodayDateString() allocates a new Date() and formats a string on every log write. This is a non-trivial overhead on the hot path, especially in high-volume logging scenarios.
More importantly, since every log message already triggers a date check, the 10-minute setInterval rotation timer (line 138) is effectively redundant — a rotation will happen on the very next log write after midnight regardless. The timer only helps if the app is running but emits zero log messages for 10+ consecutive minutes around midnight, which is an uncommon edge case.
Consider one of the following approaches:
- Keep per-write checks but remove the timer: The rotation will happen promptly on the next write. Add a comment explaining this.
- Keep only the timer and remove per-write checks: This is the conventional log-rotation model. Writes between midnight and the next timer tick (up to 10 min) will go to the previous day's file, which is acceptable.
- Cache the date string and only recompute it on the per-write check when the timer fires (or less frequently) to reduce overhead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/utils/logger.test.ts`:
- Around line 723-794: The tests are using the real logger singleton which
enables file logging on import, causing flakiness and filesystem changes; before
importing the module, stub the platform/log root environment (APPDATA or
XDG_CONFIG_HOME) and os.homedir() (or mock process.platform to force
non-Windows) so the logger uses a sandbox path, and ensure the logger starts
disabled by calling logger.disableFileLogging() in a beforeEach (or import a
disabled baseline) so subsequent calls to logger.enableFileLogging(),
logger.disableFileLogging(), and cleanOldLogs() exercise the intended code paths
without touching real user logs and so timer/interval spies on the logger
constructor are attached correctly.
In `@src/main/utils/logger.ts`:
- Around line 105-123: The migration currently skips the branch where targetPath
already exists, leaving legacyPath (maestro-debug.log) orphaned; update the
migration in the try block to explicitly handle the case when targetPath exists
by either appending the contents of legacyPath into targetPath and then deleting
legacyPath, or (simpler) deleting legacyPath if you prefer the dated file wins,
and ensure any file operations throwable are caught and logged (use legacyPath
and targetPath variables), and keep cleanOldLogs() behavior intact so orphans
are removed; preserve existing console.log/console.error behavior and reuse the
migrationError handling for errors.
- Around line 180-191: The code updates currentLogDate and logFilePath before
ensuring the new log stream is successfully created, so if
getLogsDir()/mkdirSync() or fs.createWriteStream throws the rotation state is
advanced but logFileStream remains null causing rotateIfNeeded() to return early
and break logging; fix by deferring assignment of currentLogDate/logFilePath
until after the new stream is fully opened (create the new stream into a temp
variable, ensure getLogsDir() and fs.createWriteStream succeed, then assign
this.logFileStream = newStream and only then set this.logFilePath and
this.currentLogDate), and ensure any failure leaves the old stream/state intact;
reference functions/vars: getLogsDir, getLogFilePath, this.logFileStream,
this.logFilePath, this.currentLogDate, rotateIfNeeded.
- Around line 213-226: The retention age calculation uses local midnights and
ms-difference which breaks across DST; change the math in the cleanup loop (the
block using todayMs, fileDate and computing ageInDays) to use UTC calendar-day
arithmetic instead: compute the midnight timestamps with Date.UTC(...) for today
and for the parsed file date (or otherwise compare year/month/day values), then
compute ageInDays as the difference in UTC-midnight days (divide the UTC-ms
difference by 24*60*60*1000). Update the ageInDays calculation where
logFilePattern, fileDate, todayMs and ageInDays are used so 7-day retention is
based on calendar days not wall-clock hours.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27160cb1-7db6-42ec-a0a0-6d0929e5607a
📒 Files selected for processing (2)
src/__tests__/main/utils/logger.test.tssrc/main/utils/logger.ts
- Fix silent log loss: disable file logging on rotation failure - Remove redundant 10-min rotation timer (per-write checks suffice) - Don't advance rotation state until new stream is ready - Delete orphaned legacy log file when dated target already exists - Use UTC calendar-day math for 7-day retention (DST-safe) - Add disableFileLogging() in beforeEach for file-logging test suites Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/main/utils/logger.test.ts (1)
750-793: Test operates on real filesystem.This test enables file logging against the actual platform log directory, which could interact with real Maestro logs during development. The cleanup in
finallymitigates this, but consider using a temporary directory or mocking the file system for true isolation. This is a known limitation from previous reviews.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/utils/logger.test.ts` around lines 750 - 793, The test currently writes to the real platform log directory; change it to use an isolated temp directory or a mocked filesystem: create a temporary directory (e.g., via fs.mkdtempSync(os.tmpdir() + '/maestro-test-')) before calling logger.enableFileLogging(), configure the logger to use that directory (by calling an existing setter like logger.setLogDirectory(...) or, if none exists, temporarily stub logger.getLogFilePath()/logger._logDirectory to point to the temp dir), run the rotation assertions using logger.getLogFilePath(), and in the finally block restore the original directory/stubs, call logger.disableFileLogging(), and remove the temp directory to fully isolate file I/O from real Maestro logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/main/utils/logger.test.ts`:
- Around line 750-793: The test currently writes to the real platform log
directory; change it to use an isolated temp directory or a mocked filesystem:
create a temporary directory (e.g., via fs.mkdtempSync(os.tmpdir() +
'/maestro-test-')) before calling logger.enableFileLogging(), configure the
logger to use that directory (by calling an existing setter like
logger.setLogDirectory(...) or, if none exists, temporarily stub
logger.getLogFilePath()/logger._logDirectory to point to the temp dir), run the
rotation assertions using logger.getLogFilePath(), and in the finally block
restore the original directory/stubs, call logger.disableFileLogging(), and
remove the temp directory to fully isolate file I/O from real Maestro logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c7913a8-327b-456e-9be0-cc7c3ea6f35f
📒 Files selected for processing (2)
src/__tests__/main/utils/logger.test.tssrc/main/utils/logger.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request updates the logging system to support daily log file rotation and automatic cleanup of old logs. The changes improve log organization, ensure logs are split by day, and help manage disk usage by removing logs older than a week. The most important changes are grouped below:
Log file rotation and organization
maestro-debug-YYYY-MM-DD.log), and updated the log directory path logic for platform-specific support. (getLogFilePath,getLogsDir,getTodayDateString) [1] [2]Logger.rotateIfNeeded,Logger.rotationTimer) [1] [2] [3]Log cleanup and migration
Logger.cleanOldLogs)maestro-debug.log) to the new date-based format on startup, ensuring continuity and consistency.Resource management
Logger.disableFileLogging)Summary by CodeRabbit
New Features
Tests