[repository-quality] 🎯 Repository Quality Improvement Report — Goroutine Lifecycle Management & Structured Concurrency Adoption Gap

[github-actions[bot]]

Analysis Date: 2026-07-24
Focus Area: Goroutine Lifecycle Management & Structured Concurrency Adoption Gap
Strategy Type: Custom
Custom Area: Yes — goroutine lifecycle is a repository-specific pattern gap; the codebase already uses errgroup and conc/pool correctly in 2 files but 5 fire-and-forget goroutines lack structured join/cancellation patterns

Executive Summary

The repository launches 7 production goroutines across 6 files. Two files (pkg/cli/audit.go and pkg/cli/mcp_inspect_inspector.go) use golang.org/x/sync/errgroup for structured concurrency, and pkg/cli/logs_run_processor.go uses sourcegraph/conc/pool with bounded parallelism and automatic panic recovery. However, four other goroutine launch sites — update_check.go, compile_update_check.go, docker_images.go, and bootstrap_profile_github_app.go — use ad-hoc fire-and-forget patterns with inconsistent lifecycle guarantees.

The most critical gap is in docker_images.go: StartDockerImagePull launches a goroutine with no join point whatsoever. The caller has no mechanism to know when the download finishes or whether it failed, and the goroutine can outlive the process context, deleting map entries via deferred cleanup after pullState has been garbage-collected. Similarly, CheckForUpdatesAsync in update_check.go spawns a goroutine and then does a select{time.After(100ms)} as a best-effort delay — a fire-and-forget pattern that leaks when contexts are cancelled and has no way to signal completion.

With sourcegraph/conc already in go.mod and errgroup already imported by 3 packages, adopting consistent structured concurrency primitives across all goroutine sites would eliminate goroutine leak risk, simplify testing, and standardize the codebase pattern.

Full Analysis Report

Focus Area: Goroutine Lifecycle Management & Structured Concurrency Adoption Gap

Current State Assessment

Production goroutine inventory:

File	Launch Site	Pattern	Join Point	Leak Risk
pkg/cli/update_check.go:260	CheckForUpdatesAsync	go func() + select{time.After}	None	⚠️ Medium — goroutine runs until context expires
pkg/cli/compile_update_check.go:67	StartCompileUpdateCheck	go func() + buffered chan	waitForCompileUpdateNotification	✅ Low — channel closed on exit
pkg/cli/docker_images.go:155	StartDockerImagePull	go func() + map delete	None	❌ High — no join point; map state mutated after return
pkg/cli/bootstrap_profile_github_app.go:211	HTTP server goroutine	go func() + select on resultCh	HTTP server Shutdown	✅ Low — deferred Shutdown
pkg/cli/bootstrap_profile_helpers.go:356	Browser launcher	go func() { _ = cmd.Wait() }	None	✅ Benign — intentional fire-and-forget for browser
pkg/console/spinner.go:144	Spinner loop	go func() + sync.WaitGroup	wg.Wait() in Stop()	✅ Low — WaitGroup provides join
pkg/cli/mcp_inspect_inspector.go:47	MCP inspector	errgroup	g.Wait()	✅ Low — structured

Metrics Collected:

Metric	Value	Status
Total production go func() goroutines	7	⚠️
Goroutines with no join point	2 (docker_images, update_check async)	❌
Files using errgroup	3 (audit.go, mcp_inspect_inspector.go, mcp_inspect_mcp.go)	✅
Files using conc/pool	1 (logs_run_processor.go)	✅
Goroutine launch sites with inconsistent patterns	4	⚠️
goleak usage in tests	0	❌
Goroutines with recover() guard	4	⚠️ Mixed

Findings

Strengths

• pkg/cli/logs_run_processor.go uses conc/pool.NewWithResults with WithContext and WithMaxGoroutines — an exemplary bounded-concurrency pattern
• pkg/cli/audit.go uses errgroup.WithContext throughout with structured launch helpers
• pkg/console/spinner.go uses sync.WaitGroup correctly with deferred wg.Done and wg.Wait() in Stop()
• pkg/cli/compile_update_check.go uses a buffered channel with defer close() to signal goroutine completion

