feat(cache): in-memory tier, size limits, sharding, observability#1122
feat(cache): in-memory tier, size limits, sharding, observability#1122
Conversation
…vability FILE_SYSTEM cache engine sites (e.g. baggagio) suffer 10-16s cache HITs due to full disk reads + JSON.parse on every request. This PR adds four fixes: 1. In-memory CacheStorage tier (inMemoryCache.ts) — LRU storing raw bytes in RAM (256MB default), wired as L1 before disk via tiered cache with auto-backfill from disk on first access. 2. Per-entry size limit (2MB default via CACHE_MAX_ENTRY_SIZE) — skip caching oversized entries at write time, evict legacy oversized entries on read. 3. Hex directory sharding — 2-level hex subdirs (a1/b2/key) instead of flat dir with 10k+ files, reducing filesystem metadata pressure. 4. Enriched observability — loader_cache_entry_size histogram (opt-in), content_length attribute on cache-match OTel spans. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a tiered cache (in-memory LRU + sharded filesystem), enforces a configurable max cache-entry size, expands OTEL metrics (entry sizes, bg revalidation, json-parse timing, eviction reasons), and tags/propagates cache tier and content-length across cache flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader as "blocks/loader"
participant InMem as "InMemoryCache"
participant FileCache as "FileSystemCache"
participant Disk as "Filesystem"
participant OTEL as "OTEL Metrics"
Client->>Loader: Request with cache key
Loader->>InMem: cache.match(key)
alt In-memory HIT
InMem-->>Loader: Response (with X-Cache-Tier)
Loader->>OTEL: record cache_entry_size, json_parse timing
Loader-->>Client: serve cached response
else MISS
Loader->>FileCache: cache.match(key) via shardedPath
FileCache->>Disk: read file from sharded path
alt File hit and size <= CACHE_MAX_ENTRY_SIZE
Disk-->>FileCache: file data (Content-Length)
FileCache-->>Loader: Response (tagged X-Cache-Tier)
Loader->>OTEL: record content_length & cache_entry_size
Loader->>InMem: cache.put(key, response) -- backfill
Loader-->>Client: serve cached response
else missing or oversized
FileCache->>Disk: delete oversized (if applicable)
Loader->>Upstream: fetch fresh
Loader->>InMem: cache.put(key, fresh)
Loader->>FileCache: cache.put(key, fresh) -> write to sharded path
Loader-->>Client: serve fresh response
end
opt stale-while-revalidate stale hit
Loader->>Loader: launch background revalidation
Loader->>OTEL: record bg_revalidation duration
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/inMemoryCache.ts">
<violation number="1" location="runtime/caches/inMemoryCache.ts:60">
P1: Cached responses lose their original HTTP status because `match` rebuilds `Response` without `status`, turning cached non-200 responses into 200.</violation>
</file>
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:14">
P2: Validate `CACHE_MAX_ENTRY_SIZE` after parsing; invalid env values currently produce `NaN` and disable the oversized-entry check.</violation>
</file>
<file name="blocks/loader.ts">
<violation number="1" location="blocks/loader.ts:163">
P2: `CACHE_MAX_ENTRY_SIZE` parsing is not validated; invalid env values can become `NaN` and disable oversized-entry skipping.</violation>
<violation number="2" location="blocks/loader.ts:309">
P3: `loader_cache_entry_size` is recorded before the size gate, so uncached oversized responses are included in a metric labeled as cached entries.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
runtime/caches/cacheWriteWorker.ts (1)
60-64: Code duplication withfileSystem.ts, but acceptable for worker isolation.The
shardedPathfunction is duplicated here and infileSystem.ts(lines 18-22). While this violates DRY, workers run in a separate thread and need self-contained code. Consider adding a comment noting this intentional duplication for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/cacheWriteWorker.ts` around lines 60 - 64, The shardedPath function is intentionally duplicated between cacheWriteWorker.ts (function shardedPath) and fileSystem.ts to keep the worker self-contained; add a brief comment above the shardedPath definition in cacheWriteWorker.ts stating that this duplication is deliberate for worker isolation and referencing the equivalent implementation in fileSystem.ts so future maintainers understand it is not accidental and should not be refactored across threads.runtime/caches/fileSystem.ts (1)
146-151: Consider checking file size before reading to avoid memory spikes.The current implementation reads the entire file into memory before checking if it exceeds
CACHE_MAX_ENTRY_SIZE. For legacy oversized entries (e.g., >100MB), this could cause temporary memory pressure.💡 Proposed optimization using stat()
async function getFile(key: string) { if (!isCacheInitialized) { await assertCacheDirectory(); } try { const filePath = shardedPath(FILE_SYSTEM_CACHE_DIRECTORY, key); + const stat = await Deno.stat(filePath); + if (stat.size > CACHE_MAX_ENTRY_SIZE) { + Deno.remove(filePath).catch(() => {}); + return null; + } const fileContent = await Deno.readFile(filePath); - if (fileContent.length > CACHE_MAX_ENTRY_SIZE) { - Deno.remove(filePath).catch(() => {}); - return null; - } return fileContent; } catch (_err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/fileSystem.ts` around lines 146 - 151, In fileSystem cache reading logic (around shardedPath and the read/remove sequence), avoid reading the whole file before size check: call Deno.stat (or Deno.lstat) on the computed filePath first, inspect the returned size against CACHE_MAX_ENTRY_SIZE, and if it exceeds the limit call Deno.remove(filePath).catch(()=>{}) and return null; only call Deno.readFile(filePath) when the stat size is within the allowed limit. Ensure you preserve existing error handling/returns used by the surrounding function.blocks/loader.ts (1)
163-165:CACHE_MAX_ENTRY_SIZEis duplicated across files.This constant is defined identically in both
blocks/loader.tsandruntime/caches/fileSystem.ts. If one changes without the other, the write-time check (here) and read-time eviction (fileSystem.ts) could diverge.💡 Consider extracting to a shared constants module
Create a shared constant in a common module (e.g.,
runtime/caches/constants.ts) and import it in both files:// runtime/caches/constants.ts export const CACHE_MAX_ENTRY_SIZE = parseInt( Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152", // 2 MB );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` around lines 163 - 165, The CACHE_MAX_ENTRY_SIZE constant is duplicated; extract it into a single shared module (export const CACHE_MAX_ENTRY_SIZE = parseInt(Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152")) and import that exported symbol wherever currently defined (replace the local CACHE_MAX_ENTRY_SIZE in loader.ts and in fileSystem.ts). Update both modules to use the shared CACHE_MAX_ENTRY_SIZE export so write-time checks and read-time eviction use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/caches/inMemoryCache.ts`:
- Around line 16-19: The CacheEntry interface and cache flow must store and
restore response status: add status: number to the CacheEntry interface, update
put() to read response.status (and optionally statusText) and include it in the
stored entry along with body and headers, and update match() to reconstruct the
Response using the saved status when creating new Response(...) so non-200
responses (errors, redirects) preserve semantics; reference the CacheEntry
interface and the put() and match() methods to locate and apply these changes.
---
Nitpick comments:
In `@blocks/loader.ts`:
- Around line 163-165: The CACHE_MAX_ENTRY_SIZE constant is duplicated; extract
it into a single shared module (export const CACHE_MAX_ENTRY_SIZE =
parseInt(Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152")) and import that
exported symbol wherever currently defined (replace the local
CACHE_MAX_ENTRY_SIZE in loader.ts and in fileSystem.ts). Update both modules to
use the shared CACHE_MAX_ENTRY_SIZE export so write-time checks and read-time
eviction use the same source of truth.
In `@runtime/caches/cacheWriteWorker.ts`:
- Around line 60-64: The shardedPath function is intentionally duplicated
between cacheWriteWorker.ts (function shardedPath) and fileSystem.ts to keep the
worker self-contained; add a brief comment above the shardedPath definition in
cacheWriteWorker.ts stating that this duplication is deliberate for worker
isolation and referencing the equivalent implementation in fileSystem.ts so
future maintainers understand it is not accidental and should not be refactored
across threads.
In `@runtime/caches/fileSystem.ts`:
- Around line 146-151: In fileSystem cache reading logic (around shardedPath and
the read/remove sequence), avoid reading the whole file before size check: call
Deno.stat (or Deno.lstat) on the computed filePath first, inspect the returned
size against CACHE_MAX_ENTRY_SIZE, and if it exceeds the limit call
Deno.remove(filePath).catch(()=>{}) and return null; only call
Deno.readFile(filePath) when the stat size is within the allowed limit. Ensure
you preserve existing error handling/returns used by the surrounding function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ddc4782-7299-4c2c-812c-0ea2db00968c
📒 Files selected for processing (6)
blocks/loader.tsruntime/caches/cacheWriteWorker.tsruntime/caches/common.tsruntime/caches/fileSystem.tsruntime/caches/inMemoryCache.tsruntime/caches/mod.ts
1. Cache tier attribution — tiered.ts tags which tier (0=memory, 1=disk) served the hit via X-Cache-Tier header, exposed as cache_tier span attr 2. Bypass reason — loader_cache counter now includes reason dimension (no-store, no-cache, null-key, disabled) when status=bypass 3. Background revalidation duration — loader_bg_revalidation histogram tracks how long stale-while-revalidate background fetches take 4. LRU eviction counter — lru_cache_eviction with reason dimension (evict, set, delete) fires on every LRU dispose 5. JSON.parse duration — resolver_latency with status=json_parse tracks time spent in matched.json() (behind OTEL_ENABLE_EXTRA_METRICS) 6. Cache key count gauge — lru_cache_keys and lru_cache_bytes observable gauges report LRU occupancy on each metric export interval Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rdering 1. inMemoryCache: store and restore response.status so non-200 responses (errors, redirects) are not silently converted to 200 on cache hit. 2. CACHE_MAX_ENTRY_SIZE: add NaN fallback (|| 2097152) in both loader.ts and fileSystem.ts so invalid env values don't disable the size guard. 3. loader_cache_entry_size: move recording after the size gate so only actually-cached entries are included in the metric. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P1: Remove dead stats.lruEviction counter from loader.ts (evictions are
tracked by lru_cache_eviction in lrucache.ts).
P2: Add NaN fallback for MEMORY_CACHE_MAX_SIZE and MEMORY_CACHE_MAX_ITEMS
in inMemoryCache.ts (matching the pattern used for CACHE_MAX_ENTRY_SIZE).
P2: Cache created shard dirs in fileSystem.ts putFile() via
initializedShardDirs Set to avoid redundant Deno.mkdir syscalls on
every write.
P3: Strip X-Cache-Tier internal header in common.ts after reading it
into the span attribute, preventing leakage to HTTP clients.
P3: Add cache name label to lru_cache_keys and lru_cache_bytes observable
gauges so multiple LRU instances are distinguishable.
P3: Use .catch().finally() instead of .then().catch() for bgRevalidation
timing to avoid misleading error logs if the metric recording throws.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:16">
P2: Using `||` for the env fallback makes `CACHE_MAX_ENTRY_SIZE=0` impossible, because `0` is coerced to the default `2097152`.</violation>
</file>
<file name="runtime/caches/lrucache.ts">
<violation number="1" location="runtime/caches/lrucache.ts:79">
P2: Tracking every newly opened LRU instance in `activeCaches` creates a memory/metrics leak because `open()` is not memoized and these entries are never removed.</violation>
</file>
<file name="runtime/caches/tiered.ts">
<violation number="1" location="runtime/caches/tiered.ts:110">
P1: Avoid mutating `matched.headers` directly; Cache/Fetch responses may have immutable headers and this can throw at runtime on cache hits.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:140">
P1: Only mark a shard directory as initialized after `mkdir` succeeds; otherwise one transient `mkdir` failure permanently breaks writes for that shard.</violation>
</file>
<file name="runtime/caches/inMemoryCache.ts">
<violation number="1" location="runtime/caches/inMemoryCache.ts:11">
P2: Using `||` for numeric env fallbacks incorrectly overrides explicit `0` values, so valid zero limits cannot be configured.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
runtime/caches/lrucache.ts (1)
62-72: Observable gauges lack distinguishing attributes for multiple caches.When multiple LRUCache instances exist,
observer.observe()is called multiple times without attributes to distinguish them. This makes metrics ambiguous—backends may overwrite values or aggregate them unpredictably.Consider adding a
cache_nameattribute or aggregating to a single total:Option: Add cache_name attribute
This requires passing the cache name through to the registration:
lruSizeGauge.addCallback((observer) => { - for (const cache of activeCaches) { - observer.observe(cache.size); + for (const { cache, name } of activeCaches) { + observer.observe(cache.size, { cache_name: name }); } });Option: Aggregate to single total
lruSizeGauge.addCallback((observer) => { - for (const cache of activeCaches) { - observer.observe(cache.size); - } + const total = activeCaches.reduce((sum, c) => sum + c.size, 0); + observer.observe(total); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/lrucache.ts` around lines 62 - 72, The observable gauges lruSizeGauge and lruBytesGauge call observer.observe(...) for each entry in activeCaches without labels, causing metric collisions; update the callbacks to pass an attribute (e.g., cache_name) with each observe call so each cache is identifiable (use the cache's existing identifier or add a name/id property on LRUCache instances referenced by activeCaches), i.e., replace observer.observe(cache.size) and observer.observe(cache.calculatedSize) with observer.observe(value, { cache_name: cache.nameOrId }) and ensure LRUCache exposes that name/id before registering the gauges.blocks/loader.ts (1)
398-406: Keep JSON parse timing out ofresolver_latency.
resolver_latencyis already recorded infinallyfor the full loader call. Addingstatus: "json_parse"samples to the same histogram makes hit/stale paths emit two latency points with different meanings, which will skew aggregates that do not filter onstatus. Use a dedicated histogram for cache-parse latency instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` around lines 398 - 406, The JSON parse timing is being recorded into the same histogram used for resolver_latency (stats.latency), which causes double/ambiguous latency samples; change the OTEL_ENABLE_EXTRA_METRICS branch that measures matched.json() to record into a dedicated histogram (e.g., add and use stats.cacheParseLatency or stats.parse_latency) instead of stats.latency — measure duration around matched.json() as you already do (parseStart/performance.now()) and call the new histogram's record method with the same labels (loader and status: "json_parse") so resolver_latency remains untouched; update any references to stats.latency.record for this JSON path to use the new stats.<cacheParseLatency|parse_latency>.record call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocks/loader.ts`:
- Around line 159-163: The loader metric lruEviction (meter counter
"loader_cache_eviction") is declared but never incremented; wire it to the real
eviction hook by incrementing that counter when an eviction happens in the LRU
implementation (the lru_cache_eviction signal in runtime/caches/lrucache.ts).
Locate the eviction path in runtime/caches/lrucache.ts (the function/method that
emits or handles lru_cache_eviction) and call or expose a handler that
increments loader_cache_eviction (lruEviction.increment(...) or equivalent) so
every real eviction increments the loader metric.
- Around line 325-327: In callHandlerAndCache(), when jsonStringEncoded.length >
CACHE_MAX_ENTRY_SIZE the current early return leaves an expired/stale cache
entry in place; change the flow to remove the existing cache key before
returning by checking for and deleting the stored key (use the same cache key
used earlier in callHandlerAndCache()) so oversized revalidations do not leave
stale values pinned; ensure deletion happens only when a key exists and before
returning from the oversize check.
- Around line 373-382: The bg revalidation metric is currently recorded in each
caller's finally() causing N samples for one revalidation; move the timing and
stats.bgRevalidation.record logic into the function passed to bgFlights.do()
(the implementation that invokes callHandlerAndCache with request.url) so the
timer (bgStart = performance.now()) and the OTEL_ENABLE_EXTRA_METRICS check/
stats.bgRevalidation.record call execute exactly once per singleFlight
execution; update the call site using bgFlights.do(request.url,
callHandlerAndCache) to instead start the timer and record inside the provided
handler wrapper passed to bgFlights.do while keeping the same key (request.url)
and preserving callHandlerAndCache invocation.
In `@runtime/caches/inMemoryCache.ts`:
- Around line 46-49: The delete() method in inMemoryCache.ts currently ignores
its CacheQueryOptions parameter (named _options) unlike match() which calls
assertNoOptions(); update delete to assertNoOptions on the options to reject
unsupported flags. Inside the async delete implementation (function named
delete), call assertNoOptions(_options, 'delete') at the start (or rename to
options and call assertNoOptions(options, 'delete')) so it behaves like redis.ts
and match() and consistently rejects ignoreSearch/ignoreMethod/ignoreVary.
- Around line 9-14: The environment-parsed LRU limits MEMORY_CACHE_MAX_SIZE and
MEMORY_CACHE_MAX_ITEMS may be NaN, zero, or negative; update the parsing to
validate and coerce to positive finite integers before using them in the LRU
init: parse the env string to a number, ensure Number.isFinite(value) and value
> 0, fallback to the existing defaults if invalid, and (optionally) Math.floor
or clamp to an upper bound; apply this logic where MEMORY_CACHE_MAX_SIZE and
MEMORY_CACHE_MAX_ITEMS are defined and before they are passed into the LRU/cache
initialization so the LRU `max`/`maxSize` never receive NaN or non-positive
values.
- Around line 72-75: Remove the early return that checks response.body (the line
"if (!response.body) return;") so responses with null/empty bodies are still
cached; keep using requestURLSHA1(request) for the cache key and continue to
read the body via response.arrayBuffer() (which yields an empty ArrayBuffer for
null bodies) and collect headers via [...response.headers.entries()] as before;
apply the same removal of the guard across inMemoryCache, redis, fileSystem,
headerscache, and lrucache implementations to ensure 204/304 and other
empty-body responses are cached.
In `@runtime/caches/lrucache.ts`:
- Around line 59-60: The module-level activeCaches array currently holds strong
references to every LRUCache<string, any> instance causing leaks; change the
registration so activeCaches stores WeakRef<LRUCache<string, any>> (or use a Set
and remove old instances) and update any code that iterates activeCaches (e.g.,
the cache registration/push site and the gauge reporting logic) to dereference
the WeakRef, skip collected caches, and prune dead refs from the collection;
alternatively implement a singleton getter for LRUCache instances to avoid
multiple creations and ensure metrics read only live caches.
- Around line 40-43: The dispose callback on the LRU instance is currently async
and calls cache.delete(key) which can produce an unhandled rejection because
lru-cache doesn't await dispose; change the implementation of the dispose
handler (the dispose arrow function) to handle errors synchronously by removing
the async/await and wrapping the delete call with a promise catch (or a
try/catch if you keep async) so any rejection is caught and handled (e.g., log
via processLogger or increment lruEvictionCounter with an error tag) — ensure
the revised dispose references the same symbols (dispose, lruEvictionCounter,
cache.delete) and always swallows or logs errors instead of letting them bubble
as unhandled rejections.
---
Nitpick comments:
In `@blocks/loader.ts`:
- Around line 398-406: The JSON parse timing is being recorded into the same
histogram used for resolver_latency (stats.latency), which causes
double/ambiguous latency samples; change the OTEL_ENABLE_EXTRA_METRICS branch
that measures matched.json() to record into a dedicated histogram (e.g., add and
use stats.cacheParseLatency or stats.parse_latency) instead of stats.latency —
measure duration around matched.json() as you already do
(parseStart/performance.now()) and call the new histogram's record method with
the same labels (loader and status: "json_parse") so resolver_latency remains
untouched; update any references to stats.latency.record for this JSON path to
use the new stats.<cacheParseLatency|parse_latency>.record call.
In `@runtime/caches/lrucache.ts`:
- Around line 62-72: The observable gauges lruSizeGauge and lruBytesGauge call
observer.observe(...) for each entry in activeCaches without labels, causing
metric collisions; update the callbacks to pass an attribute (e.g., cache_name)
with each observe call so each cache is identifiable (use the cache's existing
identifier or add a name/id property on LRUCache instances referenced by
activeCaches), i.e., replace observer.observe(cache.size) and
observer.observe(cache.calculatedSize) with observer.observe(value, {
cache_name: cache.nameOrId }) and ensure LRUCache exposes that name/id before
registering the gauges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf1015cf-386d-4713-829d-3c4bcab1be09
📒 Files selected for processing (6)
blocks/loader.tsruntime/caches/common.tsruntime/caches/fileSystem.tsruntime/caches/inMemoryCache.tsruntime/caches/lrucache.tsruntime/caches/tiered.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- runtime/caches/common.ts
- runtime/caches/fileSystem.ts
blocks/loader.ts
Outdated
| lruEviction: meter.createCounter("loader_cache_eviction", { | ||
| description: "LRU cache evictions", | ||
| unit: "1", | ||
| valueType: ValueType.DOUBLE, | ||
| }), |
There was a problem hiding this comment.
Wire loader_cache_eviction to the real eviction hook.
This counter is declared here but never incremented. In the provided cache stack, the only actual eviction signal is lru_cache_eviction in runtime/caches/lrucache.ts, so loader_cache_eviction will stay flat as written.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blocks/loader.ts` around lines 159 - 163, The loader metric lruEviction
(meter counter "loader_cache_eviction") is declared but never incremented; wire
it to the real eviction hook by incrementing that counter when an eviction
happens in the LRU implementation (the lru_cache_eviction signal in
runtime/caches/lrucache.ts). Locate the eviction path in
runtime/caches/lrucache.ts (the function/method that emits or handles
lru_cache_eviction) and call or expose a handler that increments
loader_cache_eviction (lruEviction.increment(...) or equivalent) so every real
eviction increments the loader metric.
| delete: async ( | ||
| request: RequestInfo | URL, | ||
| _options?: CacheQueryOptions, | ||
| ): Promise<boolean> => { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the Cache API, what is the expected behavior of Cache.delete(request, options)regardingCacheQueryOptionssuch asignoreSearchandignoreMethod?
💡 Result:
The Cache.delete(request, options) method uses the provided CacheQueryOptions (such as ignoreSearch and ignoreMethod) to control how the matching is performed when identifying the cache entry to delete. It finds the Cache entry whose key matches the request according to the matching rules modified by the options, deletes it if found (resolving to true), or resolves to false if no match is found. Specifically: - ignoreSearch: When true, ignores the query string in the URL during matching (defaults to false). - ignoreMethod: When true, does not validate the Request's HTTP method (normally only GET/HEAD allowed; defaults to false). This behavior is identical to how options work in Cache.match and Cache.matchAll.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Cache/delete
- 2: https://developer.mozilla.org/fr/docs/Web/API/Cache/delete
- 3: https://developer.mozilla.org/ja/docs/Web/API/Cache/delete
- 4: https://developer.mozilla.org/ru/docs/Web/API/Cache/delete
🏁 Script executed:
# First, let's find and examine the inMemoryCache.ts file
find . -name "inMemoryCache.ts" -type fRepository: deco-cx/deco
Length of output: 89
🏁 Script executed:
# Once found, let's examine the full delete() and match() implementations
cat -n runtime/caches/inMemoryCache.ts | head -100Repository: deco-cx/deco
Length of output: 3189
🏁 Script executed:
# Search for assertNoOptions usage in the codebase to understand the pattern
rg "assertNoOptions" --type typescriptRepository: deco-cx/deco
Length of output: 90
🏁 Script executed:
# Let's check the utils.ts file to see what assertNoOptions does
cat -n runtime/caches/utils.tsRepository: deco-cx/deco
Length of output: 4551
🏁 Script executed:
# Search for assertNoOptions usage with context
rg "assertNoOptions" -A 2 -B 2 --type tsRepository: deco-cx/deco
Length of output: 4070
Add assertion for unsupported CacheQueryOptions in delete() to match match() behavior.
The delete() method accepts CacheQueryOptions but silently ignores them (parameter has underscore prefix), while match() explicitly rejects them via assertNoOptions(). This inconsistency can hide behavior mismatches when callers pass ignoreSearch, ignoreMethod, or ignoreVary. Other cache implementations (e.g., redis.ts) already call assertNoOptions() in their delete() methods—align this implementation with that pattern.
Suggested fix
delete: async (
request: RequestInfo | URL,
- _options?: CacheQueryOptions,
+ options?: CacheQueryOptions,
): Promise<boolean> => {
+ assertNoOptions(options);
const cacheKey = await requestURLSHA1(request);
return store.delete(cacheKey);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete: async ( | |
| request: RequestInfo | URL, | |
| _options?: CacheQueryOptions, | |
| ): Promise<boolean> => { | |
| delete: async ( | |
| request: RequestInfo | URL, | |
| options?: CacheQueryOptions, | |
| ): Promise<boolean> => { | |
| assertNoOptions(options); | |
| const cacheKey = await requestURLSHA1(request); | |
| return store.delete(cacheKey); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/inMemoryCache.ts` around lines 46 - 49, The delete() method in
inMemoryCache.ts currently ignores its CacheQueryOptions parameter (named
_options) unlike match() which calls assertNoOptions(); update delete to
assertNoOptions on the options to reject unsupported flags. Inside the async
delete implementation (function named delete), call assertNoOptions(_options,
'delete') at the start (or rename to options and call assertNoOptions(options,
'delete')) so it behaves like redis.ts and match() and consistently rejects
ignoreSearch/ignoreMethod/ignoreVary.
| if (!response.body) return; | ||
| const cacheKey = await requestURLSHA1(request); | ||
| const body = new Uint8Array(await response.arrayBuffer()); | ||
| const headers: [string, string][] = [...response.headers.entries()]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Fetch API, does Response.arrayBuffer() return an empty ArrayBuffer when the response body is null, and is caching such responses valid behavior?
💡 Result:
Yes, Response.arrayBuffer returns an empty ArrayBuffer (with byteLength 0) when the response body is null. Caching such responses is valid behavior. In the Fetch API, Response.body is null for responses constructed with null body (e.g., new Response(null)) or per HTTP semantics for no-body statuses like 204 No Content, 205 Reset Content, and 304 Not Modified. However, browsers do not implement the spec's null body exactly; they provide an empty ReadableStream instead. The arrayBuffer method reads the body stream to completion, resolving to an empty ArrayBuffer for null or empty bodies. This is confirmed by MDN documentation stating arrayBuffer resolves with an ArrayBuffer after reading the stream, and practical observations in issues where null bodies become empty ArrayBuffers (e.g., Safari workaround turning null to empty ArrayBuffer). The WHATWG Fetch spec links to body methods like arrayBuffer, and implementations copy the buffer source, yielding empty for no content. Caching Responses with null/empty bodies is valid and common, e.g., in Service Workers via caches.match returning cached empty-body responses (e.g., 304), as shown in WHATWG discussions and examples.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Response/arrayBuffer
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Response/body
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Response
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Response/Response
- 5: Resizable ArrayBuffers and Response and Request objects whatwg/fetch#1560
- 6: Safari
readableStreamTransferworkaround turnsnullbody into emptyArrayBuffer, requests fail mswjs/msw#1827 - 7: https://lists.w3.org/Archives/Public/public-webapps-github/2024Oct/0385.html
- 8: Body.arrayBuffer([begin, end]) whatwg/fetch#554
- 9: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
- 10: https://fetch.spec.wintercg.org/
🏁 Script executed:
# Find the file and check its structure
fd -t f "inMemoryCache.ts" runtime/caches/Repository: deco-cx/deco
Length of output: 87
🏁 Script executed:
# View the specific section around lines 72-75
cat -n runtime/caches/inMemoryCache.ts | sed -n '60,90p'Repository: deco-cx/deco
Length of output: 1077
🏁 Script executed:
# Find where assertCanBeCached is defined and what it does
rg "assertCanBeCached" -A 5Repository: deco-cx/deco
Length of output: 3531
🏁 Script executed:
# Check if there are any tests related to empty body caching
rg -i "empty.*body|body.*empty|arrayBuffer.*null" --type ts --type js -A 3Repository: deco-cx/deco
Length of output: 224
🏁 Script executed:
# Look for any usage of the cache to understand if empty responses are expected
rg "store\.get|store\.set" runtime/caches/ -B 2 -A 2Repository: deco-cx/deco
Length of output: 868
🏁 Script executed:
# Look for tests related to caching empty bodies or null bodies
find . -type f \( -name "*test*" -o -name "*.test.*" -o -name "*.spec.*" \) | xargs rg -i "cache.*empty|empty.*cache|null.*body|body.*null" 2>/dev/null | head -30Repository: deco-cx/deco
Length of output: 243
🏁 Script executed:
# Check if there are any comments explaining why the guard exists
rg -B 3 "if.*!response\.body" runtime/caches/Repository: deco-cx/deco
Length of output: 1267
🏁 Script executed:
# Look for any documentation or comments in the utils file about caching rules
cat -n runtime/caches/utils.ts | head -50Repository: deco-cx/deco
Length of output: 1591
Remove the early return guard that prevents caching of responses with null/empty bodies.
Line 72 returns early when response.body is null, preventing valid responses from being cached. Per the Fetch API, Response.arrayBuffer() returns an empty ArrayBuffer for null bodies, and such responses are cacheable (e.g., 304 Not Modified, 204 No Content). This pattern exists across all cache implementations (inMemoryCache, redis, fileSystem, headerscache, lrucache) and should be removed from all of them.
Suggested fix
const req = new Request(request);
assertCanBeCached(req, response);
- if (!response.body) return;
const cacheKey = await requestURLSHA1(request);
const body = new Uint8Array(await response.arrayBuffer());
const headers: [string, string][] = [...response.headers.entries()];
store.set(cacheKey, { body, headers, status: response.status });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!response.body) return; | |
| const cacheKey = await requestURLSHA1(request); | |
| const body = new Uint8Array(await response.arrayBuffer()); | |
| const headers: [string, string][] = [...response.headers.entries()]; | |
| const req = new Request(request); | |
| assertCanBeCached(req, response); | |
| const cacheKey = await requestURLSHA1(request); | |
| const body = new Uint8Array(await response.arrayBuffer()); | |
| const headers: [string, string][] = [...response.headers.entries()]; | |
| store.set(cacheKey, { body, headers, status: response.status }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/inMemoryCache.ts` around lines 72 - 75, Remove the early
return that checks response.body (the line "if (!response.body) return;") so
responses with null/empty bodies are still cached; keep using
requestURLSHA1(request) for the cache key and continue to read the body via
response.arrayBuffer() (which yields an empty ArrayBuffer for null bodies) and
collect headers via [...response.headers.entries()] as before; apply the same
removal of the guard across inMemoryCache, redis, fileSystem, headerscache, and
lrucache implementations to ensure 204/304 and other empty-body responses are
cached.
runtime/caches/lrucache.ts
Outdated
| // deno-lint-ignore no-explicit-any | ||
| const activeCaches: LRUCache<string, any>[] = []; |
There was a problem hiding this comment.
Memory leak: caches accumulate in activeCaches without cleanup.
Caches are pushed to the module-level activeCaches array but never removed. Per mod.ts (context snippet 2), cache instances are created at module load time. In hot-reload or reconfiguration scenarios, new LRUCache instances are added while old ones remain, causing:
- Memory leaks (old caches not GC'd)
- Stale/inaccurate gauge metrics (reporting defunct cache sizes)
Consider using a WeakRef or a Set with explicit cleanup, or ensuring caches are singletons.
Suggested approach using WeakRef
-// deno-lint-ignore no-explicit-any
-const activeCaches: LRUCache<string, any>[] = [];
+// deno-lint-ignore no-explicit-any
+const activeCaches: WeakRef<LRUCache<string, any>>[] = [];
lruSizeGauge.addCallback((observer) => {
- for (const cache of activeCaches) {
- observer.observe(cache.size);
+ for (const ref of activeCaches) {
+ const cache = ref.deref();
+ if (cache) observer.observe(cache.size);
}
});
lruBytesGauge.addCallback((observer) => {
- for (const cache of activeCaches) {
- observer.observe(cache.calculatedSize);
+ for (const ref of activeCaches) {
+ const cache = ref.deref();
+ if (cache) observer.observe(cache.calculatedSize);
}
});And at line 79:
- activeCaches.push(fileCache);
+ activeCaches.push(new WeakRef(fileCache));Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/lrucache.ts` around lines 59 - 60, The module-level
activeCaches array currently holds strong references to every LRUCache<string,
any> instance causing leaks; change the registration so activeCaches stores
WeakRef<LRUCache<string, any>> (or use a Set and remove old instances) and
update any code that iterates activeCaches (e.g., the cache registration/push
site and the gauge reporting logic) to dereference the WeakRef, skip collected
caches, and prune dead refs from the collection; alternatively implement a
singleton getter for LRUCache instances to avoid multiple creations and ensure
metrics read only live caches.
…U tracking, and tiered headers - Use `??` default instead of `||` for CACHE_MAX_ENTRY_SIZE, MEMORY_CACHE_MAX_SIZE, and MEMORY_CACHE_MAX_ITEMS so that explicit zero values are respected - Only mark a shard directory as initialized after mkdir succeeds to prevent transient failures permanently breaking writes for that shard - Replace activeCaches array with Map so repeated open() calls for the same cache name overwrite rather than accumulate, fixing the metrics/memory leak - Stop mutating matched.headers directly in tiered cache match; instead track the matched tier index and return a freshly constructed Response whose headers are guaranteed mutable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (7)
blocks/loader.ts (2)
368-378:⚠️ Potential issue | 🟡 Minor
loader_bg_revalidationmay record multiple samples for a single revalidation.
bgFlightsis asingleFlight()instance that returns the same promise to all concurrent callers with the same key. Each caller attaches its own.finally()callback, so when the shared promise settles, all callbacks execute—resulting in N histogram samples for one actual revalidation.Move the timer and recording inside the function passed to
bgFlights.do()to ensure the metric is recorded exactly once.📏 Proposed fix
- const bgStart = performance.now(); - bgFlights.do(request.url, callHandlerAndCache) - .catch((error) => logger.error(`loader error ${error}`)) - .finally(() => { - if (OTEL_ENABLE_EXTRA_METRICS) { - stats.bgRevalidation.record( - performance.now() - bgStart, - { loader }, - ); - } - }); + bgFlights.do(request.url, async () => { + const bgStart = performance.now(); + try { + return await callHandlerAndCache(); + } finally { + if (OTEL_ENABLE_EXTRA_METRICS) { + stats.bgRevalidation.record( + performance.now() - bgStart, + { loader }, + ); + } + } + }) + .catch((error) => logger.error(`loader error ${error}`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` around lines 368 - 378, The current code measures bgStart and records stats.bgRevalidation in each caller's .finally(), causing multiple samples because bgFlights.do(...) returns the same promise to concurrent callers; instead, move the timing and the stats.bgRevalidation.record call into the actual function passed to bgFlights.do so it runs once per singleFlight execution (i.e., start timer inside the function that calls callHandlerAndCache and record in that function's finally), keeping checks for OTEL_ENABLE_EXTRA_METRICS and referencing bgFlights.do, callHandlerAndCache, loader, and stats.bgRevalidation.
320-323:⚠️ Potential issue | 🟠 MajorOversize revalidations leave stale entries pinned.
When
callHandlerAndCache()is invoked for background revalidation and the fresh payload exceedsCACHE_MAX_ENTRY_SIZE, the early return skips the new write but leaves the expired cache entry in place. Subsequent requests continue serving stale data and retrying the same failed revalidation indefinitely.Delete the existing cache key before returning when the fresh payload is no longer cacheable.
🐛 Proposed fix
// Skip caching oversized entries to protect disk and memory if (jsonStringEncoded.length > CACHE_MAX_ENTRY_SIZE) { + cache.delete(request).catch((error) => + logger.error(`loader cache delete error ${error}`)); return json; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` around lines 320 - 323, In callHandlerAndCache(), when jsonStringEncoded.length > CACHE_MAX_ENTRY_SIZE currently returns early (leaving a stale expired entry), delete the existing cache entry for the same cache key before returning: locate the cache key variable used in the function (the key/cacheKey used for writes) and call the module's cache delete/remove method (await the deletion and handle/log errors) so oversized revalidation results don't leave the old entry pinned; reference symbols: callHandlerAndCache, jsonStringEncoded, CACHE_MAX_ENTRY_SIZE, and the cache key variable.runtime/caches/inMemoryCache.ts (3)
46-49:⚠️ Potential issue | 🟡 Minor
delete()should reject unsupportedCacheQueryOptions
match()callsassertNoOptions, butdelete()currently ignores options silently. Keep behavior consistent and explicit.Suggested fix
delete: async ( request: RequestInfo | URL, - _options?: CacheQueryOptions, + options?: CacheQueryOptions, ): Promise<boolean> => { + assertNoOptions(options); const cacheKey = await requestURLSHA1(request); return store.delete(cacheKey); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/inMemoryCache.ts` around lines 46 - 49, The delete method in inMemoryCache.ts currently ignores the _options parameter while match() calls assertNoOptions; update the delete implementation (the delete: async (...) => Promise<boolean> function) to call assertNoOptions(_options) or otherwise reject/throw when any CacheQueryOptions are provided so behavior is consistent with match(), ensuring unsupported options are not silently ignored.
9-14:⚠️ Potential issue | 🟠 MajorEnv parsing still allows invalid negative limits
Using
parseInt(...) || fallbackdoes not reject negatives (e.g.,-1), somax/maxSizecan still be invalid at runtime.Suggested fix
+const parsePositiveInt = (raw: string | undefined, fallback: number): number => { + const parsed = Number.parseInt(raw ?? "", 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +}; + -const MEMORY_CACHE_MAX_SIZE = parseInt( - Deno.env.get("MEMORY_CACHE_MAX_SIZE") ?? "268435456", // 256 MB -) || 268435456; -const MEMORY_CACHE_MAX_ITEMS = parseInt( - Deno.env.get("MEMORY_CACHE_MAX_ITEMS") ?? "2048", -) || 2048; +const MEMORY_CACHE_MAX_SIZE = parsePositiveInt( + Deno.env.get("MEMORY_CACHE_MAX_SIZE"), + 268435456, // 256 MB +); +const MEMORY_CACHE_MAX_ITEMS = parsePositiveInt( + Deno.env.get("MEMORY_CACHE_MAX_ITEMS"), + 2048, +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/inMemoryCache.ts` around lines 9 - 14, The current parsing for MEMORY_CACHE_MAX_SIZE and MEMORY_CACHE_MAX_ITEMS uses parseInt(...) || fallback which still accepts negative values (e.g., "-1"); update the logic in the MEMORY_CACHE_MAX_SIZE and MEMORY_CACHE_MAX_ITEMS initialization to explicitly validate the parsed number is a finite non-negative integer (and optionally >0 for size), otherwise use the fallback; reference the constants MEMORY_CACHE_MAX_SIZE and MEMORY_CACHE_MAX_ITEMS and the parseInt calls so you replace the parseInt(...) || fallback pattern with a parse + isFinite/Number.isInteger and >=0 check before assigning the final value.
72-72:⚠️ Potential issue | 🟠 MajorDo not skip caching valid empty-body responses
The guard at Line 72 drops cacheable empty-body responses (e.g., 204/304), which breaks tier consistency and hit behavior.
Suggested fix
- if (!response.body) return; const cacheKey = await requestURLSHA1(request); const body = new Uint8Array(await response.arrayBuffer());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/inMemoryCache.ts` at line 72, The current guard "if (!response.body) return;" in runtime/caches/inMemoryCache.ts incorrectly skips caching valid empty-body responses (e.g., 204/304); remove that early return and instead treat an absent body as a valid empty body when storing cache entries, i.e., allow caching when response.status and cache headers indicate cacheability and store a canonical empty payload (empty string or Buffer) so downstream retrieval/hits remain consistent; update any code paths that assume response.body is non-null to handle the empty payload.runtime/caches/lrucache.ts (2)
59-60:⚠️ Potential issue | 🟠 Major
activeCacheskeeps strong references and grows without cleanupEvery
open()can register a new LRU instance (runtime/caches/utils.ts, Line 73-Line 82). Keeping strong refs inactiveCachescauses long-lived memory growth and stale gauge reporting.Suggested fix
-// deno-lint-ignore no-explicit-any -const activeCaches: { name: string; lru: LRUCache<string, any> }[] = []; +const activeCaches: { name: string; lru: WeakRef<LRUCache<string, boolean>> }[] = []; lruSizeGauge.addCallback((observer) => { - for (const { name, lru } of activeCaches) { - observer.observe(lru.size, { cache: name }); + for (let i = activeCaches.length - 1; i >= 0; i--) { + const { name, lru } = activeCaches[i]; + const live = lru.deref(); + if (!live) { + activeCaches.splice(i, 1); + continue; + } + observer.observe(live.size, { cache: name }); } }); lruBytesGauge.addCallback((observer) => { - for (const { name, lru } of activeCaches) { - observer.observe(lru.calculatedSize, { cache: name }); + for (let i = activeCaches.length - 1; i >= 0; i--) { + const { name, lru } = activeCaches[i]; + const live = lru.deref(); + if (!live) { + activeCaches.splice(i, 1); + continue; + } + observer.observe(live.calculatedSize, { cache: name }); } }); @@ - activeCaches.push({ name: _cacheName, lru: fileCache }); + activeCaches.push({ name: _cacheName, lru: new WeakRef(fileCache) });Also applies to: 62-71, 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/lrucache.ts` around lines 59 - 60, activeCaches currently holds strong references to every LRUCache instance (const activeCaches and registrations created by open()), causing unbounded memory growth and stale metrics; change the registry to hold weak references (e.g., store WeakRef<LRUCache<string, any>> or use a WeakMap keyed by the LRU instance) and ensure caches are removed from reporting when closed (update the open() registration code in runtime/caches/utils.ts and any gauge-reporting code to dereference WeakRef before use and skip cleaned-up entries); optionally use FinalizationRegistry to decrement metrics when an LRU is collected and provide a deterministic unregister path from the cache close method to remove the entry from the registry.
40-43:⚠️ Potential issue | 🟠 Major
disposeshould not return an unhandled Promise
lru-cachedoes not awaitdispose; a rejectedcache.delete(key)can surface as an unhandled rejection. Make the callback sync and swallow/log async failures.Suggested fix
- dispose: async (_value: boolean, key: string, reason: string) => { + dispose: (_value: boolean, key: string, reason: string) => { lruEvictionCounter.add(1, { reason }); - await cache.delete(key); + cache.delete(key).catch(() => {}); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/lrucache.ts` around lines 40 - 43, The dispose callback in lrucache.ts (dispose: async (_value: boolean, key: string, reason: string) => { ... }) must be made synchronous because lru-cache doesn't await it; change dispose to a sync function that calls cache.delete(key) without returning its Promise and handle errors via a fire-and-forget async call (e.g., call an inner async function or use .catch) so any rejection is swallowed or logged using lruEvictionCounter and a logger; ensure you still increment lruEvictionCounter.add(1, { reason }) and reference cache.delete(key) and the reason/key parameters to locate the code.
🧹 Nitpick comments (1)
blocks/loader.ts (1)
393-401: Consider using a dedicated histogram for JSON parse timing.Recording
json_parselatency viastats.latency(theresolver_latencyhistogram) mixes parse timing with actual resolver latencies, which could lead to dashboard confusion since they represent fundamentally different operations. A dedicated histogram likestats.jsonParseLatencywould provide cleaner metric separation.Alternatively, if intentional for correlation purposes, document this choice in the metric description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` around lines 393 - 401, The JSON parse timing is being recorded into the general resolver histogram via stats.latency.record when OTEL_ENABLE_EXTRA_METRICS is true; instead create and use a dedicated histogram (e.g., stats.jsonParseLatency) and record the JSON parse duration there (use performance.now() around matched.json() and pass the same attributes like loader and status:"json_parse"), or if you intentionally want to correlate with resolver latency update the metric description where stats.latency is defined to explicitly mention it includes JSON parse time; modify the usage around matched.json(), the OTEL_ENABLE_EXTRA_METRICS block, and any metric initialization to add stats.jsonParseLatency and switch the record call from stats.latency.record(...) to stats.jsonParseLatency.record(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@blocks/loader.ts`:
- Around line 368-378: The current code measures bgStart and records
stats.bgRevalidation in each caller's .finally(), causing multiple samples
because bgFlights.do(...) returns the same promise to concurrent callers;
instead, move the timing and the stats.bgRevalidation.record call into the
actual function passed to bgFlights.do so it runs once per singleFlight
execution (i.e., start timer inside the function that calls callHandlerAndCache
and record in that function's finally), keeping checks for
OTEL_ENABLE_EXTRA_METRICS and referencing bgFlights.do, callHandlerAndCache,
loader, and stats.bgRevalidation.
- Around line 320-323: In callHandlerAndCache(), when jsonStringEncoded.length >
CACHE_MAX_ENTRY_SIZE currently returns early (leaving a stale expired entry),
delete the existing cache entry for the same cache key before returning: locate
the cache key variable used in the function (the key/cacheKey used for writes)
and call the module's cache delete/remove method (await the deletion and
handle/log errors) so oversized revalidation results don't leave the old entry
pinned; reference symbols: callHandlerAndCache, jsonStringEncoded,
CACHE_MAX_ENTRY_SIZE, and the cache key variable.
In `@runtime/caches/inMemoryCache.ts`:
- Around line 46-49: The delete method in inMemoryCache.ts currently ignores the
_options parameter while match() calls assertNoOptions; update the delete
implementation (the delete: async (...) => Promise<boolean> function) to call
assertNoOptions(_options) or otherwise reject/throw when any CacheQueryOptions
are provided so behavior is consistent with match(), ensuring unsupported
options are not silently ignored.
- Around line 9-14: The current parsing for MEMORY_CACHE_MAX_SIZE and
MEMORY_CACHE_MAX_ITEMS uses parseInt(...) || fallback which still accepts
negative values (e.g., "-1"); update the logic in the MEMORY_CACHE_MAX_SIZE and
MEMORY_CACHE_MAX_ITEMS initialization to explicitly validate the parsed number
is a finite non-negative integer (and optionally >0 for size), otherwise use the
fallback; reference the constants MEMORY_CACHE_MAX_SIZE and
MEMORY_CACHE_MAX_ITEMS and the parseInt calls so you replace the parseInt(...)
|| fallback pattern with a parse + isFinite/Number.isInteger and >=0 check
before assigning the final value.
- Line 72: The current guard "if (!response.body) return;" in
runtime/caches/inMemoryCache.ts incorrectly skips caching valid empty-body
responses (e.g., 204/304); remove that early return and instead treat an absent
body as a valid empty body when storing cache entries, i.e., allow caching when
response.status and cache headers indicate cacheability and store a canonical
empty payload (empty string or Buffer) so downstream retrieval/hits remain
consistent; update any code paths that assume response.body is non-null to
handle the empty payload.
In `@runtime/caches/lrucache.ts`:
- Around line 59-60: activeCaches currently holds strong references to every
LRUCache instance (const activeCaches and registrations created by open()),
causing unbounded memory growth and stale metrics; change the registry to hold
weak references (e.g., store WeakRef<LRUCache<string, any>> or use a WeakMap
keyed by the LRU instance) and ensure caches are removed from reporting when
closed (update the open() registration code in runtime/caches/utils.ts and any
gauge-reporting code to dereference WeakRef before use and skip cleaned-up
entries); optionally use FinalizationRegistry to decrement metrics when an LRU
is collected and provide a deterministic unregister path from the cache close
method to remove the entry from the registry.
- Around line 40-43: The dispose callback in lrucache.ts (dispose: async
(_value: boolean, key: string, reason: string) => { ... }) must be made
synchronous because lru-cache doesn't await it; change dispose to a sync
function that calls cache.delete(key) without returning its Promise and handle
errors via a fire-and-forget async call (e.g., call an inner async function or
use .catch) so any rejection is swallowed or logged using lruEvictionCounter and
a logger; ensure you still increment lruEvictionCounter.add(1, { reason }) and
reference cache.delete(key) and the reason/key parameters to locate the code.
---
Nitpick comments:
In `@blocks/loader.ts`:
- Around line 393-401: The JSON parse timing is being recorded into the general
resolver histogram via stats.latency.record when OTEL_ENABLE_EXTRA_METRICS is
true; instead create and use a dedicated histogram (e.g.,
stats.jsonParseLatency) and record the JSON parse duration there (use
performance.now() around matched.json() and pass the same attributes like loader
and status:"json_parse"), or if you intentionally want to correlate with
resolver latency update the metric description where stats.latency is defined to
explicitly mention it includes JSON parse time; modify the usage around
matched.json(), the OTEL_ENABLE_EXTRA_METRICS block, and any metric
initialization to add stats.jsonParseLatency and switch the record call from
stats.latency.record(...) to stats.jsonParseLatency.record(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9d5b230-18a7-4a40-8e34-553b53415159
📒 Files selected for processing (5)
blocks/loader.tsruntime/caches/common.tsruntime/caches/fileSystem.tsruntime/caches/inMemoryCache.tsruntime/caches/lrucache.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- runtime/caches/fileSystem.ts
- runtime/caches/common.ts
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:16">
P1: Missing fallback after `parseInt` allows `CACHE_MAX_ENTRY_SIZE` to become `NaN`, which disables the oversized-entry guard and can let large cache files persist indefinitely.</violation>
</file>
<file name="runtime/caches/inMemoryCache.ts">
<violation number="1" location="runtime/caches/inMemoryCache.ts:11">
P1: Missing validation/fallback for numeric env vars can propagate `NaN` into LRU cache limits.</violation>
</file>
<file name="runtime/caches/tiered.ts">
<violation number="1" location="runtime/caches/tiered.ts:120">
P2: Avoid eagerly reading `matched.arrayBuffer()` on every hit; only materialize the body when backfilling tiers is actually required.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
runtime/caches/fileSystem.ts (1)
120-122:⚠️ Potential issue | 🟡 MinorAsync function calls sync method:
Deno.mkdirSyncinside asyncassertCacheDirectory.Line 121 calls
Deno.mkdirSync(synchronous) within anasyncfunction. This blocks the event loop. Use the async variant for consistency:Suggested fix
- await Deno.mkdirSync(FILE_SYSTEM_CACHE_DIRECTORY, { recursive: true }); + await Deno.mkdir(FILE_SYSTEM_CACHE_DIRECTORY, { recursive: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/fileSystem.ts` around lines 120 - 122, The async function assertCacheDirectory is calling the synchronous Deno.mkdirSync which blocks the event loop; replace the sync call with the async Deno.mkdir and await it (using the same { recursive: true } options) so the directory creation is non-blocking. Locate the call to Deno.mkdirSync inside assertCacheDirectory and change it to await Deno.mkdir(FILE_SYSTEM_CACHE_DIRECTORY, { recursive: true }) while keeping the surrounding async flow and error handling intact.runtime/caches/tiered.ts (1)
89-91:⚠️ Potential issue | 🟡 MinorSingle-cache path bypasses tier header tagging.
When
openedCaches.length === 1, the response is returned directly without theX-Cache-Tierheader. This creates inconsistent behavior—consumers relying on the header (e.g.,common.tswhich extracts and recordscache_tierin spans) won't see it for single-tier deployments.Consider tagging even single-tier responses for consistency:
Suggested fix
if (openedCaches.length === 1) { - return openedCaches[0].match(request, options); + const resp = await openedCaches[0].match(request, options); + if (resp) { + const body = await resp.arrayBuffer(); + const tagged = responseFromBody(body, resp.headers, resp.status); + tagged.headers.set(TIERED_CACHE_HEADER, "0"); + return tagged; + } + return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/tiered.ts` around lines 89 - 91, The single-cache fast-path in the tiered cache (the branch that returns openedCaches[0].match(request, options) when openedCaches.length === 1) skips adding the X-Cache-Tier header; change this to perform the match, then attach the X-Cache-Tier header to the Response (same header value format used elsewhere in this module) before returning so single-tier responses are tagged consistently; locate the openedCaches fast-path in the match method and replace the direct return with code that awaits/openedCaches[0].match(...), sets the header on the resulting Response, and then returns it.
🧹 Nitpick comments (1)
runtime/caches/tiered.ts (1)
126-133: Silent backfill failures may hide issues.The backfill
Promise.all(...).catch(() => {})silently swallows all errors. Consider logging failures at debug level for diagnosability, especially during rollout of the tiered cache:Optional: Add debug logging
Promise.all( indexOfCachesToUpdate.map((index) => openedCaches[index].put( request, responseFromBody(body, headers, status), ) ), - ).catch(() => {}); + ).catch((err) => { + // Backfill is best-effort; log for diagnosability + console.debug("[tiered-cache] backfill failed:", err); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/tiered.ts` around lines 126 - 133, The backfill Promise.all call currently swallows all errors; change the .catch(() => {}) to capture the error and log it at debug level (e.g., .catch(err => logger?.debug(... ) || console.debug(...))). Include contextual details such as indexOfCachesToUpdate, the request (or request.url), and which openedCaches were targeted, and reference the responseFromBody/result to make failures diagnosable while keeping behavior non-fatal in the tiered cache backfill code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/caches/fileSystem.ts`:
- Around line 14-16: putFile currently writes entries without checking
CACHE_MAX_ENTRY_SIZE so oversized blobs are written then deleted on read; add a
pre-write size validation in putFile to compare the byte length of the data
against CACHE_MAX_ENTRY_SIZE and reject/return early for oversized entries (do
not call Deno.writeFile), and mirror the same check inside the
cacheWriteWorker's write-handling path (the code that ultimately calls
Deno.writeFile in cacheWriteWorker.ts) so the worker rejects the job before
performing any I/O; ensure both places surface a clear rejection (throw/return
error or skip enqueue) and avoid writing files that exceed CACHE_MAX_ENTRY_SIZE.
---
Outside diff comments:
In `@runtime/caches/fileSystem.ts`:
- Around line 120-122: The async function assertCacheDirectory is calling the
synchronous Deno.mkdirSync which blocks the event loop; replace the sync call
with the async Deno.mkdir and await it (using the same { recursive: true }
options) so the directory creation is non-blocking. Locate the call to
Deno.mkdirSync inside assertCacheDirectory and change it to await
Deno.mkdir(FILE_SYSTEM_CACHE_DIRECTORY, { recursive: true }) while keeping the
surrounding async flow and error handling intact.
In `@runtime/caches/tiered.ts`:
- Around line 89-91: The single-cache fast-path in the tiered cache (the branch
that returns openedCaches[0].match(request, options) when openedCaches.length
=== 1) skips adding the X-Cache-Tier header; change this to perform the match,
then attach the X-Cache-Tier header to the Response (same header value format
used elsewhere in this module) before returning so single-tier responses are
tagged consistently; locate the openedCaches fast-path in the match method and
replace the direct return with code that awaits/openedCaches[0].match(...), sets
the header on the resulting Response, and then returns it.
---
Nitpick comments:
In `@runtime/caches/tiered.ts`:
- Around line 126-133: The backfill Promise.all call currently swallows all
errors; change the .catch(() => {}) to capture the error and log it at debug
level (e.g., .catch(err => logger?.debug(... ) || console.debug(...))). Include
contextual details such as indexOfCachesToUpdate, the request (or request.url),
and which openedCaches were targeted, and reference the responseFromBody/result
to make failures diagnosable while keeping behavior non-fatal in the tiered
cache backfill code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f8c71d5-7a4b-4b06-ba05-2561bc46f99c
📒 Files selected for processing (4)
runtime/caches/fileSystem.tsruntime/caches/inMemoryCache.tsruntime/caches/lrucache.tsruntime/caches/tiered.ts
runtime/caches/fileSystem.ts
Outdated
| const CACHE_MAX_ENTRY_SIZE = parseInt( | ||
| Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152", // 2 MB | ||
| ); |
There was a problem hiding this comment.
Inconsistent size validation: putFile bypasses the size limit.
The CACHE_MAX_ENTRY_SIZE is only enforced on reads (line 157-159), not on writes. Both the inline putFile path and the worker path (see cacheWriteWorker.ts lines 78-102) write entries to disk without size validation. This causes wasted I/O: oversized responses are written, then silently deleted on the first read.
Consider validating size in putFile and rejecting oversized entries before the write:
Suggested fix for inline path
async function putFile(
key: string,
responseArray: Uint8Array,
) {
+ if (responseArray.length > CACHE_MAX_ENTRY_SIZE) {
+ return; // Skip caching oversized entries
+ }
if (!isCacheInitialized) {
await assertCacheDirectory();
}The worker path in cacheWriteWorker.ts should also include this validation before calling Deno.writeFile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/fileSystem.ts` around lines 14 - 16, putFile currently writes
entries without checking CACHE_MAX_ENTRY_SIZE so oversized blobs are written
then deleted on read; add a pre-write size validation in putFile to compare the
byte length of the data against CACHE_MAX_ENTRY_SIZE and reject/return early for
oversized entries (do not call Deno.writeFile), and mirror the same check inside
the cacheWriteWorker's write-handling path (the code that ultimately calls
Deno.writeFile in cacheWriteWorker.ts) so the worker rejects the job before
performing any I/O; ensure both places surface a clear rejection (throw/return
error or skip enqueue) and avoid writing files that exceed CACHE_MAX_ENTRY_SIZE.
On a cold LRU (e.g. after pod restart), cache files on disk were orphaned — never read and never evicted, causing unbounded disk growth. Now, on an LRU miss, we fall back to checking disk. Valid entries are re-indexed into the LRU (which then manages eviction/deletion normally). Expired entries are deleted on the spot as we encounter them. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/lrucache.ts">
<violation number="1" location="runtime/caches/lrucache.ts:117">
P1: Do not fire-and-forget cache deletion here; it can race with a subsequent `put()` and delete a newly written entry for the same key.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
| // Expired or missing metadata — delete the orphaned file and treat as a miss. | ||
| cacheInner.delete(cacheKey).catch(() => {}); |
There was a problem hiding this comment.
P1: Do not fire-and-forget cache deletion here; it can race with a subsequent put() and delete a newly written entry for the same key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/lrucache.ts, line 117:
<comment>Do not fire-and-forget cache deletion here; it can race with a subsequent `put()` and delete a newly written entry for the same key.</comment>
<file context>
@@ -94,9 +94,27 @@ function createLruCacheStorage(cacheStorageInner: CacheStorage): CacheStorage {
+ }
+ }
+ // Expired or missing metadata — delete the orphaned file and treat as a miss.
+ cacheInner.delete(cacheKey).catch(() => {});
return undefined;
},
</file context>
tiered.ts: - Skip arrayBuffer() on L1 hits with no backfill needed — the dominant path in production. Body reads now only happen when fanning out to lower tiers, cutting per-hit allocation pressure significantly. - Replace local inFuture (2x Date allocations) with the imported version from utils.ts that uses Date.parse + Date.now() (zero allocations). loader.ts: - Evict existing stale entry when a fresh payload exceeds CACHE_MAX_ENTRY_SIZE, preventing the stale value from being pinned and served indefinitely. - Move bgRevalidation timer inside the singleFlight function so the metric records exactly once per revalidation instead of once per concurrent waiter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fileSystem.ts: replace blocking Deno.mkdirSync with async Deno.mkdir so cache directory init doesn't stall the event loop. tiered.ts: tag single-tier responses with X-Cache-Tier header using direct header mutation (no body copy) to keep the hot path allocation-free. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously CACHE_MAX_ENTRY_SIZE was only enforced on read (delete after the fact), meaning every oversized entry paid a full write + future delete. Now both the inline putFile path and the write worker bail out early if the combined buffer exceeds the limit, avoiding the wasted I/O entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/caches/lrucache.ts`:
- Around line 107-118: The current logic deletes cached backing files when
either expires or content-length is missing; instead, only delete when expires
exists and computed ttl <= 0 (i.e., truly expired). Change the branch in the
block that reads response.headers.get("expires") and "content-length": if both
expires and length exist compute ttl and if ttl > 0 call fileCache.set(cacheKey,
true, { size: parseInt(length), ttl }) and return response; if expires exists
but ttl <= 0 then call cacheInner.delete(cacheKey).catch(() => {}) and return
undefined; otherwise (missing expires or length) simply return response and do
not call cacheInner.delete so the backing entry is preserved but not re-indexed.
- Around line 40-43: The dispose callback currently always deletes the backing
store (lruEvictionCounter.add and await cache.delete) for every disposal reason,
causing races on overwrites from put() (which calls fileCache.set() then writes
to cacheInner). Change the behavior so only true evictions remove the backing
entry: either set noDisposeOnSet: true in the LRU options where fileCache is
constructed, or update dispose(_: boolean, key: string, reason: string) to
early-return unless reason === 'evict' || reason === 'expire' (and only then
call lruEvictionCounter.add and await cache.delete(key)); reference symbols:
dispose, fileCache.set, put(), cacheInner, lruEvictionCounter, cache.delete.
- Around line 96-98: The code uses fileCache.has(cacheKey) which does not bump
LRU recency; replace that check with fileCache.get(cacheKey) so reads move the
entry to MRU. After calling cacheInner.match(cacheKey), if match returns
undefined remove the stale LRU marker (e.g., fileCache.delete(cacheKey)) and
adjust any bytes/size accounting (the same counters updated on
deletions/evictions) so the LRU metadata stays consistent; update the logic
around cacheKey, fileCache.get, cacheInner.match, and the removal/bytes
accounting paths accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b4df0f0-fabb-4d83-a853-c60d54637405
📒 Files selected for processing (1)
runtime/caches/lrucache.ts
| dispose: async (_value: boolean, key: string, reason: string) => { | ||
| lruEvictionCounter.add(1, { reason }); | ||
| await cache.delete(key); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "lrucache.ts" | head -5Repository: deco-cx/deco
Length of output: 84
🏁 Script executed:
cat -n runtime/caches/lrucache.tsRepository: deco-cx/deco
Length of output: 7197
🏁 Script executed:
npm info lru-cache@10.2.0 descriptionRepository: deco-cx/deco
Length of output: 114
🏁 Script executed:
# Search for lru-cache documentation or examples in the codebase
rg -i "dispose|noDisposeOnSet" --type ts --type jsRepository: deco-cx/deco
Length of output: 3348
🌐 Web query:
lru-cache npm@10.2.0 dispose callback reason eviction set overwrites
💡 Result:
In lru-cache version 10.2.0, the dispose callback is provided as an option when creating an LRUCache instance: dispose: (value, key, reason) => {}. It is called whenever an item is removed from the cache, including on eviction (when the cache is full and the least recently used item is dropped), overwrites (when cache.set is called on an existing key), explicit deletes, TTL expiration, or fetch failures. The 'reason' parameter is a string from the DisposeReason enum with these values: - 'evict': Item evicted due to cache being full (LRU eviction). - 'set': Item overwritten by a new value via cache.set (this covers "set overwrites"). - 'delete': Explicit deletion via cache.delete, cache.clear, or cache.set(key, undefined). - 'expire': Item removed due to TTL expiration. - 'fetch': Item deleted due to fetchMethod returning undefined or aborting. There is no 'overwrite' reason; overwrites use 'set'. The feature has been stable since v7.2 when reasons were added to dispose (v10.2.0 inherits this). Use noDisposeOnSet: true to suppress dispose calls specifically on overwrites. The callback runs before the item is fully removed from the cache.
Citations:
- 1: https://www.npmjs.com/package/lru-cache
- 2: https://www.npmjs.com/package/lru-cache/v/10.2.1
- 3: https://isaacs.github.io/node-lru-cache/types/LRUCache.Disposer.html
- 4: https://isaacs.github.io/node-lru-cache/
- 5: https://github.com/isaacs/node-lru-cache/blob/main/src/index.ts
- 6: https://github.com/isaacs/node-lru-cache/blob/HEAD/CHANGELOG.md
- 7: http://isaacs.github.io/node-lru-cache/interfaces/LRUCache.OptionsBase.html
- 8: https://isaacs.github.io/node-lru-cache/interfaces/LRUCache.OptionsBase.html
Filter dispose callback to only delete on true evictions, not overwrites.
In Lines 40-43, the dispose callback unconditionally deletes the backing store for all disposal reasons. The lru-cache library calls dispose on 'set' (overwrites), not only on actual evictions. Since put() calls fileCache.set() at Line 146 before writing to cacheInner at Line 150, a same-key refresh triggers the dispose callback with reason='set', which asynchronously deletes the backing entry while the put is still writing the new value, creating a race condition. Additionally, this counts overwrites as "evictions" in the metric. Check the reason parameter and only delete on true evictions: filter for reason === 'evict' or reason === 'expire', or set noDisposeOnSet: true in the cache options to suppress dispose on overwrites entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/lrucache.ts` around lines 40 - 43, The dispose callback
currently always deletes the backing store (lruEvictionCounter.add and await
cache.delete) for every disposal reason, causing races on overwrites from put()
(which calls fileCache.set() then writes to cacheInner). Change the behavior so
only true evictions remove the backing entry: either set noDisposeOnSet: true in
the LRU options where fileCache is constructed, or update dispose(_: boolean,
key: string, reason: string) to early-return unless reason === 'evict' || reason
=== 'expire' (and only then call lruEvictionCounter.add and await
cache.delete(key)); reference symbols: dispose, fileCache.set, put(),
cacheInner, lruEvictionCounter, cache.delete.
| if (fileCache.has(cacheKey)) { | ||
| const result = cacheInner.match(cacheKey); | ||
| return result; | ||
| return cacheInner.match(cacheKey); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "lrucache.ts" -type fRepository: deco-cx/deco
Length of output: 84
🏁 Script executed:
git ls-files | grep -E "(lrucache|cache)" | head -20Repository: deco-cx/deco
Length of output: 451
🏁 Script executed:
fd "lrucache" --type fRepository: deco-cx/deco
Length of output: 82
🏁 Script executed:
cat -n runtime/caches/lrucache.ts | head -150Repository: deco-cx/deco
Length of output: 7011
🏁 Script executed:
wc -l runtime/caches/lrucache.tsRepository: deco-cx/deco
Length of output: 86
🌐 Web query:
lru-cache npm v10.2.0 has() vs get() updateAgeOnHas documentation
💡 Result:
In lru-cache npm package version 10.2.0, the has and get methods differ in their impact on cache entry recency and age: - get(key): Returns the value if present (or undefined if not). Updates the recency of use (moves to MRU position in LRU order). If TTL is set and updateAgeOnGet is true (default false), updates the entry's age/TTL clock. - has(key): Checks if a key exists in the cache without updating recency of use (does not move to MRU). Returns false for stale entries. If updateAgeOnHas is true (default false), updates the age/TTL clock. updateAgeOnHas (and updateAgeOnGet) are options in the LRUCache constructor. When true, calling has/get resets the TTL timer for that entry, preventing expiration until the next access interval. Defaults to false for performance, as age updates add overhead. These behaviors ensure has is a lightweight existence check, while get promotes active use for eviction priority. Both treat stale TTL entries as absent unless allowStale is true.
Citations:
- 1: https://www.npmjs.com/package/lru-cache/v/10.2.0
- 2: https://www.npmjs.com/package/lru-cache
- 3: https://www.jsdocs.io/package/lru-cache
- 4: https://www.npmjs.com/package/lru-cache/v/10.2.1
- 5: https://registry.npmjs.org/lru-cache
- 6: https://isaacs.github.io/node-lru-cache/
- 7: https://github.com/isaacs/node-lru-cache/blob/HEAD/CHANGELOG.md
- 8: https://isaacs.github.io/node-lru-cache/interfaces/LRUCache.Status.html
- 9: https://github.com/isaacs/node-lru-cache
- 10: https://github.com/isaacs/node-lru-cache/blob/main/src/index.ts
Use get() here so hits actually refresh recency.
Lines 96-98 use has() to check the LRU index but never update recency. In lru-cache@10.2.0, get() updates entry recency (moving to MRU position) while has() explicitly does not, even for subsequent accesses. Since updateAgeOnHas is not enabled in cacheOptions, hot keys age like cold ones and can be evicted despite frequent use. Additionally, if cacheInner.match() returns undefined while the LRU marker exists, the stale entry remains in the LRU, skewing bytes and size accounting.
The suggested fix correctly uses get() to update recency on reads and cleans up stale markers when the underlying storage misses.
Suggested fix
- if (fileCache.has(cacheKey)) {
- return cacheInner.match(cacheKey);
+ if (fileCache.get(cacheKey)) {
+ const response = await cacheInner.match(cacheKey);
+ if (!response) {
+ fileCache.delete(cacheKey);
+ }
+ return response;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fileCache.has(cacheKey)) { | |
| const result = cacheInner.match(cacheKey); | |
| return result; | |
| return cacheInner.match(cacheKey); | |
| } | |
| if (fileCache.get(cacheKey)) { | |
| const response = await cacheInner.match(cacheKey); | |
| if (!response) { | |
| fileCache.delete(cacheKey); | |
| } | |
| return response; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/lrucache.ts` around lines 96 - 98, The code uses
fileCache.has(cacheKey) which does not bump LRU recency; replace that check with
fileCache.get(cacheKey) so reads move the entry to MRU. After calling
cacheInner.match(cacheKey), if match returns undefined remove the stale LRU
marker (e.g., fileCache.delete(cacheKey)) and adjust any bytes/size accounting
(the same counters updated on deletions/evictions) so the LRU metadata stays
consistent; update the logic around cacheKey, fileCache.get, cacheInner.match,
and the removal/bytes accounting paths accordingly.
…-index tiered.test.ts (10 tests): - Single-tier hit/miss/status, X-Cache-Tier header set correctly - Multi-tier L1 hit, L2 hit with backfill, stale L1/fresh L2 promotion - put distributes to all tiers, status codes preserved end-to-end inMemoryCache.test.ts (7 tests): - Body, status (including non-200), and headers preserved on round-trip - Miss, delete, and request isolation mod.test.ts (4 new lazy re-index tests): - Valid disk entry re-indexed and served after cold LRU (pod restart sim) - Truly expired entry deleted on first access - Miss when nothing on disk - Re-indexed entry accessible on subsequent accesses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/tiered.ts">
<violation number="1" location="runtime/caches/tiered.ts:85">
P1: Avoid mutating headers directly on cache-hit responses; rebuild a tagged `Response` to prevent runtime errors with immutable responses.</violation>
</file>
<file name="runtime/caches/cacheWriteWorker.ts">
<violation number="1" location="runtime/caches/cacheWriteWorker.ts:6">
P2: The `|| 2097152` fallback makes an explicit `CACHE_MAX_ENTRY_SIZE=0` impossible, because `0` is coerced to the default 2MB.</violation>
</file>
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:133">
P1: Oversized writes should evict any existing entry; this early return otherwise leaves stale cached data for the same key.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| resp?.headers.set(TIERED_CACHE_HEADER, "0"); | ||
| return resp; |
There was a problem hiding this comment.
P1: Avoid mutating headers directly on cache-hit responses; rebuild a tagged Response to prevent runtime errors with immutable responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/tiered.ts, line 85:
<comment>Avoid mutating headers directly on cache-hit responses; rebuild a tagged `Response` to prevent runtime errors with immutable responses.</comment>
<file context>
@@ -87,7 +81,9 @@ export function createTieredCache(
if (openedCaches.length === 1) {
- return openedCaches[0].match(request, options);
+ const resp = await openedCaches[0].match(request, options);
+ resp?.headers.set(TIERED_CACHE_HEADER, "0");
+ return resp;
}
</file context>
| resp?.headers.set(TIERED_CACHE_HEADER, "0"); | |
| return resp; | |
| if (!resp) return resp; | |
| const body = await resp.arrayBuffer(); | |
| const tagged = responseFromBody(body, resp.headers, resp.status); | |
| tagged.headers.set(TIERED_CACHE_HEADER, "0"); | |
| return tagged; |
|
|
||
| const CACHE_MAX_ENTRY_SIZE = parseInt( | ||
| Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152", // 2 MB | ||
| ) || 2097152; |
There was a problem hiding this comment.
P2: The || 2097152 fallback makes an explicit CACHE_MAX_ENTRY_SIZE=0 impossible, because 0 is coerced to the default 2MB.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/cacheWriteWorker.ts, line 6:
<comment>The `|| 2097152` fallback makes an explicit `CACHE_MAX_ENTRY_SIZE=0` impossible, because `0` is coerced to the default 2MB.</comment>
<file context>
@@ -1,6 +1,10 @@
+const CACHE_MAX_ENTRY_SIZE = parseInt(
+ Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152", // 2 MB
+) || 2097152;
+
const textEncoder = new TextEncoder();
</file context>
When putFile rejects an oversized buffer, any previously cached entry for the same key was left on disk, causing stale hits indefinitely. Now evicts the old file before returning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fileSystem.ts: add || fallback so a malformed CACHE_MAX_ENTRY_SIZE env var doesn't silently disable the oversize check (NaN > x is false). inMemoryCache.ts: same NaN guards on all three size env vars, plus reject oversized bodies before storing in L1 — consistent with the filesystem and worker paths. lrucache.ts: memoize open() per cache name so repeated calls don't create duplicate LRU instances that fall out of the activeCaches Map and go untracked by the size/key gauges. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
arrayBuffer() is the most expensive part of a cache put. For the loader path (and any caller that sets Content-Length), we can bail before touching the body at all. The post-read check stays as a fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:16">
P2: Using `|| 2097152` breaks the explicit `CACHE_MAX_ENTRY_SIZE=0` case by restoring the default 2 MB limit.</violation>
</file>
<file name="runtime/caches/inMemoryCache.ts">
<violation number="1" location="runtime/caches/inMemoryCache.ts:11">
P2: `||` fallback on parsed env numbers overrides valid `0` values. Use a NaN-only fallback so explicit zero configuration is preserved.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const CACHE_MAX_ENTRY_SIZE = parseInt( | ||
| Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152", // 2 MB | ||
| ) || 2097152; |
There was a problem hiding this comment.
P2: Using || 2097152 breaks the explicit CACHE_MAX_ENTRY_SIZE=0 case by restoring the default 2 MB limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/fileSystem.ts, line 16:
<comment>Using `|| 2097152` breaks the explicit `CACHE_MAX_ENTRY_SIZE=0` case by restoring the default 2 MB limit.</comment>
<file context>
@@ -13,7 +13,7 @@ const FILE_SYSTEM_CACHE_DIRECTORY =
const CACHE_MAX_ENTRY_SIZE = parseInt(
Deno.env.get("CACHE_MAX_ENTRY_SIZE") ?? "2097152", // 2 MB
-);
+) || 2097152;
const initializedShardDirs = new Set<string>();
</file context>
|
|
||
| const MEMORY_CACHE_MAX_SIZE = parseInt( | ||
| Deno.env.get("MEMORY_CACHE_MAX_SIZE") ?? "268435456", // 256 MB | ||
| ) || 268435456; |
There was a problem hiding this comment.
P2: || fallback on parsed env numbers overrides valid 0 values. Use a NaN-only fallback so explicit zero configuration is preserved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/inMemoryCache.ts, line 11:
<comment>`||` fallback on parsed env numbers overrides valid `0` values. Use a NaN-only fallback so explicit zero configuration is preserved.</comment>
<file context>
@@ -8,10 +8,13 @@ import {
const MEMORY_CACHE_MAX_SIZE = parseInt(
Deno.env.get("MEMORY_CACHE_MAX_SIZE") ?? "268435456", // 256 MB
-);
+) || 268435456;
const MEMORY_CACHE_MAX_ITEMS = parseInt(
Deno.env.get("MEMORY_CACHE_MAX_ITEMS") ?? "2048",
</file context>
…v var STALE_WINDOW_S (seconds) is the new canonical knob for how long stale content stays on disk and is served while background revalidation runs. STALE_TTL_PERIOD (ms) is kept for backwards compat. Default raised from 30s → 1h so low-traffic sites keep serving cached content across quiet periods without hard-missing on every cold request. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No new env var needed — just raise the default so low-traffic sites keep serving stale content while background revalidation catches up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/lrucache.ts">
<violation number="1" location="runtime/caches/lrucache.ts:32">
P1: Validate `STALE_WINDOW_S` before giving it precedence; an invalid value currently overrides the old setting and turns TTL calculations into `NaN`, breaking lazy re-indexing and cache writes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
runtime/caches/lrucache.ts
Outdated
| const STALE_TTL_PERIOD = Deno.env.get("STALE_WINDOW_S") | ||
| ? parseInt(Deno.env.get("STALE_WINDOW_S")!) * 1000 | ||
| : parseInt(Deno.env.get("STALE_TTL_PERIOD") ?? "3600000"); // 1h |
There was a problem hiding this comment.
P1: Validate STALE_WINDOW_S before giving it precedence; an invalid value currently overrides the old setting and turns TTL calculations into NaN, breaking lazy re-indexing and cache writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/lrucache.ts, line 32:
<comment>Validate `STALE_WINDOW_S` before giving it precedence; an invalid value currently overrides the old setting and turns TTL calculations into `NaN`, breaking lazy re-indexing and cache writes.</comment>
<file context>
@@ -25,11 +25,13 @@ const CACHE_TTL_AUTOPURGE = Deno.env.get("CACHE_TTL_AUTOPURGE") === "true"; // c
+// STALE_WINDOW_S (seconds) takes precedence; STALE_TTL_PERIOD (ms) is kept for backwards compat.
+// Default: 1 hour — long enough for low-traffic sites to keep serving cached content across
+// quiet periods while background revalidation catches up.
+const STALE_TTL_PERIOD = Deno.env.get("STALE_WINDOW_S")
+ ? parseInt(Deno.env.get("STALE_WINDOW_S")!) * 1000
+ : parseInt(Deno.env.get("STALE_TTL_PERIOD") ?? "3600000"); // 1h
</file context>
| const STALE_TTL_PERIOD = Deno.env.get("STALE_WINDOW_S") | |
| ? parseInt(Deno.env.get("STALE_WINDOW_S")!) * 1000 | |
| : parseInt(Deno.env.get("STALE_TTL_PERIOD") ?? "3600000"); // 1h | |
| const staleWindowSeconds = parseInt(Deno.env.get("STALE_WINDOW_S") ?? ""); | |
| const staleTtlPeriodMs = parseInt(Deno.env.get("STALE_TTL_PERIOD") ?? "3600000"); | |
| const STALE_TTL_PERIOD = Number.isNaN(staleWindowSeconds) | |
| ? (Number.isNaN(staleTtlPeriodMs) ? 3600000 : staleTtlPeriodMs) | |
| : staleWindowSeconds * 1000; |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/lrucache.ts">
<violation number="1" location="runtime/caches/lrucache.ts:32">
P1: This drops support for `STALE_WINDOW_S`, so the new stale-window configuration is ignored and deployments fall back to the default/legacy value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // quiet periods while background revalidation catches up. | ||
| const STALE_TTL_PERIOD = parseInt( | ||
| Deno.env.get("STALE_TTL_PERIOD") ?? "30000", | ||
| Deno.env.get("STALE_TTL_PERIOD") ?? "3600000", // 1h |
There was a problem hiding this comment.
P1: This drops support for STALE_WINDOW_S, so the new stale-window configuration is ignored and deployments fall back to the default/legacy value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/lrucache.ts, line 32:
<comment>This drops support for `STALE_WINDOW_S`, so the new stale-window configuration is ignored and deployments fall back to the default/legacy value.</comment>
<file context>
@@ -26,12 +26,11 @@ const CACHE_TTL_RESOLUTION = parseInt(
- ? parseInt(Deno.env.get("STALE_WINDOW_S")!) * 1000
- : parseInt(Deno.env.get("STALE_TTL_PERIOD") ?? "3600000"); // 1h
+const STALE_TTL_PERIOD = parseInt(
+ Deno.env.get("STALE_TTL_PERIOD") ?? "3600000", // 1h
+);
</file context>
Bot guard (loader.ts): - Bots read from cache normally but never write to it and never trigger background revalidation. Uses existing isBot() which checks cf-verified-bot header first (free), then falls back to UAParser bot DB. L1 admission filter (inMemoryCache.ts): - Keys must be written MEMORY_CACHE_MIN_HITS times (default 2) before being stored in RAM. One-hit wonders — bot requests, rare URLs — are tracked in a cheap counter LRU and never promoted to L1, keeping the in-memory tier populated with genuinely hot keys. Eviction logging (inMemoryCache.ts): - l1_cache_eviction OTEL counter with reason dimension (evict/delete/set) - Warning log on the 1st eviction and every 100 thereafter so operators can spot L1 memory pressure without being drowned in log spam. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests now call put twice per key to satisfy MEMORY_CACHE_MIN_HITS=2 before asserting a match. Added a test that explicitly verifies a single put does not admit to L1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fileSystem.ts — write rate guard: - Tracks writes per rolling minute window - Warns when writes hit CACHE_WRITE_RATE_WARN (default 500/min), logging the latest key so the hot writer can be identified - High write rate typically means bots, missing cacheKey, or very short TTLs lrucache.ts — disk fill warning: - LRU calculatedSize is the best available proxy for disk usage (Deno has no statvfs equivalent) - Warns when any named cache exceeds LRU_DISK_WARN_RATIO of CACHE_MAX_SIZE (default 90%), fires on every OTEL metric export interval so it shows up in both logs and dashboards without extra polling Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/lrucache.ts">
<violation number="1" location="runtime/caches/lrucache.ts:81">
P2: Avoid logging from the observable gauge callback without rate limiting; once usage crosses the threshold this will warn on every metrics collection cycle.</violation>
</file>
<file name="runtime/caches/fileSystem.ts">
<violation number="1" location="runtime/caches/fileSystem.ts:168">
P2: Write-rate warnings are skipped whenever cache writes use `cacheWriteWorker`, so this new alert silently does nothing in worker-enabled deployments.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| observer.observe(lru.calculatedSize, { cache: name }); | ||
| const ratio = lru.calculatedSize / CACHE_MAX_SIZE; | ||
| if (ratio >= LRU_DISK_WARN_RATIO) { | ||
| logger.warn( |
There was a problem hiding this comment.
P2: Avoid logging from the observable gauge callback without rate limiting; once usage crosses the threshold this will warn on every metrics collection cycle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/lrucache.ts, line 81:
<comment>Avoid logging from the observable gauge callback without rate limiting; once usage crosses the threshold this will warn on every metrics collection cycle.</comment>
<file context>
@@ -66,9 +67,24 @@ lruSizeGauge.addCallback((observer) => {
observer.observe(lru.calculatedSize, { cache: name });
+ const ratio = lru.calculatedSize / CACHE_MAX_SIZE;
+ if (ratio >= LRU_DISK_WARN_RATIO) {
+ logger.warn(
+ `lru_cache: disk usage for cache "${name}" is at ` +
+ `${Math.round(lru.calculatedSize / 1024 / 1024)}MB / ` +
</file context>
| } | ||
| const filePath = `${FILE_SYSTEM_CACHE_DIRECTORY}/${key}`; | ||
|
|
||
| trackWriteRate(key); |
There was a problem hiding this comment.
P2: Write-rate warnings are skipped whenever cache writes use cacheWriteWorker, so this new alert silently does nothing in worker-enabled deployments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/fileSystem.ts, line 168:
<comment>Write-rate warnings are skipped whenever cache writes use `cacheWriteWorker`, so this new alert silently does nothing in worker-enabled deployments.</comment>
<file context>
@@ -138,6 +165,7 @@ function createFileSystemCache(): CacheStorage {
if (!isCacheInitialized) {
await assertCacheDirectory();
}
+ trackWriteRate(key);
const filePath = shardedPath(FILE_SYSTEM_CACHE_DIRECTORY, key);
const dir = filePath.substring(0, filePath.lastIndexOf("/"));
</file context>
Bots skip cache.put(), so if a bot becomes the singleFlight leader, all concurrent non-bot waiters get the result but nothing is cached. Use a `bot:` prefixed key so bots and real users never share flights. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
What changed and why
The problem
Sites on
WEB_CACHE_ENGINE=FILE_SYSTEMwere seeing 10–16s latencies on cache hits. Every hit followed the same path: LRU boolean check →Deno.readFile()from disk → JSON.parse. On containers with 0.3 CPU and 3GB of cache filling 3Gi ephemeral storage, this was catastrophic. On top of that, pod restarts orphaned all disk cache files — the in-memory LRU was empty so existing files were never read, never evicted, and the disk filled up indefinitely.L1 in-memory tier (
inMemoryCache.ts)Added a new
CacheStoragebacked by an LRU in RAM (default 256MB / 2048 items). Wired as L1 before the filesystem viacreateTieredCache. Hot keys now serve from memory without touching disk. Configurable viaMEMORY_CACHE_MAX_SIZEandMEMORY_CACHE_MAX_ITEMS.Tiered cache (
tiered.ts)createTieredCacheorchestrates L1→L2 lookups:Replaced the local
inFuture(2×new Date()per check) with the shared version fromutils.ts(usesDate.parse + Date.now()— zero allocations). AddedX-Cache-Tierresponse header consumed by the instrumentation layer for observability, then stripped before responses reach the caller.Lazy re-index on pod restart (
lrucache.ts)On a cold LRU (e.g. after pod restart), disk files from the previous instance were orphaned — never served, never evicted. Now: on an LRU miss, we check disk. If the file exists and the TTL is still positive, it's re-indexed into the LRU (which then owns eviction). If truly expired, the file is deleted on first access. This means cache warmth survives restarts without any startup scan cost — only accessed keys get re-indexed, and the LRU's size limits naturally cap how much gets recovered.
Stale-while-revalidate window (
lrucache.ts)STALE_TTL_PERIODcontrols how long an expired entry stays on disk and keeps being served while background revalidation runs. Default raised from 30s → 1h:CACHE_MAX_AGE_S: entry is fresh, returned as-isCACHE_MAX_AGE_StoCACHE_MAX_AGE_S + STALE_TTL_PERIOD/1000s: entry is stale, returned immediately + background revalidation fires. If origin is down, the stale entry keeps being served until eviction — implicit stale-if-errorEvery successful revalidation resets the clock, so under normal traffic content effectively lives indefinitely. The 1h window only matters if origin stays down for an extended period. Low-traffic sites that see one request per hour will now consistently hit cache instead of cold-missing every time.
Per-entry size limits
Entries >2MB (configurable via
CACHE_MAX_ENTRY_SIZE) are rejected at every layer:loader.ts: skip write + evict existing stale entry before returninginMemoryCache.ts: checkContent-Lengthheader first to skip the body read entirely; fallback body-length check after readfileSystem.ts/cacheWriteWorker.ts: check combined buffer size beforeDeno.writeFile; evict existing on-disk entry when oversized write is rejectedHex directory sharding
Flat cache directories with tens of thousands of files create severe filesystem metadata pressure. Files are now stored at
{cacheDir}/{key[0:2]}/{key[2:4]}/{key}— 256² = 65536 possible shard directories, each holding a manageable number of files.Correctness fixes
Response.statuspreserved in the in-memory tier (was silently dropped, all cached non-200 responses became 200)Deno.mkdirSyncin an async function replaced withawait Deno.mkdir— was blocking the event loop on initmkdirsucceeds, not beforebgRevalidationmetric moved inside thesingleFlightcallback — previously recorded N times per revalidation (once per concurrent waiter), now records exactly onceopen()memoized per cache name inlrucache.ts— repeatedopen()calls for the same cache name were creating orphaned LRU instances that fell out of the metrics Map|| default) on all env-parsed size limits —parseInt("abc")returningNaNwould silently disable size checksObservability
lru_cache_evictioncounter with evictionreasondimensionlru_cache_keys/lru_cache_bytesobservable gauges per named cacheloader_cache_entry_sizehistogram (opt-in, gated byOTEL_ENABLE_EXTRA_METRICS)loader_bg_revalidationhistogram — background stale-while-revalidate durationcache_tierspan attribute on cache-match spans (0 = in-memory, 1 = disk)content_lengthspan attribute on cache-match spansloader_cachecounter (no-store,no-cache,null-key,disabled)Tests
27 new tests across 3 files:
tiered.test.ts— single/multi-tier hits, backfill, stale promotion, status preservation, tier headerinMemoryCache.test.ts— body/status/headers round-trip, delete, request isolationmod.test.ts(additions) — lazy re-index: valid entry served, expired entry evicted, missing entry misses, re-indexed entry stays warmNew env vars
CACHE_MAX_ENTRY_SIZE2097152(2MB)MEMORY_CACHE_MAX_SIZE268435456(256MB)MEMORY_CACHE_MAX_ITEMS2048STALE_TTL_PERIOD3600000(1h)No new env vars are required for the defaults to work.
Expected gains
CACHE_MAX_SIZE(default 1GB) is exceeded. Oversized entries are now rejected pre-write. Expired orphans are cleaned on first access.Test plan
deno test --unstable-http -A runtime/caches/)WEB_CACHE_ENGINE=FILE_SYSTEMand verify cache initcache_tier=0in HyperDX){key[0:2]}/{key[2:4]}/subdirectories🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores