logger: address CloseAllLoggers review comments — doc fix + tests

internal/logger/file_logger.go

@@ -10,9 +10,9 @@ import (
 
 // FileLogger manages logging to a file with fallback to stdout
 type FileLogger struct {
+	lockable
 	logFile     *os.File
 	logger      *log.Logger
-	mu          sync.Mutex
 	logDir      string
 	fileName    string
 	useFallback bool
@@ -56,12 +56,6 @@ func InitFileLogger(logDir, fileName string) error {
 	return err
 }
 
-// withLock acquires fl.mu, executes fn, then releases fl.mu.
-// Use this in methods that return an error to avoid repeating the lock/unlock preamble.
-func (fl *FileLogger) withLock(fn func() error) error {
-	return withMutexLock(&fl.mu, fn)
-}
-
 // Close closes the log file
 func (fl *FileLogger) Close() error {
 	return fl.withLock(func() error {

internal/logger/global_helpers.go

@@ -10,15 +10,38 @@
 // - init*: Initialize a global logger with proper locking and cleanup of any existing logger
 // - close*: Close and clear a global logger with proper locking
 //
-// These helpers are used internally by the logger package and should not be called
-// directly by external code. Use the public Init* and Close* functions instead.
+// The unexported helpers (withMutexLock, withGlobalLogger, initGlobalLogger,
+// closeGlobalLogger) are used internally by the logger package and should not be
+// called directly by external code. Use the public Init* and Close* functions instead.
+// CloseAllLoggers is the public entry point for closing all global loggers at once.
 package logger
 
 import "sync"
 
+// lockable provides a mutex and a withLock helper method. Embed this struct
+// in logger types that need a sync.Mutex plus a withLock convenience method
+// to eliminate the repeated per-type withLock boilerplate.
+//
+// Usage:
+//
+//	type MyLogger struct {
+//	    lockable
+//	    // other fields...
+//	}
+//
+// The embedded withLock method is promoted, so code in this package can write
+// myLogger.withLock(fn) directly. The embedded mu field is also promoted, but
+// because it is unexported it is only directly accessible within the logger package.
+type lockable struct {
+	mu sync.Mutex
+}
+
+// withLock acquires l.mu, executes fn, then releases l.mu.
+func (l *lockable) withLock(fn func() error) error {
+	return withMutexLock(&l.mu, fn)
+}
+
 // withMutexLock acquires mu, calls fn, and releases mu.
-// This is the single implementation of the per-type withLock pattern
-// used by FileLogger, JSONLLogger, MarkdownLogger, and ToolsLogger.
 func withMutexLock(mu *sync.Mutex, fn func() error) error {
 	mu.Lock()
 	defer mu.Unlock()
@@ -122,3 +145,22 @@ func closeGlobalLogger[T closableLogger](mu *sync.RWMutex, logger *T) error {
 	}
 	return nil
 }
+
+// CloseAllLoggers closes all global loggers in a single call.
+// Returns the first error encountered, but attempts to close every logger.
+func CloseAllLoggers() error {
+	closers := []func() error{
+		CloseGlobalLogger,
+		CloseJSONLLogger,
+		CloseMarkdownLogger,
+		CloseToolsLogger,
+		CloseServerFileLogger,
+	}
+	var firstErr error
+	for _, fn := range closers {
+		if err := fn(); err != nil && firstErr == nil {
+			firstErr = err
+		}
+	}
+	return firstErr
+}

internal/logger/global_helpers_test.go

@@ -0,0 +1,158 @@
+package logger
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// resetAllGlobalLoggers resets all global logger pointers to nil for test isolation.
+// It acquires each logger's mutex before resetting to avoid races.
+func resetAllGlobalLoggers(t *testing.T) {
+	t.Helper()
+	globalLoggerMu.Lock()
+	globalFileLogger = nil
+	globalLoggerMu.Unlock()
+
+	globalJSONLMu.Lock()
+	globalJSONLLogger = nil
+	globalJSONLMu.Unlock()
+
+	globalMarkdownMu.Lock()
+	globalMarkdownLogger = nil
+	globalMarkdownMu.Unlock()
+
+	globalToolsMu.Lock()
+	globalToolsLogger = nil
+	globalToolsMu.Unlock()
+
+	globalServerLoggerMu.Lock()
+	globalServerFileLogger = nil
+	globalServerLoggerMu.Unlock()
+}
+
+// TestCloseAllLoggers_NoLoggersInitialized verifies that CloseAllLoggers returns nil
+// when no loggers are currently initialized.
+func TestCloseAllLoggers_NoLoggersInitialized(t *testing.T) {
+	resetAllGlobalLoggers(t)
+	t.Cleanup(func() { resetAllGlobalLoggers(t) })
+
+	err := CloseAllLoggers()
+	assert.NoError(t, err)
+}
+
+// TestCloseAllLoggers_AllSucceed verifies that CloseAllLoggers returns nil and
+// clears all global logger pointers when all loggers close without error.
+func TestCloseAllLoggers_AllSucceed(t *testing.T) {
+	resetAllGlobalLoggers(t)
+	t.Cleanup(func() { resetAllGlobalLoggers(t) })
+
+	tmpDir := t.TempDir()
+	require.NoError(t, InitFileLogger(tmpDir, "test.log"))
+	require.NoError(t, InitJSONLLogger(tmpDir, "test.jsonl"))
+	require.NoError(t, InitMarkdownLogger(tmpDir, "test.md"))
+	require.NoError(t, InitToolsLogger(tmpDir, "tools.json"))
+	require.NoError(t, InitServerFileLogger(tmpDir))
+
+	err := CloseAllLoggers()
+	assert.NoError(t, err)
+
+	globalLoggerMu.RLock()
+	assert.Nil(t, globalFileLogger, "FileLogger should be nil after CloseAllLoggers")
+	globalLoggerMu.RUnlock()
+
+	globalJSONLMu.RLock()
+	assert.Nil(t, globalJSONLLogger, "JSONLLogger should be nil after CloseAllLoggers")
+	globalJSONLMu.RUnlock()
+
+	globalMarkdownMu.RLock()
+	assert.Nil(t, globalMarkdownLogger, "MarkdownLogger should be nil after CloseAllLoggers")
+	globalMarkdownMu.RUnlock()
+
+	globalToolsMu.RLock()
+	assert.Nil(t, globalToolsLogger, "ToolsLogger should be nil after CloseAllLoggers")
+	globalToolsMu.RUnlock()
+
+	globalServerLoggerMu.RLock()
+	assert.Nil(t, globalServerFileLogger, "ServerFileLogger should be nil after CloseAllLoggers")
+	globalServerLoggerMu.RUnlock()
+}
+
+// TestCloseAllLoggers_AllCalledEvenIfEarlyFails verifies that CloseAllLoggers
+// invokes every CloseXxx function even when an earlier one returns an error.
+func TestCloseAllLoggers_AllCalledEvenIfEarlyFails(t *testing.T) {
+	resetAllGlobalLoggers(t)
+	t.Cleanup(func() { resetAllGlobalLoggers(t) })
+
+	tmpDir := t.TempDir()
+	require.NoError(t, InitFileLogger(tmpDir, "test.log"))
+	require.NoError(t, InitJSONLLogger(tmpDir, "test.jsonl"))
+	require.NoError(t, InitMarkdownLogger(tmpDir, "test.md"))
+	require.NoError(t, InitToolsLogger(tmpDir, "tools.json"))
+	require.NoError(t, InitServerFileLogger(tmpDir))
+
+	// Force CloseGlobalLogger (the first closer) to fail by pre-closing its
+	// underlying file.  The FileLogger.Close() will then return an error when
+	// it tries to close an already-closed file descriptor.
+	globalLoggerMu.Lock()
+	_ = globalFileLogger.logFile.Close()
+	globalLoggerMu.Unlock()
+
+	err := CloseAllLoggers()
+	assert.Error(t, err, "CloseAllLoggers should return an error when a closer fails")
+
+	// All loggers must be nil: every closer was attempted, not just the first one.
+	globalLoggerMu.RLock()
+	assert.Nil(t, globalFileLogger, "FileLogger should be nil after CloseAllLoggers")
+	globalLoggerMu.RUnlock()
+
+	globalJSONLMu.RLock()
+	assert.Nil(t, globalJSONLLogger, "JSONLLogger should be nil after CloseAllLoggers")
+	globalJSONLMu.RUnlock()
+
+	globalMarkdownMu.RLock()
+	assert.Nil(t, globalMarkdownLogger, "MarkdownLogger should be nil after CloseAllLoggers")
+	globalMarkdownMu.RUnlock()
+
+	globalToolsMu.RLock()
+	assert.Nil(t, globalToolsLogger, "ToolsLogger should be nil after CloseAllLoggers")
+	globalToolsMu.RUnlock()
+
+	globalServerLoggerMu.RLock()
+	assert.Nil(t, globalServerFileLogger, "ServerFileLogger should be nil after CloseAllLoggers")
+	globalServerLoggerMu.RUnlock()
+}
+
+// TestCloseAllLoggers_FirstErrorIsReturned verifies that when multiple closers fail,
+// CloseAllLoggers returns only the first error encountered.
+func TestCloseAllLoggers_FirstErrorIsReturned(t *testing.T) {
+	resetAllGlobalLoggers(t)
+	t.Cleanup(func() { resetAllGlobalLoggers(t) })
+
+	firstLogDir := filepath.Join(t.TempDir(), "first")
+	secondLogDir := filepath.Join(t.TempDir(), "second")
+
+	// Initialize the first two closers (FileLogger and JSONLLogger) in distinct
+	// directories so their errors contain distinguishable file paths.
+	require.NoError(t, InitFileLogger(firstLogDir, "test.log"))
+	require.NoError(t, InitJSONLLogger(secondLogDir, "test.jsonl"))
+
+	// Pre-close both underlying files so both closers will return errors.
+	globalLoggerMu.Lock()
+	_ = globalFileLogger.logFile.Close()
+	globalLoggerMu.Unlock()
+
+	globalJSONLMu.Lock()
+	_ = globalJSONLLogger.logFile.Close()
+	globalJSONLMu.Unlock()
+
+	err := CloseAllLoggers()
+	require.Error(t, err)
+
+	// The returned error must come from the first closer (FileLogger, using firstLogDir),
+	// not from the second closer (JSONLLogger, using secondLogDir).
+	assert.Contains(t, err.Error(), firstLogDir,
+		"error should originate from the first closer (FileLogger)")
+}

internal/logger/jsonl_logger.go

@@ -12,8 +12,8 @@ import (
 
 // JSONLLogger manages logging RPC messages to a JSONL file (one JSON object per line)
 type JSONLLogger struct {
+	lockable
 	logFile  *os.File
-	mu       sync.Mutex
 	logDir   string
 	fileName string
 	encoder  *json.Encoder
@@ -67,12 +67,6 @@ func InitJSONLLogger(logDir, fileName string) error {
 	return err
 }
 
-// withLock acquires jl.mu, executes fn, then releases jl.mu.
-// Use this in methods that return an error to avoid repeating the lock/unlock preamble.
-func (jl *JSONLLogger) withLock(fn func() error) error {
-	return withMutexLock(&jl.mu, fn)
-}
-
 // Close closes the JSONL log file
 func (jl *JSONLLogger) Close() error {
 	return jl.withLock(func() error {

internal/logger/markdown_logger.go

@@ -11,8 +11,8 @@ import (
 
 // MarkdownLogger manages logging to a markdown file for GitHub workflow previews
 type MarkdownLogger struct {
+	lockable
 	logFile     *os.File
-	mu          sync.Mutex
 	logDir      string
 	fileName    string
 	useFallback bool
@@ -68,12 +68,6 @@ func (ml *MarkdownLogger) initializeFile() error {
 	return nil
 }
 
-// withLock acquires ml.mu, executes fn, then releases ml.mu.
-// Use this in methods that return an error to avoid repeating the lock/unlock preamble.
-func (ml *MarkdownLogger) withLock(fn func() error) error {
-	return withMutexLock(&ml.mu, fn)
-}
-
 // Close closes the log file and writes the closing details tag
 func (ml *MarkdownLogger) Close() error {
 	return ml.withLock(func() error {

internal/logger/tools_logger.go

@@ -27,10 +27,10 @@ type ToolsData struct {
 
 // ToolsLogger manages logging of MCP server tools to a JSON file
 type ToolsLogger struct {
+	lockable
 	logDir      string
 	fileName    string
 	data        *ToolsData
-	mu          sync.Mutex
 	useFallback bool
 }
 
@@ -81,12 +81,6 @@ func InitToolsLogger(logDir, fileName string) error {
 	return err
 }
 
-// withLock acquires tl.mu, executes fn, then releases tl.mu.
-// Use this in methods that return an error to avoid repeating the lock/unlock preamble.
-func (tl *ToolsLogger) withLock(fn func() error) error {
-	return withMutexLock(&tl.mu, fn)
-}
-
 // LogTools logs the tools for a specific server
 func (tl *ToolsLogger) LogTools(serverID string, tools []ToolInfo) error {
 	return tl.withLock(func() error {