fix: join CheckForUpdatesAsync goroutine and eliminate time.After leak #47699
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -313,18 +313,16 @@ func TestCheckForUpdatesInCIMode(t *testing.T) { | |
|
|
||
| func TestCheckForUpdatesAsync_ContextCancellation(t *testing.T) { | ||
| // Test that async update check respects context cancellation | ||
| origGetLastCheckFilePath := getLastCheckFilePathFunc | ||
| defer func() { | ||
| getLastCheckFilePathFunc = origGetLastCheckFilePath | ||
| }() | ||
|
|
||
| // Ensure we're not in CI mode | ||
| t.Setenv("CI", "") | ||
| t.Setenv("GITHUB_ACTIONS", "") | ||
| t.Setenv("CONTINUOUS_INTEGRATION", "") | ||
| t.Setenv("GH_AW_MCP_SERVER", "") | ||
|
|
||
| // Create temporary directory for last check file | ||
| tmpDir := t.TempDir() | ||
|
|
@@ -341,18 +339,74 @@ func TestCheckForUpdatesAsync_ContextCancellation(t *testing.T) { | |
| // Cancel immediately | ||
| cancel() | ||
|
|
||
| // Call CheckForUpdatesAsync with cancelled context and join the goroutine | ||
| join := CheckForUpdatesAsync(ctx, false, false) | ||
| join() | ||
|
|
||
| // The update check should not have created a last check file | ||
| // because the context was cancelled | ||
| // Note: The check might still run if it started before cancellation, | ||
| // so we just verify no panics occurred | ||
| } | ||
|
|
||
| func TestCheckForUpdatesAsync_JoinsGoroutine(t *testing.T) { | ||
| // Test that the returned join function waits for the goroutine to complete | ||
| origGetLastCheckFilePath := getLastCheckFilePathFunc | ||
| origCheckForUpdatesWithContext := checkForUpdatesWithContextFunc | ||
| defer func() { | ||
| getLastCheckFilePathFunc = origGetLastCheckFilePath | ||
| checkForUpdatesWithContextFunc = origCheckForUpdatesWithContext | ||
| }() | ||
|
|
||
| // Ensure we're not in CI mode so that shouldCheckForUpdate returns true | ||
| t.Setenv("CI", "") | ||
| t.Setenv("GITHUB_ACTIONS", "") | ||
| t.Setenv("CONTINUOUS_INTEGRATION", "") | ||
| t.Setenv("GH_AW_MCP_SERVER", "") | ||
|
|
||
| // Create temporary directory for last check file | ||
| tmpDir := t.TempDir() | ||
| lastCheckFile := filepath.Join(tmpDir, lastCheckFileName) | ||
| getLastCheckFilePathFunc = func() string { | ||
| return lastCheckFile | ||
| } | ||
|
|
||
| started := make(chan struct{}) | ||
| release := make(chan struct{}) | ||
| checkForUpdatesWithContextFunc = func(_ context.Context, _ bool, _ bool) { | ||
| close(started) | ||
| <-release | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| join := CheckForUpdatesAsync(ctx, false, false) | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] 💡 Suggested approachUse 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 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. |
||
| <-started | ||
|
|
||
| // join() must wait until the worker exits. | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| defer close(done) | ||
| join() | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| t.Fatal("join returned before worker exited") | ||
| case <-time.After(100 * time.Millisecond): | ||
| // join is correctly blocked waiting for worker completion | ||
| } | ||
|
|
||
| close(release) | ||
|
|
||
| select { | ||
| case <-done: | ||
| // goroutine joined successfully after worker exit | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("join function did not return within 2 seconds") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. New test validates the wrong invariant — it passes for the wrong reason due to the 💡 Details
To make this test meaningful, assert that the goroutine has actually completed by checking a side-effect (e.g., the // 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. |
||
| } | ||
|
|
||
| func TestFindLatestPublishedReleaseTag(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
<-doneand<-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 — afterjoin()has returned, defeating the stated contract.The goroutine body already checks
ctx.Err()and exits promptly on cancellation. Thectx.Done()arm in the join closure is a safety-valve that masks a goroutine leak rather than curing it.