fetch: reject when a Bun.file() request body is truncated mid-upload - #36212
fetch: reject when a Bun.file() request body is truncated mid-upload#36212robobun wants to merge 11 commits into
Conversation
WalkthroughChangesFile uploads now detect premature EOF across Unix Request body truncation handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:25 PM PT - Jul 28th, 2026
❌ @robobun, your commit bf82e3b has some failures in 🧪 To try this PR locally: bunx bun-pr 36212That installs a local version of the PR into your bun-36212 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/fetch-file-upload.test.ts`:
- Line 203: Update the Promise wrapping server.listen in the fetch-file-upload
test to reject on net.Server error events while resolving on successful
listening. Wire the server’s error event to the Promise rejection so bind
failures fail the test deterministically instead of hanging or crashing.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91c42716-e9d1-4588-a3de-5e05748d0409
📒 Files selected for processing (4)
src/http/SendFile.rssrc/http/error.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/bun/http/fetch-file-upload.test.ts
There was a problem hiding this comment.
LGTM — the remaining inline comment is a minor test-hygiene nit (dead AbortSignal.timeout after the per-test timeout was dropped); it doesn't affect correctness and the test still fails-before on a regressed build via the framework timeout.
Extended reasoning...
Overview
Four files: a 3-line guard added identically to each of the three POSIX sendfile(2) backends in src/http/SendFile.rs (return Status::Err(RequestBodyTruncated) when the syscall succeeds with remain > 0), a new enum variant + name() arm in src/http/error.rs, a new match arm mapping it to a user-facing message in FetchTasklet.rs, and one new POSIX-gated regression test in fetch-file-upload.test.ts.
Security risks
None. No untrusted-input parsing, no auth/crypto/permissions. The change tightens an early-EOF detection on a client-side upload path — it turns a hang into a rejection. The error variant follows the exact pattern of the adjacent RequestBodyNotReusable.
Level of scrutiny
Low-to-medium. The native change is mechanical and I verified the sole caller at src/http/lib.rs:3378-3385 — the existing Status::Err arm already calls close_and_fail, so the new return value flows through the same path every other sendfile error takes. The three backend edits are byte-identical. The error plumbing (enum variant, name(), FetchTasklet message) mirrors sibling variants exactly.
Other factors
This PR has been through four rounds of review feedback (comment-cop, two of my prior passes, CodeRabbit), all addressed in commits f8d8cba, 7937e85, 5941c52, and eaa1790. The test now buffers to the CRLF boundary, wires socket error/close to reject the awaited promises, and rejects on server.listen failure. The one new nit — AbortSignal.timeout(10_000) becoming unreachable after the 15s per-test override was removed — is a consequence of my own earlier suggestion and is purely cosmetic: on a regressed build the 5s framework timeout still produces a clean fail-before, so the test remains valid either way.
There was a problem hiding this comment.
LGTM — all four rounds of review feedback have been applied and the current diff looks correct.
What was reviewed:
SendFile::writeon all three POSIX backends: the newremain > 0guard is only reachable whensendfile(2)succeeds with early EOF; partial-progress and EAGAIN paths still fall through toStatus::Againunchanged.Status::Errroutes toclose_and_failinsrc/http/lib.rs:3382, so the socket is closed and the promise rejected.- The test now buffers to
\r\n\r\n, wires socketerror/closeand server listenerrorto the awaited promises, and has no dead AbortSignal or per-test timeout.
Extended reasoning...
Overview
Four files touched: src/http/SendFile.rs adds a 3-line remain > 0 guard inside the errcode == SUCCESS arm on each of the Linux, FreeBSD, and macOS sendfile(2) backends; src/http/error.rs adds the RequestBodyTruncated variant and its name() mapping; src/runtime/webcore/fetch/FetchTasklet.rs adds the user-facing message arm; and test/js/bun/http/fetch-file-upload.test.ts gains a POSIX-gated regression test.
Correctness of the native change
On Linux the outer block is entered only when errcode != SUCCESS || remain == 0 || val == 0, so reaching errcode == SUCCESS with remain > 0 implies val == 0 — early EOF. A partial-progress success (0 < val < remain) does not enter the block and still returns Status::Again. On macOS/FreeBSD, errno == 0 with sbytes < len is the documented EOF signal; a full send-buffer returns EAGAIN and is handled by the existing arms. So there is no path where a previously-working upload now fails — the change strictly converts a hang into a rejection. The Status::Err arm at src/http/lib.rs:3382 already calls close_and_fail, so no caller changes were needed.
Security risks
None. No untrusted input parsing, no new allocation, no change to TLS/auth paths. The only behavioral change is that a fetch() promise which previously never settled now rejects with a descriptive error and the socket is closed.
Level of scrutiny
Medium. This is HTTP-client hot-path native code, but the change is a 3-line guard replicated identically across three #[cfg] blocks, placed inside an existing branch that was already terminal (return Status::Done). The error-enum and message additions are pure boilerplate matching neighboring variants exactly.
Other factors
This PR has been through four review iterations (comment-cop, two claude[bot] rounds, coderabbit), all resolved: the paragraph comment was removed, the test now buffers the request head to the CRLF boundary, wires every socket/server failure event to reject the awaited promise, drops the redundant per-test timeout, and drops the unreachable AbortSignal and its vacuous assertion. The author reports 7/7 pass under debug+ASAN in ~466 ms. The change mirrors the server-side short-sendfile handling landed in #34185.
When fetch() is given body: Bun.file(path), the HTTP client stats the file, writes Content-Length: <stat size>, and streams the body via sendfile(2). If the file is truncated while the upload is in flight, sendfile(2) returns 0 with bytes still owed. SendFile::write reported this as Status::Done, so the request stage flipped to Done and the client sat idle waiting for a response to a request the server was still waiting to finish reading. fetch() never settled and the origin held a half-open connection until its own idle timeout. Treat sendfile EOF with remain > 0 as RequestBodyTruncated on all three POSIX backends; the existing Status::Err arm closes the socket and rejects the fetch promise.
2ab7c15 to
07bb987
Compare
There was a problem hiding this comment.
LGTM — all four rounds of review feedback are applied and the bug hunter found nothing new.
What was reviewed: the three remain > 0 guards in SendFile::write — on Linux the branch is only reachable when val == 0 (input EOF) and on macOS/FreeBSD only when errcode == SUCCESS, so no partial-write false positive; the caller at src/http/lib.rs:3382 routes Status::Err through close_and_fail which rejects the promise and closes the socket; runtime/error.rs needs no matching variant because it wraps bun_http::Error transparently and delegates name(); the test's gotHead.reject in close/error after resolve is a no-op on an already-settled resolver so no unhandled rejection.
Extended reasoning...
Overview
Four files: src/http/SendFile.rs (+9 lines across three cfg-gated POSIX branches), src/http/error.rs (+3, new enum variant + name() arm), src/runtime/webcore/fetch/FetchTasklet.rs (+3, error-message arm), and test/js/bun/http/fetch-file-upload.test.ts (+80, one new POSIX-gated test). The behavioral change is narrow: when sendfile(2) returns success with self.remain > 0, return Status::Err(RequestBodyTruncated) instead of Status::Done. Previously this path caused fetch() to hang forever with a half-sent body; now it rejects with a clear error and closes the connection. This is the client-side mirror of #34185.
Security risks
None. No new input parsing, no auth/crypto, no user-controlled sizes. The only new code path converts a hang into a rejection.
Level of scrutiny
Moderate — this is production HTTP-client code, but the change is a 3-line guard replicated across three platform branches, placed immediately before an existing return Status::Done. I traced the Linux branch's outer condition (errcode != SUCCESS || remain == 0 || val == 0): the only way to reach the new guard with errcode == SUCCESS and remain > 0 is val == 0, which per sendfile(2) is input-file EOF — so there is no false-positive on a normal partial write (that falls through to Status::Again). On macOS/FreeBSD, SUCCESS with sbytes < len likewise means the source hit EOF; a would-block partial write returns EAGAIN and takes the fallthrough. The caller at lib.rs:3382-3384 handles Status::Err via close_and_fail, which is the existing error path for every other sendfile failure.
Other factors
I have reviewed this PR three times previously (header buffering to CRLF, dropping the per-test timeout, wiring socket error/close to reject, and dropping the unreachable AbortSignal); coderabbit added one (server listen error rejection). All five are visible in the current diff and every inline thread is resolved. The author reports 7/7 pass under debug+ASAN in ~466 ms. The i64::MAX clamp on adjusted_count means the macOS/FreeBSD branch cannot spuriously report truncation for files smaller than 9 EB. runtime/error.rs wraps bun_http::Error via #[from] and delegates name(), so no parallel variant is needed there.
There was a problem hiding this comment.
LGTM — all four earlier review nits (header framing, per-test timeout, socket error wiring, unreachable AbortSignal) are addressed and no new issues turned up this pass.
What was reviewed:
SendFile::writeon all three POSIX backends: theremain > 0guard sits inside the existingerrcode == SUCCESSbranch, so it only fires on early EOF (Linuxval == 0, macOS/FreeBSD shortsbyteswith SUCCESS); non-blocking partial writes still return via EAGAIN →Status::Again.Status::Errrouting inon_writable(src/http/lib.rs:3382) already callsclose_and_fail, so the promise rejects and the socket closes — no new plumbing needed.- New
Error::RequestBodyTruncatedvariant +name()arm + FetchTasklet message follow the siblingRequestBodyNotReusablepattern exactly; no other exhaustive match sites need updating. - Test:
gotHeadis resolved beforeclose/errorfire, so the latergotHead.reject(...)in those handlers is a harmless no-op on the happy path.
Extended reasoning...
Overview
The PR fixes a hang in fetch(url, { body: Bun.file(path) }) when the source file is truncated mid-upload. SendFile::write previously returned Status::Done whenever sendfile(2) succeeded, even if remain > 0 — the caller then set request_stage = Done and the client waited forever for a response the server would never send (it was still waiting for the rest of the advertised Content-Length). The fix inserts a remain > 0 check inside the SUCCESS branch on all three POSIX backends and returns a new Error::RequestBodyTruncated, which the existing Status::Err arm in on_writable routes through close_and_fail. A new enum variant, name() mapping, and FetchTasklet user-facing message complete the wiring. A POSIX-gated regression test in fetch-file-upload.test.ts reproduces the truncation against a paused net.Server.
Security risks
None. This is a client-side upload error path; no untrusted input parsing, auth, or crypto is involved. The change strictly converts a silent hang into a rejected promise.
Level of scrutiny
Moderate. The production diff is ~15 lines across three platform-#[cfg] blocks plus mechanical enum/message additions. The kernel-interface reasoning is subtle (when does sendfile return SUCCESS with a short write?) but the fix is conservative: even if some platform had a non-EOF reason to short-write with SUCCESS, erroring out is strictly better than the previous behavior of marking the request done and hanging indefinitely. The PR description cites #34185 as the server-side mirror of the same pattern, so there is in-repo precedent.
Other factors
This PR has been through four rounds of review feedback (three from me, one from CodeRabbit), all applied and marked resolved: header-framing accumulation before asserting, dropping the redundant per-test timeout, wiring socket error/close to reject the awaited promise, wiring server.listen errors to reject, and removing the unreachable AbortSignal.timeout(10_000) + its vacuous assertion. The resulting test awaits observable conditions, cleans up in finally, uses tempDir, and has no sleeps. The bug-hunting system found nothing this run. The change is small, focused, and obviously correct in intent — turning a hang into a diagnosable error.
|
The remaining reds are unrelated flakes that passed on isolated retry plus one test marked pre-existing on main, none of which this diff touches. Ready for review. |
On macOS the server net.Socket does not observe the client's RST via the read side alone; writing a byte (as a real origin would when trying to respond) surfaces ECONNRESET and the 'close' event on both platforms.
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed and the final revision is clean.
What was reviewed:
- The three
sendfile(2)branches inSendFile.rs: verified each only reacheserrcode == SUCCESS && remain > 0on genuine early-EOF (Linuxval == 0; macOS/FreeBSDsbytes < lenwith rc 0), and that thei64::MAXclamp onadjusted_countcannot spuriously trip the new check for realistic file sizes. - Confirmed
Status::Errflows through the existingon_writable→close_and_failpath (src/http/lib.rs:3382) with no additional plumbing needed. - Checked for other
http::Errormatch sites (retry/conversion tables) that would need a new arm — none found beyond the two updated. - Test: the
gotHead.rejectcalls in theclose/errorhandlers are harmless no-ops afterresolve(), so the post-assertion socket close doesn't produce an unhandled rejection.
Extended reasoning...
Overview
This PR fixes a hang in fetch(url, { body: Bun.file(path) }) when the source file is truncated mid-upload. It touches four files: src/http/SendFile.rs (+9 lines across three #[cfg] blocks), src/http/error.rs (new RequestBodyTruncated variant + name() arm), src/runtime/webcore/fetch/FetchTasklet.rs (user-facing message for the new variant), and a new ~85-line POSIX-gated test in test/js/bun/http/fetch-file-upload.test.ts.
Security risks
None. This is error-path handling for an upload edge case (log rotation / racing writer). No auth, crypto, permissions, or untrusted-input parsing is touched. The change strictly converts a silent hang into a diagnosable rejection.
Level of scrutiny
Low-to-medium. The production change is 15 lines total, all following the exact pattern of neighboring code (the new error variant sits next to RequestBodyNotReusable in every table; the SendFile.rs guards nest inside existing errcode == SUCCESS branches). The PR description cites #34185 as the server-side mirror, so this is applying an already-accepted design to the client side. The only subtlety is per-platform sendfile(2) semantics, which I traced: on all three backends, errno == 0 with bytes remaining can only mean early EOF — partial writes due to socket backpressure surface as EAGAIN, which takes the Status::Again path.
Other factors
This PR has already been through five rounds of my own inline review (buffer-to-framing, wire error/close events, drop per-test timeout, drop dead AbortSignal, macOS write-to-surface-close), all applied and marked resolved. The author confirmed 10/10 passes on darwin-aarch64 and 5/5 on linux-x64 debug+ASAN after the final bf82e3bd fix for the macOS socketClosed hang. The remaining CI red in build #84401 is a darwin-x64 build-cpp agent error unrelated to the diff. The bug-hunting system found nothing new in this run. Given the small scope, the iteration history, and cross-platform CI verification, I'm confident this doesn't need further human review.
…36309) ## Symptom On macOS, a `net.Server` connection never emits `'end'`, `'error'`, or `'close'` after a Bun `fetch()` on the other side aborts mid-upload (AbortSignal, or a client-side failure such as `RequestBodyTruncated` from #36212). `netstat` shows the server socket staying `ESTABLISHED` indefinitely; only writing to it provokes a fresh RST and surfaces `ECONNRESET`. On Linux the same sequence emits `'error' ECONNRESET` then `'close'`. ```js import net from "node:net"; const events = []; const server = net.createServer(s => { s.on("data", () => {}); s.on("end", () => events.push("end")); s.on("error", e => events.push("error:" + e.code)); s.once("close", () => events.push("close")); }); await new Promise(r => server.listen(0, "127.0.0.1", r)); const ac = new AbortController(); fetch(`http://127.0.0.1:${server.address().port}/`, { method: "POST", body: new Uint8Array(16 * 1024 * 1024), signal: ac.signal, }).catch(() => {}); await new Promise(r => setTimeout(r, 50)); ac.abort(); await new Promise(r => setTimeout(r, 3000)); console.log(events); // darwin: [] linux: ['error:ECONNRESET','close'] ``` ## Cause `close_and_fail` closes the socket via `terminate_socket`, which arms `SO_LINGER{1,0}` so `close()` sends an RST. XNU's `tcp_drop` builds that RST with `th_seq = snd_nxt`. With a request body still in the kernel send buffer, `snd_nxt` can sit at (or past) the receiver's `rcv_nxt + rcv_wnd`, and XNU's receiver-side acceptability check `SEQ_LT(th_seq, last_ack_sent + rcv_wnd)` drops it silently as out of window. Linux's `tcp_send_active_reset` uses `tcp_acceptable_seq()` which clamps into window, so the RST always lands there; Windows delivers it too. There is no `setsockopt` that retargets XNU's RST sequence or discards the send buffer without transmitting. ## Fix Add `HTTPContext::fail_socket` (mark dead + `CloseKind::FastShutdown`: TLS fast-shutdown, TCP FIN) and, on macOS only, route `close_and_fail` through it when the request carries a body (`original_request_body.len() > 0`, which covers `Bytes`/`Sendfile`/`Stream`). A FIN is in-order, so a reading peer drains whatever body bytes the client's kernel had already queued and then sees end-of-stream. Linux and Windows keep the RST: they deliver it in window, and a FIN would put the aborting client into `TIME_WAIT` for every aborted upload (on Windows the 16k-port dynamic range has no loopback reuse, so abort churn exhausts the ephemeral-port pool). `on_timeout`, the H2-session timeout arm, and `ClientSession::fail_all` stay on RST on every platform. ## Verification New test in `fetch-abort-stream-body.test.ts`, gated to macOS: a `net.Server` records read-side events while eight `fetch()` POSTs with 16 MB bodies are aborted mid-upload; it asserts every connection ends in `['end','close']`. An unpatched macOS build hangs on the first `await socketClosed.promise`. `rust:check-all` passes across all targets. `bun-server.test.ts` abort-signal tests, `fetch.tls.test.ts`, `fetch-keepalive.test.ts`, `fetch-http2-client.test.ts`, `fetch-http2-adversarial.test.ts`, `fetch-abort-queued.test.ts`, `fetch-abort-socket-close-race.test.ts`, `body.test.ts`, `proxy.test.ts`, `node-http-server-abort-events.test.ts`, and the Node `test-http-aborted.js` / `test-http-client-abort.js` / `test-net-*reset*.js` parallel tests pass under `bun bd` on Linux and Windows (unchanged behavior there). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-abort-stream-body.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
There was a problem hiding this comment.
LGTM — the earlier test-hygiene nits are all applied and CI is green across lanes.
What was reviewed:
SendFile::write: the newremain > 0guard is only reachable on theerrno == SUCCESSbranch; Linux partial writes (val > 0) and BSD/macOSEAGAINpartial writes still fall through toStatus::Again, so no false positives on backpressure.- The sole caller in
src/http/lib.rsalready routesStatus::Errthroughclose_and_fail, so the fd/socket cleanup path is the pre-existing errno path. http::Error::RequestBodyTruncatedis threaded through bothname()and the FetchTasklet message match; no other exhaustive match onhttp::Errorneeded updating.- Test: header framing,
'error'/'close'wiring, listen-error rejection, deadAbortSignalremoval, and the macOS write-to-surface-RST tweak all landed as discussed.
Extended reasoning...
Overview
The PR fixes a hang in fetch(url, { body: Bun.file(path) }) when the source file is truncated mid-upload. SendFile::write previously returned Status::Done on any sendfile(2) success, so early EOF (0 bytes with remain > 0) left the client parked forever with an under-length body on the wire. The fix adds a 3-line if self.remain > 0 { return Status::Err(RequestBodyTruncated) } guard inside the errno == SUCCESS branch on all three POSIX backends (Linux, FreeBSD, macOS/other-Unix), adds the RequestBodyTruncated variant to http::Error and its name() table, and maps it to a user-facing message in FetchTasklet. A POSIX-gated regression test in fetch-file-upload.test.ts reproduces the truncation against a paused net.Server.
Security risks
None. This is a client-side error path; no untrusted input parsing, no auth/crypto surface. The only new user-observable behavior is that a previously-hanging promise now rejects with a diagnostic error.
Level of scrutiny
Low-to-medium. The native change is 9 lines of identical guard logic across three #[cfg] blocks, inserted inside an existing errcode == SUCCESS arm — the surrounding control flow is untouched. I traced each platform's semantics: on Linux, partial writes return val > 0 so the outer || val == 0 gate keeps them on the Status::Again path and only true EOF (val == 0, remain > 0) reaches the new guard. On FreeBSD/macOS, partial writes return EAGAIN (outer errcode != EAGAIN gate is false), so again only SUCCESS with a short sbytes — i.e. EOF — reaches the guard. The single caller at src/http/lib.rs:3415 already handles Status::Err via close_and_fail, so cleanup is the same as any other sendfile errno. This mirrors the server-side change in #34185.
Other factors
The test went through five rounds of applied feedback (buffer to \r\n\r\n before asserting, drop the per-test timeout, wire 'error'/'close' to reject gotHead, wire listen 'error', drop the unreachable AbortSignal.timeout) plus a macOS-specific fix (socket.write("\r\n") to surface the RST on the server's write side). All threads are resolved. The author reports the file green on every lane in build #84637 including darwin, with the remaining reds being unrelated retry-passing flakes. Fail-before is the 5 s framework timeout; pass-after is ~470 ms under debug+ASAN.
2 participants
fetch(url, { body: Bun.file(path) })stats the file, writesContent-Length: <stat size>, and streams the body viasendfile(2). If the file is truncated while the upload is in flight (log rotation, non-atomic rewrite, a writer racing the uploader),sendfile(2)returns 0 with bytes still owed.SendFile::writereported this asStatus::Done, soon_writableflippedrequest_stagetoDoneand the client sat idle waiting for a response to a request the server was still waiting to finish reading.fetch()never settled (no resolve, no reject) and the origin was left holding a half-open connection until its own idle timeout. Only a caller-suppliedAbortSignalescaped.Reproduction
The wire carries
Content-Length: 83886080followed by ~10 MB of body, then silence.Cause
src/http/SendFile.rsreturnsStatus::Donewheneversendfile(2)completes witherrno == 0, regardless ofself.remain. On early EOF (val == 0on Linux,sbytes < lenwitherrno == 0on macOS/FreeBSD)remainis still positive but the caller inHTTPClient::on_writablejust setsrequest_stage = Donewithout inspecting it.Fix
When
sendfile(2)succeeds withremain > 0, returnStatus::Err(Error::RequestBodyTruncated)on all three POSIX backends. The existingStatus::Errarm inon_writablecallsclose_and_fail, which closes the socket and rejects the promise:This is the client-side mirror of the
Bun.serveshort-sendfile change in #34185.Test
test/js/bun/http/fetch-file-upload.test.tsgains a POSIX-gated test that starts a 32 MBBun.fileupload against anet.Serverthat pauses on first data, truncates the file once the kernel send buffer has filled, then resumes. On main the fetch only settles via the 10 sAbortSignal.timeout(the assertion rejectsTimeoutError); with the fix it rejects withRequestBodyTruncatedin ~500 ms and the server socket seesclose.no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/fetch-file-upload.test.ts