Skip to content

Commit 62e4f37

Browse files
linter: add timeafterleak — flag time.After in for+select cases
Adds a new custom go/analysis linter that flags time.After(...) calls used as the channel-receive expression in a select CommClause inside a for or range loop. ## What it catches time.After(d) creates a new *time.Timer whose goroutine keeps running until the timer fires, even if another select case is chosen first. In a hot loop this causes a timer goroutine leak that accumulates every iteration: for { select { case <-time.After(timeout): // ← flagged: new timer every iteration ... case <-ctx.Done(): return } } The correct pattern is to allocate time.NewTimer once outside the loop and Reset/Stop it each iteration. ## Evidence Found in 3 production sites during code scanning: - pkg/cli/add_interactive_workflow.go - pkg/cli/docker_images.go - pkg/cli/mcp_inspect_mcp_scripts_server.go ## Implementation notes - Uses the inspector Cursor API (cur.Parent() + clauseCur.Enclosing()) to precisely detect only Comm-position receives, not receives in case-body statements. - Stops at FuncLit/FuncDecl boundaries so goroutine closures launched in a loop are not false-positively flagged. - Supports //nolint:timeafterleak suppression. - Registered in cmd/linters/main.go alongside existing analyzers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6c0f459 commit 62e4f37

4 files changed

Lines changed: 254 additions & 0 deletions

File tree

