How to Review a Pull Request for Production Reliability Risks

Most pull requests that cause incidents were reviewed, approved, and green in CI. The reviewer checked that the code was correct. Nobody checked what it would do at peak traffic, with a slow dependency, against a table with forty million rows. This production code review checklist is about that second question.

These checks work in any language and any review tool. The hard part is not knowing the list. It is recognising the small, innocent-looking lines in a diff that trigger each item. This guide gives you the questions, the diff “tells” that should prompt them, an annotated example, and a PR template you can drop into a repository today.

Why do standard code reviews miss production reliability risks?

A typical review reads the diff top to bottom and asks: is this correct, is it readable, is it tested? Those are good questions, and reliability bugs pass all three. The code that loops over a customer’s orders is correct. The HTTP call without a timeout is readable. The retry decorator is tested, and the tests pass.

What fails in production is not the logic. It is the interaction between the code and conditions the test environment does not have:

• Scale. Tests use ten rows. Production has a customer with 180,000.
• Concurrency. Tests run one request. Production runs 400 per second, all competing for the same pool.
• Slow dependencies. Test doubles answer in a millisecond. Real services slow down, and they slow down at the worst time.
• Partial failure. Tests either succeed or fail. Production succeeds halfway, then times out.

The diff shows none of this directly. A reviewer has to bring that context. This is the gap described in Production Reliability vs Code Review: code review evaluates the implementation, while reliability depends on how the implementation behaves in a specific production system.

What should a PR reliability checklist check?

Nearly every reliability incident caused by a code change traces back to one of three shifts. The change made each request cost more, it changed what happens when something fails, or it sent more work to another system. A useful PR reliability checklist is organised around those three shifts, because each one has recognisable signs in a diff.

Two more areas sit alongside the three questions. Config, migrations, and feature flags change behaviour with little code. Detection and rollback decide how long a bad change stays in production. Not every PR needs every question. A change to a high-traffic request path, a shared client library, or a scheduled job deserves the full pass; a copy change does not. How to Identify High Risk Pull Requests covers how to decide which PRs get the deep review.

How do you spot a change in resource use in a diff?

A resource-use change means each request, message, or job run now consumes more of something the service has a finite amount of: CPU, memory, database connections, threads, file descriptors, or rows scanned. The change is often invisible at test scale and linear (or worse) at production scale.

New queries and loops over unbounded collections

The most common tell is a loop whose length is set by data rather than by code: for item in order.items, for user in account.members, .forEach over a query result. Inside that loop, look for a query, a network call, or an allocation. That is the classic N+1 pattern, and its cost is proportional to the size of the largest customer, not the average one.

Equally important is a query that returns an unbounded result set. .all(), findAll(), or a SELECT without LIMIT or pagination loads every matching row into memory. Ask: what is the largest realistic size of this collection, and is there an index that supports the new WHERE clause? A query plan run against production-sized data answers the second part in a minute.

Connections, threads, and pools

A new client, a new ThreadPoolExecutor, or a new connection pool is a new finite resource. Check its size, what happens when it is full (block, reject, or queue without bound), and whether it multiplies by the number of instances. A pool of 20 database connections per pod is 600 connections at 30 pods, which may exceed the database’s connection limit. Holding a connection longer counts too: a slow call added inside a transaction keeps the connection checked out for the duration, which is how pools run dry (see HikariCP: Connection Is Not Available, Request Timed Out).

Caches and payload sizes

An in-process cache without a size bound or eviction policy is a memory leak with a delay. A new field in a response, an eager-loaded relationship, or a batch size raised from 100 to 10,000 increases memory per request and bytes on the wire. The question is always the same: per request, what is the maximum, and what bounds it?

How do you spot a change in failure handling?

Failure handling decides what the service does when something it depends on is slow, down, or returns garbage. Most failure-handling bugs are not visible when everything works, which is why they survive testing. Read every new or changed network call, catch block, and transaction boundary with one question: what happens here when the other side does not answer?

Timeouts

Every network call needs a timeout, and many clients do not set one by default. Python’s requests, for example, documents that requests do not time out unless a timeout value is set explicitly. A missing timeout turns a slow dependency into a blocked worker thread, and enough blocked threads turn it into an outage.

A timeout also has to fit the caller’s budget. If the gateway in front of your service gives up after 2 seconds, a 5-second timeout inside the service means the service keeps working, and keeps loading its dependencies, for requests nobody is waiting for.

Retries

