Chapter 18: Common Bugs and Code Review

18.1 Why You Ship Bugs You Already Know

The opening transcript is easy to dismiss as a trick — a package built to embarrass the tools. It was not, and this section is about showing that the result generalises.

18.1.1 The Book Is Already a Catalogue

The obvious shape for a chapter called “Common Bugs” is a list of bugs. That shape is unavailable, because the list already exists and it is seventeen chapters long.

A hundred more would not have helped. Why not is the thing worth understanding before reading further.

18.1.2 The Corpus, and the Rule That Chose It

A detection ratio without its corpus is not a measurement — it is a number you cannot disagree with, and the number moves a great deal depending on what went in the jar.

So here is the jar, and here is the rule, stated before the bugs were chosen: one bug per chapter, taken from what that chapter teaches. Sixteen chapters, sixteen bugs, identified by the chapter they came from. If the book’s own material survives the toolchain, the result is not a trick.

The rule is not neutral — it over-represents the bugs a teaching book finds worth teaching. It is, however, stated in advance, which is the part that matters. A reader can disagree with the rule; they cannot be misled by it.

The package ships at labs/go-concurrency/code/ch18/corpus/, so every number in the next subsection is one you can reproduce rather than one you have to take.

18.1.3 What Each Layer Caught

Four commands over that package. The tool outputs are deterministic and quoted verbatim.

Then there is a fourth layer, and it is the one nobody lists because it has no command of its own.

Go 1.27 adds one more runtime layer, and it sits between the free one and -race: the goroutineleak profile. Ask the runtime — pprof.Lookup("goroutineleak"), or /debug/pprof/goroutineleak — and it reports every goroutine blocked on a channel, mutex or Cond that the garbage collector can prove no runnable goroutine will ever reach. On this corpus that is B07 and the exercise's FetchAll; it says nothing about a goroutine asleep in time.Sleep, and nothing at all unless something collects it, which none of the three commands above does. It is the first tool that reads a lifetime fact — reachability — rather than syntax or memory, and Chapter 19 (§19.3.6) shows it in use.

That is not the race detector. It is the runtime’s own map-write check, compiled into every Go binary, on by default, with no flag to enable it and none to turn it off. It covers exactly one bug class, and it covers it probabilistically — it fires when two writes actually collide, not when the code is capable of colliding.

Which means the rate is a property of the program rather than of the bug, and it is worth seeing what it moves with. Here is B08, in full, because the next table is only checkable if you can see what was varied.

// Illustrative snippet — not a complete program
// B08 (ch8): unsynchronised map write.
func B08() map[int]int {
	m := map[int]int{}
	var wg sync.WaitGroup
	for i := 0; i < 8; i++ {
		wg.Add(1)
		go func(i int) { defer wg.Done(); m[i] = i }(i)
	}
	wg.Wait()
	return m
}

Two writers never collided in twenty runs. Sixteen collided in nineteen of twenty.

But read that as a fact about this program rather than about two goroutines, because the same two-writer race says something different elsewhere. Measured: the identical map write, at two goroutines, in a standalone binary that does nothing else at all — 20 of 20 runs died. Same bug, same writer count, opposite verdict, and the only difference is what else the process was doing.

That is the whole character of the layer, and it is a sharper version of “probabilistic” than a percentage would be. The check needs two writes to land in the same instant, so its coverage tracks collision opportunity — how much other work the runtime is juggling, how many cores it has, what else is scheduled. It is free, it asks nothing of you, and it is least reliable in exactly the place you would want to depend on it: a small unit test with two goroutines and nothing else running. It catches B08 sometimes. It has nothing to say about the other fifteen, ever.

16 bugs, one per chapter, from what that chapter teaches

go test ./...        |#                    0, or 1
go test -race ./...  |##                          1
staticcheck ./...    |####                        2
go vet ./...         |######                      3
─────────────────────────────────────────────────────────
union of all four    |##########                  5
caught by nothing    |######################     11

go test's own row is the runtime's map check: one bug,
about one run in six, and the only row here that is a
probability rather than a number.

Not a failure of the tools.  A statement about what kind
of thing a tool is able to know at all.

18.1.4 The Split Is Not Random

Look at what the tools caught and what they did not, and the boundary is sharp.

Every one of the four is a local, syntactic property of one expression. B09Read takes a B09Counter by value and that type has a sync.Mutex in it — decidable from the signature. WithCancel's second result is assigned to _. An unbuffered channel reaches signal.Notify. A for loop wraps a select with an empty default. You can see each of them in one function, knowing nothing about what the program is for. Each is wrong in every program, which is precisely why a tool can decide it: the analyzer cannot be wrong about your design, because it does not know your design.

Every one of the eleven requires knowing something the source does not say:

• Who closes this channel? (B03, B06) Nothing in the type says. chan int is the same type at both ends.
• Who waits for this goroutine? (B02, B14) The go statement records no answer.
• Is this loop supposed to be interruptible? (the cold open’s Poll) A function that takes a ctx and never reads it is a legal program, and the signature is the only place the promise was made.
• Is a million-element buffer a design or an accident? (B05) make(chan T, 1<<20) is the same expression either way.
• Whose context is this, and how long does its effect last? (B12) A ctx parameter and a package-level singleton are both just values.
• Does the invariant survive between these two operations? (B11, and the cold open’s Take) Each access is correctly synchronised. The gap between them is where the bug lives, and a gap is not a syntax.
• Do these two locks always go in this order? (B10) Each function is correct read alone; the cycle exists only across the pair.

18.1.5 Why Knowing Does Not Help

You know all sixteen. You met them in chapters that were about them, and you got them right in the exercises.

The reason that does not transfer is that a chapter hands you the frame for free. Reading §11.3.3 you already know the subject is check-then-act, so two critical sections stand out immediately. Reading a diff titled “add per-tenant quota” you are checking whether the quota is per tenant.

a chapter   "this section is about check-then-act"
            └─ the frame, free, before you read a line

a diff      "add per-tenant quota accounting"
            └─ a frame that is TRUE and points elsewhere

You find what you are looking for, and the diff's own
title decides what that is.

That is not carelessness. It is what attention is: a limited resource pointed somewhere, and the diff’s title points it at the feature. §18.7 is about pointing it somewhere else on purpose.

There is a second mechanism stacked on the first, and it is why experience alone does not fix this.

Concurrency bugs are usually in code that is not the point of the change. A diff that adds per-tenant quota changes the quota logic — and touches, in passing, a goroutine that already existed, a channel somebody else owns, an error path that now has one more branch. The concurrency lives in the plumbing the change flows through, and the plumbing is exactly what a reviewer skims because it barely changed.

The cold open’s bugs have that shape. Nobody set out to write a check-then-act quota; they set out to write a quota, and two critical sections are what “make it thread-safe” produced. Nobody set out to leak goroutines; they set out to return the first result, and the leak is what “take the first answer” produced in a function that had senders still running.

Naming the failures separately helps, because each has a different remedy.

Recognition fails in three ways, and each has a different remedy. Only the middle one is a knowledge problem — which is the awkward part, because it is the only one seventeen chapters of teaching can address.

You did not see the code. The bug is in a file the review never opened, or in the four lines everyone scrolled past because they were “just moving a call”. The remedy is §18.7's rule about what to read first, which is not the diff.

You saw it and it looked right. The common one, and §18.4's subject. Check-then-act looks especially right — every access locked, symmetrical, obviously careful. The remedy is a question you ask rather than a pattern you spot.

You saw it, knew it was wrong, and it was not your file. The social failure, and the one no chapter can fix. §18.7.10 comes back to it anyway, because a review process that reliably produces correct-and-ignored comments is not working.

18.1.6 What 488 Entries Are Actually For

None of this makes the catalogue a mistake. It makes it a reference rather than a defence, and the distinction is practical.

A reference is what you reach for once you know the mechanism. When §18.3 narrows an incident to “queueing, probably §17.5.1”, chapter 17's Common Mistakes rows are exactly what you want next — dense, specific, and organised by the thing you now know you have. That is the reading direction they were written for.

What they cannot do is stop you writing the bug, because at the moment of writing you do not know which of the 488 applies. That is not a defect in the entries; it is §18.1.5's frame problem again. A catalogue answers “what goes wrong with X”, and the author of a bug does not yet know they are doing X.

18.1.7 Common Mistakes

Summary: Why You Ship Bugs You Already Know

The book already holds 488 catalogued mistakes, so this chapter cannot be another list. Measured against a sixteen-bug corpus chosen by a rule fixed in advance — one bug per chapter, from what that chapter teaches — go vet found three, staticcheck found two of which one was already vet’s, -race found one more, and eleven were caught by nothing at all. go test with no flags found nothing you can rely on — it killed the process on the map write in one to five runs of twenty, which is the Go runtime’s own map check rather than any tool you chose to run.

The split is not arbitrary. The four the static tools caught are local syntactic properties of one expression, wrong in every program. The eleven that were missed each require a fact the source never states: who closes this, who waits for this, whose context this is, whether the invariant survives the gap. No analyzer will supply those, because the information is absent from the program rather than from the tool.

And knowing the bugs does not transfer, for two reasons that compound. A chapter hands you the frame for free and a diff hands you a frame that is true and points elsewhere; and concurrency bugs live in the plumbing a change flows through rather than in the change itself.

Self-Check Questions: Why You Ship Bugs You Already Know

18.2 What the Toolchain Actually Runs

§18.1 measured what go vet and staticcheck find when you run them, and that go test ./... found nothing at all. This section is about the gap between those two sentences, which is wider than almost anyone expects and closes in one line.

18.2.1 The Seven Analyzers

go vet is not one check. It is a bundle, and go tool vet help lists what is in it. Seven of the thirty-five are about concurrency.

Read that list against the chapters. copylocks is Chapter 9. lostcancel is Chapter 13. sigchanyzer is Chapter 15's cold open, which said at the time that go vet catches it. testinggoroutine is §16.2.6. waitgroup is §2.3. Five of the seven check for bugs this book taught you, which is a genuinely good sign about the analyzer set.

The problem is not the set. It is when the set runs.

18.2.2 go test Runs One of Them

go test runs vet before compiling, as a convenience. It runs a subset, and the subset is a literal in the Go source.

Measured, from $GOROOT/src/cmd/go/internal/test/test.go:

// Illustrative snippet — not a complete program
var defaultVetFlags = []string{
    // TODO(rsc): Decide which tests are enabled by default.
    // See golang.org/issue/18085.
    // "-appends",
    // "-asmdecl",
    // "-assign",
    "-atomic",
    "-bools",
    "-buildtag",
    // "-cgocall",
    // "-composites",
    // "-copylocks",
    // "-defers",
    "-directive",
    "-errorsas",
    // "-framepointer",
    // "-hostport",
    // "-httpresponse",
    "-ifaceassert",
    // "-loopclosure",
    // "-lostcancel",
    "-nilfunc",
    "-printf",
    // "-shift",
    // "-sigchanyzer",
    "-slog",
    // "-stdmethods",
    "-stdversion",
    "-stringintconv",
    // "-structtag",
    // "-testinggoroutine",
    "-tests",
    // "-timeformat",
    // "-unmarshal",
    // "-unreachable",
    // "-unsafeptr",
    // "-unusedresult",
    // "-waitgroup",
}

Read the commented-out lines rather than the enabled ones. -copylocks and -lostcancel are commented out — two of the three analyzers that found anything in §18.1.3. So is -loopclosure (Go 1.27 spells it that way; -rangeloops is still accepted as an alias), and so are -sigchanyzer, -testinggoroutine and -waitgroup: every one of the seven is named, and six of the seven are switched off. The one line Go 1.27 added is -stdversion, which flags a standard-library symbol newer than your go.mod's go directive — a fact about the source, not a heuristic, which is exactly the kind of check the automatic set is allowed to hold.

go vet's seven concurrency analyzers
────────────────────────────────────
atomic             OK   run by go test
copylocks          --   commented out of defaultVetFlags
loopclosure        --   commented out
lostcancel         --   commented out
sigchanyzer        --   commented out
testinggoroutine   --   commented out
waitgroup          --   commented out

One of seven, and it is the one that found nothing
in the corpus.

Here is the whole thing in three commands, on one package.

Three verdicts on one package. The silent one is the command your CI runs on every commit, and a team whose pipeline is go test -race ./... — a perfectly respectable pipeline, and the one this book has been recommending — has never run copylocks on their codebase.

18.2.3 The One-Line Remedy, and What to Put in CI

This is the shortest fix in the book, and it has two forms.

Choose deliberately. -vet=all is one flag and gives the strongest guarantee: nobody can run the tests without the analyzers. The cost is that a vet finding now blocks your tests, which for a heuristic analyzer is occasionally the wrong trade on a Friday afternoon. A separate go vet ./... step reports independently, gives a cleaner failure message, and can be made non-blocking while a team works through a backlog. Teams that have been bitten pick the first. Either is enormously better than the default, and the default is what you have unless somebody chose.

One thing the default now does on its own, since Go 1.27: stdversion is in the automatic set, so a module whose go directive is older than a standard-library symbol it uses fails go test even without -vet=all. Measured, on go1.27.1: a go 1.25 module calling errors.AsType stops at errors.AsType requires go1.26 or later (module is go1.25) and FAIL [build failed], before a single test runs. The transcript above and the remedy are unchanged; this is the one finding the default set will hand you unasked.

The full recommendation is four lines, and it is worth stating concretely because “run the linters” is the kind of advice that never gets implemented.

Three notes on the shape.

Vet and staticcheck belong before the tests, not after. They are fast — seconds on a large module — and a vet finding usually means the test run would have been meaningless anyway. Putting them first also gives a cleaner failure: copylocks at a line number beats a test failure three layers down.

-race does not belong on every package. §16.3.5 measured the cost between 1.02× for code that shares nothing and about 26× for code that synchronises constantly — ordered inversely to how much real work each operation does, so it is most expensive exactly where it is most valuable. Racing the packages that own concurrency on every push and the whole tree nightly is the shape §16.3.6 argued for, and it is still right.

None of this catches the eleven. That is the honest framing to hold onto when proposing the change. The CI above costs almost nothing, has no false positives worth the name on this corpus, and leaves every bug in §18.4 exactly where it was. The failure mode of a linting initiative is a team that adopts one and concludes the concurrency problem is handled.

18.2.4 What staticcheck Adds

The two tools are usually presented as alternatives. On concurrency they are not: their sets barely intersect.

On §18.1's corpus, staticcheck found SA5004 — the spinning for+select loop — where go vet was entirely silent, and SA1017, which was sigchanyzer's bug from the other side. One new, one shared, out of three vet findings. That is the overlap in miniature, and it points both ways.

The corpus was one bug per chapter rather than one bug per checker, so it under-represents what staticcheck can do. Here are two more checks that fire where go vet says nothing.

SA2003 is the one to notice. mu.Lock() followed by defer mu.Lock() is one character from defer mu.Unlock(), it produces a self-deadlock on the next call (§10.3), and it is invisible when reading quickly because the two lines look symmetrical. go vet has nothing to say about it.

Two of the vet entries are worth a closer look, because what they fire on is narrower than the names suggest and the difference decides what a clean run means.

copylocks works through noCopy, so it catches a sync.Once or a sync.WaitGroup embedded three levels down, and it names the chain: Copy passes lock by value: Init contains sync.Once contains sync.noCopy. What it cannot catch is a lock copied through an interface or a reflect call, because the value’s type is not statically known there. That is rare, and worth knowing only as the boundary: copylocks is a type check, so it sees exactly as much as the types do.

lostcancel is genuinely good at the _ case. It is much weaker once the cancel is stored in a struct field or handed to another function, because then “is it called?” stops being a local question — the same boundary as everything else in §18.1.4.

Most teams do not run the two tools directly; they run an aggregator, and it is worth knowing what that changes.

The encouraging half: a bundled runner enables govet with its full analyzer set, not go test's subset, and tags each finding with the analyzer that produced it, which is more than go vet itself tells you. If you already run one of these, §18.2.3's remedy may already be in place.

The other half is the ceiling, and it has two parts. Four findings out of sixteen bugs — the same four, because the bundle changes who invokes the analyzers and not what an analyzer can know. And the staticcheck set it ran was narrower than staticcheck ./...: run directly, staticcheck reported SA1017 as well.

What go fix Adds

One command in the toolchain is not a reporter at all, and it belongs in a different part of the pipeline from everything above. Go 1.26 rebuilt go fix as the home of the modernizers — rewriters, built on the same analysis framework as go vet, that update a codebase to current idioms and library APIs. Go 1.27 adds four (atomictypes, embedlit, slicesbackward, unsafefuncs), removes fmtappendf, and renames waitgroup to waitgroupgo. Two of them mechanically apply this book's own recommendations: waitgroupgo rewrites the wg.Add(1) / go / defer wg.Done() triple into wg.Go (§2.3), and atomictypes rewrites atomic.AddInt64(&x, 1) and its declaration into the typed atomic.Int64 API (§11.5). A third, errorsastype, moves errors.As to Go 1.26's generic errors.AsType.

Three things about it are easy to get wrong. First, the rename is a go fix rename, not a go vet one: vet's analyzer is still called waitgroup, defaultVetFlags still spells it -waitgroup, and §18.2.1's row keeps its name — the modernizer was renamed precisely so the two would stop colliding. Second, the count in §18.2.1 is unchanged: measured, go tool vet help on go1.27.1 lists thirty-five analyzers and the same seven concurrency ones. Third, the modernizer suite deliberately has no bloop, because a benchmark rewrite can skew results — so go fix will not migrate b.N to b.Loop, and a go fix ./... line in CI must not be read as having done so. Run it the way you would read a colleague's diff: it produces edits, not findings, and the edits still want a reviewer.

18.2.5 -race Is a Third Tool, and It Needs a Test

The obvious objection to all of that is that Go ships a third tool, and it is the one everybody actually trusts. -race is not a linter. It is a runtime detector, and that difference decides what it can and cannot do for you.

Zero findings, and an exit code of zero to go with them. go vet still reports its three on that same file and staticcheck its two, because they read the source. The bugs have not gone anywhere — a runtime detector with nothing to run has nothing to say, and the way it says so looks exactly like success.

One finding, on the map write. -race executed the other four. It watched a goroutine leak, it watched a cancel go undiscarded, it watched an unjoined goroutine run, and it watched a sync.Once capture a caller’s context and hand the resulting error to a second caller who had no deadline at all — and it said nothing about any of them, correctly, because none of them is a data race.

What separates the four layers is not how much they catch but what each one reads. go vet and staticcheck read the source, so a compile is all they need. -race reads an execution, so it needs a test that reaches the bug and then needs the window to open on that run. The runtime’s map check reads a collision, so it needs the window to open and asks nothing of you at all.

So -race has a coverage story with two multipliers where the static tools have one. A static tool’s reach is the shapes someone wrote a check for. -race's reach is the code your tests execute, times the probability the window opens on that run.

18.2.6 The Blind Spot Both Tools Share

§16.2.6 measured that go vet's testinggoroutine analyzer misses t.Fatal inside wg.Go. The obvious next question is whether the other tool covers the gap.

Two independently written analysis engines, the same blind spot, on the two idioms this book has recommended since Chapter 14. Both look for a go statement; wg.Go and errgroup.Go start goroutines by calling a method, and a method call is not a go statement. The gap will widen rather than close: Go 1.27's waitgroupgo modernizer moves code from the shape the analyzers check to the shape they do not.

Note the second row, because it corrects something worth correcting: both tools trace into the helper. Vet reports go helper(t) with the note *“(helper calls (\testing.T).Fatal)”, and staticcheck names the t.Fatal inside it rather than the go statement. The claim to keep is the narrow one — they follow a direct call from a go statement, and they do not follow a function value handed to wg.Go.

That two independent engines agree on what they trace and on what they miss is worth more than either gap alone: it means the omission is a property of the problem, not a bug report you should file.

18.2.7 Your go.mod Decides Which Bugs You Can Have

loopclosure deserves a subsection because it is the one place the toolchain is doing something subtler than it gets credit for.

Loop-variable capture was the most famous bug in Go for a decade. Go 1.22 gave each iteration its own variable and the bug stopped existing — for code whose go.mod declares 1.22 or later. Older modules keep the old semantics, so the bug is still real there. loopclosure handles both.

The analyzer is correct in both directions and so is the program: at go 1.21 all three goroutines see the last value; at go 1.26 each sees its own, in whatever order the scheduler picks.

Now change the shape, keeping the bug. Instead of a go statement, append the closures to a slice and call them afterwards.

// Illustrative snippet — not a complete program
func printAll() {
    var fs []func()
    for _, u := range []string{"a", "b", "c"} {
        fs = append(fs, func() { fmt.Print(u, " ") })
    }
    for _, f := range fs {
        f()
    }
}

At go 1.21 that program has the identical bug — same wrong output — and vet says nothing, because loopclosure checks go and defer statements in a loop body and this is neither.

18.2.8 When the Tool Is Wrong

Turn on six analyzers that were not running and some of them will report code you meant. That is not a reason to turn them off again, and it is worth deciding in advance how you will handle it, because the decision made under deadline pressure is always “disable the linter”.

Three honest responses, in descending order of preference.

Change the code. More often than people expect, the analyzer is describing a real smell even when it is not describing a real bug. copylocks fires on a value receiver for a type containing a sync.Mutex — and a type containing a mutex almost always wants a pointer receiver anyway, so the finding is a design note delivered by a robot.

Suppress it, at the line, with a reason. Every tool supports this and the form matters:

// Illustrative snippet — not a complete program
func (c *Client) start(ctx context.Context) {
    // The analyzer cannot see that cancel is stored on c and
    // called in c.Close. Verified: TestClientClosesCancel
    // covers it.
    //nolint:govet // lostcancel: cancel is called in Close
    ctx, c.cancel = context.WithCancel(ctx)
    go c.loop(ctx)
}

A suppression with a reason and a named test is documentation. A bare //nolint teaches a future reader nothing and is indistinguishable from the ones somebody added to make the build pass.

Disable the check repository-wide — and write down when it comes back. Sometimes a check does not fit a codebase, and one line of configuration is honest where four hundred suppressions are not. What makes it honest is the second half: a comment saying what would have to change for it to be re-enabled. Without that, a disabled check is indistinguishable from a check nobody knows about.

18.2.9 Common Mistakes

Summary: What the Toolchain Actually Runs

go vet ships seven concurrency analyzers and go test runs one of them. Measured from defaultVetFlags in the Go 1.27 source: -copylocks, -lostcancel, -loopclosure, -sigchanyzer, -testinggoroutine and -waitgroup are all present and all commented out; atomic is the only concurrency analyzer switched on, and the one line 1.27 added is -stdversion. The consequence is three verdicts on one package — go test says ok, go vet reports three bugs, go test -vet=all reports all three and fails the build.

The conservatism is correct rather than a bug: a vet finding fails the build before any test runs, so the automatic set must not contain heuristics. That makes it a good engineering decision and a poor coverage decision, and the fix is one line in either of two forms.

staticcheck is a complement rather than an alternative — measured, it found SA5004 where vet was silent, and SA2001 and SA2003 fire on a package vet passes clean. -race is a third kind of thing entirely: measured, it reports nothing at all on a package with no test files, and one of sixteen with tests, having executed four of the others and correctly said nothing.

And both static tools share a blind spot with the same cause, twice over. Measured, each flags t.Fatal in a go statement, each traces through a helper, and neither flags wg.Go or errgroup.Go. loopclosure shows the same shape on a different bug: version-aware and correct about the version, and at go 1.21 reporting the go form of loop capture while missing the slice form, though both programs print c c c. A tool checks the shapes somebody wrote a check for.

Self-Check Questions: What the Toolchain Actually Runs

18.3 The Reverse Index

This is the section to bookmark, and it is the one thing seventeen chapters could not give you.

18.3.1 Chapters Index by Mechanism; Incidents Arrive as Symptoms

A chapter is an index by mechanism. Chapter 10 is called Deadlocks and everything in it is a deadlock; that is what makes it teachable. It is also what makes it useless at 3 a.m., because at 3 a.m. you do not have a mechanism. You have a graph that is the wrong shape, and finding the mechanism is the entire problem.

So this section inverts the book. Rows read symptom → candidate mechanisms → where it was measured, and they are grouped by where you see the symptom, because that is what you have in front of you. Nothing here explains anything; every row points at the chapter that already did.

a chapter                        an incident
─────────                        ───────────
starts with a mechanism          starts with a symptom
"here is what a deadlock is"     "p99 tripled at 14:20"
      │                                    │
      ▼                                    ▼
teaches you to recognise         you need candidates,
that mechanism next time         ranked, with evidence
                                           │
                                           ▼
                                 §18.3 is the arrow
                                 pointing the other way

18.3.2 Symptoms You See on a Dashboard

18.3.3 Symptoms You See in a Log or a Crash

18.3.4 Symptoms You See in Your Own Test Suite

18.3.5 Symptoms You See in a Diff

The fourth entry point, and the one this chapter is building toward. These are not failures yet — they are shapes that predict a category, which is exactly what makes them reviewable.

Every row is visible without running anything, which is the only reason review is worth doing at all. And every row is a prediction rather than a verdict — each shape is correct in some programs, which is exactly why the third column is a question.

18.3.6 A Worked Narrowing

An alert fires: p99 on the checkout endpoint has gone from 80 ms to 2.1 s. p50 is unchanged at 40 ms. CPU is flat. The dependency’s dashboard shows normal error rate and normal latency.

Four facts, and the index turns them into two candidates.

p99 up, p50 flat is the queueing row: most requests are fine and a subset is waiting. Candidates are §17.5.1 — capacity spent as a latency budget — and §17.4.3's head-of-line blocking behind a large acquire.

CPU flat confirms it and adds one. The machine is not doing work; it is the “everything is parked” row, which points at the same two plus §17.3.2's limiter queue.

The dependency is healthy removes a whole branch. This is not a slow downstream. It is our own admission control.

Seventeen chapters narrowed to three sections, from four numbers, in about a minute. The next step is not in this chapter — it is Chapter 19's, and it is to look at where the parked goroutines are parked.

What the index did not do is tell you the answer. It told you which three sections to re-read and which twelve chapters to stop thinking about, which at 3 a.m. is most of the value available.

18.3.7 Using It Without Fooling Yourself

Three rules, because an index like this is easy to misuse.

A candidate is a hypothesis, not a diagnosis — so read the whole row before testing any of it. Every row lists more than one mechanism deliberately, ordered by how often each turns out to be the cause rather than by severity, and a row with four candidates is telling you the symptom is genuinely ambiguous. The failure mode of symptom-driven debugging is latching onto the first plausible entry, which is usually the one you debugged most recently. Confirming which candidate it actually is belongs to Chapter 19, and this section hands over there rather than guessing.

Symptoms compose, and the first one you fix hides the rest. A service can have a leak and a misclassifying breaker, and the breaker’s refusals will mask the leak’s latency until the breaker is fixed. When a symptom half-improves after a change, that is evidence of a second mechanism rather than of a bad fix. Re-enter the index with the residual symptom rather than the original one.

Then go to the chapter, not to the code. Every candidate here has a section that measured it, and that section tells you what the mechanism looks like in source, which is what you need to search for. Reading your own code for a bug you have not characterised is how afternoons disappear.

18.3.8 Extending It for Your Own System

These tables index this book. The version worth having indexes your service, and it is built the way chapter 10's table was: from incidents you already had.

The procedure is three lines per incident, written at the end of the postmortem while the details are still true.

• What did the graph look like? Not what the bug was — what you saw first. “Checkout p99 tripled, p50 flat” is a row. “We had a check-then-act in the quota” is not; that is the answer, and answers are what the row points at.
• What did you rule out, and how fast? The branches you eliminated are half the value (§18.3.6).
• Where does it point? A file, a section, a previous incident.

Two properties separate a table people use from one they do not.

It is indexed by the first symptom, not the root cause. The temptation after a postmortem is to file it under the mechanism, because that is what you now understand. Nobody searches by mechanism at 3 a.m.; they search by the graph in front of them, and an index filed under the answer can only be searched by somebody who already has it.

It contains your own systems' symptoms. “Queue depth on the ingest topic climbs while consumer lag is flat” is worth ten generic rows, because it names things your team can see on a dashboard they already have open.

18.3.9 Common Mistakes

Summary: The Reverse Index

Chapters index by mechanism because that is how you learn a mechanism. Incidents arrive as symptoms, which is the opposite direction, and no single chapter can invert the book because no chapter sees the others.

This section is that inversion, grouped by where you see the symptom: a dashboard, a log or a crash, your own test suite, and a diff. The fourth group is the one this chapter is building toward — ten shapes that predict a category, each with the question that settles it.

Nothing here re-derives anything, which is what keeps it usable: a reverse index that explains is a chapter, and a chapter is what you already have. It is not Chapter 10's table either. That goes deeper on one failure mode and should be your first stop for a wedged process; this is shallower across seventeen chapters. And two habits make it a procedure rather than a lookup: read the whole candidate row before testing any of it, and go to the chapter before you go to your own code.

Self-Check Questions: The Reverse Index

18.4 What Only a Reader Catches

Eleven of sixteen. This section is about why, and the answer is more useful than a list of eleven bugs, because it is a taxonomy that applies to code this chapter never mentions.

18.4.1 Three Reasons a Bug Is Invisible

A tool reasons about your program. It does not reason about your intent. Every bug the tools missed violates something you meant and never wrote down, and there are three kinds of thing you can mean.

a PROTOCOL
  │ who closes it · who sends on it · who may call twice
  └─▶ the rule lives in two files, and the analyzer
      reads one of them

a LIFETIME
  │ who waits for it · whose context · what happens on
  │ the early return
  └─▶ the rule spans a call boundary the analyzer
      cannot cross

an INVARIANT
  │ one grant per unit of quota · balance never negative
  └─▶ the rule is a fact about your domain and appears
      nowhere in the types

A tool proves things about syntax and about memory.
None of these three is a fact about either one.

The categories are predictive rather than descriptive. Given an unfamiliar piece of concurrent code, deciding which of the three applies tells you what question to ask next — and the question, not the bug, is what §18.7 turns into a procedure.

18.4.2 A Property of a Protocol

A protocol is a rule shared between two pieces of code and held by neither. The canonical one is Chapter 3's, and Chapter 3's exercise is named after it: Who Closes?

A channel’s type says what flows through it. It says nothing about who may close it, how many senders there are, or whether a second close is possible. That information exists — the program does not work without it — but it lives in the programmer’s head, in a comment if you are lucky, and in the review if you are disciplined.

// Illustrative snippet — not a complete program
// producer.go -- legal, and correct in isolation
func produce(out chan<- int) {
    defer close(out) // the sender closes. Correct.
    for i := range 10 {
        out <- i
    }
}

// Illustrative snippet — not a complete program
// consumer.go -- also legal, also correct in isolation
func consume(in chan int) {
    for v := range in {
        if v > 5 {
            break // done early
        }
    }
    close(in) // "tidying up". Not correct.
}

No tool can see it, because the rule is in neither file. The sender’s file contains a close. The receiver’s file contains a close. Both are legal Go; the illegality is that there are two of them, in files the analyzer examines separately, and nothing in the type chan int records who owns the closing.

Notice which symptom you get. If consume closes first, the producer’s next send panics with send on closed channel. If the producer finishes first, consume's close panics with close of closed channel. If the producer happens to be between sends when the receiver breaks out, nothing happens at all and the code ships. One rule, three outcomes, chosen by the scheduler — which is §16.1's opening problem arriving in production code rather than in a test.

The review question: who else touches this, and what does each of them assume? If answering it requires opening a second file, you have found a protocol, and the rule is worth a comment at the point where it is easiest to break rather than where it is defined.

Here is the change that breaks one, and it is a five-line addition to a function nobody otherwise touched.

// Illustrative snippet — not a complete program
func Collect(ids []string) <-chan Result {
    out := make(chan Result)
    go func() {
        defer close(out)
        for _, id := range ids {
            out <- fetch(id)
        }
    }()

    // Everything below is what the diff added. Nothing above
    // it was touched.
    go func() {
        for r := range retries() {
            out <- r // a second sender
        }
    }()

    return out
}

