Merge #149448

149448: roachprod: fix race condition in ui/spinner r=golgeek,herkolategan a=srosenberg

The goroutine inside `Spinner.Start` could result
in `panic: sync: negative WaitGroup counter`.
The race can occur under specific scheduling.
It was observed while running `roachtest` in
local mode with `--count 1000000`.

The actual race occurs when `Spinner.Stop` is
executed _before_ the goroutine exits. Specifically, `waitGroup.Wait` is invoked _after_ `waitgroup.Add`, thereby resetting the semaphore. Finally, `waitgroup.Done` is executed by `defer`, causing the panic.

The fix moves `waitGroup.Add` outside of the goroutine.

P.S.: SA2000 [1] only checks for cases where `waitgroup.Add` is the first statement [2]. None of the other linters pick this up.

[1] https://staticcheck.dev/docs/checks/#SA2000
[2] dominikh/go-tools#360 (comment)

Epic: none
Release note: None

Co-authored-by: Stan Rosenberg <[email protected]>

Author: craig[bot]

pkg/roachprod/ui/spinner.go

@@ -118,9 +118,9 @@ func (s *Spinner) Start() func() {
 			fmt.Fprintf(s.out, "\n")
 		}
 	}
+	s.waitGroup.Add(1)
 	go func() {
 		defer s.waitGroup.Done()
-		s.waitGroup.Add(1)
 
 		var writer Writer
 		tickerDuration := 100 * time.Millisecond