Skip to content

Commit 4ee14ca

Browse files
authored
Eliminate looped time.After timer leaks, propagate cancellation correctly, and enforce timeafterleak in CI (#39188)
1 parent 7a4c2e4 commit 4ee14ca

6 files changed

Lines changed: 62 additions & 13 deletions

File tree

.github/workflows/cgo.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1118,8 +1118,9 @@ jobs:
11181118

11191119
# Enforce selected production custom analyzers without blocking on unrelated
11201120
# legacy custom analyzer findings in tests or other analyzer families.
1121+
# Note: -test=false intentionally scopes this gate to production code only.
11211122
- name: Run custom linters
1122-
run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -test=false"
1123+
run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -timeafterleak -test=false"
11231124

11241125
# Ensure no action shell scripts invoke python or python3
11251126
- name: Lint action shell scripts

pkg/cli/add_interactive_workflow.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,15 @@ func (c *AddInteractiveConfig) checkStatusAndOfferRun(ctx context.Context) error
3232
var workflowFound bool
3333
for i := range 5 {
3434
// Wait 2 seconds before each check (including the first)
35+
timer := time.NewTimer(2 * time.Second)
3536
select {
3637
case <-ctx.Done():
38+
timer.Stop()
3739
if spinner != nil {
3840
spinner.Stop()
3941
}
4042
return ctx.Err()
41-
case <-time.After(2 * time.Second):
43+
case <-timer.C:
4244
// Continue with check
4345
}
4446

pkg/cli/add_interactive_workflow_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
package cli
44

55
import (
6+
"context"
7+
"errors"
68
"testing"
79

810
"github.com/stretchr/testify/assert"
@@ -59,3 +61,14 @@ func TestGetWorkflowStatuses(t *testing.T) {
5961
})
6062
}
6163
}
64+
65+
func TestCheckStatusAndOfferRun_ContextCancelled(t *testing.T) {
66+
ctx, cancel := context.WithCancel(t.Context())
67+
cancel()
68+
69+
cfg := &AddInteractiveConfig{Verbose: true}
70+
err := cfg.checkStatusAndOfferRun(ctx)
71+
if !errors.Is(err, context.Canceled) {
72+
t.Fatalf("expected context cancellation error, got %v", err)
73+
}
74+
}

pkg/cli/docker_images.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,10 +195,12 @@ func StartDockerImageDownload(ctx context.Context, image string) bool {
195195
dockerImagesLog.Printf("Failed to download image %s (attempt %d/%d). Retrying in %ds...", image, attempt, maxAttempts, waitTime)
196196

197197
// Use context-aware sleep
198+
timer := time.NewTimer(time.Duration(waitTime) * time.Second)
198199
select {
199-
case <-time.After(time.Duration(waitTime) * time.Second):
200+
case <-timer.C:
200201
// Continue to next retry
201202
case <-ctx.Done():
203+
timer.Stop()
202204
// Context cancelled during sleep
203205
dockerImagesLog.Printf("Download of image %s cancelled during retry wait: %v", image, ctx.Err())
204206
return

pkg/cli/mcp_inspect_mcp_scripts_server.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,10 @@ func findAvailablePort(startPort int, verbose bool) int {
4343
return 0
4444
}
4545

46-
// waitForServerReady waits for the HTTP server to be ready by polling the endpoint
47-
func waitForServerReady(ctx context.Context, port int, timeout time.Duration, verbose bool) bool {
46+
var errMCPScriptsServerStartupTimeout = errors.New("mcp-scripts HTTP server failed to start within timeout")
47+
48+
// waitForServerReady waits for the HTTP server to be ready by polling the endpoint.
49+
func waitForServerReady(ctx context.Context, port int, timeout time.Duration, verbose bool) error {
4850
deadline := time.Now().Add(timeout)
4951
client := &http.Client{
5052
Timeout: 1 * time.Second,
@@ -54,13 +56,13 @@ func waitForServerReady(ctx context.Context, port int, timeout time.Duration, ve
5456
for time.Now().Before(deadline) {
5557
select {
5658
case <-ctx.Done():
57-
return false
59+
return ctx.Err()
5860
default:
5961
}
6062
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
6163
if err != nil {
6264
mcpInspectLog.Printf("Failed to create request: %v", err)
63-
return false
65+
return err
6466
}
6567
resp, err := client.Do(req)
6668
if err == nil {
@@ -70,17 +72,19 @@ func waitForServerReady(ctx context.Context, port int, timeout time.Duration, ve
7072
if verbose {
7173
mcpInspectLog.Printf("Server is ready on port %d", port)
7274
}
73-
return true
75+
return nil
7476
}
77+
timer := time.NewTimer(mcpScriptsServerStartupDelay)
7578
select {
7679
case <-ctx.Done():
77-
return false
78-
case <-time.After(mcpScriptsServerStartupDelay):
80+
timer.Stop()
81+
return ctx.Err()
82+
case <-timer.C:
7983
}
8084
}
8185

8286
mcpInspectLog.Printf("Server did not become ready within timeout")
83-
return false
87+
return errMCPScriptsServerStartupTimeout
8488
}
8589

8690
// startMCPScriptsHTTPServer starts the mcp-scripts HTTP MCP server
@@ -180,7 +184,7 @@ func startMCPScriptsServer(ctx context.Context, mcpScriptsConfig *workflow.MCPSc
180184
}
181185

182186
// Wait for the server to start up
183-
if !waitForServerReady(ctx, port, 5*time.Second, verbose) {
187+
if err := waitForServerReady(ctx, port, 5*time.Second, verbose); err != nil {
184188
if serverCmd.Process != nil {
185189
// Kill the process and log warning if it fails
186190
if err := serverCmd.Process.Kill(); err != nil && verbose {
@@ -190,7 +194,7 @@ func startMCPScriptsServer(ctx context.Context, mcpScriptsConfig *workflow.MCPSc
190194
if err := os.RemoveAll(tmpDir); err != nil && verbose {
191195
mcpInspectLog.Printf("Warning: failed to clean up temporary directory %s: %v", tmpDir, err)
192196
}
193-
return nil, nil, "", errors.New("mcp-scripts HTTP server failed to start within timeout")
197+
return nil, nil, "", err
194198
}
195199

196200
if verbose {
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
//go:build !integration
2+
3+
package cli
4+
5+
import (
6+
"context"
7+
"errors"
8+
"testing"
9+
"time"
10+
)
11+
12+
func TestWaitForServerReady_ContextCancelled(t *testing.T) {
13+
ctx, cancel := context.WithCancel(t.Context())
14+
cancel()
15+
16+
err := waitForServerReady(ctx, 65535, 100*time.Millisecond, false)
17+
if !errors.Is(err, context.Canceled) {
18+
t.Fatalf("expected context cancellation error, got %v", err)
19+
}
20+
}
21+
22+
func TestWaitForServerReady_Timeout(t *testing.T) {
23+
err := waitForServerReady(t.Context(), 65535, 10*time.Millisecond, false)
24+
if !errors.Is(err, errMCPScriptsServerStartupTimeout) {
25+
t.Fatalf("expected timeout error, got %v", err)
26+
}
27+
}

0 commit comments

Comments
 (0)