Refactor pkg mutex sites to use deferred unlocks consistently#33038
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors mutex unlock patterns in cache and logger code to use deferred unlocks more consistently.
Changes:
- Updated repository cache getters to defer mutex unlocks.
- Updated logger timestamp locking in
Print/Printf. - Split MCP permission cache access into small lock-scoped helpers and deferred unlocks in setters.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/repository_features_validation.go |
Defers unlock in current repository cache getter. |
pkg/logger/logger.go |
Defers logger mutex unlocks around timestamp updates. |
pkg/cli/repo.go |
Defers unlock in current repo slug cache getter. |
pkg/cli/mcp_server_cache.go |
Normalizes MCP cache locking helpers and setter unlocks. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/logger/logger.go:133
- This deferred unlock has the same issue as
Printf: the logger mutex remains held whilefmt.Sprintformats arbitrary values and while stderr is written. That can deadlock if an argument'sStringmethod logs through this logger, and it serializes potentially blocking I/O while also skewinglastLogtiming for concurrent callers. Limit the locked section to thelastLogupdate.
defer l.mu.Unlock()
- Files reviewed: 4/4 changed files
- Comments generated: 4
| return | ||
| } | ||
| l.mu.Lock() | ||
| defer l.mu.Unlock() |
| c.mu.Lock() | ||
| delete(c.permissions, cacheKey) | ||
| c.mu.Unlock() | ||
| c.deletePermissionEntry(cacheKey) |
| defer currentRepoSlugCache.mu.Unlock() | ||
| if !currentRepoSlugCache.done { | ||
| currentRepoSlugCache.result, currentRepoSlugCache.err = getCurrentRepoSlugUncached() | ||
| currentRepoSlugCache.done = true | ||
| } | ||
| result := currentRepoSlugCache.result | ||
| err := currentRepoSlugCache.err |
| currentRepositoryCache.mu.Lock() | ||
| defer currentRepositoryCache.mu.Unlock() | ||
| if !currentRepositoryCache.done { | ||
| currentRepositoryCache.result, currentRepositoryCache.err = getCurrentRepositoryUncached() | ||
| currentRepositoryCache.done = true | ||
| } | ||
| result := currentRepositoryCache.result | ||
| err := currentRepositoryCache.err |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — this is a consistency refactor, so the key questions are whether every site is semantically equivalent after the change and whether the new helpers earn their complexity.
Key Themes
-
Lock-scope widening in
logger.go(blocking): ThedeferinPrintfandPrintenlarges the critical section to includefmt.Sprintfandlipgloss.Fprintf. The original code deliberately released the lock before formatting and I/O. This is a silent behaviour change — all concurrent log calls on the same logger are now serialised through the write. Needs either a fix to restore the original scope or a comment explaining the widened scope is intentional. -
Redundant local variable copies (minor):
resultanderrtemporaries inrepo.goandrepository_features_validation.gowere there to safely capture values before the early manual unlock. Withdeferholding the lock through the full function, those copies add noise without value and should be removed. -
TOCTOU pattern made less visible (worth noting):
getPermissionEntry+deletePermissionEntrysplits a two-phase read-then-delete across two separate lock acquisitions. The race was pre-existing, but splitting into helpers hides the relationship between the two lock sites; inline code at least kept them adjacent. A comment documenting the accepted race would help future readers.
Positive Highlights
- ✅
SetPermission,SetRepo, and the cache getter patterns are straightforwardly improved —deferis the right tool there - ✅ The PR description is accurate and includes a good before/after example
- ✅ Scope is narrow and well-targeted
Verdict
Requesting changes on the logger.go lock-scope issue, which silently changes concurrent behaviour. The other two findings are minor and can be addressed in follow-up if preferred.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.6M
| return | ||
| } | ||
| l.mu.Lock() | ||
| defer l.mu.Unlock() |
There was a problem hiding this comment.
[/zoom-out] The defer widens the critical section beyond what the original code intended. Before this change, the mutex was released before fmt.Sprintf and lipgloss.Fprintf — formatting and I/O were intentionally outside the lock. Now they run under the mutex, serialising all concurrent log calls on the same logger for the duration of the write.
This is a silent behaviour change. If holding the lock through I/O is intentional (to avoid interleaved output lines), that should be documented with a comment. If the original narrow scope was intentional, consider:
func (l *Logger) Printf(format string, args ...any) {
if !l.enabled {
return
}
now := time.Now()
var diff time.Duration
l.mu.Lock()
diff = now.Sub(l.lastLog)
l.lastLog = now
l.mu.Unlock()
message := fmt.Sprintf(format, args...)
lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff))
}This keeps the original minimal-lock behaviour without reverting to manual Unlock() calls.
| currentRepoSlugCache.result, currentRepoSlugCache.err = getCurrentRepoSlugUncached() | ||
| currentRepoSlugCache.done = true | ||
| } | ||
| result := currentRepoSlugCache.result |
There was a problem hiding this comment.
[/improve-codebase-architecture] The local copies result and err on lines 100–101 are now redundant. With the original manual unlock, these copies were needed to capture values before releasing the lock before further processing. With defer, the lock is held throughout the function, so the struct fields can be returned directly:
func GetCurrentRepoSlug() (string, error) {
currentRepoSlugCache.mu.Lock()
defer currentRepoSlugCache.mu.Unlock()
if !currentRepoSlugCache.done {
currentRepoSlugCache.result, currentRepoSlugCache.err = getCurrentRepoSlugUncached()
currentRepoSlugCache.done = true
}
if currentRepoSlugCache.err != nil {
return "", currentRepoSlugCache.err
}
repoLog.Printf("Using cached repository slug: %s", currentRepoSlugCache.result)
return currentRepoSlugCache.result, nil
}Same applies to the parallel pattern in pkg/workflow/repository_features_validation.go.
| delete(c.permissions, cacheKey) | ||
| } | ||
|
|
||
| // GetPermission returns the cached permission for the given actor and repo, or ("", false) on cache miss. |
There was a problem hiding this comment.
[/improve-codebase-architecture] The getPermissionEntry / deletePermissionEntry helpers introduce a two-lock TOCTOU pattern: read under RLock, check expiry outside any lock, then conditionally delete under a new Lock. Between the two lock acquisitions, another goroutine could call SetPermission, inserting a fresh valid entry — which deletePermissionEntry would then remove.
This race existed before the refactor, but the helper split makes the two-phase pattern less visible. The original inline code at least co-located the two lock sites. Consider documenting the accepted race with a comment, or using sync.Map / a single write-lock for GetPermission to close it definitively.
|
@copilot review all comments and reviews |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the actionable review comments in d7147b7: logger lock scope is reduced again, the repository cache getters now use scoped deferred unlocks so logging happens after unlock, and MCP permission eviction only removes the same expired entry with regression coverage added. |
|
@copilot merge main and recompile |
…r-mu-unlock Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
…r-mu-unlock Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/mostly follows thedefer mu.Unlock()/defer mu.RUnlock()pattern, but a small set of cache and logger paths still used manual unlocks. Those sites were correct today, but inconsistent lock scopes made future early-return refactors easier to get wrong.Cache getters now use deferred unlocks consistently
pkg/cli/repo.go:GetCurrentRepoSlugandpkg/workflow/repository_features_validation.go:getCurrentRepositoryto defer the mutex unlock immediately after locking.pkg/.Logger timestamp updates follow the same lock pattern
pkg/logger/logger.goin(*Logger).Printfand(*Logger).Printto defer unlocks around thelastLogtimestamp update.MCP server cache lock scopes were normalized
pkg/cli/mcp_server_cache.gosoGetPermission,SetPermission, andSetRepouse deferred unlocks likeGetRepoalready did.GetPermissionwas split into small lock-scoped helpers so the read-lock and eviction write-lock each have a single clear deferred unlock site.Example of the pattern applied in the touched sites: