fix: preserve Response identity on Bun so Range works for new Response(Bun.file())#1869
Open
alexkahndev wants to merge 2 commits into
Open
fix: preserve Response identity on Bun so Range works for new Response(Bun.file())#1869alexkahndev wants to merge 2 commits into
alexkahndev wants to merge 2 commits into
Conversation
…Response(Bun.file()) createResponseHandler always rewraps the handler's Response in `new Response(response.body, ...)` since 1.4.23 (commit 0d9b040, "fix: can't modify immutable headers"). The rewrap reads `response.body` as a generic ReadableStream, which severs the Bun.file association on the body and disables Bun.serve's automatic Range-request handling for `new Response(Bun.file())`. This is the response shape `@elysiajs/static` returns for every static asset, so the regression silently broke HTTP byte-range support for static media on Bun. Most visible symptom: mobile Safari refuses to play `<video>` (Apple requires byte-range support). Desktop browsers tolerate it, so the regression went unnoticed. The original always-rewrap was a legitimate fix for Node and Cloudflare Workers, where response.headers is immutable. The fix here is to take the mutate-in-place path only when both conditions hold: we're on Bun (where mutation is safe) and the response doesn't need rewrapping anyway (status unchanged, no streaming required). Otherwise fall through to the existing rewrap path. Tests: - Existing range-handling tests pass unchanged (they exercise handleFile, not createResponseHandler). - Regression test asserting Response identity is preserved on the mutate path. - Regression test asserting rewrap still happens when status changes. - End-to-end test via app.listen + fetch confirming 206 + correct content-length + correct body bytes for new Response(Bun.file()) under the actual Bun.serve runtime. Related: elysiajs#1768 (ReadableStream cancel callback never invoked) reports the same root cause (always-rewrap losing body identity) for a different symptom. elysiajs/elysia-static#19 / elysiajs#79 are the downstream impact in the static plugin.
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR restores Bun’s auto-Range behavior for ChangesResponse Identity Preservation & Range Request Fix
Sequence DiagramsequenceDiagram
participant Client
participant Elysia as Elysia Handler
participant BunHTTP as Bun HTTP Layer
rect rgba(200, 120, 120, 0.5)
Note over Client,BunHTTP: Before Fix (always rewrap)
Client->>Elysia: GET /file + Range
Elysia->>Elysia: createResponseHandler rewraps<br/>Response.body read as stream
Note over Elysia: Bun.file association lost
Elysia->>BunHTTP: pass rewrapped Response
BunHTTP->>Client: 200 OK (full file)
end
rect rgba(120, 200, 150, 0.5)
Note over Client,BunHTTP: After Fix (Bun fast path)
Client->>Elysia: GET /file + Range
Elysia->>Elysia: compute mergedStatus<br/>check platform & streaming need
alt Bun & status unchanged & no streaming
Elysia->>Elysia: clone Response & mergeHeadersInPlace()
Elysia->>BunHTTP: pass original/clone preserving Bun.file
BunHTTP->>Client: 206 Partial Content
else fallback
Elysia->>Elysia: rewrap Response with merged headers/status
Elysia->>BunHTTP: pass rewrapped Response
BunHTTP->>Client: 200 or other as appropriate
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapter/utils.ts`:
- Around line 529-532: The predicate computing needsStreamHandling currently
inspects only response.headers; instead compute and use the merged headers (the
same headers used with mergeStatus) so additions from set.headers (e.g.,
transfer-encoding: chunked) are considered. Concretely, create mergedHeaders by
combining response.headers with set.headers (e.g., clone response.headers then
apply set.headers entries), then set needsStreamHandling based on
mergedHeaders.has('content-length') and mergedHeaders.get('transfer-encoding')
=== 'chunked', and use mergedHeaders (not response.headers) in the Bun fast-path
branch that checks streaming; keep references to mergedStatus and the existing
mergeStatus call intact.
- Around line 492-511: mergeHeadersInPlace currently mutates a handler-owned
Response headers in-place causing header leakage across requests; change it to
avoid in-place mutation by creating a new Headers instance (e.g., const merged =
new Headers(target); apply merges from source into merged instead of target),
return that merged Headers (or update callers to replace target.headers with the
returned Headers) so handlers that reuse a Response won't be polluted, and add a
regression test that registers a handler which returns the same Response object
for two sequential requests while passing different Context['set']['headers']
for each request and asserts the second response only contains its own merged
headers (no stale headers from the first request).
🪄 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: 32ae8f44-159c-4811-a9cc-a08b21a9f40b
📒 Files selected for processing (2)
src/adapter/utils.tstest/response/range.test.ts
…g handler-owned Response Addresses CodeRabbit review on PR elysiajs#1869: 1. Cross-request header pollution: mutating response.headers in place leaks set.headers across requests when the handler returns a cached or shared Response. Reproducer (without this commit, on Bun.serve): const shared = new Response('hello') new Elysia() .onRequest(({ set }) => { set.headers['x-request-id'] = String(++c) }) .get('/', () => shared) Request 1 mutates shared.headers to {x-request-id: '1'}; request 2's non-conflicting merge skips because the key already exists, so the second response carries stale state and `shared` is permanently polluted. Fix: clone the response (preserves Bun.file body identity per Bun's Response.clone behavior — verified end-to-end) and merge set.headers into the clone instead. 2. Streaming decision must consider merged headers: the previous code inspected response.headers only, missing the case where set.headers provides transfer-encoding: chunked. Cloning + merging first means the streaming check naturally sees the post-merge headers via cloned.headers. Tests added: - Pristine handler-owned Response after merge. - No header leakage across two real requests over Bun.serve when handler returns a shared Response (the bug CodeRabbit flagged). Existing identity-preservation test removed (no longer the contract). End-to-end Bun.file Range test still passes — clone() preserves body identity so Bun.serve's auto-Range still fires.
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
2 participants
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.
Closes #1868.
createResponseHandleralways rewraps the handler's Response innew Response(response.body, ...)since 1.4.23 (0d9b0401, "fix: can't modify immutable headers"). On Bun, that rewrap readsresponse.bodyas a genericReadableStream, which severs theBun.fileassociation on the body and disables Bun.serve's automatic Range-request handling.The original always-rewrap was a legitimate fix for Node / Cloudflare Workers, where
response.headersis immutable. This PR keeps that path for those runtimes and adds anisBun && !needsRewrapfast path that mutatesresponse.headersin place and returns the original Response — preserving every body-specific optimization Bun.serve provides.Why
Real-world impact: every static asset served by
@elysiajs/staticisnew Response(Bun.file()). The 1.4.23 rewrap silently broke 206 / Accept-Ranges for all of them on Bun. Mobile Safari refuses to play<video>without byte-range support (Apple requirement), so the regression broke video playback on iOS for every Elysia + Bun + static-plugin app. Desktop browsers tolerate it, so it went unnoticed.Approach
In
createResponseHandler:isBun), (b) the merged status equals the original response status (no rewrap forced for status reasons), and (c) the response doesn't need stream wrapping (content-lengthis set ORtransfer-encodingis not chunked).A new helper
mergeHeadersInPlacemutates a targetHeaderswith non-conflicting entries fromset.headers, mirroring the logic inmergeHeadersexactly except the target is mutated rather than constructed.Tests
app.listen/fetch) confirmingnew Response(Bun.file())returns 206 with correctcontent-length,content-range, and body bytes for a Range request.Full test suite: 1528 pass, 0 fail.
Related
#1768 (open) reports the same root cause —
createResponseHandleralways rewrapping the response — for a different downstream symptom (ReadableStreamcancelcallback never invoked when the client disconnects). This fix doesn't directly address that symptom, but the underlying bug is the same: rewrapping severs body identity. The reporter would benefit from this PR's structural change as a baseline.elysiajs/elysia-static#19and#79document the downstream impact in the static plugin. With this fix, the static plugin'snew Response(file)shape (1.4.7 and 1.4.10) gets Range support back without changes to@elysiajs/static.Summary by CodeRabbit
Bug Fixes
Tests