[sergo] Sergo Report: timesleepnocontext First-Audit (Deferred-Cleanup FP + Enforce) - 2026-06-15 #39325
Closed
Replies: 1 comment
-
|
This discussion has been marked as outdated by Sergo - Serena Go Expert. A newer discussion is available at Discussion #39495. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
Executive Summary
Run R37. The custom-linter registry grew 28 → 29 analyzers since R36. Doc surfaces have largely healed (doc.go
All 29, spec_test synced to 29) and CI enforcement advanced 13 → 14 flags (-timeafterleaklanded — R36's sg36a1 work shipped). With the new-linter trail well-covered, this run did the first-ever precision audit oftimesleepnocontext(the 26th linter), surfacing a clean novel false-positive plus an enforce-readiness path. 2 issues created.Tool / Registry Updates
cmd/linters/main.gomultichecker.Mainnow lists 29 analyzers (was 28). The new 29th sits among five style/quality linters Sergo has never audited (errorfwrapv,errormessage,excessivefuncparams,largefunc,ssljson); the shallow clone (HEADebeda50, unrelated) prevents git-dating exactly which is newest.doc.gonow saysAll 29andspec_test.documentedAnalyzers()has 29 entries. Residual: thedoc.gobullet list has only 28 bullets, missing thehardcodedfilepathbullet. Open issue doc-sync: linters/doc.go still says "All 24 active analyzers" after two new linters (hardcodedfilepath, timesleepnocontext) land [Content truncated due to length] #38787 covers doc-sync intent → not refiled.cgo.yml:1123LINTER_FLAGS= 14 (added-timeafterleak).timesleepnocontextremains unenforced.Strategy — 50 / 50 Split
Name:
tool-change-29th-detect + FIRST timesleepnocontext precision audit (deferred-cleanup FP + enforce-readiness)./cmd/... ./pkg/...) for the target call, map each site to its enclosing function signature, and distinguish real violations from non-matches. This pattern has landed repeatedly (tolowerequalfoldtolowerequalfold: convert 13 ToLower/ToUpper equality sites to strings.EqualFold, then enforce in CI #37250,httpnoctxhttpnoctx (27th linter): 3 prod *http.Client.Get calls omit context — convert to NewRequestWithContext+Do, then enforce in CI #39016,timeafterleaktimeafterleak (28th linter): 3 prod loop+select time.After sites leak a timer per iteration — convert to time.NewTimer/Stop, the [Content truncated due to length] #39180-enforced).timesleepnocontext(26th linter) had never been audited. Read the analyzer end-to-end, validated detection precision, and stress-tested the FuncLit-boundary handling — a dimension prior audits (which were on non-closure-crossing linters) never exercised.Findings
timesleepnocontextdetection is clean — it resolvestime.Sleepby package type-identity (ObjectOf → *types.PkgName → Imported().Path() == "time"), skips test files, and matches the ctx param viatypes.Identical. No syntactic-shadow bug. The issue is in how it attributes an outer function's context to a sleep inside a nested closure.sg37a1 — False positive on deferred cleanup closures
The diagnostic loop walks enclosing funcs outward across both
FuncDeclandFuncLit, and when an innerFuncLitlacks its own ctx param it falls through to the outer function's ctx. This is intentional for goroutine closures (go func(){ time.Sleep }(), tested byBadGoroutineWithCtx), but it also fires for deferred cleanup closures.Real site —
pkg/cli/mcp_inspect.go:131-148, insideInspectWorkflowMCP(ctx context.Context, ...):This sleep is the graceful
SIGINT → SIGKILLwindow. Rewriting it asselect { case <-ctx.Done(): }would skip the window exactly whenctxis cancelled (when cleanup runs) — semantically wrong. Fix: treat aDeferStmt-wrappedFuncLitas a boundary (stop the search); keepGoStmtclosures flagged.sg37a2 — Enforce-readiness sweep
All 11 non-test
time.Sleepcalls across the analyzer's run scope mapped to their enclosing function:docker_validation.go:219validateDockerImage(...)run_workflow_tracking.go:73getLatestWorkflowRunWithRetry(...)trial_repository.go:156ensureTrialRepository(...)logs_rate_limit.go:87/97/110/121checkAndWaitForRateLimit(verbose)mcp_inspect_inspector.go:164/208spawnMCPInspector(...)update_extension_check.go:411cleanupStaleWindowsBackups(...)mcp_inspect.go:138InspectWorkflowMCP(ctx, ...)(defer)The only flagged production site is the deferred-cleanup FP. After sg37a1, zero real violations remain → append
-timesleepnocontexttocgo.yml:1123.Generated Tasks → Issues Created
timesleepnocontextprecision: FP ontime.Sleepinside deferred cleanup closures. Single-file analyzer change + one testdata case.timesleepnocontextenforce-readiness: zero prod violations after the FP fix; add the CI flag. Blocked by sg37a1.Metrics
Historical Context / Reconciliation
-timeafterleaknow in CI → R36 sg36a1 (timeafterleak (28th linter): 3 prod loop+select time.After sites leak a timer per iteration — convert to time.NewTimer/Stop, the [Content truncated due to length] #39180) enforcement shipped (issue still shows open; may auto-close). R36 sg35a1/a2 (httpnoctx (27th linter): 3 prod *http.Client.Get calls omit context — convert to NewRequestWithContext+Do, then enforce in CI #39016/httpnoctx precision: isHTTPClientReceiver only matches *http.Client pointers — value & embedded http.Client receivers escape (la [Content truncated due to length] #39017) httpnoctx landed earlier.Recommendations / Next-Run Focus (R38)
errormessagechanged-file scoping,excessivefuncparams/largefuncthreshold config,errorfwrapv%v→%wtype-identity,ssljsonartifact-spec edges).GoStmt(flag) fromDeferStmtcleanup (don't flag).References:
Beta Was this translation helpful? Give feedback.
All reactions