Error Handling Mistakes That Appear in Go Code Reviews

Error handling code passes the compiler and the first test, and then fails in ways that only show up when something goes wrong, which is to say at the worst possible time. That makes it a good subject for code review: the author cannot see these bugs by running the program, and a reviewer who knows the patterns spots them in seconds. This page collects ten that keep appearing in pull requests, each with a small program that shows the symptom, and the comment to leave.

It is a roundup, so each item is short, with a link to the page that covers it properly. It belongs to the error handling guide. Everything ran on Go 1.25.6.

1. A sentinel created inside the function

The review comment: "this error can never match". A sentinel works because every caller sees the same value. Creating it inside the function makes a fresh one per call:

package main

import (
	"errors"
	"fmt"
)

func find() error {
	return errors.New("not found") // a new value every time
}

var ErrNotFound = errors.New("not found")

func findRight() error { return ErrNotFound }

func main() {
	fmt.Println(errors.Is(find(), find()))
	fmt.Println(errors.Is(findRight(), ErrNotFound))
}

false
true

The first prints false, since two calls give two different values with the same text. Declare sentinels at package level and return that variable (sentinel errors).

2. %v where %w was meant

The text is identical and the behavior is not. A reviewer cannot see the difference in logs, only in the code, and the symptom shows up in another package months later when an errors.Is stops working:

package main

import (
	"errors"
	"fmt"
	"io"
)

func main() {
	bad := fmt.Errorf("read header: %v", io.EOF)
	good := fmt.Errorf("read header: %w", io.EOF)
	fmt.Println(bad, "|", errors.Is(bad, io.EOF))
	fmt.Println(good, "|", errors.Is(good, io.EOF))
}

read header: EOF | false
read header: EOF | true

Ask for %w unless the author has a reason to hide the cause, and in that case ask for a comment saying so (wrapping with %w). go vet catches the opposite mistake of passing a non-error to %w, and not this one.

3. Comparing with == after wrapping

A cousin of the previous one. Code that was if err == io.EOF works until a layer adds context:

package main

import (
	"errors"
	"fmt"
	"io"
)

func main() {
	err := fmt.Errorf("parse: %w", io.EOF)
	fmt.Println(err == io.EOF, errors.Is(err, io.EOF))
}

false true

The comment is "use errors.Is", and the fix is mechanical. A linter (errorlint is one) flags the pattern across a codebase (Is and As).

4. Logging and returning

if err != nil {
	log.Printf("query failed: %v", err)
	return err
}

Every caller up the stack that follows the same habit logs it again, so one failure fills the log with three lines at three levels of detail and triples the error counts. The review comment is "pick one": return it with context and let the top of the call chain log it once (log errors once). The exception is code that handles the error and does not return it.

5. Matching on error text

if strings.Contains(err.Error(), "not found") { ... }

This couples the caller to wording nobody promised to keep, and it matches by accident ("user not found", "route not found", "file not found" all pass). Ask for a sentinel, a type, or a behavior method, and use errors.Is or errors.As. If the error comes from a library that offers nothing else, isolate the string match in one helper with a comment and a test that fails when the library changes its wording (error design).

6. A typed nil returned as error

This one slips through review because the code looks reasonable:

package main

import "fmt"

type ValidationError struct{ Field string }

func (e *ValidationError) Error() string { return "invalid " + e.Field }

func validate(name string) error {
	var err *ValidationError
	if name == "" {
		err = &ValidationError{Field: "name"}
	}
	return err // a nil *ValidationError is still a non-nil error
}

func main() {
	fmt.Println(validate("ada") == nil)
}

false

It prints false for a valid name. The function returns an interface that holds a nil pointer, and the caller's if err != nil branch runs on success. The fix is to return the literal nil on the success path, and to declare the variable as error, not the concrete type (the nil interface that is not nil).

7. Defer in a loop

Opening files in a loop with defer f.Close() closes nothing until the function returns, so a thousand-file loop holds a thousand descriptors and may hit the limit:

package main

import "fmt"

func main() {
	for i := range 3 {
		defer fmt.Println("deferred close", i)
		fmt.Println("processing", i)
	}
	fmt.Println("loop finished")
}

processing 0
processing 1
processing 2
loop finished
deferred close 2
deferred close 1
deferred close 0

All three closes run after loop finished, in reverse order. Move the loop body into a function so the defer runs at the end of each iteration, or close explicitly and handle the error (defer in a loop).

8. Swallowing cancellation in a retry loop

A retry loop that treats every error as retryable keeps going after the caller has given up:

package main

import (
	"context"
	"errors"
	"fmt"
	"time"
)

func attempt(ctx context.Context) error {
	select {
	case <-ctx.Done():
		return ctx.Err()
	case <-time.After(5 * time.Millisecond):
		return errors.New("transient")
	}
}

func retryBad(ctx context.Context, n int) (calls int) {
	for i := 0; i < n; i++ {
		calls++
		if attempt(ctx) == nil {
			return
		}
	}
	return
}

func retryGood(ctx context.Context, n int) (calls int) {
	for i := 0; i < n; i++ {
		calls++
		err := attempt(ctx)
		if err == nil || errors.Is(err, context.Canceled) {
			return
		}
	}
	return
}

func main() {
	ctx, cancel := context.WithCancel(context.Background())
	cancel()
	fmt.Println(retryBad(ctx, 5), retryGood(ctx, 5))
}

5 1

With a context that is already cancelled, the bad loop makes five calls and the good one makes one. The comment is "stop on context.Canceled and DeadlineExceeded" (retryable and permanent errors).

9. Internal text in a response

http.Error(w, err.Error(), 500) sends driver messages, hostnames and file paths to whoever made the request. In review, search the diff for err.Error() near anything that writes to a client, and ask for the public message and the request ID instead (user-facing errors).

10. Errors lost in goroutines

go func() {
	if err := work(); err != nil {
		return // nobody hears this
	}
}()

A return err in a goroutine goes nowhere, and a bare return after the check does the same. Ask where the error goes: a channel, an errgroup, a result slot. If the answer is "it doesn't", the failure is silent (errors from goroutines).

11. Wrapping the same context at every layer

Three functions in a row, each adding its own name, produce getUser: fetchUser: loadUser: query: connection refused. The reader learns the call graph and nothing about the intent. The review comment is "say what you were doing, once, where the information is new". A function that adds nothing returns the error as is, and a function that does add something says it in terms of the operation ("load profile for user 42"). The page on adding context has the before and after.

12. Panicking where an error belongs

A helper that panics on a missing file, a bad configuration value or a network failure forces every caller to know about it and to recover or crash. In review, any panic( outside init, Must helpers and genuinely impossible states deserves a question: could a correct program hit this? If yes, it returns an error (panic or error). The corollary is a recover() with no obvious boundary, which usually hides a bug.

13. An error check that returns nil

A small and nasty one: the check is there and does the wrong thing.

package main

import (
	"errors"
	"fmt"
)

func step() error { return errors.New("step failed") }

func run() error {
	if err := step(); err != nil {
		fmt.Println("warning:", err)
		return nil // meant: return err
	}
	return nil
}

func main() {
	fmt.Println("run returned:", run())
}

warning: step failed
run returned: <nil>

The function logs the problem and then reports success, so the caller carries on as if nothing had gone wrong. A review catches this by asking, for every if err != nil block, what the function returns on that path. The nilerr analyzer finds the pattern where a function returns nil inside a block that checks a non-nil error. A related variant is the block that assigns the error to a variable that is never read; staticcheck reports the ineffectual assignment.

What to do with the list

Don't treat the list as a lecture for the author. Turn it into automation wherever you can and into a short checklist where you can't. go vet checks the printf family, errorsas and copied locks, and the documentation for vet lists every analyzer. errcheck finds dropped results. Third-party linters such as errorlint and nilerr, run through golangci-lint, cover the comparison and nil-return patterns. What is left, the judgment calls about where to wrap and where to log, belongs in the checklist, the review template and the team's written conventions. The Go project's own code review comments are a good starting point to copy. The deeper treatments of each topic are in the guide, and the fastest way to internalise the patterns is to find each one in a real diff once.

A reviewer's pass

One habit makes the pass faster: read the failure paths first. Start each function at its if err != nil blocks and ask what each one does, before reading the happy path. Most of the bugs on this page live in those three or four lines, and a reviewer who reads top to bottom tends to follow the story of success and skim the rest.

A fast read through a diff that touches error handling asks five questions. Does every fmt.Errorf that wraps use %w? Is any error compared with == or by its text? Does a function both log and return? Could a nil pointer of a concrete error type reach a return? Does any path send err.Error() to a client? A diff that survives those is in good shape. Run errcheck and go vet in CI so the mechanical ones never reach a human (errcheck), and keep the others on a short checklist in the review template. Many of these have lint rules in golangci-lint (errorlint, nilerr and others), which turn a review habit into a build failure.