fix: #46 return 304 for cached files with matching etag#77
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe dynamic (non- Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant Cache as fileCache
participant Checker as isCached()
Client->>Server: GET/HEAD with headers (maybe If-None-Match)
Server->>Cache: lookup(pathName)
Cache-->>Server: cached Response (with ETag)
Server->>Server: extract cachedEtag from Response
Server->>Checker: isCached(requestHeaders, cachedEtag, pathName)
alt fresh (matches)
Checker-->>Server: true
Server-->>Client: 304 Not Modified (empty body)
else stale/mismatch
Checker-->>Server: false
Server-->>Client: 200 OK (cached Response clone with body/headers)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![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 (1)
src/index.ts (1)
314-323: Return validator/cache headers on cached 304 responses, okay smarty~ (￣▽￣)♡Line 320 returns a bare
304. Revalidation works, but include cached validator metadata (Etag, optionallyCache-Control) so intermediaries stay fully informed.♻️ Suggested patch
if (cache) { const cachedEtag = cache.headers.get('etag') if ( cachedEtag && (await isCached(requestHeaders, cachedEtag, pathName)) ) { - return new Response(null, { status: 304 }) + const headers = new Headers( + isNotEmpty(initialHeaders) ? initialHeaders : undefined + ) + headers.set('Etag', cachedEtag) + + const cacheControl = cache.headers.get('cache-control') + if (cacheControl && !headers.has('Cache-Control')) + headers.set('Cache-Control', cacheControl) + + return new Response(null, { status: 304, headers }) } return cache.clone() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 314 - 323, The 304 response currently returns no validator headers; update the branch that returns new Response(null, { status: 304 }) to include validator metadata from the cached response: read cachedEtag (already fetched) and the cachedCacheControl (cache.headers.get('cache-control')) and set them on the 304 response headers (e.g., 'ETag': cachedEtag and, if present, 'Cache-Control': cachedCacheControl). Keep using isCached(requestHeaders, cachedEtag, pathName) for validation and only return cache.clone() when not a 304.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/index.test.ts`:
- Around line 464-486: Tests rely on staticPlugin() running in lazy mode but
NODE_ENV can change its default; update both test instances that call new
Elysia().use(staticPlugin()) (the one checking 304 for GET and the one for HEAD)
to pass an explicit option to pin lazy mode (e.g., staticPlugin({ lazy: true }))
so the plugin behavior is deterministic regardless of environment.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 314-323: The 304 response currently returns no validator headers;
update the branch that returns new Response(null, { status: 304 }) to include
validator metadata from the cached response: read cachedEtag (already fetched)
and the cachedCacheControl (cache.headers.get('cache-control')) and set them on
the 304 response headers (e.g., 'ETag': cachedEtag and, if present,
'Cache-Control': cachedCacheControl). Keep using isCached(requestHeaders,
cachedEtag, pathName) for validation and only return cache.clone() when not a
304.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d13deaa-f053-4fce-8e97-5a37d55e1694
📒 Files selected for processing (2)
src/index.tstest/index.test.ts
If what you're saying is true, this is also resolved by #79 :P |
3 participants
Closes #46.
What this PR does
Fixes a caching bug in the static plugin. The bug causes browsers and CDNs
to re-download files they already have.
What we expected to find
The issue title says "HEAD request 404s for all static files." We started
by trying to reproduce that.
What we actually found
On Elysia 1.4.28, HEAD requests don't 404 anymore. Elysia itself now
handles HEAD requests automatically. It runs the GET handler and strips
the body. So the surface-level symptom in the issue title is already gone.
But the production problem the issue actually describes is still happening.
Caches still re-download files they already have. We dug into why. The
real bug turned out to be in how the plugin handles repeat requests.
The actual bug
When a browser requests a file it already has cached, it sends an
If-None-Matchheader with the file's etag. The server is supposed tocheck if its version matches what the browser already has. If yes, the
server replies with 304 Not Modified. The browser then uses its cached
copy.
The plugin has two layers of caching that run in this order on every
request:
the file from disk.
Here is the bug. On the second request for the same file, layer 1 returned
the saved 200 response immediately. Layer 2 never ran. The etag check that
would have returned 304 was unreachable. Browsers and CDNs got a fresh 200
with the full file body every time, even when they already had it.
HEAD requests inherited the same 200. This is because Elysia derives HEAD
responses from GET responses. Once GET is wrong, HEAD is wrong.
The fix
Made layer 1 aware of conditional requests. When the in-memory cache hits,
we now do this:
If-None-Matchheader.10 lines of code. The in-memory cache still does its job. We just stopped
it from skipping the correctness check.
This also fixes HEAD requests automatically. HEAD inherits GET's status.
Tests
Added two tests in
test/index.test.ts:If-None-Matchreturns 304If-None-Matchreturns 304The HEAD test exists to catch any future Elysia change that breaks the
GET-to-HEAD inheritance.
Verification
Tests in the PR (Bun):
Manual cross-runtime verification (not in the PR):
If-None-Matchreturn 304 against the built
dist/artifactThe PR adds Bun tests only. The Node and Deno smoke runs above are
local manual checks; happy to add Node test coverage of the 304
behavior in a follow-up if useful.
Something else we noticed but didn't fix
The eager-mode handler (
handleCache, around lines 162 to 189) has asimilar but separate problem with
If-Modified-Sincerequests. It alsochecks the in-memory cache before checking whether the client's copy is
still fresh. We left it alone to keep this PR focused on the reproducible
bug from #46. Happy to file a separate issue if useful.
Summary by CodeRabbit
New Features
Bug Fixes
Tests