From 5deb90ce0179e3efccab86755d4e10b0abe4ca7a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:02:15 +0000 Subject: [PATCH 1/4] Initial plan From da9d56ad0fc81aa54df64fa9030725401872f760 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:15:28 +0000 Subject: [PATCH 2/4] Add goroutinemissingrecover linter: flag panic-unsafe goroutines Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/README.md | 2 + pkg/linters/doc.go | 3 +- .../goroutinemissingrecover.go | 130 ++++++++++++++++++ .../goroutinemissingrecover_test.go | 16 +++ .../testdata/src/a/a.go | 46 +++++++ pkg/linters/registry.go | 2 + pkg/linters/spec_test.go | 6 +- 7 files changed, 202 insertions(+), 3 deletions(-) create mode 100644 pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go create mode 100644 pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go create mode 100644 pkg/linters/goroutinemissingrecover/testdata/src/a/a.go diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 8ec9dd722d0..949c27a0938 100644 --- a/pkg/linters/README.md +++ b/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) | diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index a99645363fb..4194977f888 100644 --- a/pkg/linters/doc.go +++ b/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 diff --git a/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go new file mode 100644 index 00000000000..a0e9e02831c --- /dev/null +++ b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go @@ -0,0 +1,130 @@ +// 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" + + "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. + call, ok := 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) { + 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 +} + +// 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) bool { + if body == nil { + return false + } + for _, stmt := range body.List { + deferStmt, ok := stmt.(*ast.DeferStmt) + if !ok { + continue + } + fn, ok := deferStmt.Call.Fun.(*ast.FuncLit) + if !ok { + continue + } + if containsRecoverCall(fn.Body) { + return true + } + } + return false +} + +// containsRecoverCall reports whether body contains a call to the built-in +// recover() function at any depth. +func containsRecoverCall(body *ast.BlockStmt) bool { + found := false + ast.Inspect(body, func(n ast.Node) bool { + if found { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + ident, ok := call.Fun.(*ast.Ident) + if ok && ident.Name == "recover" { + found = true + return false + } + return true + }) + return found +} diff --git a/pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go new file mode 100644 index 00000000000..c6b48a33376 --- /dev/null +++ b/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") +} diff --git a/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go b/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go new file mode 100644 index 00000000000..7988600ae4d --- /dev/null +++ b/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go @@ -0,0 +1,46 @@ +// 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") + }() +} diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 273162c4086..666261a9efe 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -19,6 +19,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" + "github.com/github/gh-aw/pkg/linters/goroutinemissingrecover" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" "github.com/github/gh-aw/pkg/linters/httprespbodyclose" @@ -91,6 +92,7 @@ var allAnalyzers = []*analysis.Analyzer{ fileclosenotdeferred.Analyzer, fmterrorfnoverbs.Analyzer, hardcodedfilepath.Analyzer, + goroutinemissingrecover.Analyzer, httpnoctx.Analyzer, httprespbodyclose.Analyzer, ioutildeprecated.Analyzer, diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 1a0c2fb898a..6fb3590b4fd 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -27,6 +27,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" + "github.com/github/gh-aw/pkg/linters/goroutinemissingrecover" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" "github.com/github/gh-aw/pkg/linters/httprespbodyclose" @@ -85,7 +86,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 59 analyzers +// "Public API > Subpackages" table. The README documents 60 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -93,7 +94,7 @@ type docAnalyzer struct { // // appendbytestring, appendoneelement, bytesbufferstring, bytescomparestring, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage, // errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, -// hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, +// goroutinemissingrecover, hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, @@ -116,6 +117,7 @@ func documentedAnalyzers() []docAnalyzer { {"fileclosenotdeferred", fileclosenotdeferred.Analyzer}, {"fmterrorfnoverbs", fmterrorfnoverbs.Analyzer}, {"fprintlnsprintf", fprintlnsprintf.Analyzer}, + {"goroutinemissingrecover", goroutinemissingrecover.Analyzer}, {"hardcodedfilepath", hardcodedfilepath.Analyzer}, {"httpnoctx", httpnoctx.Analyzer}, {"httprespbodyclose", httprespbodyclose.Analyzer}, From 286acdb987b8c7fa71a87c7e91efaf46485a615f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:52:38 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-48966 for goroutinemissingrecover linter Co-Authored-By: Claude Sonnet 4.6 --- ...8966-add-goroutinemissingrecover-linter.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/adr/48966-add-goroutinemissingrecover-linter.md diff --git a/docs/adr/48966-add-goroutinemissingrecover-linter.md b/docs/adr/48966-add-goroutinemissingrecover-linter.md new file mode 100644 index 00000000000..25b2d9ff54e --- /dev/null +++ b/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.* From b92a5b3f9d24b671db7bc7a9adea7e6f00b318dc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:48:03 +0000 Subject: [PATCH 4/4] fix(goroutinemissingrecover): address review feedback - nested closures, type safety, ordering, production goroutines Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/bootstrap_profile_github_app.go | 5 ++ pkg/cli/bootstrap_profile_helpers.go | 5 ++ pkg/cli/forecast_compute.go | 10 ++++ .../goroutinemissingrecover.go | 46 +++++++++++++++---- .../goroutinemissingrecover_test.go | 2 +- .../testdata/src/a/a.go | 38 +++++++++++++++ .../testdata/src/b/b.go | 18 ++++++++ pkg/linters/registry.go | 2 +- 8 files changed, 115 insertions(+), 11 deletions(-) create mode 100644 pkg/linters/goroutinemissingrecover/testdata/src/b/b.go diff --git a/pkg/cli/bootstrap_profile_github_app.go b/pkg/cli/bootstrap_profile_github_app.go index 31bb7318eb3..135081394c0 100644 --- a/pkg/cli/bootstrap_profile_github_app.go +++ b/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() { diff --git a/pkg/cli/bootstrap_profile_helpers.go b/pkg/cli/bootstrap_profile_helpers.go index fef56146fea..7bb6e0d8a4e 100644 --- a/pkg/cli/bootstrap_profile_helpers.go +++ b/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 diff --git a/pkg/cli/forecast_compute.go b/pkg/cli/forecast_compute.go index 1ef5ddaa74f..e435278cce0 100644 --- a/pkg/cli/forecast_compute.go +++ b/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) }() diff --git a/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go index a0e9e02831c..37eeedbe76b 100644 --- a/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go +++ b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go @@ -13,6 +13,7 @@ package goroutinemissingrecover import ( "go/ast" + "go/types" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -58,7 +59,8 @@ func run(pass *analysis.Pass) (any, error) { } // Only flag goroutines started with a function literal, not named functions. - call, ok := goStmt.Call.Fun.(*ast.FuncLit) + // Unwrap parentheses: go (func() { ... })() is equivalent to go func() { ... }() + call, ok := unwrapParens(goStmt.Call.Fun).(*ast.FuncLit) if !ok { return } @@ -72,7 +74,7 @@ func run(pass *analysis.Pass) (any, error) { return } - if hasTopLevelRecoverDefer(call.Body) { + if hasTopLevelRecoverDefer(call.Body, pass.TypesInfo) { return } @@ -83,11 +85,24 @@ func run(pass *analysis.Pass) (any, error) { 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) bool { +func hasTopLevelRecoverDefer(body *ast.BlockStmt, typesInfo *types.Info) bool { if body == nil { return false } @@ -96,31 +111,44 @@ func hasTopLevelRecoverDefer(body *ast.BlockStmt) bool { if !ok { continue } - fn, ok := deferStmt.Call.Fun.(*ast.FuncLit) + // Unwrap parentheses: defer (func() { ... })() is valid Go. + fn, ok := unwrapParens(deferStmt.Call.Fun).(*ast.FuncLit) if !ok { continue } - if containsRecoverCall(fn.Body) { + if containsRecoverCall(fn.Body, typesInfo) { return true } } return false } -// containsRecoverCall reports whether body contains a call to the built-in -// recover() function at any depth. -func containsRecoverCall(body *ast.BlockStmt) bool { +// 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 && ident.Name == "recover" { + 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 } diff --git a/pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go index c6b48a33376..261e4808f2f 100644 --- a/pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go +++ b/pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go @@ -12,5 +12,5 @@ import ( ) func TestGoroutineMissingRecover(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), goroutinemissingrecover.Analyzer, "a") + analysistest.Run(t, analysistest.TestData(), goroutinemissingrecover.Analyzer, "a", "b") } diff --git a/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go b/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go index 7988600ae4d..f7ebe3ed023 100644 --- a/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go +++ b/pkg/linters/goroutinemissingrecover/testdata/src/a/a.go @@ -44,3 +44,41 @@ func suppressedGoroutine() { 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 + }() +} + diff --git a/pkg/linters/goroutinemissingrecover/testdata/src/b/b.go b/pkg/linters/goroutinemissingrecover/testdata/src/b/b.go new file mode 100644 index 00000000000..38cedeb7600 --- /dev/null +++ b/pkg/linters/goroutinemissingrecover/testdata/src/b/b.go @@ -0,0 +1,18 @@ +// Package b tests the user-defined-recover false-negative guard in isolation. +// By declaring a package-level function named recover() we shadow the built-in; +// the linter must not accept that as a valid panic guard. +package b + +// recover shadows the built-in recover. //nolint:predeclared +func recover() {} + +// userDefinedRecoverGoroutine uses the local recover() (not the built-in) inside +// the defer literal — the goroutine is still unprotected and must be flagged. +func userDefinedRecoverGoroutine() { + go func() { // want `goroutine launched via a function literal without a top-level defer/recover` + defer func() { + recover() // calls the local func, not the built-in + }() + panic("oops") + }() +} diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 666261a9efe..5406f94494c 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -91,8 +91,8 @@ var allAnalyzers = []*analysis.Analyzer{ excessivefuncparams.Analyzer, fileclosenotdeferred.Analyzer, fmterrorfnoverbs.Analyzer, - hardcodedfilepath.Analyzer, goroutinemissingrecover.Analyzer, + hardcodedfilepath.Analyzer, httpnoctx.Analyzer, httprespbodyclose.Analyzer, ioutildeprecated.Analyzer,