perf: cut Bun listener per-request overhead (IP clone + compress bridge)#773
Conversation
The Bun shell cloned the whole Request on every request just to stamp the remote IP, and bridged every compressed response web -> node -> web even when the body was fully buffered. Two hot-path costs the ~1.9x listening-path figure does not account for. IP: stamp it out of band via setTrustedRemoteIp (a WeakMap clientIp reads in preference to, and to the exclusion of, the x-webjs-remote-ip header), so no per-request Request reallocation and a client still cannot spoof it. Compression: readBufferedOrStream peeks a body to classify a single bounded buffered chunk (the common SSR page / JSON / file response) vs a genuinely streamed body, without buffering a real stream. A buffered body is compressed synchronously (compressBufferSync), skipping the stream bridge; a streamed or oversized body keeps the bridge (and its #509 mid-stream-error teardown). Sync and streamed output are byte-identical, so node and Bun stay equal. Also publishes scripts/bench-listener.mjs (end-to-end SSR vs listening path) and scopes the ~1.9x claim to the listening path in the docs. Cross-runtime proof (test/bun/listener-overhead.mjs) verified on node 26.1 and bun 1.3.14: buffered compress decodes for br/gzip/deflate and the trusted IP reaches clientIp out-of-band.
The Bun listener stamps the trusted remote IP out of band (a WeakMap, #756) instead of cloning the Request to set x-webjs-remote-ip. The page-action body rebuild (parseFormBody) builds a NEW Request, which loses that WeakMap stamp and would fall back to the inbound (spoofable) header on Bun, so clientIp() inside a no-JS page action (e.g. login throttling) could trust a client-supplied x-webjs-remote-ip. Strip the inbound header on the rebuild and carry the framework-trusted IP forward via propagateTrustedRemoteIp. Node was already safe (the ingress toWebRequest strips the header), so the regression and its counterfactual are Bun-specific. The cross-runtime test now reports failures explicitly (Bun exits 0 without flushing a top-level-await rejection inside try/finally, which would otherwise silently swallow it). Also scope the compression byte-equality claim to WITHIN a runtime (Bun's bundled zlib is not Node's build, so gzip/deflate bytes can differ across runtimes; brotli matches; the wire is self-describing via content-encoding), and read the trusted IP via clientIp() in the listener parity test (the raw header is intentionally absent on Bun).
Self-review round 1A worktree-isolated read-only review (full Bun + Node parity sweep) surfaced two findings, both now fixed in 3bacbbb. 1. MUST-FIX, security: 2. Minor, accuracy: the "byte-for-byte equal across node and Bun" compression claim is false for gzip/deflate. Also surfaced + fixed: the #511 listener parity test ( Verification: Round 2 to follow. |
… review) Two MUST-FIX issues a second review round found in the Bun listener overhead work. 1. SECURITY: the `webjs.basePath` request rebuild re-opened the exact x-webjs-remote-ip spoof the page-action fix closed, on Bun. dev.js's produce() rebuilds the Request with the basePath-stripped URL; that fresh object is not in the listener's out-of-band trusted-IP WeakMap and copies the inbound headers verbatim, so clientIp fell back to the client-controlled x-webjs-remote-ip header (a rate-limit / login-throttle bypass). It also defeated the page-action fix under basePath, since runPageAction then received the already-spoofed request. Strip the inbound header on the rebuild and propagate the trusted IP, the same chokepoint pattern page-action.js uses. Node was already safe (ingress toWebRequest strips the header), so the bug + its test are Bun-specific. 2. REGRESSION: readBufferedOrStream awaited the classifying SECOND read before returning, so for a genuinely streamed body (Suspense, a streamed action) the Bun shell withheld the response head + first byte until the far-off second chunk arrived. Since compression is on by default in prod and maybeCompress is awaited before the Response reaches Bun.serve, every compressed streamed page lost progressive first paint. Race the second read against a macrotask sentinel instead: a buffered body (closed underlying stream) settles on the next tick and takes the sync fast path, while a still-pending read is handed back immediately as a stream (drainAfterPending consumes the in-flight read, never re-issuing it). Also (NIT): resolve the streaming compressor BEFORE peeking/locking the body in maybeCompress, so a defensive null backend cannot return a half-drained, locked Response. Tests: a unit test proving readBufferedOrStream returns promptly for a slow-second-chunk body (counterfactual for #2), plus cross-runtime listener-overhead sections (4) streamed-head-not-blocked and (5) basePath spoof-closed. Bun matrix green; node listener/rate-limit/page-action/base-path suites green.
Self-review round 2A fresh worktree-isolated review (IP-trust + compression traced end to end on both runtimes) found two new MUST-FIX items the round-1 fixes did not cover. Both reproduced, both now fixed in 9f93b21, both with a confirmed counterfactual. MUST-FIX 1 (SECURITY): the MUST-FIX 2 (REGRESSION): a streamed body's response head was withheld until its slow second chunk on Bun. NIT (addressed): Verification: node listener / listener-core / rate-limit / page-action / base-path suites green; Bun matrix 197 pass / 0 genuine fail; both MUST-FIX counterfactuals confirmed red-without-fix. Round 3 to follow. |
… overhead Round-3 review noted the listener-overhead proof had no dedicated Bun CI step (its wrapper comment claimed one that did not exist), so its only Bun run was through the `bun test` matrix, where two server boots + a deliberate 400ms stall risk the per-test timeout the dedicated-step pattern exists to avoid. Add the dedicated `bun test/bun/listener-overhead.mjs` step (the plain script sidesteps that timeout) so the comment is true and the Bun-side counterfactual runs reliably. Also strengthen section (1): a raw-socket head probe asserts the Bun buffered sync-compress fast path is actually TAKEN (it SETS content-length; the stream bridge DELETES it), which a fetch-based check cannot see because fetch strips content-length when it decompresses. Bun-only, since the node shell stream-compresses a buffered page too.
Self-review round 3A fresh worktree-isolated review traced the IP-trust + compression paths end to end on both runtimes and verified every round-1/round-2 fix is correct (including the node-under-basePath clientIp path, the no-unhandled-rejection SHOULD-FIX (fixed, 61d2867): no dedicated Bun CI step. The wrapper comment claimed NIT (fixed, 61d2867): the Bun proof didn't assert the buffered fast path is actually TAKEN. Section (1) only round-tripped (passes on either path). Added a raw-socket head probe asserting the buffered page carries SHOULD-FIX (pre-existing, out of scope, filed as #778): WS upgrade requests are not IP-stamped on either runtime, so Verification: node listener/listener-core/rate-limit/page-action/base-path suites green; Bun matrix 197 pass / 0 fail; both prior MUST-FIX counterfactuals re-confirmed red-without-fix. Round 4 to follow. |
…exit Round-4 review: the dedicated `bun test/bun/listener-overhead.mjs` CI step was added but the bun matrix STILL ran the `.test.mjs` wrapper under bun test's 5s default per-test timeout, the exact flake the dedicated step exists to dodge (the single test() boots two servers + a 400ms stall). Denylist the wrapper in run-bun-tests.js (same rationale as compression.test.mjs); the dedicated plain-script step + listener-core.test.js cover the Bun behavior. Also guard the proof's hard `process.exit(1)`: it is needed for standalone runs (Bun swallows a top-level-await rejection inside try/finally), but when the `.test.mjs` wrapper imports the script under `node --test` it would kill the whole single-process run and hide every other file's results. Exit only when `import.meta.main`, else throw so the harness reports one failed test.
Self-review round 4Review verified all core runtime changes correct (IP WeakMap, sync buffered compress, streamed-head classifier, basePath + page-action spoof guards, #509 teardown, no unhandled rejection, the WS gap genuinely pre-existing). Two CI-reliability findings from the round-3 edits, both fixed in 9d13623: MUST-FIX (fixed): the bun matrix still ran the proof under bun's 5s per-test timeout. Round-3 added the dedicated SHOULD-FIX (fixed): Verification: matrix now SKIPs the wrapper (196 pass / 0 genuine fail); standalone |
…t path Round-5 review NIT: the round-3 NIT fix hoisted `createCompressor` above the body peek for safety, but that allocates a native zlib/brotli Transform on EVERY compressed response, including the buffered fast path that uses `compressBufferSync` and never needs it (a waste on the exact hot path this PR optimizes). `encoding` is already guaranteed br/gzip/deflate by `negotiateEncoding`, so `createCompressor` never returns null; move it into the streamed branch only and drop the dead null-check (which is also what avoided the original NIT-3 locked-body return). No behaviour change; verified on node + bun (listener, compression, listener-overhead, the matrix).
Self-review round 5Review verdict: CLEAN, no material (blocking) findings, ready to merge. It re-verified the round-4 CI fix (DENYLIST exact-match + the dedicated Two NON-BLOCKING NITs:
Round 6 (focused on the perf hoist) to confirm convergence. |
1 participant
Closes #756
The Bun listener shell added two per-request hot-path costs the advertised "~1.9x more req/s" (a listening-path microbenchmark) does not account for: a full
new Request(req, { headers })clone on every request to stamp the remote IP, and a web -> node -> web zlib bridge for every compressed response even when the body was fully buffered.Changes
setTrustedRemoteIpstashes the socket IP in aWeakMapthatclientIpreads in preference to (and to the exclusion of) thex-webjs-remote-ipheader. The Bun shell stamps it instead of cloning the Request. A client-spoofed header is never trusted (the WeakMap is authoritative; the listener always stamps, with''when no IP, so the header is never the fallback on Bun). The node shell plus the embeddedstampRemoteIppath are unchanged (header-based).readBufferedOrStreampeeks a body (at most two reads) to classify a single bounded buffered chunk (the common SSR page / JSON / file response) vs a genuinely streamed body, WITHOUT buffering a real stream. A buffered body is compressed synchronously (compressBufferSync), skipping the per-response stream bridge entirely; a streamed or oversized body keeps the existingReadable.from(...)to compressor toReadable.toWebbridge AND its Run the full test suite under Bun in CI (Bun test matrix) #509 mid-stream-error teardown (a replayed reader that propagates a source error / cancels on early termination). Sync and streamed outputs are byte-identical (same algo plus params: brotli q4, gzip/deflate level 6), so the node (streaming) and Bun (sync-for-buffered) paths stay byte-for-byte equal.scripts/bench-listener.mjsbenchmarks an end-to-end SSR page vs the trivial listening-path route under whichever runtime runs it, and the docs (listener-bun.js header,packages/server/AGENTS.md,agent-docs/runtime.md) scope "~1.9x" to the listening path explicitly.Tests
packages/server/test/listener/listener-core.test.js):compressBufferSyncis byte-identical to the streaming compressor for br/gzip/deflate;readBufferedOrStreamclassifies single-chunk (buffered), multi-chunk (streamed, no chunk lost), oversized-single (streamed), empty, and a mid-stream source error propagates through the replay stream without hanging (the Run the full test suite under Bun in CI (Bun test matrix) #509 class).packages/server/test/rate-limit/rate-limit.test.js): out-of-band trusted IP wins over a spoofed header; an empty stamp resolves to_anon_(header never trusted); trusted IP is the remote-IP fallback undertrustProxy:true.test/bun/listener-overhead.mjs): a real server, verified on node 26.1 and bun 1.3.14. A buffered page compresses plus decodes for br/gzip/deflate, and the framework-stamped socket IP reachesclientIp(out of band on Bun, header on node) while a spoofed header does not.test/bun/compression.mjsRun the full test suite under Bun in CI (Bun test matrix) #509 mid-stream-error no-hang proof still passes on both runtimes (themaybeCompressrefactor preserves it); full listener plus rate-limit suites (62) green; node path unchanged.Notes
sendWebResponsealready uses a single web -> node bridge (not the double bridge), so the buffered fast path is scoped to the Bun shell; byte-identical compression keeps the cross-runtime contract.