Areas for Improvement

• ❌ High: docker_images.go:StartDockerImagePull — goroutine has no join point. The downloading map is mutated from the goroutine after StartDockerImagePull returns, and callers cannot block on completion or observe failure
• ⚠️ Medium: update_check.go:CheckForUpdatesAsync — fire-and-forget with a select{time.After(100ms)} heuristic. The goroutine is detached from the caller; no test can deterministically verify it ran
• ⚠️ Medium: 0 uses of go.uber.org/goleak or similar goroutine leak detector in tests, meaning goroutine leaks are undetectable in the test suite
• ⚠️ Low: bootstrap_profile_github_app.go — dual separate resultCh/errCh channels could be replaced with a single errgroup pattern for clarity

Detailed Analysis

StartDockerImagePull (docker_images.go:130-220):
This function starts a goroutine that retries docker pull up to 3 times with exponential backoff. It sets pullState.downloading[image] = true before launching the goroutine and defers delete(pullState.downloading, image) inside the goroutine. There is no mechanism for callers to await completion, observe errors, or guarantee that pullState.downloading is clean after a test. The function returns true immediately, making error-path testing impossible without race conditions.

CheckForUpdatesAsync (update_check.go:257-294):
Spawns a goroutine and then does a select { case <-time.After(100ms); case <-ctx.Done() } — this makes the caller wait up to 100ms, which is a time.Sleep anti-pattern in disguise. The goroutine itself is fully detached after that window. In tests, the goroutine may run after the test function returns, causing data races on shared loggers.

bootstrap_profile_github_app.go:205-236 (server goroutine):
Uses separate resultCh chan *result and errCh chan error (1-buffered each) instead of the idiomatic errgroup or conc pattern. The dual-channel select is correct but verbose; errgroup would collapse this into a single g.Wait() with error propagation.

---

🤖 Tasks for Copilot Agent

NOTE TO PLANNER AGENT: Split the following tasks into individual work items.

Improvement Tasks

Task 1: Add Join Point to StartDockerImagePull via Done Channel

Priority: High
Estimated Effort: Medium
Focus Area: Goroutine Lifecycle / docker_images.go

Description: StartDockerImagePull in pkg/cli/docker_images.go launches a goroutine with no join point, making it impossible to await download completion or observe errors. Refactor it to return a <-chan error (closed on completion) so callers can optionally block. Update CheckAndPrepareDockerImages to await all outstanding downloads before returning. Add a test that verifies StartDockerImagePull goroutine completes cleanly.

Acceptance Criteria:

• StartDockerImagePull returns <-chan error that closes (nil error) on success or sends an error on failure
• CheckAndPrepareDockerImages blocks on all outstanding downloads before returning
• All existing tests pass
• New test verifies the channel closes after download completes

Code Region: pkg/cli/docker_images.go

In `pkg/cli/docker_images.go`, refactor `StartDockerImagePull(ctx context.Context, image string) bool` to return `(bool, <-chan error)` instead of just `bool`. The returned channel should be a buffered `make(chan error, 1)` closed (with nil or the error) after the goroutine exits. Store the channel in `pullState.downloading` (change value from `bool` to `chan error`). Update `CheckAndPrepareDockerImages` to collect all download channels from `pullState.downloading` and do a `select` / wait for each before returning. Update all callers of `StartDockerImagePull` to accept the second return value. Run `make fmt` and all unit tests to verify.

---

Task 2: Replace Fire-and-Forget + time.After in CheckForUpdatesAsync

Priority: High
Estimated Effort: Small
Focus Area: Goroutine Lifecycle / update_check.go

Description: CheckForUpdatesAsync in pkg/cli/update_check.go spawns a detached goroutine and then does select{time.After(100ms)} — a fire-and-forget pattern that is hard to test and can cause goroutine leaks. Refactor to return a func() join function (same pattern as StartCompileUpdateCheck in compile_update_check.go), so the caller can optionally wait for the result before exiting. The goroutine should write to a buffered channel; the join function reads with a short timeout.

Acceptance Criteria:

• CheckForUpdatesAsync is renamed or replaced by a StartUpdateCheck(ctx) func() returning a join function
• The join function blocks up to compileUpdateCheckTimeout (or a new updateCheckNoWait constant) for a result
• No time.After sleep inside the function body — the timeout is only in the join function
• Callers in main.go / compile command updated
• Existing update check tests pass

Code Region: pkg/cli/update_check.go, cmd/gh-aw/main.go

In `pkg/cli/update_check.go`, refactor `CheckForUpdatesAsync` to match the pattern used by `StartCompileUpdateCheck` in `compile_update_check.go`: return a `func()` join function that waits up to a short timeout for the goroutine to report. Remove the inline `select { case <-time.After(100*time.Millisecond) }` block from `CheckForUpdatesAsync`. The goroutine should send a boolean (or struct{}) to a buffered channel when complete. The join function should use `select { case <-results: ... case <-time.After(timeout): ... case <-ctx.Done(): }`. Update callers to call the returned function before exiting. Run `make fmt` and unit tests.

---

Task 3: Refactor bootstrap_profile_github_app.go Dual-Channel to errgroup

Priority: Medium
Estimated Effort: Small
Focus Area: Goroutine Lifecycle / bootstrap_profile_github_app.go

Description: createBootstrapGitHubApp in pkg/cli/bootstrap_profile_github_app.go uses separate resultCh chan *bootstrapCreatedGitHubApp and errCh chan error channels plus a manual select. Replace the dual-channel pattern with errgroup.WithContext so the result is captured via a closure variable, matching the established project pattern from audit.go and mcp_inspect_inspector.go.

Acceptance Criteria:

• resultCh and errCh removed in favour of a shared var result *bootstrapCreatedGitHubApp captured by closure
• HTTP server goroutine launched with g.Go(func() error { return server.Serve(listener) }) (or equivalent)
• bootstrapGitHubAppFlowChannels struct removed if it only existed to hold the two channels
• Timeout handled by context.WithTimeout on the errgroup context
• All existing tests pass

Code Region: pkg/cli/bootstrap_profile_github_app.go

In `pkg/cli/bootstrap_profile_github_app.go` function `createBootstrapGitHubApp`, replace the `resultCh`/`errCh` dual-channel pattern with `golang.org/x/sync/errgroup`. Create `g, gctx := errgroup.WithContext(ctx)`. Wrap the HTTP server in `g.Go(func() error { return server.Serve(listener) })`. Remove `bootstrapGitHubAppFlowChannels` if it only held `resultCh`/`errCh`. Update `buildBootstrapGitHubAppMux` callback to store the result in a shared `var createdApp *bootstrapCreatedGitHubApp` via a closure and signal completion by calling `server.Shutdown`. Return `createdApp, g.Wait()` after a `context.WithTimeout` ensures the timeout. Run `make fmt` and tests.

---

Task 4: Add Goroutine Leak Detection to Test Suite

Priority: Medium
Estimated Effort: Medium
Focus Area: Testing / goroutine lifecycle

Description: The test suite has zero goroutine leak detection. go.uber.org/goleak can be added as a test-only dependency and called via goleak.VerifyTestMain(m) in package test main functions for pkg/cli. This would catch any goroutine leak introduced by the fire-and-forget patterns identified above and prevent regressions.

Acceptance Criteria:

• go.uber.org/goleak added to go.mod as a test dependency
• TestMain in pkg/cli (or a new pkg/cli/main_test.go) calls goleak.VerifyTestMain(m)
• Any existing goroutine leaks are either fixed or explicitly suppressed with goleak.IgnoreTopFunction
• CI unit test run passes with leak detection enabled

Code Region: pkg/cli/, go.mod

