[linter-miner] linter: add timeafterleak — flag time.After in for+select cases #39133
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # ADR-39133: Ship a Custom `timeafterleak` Linter for `time.After` in Loop Selects | ||
|
|
||
| **Date**: 2026-06-13 | ||
| **Status**: Draft | ||
|
|
||
| ## Context | ||
|
|
||
| `time.After(d)` allocates a fresh `*time.Timer` (and its backing runtime timer) on every call; the timer is not reclaimed until it fires. When a `time.After` call is the channel-receive of a `select` case inside a `for`/`range` loop, every iteration where another case fires first leaves a timer alive until its full duration elapses, leaking goroutine/heap resources in hot loops. This is a documented Go gotcha but is not flagged by any commonly enabled linter: `staticcheck` SA6001 only covers `time.Tick`, not `time.After` in select cases. A code scan of this repository found three production occurrences of the pattern, so the project needs an automated guard that fits its existing in-repo linter suite (`pkg/linters/*` registered in `cmd/linters/main.go`). | ||
|
|
||
| ## Decision | ||
|
|
||
| We will add a bespoke `go/analysis` analyzer, `timeafterleak`, to the project's custom linter suite rather than relying on an external linter. The analyzer uses `inspector.Cursor` parent-chain traversal to flag a `time.After` call only when it is the `Comm` receive expression of a `select` `CommClause` enclosed by a `for` or `range` loop, stopping at `FuncLit`/`FuncDecl` boundaries and verifying the `time` package identity via type information rather than a syntactic name match. It honors `//nolint:timeafterleak` suppression and skips test files, and is registered in the multichecker in `cmd/linters/main.go`. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| ### Alternative 1: Rely on `staticcheck` / `golangci-lint` defaults | ||
| The project already runs standard linters, so extending their configuration would avoid new code. Rejected because no default rule (including SA6001) detects `time.After` inside select cases; the specific leak pattern would go uncaught. | ||
|
|
||
| ### Alternative 2: Document the gotcha and rely on code review | ||
| A contributor guideline plus manual review requires no tooling. Rejected because manual review is unreliable for an easily overlooked pattern — three instances already reached production — and provides no enforceable, repeatable guard. | ||
|
|
||
| ### Alternative 3: Use a syntactic (string-match) detector instead of type-checked detection | ||
| A simpler analyzer could match the identifier text `time.After` directly. Rejected because it would produce false positives for shadowed identifiers or unrelated packages; type-based identity checking is more precise at modest extra complexity. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| - Catches a real, otherwise-undetected resource-leak pattern automatically in CI. | ||
| - Follows the established in-repo linter convention, so it composes with the existing `cmd/linters` multichecker and shared `internal` helpers. | ||
| - Precise: type-checked `time` identity and Comm-position scoping minimize false positives; `FuncLit` boundary handling avoids flagging per-iteration goroutine closures. | ||
|
|
||
| ### Negative | ||
| - Adds a custom analyzer that the team must maintain, including keeping pace with `go/ast`/`golang.org/x/tools` API changes. | ||
| - Narrow scope: only flags the loop-select Comm position, so other `time.After` misuse patterns remain uncaught and may create a false sense of full coverage. | ||
|
|
||
| ### Neutral | ||
| - Suppression is available via `//nolint:timeafterleak` for intentional cases. | ||
| - Test files are excluded from analysis, matching the suite's existing conventions. | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27475470275) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
Binary file not shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| package timeafterleak | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
| ) | ||
|
|
||
| // BadForLoop is the canonical timer-leak: time.After in a for+select Comm. | ||
| func BadForLoop(ctx context.Context) { | ||
| for { | ||
| select { | ||
| case <-time.After(time.Second): // want `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` | ||
| doWork() | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // BadForLoopAssign also leaks: the receive is the Comm of the case clause. | ||
|
Comment on lines
+19
to
+20
Contributor
There was a problem hiding this comment. Added in the latest commit: |
||
| func BadForLoopAssign(ctx context.Context) { | ||
| for { | ||
| select { | ||
| case t := <-time.After(time.Second): // want `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` | ||
| _ = t | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // BadRangeLoop flags time.After in a range-based loop select. | ||
| func BadRangeLoop(items []string, ctx context.Context) { | ||
| for range items { | ||
| select { | ||
| case <-time.After(time.Millisecond): // want `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` | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] Two fixture gaps: no 💡 Suggested additionsOther linters in this repo (e.g. // GoodNolintDirective: suppressed with a nolint directive — not flagged.
func GoodNolintDirective(ctx context.Context) {
for {
select {
case <-time.After(time.Second): (nolint/redacted):timeafterleak
doWork()
case <-ctx.Done():
return
}
}
}A nested-loop case would also verify that the inner CommClause is correctly flagged regardless of nesting depth: // BadNestedLoop: inner select is still inside a loop.
func BadNestedLoop(ctx context.Context) {
for {
for {
select {
case <-time.After(time.Second): // want `time\.After creates a new timer`
case <-ctx.Done():
return
}
}
}
}
Contributor
There was a problem hiding this comment. Both added: |
||
| } | ||
|
|
||
| // BadNestedLoop: the inner select is still inside a loop regardless of nesting depth. | ||
| func BadNestedLoop(ctx context.Context) { | ||
| for { | ||
| for { | ||
| select { | ||
| case <-time.After(time.Second): // want `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` | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // BadSingleCaseWithDefault: a default clause can preempt the timer — still flagged. | ||
| func BadSingleCaseWithDefault(ctx context.Context) { | ||
| for { | ||
| select { | ||
| case <-time.After(time.Second): // want `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` | ||
| default: | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // GoodNoLoop is fine: time.After in a select that is not inside a loop. | ||
| func GoodNoLoop(ctx context.Context) { | ||
| select { | ||
| case <-time.After(time.Second): | ||
| doWork() | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // GoodNewTimer uses the correct pattern: a single timer reused each iteration. | ||
| func GoodNewTimer(ctx context.Context) { | ||
| t := time.NewTimer(time.Second) | ||
| defer t.Stop() | ||
| for { | ||
| if !t.Stop() { | ||
| select { | ||
| case <-t.C: | ||
| default: | ||
| } | ||
| } | ||
| t.Reset(time.Second) | ||
| select { | ||
| case <-t.C: | ||
| doWork() | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // GoodTimeAfterInBody calls time.After inside the case body, not as the Comm. | ||
| // When time.After is used in the case body rather than as the Comm expression, | ||
| // the goroutine blocks on the receive until the timer fires — each timer is | ||
| // fully consumed before the loop can continue, so no timers accumulate. | ||
| func GoodTimeAfterInBody(ctx context.Context, ch <-chan struct{}) { | ||
| for { | ||
| select { | ||
| case <-ch: | ||
| <-time.After(time.Second) // in Body, not Comm — not flagged | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // GoodFuncLitInsideLoop: the select is inside a goroutine closure launched | ||
| // per iteration; the for loop does not directly enclose the CommClause. | ||
| func GoodFuncLitInsideLoop(ctx context.Context) { | ||
| for { | ||
| go func() { | ||
| select { | ||
| case <-time.After(time.Second): // FuncLit boundary — not flagged | ||
| case <-ctx.Done(): | ||
| } | ||
| }() | ||
| <-ctx.Done() | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // GoodSingleCaseSelect: the select has only one case and no default, so the | ||
| // timer must fire before the loop continues — no timer accumulation is possible. | ||
| func GoodSingleCaseSelect() { | ||
| for { | ||
| select { | ||
| case <-time.After(time.Second): // single case, no default — not flagged | ||
| doWork() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // GoodNolintPreviousLine: suppressed with a nolint directive on the previous line. | ||
| func GoodNolintPreviousLine(ctx context.Context) { | ||
| for { | ||
| select { | ||
| //nolint:timeafterleak | ||
| case <-time.After(time.Second): | ||
| doWork() | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // GoodNolintSameLine: suppressed with a nolint directive on the same line. | ||
| func GoodNolintSameLine(ctx context.Context) { | ||
| for { | ||
| select { | ||
| case <-time.After(time.Second): //nolint:timeafterleak | ||
| doWork() | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func doWork() {} | ||
|
Contributor
Author
There was a problem hiding this comment. Missing test case: single-case 💡 Suggested additionThe false positive described in the Add to this file: // GoodSingleCaseSelect: the select has only one case, so the timer always fires
// and can never be preempted; no leak is possible.
func GoodSingleCaseSelect() {
for {
select {
case <-time.After(time.Second): // single case — not flagged
doWork()
}
}
}
// BadSingleCaseWithDefault: default can preempt the timer → still flagged.
func BadSingleCaseWithDefault(ctx context.Context) {
for {
select {
case <-time.After(time.Second): // want `time\.After creates a new timer on each loop iteration`
default:
}
}
}
Contributor
There was a problem hiding this comment. Both added: |
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
timeafterleakhas been added to bothpkg/linters/doc.go(count updated 24→25, alphabetical entry) andpkg/linters/README.md(overview list, subpackages table, and dependencies list).