Reviewing an Agent PR That Adds Request Coalescing: The Colliding Cache Key, the Test That Lied, and the Meter That Double-Charged

A pull request landed at 09:40 with a confident summary. It claimed to add single-flight deduplication of model calls. It promised roughly 40% token savings. The diff was 214 lines: two new files, one edited test, one config flag.

The reviewer's job was not to grade intent. It was to decide three things. What to trust, what to revert, and what to test.

This review covers one class of agent-generated change: request coalescing in front of a model API. The same steps apply to caching, batching, and retry queues.

Step 1: Read the diff in three passes

Pass one reads only the summary and the file list. Pass two reads the new code in full. Pass three reads the tests and the config change together.

The order matters. Reading tests last prevents the test from defining the expected behavior. An agent writes tests against its own implementation, not against the contract.

git fetch origin pull/412/head:pr-412
git diff --stat main...pr-412
git diff main...pr-412 -- '*_test.py' '*_test.go'
git diff main...pr-412 -- src/

The stat line already said something useful. The test file was edited, not added. An edited test is a rewritten assumption.

Step 2: Extract the safety claim into a falsifiable sentence

The PR summary mixed three claims into one. Deduplication works. Savings are real. Behavior is unchanged.

Only the third claim is checkable without product context. Written as a testable sentence: two concurrent requests with different model parameters must never receive the same response object.

The reduced implementation from the PR reads like this. Logic is unchanged.

# pr_singleflight.py — verbatim logic from the PR, trimmed
import asyncio

class Flight:
    def __init__(self):
        self._locks = {}

    async def run(self, key, fn):
        lock = self._locks.setdefault(key, asyncio.Lock())
        async with lock:
            return await fn()

Two details deserve attention. The key is supplied by the caller. The lock table is never pruned.

Step 3: Build a fake upstream before running the real one

A fake server gives deterministic answers and counts calls. Standard library only, no dependencies to install.

# fake_upstream.py — stdlib only
import json, time
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer

CALLS = {"n": 0}

class Handler(BaseHTTPRequestHandler):
    def do_POST(self):
        body = self.rfile.read(int(self.headers["Content-Length"]))
        req = json.loads(body)
        CALLS["n"] += 1
        time.sleep(0.2)                     # widen the race window
        payload = {"model": req["model"], "text": f"answer-{CALLS['n']}"}
        raw = json.dumps(payload).encode()
        self.send_response(200)
        self.send_header("Content-Type", "application/json")
        self.send_header("Content-Length", str(len(raw)))
        self.end_headers()
        self.wfile.write(raw)

    def log_message(self, *args):           # keep test output readable
        pass

if __name__ == "__main__":
    ThreadingHTTPServer(("127.0.0.1", 8787), Handler).serve_forever()

Run it in one terminal. The handler echoes the model name back, so a wrong answer is visible immediately.

python fake_upstream.py

Step 4: Reproduce the collision with a two-line harness

The caller in the PR built the key from the prompt only. Model name, temperature, and max tokens were dropped. Two different models then share one flight.

# test_key_collision.py
import asyncio, json, urllib.request
from pr_singleflight import Flight

def post(model, prompt):
    body = json.dumps({"model": model, "prompt": prompt}).encode()
    req = urllib.request.Request("http://127.0.0.1:8787/v1/chat",
        data=body, headers={"Content-Type": "application/json"})
    with urllib.request.urlopen(req) as r:
        return json.loads(r.read())

async def main():
    flight = Flight()
    key = "explain this stack trace"          # prompt only, as in the PR
    small, large = await asyncio.gather(
        flight.run(key, lambda: asyncio.to_thread(post, "model-small", key)),
        flight.run(key, lambda: asyncio.to_thread(post, "model-large", key)),
    )
    print("small ->", small)
    print("large ->", large)
    assert small != large, "different models must not share a flight"

asyncio.run(main())

The output is the verdict. Both callers received answer-1 and the server counted one call. The cheaper model answered a request routed to the stronger one.

Revert the key construction. Do not revert the whole feature.

Step 5: Find out why the PR test passed

The bundled test passed in CI. It also passed against a broken implementation. That is the useful definition of a lying test.

# test_pr_suite.py — reduced, passes for the wrong reason
def test_dedup(monkeypatch):
    calls = []
    monkeypatch.setattr(pr, "call_model",
                        lambda m, p: calls.append(m) or {"text": "x"})
    run_two_concurrent_requests(model_a="small", model_b="large")
    assert len(calls) == 1          # asserts savings, not correctness