The close is untouched — still deferred, still on the goroutine that used to be the only sender. It now fires while the second goroutine is still running, and the next out <- r panics; or, if the timing goes the other way, the second sender blocks forever after the receiver has stopped.

A reviewer looking at the close sees nothing wrong, because nothing about the close changed.

18.4.3 A Property of a Lifetime

go f() starts a goroutine and records nothing about how it ends. §2.1's Four Questions exist because the language provides no place to state the answers.

The bugs are all one question unanswered:

• Who waits for this? Nobody, and the goroutine outlives the request (§2.4).
• Whose context is this? The first caller’s, cached forever (§18.6.1).
• What happens on the early return? The senders stay parked (§14.3, §18.6.5).
• When does Done run? Not on the panic path, unless it is deferred (§2.3).

A tool sees go func() { ... }() and has no way to know whether the absence of a join is a bug or the design. §2.5's fire-and-forget section exists precisely because sometimes it is the design. The one partial exception, since Go 1.27, is after the fact: the goroutineleak profile can prove that a parked goroutine will never wake, which is a lifetime fact — but only once it is parked, and only for parks on a sync primitive.

go vet's waitgroup analyzer marks the edge of what is achievable here, and two cases side by side show exactly where the edge is.

// Illustrative snippet — not a complete program
func addInsideTheGoroutine(wg *sync.WaitGroup) {
    // vet reports this: WaitGroup.Add called from inside new
    // goroutine
    go func() {
        wg.Add(1)
        defer wg.Done()
        work()
    }()
}

// Illustrative snippet — not a complete program
func doneOnOnePathOnly(wg *sync.WaitGroup) {
    // vet says nothing about this
    wg.Add(1)
    go func() {
        if !ready() {
            return // wg.Done never runs
        }
        work()
        wg.Done()
    }()
}

The first is a local shape: the Add is textually inside the go func, and that ordering is wrong in every program, so an analyzer can decide it alone. The second is wrong only if ready() can return false — a fact about a function in another file, on a path that may never be taken during analysis. Same package, same WaitGroup, and only one of them is decidable.

The review question: if this function returns right now, what is still running, and who is waiting for it? Ask it at the go statement and again at every return between there and the end.

“On which paths” is where the bugs are.

Every row but the first is a place where the author’s attention was on something else — the same mechanism §18.1.5 described for reviewers. That is not a coincidence: the paths that skip the join are precisely the paths that are about something other than the goroutine.

One lifetime fact is worth stating because it surprises people who have done everything right. §4.5 established that select chooses uniformly among ready cases, and §13.5 that a cancelled context makes Done() ready forever. Put them in one worker loop and both cases are ready on every iteration after cancellation, so the loop exits after a geometric number of turns rather than at once.

Whether that matters is a property of the work. If a unit is idempotent and cheap, the loop is correct as written. If it charges a card or sends an email, “we cancelled and it did six more” is a defect that will never reproduce on demand, because it is a coin-flip tail. Where it matters, §13.5.4's shape is the fix: check ctx.Err() unconditionally before the select, so cancellation wins outright rather than competing.

18.4.4 A Property of an Invariant

This is the category §16.3.4 promised this chapter would cover, and the promise was specific: race conditions with no data race are found by tests written against an invariant rather than an access pattern, and by code review. Not by -race.

Here is why, stated as sharply as it goes: the detector verifies that your synchronisation is present. It cannot verify that it is sufficient.

The cold open’s Take is the canonical shape. The invariant is one sentence and it appears nowhere in the code: across any number of concurrent callers, Take returns true at most quota times.

Three things in that table are worth sitting with.

The detector reported nothing, across every run, and it was right to: s.quota is read and written only under s.mu, so every pair of accesses is ordered by a real synchronisation edge. A WARNING: DATA RACE here would have been a false positive.

The plain build found nothing either — not once in a thousand rounds, and not at sixty-four callers when the same harness was rerun with more contention. The window between Unlock() and the next Lock() is a few nanoseconds wide, and the mutex itself serialises the callers that would have to hit it.

And under -race the bug appears every run. That is §16.3.2's finding in its most useful form: a race build randomises the scheduler, so it is a partial scheduling fuzzer as well as a detector. -race is the only tool that makes this bug visible, and it does so by perturbing rather than by detecting, and it reports nothing about what it surfaced.

caller A            quota            caller B
────────            ─────            ────────
Lock                  1
read 1 > 0             ◀──▶  read 1 > 0    both see 1
Unlock                                     both pass
   │                                         │
   ▼                                         ▼
Lock                                       Lock
quota--               0                    quota--   -1
Unlock                                     Unlock

Every access locked.  No data race.  Two grants
from a quota of one.

The lock protected the variable.  Nothing protected
the decision.

The tell in the source is structural and you can train yourself to see it: two critical sections where there should be one, with a decision carried between them. Unlock followed by an if that depends on what was read under the lock is the signature.

The review question: what is true across callers that is not true of any one of them? Write the sentence down. If it names more than one field, or names one field at two moments in time, one lock per access will not hold it.

The category is broader than check-then-act, and three shapes cover most of it.

Check-then-act, above: read state, decide, write, with the lock released in between.

Lost update: read a value, compute from it, write it back — the same shape with the if implicit. n := c.Load(); c.Store(n+1) is check-then-act without a check, and §11.1.3's Add exists so that you do not write it. §18.6.7 measures the version of this that gets past review because the type’s name says it is safe.

Cross-object invariants: two objects, each internally consistent, with a rule that spans them. §11.3.1 measured this — two atomics, one invariant — and the fix is the same every time: the invariant needs one owner, and the owner needs one critical section.

18.4.5 Telling Which Category You Are In

The categories are only useful if you can classify unfamiliar code quickly, so here is the decision. It takes about ten seconds.

does the rule involve code in another file?
  yes ──▶ PROTOCOL
            who closes it · who sends · who calls twice
            go and read the other file

does the rule involve a moment in time?
  yes ──▶ LIFETIME
            who waits for it · whose context is this ·
            what happens on the early return
            go and read every return between here and
            the end of the function

does the rule involve a word from your own domain?
  yes ──▶ INVARIANT
            quota · balance · exactly once · at most N
            go and write the sentence down

none of the three ──▶ probably an analyzer's job
                      run go vet ./... and move on

The last branch matters as much as the first three. If a shape is wrong in every program regardless of context — a mutex copied by value, a discarded cancel, an unbuffered signal channel — then it is exactly the kind of thing an analyzer decides, and a reviewer spending attention there is spending it in the one place it is not needed. §18.2's whole argument is that those tools should be running; the corollary is that once they are, you can stop looking for what they find.

Two refinements from practice.

A change can touch two categories, and the fix differs by which you pick. §18.6.5's happy-path buffer is a lifetime bug if you frame it as “these goroutines never exit” and a protocol bug if you frame it as “the sender and the receiver disagree about how many sends there are”. Both framings find it. The lifetime framing produces len(replicas); the protocol framing produces a comment explaining the capacity, which is arguably the better fix because it survives somebody adding a replica.

When you cannot classify it, that is the finding. Code you cannot place in one of the three is usually code whose rule has not been decided yet — and asking the author which rule they meant is a better review comment than any of the specific questions, because it is the question they have not answered themselves.

18.4.6 Making the Invisible Fact Visible

The three categories share a cause: a fact the source does not state. That suggests the constructive move, and it is the one thing in this chapter that reduces the review burden rather than adding to it — write the fact down where the compiler or a test can hold you to it.

Each category has a different best answer.

Protocol → a type. §6.4's argument, arriving with a new justification. A function that takes chan<- T cannot close it, because the compiler says so; a function that takes <-chan T cannot send. Most ownership bugs become compile errors the moment the direction is written down at the boundary. This is the only one of the three where the fact converts cleanly into something mechanical.

Lifetime → a test. There is no type for “this goroutine is joined”. There is a test: §16.5's goleak.VerifyNone, or a synctest bubble whose deadlock panic names the line that started the leak. Neither proves the design; both catch the regression, which is what you actually need once the review has happened one time.

Invariant → an assertion, in a test that varies the schedule. §16.7.4's randomised model check is the general form: run random operations against an obviously-correct sequential reference and compare. The exercise’s gate 2 is the small version — 200 rounds of concurrent Take against a stated limit — and it is the shape to copy, because it asserts the invariant rather than the access pattern.

Notice what this does to the economics of a review. A finding you fix is worth one bug. A finding you fix and convert into a type or a test is worth every future instance — and the conversion is usually smaller than the discussion about whether the bug was real.

18.4.7 Common Mistakes

Summary: What Only a Reader Catches

The eleven bugs no tool caught are invisible for exactly three reasons: they are properties of a protocol (a rule shared between files and held by neither), a lifetime (a relationship across a call boundary the analyzer cannot cross), or a domain invariant (a claim about your problem that appears nowhere in the types).

The invariant category is the sharpest case and the one §16.3.4 sent here. Measured, a quota of one granted twice in 3 to 9 rounds of 200 under a race detector that reported nothing at all — and in zero rounds of a thousand without one, because a race build randomises the scheduler. The tool that could not report the bug is the only tool that surfaced it.

Each category yields a question, and the question is what survives a language change: §18.2.7 measured a bug that one line in go.mod removes from the language, and every checklist naming it is now wrong for half its readers. Then there is a constructive move that most reviews skip — write the missing fact down where a compiler or a test can hold you to it. Protocol becomes a directional type, lifetime becomes a leak test, invariant becomes an assertion under concurrency.

Self-Check Questions: What Only a Reader Catches

18.5 The Misclassification Family

Four chapters end on the same bug wearing four costumes. No one of them could say so, because each saw only its own costume.

18.5.1 One Shape, Four Chapters

Chapter 17 noticed the family and named three of its members: “That is the shape §14.1 named and §15.4 met again at the process boundary, arriving here a third time.” It could not do more than notice, because a chapter about rate limiting cannot spend its length on chapters 14 and 15. This section finishes the job.

The shape is one sentence: the system decides what a thing means, decides wrong, and every layer that has a dashboard reports success.

Read the middle column downward. Every row is a value filed under the wrong heading — and in every case the wrong heading is the one that looks like success, or like somebody else’s fault.

§14.1.1 — the go keyword severs the error chain. A goroutine’s return value goes nowhere. The failure mode of concurrent error handling is not a wrong error; it is silence, and silence is indistinguishable from success at every layer above it.

§15.4.4 — a 200 OK with a truncated body. Cancel the base context alongside Shutdown and a handler returns politely on ctx.Done(). net/http computes a Content-Length for what was actually written, or terminates the chunk stream correctly, so the client reads a 200 OK to completion with no error of any kind and stores a truncated result. The sting is that handling cancellation correctly is what makes the truncation invisible — a handler that panics with http.ErrAbortHandler breaks the connection mid-message, and the client at least sees an unexpected EOF, which is true.

§17.3.4 — reservations abandoned without Cancel. Measured there: ten abandoned reservations left the limiter ten tokens in debt and cost the next caller 110 ms against 10 ms. The limiter has recorded ten requests as sent. Nothing was sent.

§17.6.5 — caller impatience recorded as downstream failure. Measured there: against a downstream that never failed once, a predicate of err != nil cost 200 of 200 requests in the second wave — a full outage caused by a healthy dependency and impatient callers.

§17.4.3 belongs here too, in a quieter register. Measured there: capacity 8, six units free, one goroutine queued for eight — and TryAcquire(1) returns false. The service sheds while the resource sits three-quarters idle, and Weighted exposes no waiter count that would let anyone notice.

18.5.2 The Shape

Stated once, so that it is recognisable in a costume this book has not shown you:

our side of the call         |  the other side
─────────────────────────────┼─────────────────────────
"our caller went away"       |
"we refused this ourselves"  |  ──▶  err != nil
"they returned a 503"        |       ──▶ "they failed"
                             |
three distinguishable facts  |  one bit

The predicate belongs HERE, on the left, while the three
facts can still be told apart.  Downstream of the
boundary they are gone and cannot be recovered.

Each member is a classification error rather than a mechanical one. Nothing malfunctioned. A cancelled call was classified as a failure; an abandoned reservation was classified as a request; a severed error was classified as no error. The mechanism did precisely what it was told.

That is why the fix in every one of those chapters is a predicate at the boundary rather than better handling downstream of it. §17.6.5's is the sharpest, and it is worth re-reading for its structure rather than its content: two clauses, in an order that matters, and the second tests ctx rather than err — because the question is “was our caller already gone?” and not “does this error look like a cancellation?”.

It is also a family that will be larger in your codebase next year than it is today, and the reason is structural. Every member lives at a point where a deadline meets a counter. Ten years ago most Go services had neither: no request context, no breaker, no per-tenant limiter, no retry budget. Chapters 13 through 17 are the story of adding them, and each addition creates another place where “this call ended early” has to be classified. §14.1.1's silence needed only goroutines. §15.4.4 needed a deadline. §17.6.5 needed a deadline and a counter that sets policy from it.

18.5.3 Why Monitoring Cannot Find It

The unifying property, and the reason this is a review problem rather than an observability one:

Every member reports success at every layer that has a dashboard.

The truncated 200 OK is a 200 in your metrics and a 200 in the client’s. The breaker that opens against a healthy downstream shows a falling error rate at the dependency — because it stopped sending — and rising refusals at a layer doing exactly what it was configured to do. The abandoned reservation shows a limiter enforcing its configured rate; the rate is simply lower than the number in the config, and no metric compares those two.

You cannot alert on it, because the alert would have to know the right classification, which is the thing that was got wrong.

18.5.4 The Rate-Limiting Group

Three of the five members come from one chapter, and that is not a coincidence: a limiter’s whole job is to classify arrivals, so a limiter is a machine for misfiling things.

Read the three together and Chapter 17's pattern becomes visible. Every one is a boolean or an error that means one thing on the limiter’s side and another on the caller’s. false means “not now” and is read as “not possible”. A reservation means “held” and is read as “spent”. An error means “this attempt stopped” and is read as “they are broken”.

The consequence differs by which direction the misreading runs. §17.6.5's turns a healthy dependency into an outage you will investigate. §17.4.3's sheds load while the resource sits idle, and nothing on a dashboard disagrees because Weighted exposes no waiter count. §17.3.4's is the quietest of the three: the service enforces a rate below the one in its config, most severely on exactly the paths where callers abandon most often, and no metric compares the two numbers.

18.5.5 Finding One in Review

The question is short enough to apply while reading. Here is what it looks like on a line you would otherwise skim.

// Illustrative snippet — not a complete program
func (c *Client) call(
    ctx context.Context, req *Request,
) (*Resp, error) {
    resp, err := c.downstream.Do(req.WithContext(ctx))
    if err != nil {
        c.breaker.Record(err)
        c.metrics.DownstreamErrors.Inc()
        return nil, err
    }
    return resp, nil
}

Nothing here is wrong on its face. The error is recorded, counted and returned. A reviewer reading for the feature sees error handling and moves on.

Apply the question — does this distinguish “our caller gave up” from “the dependency failed”? — and two lines are implicated rather than one. Record(err) is §17.6.5 exactly. And DownstreamErrors.Inc() is the same bug in the metric, which is worse in a specific way: the breaker’s version causes an outage you will investigate, and the metric’s version causes a dashboard that lies during the investigation.

The reason to name both lines is that fixing only the breaker leaves the dashboard wrong, and a wrong dashboard is how the next incident gets misdiagnosed. This family propagates: one misclassification at the source shows up in every derived number.

The costume changes and the question does not. A retry policy that retries on any error will retry your own rejections. A cache that caches any result will cache a cancellation. A health check that counts any failure will report an outage caused by its own timeout. Each is this family, and each is found by asking one question at one boundary.

18.5.6 Making the Classification Visible

Review is the primary defence. There are two cheap second lines, and they attack the same thing from different sides: the classification is what to record, and the classification is what to test.

Pull the predicate out of the call site and it stops being an if buried in a method. It becomes a pure function of an outcome, and a pure function can have a test.

// Illustrative snippet — not a complete program
// Now it is a function, and a function can have a table test.
func classify(ctx context.Context, err error) Outcome {
    switch {
    case err == nil:
        return Success
    case ctx.Err() != nil:
        return CallerGaveUp    // not the downstream's fault
    case errors.Is(err, ErrOverloaded):
        return Shed
    default:
        return DownstreamFailure
    }
}

// Illustrative snippet — not a complete program
tests := []struct {
    name string
    ctx  context.Context
    err  error
    want Outcome
}{
    {"caller cancelled", cancelled, context.Canceled,
        CallerGaveUp},
    {"deadline while calling", expired,
        context.DeadlineExceeded, CallerGaveUp},
    {"downstream refused", live, ErrOverloaded, Shed},
}

No goroutines, no timing, no flakes, and it runs in microseconds. What makes it worth stating separately is the order of the two artefacts: extracting the predicate is what makes the test possible, and it is also what makes the missing row visible in review. A table has rows, rows can be missing, and a missing row is something a second reader can notice without reconstructing the author’s reasoning. That is the property the if lacks — to review a condition you have to independently derive the set of outcomes it should cover, which is the same work as writing it.

Then record the decision, not just the outcome:

// Illustrative snippet — not a complete program
func (c *Client) classify(ctx context.Context, err error) error {
    if ctx.Err() != nil {
        c.metrics.CallerAbandoned.Inc()  // our caller left
        return ctx.Err()
    }
    if err != nil {
        c.metrics.DownstreamErrors.Inc() // the dependency failed
        c.breaker.Record(err)
    }
    return err
}

Two counters instead of one, and the second is now true to its name. The moment CallerAbandoned is a number on a dashboard, §17.6.5's outage becomes a five-second diagnosis rather than an afternoon: the breaker opened, and right beside it is a graph showing that every “failure” was somebody hanging up. The same is true in the log — err=context.Canceled classified=downstream_failure turns a two-day mystery into a search, where recording only err=context.Canceled leaves the decision, which is the actual bug, invisible.

The general rule: when code makes a classification, the classification is the thing to export. Exporting only the outcome throws away exactly the fact you will need, and the family’s defining property is that the outcome looks fine.

This does not replace the review question. It shortens the incident when the question was not asked, which is the realistic case.

18.5.7 Common Mistakes

Summary: The Misclassification Family

Five bugs across four chapters are one bug: a value crosses a boundary, loses the context that gave it meaning, and is read by the other side as something it never was. A severed error chain reads as no error. A truncated body ships behind a 200. An abandoned reservation reads as a request that happened — measured at ten tokens of debt and 110 ms against 10 ms. A queued waiter reads as an overloaded service, with six of eight units free. A cancelled caller reads as a failed dependency, measured at 200 of 200 requests shed against a downstream with a zero error rate.

The consistent tell is that every member reports success at every layer that has a dashboard, which is why monitoring is structurally unable to find them and a reviewer is not. The fix always belongs on the caller’s side of the boundary, and it should be a table of cases rather than a condition — a table has rows, and a missing row is something a second reader can see.

Two second lines of defence cost almost nothing. Extract the predicate so it can have a table test that needs no concurrency at all; and export the classification alongside the outcome, so the decision that is the bug stops being invisible.

Self-Check Questions: The Misclassification Family

18.6 Bugs That Live Between Chapters

Everything so far has pointed at a chapter. This section cannot, and that is its reason for existing.

Each of the seven bugs below needs two mechanisms at once, from two different chapters. A chapter about sync.Once cannot warn you about contexts, because it has not taught contexts yet; a chapter about contexts cannot warn you about sync.Once, because that was six chapters ago and it is not the subject. So these live in the gaps. They are the only material in this chapter that is new rather than indexed, each one is measured here, and each is a bug I have watched a competent engineer write.

One qualification, because six of the seven are honest and the seventh needs a footnote. §18.6.7's compound update is catalogued — §12.2.10 has the row. It is here because the entry is not about the rule, which Chapter 12 states perfectly well, but about why this instance gets past a reader who knows the rule. The other six appear nowhere in the book.

18.6.1 The Captured Context

sync.Once runs its function once. A context.Context belongs to one caller. Put them together and the first caller’s deadline becomes everybody’s, permanently.

// Illustrative snippet — not a complete program
// ✗ BROKEN: the closure captures whichever ctx arrived first
func (c *Client) Conn(ctx context.Context) (string, error) {
    c.once.Do(func() {
        select {
        case <-time.After(50 * time.Millisecond): // the dial
            c.v = "connected"
        case <-ctx.Done():
            c.err = ctx.Err()   // the FIRST caller's ctx
        }
    })
    return c.v, c.err
}

Both halves are textbook. §12.1.4 taught that Once guarantees exactly one execution — including exactly one failure. §13.3 taught that a context flows down from the caller who owns it. Neither taught that the second must not be captured by the first.

The second line is the bug. One impatient caller poisoned the client for the life of the process, and every later caller — with unlimited time available — receives a deadline error belonging to a request that finished long ago.

The “after every deploy” version is the one that reaches production: a cold process makes the first dial slower than usual, so the first caller is the one most likely to time out. The bug is scheduled by your deployment.

The review question: whose lifetime is this, and how long does its effect last? A ctx crossing into a sync.Once, a cache fill, a lazily-built singleton or any memoised value is the shape. Two fixes answer different questions. If initialisation must be retryable, Once is the wrong primitive and §12.1.7 says so — use a mutex and a state field, or singleflight, which forgets. If it genuinely is once per process, give it a lifetime context owned by the object and let callers time out of waiting for it rather than of performing it.

