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?
More Code Clinic: Code Review problems
- Review a Laravel Orders Controller · Medium
- Review an Express User Search Endpoint · Easy
- Review a Payment Webhook Handler · Medium
- Review a Sign-Up Function · Medium
- Review a CSV Export Endpoint · Medium
- Review a React Search Component · Medium