Refactor: apply `defer mu.Unlock()` consistently across mutex sites in pkg/

[github-actions]

Summary

A Sergo Run 12 (2026-05-18) audit of every sync.Mutex and sync.RWMutex site in pkg/ non-test code found that the codebase has a mostly-consistent defer mu.Unlock() idiom, but four files deviate with manual Unlock calls. One file (mcp_server_cache.go) is internally inconsistent — one method uses defer, three siblings don't. The current code is correct, but the inconsistency is fragile to future refactors that introduce an early return between Lock and Unlock.

Sites using manual Unlock (no defer)

File:line	Function	Notes
pkg/cli/repo.go:94-101	GetCurrentRepoSlug	Lock at L94, Unlock at L101. No early returns between, but the function body is 8 lines and any future addition becomes risky.
pkg/workflow/repository_features_validation.go:160-167	getCurrentRepository	Same shape as above. Network call (getCurrentRepositoryUncached) happens under the lock; if it ever needs to return early before storing the result, the lock must be released manually.
pkg/logger/logger.go:115-119	(*Logger).Printf	Manual Lock/Unlock around just lastLog timestamp update. The fmt-and-write happens outside the lock — that's intentional and correct. But defer would work here too.
pkg/logger/logger.go:132-136	(*Logger).Print	Same shape as Printf.
pkg/cli/mcp_server_cache.go:43-58	(*mcpCacheStore).GetPermission	RLock + manual RUnlock with two paths (one Unlock per path), then a separate Lock+Unlock for the eviction. Hard to read; the RLock/RUnlock split could be folded under a single defer if restructured.
pkg/cli/mcp_server_cache.go:64-73	(*mcpCacheStore).SetPermission	Manual Lock/Unlock.
pkg/cli/mcp_server_cache.go:88-96	(*mcpCacheStore).SetRepo	Manual Lock/Unlock.

Notable inconsistency: mcp_server_cache.go

In the same file, (*mcpCacheStore).GetRepo at lines 76-85 uses defer c.mu.RUnlock() correctly, while its three sibling methods (GetPermission, SetPermission, SetRepo) all use manual Unlock() calls. This split-personality is the most jarring instance and the easiest one to fix.

Sites already using defer (model pattern)

For reference, these are the prod sites that do use the recommended pattern and serve as the model to follow:

• pkg/agentdrain/miner.go:48-49 — m.mu.Lock(); defer m.mu.Unlock()
• pkg/agentdrain/coordinator.go:60-61, 73-74, 91-92, 115-117 — RLock/RUnlock and Lock/Unlock all deferred
• pkg/parser/virtual_fs.go:31-32, 48-49 — both defer
• pkg/workflow/script_registry.go:35-36 — defer
• pkg/cli/docker_images.go:89-90, 96-97, 130-131 — defer
• pkg/cli/mcp_server_cache.go:77-78 — defer (the one outlier in its own file)

Impact

• Severity: Low (correctness not currently affected) → Medium if future refactors introduce early returns
• Affected files: 4 production files in pkg/
• Risk: A reviewer adding an early-return code path (e.g., a return nil, ErrFoo for a new edge case) inside the lock will silently leak the mutex, hanging subsequent callers.

Recommendation

Apply defer mu.Unlock() immediately after every mu.Lock() (and defer mu.RUnlock() after mu.RLock()) in the listed sites. For mcp_server_cache.go.GetPermission, restructure the RLock-then-Lock upgrade so each lock scope is its own helper or block, allowing defer for both.

Example fix for pkg/logger/logger.go:111-123:

func (l *Logger) Printf(format string, args ...any) {
	if !l.enabled {
		return
	}
	diff := func() time.Duration {
		l.mu.Lock()
		defer l.mu.Unlock()
		now := time.Now()
		d := now.Sub(l.lastLog)
		l.lastLog = now
		return d
	}()

	message := fmt.Sprintf(format, args...)
	lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff))
}

Or — equivalent and arguably simpler — extract the lock-protected operation into a small helper method l.tickTime() time.Duration that uses defer.

Validation

• All existing tests still pass.
• go vet ./... and any in-tree linters still pass.
• Manual scan confirms every Lock() / RLock() in non-test code under pkg/ is paired with a defer Unlock() / defer RUnlock().
• Consider adding a lightweight in-tree linter under pkg/linters/ (alongside the existing excessivefuncparams, largefunc, ctxbackground analyzers) to enforce the rule going forward.

Related finding

Issue #32952 (extract shared singleflight cache for current-repository lookups) — the proposed syncutil.OnceLoader helper would inherently fix the manual-Unlock instances at cli/repo.go:101 and workflow/repository_features_validation.go:167.

Estimated effort

Small (1-2 hours for the mechanical fixes; longer if the optional in-tree linter is added).

Context

Discovered in Sergo Run 12 (2026-05-18) sync-primitives audit. This is a stylistic consistency finding rather than a current bug; filed because a single missed early-return after a future edit would silently introduce a goroutine-blocking lock leak that may evade tests entirely.

Generated by 🤖 Sergo - Serena Go Expert · ● 19.2M · ◷

• expires on May 25, 2026, 5:15 AM UTC