From dad4de7faf12e3a09f3395f5dc845b9862e19142 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 14 Apr 2026 08:21:54 -0700 Subject: [PATCH 1/4] refactor: deduplicate logger mutex and withLock via lockable embedding Extract lockable struct (embedded sync.Mutex + withLock helper) into global_helpers.go and embed it in FileLogger, JSONLLogger, MarkdownLogger, and ToolsLogger. This removes four identical withLock method definitions. Also add CloseAllLoggers() convenience function that closes every global logger in a single call. Fixes #3685 Fixes #3735 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/logger/file_logger.go | 8 +----- internal/logger/global_helpers.go | 43 ++++++++++++++++++++++++++++-- internal/logger/jsonl_logger.go | 8 +----- internal/logger/markdown_logger.go | 8 +----- internal/logger/tools_logger.go | 8 +----- 5 files changed, 45 insertions(+), 30 deletions(-) diff --git a/internal/logger/file_logger.go b/internal/logger/file_logger.go index 10ef3e1f..79a2bd44 100644 --- a/internal/logger/file_logger.go +++ b/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 { diff --git a/internal/logger/global_helpers.go b/internal/logger/global_helpers.go index 002e0d93..7f75deb3 100644 --- a/internal/logger/global_helpers.go +++ b/internal/logger/global_helpers.go @@ -16,9 +16,29 @@ 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 mu field and withLock method are promoted, so callers can write +// myLogger.mu.Lock() and myLogger.withLock(fn) directly. +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 +142,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 +} diff --git a/internal/logger/jsonl_logger.go b/internal/logger/jsonl_logger.go index 0a240bf5..e07ad102 100644 --- a/internal/logger/jsonl_logger.go +++ b/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 { diff --git a/internal/logger/markdown_logger.go b/internal/logger/markdown_logger.go index a695d6c4..4c3653aa 100644 --- a/internal/logger/markdown_logger.go +++ b/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 { diff --git a/internal/logger/tools_logger.go b/internal/logger/tools_logger.go index 074f9ae7..bfdc6ed7 100644 --- a/internal/logger/tools_logger.go +++ b/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 { From d3d08227abf5a744320449bb43a1b7545ce30e79 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 14 Apr 2026 08:44:15 -0700 Subject: [PATCH 2/4] Update internal/logger/global_helpers.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/logger/global_helpers.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/logger/global_helpers.go b/internal/logger/global_helpers.go index 7f75deb3..0f64e0de 100644 --- a/internal/logger/global_helpers.go +++ b/internal/logger/global_helpers.go @@ -27,8 +27,9 @@ import "sync" // // other fields... // } // -// The embedded mu field and withLock method are promoted, so callers can write -// myLogger.mu.Lock() and myLogger.withLock(fn) directly. +// 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 } From 4e05045786dbb577a550ca23bef8063f59111421 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 15:44:28 +0000 Subject: [PATCH 3/4] Initial plan From 43c66f725790c9d0a30621462d1aa2ce7a5f76cb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 15:54:00 +0000 Subject: [PATCH 4/4] Fix review comments: update file-level comment and add CloseAllLoggers tests Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/42bb66b7-0195-443a-ae7d-9e03407639fe Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/logger/global_helpers.go | 6 +- internal/logger/global_helpers_test.go | 158 +++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 2 deletions(-) create mode 100644 internal/logger/global_helpers_test.go diff --git a/internal/logger/global_helpers.go b/internal/logger/global_helpers.go index 0f64e0de..e6c17641 100644 --- a/internal/logger/global_helpers.go +++ b/internal/logger/global_helpers.go @@ -10,8 +10,10 @@ // - 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" diff --git a/internal/logger/global_helpers_test.go b/internal/logger/global_helpers_test.go new file mode 100644 index 00000000..25f90c28 --- /dev/null +++ b/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)") +}