refactor: deduplicate withLock() across logger types#2939
Merged
Conversation
Add withMutexLock() helper in global_helpers.go and delegate all four identical withLock() methods to it. Zero behavior change — locking semantics are preserved exactly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes duplicated withLock() implementations across multiple logger types by introducing a shared withMutexLock(mu, fn) helper in internal/logger/global_helpers.go, and delegating each per-type withLock() to that helper to keep locking semantics consistent.
Changes:
- Added package-level
withMutexLock(*sync.Mutex, func() error) errorhelper. - Updated
FileLogger,JSONLLogger,MarkdownLogger, andToolsLoggerwithLock()methods to callwithMutexLock(&<type>.mu, fn). - Retained the existing lock/unlock + deferred unlock behavior while reducing maintenance overhead.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/logger/global_helpers.go |
Adds withMutexLock() helper used to centralize the per-logger withLock() pattern. |
internal/logger/file_logger.go |
FileLogger.withLock() delegates to withMutexLock(). |
internal/logger/jsonl_logger.go |
JSONLLogger.withLock() delegates to withMutexLock(). |
internal/logger/markdown_logger.go |
MarkdownLogger.withLock() delegates to withMutexLock(). |
internal/logger/tools_logger.go |
ToolsLogger.withLock() delegates to withMutexLock(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Mar 31, 2026
lpcox
added a commit
that referenced
this pull request
Mar 31, 2026
## Problem Duplicate code analysis (#2907) identified 3 patterns. #2909 (withLock) was already fixed in PR #2939. This PR addresses the remaining two: ### Pattern #2908: Boilerplate initGlobal\*/closeGlobal\* Wrappers `global_helpers.go` contained **10 one-line wrapper functions** (5 init + 5 close pairs) that simply delegated to the generic `initGlobalLogger`/`closeGlobalLogger` helpers. Adding a 6th logger type would require adding yet another copy-paste pair. **Fix**: Remove all 10 wrappers. Callers now invoke the generic helpers directly. **-53 lines**. ### Pattern #2910: Auth/Guard Rejection Trio Every auth rejection in `auth.go` repeated the same 3-step block: structured log → runtime error log → HTTP error response. Any change to logging or response shape required updating every site. **Fix**: Extract `rejectRequest()` helper in `http_helpers.go` that consolidates all three steps into one call. **-6 lines, +9 lines (net: +3 with the helper)**. ## Changes | File | Change | |------|--------| | `internal/logger/global_helpers.go` | Removed 10 boilerplate wrapper functions | | `internal/logger/file_logger.go` | Call `initGlobalLogger`/`closeGlobalLogger` directly | | `internal/logger/jsonl_logger.go` | Call `initGlobalLogger`/`closeGlobalLogger` directly | | `internal/logger/markdown_logger.go` | Call `initGlobalLogger`/`closeGlobalLogger` directly | | `internal/logger/server_file_logger.go` | Call `initGlobalLogger`/`closeGlobalLogger` directly | | `internal/logger/tools_logger.go` | Call `initGlobalLogger`/`closeGlobalLogger` directly | | `internal/server/http_helpers.go` | Added `rejectRequest()` helper | | `internal/server/auth.go` | Use `rejectRequest()` instead of inline trio | ## Testing `make agent-finished` passes (format, build, lint, all tests). Both changes are **zero-behavior-change** refactors. Closes #2907 Closes #2908 Closes #2910
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Four logger types (
FileLogger,JSONLLogger,MarkdownLogger,ToolsLogger) each contained an identicalwithLock()method body. Any change to the locking strategy would require four identical edits.Reported in #2910 (from duplicate code analysis #2907).
Fix
Added a package-level
withMutexLock(mu, fn)helper inglobal_helpers.goand updated all fourwithLock()methods to delegate to it in a single line.Zero behavior change — locking semantics are preserved exactly.
Files Changed
internal/logger/global_helpers.gowithMutexLock()helperinternal/logger/file_logger.gowithLock→withMutexLockinternal/logger/jsonl_logger.gowithLock→withMutexLockinternal/logger/markdown_logger.gowithLock→withMutexLockinternal/logger/tools_logger.gowithLock→withMutexLockTesting
make agent-finishedpasses (format, build, lint, all tests).