code Review/error handling/Swallowed Exceptions and Silent Failures: Reviewing Error Handling That Hides Defects

error handling

Swallowed Exceptions and Silent Failures: Reviewing Error Handling That Hides Defects

Empty catch blocks, discarded error values, over-broad handlers, and lost stack traces — how to review error handling so failures stay visible instead of becoming data corruption.

Reviewer Detection Checklist

0 of 12 verified (0%)

Defect Patterns & Fixes

Blanket except that discards every malformed record

python

Scenario: A log ingestion worker parses lines in a batch and accumulates status-code statistics.

def parse_line_batch(lines: list[str], stats: dict[str, int]) -> None:
    for line in lines:
        try:
            parts = line.strip().split(" ")
            status_code = parts[3]
            stats[status_code] = stats.get(status_code, 0) + 1
        except Exception:
            pass

Why this is defective: The blanket except covers three unrelated failures with one response: a malformed line (expected, skippable), an IndexError from a format change (a real defect that should be loud), and a KeyboardInterrupt or MemoryError (which must never be swallowed). Because nothing is counted or logged, a format change upstream silently reduces every statistic to zero while the job continues to report success. The corrected version validates the expected condition explicitly, keeps genuine defects propagating, and returns the skipped count so the caller can alert on it.

How to Spot It in Reviews:

•except Exception or a bare except with pass or continue as the whole body.
•Ask what the three most likely exceptions here are — if they need different responses, one handler is wrong.
•A parsing or import loop with no count of failures is reporting success it has not verified.
•In Python, a bare except: also catches SystemExit and KeyboardInterrupt, so the process stops responding to signals.

Discarded error value proceeding on zero values

Scenario: A service decodes a webhook body before dispatching on its event type.

func (h *Handler) Handle(body []byte) {
    var event PaymentEvent
    _ = json.Unmarshal(body, &event)      // error discarded

    if event.Type == "payment.succeeded" {
        h.credit(event.CustomerID, event.Amount)
    }
}

Why this is defective: When decoding fails, event keeps its zero values: empty customer id, zero amount, empty type. The handler then proceeds against that empty struct as though it were real data. In this example the zero Type happens to skip the branch, which is luck rather than design — with a different field order or a partially valid payload it credits customer "" with 0, and returns success. Wrapping with %w preserves the cause so the caller can inspect it, and returning an error lets the transport decide the status code.

How to Spot It in Reviews:

•Grep for _ = and for calls whose only return value is an error being used as a statement.
•Ask what the struct or variable contains when the call fails — zero values are silently plausible.
•A function with side effects that returns nothing has no way to report failure; that signature is itself the smell.
•Check that wrapping uses %w and not %v, or errors.Is/errors.As upstream stops working.

Catch-all that conflates 'not found' with 'unreachable'

java

Scenario: A profile service loads a user's preferences from a downstream API.

public Preferences load(String userId) {
    try {
        return preferencesClient.fetch(userId);
    } catch (Exception e) {
        log.warn("Could not load preferences");
        return Preferences.defaults();
    }
}

Why this is defective: Returning defaults on any exception means a downstream outage is indistinguishable from a user who has never set preferences. Every user silently reverts to default settings — notifications re-enabled, currency reset, timezone wrong — and nothing in the metrics moves, because the method returns successfully every time. The log line makes it worse by omitting both the user id and the exception, so even a suspicious engineer cannot confirm what happened. Only the 'not found' case has a meaningful default; everything else is an outage and must say so.

How to Spot It in Reviews:

•A catch that returns a default, empty collection, or null — ask whether the caller can tell that apart from a real result.
•catch (Exception e) where the body ignores e entirely: the cause is being thrown away.
•Log statements with no interpolated identifiers cannot be correlated to a request during an incident.
•Fallback-to-default is a resilience decision that belongs where the product impact is understood, not in a data-access helper.

Resource leaked when the operation throws

csharp

