From 3f2c8f412af18e39b085fb3f194c2b5d40ed4469 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 11 Jun 2026 19:02:30 +0000 Subject: [PATCH 1/2] feat(linters): add timesleepnocontext linter Reports bare time.Sleep calls inside functions that already receive a context.Context parameter. Such calls ignore cancellation signals, leaving goroutines blocked for the full sleep duration even when the caller has cancelled or timed out the operation. The fix is to replace time.Sleep(d) with a context-aware select: select { case <-time.After(d): case <-ctx.Done(): return ctx.Err() } Evidence gathered from 14 days of issues and code scanning: - 13 bare time.Sleep calls found in non-test code inside context-receiving functions (pkg/cli/logs_rate_limit.go, pkg/cli/run_workflow_tracking.go, pkg/workflow/docker_validation.go, pkg/cli/mcp_inspect.go, and others) - Immediately caught a real violation in pkg/cli/run_workflow_execution.go:574 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/linters/main.go | 2 + .../timesleepnocontext/timesleepnocontext.go | 59 ++++++++ .../timesleepnocontext/timesleepnocontext.go | 137 ++++++++++++++++++ .../timesleepnocontext_test.go | 16 ++ 4 files changed, 214 insertions(+) create mode 100644 pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go create mode 100644 pkg/linters/timesleepnocontext/timesleepnocontext.go create mode 100644 pkg/linters/timesleepnocontext/timesleepnocontext_test.go diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 2fac9a2fb3f..5a2ee4a948b 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -38,6 +38,7 @@ import ( "github.com/github/gh-aw/pkg/linters/sortslice" "github.com/github/gh-aw/pkg/linters/ssljson" "github.com/github/gh-aw/pkg/linters/strconvparseignorederror" + "github.com/github/gh-aw/pkg/linters/timesleepnocontext" "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" ) @@ -66,6 +67,7 @@ func main() { strconvparseignorederror.Analyzer, jsonmarshalignoredeerror.Analyzer, lenstringzero.Analyzer, + timesleepnocontext.Analyzer, tolowerequalfold.Analyzer, uncheckedtypeassertion.Analyzer, ) diff --git a/pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go b/pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go new file mode 100644 index 00000000000..2642d45e362 --- /dev/null +++ b/pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go @@ -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) +} diff --git a/pkg/linters/timesleepnocontext/timesleepnocontext.go b/pkg/linters/timesleepnocontext/timesleepnocontext.go new file mode 100644 index 00000000000..ad9f9303983 --- /dev/null +++ b/pkg/linters/timesleepnocontext/timesleepnocontext.go @@ -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 + } + } + } + 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 +} diff --git a/pkg/linters/timesleepnocontext/timesleepnocontext_test.go b/pkg/linters/timesleepnocontext/timesleepnocontext_test.go new file mode 100644 index 00000000000..d0358b2f6d2 --- /dev/null +++ b/pkg/linters/timesleepnocontext/timesleepnocontext_test.go @@ -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") +} From e92e44c89b11a78249019c4a6ad9b70ef0373297 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 11 Jun 2026 20:24:06 +0000 Subject: [PATCH 2/2] docs(adr): add draft ADR-38704 for context-aware sleep linter Co-Authored-By: Claude Opus 4.8 (1M context) --- ...e-context-aware-sleep-via-custom-linter.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 docs/adr/38704-enforce-context-aware-sleep-via-custom-linter.md diff --git a/docs/adr/38704-enforce-context-aware-sleep-via-custom-linter.md b/docs/adr/38704-enforce-context-aware-sleep-via-custom-linter.md new file mode 100644 index 00000000000..b2707113a6d --- /dev/null +++ b/docs/adr/38704-enforce-context-aware-sleep-via-custom-linter.md @@ -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.*