[sergo] Sergo Report: Mutex-Audit + init()-Function-Audit - 2026-03-03

[github-actions[bot]]

Date: 2026-03-03
Strategy: mutex-audit-plus-init-function-audit
Success Score: 8/10
Run: §22645166774

Executive Summary

Today's Sergo run focused on two complementary angles: (1) auditing mutex and synchronization patterns across all pkg/ files—particularly those touched by the recent debug-logging PR #19455—and (2) a systematic sweep of all init() functions in pkg/ to detect dead or misleading initialization code introduced alongside the new logger calls.

The audit uncovered a TOCTOU (Time-of-Check-Time-of-Use) race condition in mcp_server_cache.go::GetPermission() where a valid, freshly-inserted permission entry can be spuriously evicted by a concurrent goroutine racing through an expired-entry cleanup path. Two additional findings concern misleading init() bodies—one falsely claims to be initializing regex patterns that Go has already compiled, and two others are fully dead no-op functions retained only to print a "completed" log message for removed code. A bonus finding identifies a manual bubble sort in GetAllScriptFilenames() inconsistent with the sort.Strings() convention used 100+ times elsewhere in the codebase.

🛠️ Serena Tools Update

Tools Snapshot

• Total Tools Available: 23 (unchanged from 2026-03-01)
• New Tools Since Last Run: None
• Removed Tools: None
• Modified Tools: None

Tool Capabilities Used Today

Tool	Usage
search_for_pattern	Scanned for sync.Mutex, sync.RWMutex, .Lock(), .RLock(), and init() patterns across pkg/
grep (native)	Fallback for symbol cross-reference searches after Serena timeouts
read_file	Full file reads for logger.go, mcp_server_cache.go, script_registry.go, scripts.go, js.go, expression_patterns.go, compile_watch.go
list_dir	Package structure overview

Note: Serena find_referencing_symbols and think_about_collected_information timed out mid-session (consistent with observed pattern over past 3 runs). Native grep+read tools used as reliable fallback.

📊 Strategy Selection

Cached Reuse Component (50%)

Previous Strategy Adapted: error-patterns-plus-field-registry-audit (Run 3, 2026-02-23)