Scenario: A report exporter streams rows to a temporary file and uploads it.

public async Task ExportAsync(Query query)
{
    var stream = File.Create(_tempPath);
    var writer = new StreamWriter(stream);

    foreach (var row in await _db.QueryAsync(query))
        await writer.WriteLineAsync(Format(row));   // throws on a bad row

    await writer.FlushAsync();
    writer.Dispose();
    stream.Dispose();
    await _uploader.UploadAsync(_tempPath);
}

Why this is defective: Cleanup placed on the happy path runs only when nothing fails. One badly formatted row leaks a file handle and leaves a partial temp file behind. Neither is visible immediately: the handle count climbs over days until the process hits its limit and every file operation starts failing at once, and a restart 'fixes' it, which is what keeps the real cause hidden for months. The same shape appears with database connections, locks, and semaphores; using, defer, with, and try-with-resources exist precisely so the cleanup cannot be skipped.

How to Spot It in Reviews:

•Any acquire (File.Create, OpenConnection, Lock, Acquire) whose matching release is a plain statement rather than using/defer/with/finally.
•Cleanup as the last lines of a method is a leak waiting for the first exception above it.
•Symptoms to correlate: 'fixed by a restart', slow degradation over days, 'too many open files'.
•Check that the partial artefact is cleaned up too — a leaked temp file is smaller but still real.

Why a swallowed exception is worse than a crash

A crash is a defect that reports itself. It has a stack trace, a timestamp, a line number, and an alert. Someone gets paged, someone fixes it, and the mean time to resolution is measured in hours.

A swallowed exception is a defect that has been deliberately hidden. The system keeps serving traffic while producing wrong answers, and the evidence that would have led you to the cause was caught and discarded at the exact place it was generated. Mean time to resolution is measured in weeks, and it usually starts with a customer, not a monitor.

This is why except Exception: pass deserves a blocking review comment even though it changes no behaviour on the happy path. It is not a style issue. It is a decision to run the system blind.

What it costs in production

Silent partial success. A batch of 10,000 records reports success; 400 of them threw and were skipped. The reconciliation happens a quarter later.
Corrupted state that looks healthy. An operation fails midway, the exception is swallowed, and the caller commits a transaction that should have rolled back.
Undebuggable incidents. The log line says Error processing request with no exception, no identifier, and no context, so the on-call engineer has nothing to search for.
Retry storms. A permanent failure (bad input, 403, schema mismatch) is treated as transient and retried forever, turning one broken message into sustained load against a dependency.
Leaked resources. A connection, file handle, or lock acquired before the failing line is never released because the cleanup was on the happy path. The service degrades slowly over days and recovers on restart, which makes the restart look like the fix.
Masked security failures. A signature verification that throws and is caught by a broad handler ends up returning the default — and the default is usually "allow".

How to spot it in review

Look for four shapes.

1. The empty or near-empty handler. except Exception: pass, catch (Exception) { }, catch { return null; }, if err != nil { }, .catch(() => {}). The variants that only log are barely better if the log has no identifiers and the code then continues as if nothing happened.

2. The ignored error value. Languages with returned errors make ignoring them easy and invisible: _ = json.Unmarshal(...) in Go, an unchecked Result in Rust, a bool-returning function whose result is never read, a promise with no catch, a Task that is never awaited.

3. The over-broad catch. catch (Exception e) around thirty lines catches the NullPointerException from your own bug alongside the IOException you meant to handle, and treats them identically. Ask what specific failures the author intended to handle and whether the block is scoped to just those.

4. The lost cause. throw new ServiceException("failed") without passing the original exception, or raise ValueError("bad input") from None. The type changed, the message got vaguer, and the stack trace now starts at the rethrow.

Then ask three questions about every handler that remains:

Who finds out? If the answer is "nobody", the handler is hiding a defect.
What does the caller receive? A default, an empty list, and a null are all claims about reality. Returning an empty list on a network failure tells the caller "there are no records", which is a lie.
Is this failure transient or permanent? Retrying a 400 is pointless; not retrying a 503 is wasteful. If the handler cannot tell them apart, the retry policy is guesswork.

Warning

Watch for handlers that catch, log, and then continue into code that assumes the failed operation succeeded. That is the shape that turns one failure into corrupted data.

Fixing it without breaking something else

Tightening error handling is a behaviour change, and it is the change most likely to turn a quiet system into a noisy one overnight.

Removing a swallow reveals failures that were always happening. Expect a spike in errors and alerts on deploy. That spike is information, not regression — but it needs to be anticipated, or the fix gets rolled back at 2am by someone who thinks it caused the errors.
Converting a swallow into a throw changes the caller's contract. Everything upstream must be checked: does the HTTP layer turn it into a 500? Does the queue consumer now nack and redeliver forever? Does the batch job abort at record 400 instead of finishing?
Adding retries adds load and requires idempotency. A retry on a non-idempotent operation is a duplicate charge. Retries also need a bound, a backoff, and jitter — an unbounded retry loop is a self-inflicted denial of service against your own dependency.
Narrowing a catch can let a previously-handled exception escape. If catch (Exception) is narrowed to catch (IOException), find out what else was actually being caught in production before you narrow it.
Logging the exception may log sensitive data. Request bodies, tokens, and PII travel inside exception messages more often than anyone expects.

How not to over-engineer the fix

Error handling is where speculative complexity accumulates fastest, because every layer feels entitled to its own opinion.

The proportionate fix is usually one of these three, in order of preference:

Let it propagate. Most functions cannot do anything useful about a failure. Adding a try block just to rethrow is noise. The best error handling is often none at that layer.
Handle it where you can actually decide. One layer — usually the request handler, the job runner, or the message consumer — knows the retry policy, the user-facing message, and the transaction boundary. Handle it there, once.
Add context on the way up, without changing the shape. fmt.Errorf("loading tenant %s: %w", id, err) in Go, exception chaining in Java and Python, .context(...) in Rust. Cheap, preserves the cause, and turns an anonymous failure into a searchable one.

Push back on these in review:

A custom exception hierarchy of eight types when the code distinguishes two cases.
A try/catch in every method, each one logging the same exception, producing five stack traces per failure.
A circuit breaker, bulkhead, and retry policy introduced for a call that fails because the input was invalid.
Result/Either types retrofitted through an entire call chain to fix one swallowed exception.
A global handler that catches everything and returns 200 so dashboards stay green.

Important

"Log and continue" is a decision that the operation is optional. If it is genuinely optional — telemetry, cache warming, a nice-to-have notification — say so in a comment on the line, and swallow deliberately and narrowly. Jagopy's own telemetry writes are exactly this case: they are wrapped in fail-safe handlers on purpose, because a metrics failure must never break a user request. That is a documented decision, not an accident, and the comment is what makes the difference visible to the next reviewer.

Comment templates that an agent can act on

"except Exception: pass in the parse loop discards every malformed record silently. Log the line number and the exception, and count skipped rows in the return value."
"The error from json.Unmarshal is discarded with _; a malformed payload proceeds with a zero-valued struct. Return the wrapped error."
"catch (Exception e) { return null; } makes a network failure indistinguishable from 'not found'. Let IOException propagate and return Optional.empty() only for a genuine miss."
"The connection is opened before the try and closed inside it — on failure it leaks. Use try-with-resources."
"This retries on a 400. Only retry 5xx and timeouts, cap at 3 attempts with exponential backoff and jitter."

Previous Guide

Null, Empty, and Boundary Edge Cases: The Reviewer's Systematic Checklist

Next Guide

Injection, Secrets, and Broken Access Control: Security Defects You Can Catch in a Diff

Interactive Practice

Review Defective Code in the Workbench

Test your ability to spot this defect in our interactive Monaco-powered PR code editor.

Open Workbench