The mock discards the model argument. It records only that a call happened. The assertion rewards fewer calls and never inspects which model produced the answer.

Replace it with two assertions. One on the upstream call count. One on the model recorded per caller.

def test_dedup_same_key(fake_upstream):
    run_two_concurrent_requests(model_a="small", model_b="small")
    assert fake_upstream.calls == 1
    assert fake_upstream.models == ["small"]

def test_no_dedup_across_models(fake_upstream):
    run_two_concurrent_requests(model_a="small", model_b="large")
    assert fake_upstream.calls == 2
    assert sorted(fake_upstream.models) == ["large", "small"]

The second test fails on the original diff. That is the point.

Step 6: Audit the meter as a separate change

The deduplication and the token meter were submitted together. They were reviewed together. That was a mistake.

The meter incremented usage inside the waiter path. One upstream response fanned out to N waiters and charged N times.

Three consequences follow. Reported savings are wrong. Budget limits trip early. Per-tenant billing becomes indefensible.

Charge once per upstream call. The fix is small and local.

# meter.py — move the increment to the call site
async def call_model(model, prompt, meter):
    usage = await transport(model, prompt)      # one network call
    meter.charge(usage.tokens, model=model)     # one charge
    return usage

Split the PR. Merge the coalescing fix with the collision test. Open a second PR for the meter with its own fan-out test.

Step 7: Record the decision as a table

A review that ends in prose is not reviewable later. A table is.

Component	Decision	Reason
Flight.run structure	Trust	Correct primitive for single-process dedup
Key construction	Revert	Omits model, temperature, max tokens
Lock table growth	Test	Unbounded dict, no pruning on completion
PR test suite	Revert	Mock hides the model argument
Collision harness	Merge	Fails on the original diff
Token meter	Split out	Separate failure domain, separate review

One row deserves a second look. The lock table never removes finished keys. Under high prompt cardinality, memory grows for the process lifetime.

A bounded structure solves it. So does deletion after release, guarded by a refcount.

async def run(self, key, fn):
    entry = self._locks.setdefault(key, {"lock": asyncio.Lock(), "waiters": 0})
    entry["waiters"] += 1
    try:
        async with entry["lock"]:
            return await fn()
    finally:
        entry["waiters"] -= 1
        if entry["waiters"] == 0:
            self._locks.pop(key, None)      # prune on last waiter

This patch is untested until the harness covers it. Add a third test that runs 1,000 distinct keys and asserts the table size returns to zero.

What this workflow does not cover

Single-flight with an asyncio.Lock works inside one process. Four workers mean four separate lock tables. Deduplication silently weakens, and no test in this PR would notice.

Cross-process coalescing needs shared state. That is a different design with different failure modes: lock expiry, split brain, stale keys after a crash.

Caching is also out of scope here. In-flight deduplication returns one result to concurrent callers. A cache returns an old result to a future caller. The second one needs TTLs, invalidation, and tenant isolation.

Teams with strict determinism requirements should skip this optimization. So should anyone caching responses across tenants in a shared key space. The security risk outweighs the token savings.

Streaming responses complicate the picture again. Partial chunks cannot be replayed to a late joiner without buffering the whole stream. Reviewers should reject streaming dedup unless the PR includes that buffer and its memory bound.

Why a fake server is the cheapest part of the review

The harness above runs on any laptop in under a minute. It needs no credentials and no network. It surfaces the bug the summary tried to hide.

MonkeyCode's free model access and free server option are enough to run the same three-terminal setup: fake upstream, harness, and diff. The free token allowance covers repeated runs of a fan-out test, which is otherwise the expensive part of this kind of review. That makes coalescing and metering checks cheap to iterate.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

Model names, quotas, and limits change without notice. Verify current terms on the project's own pages before relying on them in a plan.

The reusable checklist

1. Read the test diff before the source diff.
2. Restate the safety claim as one falsifiable sentence.
3. Build a counting fake upstream with a deliberate delay.
4. Write the cross-parameter test first. It must fail.
5. Check every cache key for omitted parameters.
6. Audit metering as a separate diff, always.
7. Record trust, revert, and test in one table.

The savings number in the summary is a hypothesis. The harness is the evidence. Reviewers who skip the harness are trusting the agent that wrote the bug.