A retry is a bet that the next attempt will succeed. It is a good bet for a brief network blip and a bad one when the dependency is overloaded, because retries add load exactly when there is least capacity. The Google SRE book’s chapter on cascading failures recommends randomized exponential backoff and limits on retries. In a diff, check four things for every retry: how many attempts, what backoff and jitter, which errors are retried (a 400 should not be), and whether retries also happen at another layer. Retries at three layers with three attempts each can mean 27 calls for one user request. The series post on spotting a retry storm before merge goes deeper.

Idempotency and partial failure

A timeout does not mean the operation failed. It means you do not know. If the first attempt charged a card and the response was lost, a retry charges it again. Any retry around a call with side effects needs an idempotency key the downstream service deduplicates on, or a uniqueness constraint that rejects the duplicate. The same applies to message consumers, which usually receive at-least-once delivery (see How to Prevent Duplicate Webhook Processing).

Partial failure is the related question: if step two of three fails, what state is left behind? A function that calls a payment API, then writes a row, then publishes an event can fail between any two steps. The reviewer should be able to say which state each failure point leaves and how it is reconciled.

Swallowed errors and transactions

except Exception: pass, an empty catch, or a .catch(() => null) turns a failure into silently wrong data and removes the signal your alerts depend on. Look for broad handlers that neither log, count, nor re-raise. For transactions, the tell is a remote call (HTTP, queue publish, cache write to another service) between the start and commit of a database transaction. It holds a connection and possibly row locks for the duration of the call, and the remote side effect cannot be rolled back if the transaction later fails.

How do you spot a change in downstream load?

Downstream load is the traffic your change sends to other systems: databases, internal services, third-party APIs, queues. It is the easiest risk to miss in review because the cost lands on someone else’s dashboard. The change looks cheap in your service and expensive in theirs.

The review technique is simple arithmetic. For every new or changed outbound call, estimate calls per second = requests per second × calls per request × attempts per call. Then compare the result with what the downstream service handles today.

Beyond per-request fan-out, look for these sources of new downstream load:

• Batch and scheduled jobs. A cron job that runs at midnight in every region, for every tenant, at once. Ask for its concurrency, rate limit, and schedule, and whether it shares a database with user traffic.
• Polling. A client that polls every 5 seconds instead of 60 is a 12× load increase per client, multiplied by every open browser tab or device.
• Cache TTL and key changes. Dropping a TTL from 300 to 30 seconds can multiply cache misses, and every miss is a call to the backing store. Changing a cache key format invalidates the whole cache at deploy time, which sends a burst of misses to the database at once. The series post on cache stampedes covers this failure.
• Shared clients and libraries. A change to a retry policy or timeout default in a shared HTTP client affects every service that upgrades. Its blast radius is the set of callers, not the set of changed files. How to Assess the Blast Radius of a Code Change explains how to map it.

What about config, migration, and feature flag changes?

These changes are often a few lines, reviewed quickly, and deployed everywhere at once. They deserve more attention than their size suggests.

Configuration

Look at defaults, not just the value in the file you are reading. A changed default in code applies to every environment that does not override it. Ask which environments get which value, whether the value is validated at startup (a typo in a timeout should fail the deploy, not be read as zero), and whether the change is a pool size, timeout, or concurrency limit whose effect multiplies by the instance count.

Database migrations

Migrations fail in production because of table size and lock behaviour, not because the SQL is wrong. In PostgreSQL, a plain CREATE INDEX blocks inserts, updates, and deletes on the table until the build finishes; CREATE INDEX CONCURRENTLY avoids that but cannot run inside a transaction block, which matters for migration tools that wrap each migration in one (see the PostgreSQL CREATE INDEX documentation). Many ALTER TABLE forms need a strong lock, and while waiting for it behind a long-running query, they can block the queries queued after them. Some forms, such as certain column type changes, rewrite the whole table.

Ask two questions of every migration: how long does it lock what, on the largest production table, and is the schema compatible with both the old and new version of the code? During a rolling deploy, both run at once. Renames and drops should follow an expand-then-contract sequence across separate deploys.

Feature flags

A flag is a reliability control only if it actually turns the risky path off. Check the default state, whether the new code path is fully behind the flag (including migrations and background jobs it triggers), and whether turning it off is safe after data has been written in the new format. A flag that is on by default in production is not a gradual rollout.

Diff tells: what you see in the diff and what to ask

