test: run fetch-http2-client in-process instead of one child per test - #37809
test: run fetch-http2-client in-process instead of one child per test#37809robobun wants to merge 1 commit into
Conversation
…d per test Most tests in this file only exercise the h2 client after ALPN has picked h2, which protocol: "http2" reaches through the same ClientSession code as the env flag, so they now fetch in-process against their own server instead of spawning a debug child each. The tests that are about a startup knob (env flag, --experimental-http2-fetch, protocol: "http1.1", the h1 fallback, BUN_CONFIG_HTTP_IDLE_TIMEOUT) and the use-after-free regression keep their subprocess. 60 spawns per run become 13, which also makes the ASAN spawn semaphore unnecessary. The idle-timeout test, which has to hold requests for 10s, is declared first so its hold overlaps the rest of the file's concurrent group instead of running after it. Timer-based waits are replaced with the event they were waiting for: a barrier request on the same connection for the RST_STREAM and stale-frame assertions, a PING round trip for the Expect: 100-continue proof, the server session's close for the GOAWAY reconnect, and the accept events for the coalescing tests. Assertions that only checked "rejected" or an exit code now check the exact error code, body, headers, session counts or the frames the client put on the wire.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Status: ready for review.
|
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. It's a ~900-line rewrite of 62 tests that swaps per-test subprocess isolation for in-process protocol: "http2" with a shared connection pool, and replaces most timing waits with wire-ordered barriers — the reasoning is careful and the assertions are strictly tightened, but the isolation change and the new synchronisation arguments (PING-ACK barrier, follow-up-request-as-delivery-barrier, held-stream release) are subtle enough that a human pass and a green multi-lane CI run are worth having before merge.
What was reviewed:
listenH2/listenTcpteardown destroys accepted sessions/sockets so pooled connections don't leak across concurrent tests; each server binds an ephemeral port so pool keys stay per-test.onFrameordering inwithRawH2Server: the asynconStreamsetsconn.onFramebefore yielding onawait conn.ping(), so DATA frames buffered behind HEADERS in the same read are counted.- The UAF regression test still runs in a subprocess (crash is the failure mode) and now sequences the stray END_STREAM ahead of stream 3's response instead of racing a 30ms/80ms timer.
spawnFetch's default env still sets the h2 flag, and the{}overrides for the flag-off / CLI-flag tests correctly omit it.
Extended reasoning...
Overview
Test-only change to test/js/web/fetch/fetch-http2-client.test.ts (single file, ~900 lines of diff). 49 of 62 tests move from spawning a debug subprocess (so BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT is read at startup) to fetching in-process with protocol: "http2", which the PR argues reaches the same h2::ClientSession code path once ALPN has picked h2. The remaining 13 tests that genuinely test a startup knob (env flag, CLI flag, BUN_CONFIG_HTTP_IDLE_TIMEOUT, the crash-mode UAF regression) keep their subprocess. Along the way most sleep/setTimeout synchronisation is replaced with event-ordered barriers (server-side session close, PING-ACK round-trip, follow-up request on the same connection, accept promises), and many assertions are tightened from "rejected"/exit-code checks to exact error codes and full body comparisons. The 10s idle-timeout test is moved to the top as test.concurrent so its hold overlaps the rest of the file, and the "resolves on headers" test drops its stderr-gated server and becomes concurrent.
Security risks
None. Test file only; no production code, no auth/crypto/permissions surface, no new external inputs. rejectUnauthorized: false and NODE_TLS_REJECT_UNAUTHORIZED=0 were already present for the self-signed test cert.
Level of scrutiny
High, despite being test-only. The repo's review guidelines are explicit that weakening or de-flaking tests must preserve the property the original assertion protected, and this PR rewrites the synchronisation strategy for ~50 tests and changes the isolation model (per-test subprocess → shared in-process pool keyed by ephemeral port). Each of the new barriers has a stated ordering argument (e.g. "HEADERS(3) is queued behind RST_STREAM(1) on the one connection"), and I checked the ones I could reason about statically — they hold. The state.frames / conn.onFrame plumbing added to withRawH2Server is ordered so that onFrame is installed synchronously inside the HEADERS dispatch before the await conn.ping() yields, so a client that ignored Expect would have its DATA counted. listenH2/listenTcp destroy every accepted session/socket in finally, addressing the new concern that in-process fetches leave pooled connections open past the test. The two never-ending upload streams (pull() returns a never-resolving promise) rely on fetch cancelling the stream on 303 — a small per-run leak at worst, not a correctness issue.
Other factors
- The PR was verified with 5 consecutive local ASAN runs (62/62 each), but not across the full lane matrix; the previous file had platform-specific accommodations (Windows abortive-close comment, aarch64/musl serial note) that are now removed because the mechanisms they worked around are gone. That reasoning is plausible but should be confirmed by CI.
- Two open PRs (#33338, #31788) touch this file and will need a rebase; the description calls this out.
- No prior reviews on the PR; coderabbit skipped it.
- Given the scope (62 tests re-plumbed, isolation model changed) and the repo's bar for "prove the test still fails for the right reason", a maintainer familiar with the h2 client should confirm the
protocol: "http2"equivalence claim and spot-check a couple of the new barriers on the slower lanes before merge.
|
On the two points flagged for confirmation:
CI on this push (build 93109): all 20 x64 ASAN test shards are green; the file itself reports |
|
Updated 9:05 AM PT - Aug 12th, 2026
❌ @robobun, your commit 729a516 has some failures in 🧪 To try this PR locally: bunx bun-pr 37809That installs a local version of the PR into your bun-37809 --bun |
Problem
test/js/web/fetch/fetch-http2-client.test.tsis one of the slowest non-integration test files on the x64 ASAN lane (90s wall in build 92779).BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENTcould be read at startup. Under ASAN the file capped live children at 4, so a run was about 15 rounds of startups, then the 10s idle-timeout test alone at the end.Fix
protocol: "http2", each against its own server on an ephemeral port. Only tests about a startup knob, plus the use-after-free test whose failure mode is a crash, still spawn a child: 13 processes per run instead of 60.http/1.1to the offer plus an h1 fallback. Two env-flag round trips stay end to end.test.concurrent, so the rest of the file runs during its hold. The other sleeps are replaced by wire events, and many tests now assert exact error codes, full bodies and connection counts. No test is removed, skipped or weakened; names and order are unchanged apart from that one move.bun-debug teston the file with a debug ASAN build on one machine, main vs this PR: CPU 39.3s+7.6s vs 16.2s+1.7s, wall about 26s vs about 15s, 62/62 on 5 consecutive runs of the new version.Background
BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENTand--experimental-http2-fetchare read at process startup, which is why testing them needs a child process.h2, http/1.1; the per-requestprotocol: "http2"option offersh2only and works without the flag, so a test can reach the h2 client from the test process itself.server.close()alone leaves them open.localhostresolves to both loopback addresses and fetch races a TCP connect to each, so servers that count connections bind127.0.0.1.Original description
What does this PR do?
test/js/web/fetch/fetch-http2-client.test.tsis one of the slowest non-integration test files on the x64 ASAN lane (90s wall in build 92779). Nearly all of that is process startup: 61 of its 62 tests spawned a debug child so thatBUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENTcould be read at startup (60 spawns per run). Under ASAN the default test concurrency is 5 and the file additionally capped live children at 4, so a run was ~15 rounds of child startups, followed by the 10s idle-timeout test running by itself at the end.This rewrites the file so the same 62 tests make the same (and in many places stricter) checks with much less machinery. No test is removed, skipped or weakened. Test names and order are unchanged, except for the one test noted below.
Fetch in-process where the process is not what is being tested
protocol: "http2"pins the ALPN offer to h2 only. Once the handshake has chosen h2, the flag path and the pinned path create the sameh2::ClientSession(src/http/lib.rs:alpn_offer()and thealpn == b"h2"arm of the handshake callback); the flag only addshttp/1.1to the offer plus an h1 fallback. So every test that is about the client's behaviour on an established h2 connection (the raw frame server tests, flow control, trailers, multiplexing, session reuse, GOAWAY, abort, compression, coalescing onto a cold connect, ...) now fetches in-process against its own server on an ephemeral port, which also gives each test a fresh pool entry. This is the patternfetch-http2-adversarial.test.tsalready uses.Tests that are about a startup knob keep their subprocess: the env flag itself (GET and POST round trips,
protocol: "http1.1"overriding it, the h1 re-dispatch of coalesced waiters),--experimental-http2-fetch, "flag off",protocol: "http2"from a flag-less process,BUN_CONFIG_HTTP_IDLE_TIMEOUT, and the use-after-free regression, whose failure mode is a crash. That is 13 processes per run instead of 60, so the ASAN spawn semaphore is no longer needed.spawnFetch(script)keeps its signature.Since the client now outlives each test, servers tear down what they accepted:
listenH2destroys its sessions andlistenTcpits sockets.listenTcpbinds127.0.0.1so the tests that count TCP connections are exact (localhostresolves to both loopback addresses and fetch races a connect to each of them).Overlap the one inherently slow test
The idle-timeout test (#16682) has to hold its requests for 10s; that hold is the point of the test and is unchanged. It is now declared first, as
test.concurrent, so it takes the first concurrency slot at t=0 and the rest of the file runs during the hold instead of after it. This is the only test that moved. The "resolves on headers" test was serial only because it gated the server on a child's stderr; it now ends the held stream directly, so the whole file is one concurrent group.Wait for events instead of time
GOAWAY after a request: slept 50ms before the second request. A GOAWAY'd session cannot be pooled, so the client closes it; the test now awaits the server-side session close, which makes the reconnect the only possible path.Expect: 100-continue withholds the body: proved "no DATA before the 100" with a 20ms timer. The raw server now sends a PING after HEADERS and looks for DATA when the ACK arrives. A client that ignoredExpectwrites DATA right behind HEADERS, i.e. ahead of its PING ACK, so this is ordered by the wire rather than by the clock. It also asserts the exact frames the client sent (HEADERS without END_STREAM, then one 20 byte DATA with END_STREAM).Content-Length satisfied before END_STREAM(the UAF regression): relied on a 30ms server timer and an 80ms client sleep, so on a slow machine the stray frame could arrive after the child had already exited and the test passed vacuously. The stray END_STREAM frame is now sent when the client opens its second stream and is queued ahead of that stream's response, so the second fetch resolving proves the stale frame was processed. It also checks stderr now.303 redirect on a streaming POSTandclient RSTs on a local error: asserted onstate.rstafter waiting for all sockets to close. Both now use the follow-up request on the same connection as the delivery barrier (and assertconnections: 1so the ordering argument holds), the same deflake test(fetch-http2-client): deflake 'client RSTs on local error' via same-session barrier #29954 applied to one of them.concurrent requests multiplexandserver-reset stream: held streams on 100ms / 50ms timers. The server now holds the burst until all 8 streams are open, and answers/goodonly after/badhas been reset, so the assertions are implied by the wire order.abort while coalescedandleader abort: three 100ms settle sleeps replaced by the accept events. The leader-abort test races the waiter against the second accept, so a regression fails with the waiter's error code instead of a timeout.process.exit; they are now streams whosepull()never resolves.The only timer left besides the 10s hold is the 30ms stream hold in the MAX_CONCURRENT_STREAMS test, which is a detection window for a cap violation (the test asserts something never happens), not a synchronisation wait; it cannot cause a false failure.
Stricter assertions
Many tests only checked
"rejected", a status, or an exit code. Now, among others:HTTP2RefusedStream(both REFUSED_STREAM tests),HTTP2StreamReset(PROTOCOL_ERROR reset),HTTP2ContentLengthMismatch(both content-length tests),HTTP2ProtocolError(missing:status),ZlibError(local error),HTTP2Unsupported(h1-only server, plainhttp://),AbortErroron every abort path; the server-reset test acceptsHTTP2ProtocolError|HTTP2StreamResetbecause which one it is depends on whether the node:http2 server emits RST_STREAM (node:http2: send RST_STREAM when a server stream is reset #33380) or its current empty DATAabort sends RST_STREAMasserts the server sawrstCode === NGHTTP2_CANCEL(it previously only awaited the close); the redirect and local-error tests assert the CANCEL code tooREFUSED_STREAM is retriedasserts the retry used stream id 3 on the same connection;SETTINGS_HEADER_TABLE_SIZE=0asserts all three requests rode one session (a reconnect per request would have passed it before);h1-only serverasserts no request was downgraded to h1;protocol: "http1.1"asserts the pinned request never reached the server;plain http://asserts nothing was sent and a normal fetch to the same server worksBuffer#equals, all 24x24 upload results viatoEqual), responses are compared as{status, headers, body}objects, trailers are asserted not to leak into the headers, the stripped-headers test asserts the exact surviving set, POST echoeshttpVersionHow did you verify your code works?
Same machine, same debug ASAN build (
max-concurrencydefaults to 5 under ASAN, as on the CI lane),bun-debug teston the file:bun testAbout 4.6s of the remaining wall time is loading the file and
harnessunder ASAN (an empty test file with the same imports reports 4.60s), and ~10.3s is the idle-timeout hold, which now runs concurrently; the other 61 tests finish inside that window. CPU time is the better predictor for the CI lane, which is CPU-bound on child startups.The file passed on all 5 consecutive runs of the new version (62/62 each time), with nothing shared between tests beyond the process.
Note for other open PRs touching this file: #33338 edits the body of the idle-timeout test (which moved to the top of the file) and #31788 appends a test after it; both will need a trivial rebase. The
spawnFetchhelper andisASANusage they rely on:spawnFetch(script)still works unchanged,isASANis no longer imported here.