Add goroutinemissingrecover linter: flag panic-unsafe goroutines

docs/adr/48966-add-goroutinemissingrecover-linter.md

@@ -0,0 +1,44 @@
+# ADR-48966: Add goroutinemissingrecover Linter to Enforce Goroutine Panic Recovery
+
+**Date**: 2026-07-29
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+Goroutines started via function literals (`go func() { ... }()`) that panic will terminate the entire process; unlike panics in the calling goroutine, they cannot be caught by the caller's `recover`. The codebase already has examples of the safe pattern (`pkg/console/spinner.go`, `pkg/cli/docker_images.go`) but the pattern was applied inconsistently — `pkg/cli/forecast_compute.go` launched worker goroutines with no panic protection. Manual code review had not caught this divergence. The existing `pkg/linters/` framework provides the infrastructure for adding custom `go/analysis` passes that are automatically applied across the entire codebase at review time.
+
+### Decision
+
+We will add a new `goroutinemissingrecover` custom `go/analysis` linter under `pkg/linters/goroutinemissingrecover/` that flags any goroutine started via a function literal whose body does not install a top-level `defer func() { recover() }()` guard. The linter uses the existing `nolint.HasDirectiveForLinter` mechanism so call sites that intentionally skip recovery can be explicitly documented with `//nolint:goroutinemissingrecover`. Named-function goroutines (`go f()`) are out of scope because the named function can install its own recovery.
+
+### Alternatives Considered
+
+#### Alternative 1: Rely on Code Review (Manual Enforcement)
+
+Code reviewers catch missing recover guards in goroutine function literals as part of the normal PR review process. This was already the implicit policy; the inconsistency between `forecast_compute.go` and `spinner.go`/`docker_images.go` demonstrates it is insufficient at scale. As the codebase grows, reviewers cannot reliably spot every unguarded goroutine literal across hundreds of files. Manual enforcement does not scale and produces no audit trail.
+
+#### Alternative 2: Use a Third-Party Linter (e.g., `gocritic` or `revive`)
+
+Add a general-purpose linter that has a goroutine-recovery rule, rather than building a bespoke `go/analysis` pass. This avoids new code but introduces a new external dependency for a single semantic rule. Existing third-party rules in this space do not precisely match the required semantics (function-literal goroutines only, integration with the project's `nolint` index and `filecheck` generated-file skip logic). Adapting a third-party rule would require the same investigation effort as writing the custom pass, with less control over the result.
+
+### Consequences
+
+#### Positive
+- Unguarded goroutine function literals are caught at CI time rather than at runtime, preventing process-killing panics from reaching production.
+- The `nolint:goroutinemissingrecover` suppression mechanism creates an explicit, searchable record of every intentional exception to the rule.
+- The linter follows the conventions of existing passes (`astutil.Inspector`, `nolint`, `filecheck`) and is registered automatically via `linters.All()`, requiring no changes to the runner infrastructure.
+
+#### Negative
+- Every new goroutine function literal added to the codebase must include a boilerplate `defer func() { if r := recover(); r != nil { ... } }()` block or an explicit `nolint` directive, increasing the per-goroutine authoring cost.
+- Named-function goroutines (`go f()`) are intentionally out of scope; a developer who refactors a function literal into a named function to avoid the linter may not actually add recovery to the named function, shifting rather than solving the problem.
+
+#### Neutral
+- The analyzer count in `pkg/linters/doc.go` and `pkg/linters/README.md` increments from 59 to 60; the `spec_test.go` documented-analyzer list must be kept in sync whenever a new analyzer is added.
+- Generated files are skipped via `filecheck.ShouldSkipFilename`, consistent with all other custom analyzers in this package.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/bootstrap_profile_github_app.go

@@ -215,6 +215,11 @@ func createBootstrapGitHubApp(ctx context.Context, repo, owner, repoName, ownerT
 		Handler: buildBootstrapGitHubAppMux(ctx, state, appOwner, appOwnerType, appName, description, registrationPage, flowCh),
 	}
 	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				githubAppBootstrapLog.Printf("Panic in GitHub App registration server (recovered): %v", r)
+			}
+		}()
 		_ = server.Serve(listener)
 	}()
 	defer func() {

pkg/cli/bootstrap_profile_helpers.go

@@ -358,6 +358,11 @@ func openBootstrapBrowser(url string) bool {
 		if err := cmd.Start(); err == nil {
 			bootstrapProfileHelpersLog.Printf("Launched browser via %q", args[0])
 			go func() {
+				defer func() {
+					if r := recover(); r != nil {
+						bootstrapProfileHelpersLog.Printf("Panic waiting for browser process (recovered): %v", r)
+					}
+				}()
 				_ = cmd.Wait()
 			}()
 			return true

pkg/cli/forecast_compute.go

@@ -231,6 +231,11 @@ func parallelLoadRunAICs(ctx context.Context, runs []WorkflowRun, config Forecas
 		runID := r.DatabaseID
 		go func() {
 			defer wg.Done()
+			defer func() {
+				if r := recover(); r != nil {
+					forecastRunLog.Printf("Panic in AIC worker for run %d (recovered): %v", runID, r)
+				}
+			}()
 			// Acquire semaphore slot; abort if context is cancelled while waiting.
 			select {
 			case sem <- struct{}{}:
@@ -244,6 +249,11 @@ func parallelLoadRunAICs(ctx context.Context, runs []WorkflowRun, config Forecas
 	}
 
 	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				forecastRunLog.Printf("Panic in AIC results collector (recovered): %v", r)
+			}
+		}()
 		wg.Wait()
 		close(resultsCh)
 	}()

pkg/linters/README.md

@@ -22,6 +22,7 @@ This package currently provides custom Go analyzers in the following subpackages
 - `execcommandwithoutcontext` — reports `exec.Command(...)` calls inside functions that already receive `context.Context` and should use `exec.CommandContext(...)`.
 - `fmterrorfnoverbs` — reports `fmt.Errorf` calls whose format string contains no verbs, recommending `errors.New` instead.
 - `fprintlnsprintf` — reports `fmt.Fprintln(..., fmt.Sprintf(...))` patterns and recommends direct formatting calls.
+- `goroutinemissingrecover` — reports goroutines started via a function literal whose body does not install a top-level `defer func() { recover() }()` guard.
 - `hardcodedfilepath` — reports hard-coded file path string literals that match known path constants or should be extracted into named constants; also annotates paths that appear in log/print calls.
 - `httpnoctx` — reports HTTP client and package-level HTTP calls that do not accept a `context.Context`.
 - `httprespbodyclose` — reports HTTP responses whose `Body.Close()` call is missing or not deferred.
@@ -89,6 +90,7 @@ This package currently provides custom Go analyzers in the following subpackages
 | `fileclosenotdeferred` | Custom `go/analysis` analyzer that flags file `Close()` calls that are not deferred immediately |
 | `fmterrorfnoverbs` | Custom `go/analysis` analyzer that flags `fmt.Errorf` calls with no format verbs, recommending `errors.New` |
 | `fprintlnsprintf` | Custom `go/analysis` analyzer that flags `fmt.Fprintln(..., fmt.Sprintf(...))` patterns |
+| `goroutinemissingrecover` | Custom `go/analysis` analyzer that flags goroutines started via a function literal that do not install a top-level defer/recover guard |
 | `hardcodedfilepath` | Custom `go/analysis` analyzer that flags hard-coded file path string literals that match known path constants or should be extracted as named constants; annotates paths in log/print calls |
 | `httpnoctx` | Custom `go/analysis` analyzer that flags HTTP calls that do not accept a `context.Context` |
 | `httprespbodyclose` | Custom `go/analysis` analyzer that flags HTTP response bodies that are not closed (or not deferred) |

pkg/linters/doc.go

@@ -1,6 +1,6 @@
 // Package linters is a namespace for gh-aw's custom Go analysis linters.
 //
-// All 59 active analyzers:
+// All 60 active analyzers:
 //
 //   - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...)
 //   - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x)
@@ -18,6 +18,7 @@
 //   - fileclosenotdeferred — flags file Close() calls that are not deferred
 //   - fmterrorfnoverbs — flags fmt.Errorf calls with no format verbs, recommending errors.New
 //   - fprintlnsprintf — flags fmt.Fprintln(..., fmt.Sprintf(...)) patterns
+//   - goroutinemissingrecover — flags goroutines started via a function literal whose body does not install a top-level defer/recover guard
 //   - hardcodedfilepath — flags hard-coded file path string literals that match known path constants or should be extracted as named constants
 //   - httpnoctx — flags HTTP calls that do not accept a context.Context
 //   - httprespbodyclose — flags HTTP response bodies that are not closed

pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go

@@ -0,0 +1,158 @@
+// Package goroutinemissingrecover implements a Go analysis linter that flags
+// goroutines started via a function literal whose body does not install a
+// top-level defer/recover guard.
+//
+// An unrecovered panic inside a goroutine terminates the entire process and
+// is not caught by the caller's recover, so any goroutine that might panic
+// should defer a recover to contain the failure locally.
+//
+// Only goroutines launched with a function literal (`go func() { ... }()`)
+// are checked. Goroutines that call a named function (`go f()`) are out of
+// scope because the named function can install its own recovery.
+package goroutinemissingrecover
+
+import (
+	"go/ast"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
+	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
+	"github.com/github/gh-aw/pkg/linters/internal/nolint"
+	"github.com/github/gh-aw/pkg/logger"
+)
+
+var pkgLog = logger.New("linters:goroutinemissingrecover")
+
+// Analyzer is the goroutine-missing-recover analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "goroutinemissingrecover",
+	Doc:      "reports goroutines started via a function literal that do not install a top-level defer/recover guard",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/goroutinemissingrecover",
+	Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	pkgLog.Printf("analyzing package %s", pass.Pkg.Path())
+
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
+	}
+	noLintIndex, err := nolint.Index(pass)
+	if err != nil {
+		return nil, err
+	}
+	generatedFiles, err := filecheck.Index(pass)
+	if err != nil {
+		return nil, err
+	}
+
+	nodeFilter := []ast.Node{(*ast.GoStmt)(nil)}
+	insp.Preorder(nodeFilter, func(n ast.Node) {
+		goStmt, ok := n.(*ast.GoStmt)
+		if !ok {
+			return
+		}
+
+		// Only flag goroutines started with a function literal, not named functions.
+		// Unwrap parentheses: go (func() { ... })() is equivalent to go func() { ... }()
+		call, ok := unwrapParens(goStmt.Call.Fun).(*ast.FuncLit)
+		if !ok {
+			return
+		}
+
+		position := pass.Fset.PositionFor(goStmt.Pos(), false)
+		if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) {
+			return
+		}
+
+		if nolint.HasDirectiveForLinter(position, noLintIndex, "goroutinemissingrecover") {
+			return
+		}
+
+		if hasTopLevelRecoverDefer(call.Body, pass.TypesInfo) {
+			return
+		}
+
+		pkgLog.Printf("flagging goroutine without recover at %s", position)
+		pass.ReportRangef(goStmt, "goroutine launched via a function literal without a top-level defer/recover; add defer func() { if r := recover(); r != nil { ... } }() to contain panics")
+	})
+
+	return nil, nil
+}
+
+// unwrapParens removes any surrounding *ast.ParenExpr nodes, returning the
+// innermost non-parenthesised expression. This handles the rare but valid
+// syntax `go (func() { ... })()`.
+func unwrapParens(expr ast.Expr) ast.Expr {
+	for {
+		p, ok := expr.(*ast.ParenExpr)
+		if !ok {
+			return expr
+		}
+		expr = p.X
+	}
+}
+
+// hasTopLevelRecoverDefer reports whether body contains a top-level defer
+// statement whose call is a function literal that itself calls recover().
+// Only the direct statements of body are examined; nested function bodies are
+// not descended into.
+func hasTopLevelRecoverDefer(body *ast.BlockStmt, typesInfo *types.Info) bool {
+	if body == nil {
+		return false
+	}
+	for _, stmt := range body.List {
+		deferStmt, ok := stmt.(*ast.DeferStmt)
+		if !ok {
+			continue
+		}
+		// Unwrap parentheses: defer (func() { ... })() is valid Go.
+		fn, ok := unwrapParens(deferStmt.Call.Fun).(*ast.FuncLit)
+		if !ok {
+			continue
+		}
+		if containsRecoverCall(fn.Body, typesInfo) {
+			return true
+		}
+	}
+	return false
+}
+
+// containsRecoverCall reports whether body contains a direct call to the
+// built-in recover() function. Nested function literals are not descended
+// into: recover() inside a nested closure only guards that closure's stack
+// frame, not the enclosing defer, so it does not count as a panic guard.
+func containsRecoverCall(body *ast.BlockStmt, typesInfo *types.Info) bool {
+	found := false
+	ast.Inspect(body, func(n ast.Node) bool {
+		if found {
+			return false
+		}
+		// Do not descend into nested function literals — their recover() only
+		// protects the nested function's own stack frame, not the outer defer.
+		if _, ok := n.(*ast.FuncLit); ok {
+			return false
+		}
+		call, ok := n.(*ast.CallExpr)
+		if !ok {
+			return true
+		}
+		ident, ok := call.Fun.(*ast.Ident)
+		if !ok {
+			return true
+		}
+		// Verify it is the built-in recover, not a user-defined function with
+		// the same name (matching the pattern used by mapclearloop for delete).
+		if obj, isBuiltin := typesInfo.Uses[ident].(*types.Builtin); isBuiltin && obj.Name() == "recover" {
+			found = true
+			return false
+		}
+		return true
+	})
+	return found
+}

pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go

@@ -0,0 +1,16 @@
+//go:build !integration
+
+// Package goroutinemissingrecover_test provides tests for the goroutinemissingrecover analyzer.
+package goroutinemissingrecover_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/github/gh-aw/pkg/linters/goroutinemissingrecover"
+)
+
+func TestGoroutineMissingRecover(t *testing.T) {
+	analysistest.Run(t, analysistest.TestData(), goroutinemissingrecover.Analyzer, "a", "b")
+}

pkg/linters/goroutinemissingrecover/testdata/src/a/a.go

@@ -0,0 +1,84 @@
+// Package a is the test fixture for the goroutinemissingrecover analyzer.
+package a
+
+// safeGoroutine has a top-level defer/recover — no diagnostic expected.
+func safeGoroutine() {
+	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				_ = r
+			}
+		}()
+		panic("oops")
+	}()
+}
+
+// unsafeGoroutine has no recover — should be flagged.
+func unsafeGoroutine() {
+	go func() { // want `goroutine launched via a function literal without a top-level defer/recover`
+		panic("oops")
+	}()
+}
+
+// namedFuncGoroutine calls a named function — out of scope, not flagged.
+func namedFuncHelper() {}
+
+func namedFuncGoroutine() {
+	go namedFuncHelper()
+}
+
+// unrelatedDeferGoroutine has a defer but no recover — should be flagged.
+func unrelatedDeferGoroutine() {
+	go func() { // want `goroutine launched via a function literal without a top-level defer/recover`
+		defer func() {
+			_ = 1
+		}()
+		panic("oops")
+	}()
+}
+
+// suppressedGoroutine is suppressed via nolint — no diagnostic expected.
+func suppressedGoroutine() {
+	//nolint:goroutinemissingrecover
+	go func() {
+		panic("oops")
+	}()
+}
+
+// parenthesizedGoroutine uses the parenthesised literal syntax — should be flagged.
+func parenthesizedGoroutine() {
+	go (func() { // want `goroutine launched via a function literal without a top-level defer/recover`
+		panic("oops")
+	})()
+}
+
+// nestedClosureRecoverGoroutine has recover() buried in a nested closure inside the
+// defer literal. recover() only stops a panic in the same stack frame, so the
+// nested recover does NOT protect the goroutine — should be flagged.
+func nestedClosureRecoverGoroutine() {
+	go func() { // want `goroutine launched via a function literal without a top-level defer/recover`
+		defer func() {
+			// recover() is inside a nested closure — it only guards the nested call,
+			// not the outer goroutine.
+			func() { recover() }()
+		}()
+		panic("oops")
+	}()
+}
+
+// nestedRecoverOnlyGoroutine — outer goroutine has no recover; inner func does.
+// The outer goroutine is unprotected — should be flagged.
+func nestedRecoverOnlyGoroutine() {
+	go func() { // want `goroutine launched via a function literal without a top-level defer/recover`
+		func() {
+			defer func() {
+				if r := recover(); r != nil {
+					_ = r
+				}
+			}()
+			panic("inner")
+		}()
+		panic("outer") // this panic is unrecovered
+	}()
+}
+