-
Notifications
You must be signed in to change notification settings - Fork 424
[linter-miner] feat(linters): add timesleepnocontext linter #38704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # ADR-38704: Enforce Context-Aware Sleeps via a Custom Static-Analysis Linter | ||
|
|
||
| **Date**: 2026-06-11 | ||
| **Status**: Draft | ||
|
|
||
| ## Context | ||
|
|
||
| The codebase makes heavy use of `context.Context` to propagate cancellation and deadline signals through CLI commands, workflow execution, and polling loops. A bare `time.Sleep(d)` call blocks the goroutine for the full duration regardless of whether the surrounding context has already been cancelled, which defeats prompt cancellation and can leave operations hanging well past a deadline. A scan of non-test `.go` files under `pkg/` and `cmd/` found 13 such calls across 9 files. There was no automated guardrail to catch new occurrences, so the only protection was manual code review. | ||
|
|
||
| ## Decision | ||
|
|
||
| We will add a custom `golang.org/x/tools/go/analysis` linter named `timesleepnocontext` that reports `time.Sleep` calls inside any function (or method, or context-capturing closure) that receives a `context.Context` parameter. The analyzer is registered in the existing multichecker (`cmd/linters/main.go`) and runs as part of `make golint-custom`, following the established conventions of the sibling `execcommandwithoutcontext` linter. The recommended remediation is a `select` over `time.After(d)` and `ctx.Done()`. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| ### Alternative 1: Documentation + code review only | ||
| Document the "use `select` with `ctx.Done()`" pattern in contributor guidelines and rely on reviewers to catch violations. Rejected because it does not scale, is inconsistently applied, and the existing 13 violations demonstrate that review alone has not prevented the anti-pattern. | ||
|
|
||
| ### Alternative 2: Adopt an off-the-shelf linter | ||
| Use an existing third-party linter rule instead of writing a custom analyzer. Rejected because no widely available linter targets this specific pattern (bare `time.Sleep` scoped to context-receiving functions), and the repository already maintains a suite of ~28 in-house analyzers with shared `internal/astutil` and `internal/filecheck` helpers, making a custom analyzer the lower-friction, consistent choice. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| - New bare `time.Sleep` calls in context-aware functions are caught automatically in CI rather than in review. | ||
| - Encourages a single, consistent cancellation-aware sleep idiom across the codebase. | ||
| - Reuses existing linter infrastructure and conventions, keeping the analyzer suite uniform. | ||
|
|
||
| ### Negative | ||
| - Adds ongoing maintenance burden for a hand-written AST/types analyzer. | ||
| - May produce false positives where a short, intentionally non-cancellable sleep is acceptable, requiring suppression or refactoring. | ||
| - Surfaces a backlog of 13 pre-existing violations that must be fixed or otherwise handled before the rule can be fully enforced. | ||
|
|
||
| ### Neutral | ||
| - Test files are intentionally skipped, and blank-identifier context parameters (`_ context.Context`) are ignored, scoping the rule to production code that genuinely holds a usable context. | ||
| - The diagnostic is report-only; it does not offer an automated fix. | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27374710073) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package timesleepnocontext | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
| ) | ||
|
|
||
| // Bad: time.Sleep inside a function that receives a context. | ||
| func BadSleep(ctx context.Context, d time.Duration) { | ||
| time.Sleep(d) // want `use select with ctx\.Done\(\) instead of time\.Sleep to allow context cancellation` | ||
| } | ||
|
|
||
| // Bad: time.Sleep inside a method that receives a context. | ||
| type Worker struct{} | ||
|
|
||
| func (w *Worker) Wait(ctx context.Context) { | ||
| time.Sleep(time.Second) // want `use select with ctx\.Done\(\) instead of time\.Sleep to allow context cancellation` | ||
| } | ||
|
|
||
| // Good: no context parameter — time.Sleep is acceptable. | ||
| func GoodNoContext(d time.Duration) { | ||
| time.Sleep(d) | ||
| } | ||
|
|
||
| // Good: context parameter is blank — time.Sleep is acceptable. | ||
| func GoodBlankContext(_ context.Context, d time.Duration) { | ||
| time.Sleep(d) | ||
| } | ||
|
|
||
| // Good: uses a context-aware select instead of time.Sleep. | ||
| func GoodSelectSleep(ctx context.Context, d time.Duration) error { | ||
| select { | ||
| case <-time.After(d): | ||
| return nil | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
| } | ||
|
|
||
| // Bad: time.Sleep in a goroutine closure that can close over the context. | ||
| func BadGoroutineWithCtx(ctx context.Context) { | ||
| go func() { | ||
| time.Sleep(time.Second) // want `use select with ctx\.Done\(\) instead of time\.Sleep to allow context cancellation` | ||
| }() | ||
| } | ||
|
|
||
| // Good: function literal with its own context parameter already handles it. | ||
| func GoodFuncLitWithOwnCtx() { | ||
| doWork(func(ctx context.Context, d time.Duration) { | ||
| select { | ||
| case <-time.After(d): | ||
| case <-ctx.Done(): | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func doWork(fn func(context.Context, time.Duration)) { | ||
| fn(context.Background(), time.Second) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| // Package timesleepnocontext implements a Go analysis linter that flags | ||
| // bare time.Sleep calls inside functions that already receive a | ||
| // context.Context parameter, where a context-aware select should be used | ||
| // to propagate cancellation. | ||
| package timesleepnocontext | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "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" | ||
| ) | ||
|
|
||
| // Analyzer is the time-sleep-no-context analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "timesleepnocontext", | ||
| Doc: "reports time.Sleep calls inside context-receiving functions where a context-aware select should be used to allow cancellation", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/timesleepnocontext", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp, err := astutil.Inspector(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { | ||
| call, ok := cur.Node().(*ast.CallExpr) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if !isTimeSleepCall(pass, call) { | ||
| continue | ||
| } | ||
|
|
||
| pos := pass.Fset.PositionFor(call.Pos(), false) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| continue | ||
| } | ||
|
|
||
| for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { | ||
| funcType := enclosingFuncType(encl.Node()) | ||
| if funcType == nil { | ||
| continue | ||
| } | ||
| ctxParamName, hasCtx := contextParamName(pass, funcType) | ||
| if !hasCtx { | ||
| continue | ||
| } | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: fmt.Sprintf("use select with %s.Done() instead of time.Sleep to allow context cancellation", ctxParamName), | ||
| }) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| // isTimeSleepCall reports whether call is a call to time.Sleep. | ||
| func isTimeSleepCall(pass *analysis.Pass, call *ast.CallExpr) bool { | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok || sel.Sel.Name != "Sleep" { | ||
| return false | ||
| } | ||
| ident, ok := sel.X.(*ast.Ident) | ||
| if !ok { | ||
| return false | ||
| } | ||
| obj := pass.TypesInfo.ObjectOf(ident) | ||
| if obj == nil { | ||
| return false | ||
| } | ||
| pkgName, ok := obj.(*types.PkgName) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return pkgName.Imported().Path() == "time" | ||
| } | ||
|
|
||
| func enclosingFuncType(node ast.Node) *ast.FuncType { | ||
| switch fn := node.(type) { | ||
| case *ast.FuncDecl: | ||
| return fn.Type | ||
| case *ast.FuncLit: | ||
| return fn.Type | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // contextParamName returns the name of the first context.Context parameter | ||
| // in fn, and true, or "", false if none exists. | ||
| func contextParamName(pass *analysis.Pass, fn *ast.FuncType) (string, bool) { | ||
| if fn == nil || fn.Params == nil { | ||
| return "", false | ||
| } | ||
| ctxType := contextContextType(pass) | ||
| if ctxType == nil { | ||
| return "", false | ||
| } | ||
| for _, field := range fn.Params.List { | ||
| t := pass.TypesInfo.TypeOf(field.Type) | ||
| if t == nil || !types.Identical(t, ctxType) { | ||
| continue | ||
| } | ||
| for _, name := range field.Names { | ||
| if name.Name != "_" { | ||
| return name.Name, true | ||
| } | ||
| } | ||
|
Comment on lines
+116
to
+120
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnamed When a Unnamed context parameters are idiomatic in interface implementations and stubs. Whether the intent is to treat them the same as 💡 Missing test cases to add// Unnamed context.Context — decide: Good or Bad?
func UnnamedContext(context.Context, d time.Duration) {
time.Sleep(d)
}If the intent is not to flag (same as If the intent is to flag (the parameter could be re-named and made cancellation-aware): add a |
||
| } | ||
| return "", false | ||
| } | ||
|
|
||
| // contextContextType returns the types.Type for context.Context, or nil if | ||
| // the context package is not imported. | ||
| func contextContextType(pass *analysis.Pass) types.Type { | ||
| for _, pkg := range pass.Pkg.Imports() { | ||
| if pkg.Path() == "context" { | ||
| obj := pkg.Scope().Lookup("Context") | ||
| if obj != nil { | ||
| return obj.Type() | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| //go:build !integration | ||
|
|
||
| package timesleepnocontext_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/timesleepnocontext" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, timesleepnocontext.Analyzer, "timesleepnocontext") | ||
| } |
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.
False positive: blank-context closure nested inside a context-receiving function is incorrectly flagged.
When the innermost enclosing function has
_ context.Context,contextParamNamereturnsfalseand the loopcontinues — escalating to the outer function. Atime.Sleepinsidefunc(_ context.Context)will be reported with the outer function'sctx, even though the blank_is an explicit signal that this scope intentionally ignores context.Compare the analogous
ctxbackgroundlinter (line 50): it usesbreakwhen no usable context name is found, stopping the search rather than climbing up.�� Failing scenario (not in test data)
The fix is to distinguish no context param (keep climbing) from blank/unnamed context param (stop climbing). One approach:
A companion
hasAnyContextParam(checks type only, ignores names) can be extracted fromcontextParamName.