logger: address CloseAllLoggers review comments — doc fix + tests#3770
logger: address CloseAllLoggers review comments — doc fix + tests#3770
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>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ot/fix-comments-in-review-thread Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…s 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>
There was a problem hiding this comment.
Pull request overview
Updates the logger package documentation and adds tests to lock in CloseAllLoggers semantics, while also continuing the logger mutex/withLock deduplication via lockable embedding across logger types.
Changes:
- Clarify
global_helpers.gopackage docs to distinguish internal-only helpers vs the publicCloseAllLoggersAPI. - Add
global_helpers_test.goto assertCloseAllLoggers“attempt all closers” + “first error wins” behavior. - Refactor logger structs (File/JSONL/Markdown/Tools) to embed
lockableand remove per-typemu/withLockboilerplate.
Show a summary per file
| File | Description |
|---|---|
| internal/logger/global_helpers.go | Doc clarification + defines CloseAllLoggers and lockable helper. |
| internal/logger/global_helpers_test.go | Adds test coverage for CloseAllLoggers semantics and global cleanup expectations. |
| internal/logger/file_logger.go | Switches to embedded lockable instead of per-type mutex/withLock. |
| internal/logger/jsonl_logger.go | Switches to embedded lockable instead of per-type mutex/withLock. |
| internal/logger/markdown_logger.go | Switches to embedded lockable instead of per-type mutex/withLock. |
| internal/logger/tools_logger.go | Switches to embedded lockable instead of per-type mutex/withLock. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
| // 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() |
There was a problem hiding this comment.
resetAllGlobalLoggers nils out the global logger pointers without closing any already-initialized loggers. If a previous test leaves a global logger open (or a new test is added that forgets to close), this helper will leak file descriptors and can make failures harder to debug. Consider calling CloseAllLoggers() (ignoring its error) before clearing pointers, and update the comment to reflect that it uses the global RWMutexes (not the per-logger mutexes).
| // 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 |
There was a problem hiding this comment.
PR description says this follow-up only updates global_helpers.go docs + adds CloseAllLoggers tests, but this PR also changes FileLogger to embed lockable and removes the per-type mu/withLock boilerplate. Please either update the PR description to reflect these additional refactor changes or split them into a separate PR/commit so review intent matches the diff.
Review comment on PR #3766 flagged two gaps in the
CloseAllLoggersaddition: the file-level comment impliedCloseAllLoggerswas 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) fromCloseAllLoggers(public entry point)global_helpers_test.go— four new tests forCloseAllLoggers:nilreturnednilafterwardnilThe "all called even on failure" and "first error wins" properties are verified by pre-closing the underlying
*os.Filebefore callingCloseAllLoggers, which causes the targetedClose()to return a*os.PathErrorcontaining the logger's directory path — making the two errors distinguishable.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/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/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/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/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/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: