[test] Add tests for logger.ServerFileLogger.Close (#3159)

## Test Coverage Improvement: `ServerFileLogger.Close`

### Function Analyzed

- **Package**: `internal/logger`
- **Function**: `ServerFileLogger.Close`
- **File**: `internal/logger/server_file_logger.go:108`
- **Previous Coverage**: 58.8%
- **New Coverage**: 94.1%
- **Complexity**: High (nil-receiver guard, loop over file map,
sync+close error handling with first-error-wins tracking)

### Why This Function?

`ServerFileLogger.Close` had the lowest real test coverage (58.8%) among
non-env-gated functions in the testable subset of the codebase. Its
complexity comes from:

- Nil-receiver guard (callable on `nil *ServerFileLogger`)
- Iterating a map of open log files, running two operations per file
- First-error-wins tracking: `firstErr` captures the first Sync or Close
failure without being overwritten by later errors
- Unconditional map clearing even when errors occur

All of these paths except the "Sync succeeds but Close fails" branch
(which requires NFS-level failure to trigger) are now exercised.

### Tests Added

| Test | Path Covered |
|------|--------------|
| `TestServerFileLoggerClose_NilReceiver` | Nil-receiver guard in
`Close()` |
| `TestServerFileLoggerClose_EmptyFiles` | Loop body never entered; maps
are still reset |
| `TestServerFileLoggerClose_SyncError` | `file.Sync()` error →
`firstErr` set |
| `TestServerFileLoggerClose_FirstErrorTracking` | Multiple files both
failing; first-error-wins over multiple iterations |
| `TestServerFileLoggerClose_ValidFiles` | Happy-path close (all files
flush and close cleanly) |
| `TestServerFileLoggerClose_CloseTwice` | Idempotent: second call is a
no-op with empty maps |
| `TestServerFileLoggerLog_NilReceiver` | Nil-receiver guard in `Log()`
|
| `TestServerFileLoggerLog_SyncError` | `file.Sync()` error inside
`Log()` does not panic |
| `TestServerFileLoggerGetOrCreate_FileCreationError` | `os.OpenFile`
failure in `getOrCreateLogger()` gracefully falls back to `LogDebug` |

### Coverage Report

````
Before:
  Close             58.8%
  Log               84.6%
  getOrCreateLogger 90.9%

After:
  Close             94.1%   (+35.3 pp)
  Log              100.0%   (+15.4 pp)
  getOrCreateLogger 100.0%   (+9.1 pp)
```

Overall `internal/logger` package: **88.8% → 90.7%**

### Test Execution

```
=== RUN   TestServerFileLoggerClose_NilReceiver
--- PASS: TestServerFileLoggerClose_NilReceiver (0.00s)
=== RUN   TestServerFileLoggerClose_EmptyFiles
--- PASS: TestServerFileLoggerClose_EmptyFiles (0.00s)
=== RUN   TestServerFileLoggerClose_SyncError
--- PASS: TestServerFileLoggerClose_SyncError (0.00s)
=== RUN   TestServerFileLoggerClose_FirstErrorTracking
--- PASS: TestServerFileLoggerClose_FirstErrorTracking (0.00s)
=== RUN   TestServerFileLoggerClose_ValidFiles
--- PASS: TestServerFileLoggerClose_ValidFiles (0.00s)
=== RUN   TestServerFileLoggerClose_CloseTwice
--- PASS: TestServerFileLoggerClose_CloseTwice (0.00s)
=== RUN   TestServerFileLoggerLog_NilReceiver
--- PASS: TestServerFileLoggerLog_NilReceiver (0.00s)
=== RUN   TestServerFileLoggerLog_SyncError
--- PASS: TestServerFileLoggerLog_SyncError (0.00s)
=== RUN   TestServerFileLoggerGetOrCreate_FileCreationError
--- PASS: TestServerFileLoggerGetOrCreate_FileCreationError (0.00s)
PASS
ok      github.com/github/gh-aw-mcpg/internal/logger   0.006s
````

All existing tests continue to pass.

---
*Generated by Test Coverage Improver*  
*Next run should target `writeToFile` (63.6%) or `logEntry` (66.7%)*




> Generated by [Test Coverage
Improver](https://github.com/github/gh-aw-mcpg/actions/runs/23978202716/agentic_workflow)
·
[◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-coverage-improver%22&type=pullrequests)

<!-- gh-aw-agentic-workflow: Test Coverage Improver, engine: copilot,
model: auto, id: 23978202716, workflow_id: test-coverage-improver, run:
https://github.com/github/gh-aw-mcpg/actions/runs/23978202716 -->

<!-- gh-aw-workflow-id: test-coverage-improver -->

Author: lpcox

internal/logger/server_file_logger_test.go

@@ -1,6 +1,7 @@
 package logger
 
 import (
+	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -263,3 +264,203 @@ func TestServerFileLoggerPreservesUnifiedView(t *testing.T) {
 	lines := strings.Split(strings.TrimSpace(string(unifiedContent)), "\n")
 	assert.GreaterOrEqual(t, len(lines), 3, "unified log should have at least 3 messages")
 }
+
+// TestServerFileLoggerClose_NilReceiver verifies that Close() handles nil receiver gracefully.
+func TestServerFileLoggerClose_NilReceiver(t *testing.T) {
+	var sfl *ServerFileLogger
+	err := sfl.Close()
+	assert.NoError(t, err, "Close() on nil receiver should return nil without panicking")
+}
+
+// TestServerFileLoggerClose_EmptyFiles verifies that Close() succeeds when no files are open.
+func TestServerFileLoggerClose_EmptyFiles(t *testing.T) {
+	sfl := &ServerFileLogger{
+		loggers: make(map[string]*log.Logger),
+		files:   make(map[string]*os.File),
+	}
+
+	err := sfl.Close()
+
+	assert.NoError(t, err, "Close() with no open files should return nil")
+	assert.Empty(t, sfl.loggers, "loggers map should be cleared after Close()")
+	assert.Empty(t, sfl.files, "files map should be cleared after Close()")
+}
+
+// TestServerFileLoggerClose_SyncError verifies that Close() returns the first error when file.Sync() fails.
+// This covers the sync error path and firstErr tracking within the Close() loop.
+func TestServerFileLoggerClose_SyncError(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+
+	tmpDir := t.TempDir()
+
+	// Create a real file, then close it to invalidate the descriptor.
+	// Subsequent Sync() and Close() calls on the *os.File will return an error.
+	f, err := os.CreateTemp(tmpDir, "test-server-*.log")
+	require.NoError(err, "CreateTemp should succeed")
+	f.Close() // Close to invalidate the file descriptor
+
+	sfl := &ServerFileLogger{
+		loggers: map[string]*log.Logger{
+			"server1": log.New(f, "", 0),
+		},
+		files: map[string]*os.File{
+			"server1": f,
+		},
+	}
+
+	closeErr := sfl.Close()
+
+	// Sync() on a closed file descriptor should fail, so Close() must return an error.
+	assert.Error(closeErr, "Close() should return an error when Sync() fails on an invalidated file")
+
+	// Maps must be cleared even when errors occur.
+	assert.Empty(sfl.loggers, "loggers map should be cleared after Close() even on error")
+	assert.Empty(sfl.files, "files map should be cleared after Close() even on error")
+}
+
+// TestServerFileLoggerClose_FirstErrorTracking verifies that Close() returns the first error encountered
+// and does not overwrite it with subsequent errors when multiple files fail.
+func TestServerFileLoggerClose_FirstErrorTracking(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+
+	tmpDir := t.TempDir()
+
+	// Create two files and close both to make all operations fail.
+	f1, err := os.CreateTemp(tmpDir, "test-server1-*.log")
+	require.NoError(err)
+	f1.Close()
+
+	f2, err := os.CreateTemp(tmpDir, "test-server2-*.log")
+	require.NoError(err)
+	f2.Close()
+
+	sfl := &ServerFileLogger{
+		loggers: map[string]*log.Logger{
+			"server1": log.New(f1, "", 0),
+			"server2": log.New(f2, "", 0),
+		},
+		files: map[string]*os.File{
+			"server1": f1,
+			"server2": f2,
+		},
+	}
+
+	closeErr := sfl.Close()
+
+	// At least one error should be returned; the first one wins.
+	assert.Error(closeErr, "Close() should return an error when files have already been closed")
+
+	// Maps must be cleared regardless.
+	assert.Empty(sfl.loggers, "loggers map should be cleared after Close()")
+	assert.Empty(sfl.files, "files map should be cleared after Close()")
+}
+
+// TestServerFileLoggerClose_ValidFiles verifies that Close() returns nil when all files close successfully.
+func TestServerFileLoggerClose_ValidFiles(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+
+	tmpDir := t.TempDir()
+
+	// Create two valid open files.
+	f1, err := os.OpenFile(filepath.Join(tmpDir, "server1.log"), os.O_CREATE|os.O_WRONLY, 0644)
+	require.NoError(err)
+
+	f2, err := os.OpenFile(filepath.Join(tmpDir, "server2.log"), os.O_CREATE|os.O_WRONLY, 0644)
+	require.NoError(err)
+
+	sfl := &ServerFileLogger{
+		loggers: map[string]*log.Logger{
+			"server1": log.New(f1, "", 0),
+			"server2": log.New(f2, "", 0),
+		},
+		files: map[string]*os.File{
+			"server1": f1,
+			"server2": f2,
+		},
+	}
+
+	closeErr := sfl.Close()
+
+	assert.NoError(closeErr, "Close() should return nil when all files close successfully")
+	assert.Empty(sfl.loggers, "loggers map should be cleared after Close()")
+	assert.Empty(sfl.files, "files map should be cleared after Close()")
+}
+
+// TestServerFileLoggerClose_CloseTwice verifies that calling Close() twice does not panic.
+// After the first Close(), maps are empty so the second Close() is a no-op.
+func TestServerFileLoggerClose_CloseTwice(t *testing.T) {
+	assert := assert.New(t)
+
+	sfl := &ServerFileLogger{
+		loggers: make(map[string]*log.Logger),
+		files:   make(map[string]*os.File),
+	}
+
+	err1 := sfl.Close()
+	err2 := sfl.Close()
+
+	assert.NoError(err1, "First Close() should return nil")
+	assert.NoError(err2, "Second Close() should return nil (no-op)")
+}
+
+// TestServerFileLoggerLog_NilReceiver verifies that Log() on a nil ServerFileLogger does not panic.
+func TestServerFileLoggerLog_NilReceiver(t *testing.T) {
+	var sfl *ServerFileLogger
+	assert.NotPanics(t, func() {
+		sfl.Log("server1", LogLevelInfo, "test", "message")
+	}, "Log() on nil receiver should not panic")
+}
+
+// TestServerFileLoggerLog_SyncError verifies that Log() handles file sync failures gracefully.
+// This covers the file.Sync() error path inside the Log() method.
+func TestServerFileLoggerLog_SyncError(t *testing.T) {
+	require := require.New(t)
+
+	tmpDir := t.TempDir()
+
+	// Create a file then close it to simulate a broken file descriptor.
+	f, err := os.CreateTemp(tmpDir, "test-sync-*.log")
+	require.NoError(err)
+	f.Close()
+
+	sfl := &ServerFileLogger{
+		logDir: tmpDir,
+		loggers: map[string]*log.Logger{
+			"server1": log.New(f, "", 0),
+		},
+		files: map[string]*os.File{
+			"server1": f,
+		},
+	}
+
+	// Log() should not panic even when Sync() fails internally.
+	assert.NotPanics(t, func() {
+		sfl.Log("server1", LogLevelInfo, "test", "message after close")
+	}, "Log() should not panic when file sync fails")
+}
+
+// TestServerFileLoggerGetOrCreate_FileCreationError verifies that getOrCreateLogger handles
+// file creation failures gracefully, falling back to the debug logger.
+func TestServerFileLoggerGetOrCreate_FileCreationError(t *testing.T) {
+	// Use a non-existent non-writable path to force os.OpenFile to fail.
+	sfl := &ServerFileLogger{
+		logDir:      "/nonexistent/path/that/does/not/exist",
+		loggers:     make(map[string]*log.Logger),
+		files:       make(map[string]*os.File),
+		useFallback: false, // not in fallback mode; will attempt to create files
+	}
+
+	// Log() should not panic. It falls back to LogDebug when file creation fails.
+	assert.NotPanics(t, func() {
+		sfl.Log("server1", LogLevelInfo, "test", "message")
+	}, "Log() should not panic when file creation fails")
+
+	// Since getOrCreateLogger returned an error, the file should not be in the map.
+	sfl.mu.RLock()
+	_, exists := sfl.files["server1"]
+	sfl.mu.RUnlock()
+	assert.False(t, exists, "files map should not contain server1 after creation failure")
+}