• Original Success Score: 9/10 (highest scored run)
• Last Used: 2026-02-23 (8 days ago)
• Why Reused: The field-registry audit approach—cross-referencing how recently-modified code interacts with shared state—proved highly productive. Today I applied the same lens to the 5 pkg/ files touched by the new debug-logging PR ([log] Add debug logging to 5 pkg/ files #19455), examining whether the new log variable declarations and logger.New() calls exposed any concurrent-access issues in the surrounding code.
• Modifications: Instead of tracking field-mapping divergence, I traced through the mutex lock/unlock sequences around every new log call. This led directly to the TOCTOU finding in mcp_server_cache.go.

New Exploration Component (50%)

Novel Approach: init()-function-audit

• Tools Employed: search_for_pattern (pattern func init\(\)), read_file for each hit
• Hypothesis: The new debug logging PR added init() functions to several files to emit startup trace messages. In at least some cases these functions may be no-ops (logging "completed" for work that was removed) or may misrepresent Go's initialization order.
• Target Areas: All non-test Go files in pkg/workflow/ and pkg/cli/ containing func init()

Combined Strategy Rationale

The mutex audit targets runtime correctness (race conditions), while the init() audit targets compile-time/startup correctness (misleading documentation and dead code). Both are triggered by the same PR—a natural forcing function that makes this a timely and high-signal analysis.

🔍 Analysis Execution

Codebase Context

• Total Go Files (non-test): 561 (554 in pkg/)
• Packages Analyzed in Depth: pkg/cli (mcp_server_cache, compile_watch, repo), pkg/workflow (scripts, js, expression_patterns, runtime_definitions, script_registry), pkg/logger
• init() Functions Found: 8 in pkg/ (non-test)
• Mutex-bearing Structs Found: 8 (Logger, ScriptRegistry, currentRepositoryCacheState, dockerPullState, mcpCacheStore, repoSlugCacheState, debounceMu, Spinner)

Findings Summary

• Total Issues Found: 3
• High: 1 (TOCTOU race condition)
• Medium: 2 (misleading init() bodies, inconsistent sort)
• Low: 0

📋 Detailed Findings

High Priority Issues

TOCTOU Race in mcp_server_cache.go::GetPermission()

pkg/cli/mcp_server_cache.go lines 41–61 contain a classic upgrade-race: the function acquires RLock, finds an expired entry, releases RLock, then re-acquires a write Lock to delete the entry. In the window between releasing and re-acquiring, another goroutine can call SetPermission and insert a fresh, valid entry for the same cache key—which the first goroutine then unconditionally deletes.

// Current code — TOCTOU window between RUnlock and Lock
c.mu.RLock()
entry, ok := c.permissions[cacheKey]
if ok && time.Since(entry.timestamp) < c.permissionTTL {
    perm := entry.permission
    c.mu.RUnlock()
    return perm, true
}
c.mu.RUnlock()   // ← release here
if ok {
    // Expired — remove it
    c.mu.Lock()   // ← another goroutine may have refreshed the entry in this window
    delete(c.permissions, cacheKey)  // ← silently evicts a valid fresh entry
    c.mu.Unlock()
}
return "", false

Race scenario:

1. Goroutine A: acquires RLock, finds entry expired (TTL elapsed), releases RLock
2. Goroutine B: acquires write Lock, calls SetPermission, inserts fresh entry, releases Lock
3. Goroutine A: acquires write Lock, deletes the entry — evicting the fresh entry B just inserted
4. Goroutine A: returns ("", false) — a spurious cache miss

Result: permission queries that should hit the cache are instead forced to re-fetch, and in the worst case the permission state becomes inconsistent with what was last explicitly set.

Medium Priority Issues

Misleading init() bodies in pkg/workflow/

Three init() functions introduced or modified by recent debug-logging work contain misleading log messages:

1. pkg/workflow/expression_patterns.go:67–69:

func init() {
    expressionPatternsLog.Print("Initializing expression pattern regex compilation")
}

Problem: The regex variables are compiled by var block declarations before init() runs (Go initialization order: package-level vars → init()). By the time this init() executes, all regexp.MustCompile calls have already completed. The message falsely implies that init() is responsible for the compilation, which could mislead developers tracing startup behavior.

2. pkg/workflow/scripts.go:20–22 and pkg/workflow/js.go:64–66:

// scripts.go
func init() {
    scriptsLog.Print("Script registration completed (embedded scripts removed)")
}
// js.go
func init() {
    jsLog.Print("Script registration completed (embedded scripts removed)")
}

Problem: Both functions are pure no-ops. They exist only to emit a "completed" message for script registration work that was removed when embedded scripts were migrated to actions/setup. The comment on scripts.go:18 still says "// init registers scripts" even though nothing is registered. These are dead init() functions that add startup overhead (however tiny) and create noise in debug traces.

Manual bubble sort in GetAllScriptFilenames()

pkg/workflow/script_registry.go:70–77:

sortedFilenames := make([]string, len(filenames))
copy(sortedFilenames, filenames)
for i := range sortedFilenames {
    for j := i + 1; j < len(sortedFilenames); j++ {
        if sortedFilenames[i] > sortedFilenames[j] {
            sortedFilenames[i], sortedFilenames[j] = sortedFilenames[j], sortedFilenames[i]
        }
    }
}

This is a hand-rolled O(n²) bubble sort. The codebase uses sort.Strings() in over 100 other locations (confirmed by full-codebase grep). The sort package is already imported by multiple files in the same package. The inconsistency is a maintenance liability and the quadratic complexity becomes noticeable for larger script lists.

✅ Improvement Tasks Generated

---

Task 1: Fix TOCTOU Race in mcp_server_cache.go::GetPermission()

Issue Type: Concurrency / Data Race

Problem:
After releasing an RLock for an expired cache entry and before acquiring a write Lock to delete it, a concurrent SetPermission goroutine can insert a fresh entry. The subsequent delete then evicts a valid entry, causing spurious cache misses and inconsistent permission state.

Location:

• pkg/cli/mcp_server_cache.go:51–57 — TOCTOU window between RUnlock and write Lock

Impact:

• Severity: High
• Affected Files: 1
• Risk: Under concurrent MCP server load, permission cache entries that were just refreshed get spuriously deleted, forcing redundant permission lookups and potentially causing transient permission failures in multi-goroutine scenarios.

Recommendation:
Re-check the entry's expiry inside the write lock before deleting. This is the standard pattern for safe lock-upgrade in Go:

Before:

c.mu.RUnlock()
if ok {
    // Expired — remove it
    mcpServerCacheLog.Printf("Permission cache entry expired for actor=%s, repo=%s", actor, repo)
    c.mu.Lock()
    delete(c.permissions, cacheKey)
    c.mu.Unlock()
}

After:

c.mu.RUnlock()
if ok {
    // Re-check under write lock — another goroutine may have refreshed the entry
    c.mu.Lock()
    if e, stillExists := c.permissions[cacheKey]; stillExists && time.Since(e.timestamp) >= c.permissionTTL {
        mcpServerCacheLog.Printf("Permission cache entry expired for actor=%s, repo=%s", actor, repo)
        delete(c.permissions, cacheKey)
    }
    c.mu.Unlock()
}

Validation:

• Run existing tests
• Add a concurrent test: two goroutines—one calling GetPermission on an entry at TTL boundary, one calling SetPermission simultaneously—verifying the fresh entry survives
• Review GetRepo/SetRepo for the same pattern (currently safe due to defer mu.RUnlock() in GetRepo)

Estimated Effort: Small

---

Task 2: Remove or Correct Dead init() Functions in pkg/workflow/

Issue Type: Dead Code / Misleading Debug Output

Problem:
Three init() functions in pkg/workflow/ emit log messages that are either factually incorrect (claiming work is happening when it already happened or no longer happens) or entirely dead:

1. expression_patterns.go:67–69: says "Initializing expression pattern regex compilation" but regex vars are already compiled before init() runs.
2. scripts.go:20–22: says "Script registration completed (embedded scripts removed)" — no work is done; this is pure dead code.
3. js.go:64–66: identical dead pattern to scripts.go.

Locations:

• pkg/workflow/expression_patterns.go:67–69
• pkg/workflow/scripts.go:18–22
• pkg/workflow/js.go:62–66

Impact:

• Severity: Medium
• Affected Files: 3
• Risk: Developer confusion about Go initialization order; misleading DEBUG traces make it harder to diagnose real startup issues; dead init() functions that serve no purpose.

Recommendation:

For expression_patterns.go: Move the log message to accurately reflect the state after initialization, or remove it entirely since the var block already handles compilation:

// Before (misleading):
func init() {
    expressionPatternsLog.Print("Initializing expression pattern regex compilation")
}

// After (accurate) — or simply delete the init() entirely:
func init() {
    expressionPatternsLog.Print("Expression patterns ready")
}

For scripts.go and js.go: Remove the dead init() functions entirely (and the scriptsLog / jsLog vars if no longer used, though jsLog is used elsewhere in js.go):

// Delete entirely from scripts.go:
// var scriptsLog = logger.New("workflow:scripts")
// func init() { scriptsLog.Print("Script registration completed (embedded scripts removed)") }

Validation:

• Confirm scriptsLog has no other uses in scripts.go before removing
• Confirm removing init() from expression_patterns.go doesn't change package behaviour (it shouldn't—only removes a side-effect log)
• Run full test suite to confirm no init-order regressions

