internal/ext/synctestx: Hammer misuses sync.WaitGroup, so the thundering herd can silently not happen

[trendvidia]

internal/ext/synctestx.Hammer adds to its start barrier inside the spawn loop, which violates the sync.WaitGroup contract. The visible symptom is an intermittent panic under -race. The quieter symptom is that the barrier the helper exists to provide can fail to hold, so tests using it exercise less contention than they read as.

Found while running go test -race ./... on a fork of this repository. The code in question is unmodified from upstream main.

The code

func Hammer(count int, f func()) {
	if count == 0 {
		count = runtime.GOMAXPROCS(0)
	}

	start := new(sync.WaitGroup)
	end := new(sync.WaitGroup)
	for range count {
		start.Add(1)      // concurrent with goroutines already in start.Wait()
		end.Add(1)
		go func() {
			defer end.Done()
			start.Done()
			start.Wait()
			f()
		}()
	}

	end.Wait()
}

sync.WaitGroup documents the requirement directly:

Note that calls with a positive delta that occur when the counter is zero must happen before a Wait.

Here goroutine i can run start.Done(), dropping the counter to zero, and enter start.Wait() before the parent's start.Add(1) for iteration i+1 raises it again. That Add occurs when the counter is zero and does not happen before a Wait, which is exactly the case the contract excludes.

Two consequences

1. A panic, intermittently. Observed on go1.26.5 darwin/arm64 with the race detector:

WARNING: DATA RACE
Read at 0x00c000218058 by goroutine 12:
  github.com/bufbuild/protocompile/internal/intern_test.TestExhaust()
      internal/intern/intern_test.go:151

Previous write at 0x00c000218058 by goroutine 71:
  ...
Goroutine 71 (running) created at:
  github.com/bufbuild/protocompile/internal/ext/synctestx.Hammer()
      internal/ext/synctestx/synctestx.go:37
==================
panic: sync: WaitGroup is reused before previous Wait has returned

goroutine 97 [running]:
sync.(*WaitGroup).Wait(0xc000218050)
	sync/waitgroup.go:213
github.com/bufbuild/protocompile/internal/ext/synctestx.Hammer.func1()
	internal/ext/synctestx/synctestx.go:43
FAIL	github.com/bufbuild/protocompile/internal/intern	2.446s

The misuse is a contract violation rather than a property of one release, so this is not specific to the toolchain that surfaced it.

2. The barrier can silently fail to hold, which is the part worth fixing. When a goroutine's Wait returns early, it calls f() while the parent is still spawning. The thundering herd the helper exists to create never happens, and nothing reports it. The two callers in the tree today, internal/intern (TestHammer, TestExhaust) and internal/ext/syncx (log test), are then testing a weaker interleaving than their names claim, on an unknown fraction of runs.

Reproduction

Timing dependent: the window is one scheduling decision wide. It did not reproduce for me at -count=200 of the single test that tripped it, nor at -count=60 across -cpu 2,4,8,16. Hammering Hammer itself makes it deterministic within about a second:

func TestHammerRepeated(t *testing.T) {
	t.Parallel()
	for i := range 2000 {
		var calls atomic.Int64
		synctestx.Hammer(4, func() { calls.Add(1) })
		if got := calls.Load(); got != 4 {
			t.Fatalf("iteration %d: f ran %d times, want 4", i, got)
		}
	}
}

Run with go test -race. On the current code this panics; on the fix below it passes.

Suggested fix

Raise both counters before any goroutine exists, so every Add happens before every Wait and the barrier holds by construction:

	start := new(sync.WaitGroup)
	start.Add(count)
	end := new(sync.WaitGroup)
	end.Add(count)

	for range count {
		go func() {
			defer end.Done()
			start.Done()
			start.Wait()
			f()
		}()
	}

	end.Wait()

A close(chan struct{}) start gun would also remove the misuse, but it is a weaker barrier for this purpose: it releases when the parent reaches the close, whereas the WaitGroup form releases only once every goroutine has actually reached the barrier. That difference is the property Hammer is for, so keeping the WaitGroup seems right.

I verified the callers with the barrier actually holding, in case a real herd surfaced something the broken one had been masking: internal/intern and internal/ext/syncx under -race at -count=25, and at -count=10 across GOMAXPROCS 2, 8 and 16. All green.

Happy to open a PR with the fix and the regression test if that is useful.