test(fetch): speed up fetch.test.ts (~30s to ~12s release) and tighten its assertions - #38470
test(fetch): speed up fetch.test.ts (~30s to ~12s release) and tighten its assertions#38470robobun wants to merge 3 commits into
Conversation
Release build of this file goes from ~30s to ~12s, debug+ASAN in this container from 225s to 41s. Where the time went and what changed: - explicit-timeout idle test held every request for a fixed 10s; the server now holds them until the control request has been aborted by the 1s default, and the control is only sent once the explicit requests have been seen, so the check stays deterministic. - absolute-deadline idle test: /b now drips its body until /h has timed out and is completed afterwards, so the body re-arm check no longer depends on the sweep phase. Both idle tests run concurrently. - testBlobInterface compared buffers one byte at a time with a forced GC before and after every byte (~6300 Bun.gc(true) calls); compare the whole array after a GC instead. - the four RSS cases in "fetch should allow duplex" run concurrently. - very long redirect URL test runs 20 iterations instead of 100 and also asserts the redirected flag and the final body. Assertions: the idle tests assert the exact child output and an empty stderr, the Response life cycle test pipes and asserts both children's output, #3545 uses a local server and checks the request target instead of fetching example.com, the invalid header test asserts the TypeError and its message, and the 100-continue tests listen on port 0 instead of a hardcoded 8080.
|
Warning Review limit reached
Next review available in: 1 minute 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 |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit c4ccb03 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38470That installs a local version of the PR into your bun-38470 --bun |
|
Status: ready for review. CI for the current head (bf6aa70, build 96302) passed all 177 jobs that ran; the only things left in that build are the two darwin jobs sitting in the agent queue, as in the previous build. The remaining annotations are retried flakes in unrelated files. Measured locally on the same binaries before and after this change: bf6aa70 came out of a self-review: the long-redirect test's loop comment cited a pooling path that no longer exists (a 302 with a body has its connection closed since #33613), so the test now runs two rounds with a comment describing the reuse the second round actually exercises. The seven currently mergeable PRs that also touch this file (#37436, #37913, #35988, #37425, #34896, #38043, #29198) still merge cleanly on top of this branch. |
![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. The mechanical parts (port 0 for the 100-continue tests, toEqual in place of the per-byte GC loops, local server for #3545, it.concurrent on the isolated duplex RSS cases, piping the life-cycle fixtures) are straightforward improvements. The two idle-timeout test redesigns are the substantive part — they replace fixed holds with event-driven release keyed to the uSockets 4s sweep and rely on an ordering argument about when each socket's timer is armed relative to the control. The reasoning in the description is thorough and I couldn't find a hole in it, but it's subtle enough that a human familiar with the fetch client's idle-timeout path should confirm the rewritten tests still guard the same regressions.
Checked: the four duplex it.concurrent cases each spawn their own subprocess/servers with no shared state; the two idle tests are adjacent (so they actually overlap) and don't share HTTP-thread state (one runs in a child with BUN_CONFIG_HTTP_IDLE_TIMEOUT, the other uses an explicit per-request timeout in the test process); withoutAggressiveGC is only removed from this file's imports, not from harness; the 100→20 redirect loop still exercises pooled keep-alive reuse and now asserts redirected and the body.
Extended reasoning...
Overview
Test-only change to test/js/web/fetch/fetch.test.ts that speeds the file from ~30s to ~12s (release) and tightens several assertions. No production code is touched. The changes fall into three buckets:
- Mechanical tightening — replace per-byte
gc()+compare loops intestBlobInterfacewith a singletoEqualafter one GC (plustoBeInstanceOf(Uint8Array)forbytes()); switch the two 100-continue tests from hardcoded port 8080 to port 0; rewrite#3545to hit a localBun.serveand assert the parsed request target instead of firing an unawaited fetch at example.com; rewrite the invalid-header test to assertTypeError+ the specific message; pipe and assert stdout/stderr for both children in the Response life-cycle test. - Concurrency — mark the four duplex RSS cases and the two idle-timeout tests
it.concurrent. Each of these spawns its own subprocess or servers withport: 0and shares no module-level state, so they are safe to overlap. - Timing-test redesign — the explicit-
timeouttest's server now holds requests until the no-timeoutcontrol has been aborted (instead of a fixed 10s), and the absolute-deadline test now drips/b's body until/hhas timed out (instead of completing/bat a fixed 6s that fell inside the 4–8s sweep window). Both changes are argued to be strictly tighter than before.
Security risks
None. Test file only; no auth, crypto, or user-input handling.
Level of scrutiny
Bucket 1 is low-risk and follows repo test conventions (never contact external hosts; port 0; assert exact error class/message; toEqual over many toBe). Bucket 2 is low-risk given each case is isolated. Bucket 3 is the part that warrants a human look: the redesigned idle-timeout tests depend on specific reasoning about uSockets' LIBUS_TIMEOUT_GRANULARITY sweep and the relative arming order of client-side idle timers. The argument is well-stated (the explicit requests / /b are sent first and only after the server has seen them is the control / /h sent, so the control's timer cannot be armed earlier), and I could not find a flaw, but this is exactly the kind of subtle timing invariant that should be confirmed by someone who owns the fetch idle-timeout implementation — a mistake here would silently drop regression coverage rather than fail loudly.
Other factors
- The redirect-URL loop is reduced 100→20 with a comment naming what it guards (#8874, undrained redirect body poisoning the pooled connection); 20 sequential iterations still exercise keep-alive reuse and the test now additionally asserts
redirectedand the body, so net coverage is not obviously reduced. - One candidate issue was raised and refuted by verifiers: whether
fetch()with an invalid header throws synchronously (which would make the newconst promise = fetch(...)line throw before the.rejectsassertion). The verifier concluded it rejects rather than throws; the author reports the test passes on both debug and release. - The
withoutAggressiveGCimport removal is scoped to this file; the helper remains inharness.tsand is used elsewhere. - The Response life-cycle test now pipes the server child's stdout/stderr but only drains them after
kill(); the assertion is that both are empty, so there's no realistic pipe-buffer risk.
fetch() resolves once the response head is in, so a timeout on /b's body surfaces from text() and was thrown out of the test instead of showing up in the hdr/bod comparison. Also only let the test end /b's body: running out of drip bytes no longer completes the response by itself.
|
On the ordering argument the idle tests now rest on, two pointers plus a check, for whoever looks at the idle-timeout path:
The second commit came out of that check: |
A 302 that carries a body has its connection closed rather than drained and pooled, so repeating the round trip cannot surface an undrained redirect body; the only reuse the loop adds is the second round's /redirect riding the connection pooled by the first round's final response. Run it twice and say so, and point at fetch-keepalive.test.ts for the redirect pooling matrix.
There was a problem hiding this comment.
I re-reviewed after the reply on the invalid-header test — that finding was wrong (it was checked against 1.3.14; reject_on_exception in src/runtime/webcore/fetch.rs confirms current main returns a rejected promise, and the case passes on CI). No bugs found this run.
Given the two idle-timeout tests are rewritten around a specific uSockets sweep-ordering argument, and a couple of hunks trade coverage for speed (redirect loop 100→2, per-byte-GC compare → single toEqual), a human sign-off on those judgment calls would still be worthwhile.
Checked this run: withoutAggressiveGC is only the import here (helper still used by 15+ other files); the four it.concurrent duplex cases each own their subprocess/servers so no shared state; port 8080→0 and the local-server #3545 rewrite remove the last external-host / fixed-port cases in the touched hunks.
Extended reasoning...
Overview
Test-only change to test/js/web/fetch/fetch.test.ts (~200 lines): replaces per-byte GC-in-a-loop comparisons in testBlobInterface with a single toEqual; rewrites the two fetch idle-timeout tests to release held responses on an observed event (the control request timing out / /h failing) instead of fixed sleeps; converts six independent cases to it.concurrent; drops the long-redirect loop from 100 to 2 iterations; moves #3545 off example.com onto a local server; tightens the invalid-header assertion to the exact TypeError message via .rejects; switches the two 100-continue tests from port 8080 to port 0; pipes and asserts child stdout/stderr in the Response-life-cycle test. No production code touched.
Prior finding withdrawn
My earlier inline comment claimed fetch() throws synchronously on an invalid header. That was checked against Bun 1.3.14. On current main the fetch entry point is wrapped by reject_on_exception (src/runtime/webcore/fetch.rs:341-368, comment: "fetch() never throws synchronously"), so the call returns an already-rejected promise and the .rejects assertions run. The author's reply and CI build 95959 (fetch.test.ts green on every lane) confirm this; the thread is resolved and I'm not re-raising it.
Security risks
None. Test-only; the only network endpoints introduced are local Bun.serve({ port: 0 }) / net.createServer on 127.0.0.1, replacing prior example.com and hardcoded-8080 uses.
Level of scrutiny
Medium. It's test-only and CI-green, but the two idle-timeout rewrites rest on a non-trivial ordering argument about uSockets' 4s sweep granularity (the explicit requests' timers are armed no later than the control's, so a regressed build would abort them by the same sweep). The author simulated both regressions and showed the tests fail through their toEqual diff, which is convincing, but this is timing-adjacent logic a maintainer familiar with src/http idle-timeout arming should confirm.
Other factors
Two hunks reduce what the original tests exercised: the redirect loop drops from 100 iterations (original comment: "run it more times to check Malformed_HTTP_Response errors") to 2, with a new rationale that the pooled-connection-after-redirect property is covered in two rounds and that connection counts are pinned separately in fetch-keepalive.test.ts; and the testBlobInterface per-byte compare with a forced GC between every read becomes one toEqual after one GC. Both are reasonable trade-offs for a ~5.5× debug-build speedup, but they're coverage decisions rather than mechanical refactors, so deferring for a human to confirm they're acceptable. The PR body says "20 iterations" for the redirect loop while the code (and its comment) say 2 — the code's comment is self-consistent, so this reads as the description not being updated after commit bf6aa70, not a code bug.
|
The description was updated together with bf6aa70; it now describes the two-round loop (and why two rounds are what the test can exercise). The two places where this trades repetition for a single check, that loop and the whole-array compare in |
1 participant
Problem
test/js/web/fetch/fetch.test.tsis a serial-phase file that takes ~29s on every CI lane (41s on debian x64 ASAN), and a release build is barely faster than a debug one, so the time is spent waiting rather than running.timeoutidle test, 6-8s in the absolute-deadline idle test, ~7s ofBun.gc(true)calls intestBlobInterface(two forced GCs per compared byte, ~6,300 calls in total), and ~4s of independent RSS cases infetch should allow duplexrunning one after another.testBlobInterfaceGC loops are synchronous and run insidedescribe.concurrent("Bun.file"), so they also stall the HTTPS cases that share that concurrent batch (on a slow machine those time out).#3545fires an unawaitedfetch()at example.com and only asserts that nothing throws synchronously, the 100-continue tests listen on a hardcoded port 8080, and the explicittimeouttest assertstoStartWith("ERR:")on what is actually the DOMException legacy code (ERR:23).Fix
timeoutidle test (10.0s to 4.0s): the child's server holds every request until the control request (notimeout) has been aborted by the 1s default, instead of for a fixed 10s. The three explicit requests are sent first and the control only once the server has seen them, so their idle timers are armed no later than the control's: a build that ignored the explicittimeoutwould have aborted them by the sweep that aborts the control, before the server releases anything, so the check stays deterministic. The control fires at the first sweep tick, ~4s in./bnow drips body bytes until/hhas timed out and is only then completed. Before,/bcompleted after a fixed 6s, inside the 4-8s window in which/h's deadline fires, so a body path that stopped re-arming was only caught when the sweep phase happened to fall under 6s; now/bhas to survive the very sweep that killed/h./bis sent first and/honly after the server has/b's request, for the same ordering argument as above. Its duration is/h's deadline: two sweep ticks, which is ~8s in practice because this test's own sockets are what start the sweep timer (4-8s only when earlier sockets left it running).it.concurrent, so together they cost ~8s instead of 16-18s. Becauseit.concurrent(name, fn, timeout)is not one of prettier's test-call shapes, prettier wraps both calls and re-indents their bodies;git diff -wshows the real changes.testBlobInterface(~7s to ~0.2s in release): thearrayBuffer/bytes/arrayBuffer -> arrayBuffer/arrayBuffer -> bytescases compare the whole array withtoEqualafter one GC instead of callinggc()before and after every byte; that is the same check (the buffer is read after a full GC), and thebytes()cases additionally assert the result is aUint8Array.withoutAggressiveGChad no other users. This is where most of the drop in theexpect()count (7,438 to ~1,000) comes from.fetch should allow duplexareit.concurrent(4.2s to 2.0s); they each spawn their own child or servers. One name is shortened by a few characters (... is backpressuredto... is stalled, matching its sibling) so that prettier keeps the call on one line instead of re-indenting the test. Their RSS observation windows are unchanged.should allow very long redirect URLSruns its round trip twice instead of 100 times (each round is two requests plus a forced GC in the handler; 5s+ in this container's debug build, 0.3s now). The loop dates from fix(fetch) redirects bodies should be handled #8874, when a redirect response's body was drained and its connection pooled; since fetch: follow redirects on the response head instead of awaiting the 3xx body #33613 a 3xx that carries a body has its connection closed instead (src/http/lib.rs, theis_redirect_pendingbranch of the body-length check), so repeating the round trip cannot surface an undrained redirect body. What a second round does add is the next/redirectriding the connection pooled by the previous round's final response (checked with a raw server: two rounds use three connections, and the second connection serves round 1's final response and then round 2's/redirect); the comment now says exactly that, and which redirect responses get pooled is already pinned by connection count infetch-keepalive.test.ts("302 carrying a body" and friends). The test also assertsredirectedand the final body now.TimeoutError: The operation timed out.for the control) and an empty stderr; the Response life cycle test pipes both children and asserts the client's stderr is empty, that it logged all 10 iterations, and that the server printed nothing;#3545fetcheshttp://host:port?a=bfrom a local server and asserts the request target it received (/+?a=b),response.urland the status; the invalid header test asserts aTypeErrorwithInvalid header name: ...(on current mainfetch()returns a rejected promise here rather than throwing; older releases threw); the 100-continue tests listen on port 0.timeout, which is what a client that ignored the option would do;/b's body bytes withheld, which is what the client sees if body reads stop re-arming). On release and debug builds both tests then fail through theirtoEqualdiff: the three explicit requests come backTimeoutErrorat the same ~4s sweep as the control, and/bis already aTimeoutErrorwhen/hfails (output in the details below). The second commit makes a/bbody timeout land in that diff instead of being thrown out of the test (fetch()resolves on the head, so it surfaces fromtext()), and only lets the test complete/b's body.bun bd test test/js/web/fetch/fetch.test.ts(debug + ASAN, same binary) 225.5s before, 41.0s after;USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch.test.ts(release) 29.9s before, 12.3s after. On this PR's CI run (build 95959) the file took 12.1-12.2s on the linux release lanes and 12.5s on debian x64 ASAN, against the 28-30s / 41s it took on the builds quoted above. The set of failing cases in this container is identical before and after (see details); everything this PR touches passes on both builds.timeouttest runs inside it), ~2s the duplex RSS windows, ~0.8s the Response life cycle fixtures. The 8s is a floor set by uSockets' fixed 4s sweep; the two idle tests are written so they do not depend on the tick length, so a test-only override of the sweep period in bun-usockets (serve.test.ts and fetch-http2-client.test.ts pay the same floor) would be the next step and would let these two tests shed their ordering comments. Not attempted here.Background
LIBUS_TIMEOUT_GRANULARITY), andus_socket_timeout(s, seconds)arms the socket forceil(seconds / 4)sweeps, so a 1s timeout fires at the next sweep (0-4s later) and a 5s timeout at the second one (4-8s later). The sweep timer only runs while the HTTP thread's loop has sockets and starts when the first one is created, which is why the child in the explicittimeouttest fires at almost exactly 4s, why the absolute-deadline test usually lands at 8s, and why that test needs a 2-sweep timeout at all: a 1-sweep timeout fires at the next sweep whether or not it was re-armed in between.BUN_CONFIG_HTTP_IDLE_TIMEOUTsets the default idle timeout (seconds) when the HTTP thread starts, which is why that test runs in a child process;fetch(url, { timeout: ms })overrides it per request.it.concurrentcases run together with theit.concurrentcases declared directly before and after them; a plainitin between is a barrier. That is why only the adjacent groups above were converted, and why the two idle tests had to both become concurrent to overlap.Local failure set (unchanged by this PR)
In this container 25 cases fail identically before and after:
localhostresolves to::1first here, so listeners bound to the namelocalhostare unreachable from the client (the situation #37442 addresses), and thebad permissions throwscases do not apply when running as root. Before this change the debug run additionally timed out in the 12utf16 ... (with gc)cases, in the HTTPS cases that share their concurrent batch, and in the long redirect test; those all pass now.Simulated regressions against the rewritten idle tests (release build; the debug build fails the same way). Explicit requests sent without a
timeout:/b's body bytes withheld (what the client sees if body reads stop re-arming the timer);/bhad already failed at the same sweep as/h(both at 8007ms in a standalone process, where the sweep starts with the first socket):Release per-case profile before: explicit
timeouttest 10.0s, absolute-deadline test 6.0s,bounds memory when the upload target is slower2.0s,suspends a type:'direct' body1.5s,Bun.fileconcurrent batch ~1.75s (dominated by its GC loops),Response/Request/Blobutf16 ... (with gc)cases 0.3-0.6s each (16 cases),should not keep Response alive0.65s,bounds memory when a handler forwards0.63s.