The table below is a reviewer’s cheat sheet. Each row is a pattern that is easy to see in a diff, the risk it may introduce, and the question that resolves it. A tell is not a defect. It is a prompt to ask.

An annotated example: reviewing a pull request for production reliability risks

Here is a realistic pull request in a Python service. The stated goal is to show “stock status and past orders” on an order summary page, and to make payment recording “more robust”. It is about twenty changed lines. It passes its tests. Markers [1] to [6] are reviewer annotations.

@@ def build_order_summary(order_id):
     order = db.session.get(Order, order_id)
-    stock = inventory_client.batch_lookup([i.sku for i in order.items], timeout=0.5)
+    history = db.session.execute(                                    [1]
+        select(Order).where(Order.customer_id == order.customer_id)
+    ).scalars().all()
+    stock = {}
+    for item in order.items:                                         [2]
+        try:
+            resp = requests.get(f"{INVENTORY_URL}/skus/{item.sku}")  [3]
+            stock[item.sku] = resp.json()["available"]
+        except Exception:                                            [4]
+            pass
     summary = render_summary(order, stock, history)
-    cache.set(f"summary:{order_id}", summary, timeout=300)
+    cache.set(f"summary:{order_id}", summary, timeout=30)            [5]
     return summary

+@retry(stop=stop_after_attempt(5))                                   [6]
 def record_payment(order, amount):
     payments_api.charge(order.customer_id, amount)
     db.session.add(Payment(order_id=order.id, amount=amount))
     db.session.commit()

Two details are worth calling out. Tenacity’s @retry retries on any exception and waits zero seconds between attempts unless you pass retry= and wait= arguments, so [6] hammers the payment API back to back. And the risks compound: the shorter TTL in [5] raises how often the expensive path in [1] through [4] runs. Reviewed line by line, each change looks reasonable. Reviewed as a change to production behaviour, the PR multiplies load on two dependencies and can double-charge customers.

RECENT_ORDERS_LIMIT = 20

def build_order_summary(order_id):
    order = db.session.get(Order, order_id)
    history = db.session.execute(
        select(Order.id, Order.created_at, Order.total)
        .where(Order.customer_id == order.customer_id)
        .order_by(Order.created_at.desc())
        .limit(RECENT_ORDERS_LIMIT)                          # [1] bounded
    ).all()
    try:
        stock = inventory_client.batch_lookup(               # [2] one call
            [i.sku for i in order.items], timeout=0.5)       # [3] timeout
    except InventoryUnavailable:                             # [4] specific
        metrics.increment("summary.inventory_unavailable")
        stock = None                                         # shown as unknown
    summary = render_summary(order, stock, history)
    cache.set(f"summary:{order_id}", summary, timeout=300)   # [5] unchanged
    return summary

def record_payment(order, amount):
    # [6] no blind retry: the payments API deduplicates on the idempotency key,
    #     and a unique constraint on payments.order_id rejects a second row
    payments_api.charge(order.customer_id, amount,
                        idempotency_key=f"order-{order.id}")
    db.session.add(Payment(order_id=order.id, amount=amount))
    db.session.commit()

What to check before merging backend code: a production code review checklist

A checklist only helps if authors fill it in before review, so put it where they cannot miss it. GitHub pre-fills new pull request descriptions from a pull request template such as .github/pull_request_template.md on the default branch. Ask for concrete answers (a timeout value, a call rate) rather than ticks, and allow “n/a” with a one-line reason.

## Reliability impact
<!-- Answer each line, or write "n/a" with a one-line reason. -->

### Resource use
- [ ] New or changed queries are bounded (LIMIT / pagination) and indexed.
      Largest realistic result size: ___
- [ ] Loops over data-sized collections: max size ___ ; no query or call per item
- [ ] New pools / threads / connections / in-memory caches: size ___ x instances ___
- [ ] Payload or batch size change: ___

