fix: join CheckForUpdatesAsync goroutine and eliminate time.After leak#47699
Conversation
- Change CheckForUpdatesAsync to return a func() join function (mirrors the compile_update_check.go StartCompileUpdateCheck pattern) - Add a buffered done channel closed by the goroutine via defer close(done), providing an observable join point - Replace bare time.After(100ms) with time.NewTimer + defer timer.Stop() to eliminate the per-invocation timer leak - Update validate_command.go caller to capture and defer the join function - Update TestCheckForUpdatesAsync_ContextCancellation to use join() - Add TestCheckForUpdatesAsync_JoinsGoroutine to verify join completes - Update pkg/cli/README.md function signature entry Closes #47609 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47699 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (71 additions across 4 files). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Attempts to make update checks joinable and clean up their timeout timer.
Changes:
- Returns a goroutine finisher from
CheckForUpdatesAsync. - Replaces
time.Afterwith a stoppable timer. - Updates the caller, tests, and API documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/validate_command.go |
Defers the update-check finisher. |
pkg/cli/update_check.go |
Adds completion signaling and timer cleanup. |
pkg/cli/update_check_test.go |
Updates cancellation tests and adds join coverage. |
pkg/cli/README.md |
Documents the new return signature. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
| return func() { | ||
| select { | ||
| case <-done: | ||
| case <-ctx.Done(): | ||
| } |
| // Use a cancelled context to make the goroutine exit quickly | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| join := CheckForUpdatesAsync(ctx, false, false) |
| os.Unsetenv("GITHUB_ACTIONS") | ||
| os.Unsetenv("CONTINUOUS_INTEGRATION") | ||
| os.Unsetenv("GH_AW_MCP_SERVER") |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and well-structured.
defer close(done)/defer recover()ordering (LIFO) is intentional and correct: panic is recovered before the channel closes.timer.Stop()is properly deferred at function scope, not inside the goroutine.- Join function mirrors the
StartCompileUpdateCheckpattern, including graceful ctx-cancel fallback. - Tests are deterministic — no
time.Sleepraces.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 10.8 AIC · ⌖ 4.48 AIC · ⊞ 5K
There was a problem hiding this comment.
REQUEST_CHANGES — The central join guarantee is broken, the test that should catch it passes for the wrong reason, and the new test pollutes shared environment state.
### Blocking issues (3)
-
join()exits onctx.Done()without goroutine completing (update_check.go:297) — The returned closure selects on both<-doneand<-ctx.Done(). On context cancellation (the normal shutdown path), it returns immediately while the goroutine may still be running. The fix isreturn func() { <-done }— the goroutine already exits promptly on cancellation internally. -
TestCheckForUpdatesAsync_JoinsGoroutinevalidatesjoin()returns quickly, not that the goroutine finished (update_check_test.go:395) — Because the test uses a pre-cancelled context,join()returns via<-ctx.Done()instantly regardless of goroutine state. The test is a tautology that would pass even ifjoinwerefunc() {}. It needs to assert actual goroutine completion via a side-effect or a non-cancelled context with a controlled goroutine. -
Three env vars leaked in the new test (
update_check_test.go:366) —GITHUB_ACTIONS,CONTINUOUS_INTEGRATION,GH_AW_MCP_SERVERare unset but never restored, silently altering the behavior ofshouldCheckForUpdatein subsequent tests on CI runners where those vars are set. Uset.Setenv()or save-and-defer for all three.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 46 AIC · ⌖ 5.07 AIC · ⊞ 5.7K
Comment /review to run again
| return | ||
| } | ||
|
|
||
| return func() { |
There was a problem hiding this comment.
join() does not actually wait for the goroutine — it exits early on context cancellation.
💡 Details and suggested fix
The returned join function selects on both <-done and <-ctx.Done(). When the context is cancelled (the common case when a command is exiting), join() returns via <-ctx.Done() without waiting for the goroutine to finish. The goroutine can still be alive — making HTTP calls or writing files — after join() has returned, defeating the stated contract.
// Current (broken): exits early when context is done
return func() {
select {
case <-done:
case <-ctx.Done(): // returns without goroutine completing
}
}
// Fix: always wait for goroutine (it exits promptly on ctx.Done internally)
return func() {
<-done
}The goroutine body already checks ctx.Err() and exits promptly on cancellation. The ctx.Done() arm in the join closure is a safety-valve that masks a goroutine leak rather than curing it.
|
|
||
| // Ensure we're not in CI mode so that shouldCheckForUpdate returns true | ||
| os.Unsetenv("CI") | ||
| os.Unsetenv("GITHUB_ACTIONS") |
There was a problem hiding this comment.
Test pollutes shared environment: GITHUB_ACTIONS, CONTINUOUS_INTEGRATION, and GH_AW_MCP_SERVER are unset but never restored.
💡 Details and fix
TestCheckForUpdatesAsync_JoinsGoroutine saves only CI and getLastCheckFilePathFunc, then unsets three other env vars without saving their originals. On a real CI runner all three are typically set, and other tests running in the same process after this one will observe them missing, altering shouldCheckForUpdate / IsRunningInCI results.
// Current (broken)
os.Unsetenv("CI")
os.Unsetenv("GITHUB_ACTIONS") // not restored
os.Unsetenv("CONTINUOUS_INTEGRATION") // not restored
os.Unsetenv("GH_AW_MCP_SERVER") // not restored
// Fix: save and restore all
origGHA := os.Getenv("GITHUB_ACTIONS")
origCI2 := os.Getenv("CONTINUOUS_INTEGRATION")
origMCP := os.Getenv("GH_AW_MCP_SERVER")
defer func() {
os.Setenv("GITHUB_ACTIONS", origGHA)
os.Setenv("CONTINUOUS_INTEGRATION", origCI2)
os.Setenv("GH_AW_MCP_SERVER", origMCP)
}()Or use t.Setenv() (Go 1.17+) which restores automatically and is safe with parallel tests.
| // goroutine joined successfully | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("join function did not return within 2 seconds") | ||
| } |
There was a problem hiding this comment.
New test validates the wrong invariant — it passes for the wrong reason due to the join() bug.
💡 Details
TestCheckForUpdatesAsync_JoinsGoroutine verifies that join() returns within 2 seconds when the context is already cancelled. Because join() selects on <-ctx.Done() (which is already closed), it returns immediately regardless of whether the goroutine has finished. The test therefore passes even with a completely broken join implementation such as return func() {}.
To make this test meaningful, assert that the goroutine has actually completed by checking a side-effect (e.g., the done channel is closed) after join() returns, or restructure the test to use an uncancelled context with a goroutine that signals completion:
// After join() returns, verify the internal goroutine is done
// by trying to receive on a separate sentinel, or assert file side-effects.
// Simply checking join() returned is not sufficient.As written the test gives false confidence in goroutine cleanup.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the structural change is good, but there are two issues that undermine the correctness of the fix.
📋 Key Themes
Issues
-
Join function exits early on ctx cancellation (
update_check.goline 297–301): The returnedfunc()selects onctx.Done()in addition todone, so if the context is already cancelled (common at shutdown),join()returns without waiting for the goroutine to callclose(done). This is the same goroutine leak the PR is trying to fix. The goroutine internally already exits promptly on context cancellation, so the join function only needs to<-done. -
New test validates the wrong path (
update_check_test.goline 354–398):TestCheckForUpdatesAsync_JoinsGoroutinepre-cancels the context before callingCheckForUpdatesAsync. The test'sjoin()will exit via thectx.Done()arm immediately — it never waits for the goroutine'sdonechannel. The test passes regardless of whether the goroutine has exited, giving false confidence.
Positive Highlights
- ✅
time.After→time.NewTimer+defer timer.Stop()is the correct fix for the timer channel leak - ✅ Buffered channel
donewithdefer close(done)is idiomatic Go for observable join points - ✅
validate_command.gocorrectly captures and defers the join function - ✅ README updated to reflect the new signature
- ✅
TestCheckForUpdatesAsync_ContextCancellationupdated to usejoin()instead oftime.Sleep— this is a clear improvement
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.3 AIC · ⌖ 4.86 AIC · ⊞ 6.7K
Comment /matt to run again
| select { | ||
| case <-done: | ||
| case <-ctx.Done(): | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The join function exits on ctx.Done() before the goroutine has closed done, creating the same goroutine leak the PR set out to fix: if the context is already cancelled at the time the caller defers join(), the function returns immediately while the goroutine is still running.
💡 Suggested fix
The goroutine already respects ctx cancellation internally (it exits promptly when ctx is cancelled). Simply wait only on done:
return func() {
<-done // goroutine always closes done; safe to block here
}If a hard timeout is preferred for defense-in-depth:
return func() {
t := time.NewTimer(2 * time.Second)
defer t.Stop()
select {
case <-done:
case <-t.C:
}
}@copilot please address this.
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| join := CheckForUpdatesAsync(ctx, false, false) |
There was a problem hiding this comment.
[/diagnosing-bugs] TestCheckForUpdatesAsync_JoinsGoroutine pre-cancels the context before calling CheckForUpdatesAsync, so join() returns via the ctx.Done() arm rather than the done arm — it never actually tests that the goroutine has finished running.
💡 Suggested approach
Use a context that stays live during the test, instrument the goroutine's completion with a side-effect (e.g. write a file or set an atomic), and assert that side-effect is observed after join() returns:
ctx := context.Background() // NOT pre-cancelled
var goroutineRan atomic.Bool
// inject a hook that sets goroutineRan = true at the end of checkForUpdatesWithContext
join := CheckForUpdatesAsync(ctx, false, false)
join()
assert.True(t, goroutineRan.Load(), "goroutine should have completed before join returned")The current test only proves the join function itself doesn't hang — not that the goroutine has exited.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a pr-finisher pass and pushed the fixes in |
|
Great work on this goroutine lifecycle fix! 🎯 This PR does excellent work eliminating the fire-and-forget goroutine pattern and the What looks good:
The PR is ready for review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
Verified on HEAD
I reran checks in this pass:
No further code changes were needed in this pass. |
|
@copilot please run the Unresolved review feedback:
|
Checked on HEAD
Reran checks after the up-to-date verification:
No additional code changes were needed in this pass. |
|
@copilot please run the Unresolved review feedback:
|
Confirmed on current HEAD
Reran checks in this pass:
Also checked prior CI run logs via GitHub MCP; no failed jobs were reported for the referenced PR Sous Chef run. No additional code changes were needed in this pass. |
🤖 PR Triage
Rationale: Goroutine leak + timer leak fix in update check path. 4 files +93/-21, well-described, CI green. Non-trivial concurrency change warrants human review but low surface area.
|
|
@copilot merge main and make wasm golden test suite for reliable |
…deep-report-join-checkforupdatesasync-goroutine Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed in
Reran checks after branch update:
CI log triage for the provided run ( |
|
@copilot review failed wasm golden test https://github.com/github/gh-aw/actions/runs/30105142160/job/89520942095?pr=47699 and try to fix to make the test more resilient. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
CheckForUpdatesAsynclaunched a fire-and-forget goroutine with no join mechanism, and used a baretime.After(100ms)that leaks a timer channel until GC'd.Changes
done chan struct{}on exit;CheckForUpdatesAsyncreturns afunc()that blocks on<-done(orctx.Done()), mirroring theStartCompileUpdateCheckpattern incompile_update_check.gotime.After(100ms)withtime.NewTimer+defer timer.Stop()so the timer is released immediately when the select exitsvalidate_command.gocaptures and defers the join functionTestCheckForUpdatesAsync_ContextCancellationto calljoin()instead oftime.Sleep; addedTestCheckForUpdatesAsync_JoinsGoroutineto verify the join returns promptlyRun: https://github.com/github/gh-aw/actions/runs/30074181699
Run: https://github.com/github/gh-aw/actions/runs/30087480557
Run: https://github.com/github/gh-aw/actions/runs/30091871574