refactor(logger): extract syncFileWithWarning to eliminate duplicated sync boilerplate#8504
Merged
Conversation
Closed
4 tasks
…licate sync boilerplate
Copilot
AI
changed the title
[WIP] Refactor duplicate logging code in FileLogger and ServerFileLogger
refactor(logger): extract syncFileWithWarning to eliminate duplicated sync boilerplate
Jul 2, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the logger package to remove duplicated “sync log file + warn on failure” boilerplate by introducing a shared helper used by both the unified file logger and the per-server file logger.
Changes:
- Added
syncFileWithWarning(file *os.File, context string)helper to centralize file sync + warning emission and to nil-guard missing files. - Updated
FileLogger.Log()to use the helper instead of inlining the sync/warn block. - Updated
ServerFileLogger.Log()to use the helper, preserving the existing per-server warning context.
Show a summary per file
| File | Description |
|---|---|
| internal/logger/global_helpers.go | Adds the shared syncFileWithWarning helper (nil-guard + sync + stderr warning) to centralize behavior. |
| internal/logger/file_logger.go | Replaces inline sync/warn logic with syncFileWithWarning(fl.logFile, ""). |
| internal/logger/server_file_logger.go | Replaces inline sync/warn logic with syncFileWithWarning(sfl.files[serverID], " for server "+serverID). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
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
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.
FileLogger.Log()andServerFileLogger.Log()both inlined the same three-step pattern: callfile.Sync(), and on failure emitlog.Printf("WARNING: Failed to sync log file..."). Any future change to sync behavior (throttling, retry, warning format) had to be applied twice.Changes
internal/logger/global_helpers.go— addssyncFileWithWarning(file *os.File, context string): nil-guards the file, syncs it, and logs a warning to stderr on failure. Thecontextparameter (e.g." for server github") lets both call sites produce their original distinct warning messages.internal/logger/file_logger.go— replaces the inlineif logFile != nil { Sync/warn }block withsyncFileWithWarning(fl.logFile, "").internal/logger/server_file_logger.go— replaces the inlineif file, exists := sfl.files[serverID]; exists { Sync/warn }block withsyncFileWithWarning(sfl.files[serverID], " for server "+serverID). Go's zero-value map lookup (nilfor a missing*os.File) is handled by the helper's nil guard, preserving identical behaviour.