[duplicate-code] Duplicate Code Pattern: ServerFileLogger.Close() Reinvents closeLogFile Sync+Close Pattern

[github-actions]

🔍 Duplicate Code Pattern: ServerFileLogger.Close() Sync+Close Loop

Part of duplicate code analysis: #8170

Summary

internal/logger/server_file_logger.go's Close() method manually implements a multi-file Sync+Close loop with firstErr tracking, while the existing closeLogFile helper in internal/logger/common.go already encapsulates the per-file Sync+Close pattern. The per-file helper is unused inside Close(), leading to divergent implementations of the same Sync→log→Close sequence.

Duplication Details

Pattern: Repeated per-file Sync+Close with firstErr collection

• Severity: Medium
• Occurrences: 2 related instances
• Locations:
  • internal/logger/server_file_logger.go — ServerFileLogger.Close() (lines 105–134)
  • internal/logger/common.go — closeLogFile() (lines 351–365)

ServerFileLogger.Close() inner loop (server_file_logger.go:113-133):

var firstErr error
for serverID, file := range sfl.files {
    if err := file.Sync(); err != nil {
        log.Printf("WARNING: Failed to sync log file for server %s: %v", serverID, err)
        if firstErr == nil {
            firstErr = err
        }
    }
    if err := file.Close(); err != nil {
        log.Printf("WARNING: Failed to close log file for server %s: %v", serverID, err)
        if firstErr == nil {
            firstErr = err
        }
    }
}

Existing closeLogFile helper (common.go:351-365):

func closeLogFile(file *os.File, mu *sync.Mutex, loggerName string) error {
    if file == nil {
        return nil
    }
    if err := file.Sync(); err != nil {
        log.Printf("WARNING: Failed to sync log file for %s: %v", loggerName, err)
    }
    return file.Close()
}

The two diverge on sync-error handling: closeLogFile discards sync errors (logs only), while the Close() loop tracks sync errors in firstErr. This inconsistency may be unintentional.

Impact Analysis

• Maintainability: Adding a new step to file cleanup (e.g., chmod or rename) would need to be applied in both locations
• Bug Risk: The inconsistent sync-error tracking between the two implementations is a latent behavioral discrepancy — one suppresses sync errors, the other propagates them
• Code Bloat: ~15 lines of the Close() loop replicate logic already expressed in closeLogFile

Refactoring Recommendations

1. Extract syncAndCloseFile helper (align error handling first)

Decide on consistent sync-error behavior, then extract:

// syncAndCloseFile syncs and closes a single file, returning the first error encountered.
// Both sync and close errors are returned (unlike closeLogFile which discards sync errors).
func syncAndCloseFile(file *os.File, loggerName string) error {
    var firstErr error
    if err := file.Sync(); err != nil {
        log.Printf("WARNING: Failed to sync log file for %s: %v", loggerName, err)
        firstErr = err
    }
    if err := file.Close(); err != nil {
        log.Printf("WARNING: Failed to close log file for %s: %v", loggerName, err)
        if firstErr == nil {
            firstErr = err
        }
    }
    return firstErr
}

ServerFileLogger.Close() inner loop becomes:

var firstErr error
for serverID, file := range sfl.files {
    if err := syncAndCloseFile(file, "server:"+serverID); err != nil && firstErr == nil {
        firstErr = err
    }
}

2. (Alternative) Make closeLogFile track sync errors and use it in the loop

If sync-error propagation is desired, update closeLogFile to return both errors and reuse it.

Implementation Checklist

• Decide on canonical sync-error behavior (propagate vs. log-only)
• Extract syncAndCloseFile (or update closeLogFile) in common.go
• Refactor ServerFileLogger.Close() to use the shared helper
• Verify tests pass

Parent Issue

See parent analysis report: #8170
Related to #8170

Generated by Duplicate Code Detector · 936.3 AIC · ⊞ 35.7K · ◷

• expires on Jul 4, 2026, 3:46 AM UTC