From ee35ad3a572de87ca04760a1438116906149578b Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Wed, 3 Sep 2025 09:25:49 +0400 Subject: [PATCH] fix: remove hold WaitGroup in TestConcurrentFetch (#19617) Fixes: https://github.com/coder/internal/issues/950 Pretty sure the intention of the `hold` wait group is to try to get the two goroutines that the test starts running at the same time. But, that should be the case for two goroutines started anyway. The use of `hold` doesn't actually guarantee concurrent execution of `Acquire`, just that both goroutines get as far as `Done()` --- the go scheduler could run them serially without incident. So I've chosen to just remove the use of `hold` to simplify. But, for posterity, the data race was due to incrementing by 1 in the loop along with the goroutine that calls Done. You could increment by 1 and then back down to 0 before the second iteration of the loop starts. This then causes a data race with calling `Wait()` in the first goroutine and `Add()` in the second iteration. c.f. https://pkg.go.dev/sync#WaitGroup.Add --- coderd/files/cache_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/coderd/files/cache_test.go b/coderd/files/cache_test.go index b81deae5d9..72a3482eeb 100644 --- a/coderd/files/cache_test.go +++ b/coderd/files/cache_test.go @@ -100,21 +100,15 @@ func TestConcurrentFetch(t *testing.T) { ctx := dbauthz.AsFileReader(testutil.Context(t, testutil.WaitShort)) // Expect 2 calls to Acquire before we continue the test - var ( - hold sync.WaitGroup - wg sync.WaitGroup - ) + var wg sync.WaitGroup + wg.Add(2) for range 2 { - hold.Add(1) // TODO: wg.Go in Go 1.25 - wg.Add(1) go func() { defer wg.Done() - hold.Done() - hold.Wait() _, err := cache.Acquire(ctx, dbM, fileID) - require.NoError(t, err) + assert.NoError(t, err) }() }