Review Agent PRs That Add In-Memory Caches

A queue worker PR landed on a Monday. An agent wrapped loadAccount in a module-level Map. The happy-path demo dropped two database round trips. Then a second tenant reused the same process. The Map still held the first tenant's plan flag. Billing applied the wrong ceiling for eleven minutes.

Agent cache hunks hide inside performance commits. Busy reviewers treat them as local tweaks. They are process-wide behavior changes, not local tweaks.

Treat a cache as shared state

An in-memory cache is shared mutable state. It outlives a single request handler. It ignores tenant boundaries unless the key encodes them. It also ignores writes unless a bust path exists.

Agent patches often add the Map first. They rarely add invalidation next. They almost never add a size bound. They sometimes key only on userId. That key collides across tenants in one process.

What these hunks look like

The following snippet is a review fixture. It is not production code. It shows a common agent pattern.

// agent-generated "optimization"
const accountCache = new Map();

async function loadAccount(accountId) {
  if (accountCache.has(accountId)) {
    return accountCache.get(accountId);
  }
  const row = await db.accounts.findById(accountId);
  accountCache.set(accountId, row);
  return row;
}

The write path stayed completely untouched. updatePlan still wrote SQL only. Readers kept serving the stale row. No TTL or maxSize existed on the Map. The worker process became a silent source of truth.

A slightly safer agent variant still fails review.

import { LRUCache } from "lru-cache";

const cache = new LRUCache({ max: 500, ttl: 60_000 });

export async function loadAccount(ctx, accountId) {
  const hit = cache.get(accountId);
  if (hit) return hit;
  const row = await db.accounts.findById(accountId);
  cache.set(accountId, row);
  return row;
}

The LRU bound helps memory pressure. The TTL helps accidental freshness. The key still omits ctx.tenantId. Cross-tenant bleed remains possible after deploy. Review must treat that omission as a defect.

Request scope versus module scope

Module scope is the usual failure. Request scope is a different class. The next fixture keeps data on req only.

function accountCacheFor(req) {
  if (!req.accountCache) {
    req.accountCache = new Map();
  }
  return req.accountCache;
}

export async function loadAccount(req, accountId) {
  const cache = accountCacheFor(req);
  if (cache.has(accountId)) return cache.get(accountId);
  const row = await db.accounts.findById(accountId);
  cache.set(accountId, row);
  return row;
}

The Map dies with the request. Tenant bleed across requests cannot happen here. Write coupling still matters inside one request. Reviewers should still test that path.

Five signals on every cache hunk

Reviewers can score the hunk with five signals. Each signal is a merge gate. Missing any signal means extra tests.

1. Key composition. The key must include tenant, locale, and auth scope.
2. Write coupling. Every mutating path must bust or update the entry.
3. Bound and TTL. Unbounded maps grow until the worker OOMs.
4. Null caching. Cached undefined can hide newly created rows.
5. Process shape. Cluster workers do not share a Map, so sticky state appears.

Skip vibe-based approval on green CI. Score the five signals in the review note. Paste the score above the merge button.

Numbered review workflow

Use this sequence on the raw diff. Do not start in the rendered UI.

1. Isolate hunks that allocate Map, WeakMap, LRUCache, or memoize wrappers.
2. List every read function that now returns cached data.
3. List every write function that mutates the same records.
4. Check whether those writes sit in the same pull request.
5. Demand a key builder that is pure and total.
6. Revert the cache if writes live in another service.
7. Keep the cache only when tests cover bleed, stale reads, and eviction.

The workflow is mechanical on purpose. Agents repeat the same shortcut. Humans miss it under calendar pressure.

Trust, revert, or test

Hunk pattern	Review action	Reason
Pure memo of a referentially transparent helper	Trust after a unit test	No shared domain state
Module Map keyed on id only	Revert	Tenant and auth bleed
LRU with TTL, still no bust on write	Test, then likely revert	Stale reads after updates
Cache beside the write in one module	Test invalidation and size	Can be correct
Cache of authorization decisions	Revert	Policy must be fresh
Request-scoped WeakMap on req	Test, often trust	Lifetime matches the request
Cluster-local cache for global config	Test plus documented lag	Workers diverge

Authorization caches deserve a hard revert. A delayed deny becomes an allow. A delayed allow becomes a lockout. Neither belongs in an agent drive-by patch.

Artifact: scan a unified diff

The script below is a review aid. It is not production software. Reviewers should pipe git diff into it. It prints cache-like added lines with file and line.

#!/usr/bin/env node
"use strict";

const fs = require("fs");

const PATTERNS = [
  { name: "map_alloc", re: /\bnew\s+(Map|WeakMap|LRUCache)\b/ },
  { name: "memoize", re: /\b(memoize|lru_cache|cacheable)\s*\(/i },
  { name: "cache_ident", re: /\b\w*[Cc]ache\w*\.(get|set|has|delete|clear)\s*\(/ },
  { name: "ttl", re: /\b(ttl|maxAge|expiresIn)\b/ },
  { name: "unbounded_set", re: /\.\s*set\s*\([^,]+,\s*[^)]+\)/ },
];

function scan(diff) {
  const findings = [];
  let file = "";
  let newLine = 0;
  for (const raw of diff.split(/\r?\n/)) {
    if (raw.startsWith("+++ b/")) {
      file = raw.slice(6);
      continue;
    }
    if (raw.startsWith("@@")) {
      const m = raw.match(/\+(\d+)/);
      newLine = m ? Number(m[1]) : 0;
      continue;
    }
    if (raw.startsWith("+") && !raw.startsWith("+++")) {
      const line = raw.slice(1);
      for (const p of PATTERNS) {
        if (p.re.test(line)) {
          findings.push({
            file,
            line: newLine,
            signal: p.name,
            text: line.trim(),
          });
        }
      }
      newLine += 1;
      continue;
    }
    if (raw.startsWith(" ")) newLine += 1;
  }
  return findings;
}

const diff = fs.readFileSync(0, "utf8");
const findings = scan(diff);
if (!findings.length) {
  console.log("No cache-like added lines.");
  process.exit(0);
}
for (const f of findings) {
  console.log(`${f.file}:${f.line} [${f.signal}] ${f.text}`);
}
process.exit(2);

Run it from a clean checkout. The commands below stay local and repeatable.

git fetch origin pull/812/head:pr-812
git checkout pr-812
git diff main...HEAD -- '*.js' '*.ts' ':!dist' ':!vendor' \
  | node scan-cache-hunks.js

Exit code 2 means a human must score the five signals. Exit code 0 means the scanner saw nothing. Nothing is not safety. Dynamic caches can hide behind helpers named remember.

A concrete test plan

Do not merge on green unit tests alone. Cached reads need adversarial cases.

1. Two tenants, same accountId space, sequential reads. Expect isolation.
2. Update a cached field, then read immediately. Expect the new value.
3. Cache a missing row, then insert it. Expect a hit of the new row.
4. Fill past max and read the oldest key. Expect eviction, not OOM.
5. Expire TTL, then read. Expect a store round trip.
6. Run two Node cluster workers. Expect no cross-worker assumption.

A minimal test double for signal 1 looks like this. Label it as a required example, not a measured run.

test("cache key includes tenant", async () => {
  const t1 = { tenantId: "acme", accountId: "001" };
  const t2 = { tenantId: "globex", accountId: "001" };
  await loadAccount(t1);
  await db.accounts.updatePlan("acme", "001", "pro");
  const a = await loadAccount(t1);
  const b = await loadAccount(t2);
  assert.equal(a.plan, "pro");
  assert.equal(b.tenantId, "globex");
  assert.notEqual(a.plan, b.plan);
});

Agents often test only the hit path. The hit path is the least interesting path. Stale writes and tenant collisions are the merge gates.

Run the scanner off the laptop

Large diffs stall cheap review laptops. A remote shell keeps the review machine cool.

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

MonkeyCode is an open-source project. It offers free hosted model access. It also offers a free server option. Reviewers can place the scanner and the diff on that server. A hosted model can draft a tenant-aware key builder. The server can run the git diff pipeline above. The reviewer still owns the merge decision. The scanner does not.

This is not a benchmark. No latency or quota numbers are claimed here.

Limitations

The regex scanner misses factories. A helper named remember will slip through. Comments that mention cache will false-positive. Minified vendor files will noise the report. Exclude vendor/ and dist/ in the diff pathspec.

The playbook also ignores distributed caches. Redis invalidation is a different review. This article covers process memory only. TTL values are product policy. A scanner cannot pick them. Owners must.

Who should not use this approach

Skip this playbook for pure function memoization inside a module test. Skip it when a platform cache already sits behind a reviewed library. Skip it when the team has no test database. A cache without isolation tests is a production incident waiting for traffic.

Do not use the scanner as a CI green light. Use it as a triage net only. Humans still read the write path. Humans still reject authorization caches.

Close the loop on the Monday PR

The Monday worker needed one extra key field. tenantId belonged in the Map key. updatePlan needed accountCache.delete(key). A 60-second TTL needed a documented lag. Until those landed, the right review action was revert.

Cache hunks feel like gifts from the agent. They are ownership of stale state. Review them as state machines, not as speedups. Trust only the request-scoped cases. Revert policy caches on sight. Test everything that survives the revert pass.

MonkeyCode provides free models that can run this workflow.