Add `go.uber.org/goleak` as a test dependency: run `go get go.uber.org/goleak` and ensure it appears under `require` in `go.mod`. Create `pkg/cli/main_test.go` with:
```go
package cli_test

import (
    "testing"
    "go.uber.org/goleak"
)

func TestMain(m *testing.M) {
    goleak.VerifyTestMain(m)
}

Run the tests. For any goroutine that is legitimately fire-and-forget (e.g. the browser launcher goroutine in bootstrap_profile_helpers.go), add goleak.IgnoreTopFunction("os/exec.(*Cmd).Wait") or equivalent. Fix any goroutine leaks revealed by the detector. Run make fmt and make test-unit.

---

#### Task 5: Document Goroutine Lifecycle Pattern in `pkg/cli/doc.go`

**Priority**: Low
**Estimated Effort**: Small
**Focus Area**: Documentation / goroutine conventions

**Description:** `pkg/cli/doc.go` documents command file layout but has no section on concurrency conventions. Add a section that prescribes: (1) use `errgroup` for goroutines that must complete before the function returns, (2) use `conc/pool` for bounded-parallelism fan-out, (3) fire-and-forget goroutines must return a join `func()`, (4) all goroutines must be context-aware. Reference the existing exemplary patterns.

**Acceptance Criteria:**
- [ ] `pkg/cli/doc.go` gains a `Concurrency Conventions` section
- [ ] Section references `audit.go` (errgroup), `logs_run_processor.go` (conc/pool), `compile_update_check.go` (channel + join func)
- [ ] Section explicitly prohibits raw `time.After` sleeps in goroutines without context awareness

**Code Region:** `pkg/cli/doc.go`

```markdown
In `pkg/cli/doc.go`, add a new doc comment section titled `Concurrency Conventions` after the existing architecture documentation. The section should state:
- Goroutines that must complete before a function returns: use `golang.org/x/sync/errgroup` (see `audit.go`)
- Bounded fan-out over a slice of work items: use `sourcegraph/conc/pool` with `WithContext` and `WithMaxGoroutines` (see `logs_run_processor.go`)
- Background goroutines that may outlive the caller: return a `func()` join function so callers can await completion before exiting (see `compile_update_check.go`)
- All goroutines must check `ctx.Done()` at loop boundaries or use context-aware operations
- Never use bare `time.After` in a goroutine without a paired `ctx.Done()` case
Reference the exemplary files by name so future developers can find working examples.

---

📊 Historical Context

Previous Focus Areas (last 5)

Date	Focus Area	Type	Custom	Key Outcomes
2026-07-22	Output Stream Abstraction Gap	Custom	Y	167 os.Stdout refs; 6 renderLogs* without io.Writer
2026-07-17	Error Chain Transparency	Custom	Y	788 non-wrapping fmt.Errorf; 0 errgroup vs 8 goroutines
2026-07-16	Options Struct Pattern Adoption	Custom	Y	2 functions >9 params; 21 at limit
2026-07-15	Compile-Time Interface Assertions	Custom	Y	Zero var _ Interface = (*Type)(nil) guards
2026-07-14	Logger Name Collision	Custom	Y	3 duplicate logger names; 6 var log shadows

---

🎯 Recommendations

Immediate Actions (This Week)

1. Fix StartDockerImagePull to return a join channel — Priority: High
2. Refactor CheckForUpdatesAsync to match StartCompileUpdateCheck pattern — Priority: High

Short-term Actions (This Month)

1. Refactor bootstrap_profile_github_app.go dual-channel to errgroup — Priority: Medium
2. Add goleak.VerifyTestMain to pkg/cli tests — Priority: Medium

Long-term Actions (This Quarter)

1. Document goroutine lifecycle conventions in pkg/cli/doc.go — Priority: Low

---

📈 Success Metrics

• Goroutines with no join point: 2 → 0
• Goroutine patterns using errgroup/conc: 3/7 → 6/7
• Packages with goroutine leak detection: 0 → 1 (pkg/cli)
• Fire-and-forget goroutines (no join func): 2 → 0

---

Next Steps

1. Review and prioritise the tasks above
2. Assign tasks to Copilot coding agent via planner agent
3. Track progress on improvement items
4. Re-evaluate this focus area in 2 weeks

---

References:

• §30096216536

Generated by ⚡ Repository Quality Improvement Agent · sonnet46 · 90.1 AIC · ⌖ 6.53 AIC · ⊞ 6.7K · ◷

• expires on Jul 25, 2026, 5:25 AM UTC-08:00

[github-actions[bot]]

This discussion was automatically closed because it expired on 2026-07-25T13:25:54.974Z.

Closed by Workflow