### Failure handling
- [ ] Every new network call has a timeout: ___ ms (caller's budget: ___ ms)
- [ ] Retries: attempts ___ , backoff + jitter, retried errors ___ ; operation is idempotent
- [ ] No errors caught and ignored; partial-failure state described below
- [ ] No remote calls while a DB transaction or lock is held

### Downstream load
- [ ] New calls per request to ___ : ___ (x items? x retries?) ; owning team aware
- [ ] Batch jobs / schedules / polling: rate ___ , concurrency ___
- [ ] Cache TTL or key changes: expected effect on hit rate and backend QPS

### Config, migrations, flags
- [ ] Config defaults and per-environment values listed; validated at startup
- [ ] Migration lock impact on largest table; compatible with the running version
- [ ] Feature flag: name ___ , default ___ , whole new path behind it

### Detect and roll back
- [ ] How we will know it is broken: metric / alert / dashboard ___
- [ ] Rollback is a revert: yes / no (why: data written, migration, external calls)

The template is the author’s half. The reviewer’s half is the procedure behind it:

1. Size the blast radius first. Which request paths, jobs, and services run this code, and how much traffic do they carry? Match review depth to that.
2. Check resource use. Find every loop over data, every new query, and every new pool or cache, and ask for the realistic maximum.
3. Check failure handling. For each outbound call: timeout, retries, idempotency, error handling, and whether it sits inside a transaction.
4. Check downstream load. Do the arithmetic: requests per second × calls per request × attempts. Include jobs, polling, and cache changes.
5. Check config, migrations, and flags. Defaults, lock behaviour at production size, compatibility during rollout, and a real kill switch.
6. Confirm detection and rollback. Which signal will show the change is failing, and is reverting it safe?
7. Separate automated findings from judgement. Let tools flag patterns; spend human attention on context the diff does not contain.

For the same discipline applied at the service level rather than the PR level, see the Production Readiness Checklist for a Backend Service.

Where do status checks and AI review fit, and what needs a human?

GitHub’s status checks report the results of CI, linters, and GitHub Apps on each commit in a pull request, and branch protection can require specific checks to pass before merging. AI review adds comments on the diff itself. Both are useful for reliability, within limits.

What automation does well: anything that is a pattern in the text of the diff. A lint rule can flag an HTTP call without a timeout or a bare except. A migration linter can flag a non-concurrent index build. AI review is good at reading intent and pointing out a query inside a loop, a retry around a non-idempotent call, or an error that is caught and dropped. Making those checks required means the obvious tells never reach a human unflagged.

What still needs a human: anything that depends on facts outside the diff. How much traffic hits this endpoint. How large the biggest tenant’s collection is. What the downstream service’s capacity and the caller’s timeout are. Whether the payments API honours idempotency keys. Whether a partial write is acceptable for this business process. An AI reviewer can ask these questions; it usually cannot answer them from the diff alone, and a confident-sounding answer without that context is worse than a question.

The practical split: let automation raise the tells, and require a human to resolve the ones on high-risk paths. Pre Merge Reliability Analysis describes how to bring more of that production context into the pre-merge step so the human is not working from memory.

How Tomosu helps

The hard part of a reliability review is not the checklist. It is knowing, for a given diff, whether it changes resource use, failure handling, or downstream load, and on which production paths. Tomosu scans repositories and pull requests for these conditions and surfaces the evidence a reviewer needs to answer the questions above:

• Resource use: flags queries and outbound calls inside loops, unbounded reads, and new pools or caches, and maps which request paths reach them.
• Failure handling: flags network calls without timeouts, retries without backoff or around non-idempotent operations, swallowed errors, and remote calls inside transactions.
• Downstream load: maps new or changed calls to other services, jobs, and caches, so a reviewer can see where the extra traffic lands.
• Blast radius weighting: weighs each finding by the code paths and callers it affects, so a missing timeout on the checkout path ranks above the same pattern in an internal script.

These findings roll up into the Production Reliability Index, including the Fragility Index and Code Volatility, and are available in the VS Code plugin, the GitHub App, and the MCP server, so the same evidence reaches the author before the PR opens and the reviewer while it is open.

Scan your repository with Tomosu →

Key takeaways

• Most reliability bugs pass correctness review and tests, because they depend on scale, concurrency, and slow dependencies the tests do not have.
• Review every PR on a hot path through three lenses: resource use, failure handling, and downstream load.
• Loops over data-sized collections, calls without timeouts, broad catch blocks, retry decorators, and TTL changes are the most common diff tells.
• Do the load arithmetic: requests per second × calls per request × attempts per call.
• Retries are safe only for idempotent operations, with backoff, jitter, and a total deadline inside the caller’s budget.
• Config, migrations, and flags change production with few lines; review them for defaults, locks, compatibility, and a real kill switch.
• Let status checks and AI review find the patterns; keep humans on the context outside the diff.

Frequently asked questions

A reliability review is three questions asked of every risky diff: what does this cost, what happens when it fails, and who else pays for it. Tomosu surfaces the evidence to answer them before merge. Assess your repository →