Go Concurrency Mistakes That Keep Showing Up in Review

Reviewing Go code for a few years gives you a short list of concurrency mistakes that recur no matter who wrote the code. They are not exotic. They are the first-order slips: small things that are right most of the time, written by people who understand goroutines perfectly well, caught in review because someone read the diff with the right question in mind. This is that list, as a checklist. Each item has what the mistake looks like, why it is wrong, and what to write instead. The first two are demonstrated with output because they are the ones people refuse to believe until they see them.

For the bugs that survive review and tests, see concurrency bugs that tests will not catch. This list sits next to data races, the race detector and the rest of the concurrency patterns cluster. Everything ran on Go 1.25.6.

1. WaitGroup.Add inside the goroutine

The most common one. The Add call belongs before the goroutine starts, because Wait only knows about increments that have already happened. When Add is inside the goroutine, Wait can run before any goroutine has started, see a counter of zero, and return immediately:

package main

import (
	"fmt"
	"sync"
	"sync/atomic"
)

func main() {
	var wg sync.WaitGroup
	var finished atomic.Int32

	for range 100 {
		go func() {
			wg.Add(1) // too late: the main goroutine may already be past Wait
			defer wg.Done()
			finished.Add(1)
		}()
	}
	wg.Wait()
	fmt.Println("Wait returned with", finished.Load(), "of 100 goroutines finished")
}

Wait returned with 88 of 100 goroutines finished

In this run Wait returned with 88 of 100 goroutines finished, and the number changes from run to run. Wait returned while goroutines had not yet reached their Add. The fix is to call wg.Add(1) in the loop before go, or, on Go 1.25 and later, to use wg.Go(func() {...}), which does the Add and the deferred Done for you and removes this class of bug entirely (WaitGroup.Go). go vet flags the pattern in recent versions.

2. One err variable shared by goroutines

A function declares var err error and several goroutines assign to it. Every assignment is an unsynchronized write, so it is a data race, and the final value is whichever write landed last, not the first or the most serious error:

package main

import (
	"errors"
	"fmt"
	"sync"
)

func main() {
	var err error // shared by every goroutine below
	var wg sync.WaitGroup
	for i := range 4 {
		wg.Go(func() {
			if i%2 == 1 {
				err = fmt.Errorf("task %d failed", i) // racing writes
			}
		})
	}
	wg.Wait()
	fmt.Println("error:", err, "| is nil:", errors.Is(err, nil))
}

error: task 1 failed | is nil: false
==================
WARNING: DATA RACE
Write at 0x00c000012070 by goroutine 8:
  main.main.func1()
      example.test/lab/main.go:15 +0x90
  sync.(*WaitGroup).Go.func1()
      sync/waitgroup.go:239 +0x54

Previous write at 0x00c000012070 by goroutine 10:
  main.main.func1()
      example.test/lab/main.go:15 +0x90
  sync.(*WaitGroup).Go.func1()
      sync/waitgroup.go:239 +0x54

Goroutine 8 (running) created at:
  sync.(*WaitGroup).Go()
      sync/waitgroup.go:237 +0x78
  main.main()
      example.test/lab/main.go:13 +0x90

Goroutine 10 (finished) created at:
  sync.(*WaitGroup).Go()
      sync/waitgroup.go:237 +0x78
  main.main()
      example.test/lab/main.go:13 +0x90
==================
Found 1 data race(s)
exit status 66

The race detector reports the write-write conflict on err. The correct tools depend on intent: errgroup returns the first error and cancels the others (errgroup); a slice of errors with one slot per goroutine collects all of them; errors.Join merges the slice at the end (joining errors). What does not work is a shared variable.

3. Lock without defer Unlock

mu.Lock()
if err := validate(x); err != nil {
	return err // the lock is never released
}
mu.Unlock()

An early return, a panic or a new branch added later leaves the mutex locked forever. Write mu.Lock() and defer mu.Unlock() as a pair, on consecutive lines. When the critical section must be shorter than the function, put it in a small function or method of its own so the deferred unlock runs at the right time (mutexes).

4. Holding a lock across slow work

A defer mu.Unlock() at the top of a function makes the whole function the critical section, including the HTTP call, the log line and the callback in the middle. Every other goroutine waits for all of it. Copy what you need under the lock, release it, then do the slow part (lock contention). Calling a function value or an interface method while holding a lock is also how lock-ordering deadlocks are born, since you do not know what that code locks.

5. The receiver closes the channel

Only the sender knows when no more values are coming. If a receiver closes the channel, the next send from any producer panics. The rule: the goroutine that sends closes, and if there are several senders, a coordinator closes after they have all finished (the WaitGroup plus a closer goroutine, as in merging channels). A channel that never needs closing, because the consumer stops by context, is fine too.

6. Sending or receiving with no way out

func produce(out chan<- int) {
	for i := 0; ; i++ {
		out <- i // blocks forever if the consumer stops reading
	}
}

Every channel operation that can block should be in a select with a cancellation case. A bare send or receive inside a goroutine is a leak the day its counterpart disappears (goroutine leaks). Review question: what stops this goroutine?

7. A goroutine nobody waits for

go sendEmail(user) in a request handler returns immediately, and the process may exit before the email is sent, or a test may finish with the goroutine still running and touching freed state. Fire-and-forget is acceptable only when the work is truly optional. Otherwise the code needs a WaitGroup or errgroup owned by something with a lifetime, and the shutdown path waits for it (starting and stopping goroutines).

8. Loop variable capture on old modules

Before Go 1.22, for _, u := range users { go func() { use(u) }() } shared one u between all iterations, and most goroutines saw the last user. With go 1.22 or later in go.mod, every iteration has its own variable and the bug is gone (loop variables). It still appears in reviews of older modules, and in any code that takes the address of a loop variable and keeps it. Check the go line before assuming the new semantics.

9. time.Sleep as synchronization

"Wait a second for the goroutine to finish" is a guess about timing that passes on your laptop and fails in CI. Wait for a signal instead: a channel, a WaitGroup, a callback (testing concurrent code). In production code, a sleep in a retry loop is fine; a sleep standing in for a happens-before edge is a bug.

10. Unbounded goroutines

for _, item := range items { go process(item) } is fine for a hundred items and an outage for a million. Review any loop that starts goroutines for a bound: a worker pool, errgroup.SetLimit or a semaphore (bounded parallelism).

11. Mutex in a copied struct

A struct with a sync.Mutex passed by value or returned by value copies the lock, and the copies do not exclude each other. Methods that contain locks have pointer receivers, and go vet catches the copy (struct copies).

12. Value receivers on types that hold state

A close cousin of the copied mutex: a method with a value receiver, func (c Counter) Inc(), operates on a copy, so c.n++ changes the copy and is lost. It compiles and does nothing. If a type has any method that mutates it, or contains a lock, all of its methods should use pointer receivers, for consistency as much as correctness.

13. Ignoring the error from the group

g.Wait() returns the first error, and a call like g.Wait() on its own line discards it. The same applies to errgroup and Close methods returning errors in deferred calls. Check what Wait returns, and decide what an error means for the caller.

What newer Go versions fixed

Several of these mistakes have become harder to make as the language and tools improved, and it helps to know which. Go 1.22 gave each loop iteration its own variable, ending most of item 8. Go 1.25 added WaitGroup.Go, ending item 1 for code that uses it, and go vet gained a check for Add called inside the goroutine's function. The race detector has caught item 2 since the beginning. Go 1.23 fixed the time.After timer leak, so that one no longer belongs on a leak checklist for modules that declare it. None of this changes the review questions for items 3 through 7 and 9 through 12, which are about design, not language rules. When reviewing code in a module with an older go directive in go.mod, apply the older rules, because the language version, not the compiler you run, decides the semantics.

How I use the list

I do not run through thirteen items on every pull request. Two habits cover most of them. First, for every go statement, I ask who waits for it, what stops it and what it shares; that single question catches 1, 5, 6, 7, 10 and 13. Second, for every Lock, I look at the next line for the deferred unlock and at the lines until the unlock for anything slow; that catches 3, 4, 11 and 12. The rest are caught mechanically: go vet ./... and go test -race ./... in CI find 2, 8, 11 and 12, and a leak check finds what remains of 6 and 7. The Go memory model is the reference for why items 1 and 2 are wrong, and the sync package documentation states the WaitGroup.Add rule explicitly.