18.6.2 The Reciprocal Wait

errgroup.SetLimit bounds how many goroutines a group may run (§14.4.4). A semaphore bounds how many holders a resource may have (§17.4). Each is a queue, each is correct alone, and a goroutine that holds one while waiting for the other is Chapter 10's hold-and-wait with a modern face.

// Illustrative snippet — not a complete program
// ✗ BROKEN: errgroup limit 2, semaphore capacity 2
func fanOutUnderTwoBounds(ctx context.Context) error {
    g := new(errgroup.Group)
    g.SetLimit(2)
    sem := semaphore.NewWeighted(2)

    for i := 0; i < 2; i++ {
        g.Go(func() error {
            if err := sem.Acquire(ctx, 1); err != nil {
                return err
            }
            defer sem.Release(1)
            g.Go(func() error { return nil }) // no slot left
            return nil
        })
    }
    return g.Wait()
}

The defect is that g.Go blocks when the limit is reached — which the documentation says plainly and which nobody remembers, because in every example the group is filled from outside. Both parents hold a permit; both need a slot in a group whose two slots they occupy.

Read the last column downward, because it is the whole argument about load. The cycle needs every slot held at once, and the more slots there are the less likely it is that every parent reaches its nested g.Go before any of them returns. So raising the limit does not remove the cycle — it converts a bug that always fires into one that fires sometimes, which is the profile of a bug that passes review, passes CI, and waits for the day your traffic supplies the overlap for free.

At SetLimit(64) you need sixty-four concurrent parents, which is precisely the load your tests do not produce and production does.

It is also invisible in a goroutine dump until you know the shape. Every parked goroutine is in errgroup.(*Group).Go, which reads as “waiting for a slot” and looks like healthy backpressure. §10.4 would call it a partial deadlock: the runtime detector stays quiet because the rest of the process is fine.

The review question: does anything inside this bound acquire another bound? The fix is Chapter 10's, unchanged — do not acquire a second bounded resource while holding the first. Spawn children before taking the permit, or give the children their own group, which is also the clearer design because the two limits are different concerns and deserve different numbers.

18.6.3 The Uninterruptible Wait

Chapter 13 propagates deadlines through a call tree. §9.2 protects shared state with a mutex. Deadline propagation stops dead at the mutex, and nothing warns you.

// Illustrative snippet — not a complete program
// ✗ BROKEN: the reader's deadline cannot interrupt the lock
func (s *store) slowWrite() {
    s.mu.Lock()
    defer s.mu.Unlock()
    slowDependencyCall() // 500ms, under the lock
}

func (s *store) read(ctx context.Context) (int, error) {
    s.mu.Lock() // sync.Mutex.Lock takes no context
    defer s.mu.Unlock()
    return s.v, nil
}

read accepts a ctx, and its signature therefore promises that a caller can give up. It cannot. sync.Mutex.Lock has no context parameter, no timeout and no failure mode; it returns when it returns.

The reader waited nearly ten times its deadline and then reported success. Its own timeout never fired, because there was nothing to fire against: the goroutine was parked in the runtime, not selecting on anything.

The deceptive part is that cancellation appears to work. The writer’s own call is cancellable, so §13's contract is satisfied — for that caller. The queue behind it is not, and the queue is where your p99 is.

The review question: can anything under this lock block, and can the blocked thing take a context? Three fixes, in descending order of preference. Do not hold the lock across a slow call — take what you need, unlock, call, re-lock to store; this is right almost always, and it is the exercise’s fifth gate. Use a semaphore of one if you genuinely need a cancellable lock, because semaphore.Acquire does take a context. Bound the critical section so that the wait is short enough for the deadline to be a formality.

This one also survives review for a reason worth naming: the lock and the slow call are usually written months apart. read starts as a map lookup under a mutex, which is correct and obviously so. The fetch arrives later, inside the existing if, because that is where a cache miss is handled — and the diff that adds it is three lines and does not mention locking. That is §18.1.5's mechanism at the level of a codebase rather than a review: the bug is introduced by a change about something else, into code that was correct when it was written.

18.6.4 The Discarded Queue

Chapter 15 shuts a process down gracefully. Chapter 17 puts a limiter in front of it. Between them is a question neither asks: what happens to the callers parked in the limiter when shutdown begins?

A drain that took no time at all, reported as clean, having discarded twenty requests the service had already accepted. Both components did exactly what they were told: Acquire(ctx, 1) returns an error when ctx is done, which is its contract and §17.3 relies on it; shutdown cancels the tree, which is its contract and §15.5 relies on it. The failure is in the composition, and it appears at exactly the moment a deploy happens — which is also the moment nobody is looking at per-request outcomes.

§15.5.3 drew the distinction this depends on: in-flight work versus queued work. A limiter’s parked waiters are the second category and they are invisible, because §17.4.3 measured that Weighted exposes no waiter count and rate.Limiter exposes no queue depth.

The review question: what is queued behind this limiter when shutdown starts, and does anything drain it? The fix is a decision rather than a line of code: are these callers accepted, or merely arrived? If accepted, shutdown must drain them, which means the limiter’s context cannot be the one cancellation reaches first. If merely arrived, refusing them is correct — and then the shutdown report should say twenty were refused rather than reporting a clean drain in zero seconds.

18.6.5 The Happy-Path Buffer

Chapter 3 teaches channel capacity. Chapter 14 teaches returning on the first result or the first error. Together they produce a leak that Chapter 16 lists in a Common Mistakes row and no chapter measures.

// Illustrative snippet — not a complete program
// ✗ BROKEN: the buffer is sized for the answer, not the workers
results := make(chan int, 1) // "one result is all we need"
for _, r := range replicas {
    go func(r Replica) {
        v, err := r()
        if err != nil {
            return
        }
        results <- v
    }(r)
}
select {
case v := <-results:
    return v, nil
case <-ctx.Done():
    return 0, ctx.Err()
}

The capacity was chosen by asking how many results do I need? The right question is how many goroutines did I start? The first sender fills the buffer, the function returns, and nobody receives again.

Six goroutines blocked forever, in a function that returned successfully and reported no error. go vet, staticcheck and go test -race all report nothing on this code, which is why it is the third bug in this chapter’s exercise.

The general form has nothing to do with the happy path being a select. Any collector with an early return needs a buffer, or the senders it abandons become a leak. §14.3's result-struct pattern and §7.2's fan-in are where it shows up in production code, and §16.2.8 states the same rule for tests, where t.Fatalf is what makes the receiver leave early.

The reason it is easy to miss is that the unbuffered version is correct until somebody adds the early return — which is §18.4.2's pattern in the lifetime category: the line that becomes wrong is not the line that changed.

The review question: if this receive loop returns early, who is still sending? The fix is one expression, make(chan int, len(replicas)), and every send then completes whether anyone is listening or not.

18.6.6 The Cleanup Context

Chapter 13 gives a context to everything downstream. Chapter 14 gives you a group that makes one for you. The interaction is documented, universally forgotten, and it fires on the success path.

// Illustrative snippet — not a complete program
// ✗ BROKEN: ctx is already cancelled by the time cleanup runs
func run(parent context.Context) error {
    g, ctx := errgroup.WithContext(parent)
    g.Go(func() error { return work(ctx) })

    err := g.Wait()
    // Wait has returned. So has ctx.
    if cerr := cleanup(ctx); cerr != nil {
        return errors.Join(err, cerr)
    }
    return err
}

errgroup.WithContext returns a context it cancels when Wait returns — not when a goroutine fails, but whenever the group finishes, however it finishes. The documentation says so in one clause. Every use of that context after g.Wait() is a use of a cancelled context.

Read the first row. Nothing failed. Every goroutine returned nil, Wait returned nil, and the cleanup that runs afterwards was handed a dead context. Whatever it was going to do — release a lease, delete a temporary object, deregister from discovery — did not happen, and the function returned success.

This is §15.5.6's context.WithoutCancel arriving one chapter early and in a place nobody looks for it. The fix is to be explicit about which lifetime the cleanup belongs to: pass parent, or context.WithoutCancel(parent) if the parent may itself be cancelled, and never the group’s context. The group’s context is for the group’s work, and its lifetime ends where Wait does.

18.6.7 The Compound Update Through a Safe Type

§12.2 teaches sync.Map as a map safe for concurrent use. §11.3 teaches that atomic operations do not compose. Put them together and the type’s name does the damage.

// Illustrative snippet — not a complete program
// ✗ BROKEN: two atomic operations, one invariant
func (c *Counters) Inc(k string) {
    v, _ := c.m.LoadOrStore(k, 0)
    c.m.Store(k, v.(int)+1)
}

Every operation on c.m is safe. LoadOrStore is atomic, Store is atomic, and there is no data race — -race agrees, correctly. The count is still wrong, because the read and the write are two operations and the world moves in between.

Not occasionally — near enough to every round, in every run, on a plain build and on a race build alike. A counter incremented fifty times finished at 2.

This is §18.4.4's check-then-act, and it earns its own entry because of why it gets past review. The type is called sync.Map, the package is called sync, and both operations are documented as atomic. Every local signal says this code is safe. The unsafe thing is the pair, and nothing about the pair is visible in either line.

The review question: is this one operation, or two operations and an assumption? The same question catches Load then Store on an atomic.Int64 where Add was meant.

This is also the one entry in this section that a chapter does own: §12.2.10 already carries the row, “Load-then-Store to increment — check-then-act race — LoadOrStore an atomic”. It is here anyway, and it is here for a different reason. Chapter 12 tells you the rule. What no chapter can tell you, because it needs Chapter 11's non-composition alongside Chapter 12's type, is why this particular instance survives review when the equivalent bug on a plain map would not: every local signal — the package name, the type name, the documented atomicity of both calls — says the code is safe, and all of them are true.

18.6.8 What These Seven Have in Common

Each is the product of two correct decisions.

one mechanism    ×  another          the bug that needs both
─────────────────────────────────────────────────────────
ch12 sync.Once   ×  ch13 context      the captured context
ch14 SetLimit    ×  ch17 semaphore    the reciprocal wait
ch9  mutex       ×  ch13 deadline     the uninterruptible
ch15 shutdown    ×  ch17 limiter      the discarded queue
ch3  capacity    ×  ch14 early exit   the happy-path buffer
ch14 errgroup    ×  ch13 context      the cleanup context
ch12 sync.Map    ×  ch11 composition  the compound update

Every chapter is right about its own half.
The pairing is the defect, and nothing owns a pairing.

Every §18.4 review question passes on §18.6.2 individually. The channel protocol is fine; there is no channel. The lifetimes are fine; Wait joins everything. The invariant is fine. It is a resource cycle, and you only see it by noticing that two bounded things are held at once.