Estimated Effort: Small

---

Task 3: Replace Manual Bubble Sort with sort.Strings() in GetAllScriptFilenames()

Issue Type: Code Quality / Inconsistency

Problem:
pkg/workflow/script_registry.go:GetAllScriptFilenames() contains a hand-written O(n²) bubble sort. The codebase uses sort.Strings() in over 100 other locations—this is the only known outlier.

Location:

• pkg/workflow/script_registry.go:69–77

Impact:

• Severity: Medium
• Affected Files: 1
• Risk: O(n²) vs O(n log n) for potentially large script lists; maintenance inconsistency; reader confusion ("why doesn't this use sort.Strings like everything else?").

Recommendation:

Before:

sortedFilenames := make([]string, len(filenames))
copy(sortedFilenames, filenames)
for i := range sortedFilenames {
    for j := i + 1; j < len(sortedFilenames); j++ {
        if sortedFilenames[i] > sortedFilenames[j] {
            sortedFilenames[i], sortedFilenames[j] = sortedFilenames[j], sortedFilenames[i]
        }
    }
}

After:

sort.Strings(filenames)

(The copy into a separate slice is unnecessary since filenames is a local variable built in this function and not shared.)

Also add "sort" to the import block if not already present (it is available via the "sort" standard library package).

Validation:

• Run existing tests; GetAllScriptFilenames() is called from pkg/cli/actions_build_command.go:345
• Verify that callers do not depend on any ordering guarantee beyond lexicographic ascending (they don't—callers iterate or embed the list)

Estimated Effort: Small (1–3 lines)

---

📈 Success Metrics

This Run

• Findings Generated: 3
• Tasks Created: 3
• Files Analyzed in Depth: 7
• Mutex-bearing Structs Surveyed: 8
• init() Functions Inspected: 8
• Success Score: 8/10

Reasoning for Score

• Findings Quality (3/4): One HIGH race condition with a concrete fix; two MEDIUM issues that are genuine code quality problems, not just stylistic preferences. All three are actionable.
• Coverage (2.5/3): Covered all mutex-bearing structs in pkg/ and all init() functions. Some deeper reference tracing was blocked by Serena timeouts.
• Task Generation (2.5/3): Three well-scoped, small-effort tasks with before/after code examples.

📊 Historical Context

Strategy Performance

Date	Strategy	Score	Findings
2026-02-21	symbol-analysis-plus-error-patterns	8	3
2026-02-22	interface-coverage-plus-registry-analysis	8	3
2026-02-23	error-patterns-plus-field-registry-audit	9	3
2026-02-24	error-wrapping-plus-context-propagation	8	3
2026-02-25	blank-identifier-audit-plus-incomplete-impl	8	3
2026-02-26	registry-nil-plus-goroutine-lifecycle	8	3
2026-02-28	config-field-audit-plus-subprocess-timeout	7	3
2026-03-01	context-propagation-revisit-plus-pagination-audit	8	3
2026-03-02	context-sleep-audit-plus-polling-infra	8	3
2026-03-03	mutex-audit-plus-init-function-audit	8	3

Cumulative Statistics

• Total Runs: 10
• Total Findings: 30
• Total Tasks Generated: 30
• Average Success Score: 8.0/10
• Most Successful Strategy: error-patterns-plus-field-registry-audit (9/10)
• Serena Stability: Consistent timeout on find_referencing_symbols and think_* tools in sessions with >3 queries. Native grep/read used as reliable fallback.

Unfixed Recurring Issues (across multiple runs)

These issues have been reported in previous runs but remain unfixed as of 2026-03-03:

1. findIncludesInContent ignores baseDir (HIGH data-loss bug) — reported in runs 5, 7, 9
2. GetSupportedEngines/GetAllEngines/GetEngineByPrefix non-deterministic map iteration — reported in runs 2, 6, 8
3. mcp_registry.go::createRegistryRequest uses http.NewRequest without context — reported in runs 6, 8
4. getLatestWorkflowRunWithRetry missing ctx + should use PollWithSignalHandling — reported in run 9

🎯 Recommendations

Immediate Actions

1. Fix TOCTOU in GetPermission() (High) — 3-line fix; add a re-check after write-lock acquisition
2. Remove dead init() functions (Medium) — Delete scripts.go:init(), correct expression_patterns.go:init() message
3. Replace bubble sort with sort.Strings() (Medium) — 1-line fix for consistency and correctness

Long-term Improvements

• Consider a lint rule for the lock-upgrade pattern (RLock → RUnlock → Lock without re-check). This is a recurring category of concurrency bug.
• The three unfixed HIGH-severity issues from runs 5–9 (especially findIncludesInContent baseDir and non-deterministic engine map iteration) warrant dedicated fix PRs; their persistence across multiple Sergo reports suggests they need explicit escalation.

🔄 Next Run Preview

Suggested Focus Areas

• Unexplored: pkg/tty, pkg/timeutil, pkg/mathutil, pkg/sliceutil, pkg/stringutil — smaller utility packages that haven't been analyzed yet; may contain subtle issues in utility functions relied on broadly.
• Revisit: channel direction typing and unbuffered channel patterns across goroutine-heavy files in pkg/workflow/.

Strategy Evolution

Today's init() audit was productive specifically because of the recent debug-logging PR. For future runs, tying the analysis to the most-recent commit (using git show --name-only) is a reliable way to ensure findings are timely and directly actionable.

---

References:

• §22645166774

AI generated by Sergo - Serena Go Expert · history

• expires on Mar 4, 2026, 10:27 PM UTC