Cache Keys Are Contracts: Reviewing Agent PRs That Memoize

Agent pull requests that add a cache usually ship a correctness bug, not a speedup. Green tests do not contradict that. They almost never write, then read, then write again.

Review the cache key, the invalidation path, and object identity before you merge the decorator. Speed is a secondary claim. It is cheap to measure after the contract is real.

Why this class of PR keeps landing

Coding agents are rewarded for making a slow function look fast. A one-line decorator is a high-probability patch. functools.lru_cache, a module-level dict, or a thin Redis wrapper all read as helpful in a diff.

The surrounding tests stay green for a structural reason. They construct a service, call it once, and assert a number. Nothing in that shape can see a stale hit.

This sits next to the current wave of agent-written application code. The question is not whether a model can type a decorator. The question is whether the PR changed a data contract while claiming a performance win.

A constructed PR, not a war story

The patch below is a constructed example. It is labeled so it is not mistaken for a production incident or a measured benchmark.

# constructed example — agent-style "perf" patch
from functools import lru_cache
from typing import Any


class InvoiceService:
    def __init__(self, repo: Any, tenant_id: str) -> None:
        self.repo = repo
        self.tenant_id = tenant_id

    @lru_cache(maxsize=256)
    def get_totals(self, account_id: str) -> dict:
        rows = self.repo.fetch_open_invoices(account_id)
        return {
            "count": len(rows),
            "sum": sum(r.amount for r in rows),
        }

Three defects sit in that decorator. They are independent. Any one of them is merge-blocking.

1. self is part of the cache key. The bound method pins the instance. A test can swap repo and still receive the old answer.
2. tenant_id lives on the instance and is missing from the key. Two tenants that share an account_id format collide.
3. The return value is a mutable dict. A caller that runs totals["sum"] += fee mutates the cache.

None of those show up in a single-call unit test. That is why the CI screenshot in the PR is not evidence.

What to trust

Trust is narrow. It is not a vibe about “simple caching.”

• Pure functions of immutable inputs. tuple, frozenset, str, int. No ORM objects. No self.
• Explicit key builders that include tenant, principal, resource version, and a schema version.
• Invalidation on the write path, in the same PR, with a test that writes then reads.
• Hit/miss counters that do not log raw keys if those keys contain identifiers you would not put in an access log.

Trust the comment that says “this is a pure projection of X” only after you prove X cannot change under the cache. If X can change, the comment is a wish.

What to revert

Revert is cheaper than a follow-up ticket. Agents will re-propose the decorator on the next pass.

Revert the cache when any of these are true:

• The function is an instance method and the decorator is lru_cache or cache.
• The key omits tenant, region, or principal.
• There is no write-path invalidation and no TTL with a stated staleness budget.
• The cached object is mutable or contains nested mutables.
• The PR “fixes” a slow test by caching, rather than fixing the query.
• A process-local map is proposed for a multi-worker server as if it were shared state.

# commands that surface the usual tells
git fetch origin
git diff origin/main...HEAD -G 'lru_cache|cachetools|@cache|memoize|ttl_cache'
git diff origin/main...HEAD -- '*.py'
rg -n 'lru_cache|cache_clear|TTL|invalidate|_cache\s*=' $(git diff --name-only origin/main...HEAD)

If the only new lines are the decorator and a changelog bullet that says “performance,” revert the decorator. Keep any legitimate query rewrite that arrived in the same PR. Split the commit if the two changes are tangled.

What to test

Do not add a benchmark first. Add a contract test that would have failed on the constructed PR.

# constructed tests — labeled, intended to run under pytest
import threading
from dataclasses import dataclass


@dataclass
class Row:
    amount: int


class FakeRepo:
    def __init__(self) -> None:
        self.rows = {"acct-1": [Row(10), Row(5)]}
        self.calls = 0

    def fetch_open_invoices(self, account_id: str):
        self.calls += 1
        return list(self.rows.get(account_id, []))

    def record_payment(self, account_id: str, amount: int) -> None:
        current = self.rows.get(account_id, [])
        self.rows[account_id] = [Row(r.amount) for r in current if r.amount != amount]


def test_write_then_read_sees_payment():
    repo = FakeRepo()
    svc = InvoiceService(repo, tenant_id="t-east")
    first = svc.get_totals("acct-1")
    repo.record_payment("acct-1", 10)
    second = svc.get_totals("acct-1")
    assert first["sum"] == 15
    assert second["sum"] == 5  # fails on lru_cache(self, account_id)


def test_tenants_do_not_share_account_keys():
    east_repo = FakeRepo()
    west_repo = FakeRepo()
    west_repo.rows = {"acct-1": [Row(99)]}
    east = InvoiceService(east_repo, tenant_id="t-east")
    west = InvoiceService(west_repo, tenant_id="t-west")
    assert east.get_totals("acct-1")["sum"] != west.get_totals("acct-1")["sum"]


