refactor: deduplicate logger mutex and withLock via lockable embedding#3766
refactor: deduplicate logger mutex and withLock via lockable embedding#3766
Conversation
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>
There was a problem hiding this comment.
Pull request overview
This PR refactors the internal/logger package to reduce repeated mutex boilerplate by introducing a shared embedded lockable type, and adds a single convenience entry point to close all global loggers.
Changes:
- Added a
lockableembedded helper (mu sync.Mutex+withLock) to deduplicate per-logger locking wrappers. - Updated
FileLogger,JSONLLogger,MarkdownLogger, andToolsLoggerto embedlockableand removed their duplicatedwithLockmethods. - Added
CloseAllLoggers()to close all global logger instances in one call.
Show a summary per file
| File | Description |
|---|---|
internal/logger/global_helpers.go |
Introduces lockable and adds CloseAllLoggers() convenience closer. |
internal/logger/file_logger.go |
Embeds lockable and removes the local withLock wrapper. |
internal/logger/jsonl_logger.go |
Embeds lockable and removes the local withLock wrapper. |
internal/logger/markdown_logger.go |
Embeds lockable and removes the local withLock wrapper. |
internal/logger/tools_logger.go |
Embeds lockable and removes the local withLock wrapper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
| // 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 | ||
| } |
There was a problem hiding this comment.
CloseAllLoggers introduces new exported behavior (ordering, first-error semantics, and “attempt all closes”). Please add unit tests to lock in: (1) all CloseXxx functions are invoked even if an earlier one returns an error, and (2) the first error is returned. Also consider updating the file-level comment that says helpers here “should not be called directly by external code”, since this function is intended as a public entry point.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
) Review comment on PR #3766 flagged two gaps in the `CloseAllLoggers` addition: the file-level comment implied `CloseAllLoggers` was internal-only (it isn't), and there were no tests locking in its "attempt all, return first error" semantics. ## Changes - **`global_helpers.go`** — updated package-level doc comment to distinguish the unexported helpers (internal use only) from `CloseAllLoggers` (public entry point) - **`global_helpers_test.go`** — four new tests for `CloseAllLoggers`: - No loggers initialized → `nil` returned - All loggers close cleanly → all globals are `nil` afterward - First closer fails (pre-closed fd) → remaining closers still run, all globals `nil` - Two closers fail → error from the **first** closer is returned, not the second The "all called even on failure" and "first error wins" properties are verified by pre-closing the underlying `*os.File` before calling `CloseAllLoggers`, which causes the targeted `Close()` to return a `*os.PathError` containing the logger's directory path — making the two errors distinguishable. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build22527721/b514/launcher.test /tmp/go-build22527721/b514/launcher.test -test.testlogfile=/tmp/go-build22527721/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W emplate/v3@v3.0.-errorsas emplate/v3@v3.0.-ifaceassert x_amd64/vet gci-lint failed /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet chr/testify/requ-atomic --64 x_amd64/vet -I g_.a -oCxtYgSg x_amd64/vet --gdwarf-5 5389254/b265/ -o x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build22527721/b496/config.test /tmp/go-build22527721/b496/config.test -test.testlogfile=/tmp/go-build22527721/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build22527721/b392/vet.cfg g_.a -I x_amd64/vet --gdwarf-5 /v1.37.0 -o x_amd64/vet -uns�� 1.80.0/internal/-errorsas 1.80.0/internal/-ifaceassert x_amd64/vet d -n 10 nal/detrand cal/bin/git x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build22527721/b514/launcher.test /tmp/go-build22527721/b514/launcher.test -test.testlogfile=/tmp/go-build22527721/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W emplate/v3@v3.0.-errorsas emplate/v3@v3.0.-ifaceassert x_amd64/vet gci-lint failed /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet chr/testify/requ-atomic --64 x_amd64/vet -I g_.a -oCxtYgSg x_amd64/vet --gdwarf-5 5389254/b265/ -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build22527721/b514/launcher.test /tmp/go-build22527721/b514/launcher.test -test.testlogfile=/tmp/go-build22527721/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W emplate/v3@v3.0.-errorsas emplate/v3@v3.0.-ifaceassert x_amd64/vet gci-lint failed /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet chr/testify/requ-atomic --64 x_amd64/vet -I g_.a -oCxtYgSg x_amd64/vet --gdwarf-5 5389254/b265/ -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build22527721/b523/mcp.test /tmp/go-build22527721/b523/mcp.test -test.testlogfile=/tmp/go-build22527721/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� .cfg 5389254/b288/ 64/pkg/tool/linux_amd64/vet get --global x_amd64/compile 64/pkg/tool/linux_amd64/vet 5389�� .cfg /tmp/go-build98228851/b163/vet.c-ifaceassert x_amd64/vet get mcpg /opt/hostedtoolc(create|run) x_amd64/vet` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
Summary
Extracts the repeated
mu sync.Mutex+withLockpattern into a sharedlockablestruct inglobal_helpers.go, then embeds it in all four logger types:withLockmethodwithLockmethodwithLockmethodwithLockmethodAlso adds
CloseAllLoggers()convenience function that closes every global logger in a single call.ServerFileLoggeris intentionally excluded — it usessync.RWMutex(read-write lock) rather thansync.Mutex.Changes
global_helpers.golockablestruct +CloseAllLoggers()file_logger.golockable, removewithLockjsonl_logger.golockable, removewithLockmarkdown_logger.golockable, removewithLocktools_logger.golockable, removewithLockSub-issue resolution
logFuncsmap inglobal_helpers.gocloseGlobalLogger— minimal dedup valueMutexvsRWMutex) make dedup impracticalFixes #3685
Fixes #3735