Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Status: ready for review.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a substantial test rewrite (~650→370 lines) that introduces a novel loopbackClock() pacing primitive relying on loopback FIFO ordering + immediate scheduling, and removes the ASAN serialization gate from #29569 whose original reason wasn't recorded — a human look would be worthwhile before this pattern is adopted elsewhere (e.g. #32887).
What was reviewed:
- Mapped every scenario on main to the new tables — all 12 chunked cases plus the tiny-send-buffer test are present with the same wire bytes, and assertions are strictly stronger (exact status/body/
seenvstoContain). - Checked
loopbackClock()for resource cleanup (Symbol.disposecloses socket + listener;usingat all call sites) and that the premise test guards against silent coverage loss if event-loop ordering changes. - Verified the tiny-send-buffer rewrite fixes the throw-in-
close-callback hang and now wireserrortoreject.
Extended reasoning...
Overview
This PR rewrites test/js/bun/http/http-server-chunking.test.ts from 12 subprocess-spawning tests into in-process Bun.serve tests driven by a shared exchange() helper, plus a rewrite of the tiny-send-buffer test. The goal is CI speed on the ASAN lane (67s → expected much less). No production code is touched.
Security risks
None. Test-only change; no auth, crypto, or user-facing surface. The scenarios themselves are security regression tests (request smuggling, chunk-extension DoS, Content-Length aliasing) and the new assertions are strictly tighter than main's (seen is now an exact list rather than <= 1, body is exactly "" on rejection rather than not.toContain("/smuggled")), so coverage is not weakened.
Level of scrutiny
Medium-high. While test-only, this is not a mechanical change: it introduces loopbackClock(), a synchronization primitive that replaces setTimeout(50) pacing by round-tripping a byte through a spare loopback connection and awaiting one immediate. The mechanism depends on two event-loop-ordering assumptions (loopback delivers in write order across sockets in the same poll; an immediate queued during dispatch runs after the whole poll batch). The PR guards this with a dedicated premise test and extensive rerun data (280/280, 210/210 on debug+ASAN; 420/420 on release; passes pinned to ~1/3 of one core), which is convincing — but it's a design decision a maintainer should sign off on, especially since the PR notes #32887 would adopt the same helper.
Other factors
- The
test.concurrentIf(!isASAN)→test.concurrentchange removes a gate added in #29569. The PR's reasoning (the gate existed for concurrent ASAN subprocess cost, now gone) is plausible and rerun-tested, but the original reason wasn't recorded, so a maintainer may want to confirm. - The tiny-send-buffer test drops the
written > 1sanity check onsocket.write(buf, off, 1)and changes completion detection from counting-in-datatoConnection: closeon the last request. Both are reasonable simplifications; the old throw-in-closefailure mode (which hung rather than failed) is fixed. - Resource cleanup looks correct:
await using server,using clock,using socket, andloopbackClock'sSymbol.disposecloses both the connect socket and listener. - No prior human reviews on the timeline; only a rate-limited CodeRabbit stub.
|
Two data points for the points raised above, now that the first CI run is mostly in:
|
|
Updated 11:04 AM PT - Aug 12th, 2026
✅ @robobun, your commit a587633e4deab1338d25565784ede2f675b1fb8b passed in 🧪 To try this PR locally: bunx bun-pr 37818That installs a local version of the PR into your bun-37818 --bun |
|
Closing this PR because it has been inactive for more than 90 days. |
Problem
test/js/bun/http/http-server-chunking.test.tstook 67s on the debian 13 x64-asan lane (build 92779), one of the slowest non-integration files there. Nothing in it was failing; this PR only changes how the tests are built.bunto host the server, and under ASAN they were gated to run one at a time. That lane setsdetect_leaks=1, so every child paid a LeakSanitizer scan on exit (about 5s each), and the file's time was those scans added up.toContain("400")) that mostly did not look at what the handler received.Fix
Bun.servethrough oneexchange()helper and istest.concurrenton every lane. This is safe because no assertion was about exit codes or crash isolation; all of them were about the bytes the server wrote back, and every scenario still sends the same bytes.setImmediatemakes only that test fail.toEqualon the exact status line, the exact body after the headers, and the exact list of requests the handler saw, so smuggled requests and mid-body rejections are checked directly. The tiny-send-buffer workload is unchanged but now asserts exactly 20 responses and 20 parsed requests per connection instead of counting substrings.Ran 14 tests across 1 file. [312.00ms]against 66.68s for main;--rerun-eachruns went 280/280 and 210/210 on debug+ASAN and 420/420 on release.Background
<hex size>\r\n<data>\r\npieces ending in0\r\n\r\n. These tests end a read at awkward positions (between\rand\n, mid chunk-size line) and check the parser resumes, or answers 400 instead of treating the leftover bytes as a second, smuggled request.detect_leaks=1, so every bun subprocess pays a LeakSanitizer scan at exit. A test file that spawns one child per case pays it once per case, and more if the cases are serialized.setImmediateandBun.sleep(0)both run before the loop polls again, so they do not split writes; the loopback clock waits for an actual read.test.concurrentandtest.concurrent.eachlet bun:test overlap the tests within a file;test.concurrentIf(cond)is the conditional form the old file used. llhttp caps chunk extensions at 16 KiB per chunk independently ofmaxRequestBodySize, which is what the 413 case exercises.Original description
test/js/bun/http/http-server-chunking.test.tswas one of the slowest non-integration test files on the ASAN lane (67s on debian 13 x64-asan in build 92779). Nothing in it was failing; this only changes how the tests are built, not what they cover.Why it was slow
bunto host the server, and every test was declaredtest.concurrentIf(!isASAN), so on the ASAN lane the 12 children ran one after another. The ASAN lane's log for build 92779 shows where the time went: the file reportedRan 13 tests across 1 file. [66.68s], with the per-test progress dots landing about 5.1s apart, one per child. The CI runner setsASAN_OPTIONS=...detect_leaks=1...on that lane and the children inherit it, so each child pays a LeakSanitizer scan of a process that has run a server on exit. Locally the same child script exits in 0.46s with bun's defaultdetect_leaks=0and takes 5.4s withdetect_leaks=1, which matches the CI spacing. The same applies to any test that spawns one bun child per test on that lane; serializing them, as this file did, just adds the scans up.!isASANgate was added in test: speed up 16 more slow test files (concurrency, batched spawns, reduced sleeps) #29569 (test: run http-server-chunking serially under ASAN, no reason recorded). The tests never shared state; the only thing they shared was the cost of a dozen ASAN children exiting at once, so with no children left there is nothing to serialize.setTimeout(..., 50)/Bun.sleep(20), 50 to 100ms of sleeping per test.What changed
Bun.servethrough oneexchange(segments, options)helper, and everything istest.concurrenton every lane. The assertions never needed a subprocess: none of them was about exit codes or crash isolation, they were about the bytes the server wrote back.test.concurrent.eachtables (accepted input with the expected echoed body; rejected input with what the handler saw) plus the 413 case, which needsmaxRequestBodySize. Every scenario from main is still here with the same bytes on the wire; the rationale comments moved onto the table rows.loopbackClock()instead of a delay. After writing a segment,exchange()pushes a byte through a spare loopback connection to an in-process listener and waits for it to arrive, then for one immediate. Loopback delivers in order, so when that byte shows up the segment had reached the server's socket and was readable in the same poll, and the immediate lets that poll's batch (the server's read included) finish. It does this twice per segment: once so the server has read the segment, once more so whatever the server wrote back in reaction has been read on the client side. Once the server has answered or closed, the remaining segments are not written, so a rejecting server is never written to after it hung up (no RST race with the response bytes), while a server that wrongly kept a request going still receives the rest and gets caught answering 200.setImmediatedoes not work for this, which is worth knowing because it looks like it should: an immediate queued from ordinary JS runs before the loop's next poll, andBun.sleep(0)fires at the end of the tick that armed it, also before the next poll. Measured with a rawBun.listenreceiver, 200 of 200 write pairs paced by onesetImmediateor oneBun.sleep(0)coalesced into a single read; paced by the clock, 960 of 960 four-segment writes (32 connections at a time, debug+ASAN build) arrived as four reads. Timers with a nonzero delay do split, but only while the delay outlasts the tick, which is what the old 50ms was buying.Bun.listenas three reads. Without it the split cases would keep passing on coalesced input if the event loop ever changed shape; replacing the clock tick with a singlesetImmediatemakes exactly this test fail and all 13 others still pass.Assertions
Each scenario is now one
toEqualon{ status, body, seen }:statusis the exact status line (HTTP/1.1 400 Bad Request,HTTP/1.1 413 Payload Too Large,HTTP/1.1 200 OK) instead oftoContain("400")/toContain("413")/not.toContain("200")on the whole stream.bodyis exactly what followed the response headers: the echoed body on success, and""on rejection, which also proves no second response was ever written (main checkednot.toContain("/smuggled")).seenis the exact list of requests the handler got, with each one'sreq.text()outcome: the body on success, andAbortError: The connection was closed.on rejection, so a request that was rejected mid-body is shown both to have been rejected and to have had its pending body read settled rather than left hanging. The smuggling cases assertseenis exactly the first request (main allowed<= 1requests) and the Content-Length case asserts it is[], i.e. refused beforefetch()ran. Main's rejection cases mostly did not look at the handler at all.HTTP/1.1 200 OKstatus lines and that the handler saw exactly 20 requests with the right method and header set (main only counted200 OKsubstrings perdataevent and never looked at what was parsed; on a shortfall it threw inside theclosecallback, which hung the test instead of failing it). The last request on each connection carriesConnection: closeso the client reads until the server closes instead of counting insidedata.exitCode === 0,stderr === "") went away with the subprocesses. Nothing asserted the absence of "panic" or similar.This test's workload is unchanged and is now most of the file's remaining cost (about 1.75s of the post-startup time locally); #33622 deliberately kept it at this size, so I left it alone.
Timing
On the lane this was filed for (debian 13 x64-asan), as reported by
bun testin the job logs: build 92779 (main)Ran 13 tests across 1 file. [66.68s]; build 93136 (this PR)Ran 14 tests across 1 file. [312.00ms].Locally, same machine, same
bun bd(debug+ASAN,detect_leaks=0) binary,bun teston the file; "before" is the file as of main (9a543cc) run from the same checkout. The debug build makes the unchanged tiny-send-buffer test itself the dominant cost, which is why the local ratio is much smaller than CI's:Ran ... [..](4 and 5 runs)A trivial test file takes about 2.35s on this build (runner startup plus harness import), and the unchanged tiny-send-buffer test about 1.75s, so the 12 chunked scenarios went from roughly 5.2s of serial subprocess time to about 0.2s of concurrent in-process time. In CI the subprocess spawns are a much larger share of the 67s than they are here, so the saving there should be larger in proportion.
Flakiness
bun bd test <file> --rerun-each=20: 280/280; a later revision--rerun-each=15: 210/210 (debug+ASAN, all reruns of a file's concurrent tests interleaved).USE_SYSTEM_BUN=1 bun test <file> --rerun-each=30: 420/420 (release).The file stays
describe.if(isPosix)as on main. #32887 adds tests to the end of this file; they will need a small rebase onto the new structure, and itssetImmediatepacing has the coalescing problem described above, soexchange()would suit them as well.