Description
CheckForUpdatesAsync in pkg/cli/update_check.go:260 spawns a goroutine with a fire-and-forget pattern: it uses select{time.After(100ms)} as a best-effort delay with no join point. This means:
- The goroutine leaks if the context is cancelled before 100ms
- Tests cannot deterministically verify the goroutine ran
- No way to observe failure or completion
The codebase already uses errgroup (in audit.go, mcp_inspect_inspector.go) and conc/pool (in logs_run_processor.go) — consistent structured concurrency should be adopted here too.
Suggested Changes
- Replace the
time.After(100ms) heuristic with a proper channel or errgroup that the caller can wait on
- Return a wait function or channel from
CheckForUpdatesAsync so callers can optionally join
- OR convert to a synchronous call with a deadline-bounded context
- Add a test that deterministically verifies the update check ran/did not leak
Files Affected
pkg/cli/update_check.go (lines ~260, CheckForUpdatesAsync function)
Success Criteria
- No unjoined goroutines in
CheckForUpdatesAsync
- Goroutine leak detector (
goleak) passes in tests
- Existing behavior (non-blocking update check) is preserved
Source
Extracted from Goroutine Lifecycle Management & Structured Concurrency Adoption Gap discussion #47797
Priority
Medium — reduces goroutine leak risk and improves testability
🔍 Task mining by Discussion Task Miner - Code Quality Improvement Agent · sonnet46 · 58.5 AIC · ⌖ 5.43 AIC · ⊞ 7.1K · ◷
Description
CheckForUpdatesAsyncinpkg/cli/update_check.go:260spawns a goroutine with a fire-and-forget pattern: it usesselect{time.After(100ms)}as a best-effort delay with no join point. This means:The codebase already uses
errgroup(inaudit.go,mcp_inspect_inspector.go) andconc/pool(inlogs_run_processor.go) — consistent structured concurrency should be adopted here too.Suggested Changes
time.After(100ms)heuristic with a proper channel orerrgroupthat the caller can wait onCheckForUpdatesAsyncso callers can optionally joinFiles Affected
pkg/cli/update_check.go(lines ~260,CheckForUpdatesAsyncfunction)Success Criteria
CheckForUpdatesAsyncgoleak) passes in testsSource
Extracted from Goroutine Lifecycle Management & Structured Concurrency Adoption Gap discussion #47797
Priority
Medium — reduces goroutine leak risk and improves testability