Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughSendfile request bodies can be replayed across redirects. TLS requests and HTTPS proxy tunnels buffer sendfile data in memory. Request state tracks the send cursor, and cleanup drops the buffer after state. ChangesSendfile state and file reads
TLS buffering and redirect replay
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change preserves file bodies across redirects and buffers them for encrypted transports. No actionable merge-blocking issue was established; merge readiness remains subject to normal build and test checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on bun 1.4.0 and on current main with the script in the PR description (hop 2 receives The head is 1057464, a rebase onto main bc7a813 (one import conflict, see the comment below). CI build 122871 on that head is complete: 180 of 181 jobs passed, macOS included. The one red test is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the HTTP client's redirect state machine, adds a new self-borrowing owned buffer with an unsafe lifetime detachment, and makes a design choice the description explicitly flags as reviewable (synchronous pread on the HTTP thread when a sendfile hop lands on TLS vs. failing with RequestBodyNotReusable), a human look would still be worthwhile.
What was reviewed:
request_body_for_redirectand the newstate.sendfilecursor:original_request_bodyis never advanced, so replaying it on the next hop restarts from offset 0; the 303 / 301+302-POST GET-downgrade path still clearsresend_request_body_on_redirectand reaches theBytes(b"")arm.buffered_sendfile_bodylifecycle: initialized toVec::new()inmake_client(so the bitwise-shared JS-thread original has nothing to free), filled at most once on the clone, andmem::take'd afterstateat both teardown sites, mirroringcompressed_request_body.SendFile::read_to_vec: usestry_reserve_exact(OOM-safe) +bun_sys::File::pread_allfrom the stored offset, truncating on short read;content_sizeis the same valuefetch()already stat'd, so the allocation is bounded.- The
on_writableSendfilearm now mutatesstate.sendfileinstead oforiginal_request_body, and theIS_SSLpanic is now unreachable becausebuffer_sendfile_body_for_tlsruns first instart_.
Extended reasoning...
Overview
This PR fixes two related bugs around fetch(url, { body: Bun.file(...) }) when the file is large enough (≥ 32 KiB) to be sent via sendfile(2): (1) following a 307/308 (or a non-POST 301/302) sent Content-Length: 0 on the redirected hop instead of replaying the file, and (2) an https:// proxy taken from the environment reached the sendfile is only supported without SSL panic. It touches five Rust files in src/http/ (the HTTP client state machine, per-attempt state, the sendfile shim, and the two clone-teardown sites) plus adds ~250 lines of tests across two files.
The fix has three pieces: a per-attempt sendfile cursor on InternalState so original_request_body stays intact for replay; a shared request_body_for_redirect() helper used by both do_redirect and do_redirect_multiplexed that hands Bytes/Sendfile bodies to start() unchanged; and a buffer_sendfile_body_for_tls fallback in start_() that preads the file range into a new client-owned buffered_sendfile_body: Vec<u8> and swaps the body to Bytes when the hop is TLS or a CONNECT tunnel.
Security risks
None identified. The file read uses the same fd and content_size the JS side already opened and stat'd for the request; try_reserve_exact guards the allocation and pread_all bounds the read to the pre-sized buffer. No new user-controlled input reaches a parser or a path resolver. The redirect body-replay semantics match WHATWG HTTP-redirect fetch (307/308 keep the body; 303 and 301/302-POST become bodiless GET), which the tests cover explicitly.
Level of scrutiny
This is a correctness fix in a critical, memory-safety-sensitive path. src/http/lib.rs is the HTTP client's core state machine; the change adds an owned Vec on HTTPClient that state.original_request_body borrows via unsafe { bun_ptr::detach_lifetime(...) }, with correctness depending on (a) the buffer being filled at most once (guarded by the body becoming Bytes afterward and a debug assert) and (b) both explicit teardown sites dropping state before the buffer. The reasoning in the SAFETY comment holds and the pattern mirrors compressed_request_body, but it is exactly the kind of self-borrow lifetime invariant the repo's review guidance calls out as most-blocked and worth a maintainer's eyes.
Other factors
The PR description explicitly offers an alternative design — failing the TLS hop with RequestBodyNotReusable instead of buffering on the HTTP thread — and says it is "easy to switch if buffering on the HTTP thread is not wanted." That is a design decision a maintainer should sign off on: the synchronous pread runs on the single HTTP thread and, while rare (only when a sendfile upload is redirected onto TLS or routed through an https env-proxy), can block every other in-flight request for the duration of the read. The author also notes an interaction with #35792 that will need a rebase. Test coverage is thorough (redirect status × method matrix, sliced files, http→https→https chain, mid-upload interruption, env-proxy subprocess tests) and the description reports the relevant existing suites pass; no bugs were found by the automated bug hunt.
|
Updated 7:41 AM PT - Oct 2nd, 2026
❌ @robobun, your commit 1057464 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38406That installs a local version of the PR into your bun-38406 --bun |
|
Cross-reference: the second symptom in this PR's description ( For reference, the decision-side version of that part of the fix is on branch |
65c23b0 to
4c68c31
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto main (4c68c31). The only conflict was in The doc comments the comment check flagged are now one line each (7cddd52, 12f28f7). The new tests and the related fetch, proxy, h2, and h3 suites pass on the rebased debug build; the new cases still fail on the unfixed binary. The coderabbit notices on this PR are rate-limit messages only. The decision-side variant on |
|
Rebased onto main bf42a52 (was 12f28f7, now 7e81605). The conflicts and their resolutions:
The added and removed lines in Checked on a debug build of the rebased branch:
|
12f28f7 to
7e81605
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the unsafe detach_lifetime borrow in buffer_sendfile_body_for_tls: start() builds self.state fresh immediately before start_(), so re-seating it there loses no per-hop state, the body is Bytes for every later hop so the Vec is never reassigned while borrowed, and both teardown sites drop buffered_sendfile_body after state.
Extended reasoning...
The change makes the HTTP client replay a Sendfile body across redirects and buffer it into memory when a hop lands on TLS or a CONNECT tunnel, touching InternalState, SendFile, the redirect paths and two teardown sites; it touches no auth or crypto surface. Two findings are being posted inline (synchronous file read on the shared HTTP thread, and the keep-alive retry predicate still excluding Sendfile), so approval is not on the table; this note only records the lifetime and drop-order checks that were examined and ruled out.
| debug_assert!(self.buffered_sendfile_body.is_empty()); | ||
| self.buffered_sendfile_body = sendfile.read_to_vec()?; |
There was a problem hiding this comment.
🟡 (optional) Every other in-flight fetch in the process stalls while a large Bun.file() body is read into memory on the HTTP thread when a redirect (or env https proxy) moves it onto TLS. buffer_sendfile_body_for_tls at src/http/lib.rs:2947 calls read_to_vec synchronously on the single shared HTTP thread, allocating and pread-ing the whole content_size (the full file, possibly GBs) before the hop can start. Fix: keep the HTTP thread non-blocking for this fallback, e.g. fail the hop with RequestBodyNotReusable (the spec-conformant option the PR itself names) or bounce the read to the JS/IO side, while still buffering small files so the http->https redirect case keeps working.
Why this was flagged
A user calls fetch("http://...", { method: "PUT", body: Bun.file(largePath) }) where the file is >= 32 KiB, so src/runtime/webcore/fetch.rs:1599-1629 turns the body into HTTPRequestBody::Sendfile. The first hop answers 307 with an https Location (or http_proxy=https://... is set in the environment). do_redirect calls start() (src/http/lib.rs:2703) and start_:: runs buffer_sendfile_body_for_tls (src/http/lib.rs:2793). That function calls SendFile::read_to_vec (src/http/SendFile.rs:27-38), which try_reserve_exact + resize the full content_size and pread_all the whole range synchronously on the HTTP thread. For a multi-hundred-MiB or GiB file on a cold disk this blocks the one HTTP thread for seconds and holds the whole file in memory, so every other fetch() in the process (unrelated requests, timeouts, keep-alive handling) stops progressing until the read finishes. On the base branch this hop sent Content-Length: 0 (or panicked for the env-proxy case), so the stall is new.
Verification: buffer_sendfile_body_for_tls (src/http/lib.rs:2937-2954) does self.buffered_sendfile_body = sendfile.read_to_vec()?; when IS_SSL or http_proxy.is_some() && url.is_https(). SendFile::read_to_vec (src/http/SendFile.rs:27-38) does try_reserve_exact(self.content_size), then pread_all, which loops pread until the whole buffer is filled. Nothing yields to the uws loop, so every other fetch on the process's HTTP thread makes no progress.
There was a problem hiding this comment.
This cost is real. The read is synchronous on the HTTP thread and holds content_size bytes, and http to https is a common redirect. I am evaluating a version that keeps the body as Sendfile on a TLS hop and reads the file in bounded chunks in the Body and ProxyBody write arms. That removes this function and the owned buffer. The current head stays unchanged until that is decided. I will post the result here.
There was a problem hiding this comment.
Decision: the head stays at 7e81605. A merge does not wait on me.
The cost of this read is about 0.55 ms per MiB with a warm cache (563 ms for 1 GiB), and the file stays in memory until the request ends. A pre-merge check on this head reported 0.1 to 1 s and 1024 MB for 1 GiB. The same file sent to https from the start costs 1 s and 2048 MB on main, because that path reads the whole file on the JS thread. So this fallback is not worse than the https upload path that exists today.
A read in bounded chunks in the write path is still the better end state. It applies to uploads that start on https too, so it is a separate change and not part of this PR. The PR body now lists this cost under Downsides.
| // Only an in-memory body is retried; a Stream body is consumed as it is written. | ||
| && matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_)) |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: an idempotent upload of a Bun.file() of 32 KiB or more on a stale keep-alive socket still fails with ConnectionClosed, while a smaller file is retried transparently. The retry predicate at src/http/lib.rs:2187 still admits only HTTPRequestBody::Bytes even though this PR makes a Sendfile body replayable (the write cursor now lives in state.sendfile, so original_request_body stays pristine). Fix: let the keep-alive retry restart Sendfile bodies too, i.e. match Bytes() | Sendfile() there, so the same sibling site as do_redirect uses the pristine body; the PR says this site was left as is on purpose, but the reason given (the body is consumed as it is written) no longer holds after this change.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user calls fetch(url, { method: "PUT", body: Bun.file(path) }) with a file of st_size >= 32 KiB to a plain http:// origin, so src/runtime/webcore/fetch.rs:1629 hands the HTTP thread an HTTPRequestBody::Sendfile. The client picks a pooled keep-alive socket that the server has already closed; on_close runs with in_progress true and response_stage before Body. At src/http/lib.rs:2187 the retry requires matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_)), so the Sendfile request falls through to self.fail(crate::Error::ConnectionClosed) at src/http/lib.rs:2202 and the fetch rejects. A file below 32 KiB is a Bytes body and is silently retried by self.start(body) at src/http/lib.rs:2198. The base branch fails the same way, but on the base the exclusion was justified because SendFile::write mutated original_request_body; after this PR InternalState::init seats a separate cursor in state.sendfile (src/http/InternalState.rs:159-161) and do_redirect already replays the pristine Sendfile through request_body_for_redirect (src/http/lib.rs:2713).
Verification: An idempotent fetch with a Bun.file() body of st_size >= 32 KiB to a plain http:// origin is assigned a pooled keep-alive socket that the server has already closed. on_close only retries if matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_)) (src/http/lib.rs:2187); a Sendfile body falls through to self.fail(crate::Error::ConnectionClosed) at line 2203.
There was a problem hiding this comment.
Correct. The old reason does not hold for Sendfile now that the send cursor lives in state.sendfile. The behavior at this site is the same as on main. I am deciding this together with the thread on buffer_sendfile_body_for_tls, and I will reply here with the outcome.
There was a problem hiding this comment.
Decision: not in this PR. The head stays at 7e81605, so a merge does not wait on this. A retry for Sendfile bodies at this site is a follow-up. The PR body lists it under Downsides.
A Bun.file() body of 32 KiB or more is sent with sendfile(2). do_redirect only replayed HTTPRequestBody::Bytes, so the redirected request went out with Content-Length: 0 and an empty body, and resolved with the next hop's status as if the upload had succeeded. Keep state.original_request_body untouched by the send path (the sendfile cursor now lives in InternalState, next to the Bytes cursor) so a redirect can hand a Sendfile body to start() again. A hop that lands on a TLS socket or a CONNECT tunnel cannot sendfile; start_ reads the file range into a client-owned buffer and sends it as Bytes, which is also what a request built for an https:// URL does up front. That same check replaces the panic hit when an https:// proxy from the environment was applied to a request that had already picked sendfile.
7e81605 to
1057464
Compare
|
The head moved from 7e81605 to 1057464. Main changed Checks on the rebased debug build: |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/http/SendFile.rs— pre-existing: a fetch whose Bun.file() body was truncated on disk between redirect hops now hangs for the 300 s idle timeout instead of resolving. SendFile::write treats a zero-byte sendfile(2) with bytes still owed as Status::Done at src/http/SendFile.rs:73-75, so the replayed plain-http hop declares the original Content-Length and then stops short. Fix: when sendfile returns 0 (or success with sbytes == 0) while remain > 0, return Status::Err(crate::Error::RequestBodyLengthMismatch) so the hop fails fast like the stream path at src/http/lib.rs:3165, on the Linux, FreeBSD and macOS arms alike (SendFile.rs:73, :103, :134). The TLS hop already handles this by truncating in read_to_vec; only the plain-socket replay is left inconsistent.Why this was flagged
A user calls fetch("http://...", { method: "POST", body: Bun.file(path) }) with a file of 32 KiB or more, the first hop answers 307, and the file is truncated before hop 2 starts. src/http/lib.rs:2713 replays the pristine Sendfile cursor and InternalState::init seats it at src/http/InternalState.rs:160-167, so hop 2 sends Content-Length: content_size from the original size. In the Body arm at src/http/lib.rs:3492 SendFile::write runs sendfile(2); once the shortened file hits EOF the syscall returns 0 with self.remain > 0, and src/http/SendFile.rs:73-75 returns Status::Done, setting request_stage = Done at src/http/lib.rs:3495. The server keeps waiting for the owed bytes, so the request sits until IDLE_TIMEOUT_SECONDS (300, src/http/lib.rs:313) fires and the fetch rejects with Timeout. On the base branch hop 2 went out with Content-Length: 0 and resolved 200 immediately, so the hang on this path is new, while the same Done-on-short-read exists on the base for a truncation during the first hop (pre-existing).
Verification: src/http/SendFile.rs:73-75 reports a zero-byte sendfile(2) with remain > 0 as Done; the FreeBSD/other-unix arms at :103-106 and :134-137 do the same. src/http/lib.rs:2713 forwards the pristine Sendfile body, and the Body arm at lib.rs:3492-3497 maps Status::Done to request_stage = Done. The base branch already hangs by this route on a single-hop truncation, so this is pre-existing.
|
On the review finding about a file that is truncated between hops: that case is real and it is listed under Downsides in the description. I reproduced it on a debug build of this PR (hop 2 sends The fix belongs in |
|
Status: this fix is included in #46528, as I ran the repro of this PR and a grid of 40 cells (5 sizes, http or https target, 307 or 308, The two limits in this PR's Downsides (the keep-alive retry of a file body, and a file that shrinks between hops) are the same on #46528. They are listed with tests in #46528 (comment). |
Problem
fetch()with aBun.file()body of 32 KiB or more follows a 307/308 withContent-Length: 0and no body, then resolves 200.HTTPClient::do_redirect(src/http/lib.rs) replays onlyHTTPRequestBody::Bytes. A file this size isSendfile.http_proxy=https://...the same upload aborts:panic: sendfile is only supported without SSL. This code should never have been reached!Fix
InternalState(the state of one attempt) gets asendfilesend cursor.original_request_bodystays unchanged. A redirect gives it tostart()again.start_()reads the file intobuffered_sendfile_bodyand sendsBytes.test/js/web/fetch/fetch-redirect.test.ts(9 of 13 new cases fail on main),test/js/bun/http/proxy.test.ts. More in Notes.Background
Sendfileis a request body thatsendfile(2)copies from an fd to a plaintext socket. It cannot go through TLS.Downsides
on_closestill skipsSendfilebodies.Notes
Repro (before: hop 2 gets 0 bytes. After: 32768.)
Before (bun 1.4.0 and current main):
After:
Tests
fetch-redirect.test.ts: 307/308/301/302 with POST, PUT, PATCH, DELETE. The 303 and 301/302 POST change to GET (same as before). ABun.file().slice()window across a chain of 3 hops. A chain http, https, https. A server that redirects while a 4 MiB upload is in flight.proxy.test.ts: two subprocess tests with a proxy from the environment. On main the TLS proxy case aborts with the panic above. The case that redirects into a CONNECT tunnel uploads 0 bytes.fetch-file-upload,fetch-compress,fetch-keepalive,fetch-url-after-redirect,fetch-http2-client,fetch-http3-client.Cost of the TLS fallback read
SendFile::read_to_vec(allocate, zero-fill,preaduntil full). Warm page cache, 5 runs for each size, Linux x64. It is not a run of a release build of this PR.https://reads the whole file on the JS thread before it sends (fetch.rs).Cost for a request with no file body
InternalState400,HTTPClient1680,AsyncHTTP2208 bytes. This head: 440, 1744, 2272.sendfilecursor and 24 for the emptybuffered_sendfile_body. From the diff, that path has no new allocation and no new syscall. I did not measure those two.File that shrinks
Content-Length.SendFile::writereportsDonewhensendfile(2)returns 0 with bytes still owed. The server then waits for the rest of the body.Content-Length: 0.Content-Length: 4194304and then 65536 bytes. The fetch is still pending after 8 s (3 of 3 runs). A pre-merge check saw the same.Other
The head is 1057464, a rebase of 7e81605 onto main bc7a813 (one import conflict in
InternalState.rs). The numbers in these notes were taken on 7e81605.The fd behind a
Sendfilebody stays valid across hops. TheFetchTaskletowns it and closes it after the final result.The running
HTTPClientis a bitwise copy of the JS-thread original. It freesbuffered_sendfile_bodyin the two teardown sites (AsyncHTTP.rs,HTTPThread.rs), afterstate, which borrows it.A plain
http://proxy from the environment keepssendfile(2).fetch: stream FormData bodies that contain Bun.file() parts #35792 adds redirect replay for streaming FormData bodies and touches the same
do_redirectlines.Branch
farm/89990154/fetch-sendfile-env-proxyhas a variant that selectssendfileonly after the environment proxy is known. It cannot help a redirect to https, so it is not in this PR.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-redirect.test.ts