That is also the practical signal: look where two subsystems that were designed separately now touch. A limiter added in front of an existing server. A cache wrapped around an existing client. A group limit introduced into an existing pipeline. A retry wrapped around a call. The bug is rarely in either component; it is in the assumption each makes about the other’s blocking behaviour.

18.6.9 Finding Your Own

Seven is not the list. It is seven instances of a search you can run yourself, and the search states in a sentence: look for a value from one chapter stored inside a value from another.

Every entry above is that shape. A ctx inside a Once. A permit held across a g.Go. A limiter’s waiters governed by shutdown’s context. A dependency call inside a critical section. A worker’s send governed by a receiver’s early return. A group’s context used after the group. A read and a write through a type whose name promises safety.

That gives three places to look in an unfamiliar codebase, and they are quick.

Every field whose type comes from a different chapter than the struct’s purpose. A context.Context in a struct is the loudest — §18.6.1 and §18.6.6 are both this. A sync.Once next to anything request-scoped. A *rate.Limiter in a type that also has a Shutdown method.

Every place two bounded things are held at once. SetLimit plus a semaphore, a semaphore plus a pool, a pool plus a mutex. §10.5.3's ordering rule applies and is almost never applied, because only one of the four has Lock in its name.

Every defer or cleanup that runs after a lifetime has ended. After Wait, after cancel(), after a handler returns. The value is still in scope, and that is what makes it invisible.

There is one more signal worth trusting. If a bug required you to read two chapters of this book to understand, it is a seam bug — and the fix almost never belongs in either component. It belongs at the boundary, usually as an explicit statement of the assumption that was implicit. §18.6.4's fix is not in the limiter or in the drain; it is the sentence “parked waiters are not in-flight work”, turned into two separate populations.

18.6.10 Common Mistakes

Summary: Bugs That Live Between Chapters

Seven bugs, each the product of two correct decisions from two different chapters, so that no chapter could have owned any of them.

Measured in each case. A sync.Once capturing a 10 ms context poisoned 100 of 100 later callers who had no deadline at all. A bound acquired inside another bound deadlocked 20 of 20 times at a bound of 2 and 7 to 11 of 20 at a bound of 16 — raising the limit turned an always-bug into a sometimes-bug and removed nothing. A 50 ms deadline waited 480 ms on a mutex and returned err=<nil>, because sync.Mutex.Lock takes no context. A shutdown that cancelled a limiter’s context refused 20 of 20 queued callers and reported a clean drain in 0 s. A result channel sized for the answer left 6 of 8 goroutines parked in a function that returned success, where sizing it for the workers left none. An errgroup's own context, cancelled by Wait, was handed to a cleanup that quietly did nothing — on the path where nothing failed. And a read-modify-write through sync.Map was wrong in 98–100 of 100 rounds with every tool silent.

Each is correct in both halves, so the review questions are all about the composition: what does this Once capture, what else does this bounded region acquire, what can block under this lock, what is queued behind this limiter at shutdown, how many sends can this channel take, whose lifetime is this context, and is this one operation or two.

Self-Check Questions: Bugs That Live Between Chapters

18.7 Reviewing Concurrent Code

Half of this chapter’s title is code review, and everything so far has been building one thing: a procedure short enough that you will actually run it on a Tuesday afternoon on somebody else’s pull request.

18.7.1 The Four Questions, Promoted

Chapter 2 opened with four questions, at §2.1, and framed them as something to answer before writing go.

Sixteen chapters later they are the review frame, and the reframing is small: you asked them as an author about code you were about to write, and you ask them as a reviewer about code somebody already wrote. The questions are unchanged. What changed is that Chapter 2 could point three of them at one place each, and now each points at considerably more.

They also map onto §18.4's three categories, which is not a coincidence: §18.4 derived its categories from what the tools cannot see, and Chapter 2 derived its questions from what go does not record. Both arrived at the same facts. How does it exit is lifetime. How does it communicate is protocol. What data does it access is invariant. How are errors handled is protocol plus §18.5.

Notice that the questions did not change; only the answers got longer. That is §18.4.6's argument arriving as a practice — a question generalises and a list does not.

18.7.2 Read the Lifetime, Not the Diff

This is the single highest-yield change to how most people review concurrent code.

A diff shows you changed lines. Every bug in §18.4 and §18.6 is a relationship between a changed line and an unchanged one — a go statement here and a return twelve lines down, a close in this file and a send in another, a Once in a constructor and a ctx in a request handler, a newly added sender and a close that was correct all along. Reviewing the diff shows you one end of each relationship.

So do not read it in order. Read it in this one.

1  every  go / wg.Go / g.Go / AfterFunc  the change
   |      touches, and every one in the functions
   |      it calls          ── §2.1's four questions
   v
2  every  return  between there and the end of the
   |      enclosing function  ── what is still running?
   v
3  every  make(chan ...)  and every close
   |      ── who owns it, who else sends, why that
   |         capacity
   v
4  every  Lock  ── what is called while it is held
   v
5  every place an outcome becomes a status
   |      ── what else could produce this outcome
   v
6  now read the diff

Steps 1 to 5 are five greps and are bounded.
Step 6 is not, which is why it goes last.

Each of the first five is a grep. On most diffs all five come back empty and the concurrency review is over in twenty seconds — which is the point. The procedure has to be cheap enough to run on diffs that turn out not to need it, or you will only run it on the ones that look frightening, and the bugs in this chapter do not look frightening.

On the change where it takes twenty minutes, that is the change you wanted to spend twenty minutes on.

18.7.3 The Question at Each Site

§18.4 produced three questions and §18.7.1 has four more. In practice they collapse to a shorter set, because the reviewer’s job is to classify first and ask second.

At a go, wg.Go or g.Go, in order:

1. Who waits for this? If nothing does, is that deliberate and documented, or is it a leak? §2.5's fire-and-forget is a legitimate answer; “nobody, and nobody thought about it” is not.
2. If this function returns right now, what is still running? Ask again at every return and every t.Fatal between here and the end.
3. What does it send, and is there room? Size by goroutines started, not by results wanted (§18.6.5).
4. Whose context is this, and where does it get stored? A ctx that outlives its call is §18.6.1.
5. Is this group already being used by something that is itself in the group? (§18.6.2.)

At a make(chan ...) or a close: who owns it, and is the close on the owner’s path? If there is more than one sender, who coordinates the close? If a receiver ranges over it, who closes it? (§3.3, §6.4, §7.3.) And if the diff added a sender: is that close still right?

At a Lock, in order:

1. What is called while this is held? A user callback (§9.3), a channel operation, a dependency call, a second bounded resource — four different bugs, all hold-and-wait.
2. Can the blocked thing take a context? If callers pass a ctx and the wait is a mutex, the signature is promising something it cannot deliver (§18.6.3).
3. Is the invariant one field, or one field at two moments? If either, one lock per access is not enough (§18.4.4).

At a boundary where an outcome becomes a status, one question: what else could produce this outcome? (§18.5.)

18.7.4 A Worked Review

Here is a real diff, of the size you get in a real pull request. The title is “enrich records with display names”, which is accurate and points at the feature.

// Illustrative snippet — not a complete program
// EnrichAll looks up a display name for every record,
// concurrently.
func (e *Enricher) EnrichAll(
    ctx context.Context, recs []Record,
) ([]Record, error) {
    out := make(chan Record)
    errc := make(chan error, 1)

    var wg sync.WaitGroup
    for _, r := range recs {
        wg.Go(func() {
            name, err := e.name(ctx, r.ID)
            if err != nil {
                select {
                case errc <- err:
                default:
                }
                return
            }
            r.Name = name
            out <- r
        })
    }
    go func() { wg.Wait(); close(out) }()

    var res []Record
    for {
        select {
        case err := <-errc:
            return nil, err
        case r, ok := <-out:
            if !ok {
                return res, nil
            }
            res = append(res, r)
        }
    }
}

func (e *Enricher) name(
    ctx context.Context, id string,
) (string, error) {
    e.mu.Lock()
    defer e.mu.Unlock()
    if v, ok := e.cache[id]; ok {
        return v, nil
    }
    v, err := e.fetch(ctx, id)
    if err != nil {
        return "", err
    }
    e.cache[id] = v
    return v, nil
}

It is careful code. The error channel is buffered so a failing goroutine cannot block. The select with a default drops a second error rather than deadlocking. The cache is behind a mutex. wg.Go is the modern idiom. Somebody thought about this.

Now the procedure from §18.7.2, in order.

1. Goroutine creation sites. One wg.Go, one bare go func. Who joins them? The wg.Go calls are joined by wg.Wait inside the closing goroutine. Now the second half: on which paths? The senders reach out <- r on the success path — and the receive loop returns at case err := <-errc. After that return, nobody receives from out again, and out is unbuffered.

That is the finding.

Every non-failing fetch on every call is parked forever on its send.

2. Channel creation sites. out := make(chan Record) is owned by EnrichAll and closed by the wg.Wait goroutine, which is correct. errc is buffered to 1 and never closed, which is fine because nobody ranges it. The ownership is sound; the capacity is the bug, and §18.6.5 is the general form.

3. Context parameters. ctx is passed to e.name and on to e.fetch. It belongs to the caller of EnrichAll and does not outlive it — no sync.Once, no struct field, no cache key. This one is clean.

4. Locks. e.mu.Lock() with defer e.mu.Unlock(), and between them: e.fetch(ctx, id). A network call under the lock.

That is §18.6.3. The first caller’s ctx works — it will return when its deadline expires. Everybody else is queued on e.mu, which reads no context at all.

Worse, this is a cache — its entire purpose is to be on the hot path — so under a cold start every request in the service serialises behind one slow fetch.

5. Outcome to status. Nothing here turns an outcome into a policy decision, so this step is empty.

Then the feature. Does it enrich records correctly? Yes.

Two real bugs in forty lines that both linters passed. Neither is exotic; neither required knowing anything about this codebase. Both came from a question asked at a site, in an order that has nothing to do with how the diff was written.

The comments to leave are short.

Both are questions. Both name the site. Neither says what to do, because the author knows whether that cache is meant to be a single-flight barrier and you do not.

18.7.5 The Same Five Steps on a Four-Line Diff

The worked review above is the case where the procedure obviously earns its keep. Here is the case where it looks like overkill and is not: a one-word change inside a four-line function.

// Illustrative snippet — not a complete program
// The whole diff is one word: `c.fetch(ctx, k)` became
// `go c.fetch(ctx, k)`. Four lines of context, one changed.
func (c *Cache) Warm(ctx context.Context, keys []string) error {
    for _, k := range keys {
        go c.fetch(ctx, k)
    }
    return nil
}

A performance change: warm the cache in parallel instead of serially. Obviously correct, and it is not.