def test_caller_cannot_mutate_cache_entry():
    repo = FakeRepo()
    svc = InvoiceService(repo, tenant_id="t-east")
    a = svc.get_totals("acct-1")
    a["sum"] = 0
    b = svc.get_totals("acct-1")
    assert b["sum"] == 15

Add a concurrency check if the service is threaded. A process-local dict without a lock is a race, not a cache.

def test_no_torn_read_under_parallel_fill():
    repo = FakeRepo()
    svc = InvoiceService(repo, tenant_id="t-east")
    errors: list[Exception] = []

    def call() -> None:
        try:
            totals = svc.get_totals("acct-1")
            assert set(totals) == {"count", "sum"}
        except Exception as exc:  # constructed harness
            errors.append(exc)

    threads = [threading.Thread(target=call) for _ in range(32)]
    for t in threads:
        t.start()
    for t in threads:
        t.join()
    assert errors == []

If the PR introduces a shared store, test serialization as its own contract. JSON that drops keys, reorders sets, or coerces Decimal to float is a silent schema change. Cache hits will freeze that wrong shape.

Decision table for the review comment

Paste a table. Agents respond to structure. A paragraph of taste does not.

Signal in the diff	Trust	Revert	Minimum test
@lru_cache on a static method of immutable args	Conditional	No, if the key is complete	Collision + cache_clear on write
@lru_cache on an instance method	No	Yes, the decorator	Write-then-read; instance identity
Module-level dict keyed on id only	No	Yes, until tenant+version are in the key	Cross-tenant; two ids, same shape
TTL with no jitter on a hot key	Partial	Keep TTL; reject as-is if a stampede is possible	Expiry window
Cache returns an ORM instance	No	Yes	Mutate instance; second read
Write path deletes the same key the read path uses	Conditional	No, if sibling keys are also covered	Write-then-read; missed-delete of siblings
Cache added only to make CI faster	No	Yes	Restore the real query assertion

A safer shape to request instead of “just cache it”

Ask for an explicit key and an immutable value. That is a smaller change than a new caching product.

from dataclasses import dataclass

SCHEMA = 1


@dataclass(frozen=True)
class Totals:
    count: int
    sum: int


def totals_key(tenant_id: str, account_id: str) -> tuple:
    return (SCHEMA, tenant_id, account_id)


# constructed sketch — write path must delete totals_key(...)
def record_payment_and_drop(cache, tenant_id: str, account_id: str, amount: int) -> None:
    persist_payment(tenant_id, account_id, amount)
    cache.delete(totals_key(tenant_id, account_id))

If the agent cannot name totals_key in both the read and write paths, it does not understand the contract. Do not accept a decorator as a substitute for that name.

A review workflow that does not depend on the model

The pass is mechanical. It fits a short review window.

1. Search the diff for cache primitives: lru_cache, cachetools, @cache, memoize, ttl, redis, aiocache.
2. For each hit, write the key as a tuple on the review: (schema, tenant, principal, resource, inputs...).
3. Find the write path in the same PR. If it is missing, request it or revert the cache.
4. Require one write-then-read test and one cross-tenant test. Reject a benchmark as a substitute.
5. Check mutability of the stored value. Prefer tuple, NamedTuple, or a frozen dataclass.
6. If the service is multi-process, reject process-local maps unless the staleness budget is explicit in the PR body.

You can do that in the hosting UI. You can also dump the diff and annotate it locally:

git diff origin/main...HEAD > /tmp/pr.diff
rg -n 'lru_cache|@cache|memoize|ttl_cache|cache_clear' /tmp/pr.diff

A second mechanical pass is optional. Replay the same checklist against the diff in a throwaway environment, not against production data.

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

MonkeyCode is an open-source coding environment with free model access and a free server option. It is relevant here only as a place to re-run the review prompt on a branch you already intend to discard. It does not replace the decision table. It does not prove a cache is safe. If you use that second pass, paste the diff, the table, and the three tests. Ask for missing keys and missing write-path deletes. Discard any suggestion that adds more caching “to be sure.”

Limitations

This approach assumes you can name the write paths. Eventual-consistency stores, CDC pipelines, and caches filled by another service will not yield a one-PR invalidation story. Do not pretend they will.

Do not use process-local memoization for:

• Multi-tenant responses that include any identifier not in the key
• Values with a legal or financial staleness budget of zero
• Objects that embed permissions, because a permission change is a write you will miss
• Endpoints that already have HTTP caching with a different key design

The tests above are constructed. They are not evidence from a particular codebase. They fail closed on the usual agent patch. They will not catch a wrong TTL that is still “eventually” correct. They will not catch a cache stampede unless you add load.

Who should skip this workflow: reviewers on a PR that does not touch caching; teams that already have a shared cache library with mandatory key helpers and CI rules that ban raw lru_cache on methods. In those shops, enforce the library. Do not re-litigate each decorator.

What merge means

Merging a cache is merging a new contract. The key is the schema. The invalidation path is the migration. The test is the only proof.

If those three are not in the PR, the performance claim is unfinished work. Revert the decorator. Leave the query. Ask for a write-then-read test. That is the whole review.