feat(logging): rotate diagnostic logs by size and time#738
Open
nk-tedo-001 wants to merge 1 commit into
Open
Conversation
Add automatic rotation of the ~/Library/Logs/Thaw diagnostic logs so a single file stays under GitHub's 25 MB upload limit. Rotation triggers on file size (default 10 MB) and optionally on a time interval (off/hourly/ daily). Retention switches from a fixed file count to an age-based policy (default 2 days) plus a count cap. The main app owns rotation; on each rotation it re-points the XPC service to the new segment over the existing configureLogging channel, whose path is now optional (nil disables logging, which also fixes toggling logging after launch). All file-descriptor operations are serialized on the write queue, the new file is opened before the old one is closed, and log files are created with 0o600 permissions and O_NOFOLLOW. New settings (max file size, retention days, rotation interval) are exposed in Advanced → Diagnostics and carried in profiles and settings reset. Closes stonerl#732
Contributor
📝 WalkthroughWalkthroughAdds a size-based and time-based log rotation system to ChangesDiagnostic Log Rotation
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over AdvancedSettings,DiagnosticLogger: Settings change triggers rotation policy update
AdvancedSettings->>DiagnosticLogger: setRotationPolicy(RotationPolicy)
DiagnosticLogger->>DiagnosticLogger: rescheduleMaintenanceTimer()
end
rect rgba(144, 238, 144, 0.5)
note over DiagnosticLogger,MenuBarItemService: Rotation event propagates across XPC boundary
DiagnosticLogger->>AppState: onRotate(newURL)
AppState->>MenuBarItemServiceConnection: configureLogging(to: newURL.path)
MenuBarItemServiceConnection->>MenuBarItemServiceConnection: update LoggingState (generation++)
MenuBarItemServiceConnection->>MenuBarItemService XPC: configureLogging(filePath: newPath)
MenuBarItemService XPC-->>MenuBarItemServiceConnection: response
MenuBarItemServiceConnection->>MenuBarItemServiceConnection: clear dirty if generation matches
end
rect rgba(255, 215, 0, 0.5)
note over MenuBarItemServiceConnection,DiagnosticLogger: Retry on next sourcePIDs call
MenuBarItemServiceConnection->>MenuBarItemServiceConnection: resendLoggingIfDirty()
MenuBarItemServiceConnection->>MenuBarItemService XPC: configureLogging(filePath: desiredPath)
MenuBarItemService XPC->>DiagnosticLogger: attachToFile(at: URL)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Shared/Utilities/DiagnosticLogger.swift`:
- Around line 28-39: The current implementation has a race condition where
concurrent assignments to isEnabled can lose the latest toggle intent. The
snapshot read at the beginning of the setter happens before openLogFile()
completes its work, so if isEnabled=true is in-flight and isEnabled=false
arrives concurrently, the disable path may return early based on the stale
snapshot while the enable operation publishes its state afterwards. Serialize
both the enable and disable paths (the branches that call openLogFile() and
closeLogFile()) onto a single serialized queue such as writeQueue to ensure that
the last assignment always wins and the final state reflects the most recent
intent.
In `@Thaw/Settings/Models/AdvancedSettings.swift`:
- Around line 245-248: In the sink block where maxFileSizeBytes is calculated,
clamp the maxSizeMB value to a safe maximum before performing the multiplication
by 1024 * 1024. Instead of converting and multiplying the raw maxSizeMB value
directly, first constrain it to a reasonable upper limit (such as 100 to match
UI constraints) using the max function, then cast to UInt64 and perform the
multiplication. This prevents integer overflow when persisted or tampered
profile data contains out-of-range values that bypass UI validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56c338cf-59ff-42cb-8da5-426656aff964
📒 Files selected for processing (11)
MenuBarItemService/Listener.swiftShared/Services/MenuBarItemService.swiftShared/Utilities/DiagnosticLogger.swiftThaw/Main/AppState.swiftThaw/MenuBar/MenuBarItems/MenuBarItemServiceConnection.swiftThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/SettingsResetter.swiftThaw/Settings/SettingsPanes/AdvancedSettingsPane.swiftThaw/Utilities/Defaults.swiftThawTests/DiagnosticLoggerTests.swift
Comment on lines
+28
to
39
| let wasEnabled = isEnabledLock.withLock { $0 } | ||
| guard newValue != wasEnabled else { return } | ||
| if newValue { | ||
| // `openLogFile()` flips `isEnabled` to true only after the file | ||
| // handle is installed, so a concurrent `log()` never sees | ||
| // enabled with no handle. | ||
| openLogFile() | ||
| } else if !newValue, oldValue { | ||
| } else { | ||
| // Stop new writes before closing so no line lands after the footer. | ||
| isEnabledLock.withLock { $0 = false } | ||
| closeLogFile() | ||
| } |
Contributor
There was a problem hiding this comment.
Serialize isEnabled transitions so the latest toggle cannot be lost.
At Line 29, the equality short-circuit uses a snapshot read before openLogFile() publishes enabled state. If isEnabled = true is in-flight and a concurrent isEnabled = false arrives, the disable path can return early (still seeing false) and logging ends up enabled after the open completes. Reconcile enable/disable on a single serialized path (e.g., writeQueue) so last-write intent wins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Shared/Utilities/DiagnosticLogger.swift` around lines 28 - 39, The current
implementation has a race condition where concurrent assignments to isEnabled
can lose the latest toggle intent. The snapshot read at the beginning of the
setter happens before openLogFile() completes its work, so if isEnabled=true is
in-flight and isEnabled=false arrives concurrently, the disable path may return
early based on the stale snapshot while the enable operation publishes its state
afterwards. Serialize both the enable and disable paths (the branches that call
openLogFile() and closeLogFile()) onto a single serialized queue such as
writeQueue to ensure that the last assignment always wins and the final state
reflects the most recent intent.
Comment on lines
+245
to
+248
| .sink { maxSizeMB, retentionDays, interval in | ||
| var policy = DiagnosticLogger.RotationPolicy() | ||
| policy.maxFileSizeBytes = UInt64(max(0, maxSizeMB)) * 1024 * 1024 | ||
| policy.retentionDays = max(1, retentionDays) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current bounds/sanitization path for diagnosticLogMaxSizeMB.
rg -n 'diagnosticLogMaxSizeMB|maxFileSizeBytes|CombineLatest3' Thaw/Settings/Models/AdvancedSettings.swift -C3
rg -n 'diagnosticLogMaxSizeMB' Thaw/Settings/Models/Profile.swift Thaw/Settings/SettingsPanes/AdvancedSettingsPane.swift -C2Repository: stonerl/Thaw
Length of output: 7302
🏁 Script executed:
# Find DiagnosticLogger and check RotationPolicy definition
fd -t f DiagnosticLogger -e swift | head -5
rg -n "class RotationPolicy|struct RotationPolicy" -A 10Repository: stonerl/Thaw
Length of output: 1176
🏁 Script executed:
# Check if there's any validation in the Defaults system for diagnosticLogMaxSizeMB
rg -n "DefaultValue.diagnosticLogMaxSizeMB|.diagnosticLogMaxSizeMB.*case" -B 2 -A 2Repository: stonerl/Thaw
Length of output: 2953
Clamp before multiplication to prevent overflow in maxFileSizeBytes derivation.
At line 247, UInt64(max(0, maxSizeMB)) * 1024 * 1024 can overflow when maxSizeMB contains out-of-range values from persisted or tampered profiles, potentially crashing during policy recomputation. While the UI constrains input to 1–100 MB, the persisted Int value bypasses this constraint and reaches the sink without validation.
Proposed fix
.sink { maxSizeMB, retentionDays, interval in
var policy = DiagnosticLogger.RotationPolicy()
- policy.maxFileSizeBytes = UInt64(max(0, maxSizeMB)) * 1024 * 1024
+ let bytesPerMB: UInt64 = 1_048_576
+ let normalizedMB = max(1, maxSizeMB)
+ let clampedMB = min(UInt64(normalizedMB), UInt64.max / bytesPerMB)
+ policy.maxFileSizeBytes = clampedMB * bytesPerMB
policy.retentionDays = max(1, retentionDays)
policy.rotationInterval = interval.seconds
DiagnosticLogger.shared.setRotationPolicy(policy)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .sink { maxSizeMB, retentionDays, interval in | |
| var policy = DiagnosticLogger.RotationPolicy() | |
| policy.maxFileSizeBytes = UInt64(max(0, maxSizeMB)) * 1024 * 1024 | |
| policy.retentionDays = max(1, retentionDays) | |
| .sink { maxSizeMB, retentionDays, interval in | |
| var policy = DiagnosticLogger.RotationPolicy() | |
| let bytesPerMB: UInt64 = 1_048_576 | |
| let normalizedMB = max(1, maxSizeMB) | |
| let clampedMB = min(UInt64(normalizedMB), UInt64.max / bytesPerMB) | |
| policy.maxFileSizeBytes = clampedMB * bytesPerMB | |
| policy.retentionDays = max(1, retentionDays) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/Settings/Models/AdvancedSettings.swift` around lines 245 - 248, In the
sink block where maxFileSizeBytes is calculated, clamp the maxSizeMB value to a
safe maximum before performing the multiplication by 1024 * 1024. Instead of
converting and multiplying the raw maxSizeMB value directly, first constrain it
to a reasonable upper limit (such as 100 to match UI constraints) using the max
function, then cast to UInt64 and perform the multiplication. This prevents
integer overflow when persisted or tampered profile data contains out-of-range
values that bypass UI validation.
Author
|
I'm happy to fix the AI review if the code owners approve it. |
Collaborator
|
Please do @nk-tedo-001 |
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.
Summary
Add automatic rotation of the ~/Library/Logs/Thaw diagnostic logs so a single file stays under GitHub's 25 MB upload limit. Rotation triggers on file size (default 10 MB) and optionally on a time interval (off/hourly/ daily).
Retention switches from a fixed file count to an age-based policy (default 2 days) plus a count cap.
The main app owns rotation; on each rotation it re-points the XPC service to the new segment over the existing configureLogging channel, whose path is now optional (nil disables logging, which also fixes toggling logging after launch).
All file-descriptor operations are serialized on the write queue, the new file is opened before the old one is closed, and log files are created with
0o600permissions andO_NOFOLLOW.New settings (max file size, retention days, rotation interval) are exposed in Advanced → Diagnostics and carried in profiles and settings reset.
Closes #732
fstat) plus aperiodic owner-side poll that also catches growth driven solely by the XPC
service.
(default 2 days) plus a file-count cap; never deletes the current or the
just-rotated segment.
configureLogging(filePath:)is now optional — the main app(sole rotation owner) re-points the service to each new segment, and
nildisables logging (also fixes toggling logging after launch).
new file opened before the old is closed, log files created
0o600withO_NOFOLLOW.PR Type
Does this PR introduce a breaking change?
What is the new behavior?
Add automatic rotation of the ~/Library/Logs/Thaw diagnostic logs so a single file stays under GitHub's 25 MB upload limit.
PR Checklist
swiftformat .to keep the code style consistent.Other information
Screenshots, notes, or anything useful for reviewers.
Summary by CodeRabbit
New Features
Tests