cmd/linters/main.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import (
4040
"github.com/github/gh-aw/pkg/linters/sortslice"
4141
"github.com/github/gh-aw/pkg/linters/ssljson"
4242
"github.com/github/gh-aw/pkg/linters/strconvparseignorederror"
43+
"github.com/github/gh-aw/pkg/linters/timeafterleak"
4344
"github.com/github/gh-aw/pkg/linters/timesleepnocontext"
4445
"github.com/github/gh-aw/pkg/linters/tolowerequalfold"
4546
"github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion"
@@ -71,6 +72,7 @@ func main() {
7172
strconvparseignorederror.Analyzer,
7273
jsonmarshalignoredeerror.Analyzer,
7374
lenstringzero.Analyzer,
75+
timeafterleak.Analyzer,
7476
timesleepnocontext.Analyzer,
7577
tolowerequalfold.Analyzer,
7678
uncheckedtypeassertion.Analyzer,
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
package timeafterleak
2+
3+
import (
4+
"context"
5+
"time"
6+
)
7+
8+
// BadForLoop is the canonical timer-leak: time.After in a for+select Comm.
9+
func BadForLoop(ctx context.Context) {
10+
for {
11+
select {
12+
case <-time.After(time.Second): // want `time\.After creates a new timer on each loop iteration`
13+
doWork()
14+
case <-ctx.Done():
15+
return
16+
}
17+
}
18+
}
19+
20+
// BadForLoopAssign also leaks: the receive is the Comm of the case clause.
21+
func BadForLoopAssign(ctx context.Context) {
22+
for {
23+
select {
24+
case t := <-time.After(time.Second): // want `time\.After creates a new timer on each loop iteration`
25+
_ = t
26+
case <-ctx.Done():
27+
return
28+
}
29+
}
30+
}
31+
32+
// BadRangeLoop flags time.After in a range-based loop select.
33+
func BadRangeLoop(items []string, ctx context.Context) {
34+
for range items {
35+
select {
36+
case <-time.After(time.Millisecond): // want `time\.After creates a new timer on each loop iteration`
37+
case <-ctx.Done():
38+
return
39+
}
40+
}
41+
}
42+
43+
// GoodNoLoop is fine: time.After in a select that is not inside a loop.
44+
func GoodNoLoop(ctx context.Context) {
45+
select {
46+
case <-time.After(time.Second):
47+
doWork()
48+
case <-ctx.Done():
49+
return
50+
}
51+
}
52+
53+
// GoodNewTimer uses the correct pattern: a single timer reused each iteration.
54+
func GoodNewTimer(ctx context.Context) {
55+
t := time.NewTimer(time.Second)
56+
defer t.Stop()
57+
for {
58+
if !t.Stop() {
59+
select {
60+
case <-t.C:
61+
default:
62+
}
63+
}
64+
t.Reset(time.Second)
65+
select {
66+
case <-t.C:
67+
doWork()
68+
case <-ctx.Done():
69+
return
70+
}
71+
}
72+
}
73+
74+
// GoodTimeAfterInBody calls time.After inside the case body, not as the Comm.
75+
// The timer fires exactly once here, so there is no accumulation leak.
76+
func GoodTimeAfterInBody(ctx context.Context, ch <-chan struct{}) {
77+
for {
78+
select {
79+
case <-ch:
80+
<-time.After(time.Second) // in Body, not Comm — not flagged
81+
case <-ctx.Done():
82+
return
83+
}
84+
}
85+
}
86+
87+
// GoodFuncLitInsideLoop: the select is inside a goroutine closure launched
88+
// per iteration; the for loop does not directly enclose the CommClause.
89+
func GoodFuncLitInsideLoop(ctx context.Context) {
90+
for {
91+
go func() {
92+
select {
93+
case <-time.After(time.Second): // FuncLit boundary — not flagged
94+
case <-ctx.Done():
95+
}
96+
}()
97+
<-ctx.Done()
98+
return
99+
}
100+
}
101+
102+
func doWork() {}
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
// Package timeafterleak implements a Go analysis linter that flags
2+
// time.After calls used as select case channel receives inside loops,
3+
// which allocate a new timer on every iteration that is not garbage
4+
// collected until it fires when another case is selected first.
5+
package timeafterleak
6+
7+
import (
8+
"go/ast"
9+
"go/token"
10+
"go/types"
11+
12+
"golang.org/x/tools/go/analysis"
13+
"golang.org/x/tools/go/analysis/passes/inspect"
14+
"golang.org/x/tools/go/ast/inspector"
15+
16+
"github.com/github/gh-aw/pkg/linters/internal/astutil"
17+
"github.com/github/gh-aw/pkg/linters/internal/filecheck"
18+
"github.com/github/gh-aw/pkg/linters/internal/nolint"
19+
)
20+
21+
// Analyzer is the time-after-leak analysis pass.
22+
var Analyzer = &analysis.Analyzer{
23+
Name: "timeafterleak",
24+
Doc: "reports time.After calls in select cases inside loops that leak a timer goroutine on each iteration when another case fires first",
25+
URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/timeafterleak",
26+
Requires: []*analysis.Analyzer{inspect.Analyzer},
27+
Run: run,
28+
}
29+
30+
func run(pass *analysis.Pass) (any, error) {
31+
insp, err := astutil.Inspector(pass)
32+
if err != nil {
33+
return nil, err
34+
}
35+
noLintLinesByFile := nolint.BuildLineIndex(pass, "timeafterleak")
36+
37+
for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) {
38+
call, ok := cur.Node().(*ast.CallExpr)
39+
if !ok {
40+
continue
41+
}
42+
if !isTimeAfterCall(pass, call) {
43+
continue
44+
}
45+
46+
pos := pass.Fset.PositionFor(call.Pos(), false)
47+
if filecheck.IsTestFile(pos.Filename) {
48+
continue
49+
}
50+
if nolint.HasDirective(pos, noLintLinesByFile) {
51+
continue
52+
}
53+
54+
if !isInsideLoopSelectComm(cur) {
55+
continue
56+
}
57+
58+
pass.ReportRangef(call,
59+
"time.After creates a new timer on each loop iteration that is not garbage collected until it fires; use time.NewTimer with Reset and Stop instead")
60+
}
61+
62+
return nil, nil
63+
}
64+
65+
// isTimeAfterCall reports whether call is an invocation of time.After.
66+
func isTimeAfterCall(pass *analysis.Pass, call *ast.CallExpr) bool {
67+
sel, ok := call.Fun.(*ast.SelectorExpr)
68+
if !ok || sel.Sel.Name != "After" {
69+
return false
70+
}
71+
ident, ok := sel.X.(*ast.Ident)
72+
if !ok {
73+
return false
74+
}
75+
obj := pass.TypesInfo.ObjectOf(ident)
76+
if obj == nil {
77+
return false
78+
}
79+
pkgName, ok := obj.(*types.PkgName)
80+
if !ok {
81+
return false
82+
}
83+
return pkgName.Imported().Path() == "time"
84+
}
85+
86+
// isInsideLoopSelectComm reports whether cur is a time.After call used as the
87+
// channel receive expression in the Comm field of a select CommClause that is
88+
// enclosed by a for or range loop, without crossing a function literal boundary.
89+
func isInsideLoopSelectComm(cur inspector.Cursor) bool {
90+
// The immediate parent of time.After(...) must be a channel-receive UnaryExpr.
91+
recvCur := cur.Parent()
92+
unary, ok := recvCur.Node().(*ast.UnaryExpr)
93+
if !ok || unary.Op != token.ARROW {
94+
return false
95+
}
96+
97+
// The parent of the receive expression must be the Comm statement of a CommClause.
98+
// Comm is an ExprStmt (case <-ch:) or AssignStmt (case v := <-ch:).
99+
commStmtCur := recvCur.Parent()
100+
var commStmt ast.Stmt
101+
switch s := commStmtCur.Node().(type) {
102+
case *ast.ExprStmt:
103+
commStmt = s
104+
case *ast.AssignStmt:
105+
commStmt = s
106+
default:
107+
return false
108+
}
109+
110+
// The parent of the Comm statement must be a CommClause, and commStmt must
111+
// be the Comm field (not a statement in the Body).
112+
clauseCur := commStmtCur.Parent()
113+
cc, ok := clauseCur.Node().(*ast.CommClause)
114+
if !ok || cc.Comm != commStmt {
115+
return false
116+
}
117+
118+
// Walk up from the CommClause to find an enclosing for or range loop,
119+
// stopping at any function literal or declaration boundary.
120+
for encl := range clauseCur.Enclosing(
121+
(*ast.ForStmt)(nil),
122+
(*ast.RangeStmt)(nil),
123+
(*ast.FuncLit)(nil),
124+
(*ast.FuncDecl)(nil),
125+
) {
126+
switch encl.Node().(type) {
127+
case *ast.ForStmt, *ast.RangeStmt:
128+
return true
129+
case *ast.FuncLit, *ast.FuncDecl:
130+
return false
131+
}
132+
}
133+
return false
134+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
//go:build !integration
2+
3+
package timeafterleak_test
4+
5+
import (
6+
"testing"
7+
8+
"golang.org/x/tools/go/analysis/analysistest"
9+
10+
"github.com/github/gh-aw/pkg/linters/timeafterleak"
11+
)
12+
13+
func TestAnalyzer(t *testing.T) {
14+
testdata := analysistest.TestData()
15+
analysistest.Run(t, testdata, timeafterleak.Analyzer, "timeafterleak")
16+
}

0 commit comments

Comments
 (0)