Skip to content

Commit 7a3706f

Browse files
authored
[fix] eliminate select race in checkUpdateHint (#301 regression) + test isolation (#303)
* [fix] eliminate select race in checkUpdateHint that drops update hints (#301 regression) The two-goroutine design introduced in #301 has a scheduler race: the fetch goroutine's defer cancel() fires the instant it sends its result, making both the result case and tctx.Done() ready simultaneously in the collector's select. Go picks pseudo-randomly, dropping the result ~50% of the time on linux CI. Fix: collapse to a single goroutine that writes to a buffered out channel directly. With no select between "result" and "cancellation", the result is un-droppable by construction. Adds a 500-iteration stress test (TestCheckUpdateHintNeverDropsResult) as a regression guard. * [test] restore global rootCmd in TestExecute to fix order-dependent TestExamplePresent Execute() triggers cobra's InitDefaultHelpCmd(), permanently grafting a "help" subcommand onto the global rootCmd. Any later test iterating rootCmd.Commands() (e.g. TestExamplePresent) saw a runnable "help" with no Example and failed. Failure was order-dependent: ~80% of -shuffle seeds triggered it. Clean up by removing the cobra-added "help" command in TestExecute's t.Cleanup, restoring the pre-Execute command set for subsequent tests.
1 parent 6d663a2 commit 7a3706f

3 files changed

Lines changed: 37 additions & 23 deletions

File tree

cmd/root_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,14 @@ func TestExecute(t *testing.T) {
222222
// cancelled when Execute returns (via defer stop()). Reset it so
223223
// subsequent tests that call cmd.Context() get a live context.
224224
rootCmd.SetContext(context.Background())
225+
// Execute() makes cobra graft a default "help" command onto the global
226+
// rootCmd; remove it so later tests that iterate rootCmd's children
227+
// (e.g. TestExamplePresent) aren't polluted by execution order.
228+
for _, c := range rootCmd.Commands() {
229+
if c.Name() == "help" {
230+
rootCmd.RemoveCommand(c)
231+
}
232+
}
225233
})
226234

227235
if err := Execute(); err != nil {

cmd/update_hint.go

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,17 @@ var newUpdater func(string) *updater.Updater = updater.New
1818
// Returns nil if the version is dev, the check fails, times out, or is up to date.
1919
// The check is cancelled when ctx is done or timeout elapses — whichever comes first.
2020
func checkUpdateHint(ctx context.Context, version string, timeout time.Duration) <-chan *updater.CheckResult {
21-
ch := make(chan *updater.CheckResult, 1)
21+
out := make(chan *updater.CheckResult, 1)
2222

2323
if updater.IsDevVersion(version) {
24-
close(ch)
25-
return ch
24+
close(out)
25+
return out
2626
}
2727

28-
// Derive a timeout-scoped child so the HTTP request is actually cancelled
29-
// when timeout elapses, not just when the caller stops waiting.
28+
// Bound the check by timeout so the HTTP request is actually cancelled, not
29+
// just abandoned. u.Check honours tctx, so the goroutine always terminates
30+
// within timeout — no leak, and the result can never be lost to a racing
31+
// cancellation (there is no select between "result" and "ctx done").
3032
tctx, cancel := context.WithTimeout(ctx, timeout)
3133

3234
// Read newUpdater before spawning the goroutine: goroutine launch
@@ -37,26 +39,10 @@ func checkUpdateHint(ctx context.Context, version string, timeout time.Duration)
3739
defer cancel()
3840
result, err := u.Check(tctx)
3941
if err != nil || result.UpToDate {
40-
close(ch)
41-
return
42-
}
43-
ch <- result
44-
}()
45-
46-
// Return a channel that resolves with the result or nil when tctx expires.
47-
out := make(chan *updater.CheckResult, 1)
48-
go func() {
49-
defer cancel()
50-
select {
51-
case r, ok := <-ch:
52-
if ok {
53-
out <- r
54-
} else {
55-
close(out)
56-
}
57-
case <-tctx.Done():
5842
close(out)
43+
return
5944
}
45+
out <- result // buffered (cap 1) — never blocks
6046
}()
6147

6248
return out

cmd/update_hint_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,26 @@ func TestCollectHintValue(t *testing.T) {
152152
}
153153
}
154154

155+
func TestCheckUpdateHintNeverDropsResult(t *testing.T) {
156+
// Regression guard for the #301 select race: a successfully-fetched, newer
157+
// version must never be dropped. The race is scheduler-biased (reliably lost
158+
// on linux CI, won on macOS), so this asserts the invariant over many runs.
159+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
160+
w.Header().Set(hdrContentType, mimeJSON)
161+
_, _ = w.Write([]byte(`{"tag_name":"v99.0.0","assets":[
162+
{"name":"rimba_99.0.0_linux_amd64.tar.gz","browser_download_url":"https://github.com/lugassawan/rimba/releases/download/v99.0.0/rimba_99.0.0_linux_amd64.tar.gz"},
163+
{"name":"checksums.txt","browser_download_url":"https://github.com/lugassawan/rimba/releases/download/v99.0.0/checksums.txt"}]}`))
164+
}))
165+
t.Cleanup(srv.Close)
166+
overrideNewUpdater(t, srv)
167+
168+
for i := range 500 {
169+
if got := collectHint(checkUpdateHint(context.Background(), testVersionHint, 2*time.Second)); got == nil {
170+
t.Fatalf("iteration %d: update hint dropped (nil result) for a newer version", i)
171+
}
172+
}
173+
}
174+
155175
func TestCheckUpdateHintCancelledContext(t *testing.T) {
156176
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
157177
// A pre-cancelled context should cause the HTTP client to fail before

0 commit comments

Comments
 (0)