A Cache Is a Second Database: Reviewing Agent PRs That Memoize Too Early

Agent pull requests that add caching usually look like performance work. They are almost always consistency work. Until the key, lifetime, invalidation path, and stampede behavior are specified, treat an unsolicited cache as a second database that the rest of the system does not know about.

This review walks a constructed agent PR. The diff is typical: a hot read path, a green unit suite, and a memoization wrapper presented as a free speedup. The merge question is not “is Redis faster than Postgres?” The merge question is whether the PR created a new source of truth.

The PR under review

Label: constructed example, not a production incident. An agent was asked to “speed up invoice reads.” It added an in-process memo plus a Redis overlay and left the write path untouched.

# invoice_service.py  (agent diff, simplified)
import json
from functools import lru_cache

TTL_SECONDS = 3600

@lru_cache(maxsize=4096)
def _local(invoice_id: str) -> dict:
    return repo.fetch_invoice(invoice_id)

async def get_invoice(invoice_id: str) -> dict:
    cached = await redis.get(f"invoice:{invoice_id}")
    if cached:
        return json.loads(cached)

    invoice = _local(invoice_id)
    await redis.set(
        f"invoice:{invoice_id}",
        json.dumps(invoice, default=str),
        ex=TTL_SECONDS,
    )
    return invoice

async def void_invoice(invoice_id: str) -> None:
    await repo.mark_void(invoice_id)
    # agent comment: cache will expire in an hour

The unit tests still pass. They construct the service with a fake repo, call get_invoice once, and assert on field names. No write is issued. No second reader is issued. No clock is advanced.

# test_invoice_service.py  (also agent-authored)
def test_get_invoice_returns_number(service):
    invoice = asyncio.run(service.get_invoice("inv_1"))
    assert invoice["number"].startswith("INV-")

That test cannot see stale data. It never creates any.

What the diff is actually changing

Three stores now exist for one invoice:

1. The relational row in repo.
2. The process-local lru_cache on _local.
3. The Redis string at invoice:{id}.

Reads prefer Redis, then the process cache, then the database. Writes touch only the database. After void_invoice, a reader in the same process can still return the cached dict for up to an hour. A reader in another process can do the same from Redis. A deploy that restarts one replica clears only that replica’s LRU. The other replicas keep serving the voided invoice.

This is not an optimization sitting on top of an unchanged API. It is a new consistency model with no name, no metric, and no owner.

Trust, revert, test

Trust these parts, if they exist

• A measured hot path. A profile, slow query log, or trace that names fetch_invoice is a real constraint. A comment that says “this might be slow” is not.
• Tenant-aware key prefixes, if they are already the house style (tenant:{tid}:invoice:{id}).
• Explicit serialization, if the payload is a documented DTO rather than json.dumps(obj, default=str).

None of those, by themselves, make the cache mergeable.

Revert these parts by default

• Process-local lru_cache on anything that can be mutated by another request, worker, or replica.
• Redis writes with a TTL and no delete on the mutating path.
• default=str JSON. It turns datetimes and Decimals into strings that later reads will not round-trip.
• Keys that omit tenant, environment, or schema version.
• Comments that defer correctness to expiry. A TTL is a bound on staleness, not an invalidation protocol.

If those lines are the entire “performance” change, the honest review is a revert with a request for a cache design, not a nit about naming.

Test these behaviors before merge

Green field-name tests are not enough. The smallest honest suite is a state machine: read, mutate, read again, from more than one client.

# test_invoice_cache_contract.py
import asyncio
import json
import pytest

@pytest.mark.asyncio
async def test_void_is_visible_to_second_reader(service, redis, repo):
    first = await service.get_invoice("inv_1")
    assert first["status"] == "open"

    await service.void_invoice("inv_1")

    # Direct DB truth.
    assert repo.fetch_invoice("inv_1")["status"] == "void"

    # Cached read must not contradict DB.
    second = await service.get_invoice("inv_1")
    assert second["status"] == "void"

    raw = await redis.get("invoice:inv_1")
    if raw is not None:
        assert json.loads(raw)["status"] == "void"


@pytest.mark.asyncio
async def test_two_readers_do_not_stampede_writes(service, redis, monkeypatch):
    calls = {"n": 0}
    original = service.repo.fetch_invoice

    def counted(invoice_id):
        calls["n"] += 1
        return original(invoice_id)

    monkeypatch.setattr(service.repo, "fetch_invoice", counted)
    await redis.delete("invoice:inv_1")

    await asyncio.gather(
        service.get_invoice("inv_1"),
        service.get_invoice("inv_1"),
        service.get_invoice("inv_1"),
    )
    # A correct fill uses a lock, singleflight, or request coalescing.
    assert calls["n"] == 1

The second test will fail on the agent diff. That failure is the review artifact. Do not “fix” it by weakening the assertion to calls["n"] >= 1.

A compact review table

Signal in the PR	Meaning	Review action
Cache added, write path unchanged	Second database with no replication story	Revert cache, or add invalidation + tests
TTL used as the only invalidation	Staleness is unbounded by product rules, bounded by a constant	Require a documented stale budget
lru_cache on a request-level helper	Replica-local memory, not a shared cache	Revert or move to explicit request memo
Unit test hits a cold cache once	Suite cannot observe stale reads	Block until a mutate-then-read test exists
Key is invoice:{id}	Cross-tenant collision risk	Require the same prefix rules as the DB
json.dumps(..., default=str)	Irreversible type loss	Require a schema or codec
Comment promises a follow-up PR	The follow-up is not in this diff	Treat as absent

Commands that make the review cheap

Inspect whether the agent touched every mutating path, not just the getter.

git fetch origin pull/1842/head:pr-1842
git checkout pr-1842

git diff main...HEAD -- '*.py' | rg -n "lru_cache|redis\.(get|set|delete)|invalidate|void_|update_|mark_"

rg -n "get_invoice|void_invoice|pay_invoice|refund_invoice" -g '*.py'

If set appears and delete does not, the write path is unfinished. If void_invoice exists in the service and not in the diff, the agent optimized a read and ignored the business event that makes the read wrong.

Reproduce a stale read without a full cluster:

pytest -q test_invoice_cache_contract.py -k void_is_visible

If that test does not exist yet, the review is incomplete. Adding the test is cheaper than arguing about intended TTL behavior in comments.

Where a disposable model and server help

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

The review above does not need a model. It needs a mutating test and a second reader. Models are useful for one narrow job: enumerating invalidation paths the agent missed. That list is a candidate checklist, not evidence that the PR is safe.

A practical split:

1. Ask a model, on a throwaway branch, to list every function that can change invoice state.
2. Diff that list against the PR with rg, not against the model’s confidence.
3. Run the contract tests on a dedicated process so lru_cache and Redis are shared the way they will be in staging.

MonkeyCode’s free model access is enough for step 1 if you already have the repo context. The free server option is useful for step 3 when the cache only misbehaves under concurrent clients, which a single local pytest process can hide. Neither step replaces the table above.

Example prompt, treated as a draft checklist only:

Given these files, list mutating operations on Invoice.
For each, name the cache key that must be deleted or overwritten.
Do not propose a new cache. Do not claim the current cache is correct.

Keep the model output in the review notes. Do not paste it into the PR as documentation of runtime behavior.

Failure analysis: why this class of PR ships

Agents are rewarded for making a slow function return faster on the next call. Caches do that on the happy path. Reviewers are rewarded for not blocking a green suite. The combination produces optimization theater: latency charts that improve in a demo, and a voided invoice that remains payable for 59 minutes.

Two mechanical tells show up repeatedly.

Tell A: the cache is introduced in the same patch as the measurement. There is no before/after harness, only a comment. Without a harness, there is no way to know whether the slowness was the query, the N+1 caller, or a missing index.

Tell B: invalidation is described in prose. “We can add delete later” is a product fork. Later means another PR, another reviewer, and a window where production already has the second database.

If either tell is present, do not negotiate on TTL values. Revert the cache lines and keep any legitimate query fix.

A minimal mergeable shape

If the team actually wants a cache, the PR has to name the store and the invalidation. One acceptable skeleton:

CACHE_KEY = "v1:invoice:{invoice_id}"

async def get_invoice(invoice_id: str) -> dict:
    key = CACHE_KEY.format(invoice_id=invoice_id)
    raw = await redis.get(key)
    if raw is not None:
        return InvoiceDTO.model_validate_json(raw).model_dump()

    invoice = repo.fetch_invoice(invoice_id)
    payload = InvoiceDTO.model_validate(invoice).model_dump_json()
    await redis.set(key, payload, ex=60)  # stale budget, not a design
    return json.loads(payload)

async def void_invoice(invoice_id: str) -> None:
    await repo.mark_void(invoice_id)
    await redis.delete(CACHE_KEY.format(invoice_id=invoice_id))

Still missing from that skeleton: singleflight, stampede tests, tenant prefixes, and what happens when delete fails. It is mergeable only relative to the original agent diff, and only with the contract tests above. It is not a general cache framework.

Limitations

This review pattern assumes a single object cache keyed by ID. It does not cover HTTP Cache-Control, CDN layers, ORM identity maps, or materialized views. Those have different invalidation graphs.

It also assumes you can run a second reader against the same Redis and the same process memory. If the agent cached inside a serverless isolate that dies after each request, lru_cache may be accidentally harmless and Redis may still be wrong. Harmless in one runtime is not a reason to keep the decorator.

Do not use the model-enumeration step on repositories that contain secrets in fixtures. Cache keys and TTL constants are enough context.

Who should not use this approach

• Teams with a standing cache standard and a shared helper. Review against that helper, not against this article’s skeleton.
• Changes that are actually query plans or indexes. If EXPLAIN already shows the win, adding Redis is a new product.
• Reviewers looking for a reason to auto-approve “performance” PRs. The point of the table is to block incomplete stores, not to rubber-stamp them when a TTL is present.
• Anyone treating a model-generated invalidation list as complete. Models omit event handlers, admin scripts, and out-of-band SQL.

A cache is a second database. Agent PRs that add one without a write protocol are not unfinished optimizations. They are unfinished data models. Revert the store, keep the measurement, and do not merge a TTL as if it were a spec.

If you need a throwaway box to run the two-reader contract tests away from your laptop, MonkeyCode’s free server option is one place to park that harness while you decide whether the cache should exist at all.