At 9:04 a.m. a reviewer opens a pull request titled "add exponential backoff". The diff is 62 lines long. CI is green. The code reads like a textbook.
It also retries a 401 six times. Along the way it sleeps about 63 seconds.
That pattern is common in agent-generated PRs. The happy path is correct. The failure path is never examined by anyone.
This review walks through one such PR end to end. The agent session that produced it used MonkeyCode's free model access, and the harness below ran on its free server option. Both are operator-supplied availability claims, not benchmarks.
Disclosure: This article was prepared as part of MonkeyCode's product outreach.
Step 1 — Read the diff for three retry smells
The submitted hunk is short. It hides three defects.
- return _post(url, payload, api_key)
+ for attempt in range(6):
+ try:
+ return _post(url, payload, api_key)
+ except Exception:
+ time.sleep(2 ** attempt + random.random())
+ return None
The first smell is except Exception. It catches KeyError from a bug in the caller. It catches programming errors and network errors with the same hand.
The second smell is the missing status filter. A 400 and a 503 produce identical behavior. Authentication failures get retried, which is pure latency with no upside.
The third smell is return None. A silent None corrupts every downstream caller. Every consumer now needs a new branch for a failure that used to raise.
Two smaller issues sit beside those. Retry-After is ignored entirely. And random.random() adds a fraction of a second, which is jitter in name only.
Step 2 — Stand up a fake server that records timestamps
You cannot review timing by reading code. Build a server that writes down every request.
The harness below uses only the standard library. It needs no network access and no API key.
# fake_limiter.py — stdlib only
import json, threading, time
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
class Recorder:
def __init__(self):
self.hits = []
self.lock = threading.Lock()
def add(self, path, headers):
with self.lock:
self.hits.append({"path": path, "t": time.monotonic(), "headers": dict(headers)})
def make_server(recorder, script):
"""script: list of (status, extra_headers, body); the last entry repeats."""
state = {"i": 0}
class Handler(BaseHTTPRequestHandler):
protocol_version = "HTTP/1.1"
def do_POST(self):
length = int(self.headers.get("Content-Length") or 0)
self.rfile.read(length)
recorder.add(self.path, self.headers)
i = min(state["i"], len(script) - 1)
state["i"] += 1
status, extra, body = script[i]
payload = json.dumps(body).encode()
self.send_response(status)
for k, v in extra.items():
self.send_header(k, v)
self.send_header("Content-Type", "application/json")
self.send_header("Content-Length", str(len(payload)))
self.end_headers()
self.wfile.write(payload)
def log_message(self, *args):
pass
srv = ThreadingHTTPServer(("127.0.0.1", 0), Handler)
threading.Thread(target=srv.serve_forever, daemon=True).start()
return srv, srv.server_address[1]
ThreadingHTTPServer gets a random free port. The recorder stores time.monotonic() per hit. That timestamp is the whole artifact.
Step 3 — Write five assertions against the recorded timeline
Each test fixes one script and then checks the request log. Timing tolerance is deliberately loose.
# test_backoff.py — python -m unittest -v test_backoff.py
import unittest
from fake_limiter import Recorder, make_server
import client_fixed as client # point at client_pr to watch it fail
class BackoffReview(unittest.TestCase):
def setUp(self):
self.rec = Recorder()
def _serve(self, script):
srv, port = make_server(self.rec, script)
self.addCleanup(srv.shutdown)
return f"http://127.0.0.1:{port}/v1/chat"
def test_no_retry_on_401(self):
url = self._serve([(401, {}, {"error": "unauthorized"})])
with self.assertRaises(client.ModelError):
client.call_model(url, {"prompt": "hi"}, "bad-key")
self.assertEqual(len(self.rec.hits), 1)
def test_honours_retry_after(self):
url = self._serve([(429, {"Retry-After": "1"}, {"error": "slow down"}),
(200, {}, {"output": "ok"})])
out = client.call_model(url, {"prompt": "hi"}, "k")
self.assertEqual(out["output"], "ok")
gap = self.rec.hits[1]["t"] - self.rec.hits[0]["t"]
self.assertGreaterEqual(gap, 0.95)
def test_raises_when_attempts_exhausted(self):
url = self._serve([(429, {"Retry-After": "1"}, {"error": "slow down"})])
with self.assertRaises(client.ModelError):
client.call_model(url, {"prompt": "hi"}, "k", max_attempts=3)
self.assertEqual(len(self.rec.hits), 3)
def test_idempotency_key_is_stable(self):
url = self._serve([(429, {"Retry-After": "0"}, {}), (200, {}, {"output": "ok"})])
client.call_model(url, {"prompt": "hi"}, "k")
keys = {h["headers"].get("Idempotency-Key") for h in self.rec.hits}
self.assertEqual(len(keys), 1)
self.assertNotIn(None, keys)
def test_stops_before_budget(self):
url = self._serve([(503, {}, {"error": "unavailable"})])
with self.assertRaises(client.ModelError):
client.call_model(url, {"prompt": "hi"}, "k", max_attempts=10, budget_s=1.0)
elapsed = self.rec.hits[-1]["t"] - self.rec.hits[0]["t"]
self.assertLess(elapsed, 2.0)
The test that passed for the wrong reason
A first draft asserted len(self.rec.hits) > 1. That proves a retry happened. It says nothing about whether the delay obeyed the server.
The draft also asserted total wall-clock time under five seconds. It passed, because the second response was a 200. The underlying defect only appears when every attempt fails.
Assert on the interval between recorded requests. Assert on the terminal state. Never assert on a happy path you already know works.
Step 4 — Patch the loop and keep what was right
The agent was right about one thing. Backoff belongs in the client, not in every caller. Keep the retry wrapper and replace its body.
# client_fixed.py
import json, random, time, urllib.error, urllib.request
RETRY_STATUS = {429, 500, 502, 503, 504}
class ModelError(RuntimeError):
pass
def call_model(url, payload, api_key, *, max_attempts=4, budget_s=20.0, idem_key=None):
idem_key = idem_key or f"{int(time.time() * 1000)}-{random.randrange(1 << 20):05x}"
headers = {
"Content-Type": "application/json",
"Authorization": f"Bearer {api_key}",
"Idempotency-Key": idem_key,
}
body = json.dumps(payload).encode()
start, last = time.monotonic(), None
for attempt in range(1, max_attempts + 1):
try:
req = urllib.request.Request(url, data=body, headers=headers, method="POST")
with urllib.request.urlopen(req, timeout=30) as r:
return json.loads(r.read())
except urllib.error.HTTPError as e:
last = f"HTTP {e.code}"
if e.code not in RETRY_STATUS:
raise ModelError(last) from e # 400/401/403: fail fast
wait = _retry_after(e.headers.get("Retry-After"))
except (urllib.error.URLError, TimeoutError) as e:
last = f"transport: {e}"
wait = None
if attempt == max_attempts:
break
if wait is not None:
delay = wait
else:
delay = min(8.0, 0.5 * 2 ** (attempt - 1)) * random.uniform(0.5, 1.0)
if time.monotonic() - start + delay > budget_s:
break
time.sleep(delay)
raise ModelError(f"gave up after {attempt} attempts: {last}")
def _retry_after(raw):
if not raw:
return None
try:
return max(0.0, float(raw))
except ValueError:
return None # HTTP-date form: fall back to computed backoff
Three properties matter here. Non-retryable codes raise immediately. Retry-After wins over computed backoff. The elapsed budget caps total sleep, so a 429 storm cannot hang a worker.
The idempotency key is generated once and reused across attempts. A retried POST then stays one logical request.
Step 5 — Record the review decision
The table below is the actual output of the review. It is short enough to paste into the PR description.
| Area | Decision | Evidence |
|---|---|---|
| Retry wrapper placement | Trust | Callers keep one code path |
Constant 2 ** attempt sleep |
Revert | Cap and budget are missing |
except Exception |
Revert | Hides KeyError from the caller |
return None |
Revert | Silent failures reach production |
Retry-After handling |
Test | test_honours_retry_after |
| 4xx handling | Test | test_no_retry_on_401 |
| Key stability | Test | test_idempotency_key_is_stable |
| Total wait ceiling | Test | test_stops_before_budget |
Trust covers pure structure. Revert covers anything that changes failure semantics. Test covers behavior that only appears over time.
That split keeps review moving. You do not rewrite the PR. You revert three hunks and prove the rest.
Where this harness lies to you
The fake server is one process on loopback. It never models TLS handshakes, connection reuse, or proxy-generated 502s. A passing suite says nothing about those.
Timing assertions are the fragile part. A loaded CI runner can add tens of milliseconds. Keep thresholds loose, or mark timing tests as serial.
The harness also ignores fairness. It cannot tell you whether one tenant starves another under concurrency limits.
Finally, free-tier behavior can change without notice. Treat the environment as convenient, not contractual.
Who should skip this approach
Teams behind a paid tier with documented retry semantics should test against that contract instead. Their provider publishes limits and status codes worth encoding directly.
Teams that need per-tenant rate limiting need a shared limiter, not a per-client sleep loop. The harness here will not surface that class of bug.
Anyone without a way to run the client against a local endpoint should stop at Step 1. The three smells are visible by reading, and they cost nothing to flag.
What carries over
Agent-generated retry code fails in predictable places. It over-catches. It ignores server hints. It converts errors into None.
A recorded request timeline catches all three in under an hour. The server is 40 lines. The tests are five assertions.
The rest of the PR is usually fine, and saying so is part of the review. If you want to reproduce this workflow, MonkeyCode's free model access and free server option are enough to generate a candidate PR and run the harness against it. Point the import at your own client first.
Top comments (1)
Could you add a slow-200 case to the budget test? The example keeps a 30-second request timeout, while budget_s is checked around retry sleeps. A first request could succeed after the one-second budget and still return normally. Measuring the last request's start time would miss that. An end-to-end elapsed-time assertion, with each attempt's timeout derived from the remaining deadline, would distinguish a backoff budget from a bound on the whole operation.