Step 1 finds one go statement, and the Four Questions do not have four answers. How does it exit? When fetch returns, presumably — nothing in view bounds its runtime and nothing waits for it. How does it communicate? It does not: Warm returns nil before any fetch has finished, so the contract changed from “the cache is warm” to “the cache will be warm at some point, or not”. How are errors handled? The serial version discarded fetch's error too — a pre-existing bug this change makes unfixable, because there is no longer a caller to return it to. What data does it access? c, from len(keys) goroutines at once, and whether fetch is safe for concurrent use is said by neither the diff nor the function name.

Step 2 finds one return, immediately, with len(keys) goroutines running at that moment. Steps 3 and 5 are empty. Step 4 finds no lock in the diff — but step 1's last question already sent you to read fetch, which is where a lock will be if there is one. Step 6, the change itself, is by now the least interesting part.

Three comments come out of that.

Each is a missing answer, each is answerable in one line by the author, and the third is the one most likely to be a real bug. Total time: about two minutes, most of it spent reading a function the diff did not touch — which is §18.7.2's whole argument.

And notice what the tools would have said: nothing. There is no data race until fetch has one, no lost cancel, no copied lock, and -race will report only if some test happens to warm two overlapping keys. The change is a lifetime bug and a contract change, and both are invisible by construction.

18.7.6 What to Say, and What to Let Go

Three comments are almost always worth making, and all three are questions rather than assertions — the author knows the intent and you do not.

“Who closes this, and how do you know the senders are done?” — asked of the doc comment, not the code, and asked whenever a close appears in a function that is not obviously the sole sender. If the package’s documentation does not say who owns a channel’s lifecycle, the protocol does not exist and §18.4.2's whole family becomes possible. The answer is a WaitGroup, a sole-sender argument, or a bug, and you do not have to know which — the question makes the author check.

“What happens here if the caller has already gone?” — at an error path, wherever an outcome becomes a status. This is §18.5's question and it finds the whole family in ten seconds.

“Is there a test that fails without this?” — when a concurrency fix arrives without one. §16.7.7 lists the step almost everyone skips; a fix nobody watched fail is a fix taken on faith.

Then stop, because concurrency review has a specific failure mode: the reviewer who has just read a chapter like this one and flags every goroutine in the codebase.

Three findings that are usually not findings, offered because every reviewer who reads a chapter like this one produces them for a while.

“This goroutine has no context.” Plenty should not have one. A goroutine that owns a resource for the life of the process — a metrics flusher, a log shipper, a pool’s reaper — is stopped by closing its input or by the process exiting, and threading a context into it adds a parameter nobody cancels. The question is how it exits, not whether it takes a ctx.

“This channel is unbuffered.” §5.3 spent a section on when that is the right answer, and the rendezvous property is often the point. The finding is only real if you can name a sender that will be abandoned — which is §18.4.3's question, and it has an answer.

“This should use errgroup.” Maybe. errgroup buys first-error semantics and a limit; if the code needs neither, a WaitGroup is smaller, and §15.6.6 argued the same from the other direction. A comment that names a construct invites an argument about constructs; a comment that names a missing answer invites a check.

2 comments, each one naming a missing answer
   │
   └─ both read, both discussed, both fixed

9 comments, 2 of which name a missing answer
   │
   ├─ the author skims them, and finds 7 preferences
   ├─ starts skimming your next review as well
   └─ the 2 real ones are in there somewhere

You spend the same budget either way.  The only thing you
actually choose is what you spend it on.

The corollary for the author’s side is worth stating too: when a reviewer asks “who closes this?”, the answer is not a defence. It is a sentence in a doc comment, or a directional type (§18.4.6), and then nobody has to ask again.

18.7.7 Review the Tests, Not Only the Code

A concurrency change arrives with tests. Reviewing them is a separate pass and a short one, because Chapter 16 supplied every question.

Does the test assert an invariant or a schedule? §16.1.4's distinction, and the single most useful thing to ask. if got != want after a time.Sleep is a schedule assertion: it will pass, and it will keep passing when the code breaks. A sleep between the act and the assert is the comment.

Would it fail without the fix? §16.7.7's step. Asking costs one sentence.

Does it run enough times to mean anything? A rare-branch race condition needs many runs before a single pass means much, and §16.7.5 gives the arithmetic. Either the test is deterministic — a synctest bubble, a barrier — or it needs -count.

Does it assert from the test goroutine? §16.2's rule. A t.Fatal inside a wg.Go is the bug neither linter catches (§18.2.6), and a test file is exactly where it appears.

Four questions, all from Chapter 16, and they take about as long to ask as reading the test does. The reason to do them as a separate pass is that a test file reads like scaffolding, and scaffolding is where attention goes to die.

18.7.8 When the Diff Is Too Big

The five greps assume a diff you can read. Sometimes you get four thousand lines and a deadline, and the honest answer is not to pretend otherwise.

Triage by what changed about concurrency, not by what changed.

The second command is the whole review surface. A four-thousand-line diff that adds no goroutine, no channel, no lock and no context needs no concurrency pass at all, and finding that out takes one command. When it does return lines, those are the ones to read in full, with §18.7.3's question at each.

Two habits make it reliable.

Read the deletions too — which is why the pattern above matches ^[+-] rather than ^\+. A removed defer cancel(), a removed close, a select that lost its ctx.Done() case: deletions do not read as concurrency changes and they are half of this class.

Ask for the diff to be split before you ask for anything else. A change that adds a limiter and refactors the handler is two reviews wearing one hat, and §18.6 is entirely about the bug being at the seam between them. “Can the limiter land on its own first?” is often the highest-value comment available, and it is not a concurrency comment at all.

18.7.9 Reviewing Your Own Code

Most concurrent code is read most often by the person who wrote it, and self-review fails differently from peer review. A reviewer does not know what you meant, so they have to read what you wrote. You know what you meant, so you read what you meant — which is why re-reading your own diff finds typos and never finds a lifetime bug. The intended lifetime is right there in your head, filling the gap the code left.

There is one structural advantage to set against it: you are not limited to the diff. A reviewer sees the changed hunks; you can see the whole file, and §18.7.2's procedure wants the file. A change that adds one case to a select is a two-line diff and a whole-function question, and you are the only reader positioned to ask it.

Four habits work against the handicap, in ascending order of cost.

Run §18.2's step before you send it, not after. go vet ./... and staticcheck ./... take seconds and they are a reader whose attention is free. Every mechanical finding they report is one that would otherwise have consumed a human reviewer’s limited attention on something a machine already knew.

Write the invariant down first. Before the code, in the comment. “At most quota grants across all callers.” Now the sentence exists outside your head and the code can be compared against it. This is the highest-yield habit in the chapter, because it costs nothing and it changes the code rather than the review — one sentence naming two moments in time is visibly not protected by one lock per access, and §18.4.4's bug cannot survive being written down. Most check-then-act bugs are written by somebody who could have stated the invariant correctly and was never asked to.

Answer the Four Questions in writing, in the PR description. Not as ceremony: as a forcing function. “This goroutine exits when the context is cancelled; results come back on results, buffered to len(replicas); errors join into the return; it touches only its own arguments.” Four sentences. If any of them is hard to write, that is the finding, and you found it before anyone else spent time on it.

Come back tomorrow, or make a machine be the stranger. Twenty-four hours is enough to forget what you meant, which is exactly the deficit you need. Where you do not have a day, Chapter 16 is the substitute: a test is a reader with no memory of your intent.

18.7.10 When to Stop, and a Checklist

The half that is always missing, and where a review process actually fails.

Stop when every site has an answer, not when you feel certain. You will not feel certain — §18.4's bugs are invisible by construction, so a reviewer waiting to feel sure waits forever and then approves anyway, which is worse than stopping on a rule. The sites are enumerable, which is what makes the standard reachable: every go the change touches, every return between there and the end, every channel, every lock, every boundary where a value becomes a status. When each has an answer — in the code, in a comment, in a test that would fail if it were wrong, or in the author’s reply — you are done.

Stop escalating a disagreement about a hypothetical. “This could deadlock if two callers arrive together” is a testable claim, and Chapter 16 made all three of the usual claims cheap to settle.

A test settles it and then keeps settling it, which is what makes it worth doing even when you turn out to be wrong: the disagreement was worth a day of two people’s attention exactly once, and the regression guard is worth it every time somebody touches that code afterwards. The corollary is a stopping rule in the other direction — if the claim is not worth a test, it is not worth the thread.

Stop reviewing for what the tools catch. If §18.2's step is in CI, then copied locks, discarded cancels, unbuffered signal channels, Add inside goroutines, deferred Locks and spinning select loops are handled. Reading for them is unpaid work, and worse, it consumes the attention the eleven need.

Unusual is not wrong. Some correct concurrent code looks alarming. A mutex unlocked explicitly rather than deferred, so that a slow call happens outside the critical section, is the fix for §18.6.3 and reads like a violation of the defer rule. This chapter’s exercise has a gate that fails you for “correcting” it, because a reviewer who normalises every unusual shape removes the deliberate ones.

Chapter 10 already ships a code review checklist. It is a good one and it is about deadlocks — lock ordering, nested acquisition, callbacks and I/O under lock. If the change you are reviewing is lock-heavy, use that one; it goes deeper on that mechanism than this chapter can afford to. This is the other altitude: the pass you make before you know what the subject is.

Ten rows, all questions, none of them a rule about how to write Go. The honest claim about them is narrow: they are the questions that would have caught the bugs in this book. They are not a proof of correctness and no checklist is. What they buy is that the categories get visited, which is the thing an unaided reader reliably fails to do — not because the questions are hard, but because nothing prompts them.

18.7.11 Common Mistakes

Summary: Reviewing Concurrent Code

Chapter 2's Four Questions were the review frame all along; sixteen chapters later the only change is that you ask them about somebody else’s code, and the answers got longer while the questions did not.

The procedure is to read the lifetime before the diff, because every bug in §18.4 and §18.6 is a relationship between a changed line and an unchanged one, and the diff shows you one end of each. Five greps — goroutine creation, early returns, channels and closes, locks, outcome-to-status — each with one question attached, and then the feature. On most diffs all five come back empty, which is exactly what makes the pass cheap enough to run every time.

A worked review of a forty-line diff that both linters passed produced two real bugs: measured, senders parked from 1 goroutine to 23–37 after ten calls with one failing key, and a second caller with a 50 ms deadline blocked past 300 ms behind a network call under a cache lock. The same five steps on a four-line diff produced three missing answers in two minutes, most of it spent reading a function the diff never touched.

The discipline that keeps it useful is knowing when to stop. Flag a missing answer rather than a shape. Treat unusual as unusual rather than wrong. Settle a hypothetical with a test rather than a thread. And stop when every site has an answer, because certainty is not available for bugs that are invisible by construction.

Self-Check Questions: Reviewing Concurrent Code