Skip to content

Eliminate looped time.After timer leaks, propagate cancellation correctly, and enforce timeafterleak in CI #39188

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.

Sign up for GitHub

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

Merged
pelikhan merged 4 commits into from
Jun 14, 2026
Merged

Eliminate looped time.After timer leaks, propagate cancellation correctly, and enforce timeafterleak in CI #39188

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d35202
Initial plan
Copilot Jun 14, 2026
5589935
chore: start timeafterleak remediation plan
Copilot Jun 14, 2026
717e33f
fix: eliminate looped time.After timer leaks and enforce linter
Copilot Jun 14, 2026
3dba05a
fix: finish pr feedback for timer leak remediation
Copilot Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/cgo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ jobs:
# Enforce selected production custom analyzers without blocking on unrelated
# legacy custom analyzer findings in tests or other analyzer families.
- name: Run custom linters
run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -test=false"
run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -timeafterleak -test=false"

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] -test=false means the timeafterleak linter skips all test files. Test helpers, integration test fixtures, and fake clocks that use time.After in a loop will not be caught by this gate. Consider adding a separate lint step (or a comment in the CI YAML) noting this intentional scope limit so future contributors know test files are unprotected.


# Ensure no action shell scripts invoke python or python3
- name: Lint action shell scripts
Expand Down
4 changes: 3 additions & 1 deletion pkg/cli/add_interactive_workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,15 @@ func (c *AddInteractiveConfig) checkStatusAndOfferRun(ctx context.Context) error
var workflowFound bool
for i := range 5 {
// Wait 2 seconds before each check (including the first)
timer := time.NewTimer(2 * time.Second)

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] No regression test covers context cancellation in this polling loop — the most common real-world code path for the timer fix.

💡 Suggested test skeleton 
func TestCheckStatusAndOfferRun_ContextCancelled(t *testing.T) {
    ctx, cancel := context.WithCancel(context.Background())
    cancel() // pre-cancel
    cfg := &AddInteractiveConfig{Verbose: true}
    err := cfg.checkStatusAndOfferRun(ctx)
    if !errors.Is(err, context.Canceled) {
        t.Errorf("expected context.Canceled, got %v", err)
    }
}

The current timer fix is correct, but without a test a future refactor could reintroduce time.After (or forget timer.Stop()) and silently pass CI since the linter only covers production files (-test=false).

select {
case <-ctx.Done():
timer.Stop()
if spinner != nil {
spinner.Stop()
}
return ctx.Err()
case <-time.After(2 * time.Second):
case <-timer.C:
// Continue with check
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/cli/docker_images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,12 @@ func StartDockerImageDownload(ctx context.Context, image string) bool {
dockerImagesLog.Printf("Failed to download image %s (attempt %d/%d). Retrying in %ds...", image, attempt, maxAttempts, waitTime)

// Use context-aware sleep
timer := time.NewTimer(time.Duration(waitTime) * time.Second)

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/improve-codebase-architecture] This is the third file in this PR that implements the same NewTimer → select → Stop-on-cancel idiom. A shared helper would prevent future drift and consolidate the canonical pattern.

💡 Suggested helper 
// contextSleep pauses for d or until ctx is cancelled.
// Returns ctx.Err() if the context fires first, nil otherwise.
func contextSleep(ctx context.Context, d time.Duration) error {
    timer := time.NewTimer(d)
    select {
    case <-ctx.Done():
        timer.Stop()
        return ctx.Err()
    case <-timer.C:
        return nil
    }
}

All three call sites (add_interactive_workflow.go, docker_images.go, mcp_inspect_mcp_scripts_server.go) would reduce to a single if err := contextSleep(ctx, d); err != nil { return err }. Any future fix or improvement (e.g., adding drain after Stop()) would only need to be made once.

select {
case <-time.After(time.Duration(waitTime) * time.Second):
case <-timer.C:
// Continue to next retry
case <-ctx.Done():
timer.Stop()
// Context cancelled during sleep
dockerImagesLog.Printf("Download of image %s cancelled during retry wait: %v", image, ctx.Err())
return
Expand Down
4 changes: 3 additions & 1 deletion pkg/cli/mcp_inspect_mcp_scripts_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,12 @@ func waitForServerReady(ctx context.Context, port int, timeout time.Duration, ve
}
return true
}
timer := time.NewTimer(mcpScriptsServerStartupDelay)
select {
case <-ctx.Done():
timer.Stop()

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] return false silently drops the cancellation reason — callers cannot distinguish "context was cancelled" from "server never became ready".

💡 Suggested fix: return error instead of bool

Changing the function signature to func waitForServerReady(...) error lets the cancellation propagate:

case <-ctx.Done():
    timer.Stop()
    return ctx.Err()   // propagates cancellation
case <-timer.C:
}
// ...
mcpInspectLog.Printf("Server did not become ready within timeout")
return fmt.Errorf("server did not become ready within timeout")

With bool, a cancelled context and a genuine timeout produce the same false result. Callers log "server not ready" even when the real cause is an upstream shutdown, making incidents harder to diagnose.

return false
case <-time.After(mcpScriptsServerStartupDelay):
case <-timer.C:
}
}

Expand Down
Loading
Loading