Code review · Python

Review a Python Service Client

Medium Code review

A backend service calls an internal billing API through this helper. It runs in a pool of 40 workers.

Review it: list the problems, what would happen in production, and how to fix each one.

The code to review

There are 6 issues to find.

import logging

import requests

log = logging.getLogger(__name__)
API_KEY = "sk_live_51Hx9cQe2X7fK"


def get_invoice(invoice_id):
    headers = {"Authorization": f"Bearer {API_KEY}"}
    log.info("calling billing api with headers %s", headers)
    while True:
        try:
            r = requests.get(f"https://billing.internal/invoices/{invoice_id}", headers=headers)
            return r.json()
        except:
            log.info("retrying")

Write your review

Loading the editor… (JavaScript is required to run code in your browser.)

The issues and how to fix them

Show the issues

1. Secret hard-coded in source

Critical, line 6

A live API key in the repository is visible to everyone with read access, ends up in backups and CI logs, and cannot be rotated without a deploy.

Fix: Load it from the environment or a secrets manager, rotate the leaked key now, and add secret scanning to CI.

2. Secret written to logs

Critical, line 11

Logging the headers writes the bearer token to every log line, and logs are shipped to systems with much wider access.

Fix: Never log credentials. Log the invoice ID and outcome, and add a logging filter that redacts Authorization headers.

3. No timeout

Major, line 14

requests waits forever by default. If billing hangs, every worker eventually blocks on this call and the whole service stops responding.

Fix: Always pass a timeout, for example timeout=(3, 10) for connect and read.

4. Infinite retry loop with no backoff (retry storm)

Major, line 12-17

Any error, including a 400 or a bug in our code, is retried immediately and forever. When billing is struggling, 40 workers retrying in a tight loop multiply its load and keep it down.

Fix: Retry a bounded number of times (e.g. 3), only for transient errors (connection errors, 502/503/504, 429), with exponential backoff and jitter; then raise.

5. Bare except and unchecked status

Major, line 15-16

except: also catches KeyboardInterrupt and SystemExit, and swallows the error details. A 404 or 500 response is not an exception, so r.json() returns an error body as if it were an invoice (or raises on HTML).

Fix: Call r.raise_for_status(), catch specific exceptions (requests.RequestException), and log them with log.warning(..., exc_info=True).

6. A new connection per call

Minor, line 14

requests.get opens a new TCP and TLS connection every time.

Fix: Reuse a requests.Session (connection pooling) configured with the retry policy via HTTPAdapter(max_retries=Retry(...)).

Reliability reviews check four things on every outbound call: secrets, timeouts, retries (bounded, with backoff, only when safe) and error handling. Explaining how unbounded retries turn a partial outage into a full one is a strong signal for SRE and backend roles.

Fixed version

import logging
import os

import requests
from requests.adapters import HTTPAdapter
from urllib3.util.retry import Retry

log = logging.getLogger(__name__)

session = requests.Session()
session.headers["Authorization"] = f"Bearer {os.environ['BILLING_API_KEY']}"
session.mount("https://", HTTPAdapter(max_retries=Retry(
    total=3, backoff_factor=0.5, status_forcelist=(429, 502, 503, 504),
    allowed_methods=("GET",), respect_retry_after_header=True)))


def get_invoice(invoice_id: str) -> dict:
    try:
        r = session.get(f"https://billing.internal/invoices/{invoice_id}", timeout=(3, 10))
        r.raise_for_status()
        return r.json()
    except requests.RequestException:
        log.warning("billing call failed for invoice %s", invoice_id, exc_info=True)
        raise

Follow-up questions

  • When is it unsafe to retry a request? (non-idempotent POST)
  • What would a circuit breaker add?