How to Review Error Handling Before It Ships

Error handling is the part of a codebase everyone agrees matters and nobody wants to review. It sits at the bottom of the pull request, a few lines of try/catch wrapped around the interesting work, and the eye slides past it toward the logic that actually does something. That habit is expensive. Error paths are the code that runs on the worst day of a product's life, when the database is down or the payment processor is timing out, and they are the code least likely to have been read by a second person before that day arrives.

A review of error handling is not a review of whether errors exist. It is a review of what happens after they occur, and that is a different question with a different checklist. The goal is to catch the failure modes that survive testing and detonate in production: swallowed exceptions, misleading messages, retries that multiply load, and cleanup that never runs.

Find the catch blocks that do nothing

The empty catch is the oldest sin and still the most common. A block that catches an exception and does nothing, or logs it at debug level, or assigns it to a variable that is never read, is a place where the program has decided to continue as if nothing went wrong. Sometimes that decision is correct. A failed metrics ping should not crash a checkout. But the reviewer's job is to force the author to say which case this is.

A useful question during review: if this catch block fires in production at 3 a.m., what does the on-call engineer see, and what do they do next? If the answer is nothing and nothing, the block needs a comment explaining why silence is the right response, or it needs to be deleted so the exception propagates to somewhere that handles it.

Log-and-rethrow deserves separate scrutiny. Catching an exception, logging it, and throwing it again produces duplicate entries in the log, and the duplicate is often the one that gets paged on. The original context is lost by the time the second log line appears. Either log at the boundary where the error is finally handled, or add context and rethrow without logging.

Check what the error message actually says

Error messages are written for the developer who is debugging at the moment of the throw, and then read by someone else six months later with no context. The gap between those two readers is where most bad messages live. A message like "Operation failed" tells the reader that something did not work, which they already knew from the fact that an error was thrown. It wastes the one chance the code has to explain itself.

Good messages name the operation, the input that caused it, and the condition that was violated. "Failed to charge order 8812: gateway returned decline code 51" is worth ten stack traces. During review, scan every throw statement and every log call in an error path and ask whether the message would let a stranger reproduce the problem. If it would not, the message is a placeholder.

There is a second audience for error text: the end user. Any message that reaches a customer interface needs to be read as a customer would read it. Stack traces, internal hostnames, and database column names leaking into a user-facing error are a small security problem and a large support-ticket problem. The review should trace at least one error all the way from the throw to the screen and confirm the customer sees something honest and useful.

Retries are where error handling becomes load

A retry looks like resilience in a diff. Three attempts with exponential backoff reads as responsible engineering. In aggregate, retries are a load multiplier, and the multiplier lands exactly when the downstream system is already struggling. The classic failure: a service starts timing out, every caller retries three times, and the downstream system now receives triple the traffic it was already failing to handle. The retry logic turns a brownout into an outage.

Reviewers should check three things on any retry loop. First, is the operation idempotent? Retrying a charge or a write that has already partially succeeded can duplicate side effects. Second, is there jitter in the backoff? Without jitter, every client retries on the same schedule and the retries arrive in synchronized waves. Third, is there a cap on total attempts and a circuit breaker somewhere upstream? A retry without a budget is a promise to keep hammering a system that has already said no.

There is also the quiet retry, buried in a client library or a framework default, that the author does not know is there. When a diff adds a new HTTP client or a new database driver, the review should confirm what its default retry behavior is. Defaults are opinions, and some of them are aggressive.

Look for cleanup that only runs on success

Resources acquired before an operation and released after it are the classic shape of a leak. A file handle opened, a lock taken, a temporary row inserted, a connection checked out of a pool: each of these needs a release path that runs whether the operation succeeds or fails. When the release lives in the happy path only, every error becomes a small permanent leak, and enough small leaks become a crash.

Most modern languages make this easy to get right with try/finally, defer, using, or context managers. The review should confirm the pattern is actually used and not merely imitated. A finally block that itself can throw, for example, will mask the original exception. Cleanup code that assumes the operation succeeded will fail inside the error path. These are subtle and they are exactly what a second pair of eyes is for.

Locks deserve a special mention. A lock acquired and not released on the error path can deadlock a process, and deadlocks are the failure mode that turns a single bad request into a frozen service. If a diff touches anything that takes a lock, read the unlock path first, before reading anything else.

Confirm the error is classified, not just caught

Not all errors are the same kind of event. A malformed user request is a client problem that should return a 4xx and never page anyone. A missing configuration value is a deployment problem that should fail loudly at startup. A transient network blip is a retry candidate. A bug in the code is a 5xx and a pager. When all of these flow into a single catch-all that returns 500, the system loses its ability to respond appropriately to each.

During review, look at the catch hierarchy and ask whether the code distinguishes these categories. A common anti-pattern catches a broad base exception type, logs it, and returns a generic failure. That works until the day a validation error and a disk-full error produce identical responses and identical alerts. The reviewer should push for explicit handling of the categories the code actually expects, with a broad catch only at the very top as a last-resort boundary.

Test the failure path, not just the happy one

Error handling that has never been executed is error handling that does not work. The tests in a pull request almost always cover the success case, because the success case is easy to construct. The failure case requires a mock that throws, a fixture that is malformed, or a dependency that is deliberately made unavailable, and that is more work.

A review can ask a simple question: which test in this diff exercises an error path? If the answer is none, the error handling is unverified speculation. A single test that forces the exception and asserts on the resulting message, status code, or cleanup is worth more than a paragraph of design discussion. It also protects the error path from future refactors, which tend to break untested code first.

The three questions worth asking out loud

Reviewing error handling does not require a formal process or a long checklist pasted into the pull request template. It requires reading the error paths with the same attention as the success paths and asking, in the review thread, the questions the author has not answered. Three of them catch most of the problems.

What happens to this error after it is caught, and who finds out about it? Is the operation safe to retry, and what stops the retries from compounding? What does the user see, and what does the on-call engineer see?

None of these are trick questions. They are the questions that force the author to think about the code from the perspective of the person who will be woken up by it. The reviewer who asks them is doing the most valuable thing a review can do: making the worst day a little less bad before it happens.