Conversation
The sendfile header-tail test took about 60% of the file's time on a debug build. Most of that time was not part of the race it detects: - The server validated the 16MB x-pad header value again for each response. The test now builds the Headers object once. - The client copied each chunk and scanned it with indexOf. It now receives into one buffer and checks the pad with equals(). The client also sets SO_RCVBUF to 16KB. The small receive window makes each header write stall with a buffered tail, and it makes the window-update ACKs frequent. An unfixed build now fails the test in the first wave of clients, so each client runs 8 requests, not 16. The client checks more: the whole pad, the whole file body, the status line and Content-Length. The test requires the exact client output, an empty stderr and exit code 0. Other assertions in the file now check actual values: - The stress tests compare each response with the file, not with the first response. - The POST If-Modified-Since test uses a route that takes POST. The old route sent the POST to the fallback handler. - Content-Type checks use the exact value. - Range on POST, the slice window, If-Match and If-Unmodified-Since results check the status and the body. - The abort test checks for a DOMException named AbortError. - The late request body test checks stderr.
|
Status: ready for review.
|
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughChangesThe HTTP file-serving tests now use exact content, header, status, body, and error assertions. Conditional requests, ranges, streaming regressions, and concurrent HTTP file-serving tests
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
test/js/bun/http/bun-serve-file.test.ts— nit, pre-existing: the HEAD variant of "If-Match: matching tag on a file route with ETag → 200" still asserts nothing about the body, so a HEAD that leaks "Hello, World!" passes. Line 496 keepsif (method === "GET") expect(await res.text())...while every sibling in this describe was changed to the unconditional ternary form. Fix: assert the body unconditionally,expect(await res.text()).toBe(method === "GET" ? "Hello, World!" : ""), matching lines 505, 543 and 571.Extended reasoning...
The describe.each(["GET", "HEAD"]) block at lines 480-573 runs each test for both methods. The PR converted lines 505, 543 and 571 from status-only checks to
expect(await res.text()).toBe(method === "GET" ? "Hello, World!" : ""), so those tests now fail if a HEAD response carries a body. Line 496 was left as a conditional assertion: for method === "HEAD" no expect on the body runs at all. REVIEW.md lists conditional assertions among the vacuous patterns to hunt. Consequence relative to base: none (this line is unchanged), but it is the one site in the tightened block that still cannot fail for HEAD. The fix is the same one-line ternary the sibling sites use.Verification: pre-existing (nit). Triggering condition: the HEAD iteration of
describe.each(["GET", "HEAD"])at test/js/bun/http/bun-serve-file.test.ts:480. Line 496 is unchanged from base (base line 498):if (method === "GET") expect(await res.text()).toBe("Hello, World!");— for method === "HEAD" no assertion on the body executes, so a HEAD response carrying "Hello, World!" would pass this…
… tests Two tests in bun-serve-file.test.ts checked the body only for GET. They now use the same expression as the other tests in the block. fetch() reads no body for HEAD. The "" that these tests expect for HEAD shows only what the client exposes. A comment in the block says so.
|
Applied in 0ae7930. The flagged test and one more with the same pattern ( One correction to the premise, because it also applies to the sibling sites that this PR changed earlier. The unconditional form does not catch a HEAD response that leaks a body. So for GET these assertions are a real check. For HEAD, A leaked HEAD body is visible only on the wire, so it needs a raw-socket test like the |
|
Updated 7:16 AM PT - Sep 20th, 2026
✅ @robobun, your commit 1a19265c05ab467ef753baa32e1c08eb3db8e2a2 passed in 🧪 To try this PR locally: bunx bun-pr 43627That installs a local version of the PR into your bun-43627 --bun |
This branch has not been deployed
1 participant
Problem
bun-serve-file.test.tstakes 31 s on debian 13 x64-asan (build 118779).sendfile does not overtake a buffered response header tailis 14 s of a 24 s local debug run. Most of that is not the race. The server validates the 16 MBx-padheader for each response. The client copies and scans each chunk.If-Modified-Sincetest never reaches the file route.Fix
Headersonce. The client receives into one buffer, compares the pad withequals(), setsSO_RCVBUFto 16 KB, and sends 8 requests, not 16.bun bd test test/js/bun/http/bun-serve-file.test.tsgoes from 24.0 s to 12.7 s (medians of 3 alternating runs). Release: 9.3 s to 5.0 s.Background
Bun.servesends a file body of 1 MB or more withsendfile(2), directly to the socket. The headers go through a userspace buffer. Bun.serve: keep the first sendfile behind a buffered response header tail #37100 fixed a race where file bytes overtook the buffered tail of the headers.sendfilecall. A client read opens it with a window-update ACK.SO_RCVBUFsets the client's receive buffer. At 16 KB the header write always stalls, and eachrecv()sends a window update.Notes
Where the time went (debug ASAN build, local)
bun bd testThe CI row is one run each, on different days, so it confirms the direction and not an exact ratio. The new file also passes on a Windows 11 aarch64 machine (3 of 3 runs, 5.0 s, the same as before). The runs alternate old and new on one shared host. The host was busy for part of the session (load average 75 to 100). Six more alternating pairs under that load gave 36.6 s to 54.1 s for the old file and 20.2 s to 30.5 s for the new file.
On a debug build the old sendfile test spent about 70 ms of each request on
new Headers({ "x-pad": <16 MB> }), and about 75 ms of each request in the client onindexOfscans of the pad. Neither is part of the race.Detection on unfixed builds
I made scratch builds with the
get_buffered_amount()check inFileResponseStream::on_sendfileremoved (debug and release). They are not part of this PR. "Hit rate" is the share of requests in which the client saw a file byte before the header terminator.SO_RCVBUFSO_RCVBUF256 KBSO_RCVBUF64 KBSO_RCVBUF16 KB (this PR)Each figure comes from 18 to 24 client processes. A faster client alone made the release hit rate worse: the client drained so fast that the kernel often took the whole header block, and then no tail was buffered. The small window fixes that. With the debug log scope on, 48 of 48 requests had a buffered tail of 8.7 MB to 13.5 MB at the first
sendfilecall.Whole test against the unfixed builds:
The new test passes on the fixed builds in 27 of 27 debug runs and 21 of 21 release runs.
Assertion changes
Content-Length. The test requires the exact client output, an empty stderr and exit code 0. The first failure of any kind ends the client.ignores If-Modified-Since for non-GET/HEAD requests:/hello.txttakes only GET and HEAD, so the POST went to the fallback handler andnot.toBe(304)passed there. The test now uses/hello-blob.txt, checks that GET gives 304, and that POST gives 200 with the body.ignores Range for non-GET/HEAD methods:not.toBe(206)is now status 200, noContent-Range, and the full body.Content-Typechecks use the exact value, not a regular expression.If-Match,If-Unmodified-Since,If-None-Matchand pastIf-Modified-Sincetests now check the body for GET and for HEAD. Before, several checked only the status, and two checked the body only for GET. For GET this is a real check. For HEAD it is weak:fetch()reads no body for HEAD, so""shows only what the client exposes. I confirmed this with a raw server that sends 13 body bytes after a HEAD response head:res.text()still gives"". A body that a server leaks on a HEAD response needs a raw-socket test. This PR does not add one.DOMExceptionwith the nameAbortError.serves large fileuses a small helper that reports the first character that differs. It replaces 20 lines ofconsole.log.Not changed, and why
Response(Bun.file) does not double-close the fd on Windowsis most of the time on Windows (3.3 s of 5.1 s on a Windows 11 aarch64 machine). Its 160 rounds are its detection budget: io(windows): honor CLOSE_HANDLE in WindowsBufferedReader close path #33931 measured 5 of 15 fail-before runs. I did not reduce it.describe.todo("Range requests")block. This PR does not touch it.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-file.test.ts