feat(cache): lazy re-index disk entries into LRU on pod restart#1126
Open
feat(cache): lazy re-index disk entries into LRU on pod restart#1126
Conversation
When the LRU is cold (e.g. after pod restart), check if the file exists on disk on cache miss. If the entry is still valid (TTL > 0), re-index it into the LRU so eviction is managed normally. If expired, delete the orphaned file. This allows pods to reuse existing disk cache across restarts instead of treating all entries as misses until they're naturally re-fetched. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughOn LRU miss, the cache lazily re-indexes by reading the on-disk entry, validating Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LRU as LRU Cache
participant Disk as Disk Cache
participant TTL as TTL Logic
Client->>LRU: match(key)
LRU->>LRU: check in-memory LRU
alt LRU hit
LRU-->>Client: return cached response
else LRU miss
LRU->>Disk: match(key)
Disk-->>LRU: entry / undefined
alt disk entry present
LRU->>TTL: read expires & content-length
TTL-->>LRU: compute ttl = Date.parse(expires) - now + STALE_TTL_PERIOD
alt ttl > 0
LRU->>LRU: set(key, { size=parseInt(length,10), ttl })
LRU-->>Client: return response
else ttl <= 0
LRU->>Disk: delete(key)
LRU-->>Client: return miss
end
else no disk entry
LRU-->>Client: return miss
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
runtime/caches/lrucache.ts (1)
69-82: Lazy re-index logic looks correct.The logic properly handles:
- Cache misses from disk (line 70)
- Valid entries with remaining TTL (lines 74-77)
- Expired or metadata-missing entries with cleanup (line 81)
Minor suggestion: add explicit radix to
parseIntfor defensive coding.🔧 Suggested improvement
if (ttl > 0) { - fileCache.set(cacheKey, true, { size: parseInt(length), ttl }); + fileCache.set(cacheKey, true, { size: parseInt(length, 10), ttl }); 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 69 - 82, The parseInt call in the lazy re-index logic should explicitly specify a radix to avoid ambiguous parsing; update the parseInt(length) used when calling fileCache.set (in the block that checks expires and length and computes ttl using STALE_TTL_PERIOD) to parseInt(length, 10), keeping the rest of the cacheInner.match/cacheKey/fileCache.set logic unchanged.runtime/caches/mod.test.ts (1)
153-182: Consider consolidating with existingtestCacheStorage.
sharedMapCacheStorageis functionally identical totestCacheStorage(lines 11-45)—both accept an externalMapparameter and return the same mockCacheStorage. The comment suggests a distinction about "shared" maps, buttestCacheStoragealready supports this pattern.♻️ Proposed consolidation
Reuse
testCacheStoragein the lazy re-index tests:Deno.test({ name: "lru_cache_lazy_reindex: valid entry is served after LRU restart", sanitizeResources: false, sanitizeOps: false, }, async () => { const disk = new Map<RequestInfo | URL, Response>(); - const storage1 = sharedMapCacheStorage(disk); + const storage1 = testCacheStorage(disk); // ... rest of test - const storage2 = sharedMapCacheStorage(disk); + const storage2 = testCacheStorage(disk);Then remove the
sharedMapCacheStoragefunction entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/mod.test.ts` around lines 153 - 182, The new sharedMapCacheStorage function duplicates behavior already provided by testCacheStorage; replace uses of sharedMapCacheStorage with testCacheStorage (passing the external Map) in the lazy re-index tests and remove the redundant sharedMapCacheStorage function declaration; update any imports/refs to point to testCacheStorage and ensure its signature matches the Map<RequestInfo|URL, Response> parameter usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@runtime/caches/lrucache.ts`:
- Around line 69-82: The parseInt call in the lazy re-index logic should
explicitly specify a radix to avoid ambiguous parsing; update the
parseInt(length) used when calling fileCache.set (in the block that checks
expires and length and computes ttl using STALE_TTL_PERIOD) to parseInt(length,
10), keeping the rest of the cacheInner.match/cacheKey/fileCache.set logic
unchanged.
In `@runtime/caches/mod.test.ts`:
- Around line 153-182: The new sharedMapCacheStorage function duplicates
behavior already provided by testCacheStorage; replace uses of
sharedMapCacheStorage with testCacheStorage (passing the external Map) in the
lazy re-index tests and remove the redundant sharedMapCacheStorage function
declaration; update any imports/refs to point to testCacheStorage and ensure its
signature matches the Map<RequestInfo|URL, Response> parameter usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 406b9982-c187-4478-9fd0-43de2efd617a
📒 Files selected for processing (2)
runtime/caches/lrucache.tsruntime/caches/mod.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 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/mod.test.ts">
<violation number="1" location="runtime/caches/mod.test.ts:153">
P3: This helper substantially duplicates `testCacheStorage`; reuse the existing helper to avoid divergence in test behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
runtime/caches/mod.test.ts (2)
178-207: Assert orphan file deletion in the expired-entry case.Line [206] verifies miss behavior, but this test should also assert that disk cleanup happened (a key PR objective).
🧪 Proposed test tightening
- const response = await cache2.match(createRequest(200)); + const request = createRequest(200); + const response = await cache2.match(request); assertEquals(response, undefined); + assertEquals(disk.has(request.url), false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/mod.test.ts` around lines 178 - 207, The test currently only asserts a cache miss but doesn't verify orphaned disk entries were removed; after the existing assertEquals(response, undefined) add an assertion that the underlying disk Map (disk) no longer contains the entry for the request inserted earlier (or that disk.size === 0) to ensure headersCache/lruCache performed cleanup; reference the test variables disk, cache1/cache2, CACHE_NAME and the request created by createRequest(200) when locating where to add the check.
209-221: Add a test for missing-metadata orphan cleanup.This block covers “missing on disk,” but not the
missing expires/content-lengthcleanup path inruntime/caches/lrucache.ts(Lines 71-82).🧪 Suggested additional test
+Deno.test({ + name: "lru_cache_lazy_reindex: entry with missing metadata is deleted and missed", + sanitizeResources: false, + sanitizeOps: false, +}, async () => { + const disk = new Map<RequestInfo | URL, Response>(); + const request = createRequest(301); + disk.set(request.url, new Response("bad", { + headers: { "content-length": "3" }, // missing expires + })); + + const cache = await headersCache(lruCache(testCacheStorage(disk))).open(CACHE_NAME); + const response = await cache.match(request); + + assertEquals(response, undefined); + assertEquals(disk.has(request.url), false); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/mod.test.ts` around lines 209 - 221, Add a new test alongside "lru_cache_lazy_reindex: entry missing from disk is a miss" that verifies the orphan-cleanup path in runtime/caches/lrucache.ts where entries exist on disk but are missing metadata (expires or content-length). In the test setup use the same helpers (testCacheStorage, headersCache, lruCache, CACHE_NAME, createRequest) to create a storage Map containing a Response on disk but intentionally omit or remove the associated metadata (expires/content-length) for that request; then open the cache (await headersCache(lruCache(storage)).open(CACHE_NAME)) and assert that cache.match(...) returns undefined and that the disk Map no longer contains the orphaned entry, ensuring the cleanup code in lrucache.ts lines ~71-82 ran. Ensure the test toggles sanitizeResources/sanitizeOps like the adjacent test and gives a clear name e.g. "lru_cache_lazy_reindex: missing-metadata orphan is cleaned up".runtime/caches/lrucache.ts (1)
73-82: Validate parsedcontent-lengthbefore re-indexing.On Line [76],
content-lengthis parsed but not validated. Malformed metadata should be treated as invalid/orphan and cleaned up, not re-indexed.🔧 Proposed fix
- if (expires && length) { - const ttl = Date.parse(expires) - Date.now() + STALE_TTL_PERIOD; - if (ttl > 0) { - fileCache.set(cacheKey, true, { size: parseInt(length, 10), ttl }); - return response; - } - } + if (expires && length) { + const ttl = Date.parse(expires) - Date.now() + STALE_TTL_PERIOD; + const size = Number.parseInt(length, 10); + if (ttl > 0 && Number.isFinite(size) && size > 0) { + fileCache.set(cacheKey, true, { size, ttl }); + 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 73 - 82, The code currently parses content-length into parseInt(length, 10) and uses it without validation when calling fileCache.set; update the logic in the block that handles expires and length (the section referencing expires, length, STALE_TTL_PERIOD, cacheKey, fileCache.set, and cacheInner.delete) to validate the parsed size first: call parseInt(length, 10) (or Number(length)) into a variable (e.g., parsedSize) and ensure it is a finite integer > 0 before using it in fileCache.set; if parsedSize is NaN, non-positive, or otherwise invalid, treat the metadata as orphaned by calling cacheInner.delete(cacheKey) and returning undefined instead of re-indexing. Ensure the TTL calculation (Date.parse(expires) - Date.now() + STALE_TTL_PERIOD) still requires ttl > 0 before setting the cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@runtime/caches/lrucache.ts`:
- Around line 73-82: The code currently parses content-length into
parseInt(length, 10) and uses it without validation when calling fileCache.set;
update the logic in the block that handles expires and length (the section
referencing expires, length, STALE_TTL_PERIOD, cacheKey, fileCache.set, and
cacheInner.delete) to validate the parsed size first: call parseInt(length, 10)
(or Number(length)) into a variable (e.g., parsedSize) and ensure it is a finite
integer > 0 before using it in fileCache.set; if parsedSize is NaN,
non-positive, or otherwise invalid, treat the metadata as orphaned by calling
cacheInner.delete(cacheKey) and returning undefined instead of re-indexing.
Ensure the TTL calculation (Date.parse(expires) - Date.now() + STALE_TTL_PERIOD)
still requires ttl > 0 before setting the cache.
In `@runtime/caches/mod.test.ts`:
- Around line 178-207: The test currently only asserts a cache miss but doesn't
verify orphaned disk entries were removed; after the existing
assertEquals(response, undefined) add an assertion that the underlying disk Map
(disk) no longer contains the entry for the request inserted earlier (or that
disk.size === 0) to ensure headersCache/lruCache performed cleanup; reference
the test variables disk, cache1/cache2, CACHE_NAME and the request created by
createRequest(200) when locating where to add the check.
- Around line 209-221: Add a new test alongside "lru_cache_lazy_reindex: entry
missing from disk is a miss" that verifies the orphan-cleanup path in
runtime/caches/lrucache.ts where entries exist on disk but are missing metadata
(expires or content-length). In the test setup use the same helpers
(testCacheStorage, headersCache, lruCache, CACHE_NAME, createRequest) to create
a storage Map containing a Response on disk but intentionally omit or remove the
associated metadata (expires/content-length) for that request; then open the
cache (await headersCache(lruCache(storage)).open(CACHE_NAME)) and assert that
cache.match(...) returns undefined and that the disk Map no longer contains the
orphaned entry, ensuring the cleanup code in lrucache.ts lines ~71-82 ran.
Ensure the test toggles sanitizeResources/sanitizeOps like the adjacent test and
gives a clear name e.g. "lru_cache_lazy_reindex: missing-metadata orphan is
cleaned up".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bddf0dc0-bac7-4b5d-9051-bc1eccc609f1
📒 Files selected for processing (2)
runtime/caches/lrucache.tsruntime/caches/mod.test.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
runtime/caches/lrucache.ts (1)
72-76: Consider validatingparseIntresult for robustness.If a malformed
content-lengthheader (e.g.,"abc") reaches this path,parseInt(length, 10)returnsNaN, and the LRU cache may behave unexpectedly with aNaNsize. While unlikely with well-formed HTTP responses, you could add a guard:if (expires && length) { const ttl = Date.parse(expires) - Date.now() + STALE_TTL_PERIOD; - if (ttl > 0) { + const size = parseInt(length, 10); + if (ttl > 0 && !Number.isNaN(size)) { fileCache.set(cacheKey, true, { size: parseInt(length, 10), ttl });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/lrucache.ts` around lines 72 - 76, The code reads content-length via response.headers.get("content-length") and passes parseInt(length, 10) directly into fileCache.set (cacheKey, true, { size, ttl }) which can yield NaN for malformed headers; update the logic around response.headers.get, parseInt, and fileCache.set to validate the parsed size (e.g., const size = parseInt(length, 10); if (!Number.isFinite(size) || size < 0) { skip setting the size or set size = 0; } or skip caching altogether) so fileCache.set is never called with a NaN size and keep the existing ttl/STALE_TTL_PERIOD and cacheKey behavior intact.
🤖 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 63-82: The CI is failing due to formatting in the lazy re-index
block in runtime/caches/lrucache.ts (the area using cacheInner.match,
response.headers.get("expires"/"content-length"), STALE_TTL_PERIOD,
fileCache.set, and cacheInner.delete); run the formatter (deno fmt) and commit
the resulting changes so the spacing/indentation and line breaks around that
block match project style—no logic changes required, just apply the automatic
formatting to the function containing the lazy re-index code.
---
Nitpick comments:
In `@runtime/caches/lrucache.ts`:
- Around line 72-76: The code reads content-length via
response.headers.get("content-length") and passes parseInt(length, 10) directly
into fileCache.set (cacheKey, true, { size, ttl }) which can yield NaN for
malformed headers; update the logic around response.headers.get, parseInt, and
fileCache.set to validate the parsed size (e.g., const size = parseInt(length,
10); if (!Number.isFinite(size) || size < 0) { skip setting the size or set size
= 0; } or skip caching altogether) so fileCache.set is never called with a NaN
size and keep the existing ttl/STALE_TTL_PERIOD and cacheKey behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80819416-edf1-4833-9be5-7112c2bac220
📒 Files selected for processing (1)
runtime/caches/lrucache.ts
Comment on lines
+63
to
82
| // Lazy re-index: on a cold LRU (e.g. after pod restart), check if the file | ||
| // exists on disk. If still valid, re-index into the LRU so eviction is managed | ||
| // normally from this point on. If expired, delete the orphaned file and miss. | ||
| // TODO: add a background sweep at startup that deletes expired orphaned files | ||
| // that are never re-accessed — they accumulate on disk across restarts and are | ||
| // never evicted without this sweep. | ||
| const response = await cacheInner.match(cacheKey); | ||
| if (!response) return undefined; | ||
| const expires = response.headers.get("expires"); | ||
| const length = response.headers.get("content-length"); | ||
| if (expires && length) { | ||
| const ttl = Date.parse(expires) - Date.now() + STALE_TTL_PERIOD; | ||
| if (ttl > 0) { | ||
| fileCache.set(cacheKey, true, { size: parseInt(length, 10), ttl }); | ||
| return response; | ||
| } | ||
| } | ||
| // Expired or missing metadata — delete the orphaned file and treat as a miss. | ||
| cacheInner.delete(cacheKey).catch(() => {}); | ||
| return undefined; |
There was a problem hiding this comment.
Lazy re-index logic is sound; fix the CI formatting failure.
The logic correctly handles the cold-LRU scenario: validating TTL with STALE_TTL_PERIOD extension (consistent with put()), re-indexing valid entries, and cleaning up expired/orphaned files.
The CI failure indicates a formatting issue on line 76. Run deno fmt to fix.
🔧 Suggested formatting fix (run `deno fmt` to confirm exact output)
- fileCache.set(cacheKey, true, { size: parseInt(length, 10), ttl });
+ fileCache.set(cacheKey, true, {
+ size: parseInt(length, 10),
+ ttl,
+ });🧰 Tools
🪛 GitHub Actions: ci
[error] 76-76: deno fmt --check failed: Found unformatted file(s). Example diff indicates formatting changes around fileCache.set(cacheKey, true, { size: parseInt(length, 10), ttl }).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/lrucache.ts` around lines 63 - 82, The CI is failing due to
formatting in the lazy re-index block in runtime/caches/lrucache.ts (the area
using cacheInner.match, response.headers.get("expires"/"content-length"),
STALE_TTL_PERIOD, fileCache.set, and cacheInner.delete); run the formatter (deno
fmt) and commit the resulting changes so the spacing/indentation and line breaks
around that block match project style—no logic changes required, just apply the
automatic formatting to the function containing the lazy re-index code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
deno test runtime/caches/mod.test.ts🤖 Generated with Claude Code
PR 3 of 5 — split from #1122. Merge order: 1 → 2 → 3 → 4 → 5
Summary by CodeRabbit
Summary by cubic
Enable lazy re-indexing of disk-cached entries into the in-memory LRU after pod restarts to reuse existing cache and avoid cold starts. Valid entries are re-added with TTL/size; expired or metadata-missing files are deleted.
New Features
testCacheStoragehelper.Bug Fixes
parseIntcalls.testCacheStoragehelper to reduce duplication.Written for commit cb31fc3. Summary will update on new commits.