Skip to content

JMCP: Papers - jmcp/add-a-request-scoped-cache-hit-miss-coun-qggS4vBG#38

Merged
landigf merged 1 commit intomainfrom
jmcp/add-a-request-scoped-cache-hit-miss-coun-qggS4vBG
Mar 25, 2026
Merged

JMCP: Papers - jmcp/add-a-request-scoped-cache-hit-miss-coun-qggS4vBG#38
landigf merged 1 commit intomainfrom
jmcp/add-a-request-scoped-cache-hit-miss-coun-qggS4vBG

Conversation

@landigf
Copy link
Copy Markdown
Owner

@landigf landigf commented Mar 25, 2026

Review: Request-scoped cache hit/miss counter

Change summary

Adds an AsyncLocalStorage-backed per-request cache layer for DemoState with hit/miss counters, exposed via /api/health. Specifically:

  1. packages/db/src/cache-counter.ts (new) — Uses AsyncLocalStorage to hold a { hits, misses, state } store. Exports runWithCacheCounter, getCacheStats, getCachedState, setCachedState, recordHit, recordMiss, invalidateCachedState.
  2. packages/db/src/demo-store.tsreadDemoState() now checks the ALS-scoped cache before reading from disk; writeDemoState() invalidates it.
  3. packages/db/src/index.ts — Re-exports CacheStats, getCacheStats, runWithCacheCounter.
  4. apps/web/app/api/health/route.ts — Adds cache: getCacheStats() to health response.
  5. packages/auth/package.json + package-lock.json — Bumps drizzle-orm ^0.44.5 → ^0.45.1 (unrelated).
  6. packages/db/test/cache-counter.test.ts (new) — 5 tests covering isolation, null-outside-scope, hit/miss tracking, state caching, and invalidation.

Validation confidence: Medium-High

  • The cache-counter.ts module is clean and well-scoped. Tests cover the core API surface.
  • readDemoState / writeDemoState integration is straightforward.
  • No callers of runWithCacheCounter exist in the diff — the ALS scope is never actually entered, so getCacheStats() in the health route will always return null today.

Risks

# Risk Severity
1 No caller wraps requests in runWithCacheCounter — middleware or request handler needs to call it for the feature to function. Without that, every getCacheStats() returns null and every getCachedState() returns null (cache is a no-op). This is the critical missing piece. High
2 readDemoState is cached but mutations go through writeDemoStatereadDemoState → mutate → writeDemoState. If two concurrent requests share the same ALS scope (they won't, but worth noting), the in-memory DemoState object is mutable and could be mutated in-place between the cache-set and a later cache-get, serving stale/corrupted state. Within a single request scope this is fine, but the cached object is the same reference — any in-place mutation before writeDemoState is called silently alters the cached copy. Low-Medium
3 Drizzle bump is unrelated and unmentioneddrizzle-orm ^0.44.5 → ^0.45.1 is a semver-minor bump sneaked into this branch. Should be a separate commit/PR or at least called out. Low
4 Health route calls getCacheStats() outside any ALS scope — it will always be null there regardless of whether middleware wraps other requests. The health endpoint itself isn't wrapped. Consider whether cache: null in the health response is the intended UX or if global aggregate stats would be more useful for observability. Low

Recommendation

Request changes — the feature is well-structured but incomplete. The main blocker is risk #1: add middleware (or a Next.js instrumentation hook) that wraps each incoming request in runWithCacheCounter() so the counters actually accumulate. Without that, this is dead code behind a nice API.

@landigf landigf merged commit 911b60f into main Mar 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant