Code Review

Review the Error Path First, Because That Is Where It Will Break

Rohit Bhadani

Rohit Bhadani

— 4 min read
Share
Review the Error Path First, Because That Is Where It Will Break. Abstract code review illustration in orange and dark grey on debugly.dev

When I review a pull request I read the error handling before I read the feature. It is an odd order and it occasionally annoys people, but it comes from a simple observation: almost every incident I have been part of was an error path that had never been executed once, by anyone, in any environment.

The happy path has tests. The happy path runs in every deploy. The happy path is exercised by the author, by CI and by users. Error paths run for the first time in production, during the incident, in front of the person with the least context.

So I review them first, while I still have the energy to be pedantic.

The five things I look for

1. Catch blocks that swallow

try {
  await syncInventory(order);
} catch (err) {
  logger.error("sync failed");
}

This is the single most common defect I find, and it is common because it looks responsible. There is a try, there is a catch, there is a log line. It passes review because it passes a glance.

The problems: the error object is discarded, so there is no message and no stack. The log line has no identifier, so you cannot correlate it to an order. And the caller has no idea the sync failed, so it proceeds as though inventory is now correct.

What I want to see instead:

} catch (err) {
  logger.error({ err, orderId: order.id }, "inventory sync failed");
  throw new InventorySyncError(order.id, { cause: err });
}

The error is preserved, the context is attached, and the failure propagates to someone who can decide what to do about it.

2. Errors converted into nothing

The Go version of the same mistake:

value, _ := cache.Get(key)

An ignored error is not the same as a handled error, and the underscore makes it invisible to grep. I will ask for every _ in an error position to be justified in the review, and "the function always succeeds" is not a justification, it is a prediction.

In TypeScript the equivalent is a .catch(() => {}) at the end of a promise chain. Same defect, different punctuation.

3. Wrapping that loses the cause

} catch (SQLException e) {
    throw new ServiceException("Database error");
}

Now the original message, the SQL state and the stack are gone, and the person debugging at two in the morning has four words to work with. Every wrapping exception should carry the original as a cause. In Java that is the second constructor argument, in JavaScript it is { cause: err }, in Go it is fmt.Errorf("...: %w", err).

If your language supports error causes and your codebase is not using them, that is a review standard worth adding today. It is free.

4. Retries without a budget

Every retry needs three answers before it ships. What is the maximum number of attempts. Is there jitter, so a thousand clients do not retry in lockstep. And what happens if the operation is not idempotent and the first attempt actually succeeded.

The third question is the one that gets skipped, and it is how you get duplicate charges. Reviewing retry and backoff logic goes through this in full.

5. Errors that cross a boundary unshaped

An internal ENOTFOUND reaching a user as a 500 with a stack trace is both a bad experience and an information leak. An internal UniqueViolation reaching a user as a 500 when it should be a 409 is a bug your client will work around badly.

At every boundary I want to see an explicit mapping from internal error types to external responses. Not a catch all that returns 500, which is the absence of a mapping wearing a costume.

The question that surfaces most of it

If I only have time for one question on a large diff, it is this:

Walk me through what happens when this fails.

Not "is there error handling". Not "did you add a try block". Ask them to narrate the failure, out loud, in the review. Two things happen consistently. Either they describe it clearly and I learn something about the design, or they stop partway through and realise they have not thought about it.

The second outcome is the valuable one, and it happens more often than you would expect on code that already has comprehensive error handling on the surface. Handling and understanding are different things.

What I do not ask for

I do not ask for exhaustive error handling on paths where failure is genuinely unrecoverable and the correct response is to crash. A startup configuration error should kill the process loudly, not be caught and logged. Defensive try blocks around code that has no plausible failure mode make the codebase worse, because they hide the places where failure actually matters.

I also do not ask for custom exception hierarchies in a small service. Two or three error types is usually enough. The hierarchy is a solution to a coordination problem you probably do not have yet.

The standard worth writing down

If your team agrees on nothing else about errors, agree on this: no error may be discarded without a comment explaining why discarding it is correct.

That one rule catches swallowed exceptions, ignored returns and empty catch blocks without requiring anyone to memorise a checklist. It shifts the burden onto the person writing the code, who has the context, instead of the person reviewing it, who has to reconstruct it.

Most of the debugging work I write about on this site starts with an error that was technically handled. The handling was the problem. See error messages are a user interface for the other half of this, which is what to do once the error has survived long enough to be read.

Read more