Conversation
…ive requests The dispatch path corked the connection's socket Duplex after every 'request' emit but never uncorked it. ServerResponse writes through the native response handle (not the Duplex), so the cork was a no-op for response bytes but left the Duplex's writableState.corked incrementing on every kept-alive request. When a CONNECT or Upgrade later arrived on the same connection and the handler wrote to the raw socket, those writes sat in the Writable's cork buffer forever and never reached the wire. This is the proxy-chain (puppeteer-with-proxy) failure in #12213: proxy-chain's upstream chain() sends a CONNECT over the kept-alive socket the prior forward() request left corked; the '200 Connection Established' it writes never reaches the client, so Chrome sees net::ERR_TUNNEL_CONNECTION_FAILED. Drop the stray dispatch cork, uncork the connection in ServerResponse.prototype.end like Node's OutgoingMessage.end does, and fully uncork before handing the socket to 'connect'/'upgrade'.
|
Warning Review limit reached
Next review available in: 11 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 (2)
Comment |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The removed socket.cork() had no matching uncork() on any path and ServerResponse writes go through the native handle (handle.writeHeadAndEnd/handle.end), not the socket Duplex, so the cork was indeed leaking a count per kept-alive request rather than coalescing anything. The two added uncork blocks mirror Node's OutgoingMessage.prototype.end (corked = 1; uncork()). Deferring only because this touches the node:http server request-dispatch and ServerResponse.prototype.end hot paths — worth a human confirming nothing else was relying on that cork.
What was reviewed:
- Confirmed no other
uncork()in_http_server.tspaired with the removedcork()— it was a genuine leak. - Checked
ServerResponse.prototype.cork/uncork/writableCorkedforward tothis.socket, sores.cork()corks the same Duplex the newend()block clears. - New
end()block guards onreq?.socketand_writableState?.corked, so the FakeSocket / no-socket paths fall through unchanged. - Tests cover CONNECT × Upgrade × sync/async write plus a 3-request kept-alive
writableCorkedassertion; all wireerrorto reject and useport: 0.
Extended reasoning...
Overview
Three edits to src/js/node/_http_server.ts:
- Removes a lone
socket.cork()in theonNodeHTTPRequestdispatch path (right after the'request'/'checkContinue'/'checkExpectation'emits). This cork had no matchinguncork()anywhere in the file, sosocket._writableState.corkedincremented once per kept-alive request. Because Bun'sServerResponsewrites go through the nativehandle(writeHeadAndEnd,end) rather than the socket Duplex, the cork was not coalescing response bytes — it only leaked state. - Adds a full-uncork (
corked = 1; uncork()) inServerResponse.prototype.end, matching Node'sOutgoingMessage.prototype.end, so a userres.cork()is cleared when the response completes. - Adds the same full-uncork in
detachSocketListenersForHandoffso the raw socket handed to'connect'/'upgrade'listeners haswritableCorked === 0.
Five new tests in test/js/node/http/node-http-connect.test.ts: a 2×2 matrix (CONNECT/Upgrade × sync/async write) that first sends a kept-alive GET then hands the socket off, asserting writableCorked === 0 at handoff and that socket.write() bytes reach the client; plus a test that sends 3 kept-alive GETs with res.cork(); res.end() and asserts writableCorked is 0 before and after each.
Security risks
None identified. The change only removes a stray cork and adds guarded uncork calls; no parsing, auth, or resource-limit logic is touched.
Level of scrutiny
High. _http_server.ts is the node:http server implementation — one of Bun's most heavily exercised Node-compat surfaces. ServerResponse.prototype.end runs on every HTTP response. That said, the diff is small (−2, +15 in src) and each addition is gated on _writableState?.corked being nonzero, so the common path is a single falsy check. The removal is the substantive change, and the analysis that it was dead (no matching uncork; native-handle writes bypass the Duplex) checks out against the surrounding code.
Other factors
- Git history for the removed
socket.cork()traces back only to the file's initial import commit, so its original intent can't be recovered from blame. The PR description's reasoning (native handle bypasses the Duplex) is consistent with the code I read. - Tests follow harness conventions:
port: 0,once()for readiness, error handlers wired to reject, polling on received data rather than sleeps,await usingfor server cleanup. - The new
end()block readsreq?.socketand_writableState?.corkedwith optional chaining, then only dereferences_writableState.corkedinside the truthy branch — safe whenreqor the writable state is absent. - Fixes a real user-facing failure (#12213, proxy-chain + puppeteer) with a clear repro that Node passes and pre-fix Bun fails.
Deferring to a human because the dispatch path and end() are hot enough that removing a long-standing cork() — even one that appears dead — deserves a maintainer's confirmation.
|
Updated 11:06 PM PT - Jul 25th, 2026
❌ @robobun, your commit 62d7e04 has some failures in 🧪 To try this PR locally: bunx bun-pr 35664That installs a local version of the PR into your bun-35664 --bun |
a107fde to
ef4471e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…tunnel write check load-bearing - ServerResponse.end uncorks the same storage res.cork() corked (this[fakeSocketSymbol]); req.socket can be nulled by the stream destroyer before end() runs. - Drop the defensive uncork in detachSocketListenersForHandoff: Node does not uncork before emitting 'connect'/'upgrade', and with the dispatch cork removed and ServerResponse.end uncorking it was unreachable for the scenario it documented. - Test: write() without end() so socket.writableLength is the load-bearing signal (Writable.end's force-uncork masked the leak), with the client half-closing so the fail-before case doesn't hang.
…ws FIN-before-write ordering doesn't drop the write
90477c1 to
2ce6b6f
Compare
There was a problem hiding this comment.
All prior feedback is addressed at 2ce6b6f — both source hunks are present (the stray socket.cork() removed from the dispatch path, and the this[fakeSocketSymbol] full-uncork in ServerResponse.prototype.end), and no accidental revert this time. Deferring only because this sits in the hot path every server request runs through; worth a human sign-off and a green CI across platforms.
Checked this round: the removed socket.cork() had no paired uncork() anywhere in the dispatch path (confirmed orphan); the new uncork reads the same storage res.cork() writes (fakeSocketSymbol, set by assignSocketInternal via the socket setter); the early returns above the uncork block either delegate to OutgoingMessagePrototype.end (which uncorks itself) or have no handle/socket to uncork.
Extended reasoning...
Overview
Two-file change: src/js/node/_http_server.ts (net +7/-2) removes an orphaned socket.cork() from onNodeHTTPRequest after the 'request' emit, and adds a Node-matching full-uncork of this[fakeSocketSymbol] inside ServerResponse.prototype.end. test/js/node/http/node-http-connect.test.ts (+111) adds a describe with four parameterized tests (CONNECT/Upgrade × sync/async write) that send a keep-alive GET followed by a handoff on the same connection and assert writableCorked === 0, writableLength === 0, and that the tunnel bytes reach the client, plus a fifth test asserting req.socket.writableCorked stays 0 across three keep-alive requests even when the handler calls res.cork().
History on this PR
I reviewed three earlier revisions: two flagged accidental worktree-reset reverts of the source hunks (a107fde, 90477c1 — both force-push-fixed), and one flagged that the uncork read req?.socket instead of the response's own socket storage (fixed in 0edc22f to use this[fakeSocketSymbol], matching Node's OutgoingMessage.end which uncorks this[kSocket]). All three are resolved at HEAD; the net diff against base is 2 files / 118 insertions / 2 deletions with both source hunks intact.
Security risks
None. This is Writable cork-counter bookkeeping on the server-side socket Duplex; no parsing, auth, or trust-boundary changes.
Level of scrutiny
High — onNodeHTTPRequest and ServerResponse.prototype.end execute for every node:http server request. That said, the change is narrow: the removed line was demonstrably orphaned (no paired uncork exists in the file, and Bun's ServerResponse writes bypass the Duplex via the native handle so the cork never coalesced anything), and the added block mirrors Node's OutgoingMessage.prototype.end exactly (corked = 1; uncork()). I confirmed res.cork() (line 2906) writes through this.socket → this[fakeSocketSymbol], so cork and uncork operate on the same object. The early-return at 3120 delegates to OutgoingMessagePrototype.end which has its own uncork; the early-return at 3125 has no handle/socket; the throw e at 3204 is an error path where the response is failing anyway.
Other factors
Test coverage is thorough for the bug class (both handoff kinds, sync and setImmediate-deferred writes, plus the plain keep-alive counter). The tests use write() without end() specifically so Writable.end's force-uncork can't mask the regression, and assert a combined object so failure output shows all three observations at once. The Windows FIN-vs-setImmediate ordering is handled by deferring socket.end() until both the client FIN and the deferred respond have happened. The bug-hunting system found nothing this round. Not approving because per the guidelines this touches a critical code path — a maintainer should confirm removing the dispatch-path cork has no unintended effect on any consumer that writes to res.socket directly during a request, and that CI (build #81303) is green on Windows given the test's platform-sensitive ordering.
There was a problem hiding this comment.
Beyond the inline nit: verified both source hunks are intact at 5346307 after the earlier accidental reverts. Confirmed the removed socket.cork() had no paired uncork() anywhere in the dispatch path, and that the early-return branches in ServerResponse.prototype.end ahead of the new uncork either delegate to OutgoingMessage.prototype.end (which already uncorks via this[kSocket]) or are aborted / already-finished / pipelined-queued states where a cork cannot leak onto a subsequent kept-alive request.
Extended reasoning...
This is my fourth pass on this PR. The three prior rounds flagged (1) an accidental full source revert in a107fde, (2) the uncork reading req?.socket instead of this[fakeSocketSymbol], and (3) a second accidental revert in 90477c1 — all now resolved at HEAD (5346307). The net diff is 2 files / 118 insertions / 2 deletions with the socket.cork() removal at the dispatch site and the full-uncork block in ServerResponse.prototype.end both present. The one remaining finding is a test-diagnostic nit (poll loop lacks a 'close' escape); it does not affect correctness of the fix. I also checked the ruled-out candidate that early exits in end() skip the uncork: the !handle branch delegates to OutgoingMessagePrototype.end which has its own uncork, and the other early returns (aborted, already-finished, pipelined-queued, closed_or_completed) do not leave a live kept-alive socket corked. Not approving because this removes a call from the per-request node:http server dispatch hot path — small and well-justified, but worth a maintainer eyeball.
…ose' always resolves the awaited promise
There was a problem hiding this comment.
The 62d7e04 rewrite closes the last hang window I flagged — no findings this pass. Deferring to a human because the two source hunks sit in the node:http server dispatch path and ServerResponse.prototype.end, which is more reach than I'll auto-approve.
Checked: the removed socket.cork() had no matching uncork anywhere in the file and ServerResponse writes go through the native handle, so no response-byte coalescing is lost. The new uncork reads this[fakeSocketSymbol] — the same storage ServerResponse.prototype.cork writes through — so it survives req.socket being nulled by the stream destroyer. The five new tests wire 'error'/'close' before any write and assert writableCorked/writableLength directly, so a regression fails fast rather than timing out.
Extended reasoning...
Overview
Two source hunks in src/js/node/_http_server.ts: (1) delete the stray socket.cork() in onNodeHTTPRequest after the 'request' emit, and (2) add a full-uncork block to ServerResponse.prototype.end that reads the connection via this[fakeSocketSymbol] and mirrors Node's OutgoingMessage.prototype.end (corked = 1; uncork()). Five new tests in test/js/node/http/node-http-connect.test.ts cover CONNECT/Upgrade × sync/async handoff writes on a kept-alive connection plus a three-request cork-count assertion.
Security risks
None identified. No parsing of untrusted input is added; the change only stops leaking a Writable cork count and clears it on res.end(). It does not loosen any validation or expose new surface.
Level of scrutiny
High. onNodeHTTPRequest is the per-request dispatch hot path for every node:http server, and ServerResponse.prototype.end runs on every response. The fix is small and matches Node's documented OutgoingMessage.end behavior, but the blast radius (proxy-chain, undici ProxyAgent, WebSocket upgrade over kept-alive sockets, and the five auto-suggested linked issues) means a human should confirm CI is green across platforms — the tests carry Windows-specific FIN-ordering handling and the PR's own evidence footer defers platform coverage to CI.
Other factors
This PR has been through four prior review rounds from me: two caught accidental worktree-reset reverts of the source hunks (both since force-pushed away), one moved the uncork from req?.socket to this[fakeSocketSymbol] so pipeline/compose's destroyer nulling req.socket can't skip it, and two tightened the kept-alive poll test so a premature close rejects with a diagnostic instead of hanging to timeout. All are addressed at 62d7e04; grep confirms no socket.cork() remains in _http_server.ts and the uncork block is present at ServerResponse.prototype.end. No new issues found this pass.
|
The diff is green: |
|
Independently hit this from a fuzz ledger (ledger #9800: upgrade/CONNECT write-stalled when not the first request on a keep-alive connection) and arrived at the same root cause and fix. Closed #36482 in favor of this PR. My branch |
…olicited data (#42128) ### Problem - A socket parked in `agent.freeSockets` (`keepAlive: true`) keeps reading while idle, and unsolicited bytes do not retire it. A whole response written there is attributed to the next request. - The cause is the Agent's `'free'` handler (`src/js/node/_http_agent.ts:123`). It pools the socket with nothing watching the read side, where the parser is already detached. Node fixed the same hole as CVE-2026-48931 (https://hackerone.com/reports/3582376). This ports that one fix only. ### Fix - The Agent marks a pooled socket with `kDestroyOnRead`, and `reuseSocket` clears it. A freed socket that already holds buffered readable data is destroyed before it can be pooled or handed to a queued request. - In `node:net`, `pushDataToSocket` (the one function through which all three handler tables feed the stream, from #35347) destroys a marked socket instead of pushing the bytes. - Like node's guard, it adds no public stream listener (Notes). - Verified: `test/js/node/http/node-http-agent-free-socket.test.ts`, 8 `node:test` cases that Bun runs in-process, plus one that runs the same file under Node.js. 6 fail on released bun. On Node they pass from v26.4.0, where the upstream fix shipped (Notes). Also `test/js/node/http/`, `test/js/node/net/`, and the vendored http, https, net, tls suites. Self-reviewed: 4 concerns, 3 addressed, 1 declined (Notes, "Scope"). ### Background - `Agent.freeSockets` holds idle keep-alive sockets per origin. `addRequest` takes one out and calls `reuseSocket`. - A pooled socket has no parser and no `'data'` listener: what it receives goes nowhere, or reaches the next response's parser. - A bun `net.Socket` reads through a native handler table passed at dial time. One table serves every socket that used it, so a per-socket hook must live on the socket. <details><summary>Notes</summary> **Reproduction** (bun 1.4.3, linux x64). A raw origin answers each request with its own path, then writes a complete unsolicited response on the idle pooled connection. | when the stray response arrives | stock bun | this branch | | --- | --- | --- | | the event loop polls before the next request | socket stays pooled, bytes discarded | socket destroyed, next request dials a fresh connection | | the next request is issued in the same tick | next request reads `poison` | next request reads `poison` | Node v26.3.0 (before the guard shipped) behaves like stock bun in both rows. **Node versions.** The upstream fix first shipped in Node v26.4.0 (`lib/_http_agent.js` at the v26.3.0 tag has no `installFreeSocketDataGuard`). Forced to run everywhere: Node v26.3.0 passes 2 of 8 (the reuse cases) and fails the 6 guard cases, like stock Bun; Node v26.4.0 passes 6 and fails the 2 queued-request cases (Node checks buffered bytes only on the pool path); this branch passes 8. In the file the guard cases skip on Node < 26.4.0 and the queued-request case skips on Node, each with the reason. **No public listener.** Node's first version of this guard used a `'data'` listener plus `resume()`. node-fetch@2 reads `socket.listenerCount('data')` while a response closes and started reporting false `ERR_STREAM_PREMATURE_CLOSE` errors (nodejs/node#63989), so node reworked the guard onto the stream handle's internal `onread` hook. Bun has no per-socket equivalent of that hook: the handler table given to `Bun.connect` is one shared cell, and `socket.reload()` mutates it for every socket that shares it. Hence the per-socket flag, read in `pushDataToSocket`. The test asserts `listenerCount('data')` and `listenerCount('readable')` are still 0 on a free socket, as node's does. The handler table built for the `onread` socket option keeps its own `data` callback: it bypasses the stream, and `node:http` cannot use such a socket. **The same-tick row is the residual race, and node has it too.** The stray bytes are still unread in the kernel when `addRequest` hands the socket out, so no check in JS can see them. Node's own test says as much ("in a real attack, there is always time between the poison arriving and the next client request"). Closing it needs a peek of the read side at checkout, which is what #41987 does for `fetch()`'s pool in Rust. Unrelated to this change: when bytes that win that race are not a valid response, the client's parse error surfaces as an uncaught exception instead of an `'error'` event on the request, on stock bun and on this branch alike. **The upstream test is not vendored yet.** `test/parallel/test-http-agent-free-socket-data-guard.js` injects the stray bytes with `req.socket.write()` on a `node:http` server. Bun's server leaves that socket corked after the request (`socket.writableCorked === 1`, the bytes sit in the writable buffer), which is #35664. Once that lands, the upstream file can be vendored as is and the bespoke cases trimmed. The new cases drive a raw `net`/`tls` server instead, which is also how they cover `https.Agent`. **Scope.** CVE-2026-48931 shipped in a Node security release together with other advisories. This PR does not examine or claim anything about the others. The self-review asked for a public tracking issue that lists them against bun; I left that to the maintainers, since it amounts to publishing an unverified vulnerability list. **Already-buffered bytes.** Node's `installFreeSocketDataGuard` destroys a socket whose `readableLength > 0`, but the caller still pushes it into `freeSockets`, where `'close'` prunes it a tick later (and `addRequest` can pop it first under `lifo`), and the check does not run at all when the freed socket goes straight to a request queued in `agent.requests`. Here the check runs right after the `writable` check in the `'free'` handler, so both hand-off paths share it and a destroyed socket is never pooled. For the queued path, the destroyed socket's `'close'` reaches `removeSocket`, which dials a new connection for the waiting request. The "holds unsolicited data" cases cover both paths: they `push()` the stray bytes in the response's `'end'` handler, which is after the parser detached and one tick before `'free'`. **Disarm point.** `reuseSocket` clears the flag, as in Node, where `Agent.prototype.reuseSocket` is the only caller of `removeFreeSocketDataGuard` and nothing else restores `_handle.onread` (`initSocketHandle` runs only for a new or reconnected socket). A subclass that replaces `reuseSocket` without calling the parent loses reused sockets on both runtimes. **TLS.** The guard sees decrypted application data only, so a post-handshake `NewSessionTicket` on an idle pooled socket does not trip it. The `https` cases cover a parked TLS socket that is poisoned, one freed with buffered bytes (pooled and queued paths), and one that is reused. **Cost.** One symbol-property load per received chunk. The `src/` diff is 28 lines. `kDestroyOnRead` is initialized in the `Socket` constructor so the read path stays monomorphic. **Pre-existing failures in this container, with and without this diff** (each one rechecked against main's `src/js` on the same build): `test-http-agent-keepalive.js` (`agent.sockets[name]` is not cleaned up after the server closes the socket), `test-http-client-timeout-option.js`, the `test-http(s)-proxy-request*` family, one subprocess case of `node-http-syscall-fault.test.ts`, 10 `node-net.test.ts` cases that fail here with `ECONNREFUSED`, `test-net-server-async-dispose.mjs`, `test-net-connect-custom-lookup-non-string-address.mjs`, `test-tls-client-allow-partial-trust-chain.js`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 2 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 6 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-http-agent-free-socket.test.ts bun test v1.4.3 (4ff9193) test/js/node/http/node-http-agent-free-socket.test.ts: 157 | assert.strictEqual(freeSocket.listenerCount("readable"), 0); 158 | 159 | serverSockets[0].write(poisonedResponse); 160 | 161 | await pollUntil(() => freeSocket.destroyed && agent.freeSockets[name] === undefined); 162 | assert.strictEqual(freeSocket.destroyed, true); ^ AssertionError: Expected values to be strictly equal: false !== true generatedMessage: true, actual: false, expected: true, operator: "strictEqual", diff: "simple", code: "ERR_ASSERTION" at /workspace/bun/test/js/node/http/node-http-agent-free-socket.test.ts:162:16 at withAgent (/workspace/bun/test/js/node/http/node-http-agent-free-socket.test.ts:118:11) at /workspace/bun/test/js/node/http/node-http-agent-free-socket.test.ts:150:13 at node:test:1781:26 at executeTestNode (node:test:1785:63) at processTicksAndRejec ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (9f655b7) test/js/node/http/node-http-agent-free-socket.test.ts: (pass) http.Agent free keep-alive socket over http > destroys a free socket that receives unsolicited data [22.64ms] (pass) http.Agent free keep-alive socket over http > does not pool a socket that holds unsolicited data when it is freed [3.12ms] (pass) http.Agent free keep-alive socket over http > does not hand a freed socket that holds unsolicited data to a queued request [2.17ms] (pass) http.Agent free keep-alive socket over http > reuses a free socket that received nothing [1.64ms] (pass) http.Agent free keep-alive socket over https > destroys a free socket that receives unsolicited data [41.32ms] (pass) http.Agent free keep-alive socket over https > does not pool a socket that holds unsolicited data when it is freed [5.10ms] (pass) http.Agent free keep-alive socket over https > does not hand a freed socket that holds unsolicited data to a queued request [4.67ms] (pass) http.Agent free keep-alive socket over https > reuses a free socket that received nothing [3.29ms] (pass) Node.js compatibility > all tests pass in Node.js [156.92ms] 9 pass 0 fail Ran 9 tests across 1 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-http-agent-free-socket.test.ts bun test v1.4.3 (4ff9193) test/js/node/http/node-http-agent-free-socket.test.ts: (pass) http.Agent free keep-alive socket over http > destroys a free socket that receives unsolicited data [1158.89ms] (pass) http.Agent free keep-alive socket over http > does not pool a socket that holds unsolicited data when it is freed [172.89ms] (pass) http.Agent free keep-alive socket over http > does not hand a freed socket that holds unsolicited data to a queued request [115.53ms] (pass) http.Agent free keep-alive socket over http > reuses a free socket that received nothing [100.60ms] (pass) http.Agent free keep-alive socket over https > destroys a free socket that receives unsolicited data [474.62ms] (pass) http.Agent free keep-alive socket over https > does not pool a socket that holds unsolicited data when it is freed [200.09ms] (pass) http.Agent free keep-alive socket over https > does not hand a freed socket that holds unsolicited data to a queued request [147.77ms] (pass) http.Agent fre ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 400b9a9 features baseline 23 deps, 131 codegen, 1172 objects in 697ms ninja: Entering directory `/workspace/bun/build/release' [1/146] fetch picohttpparser [picohttpparser] up to date [2/146] fetch WebKit (prebuilt) [WebKit] up to date [3/146] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystemRouter (5 fields) - FrameworkFileSystemRouter (2 fields) - MatchedRoute (8 fields) Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/net/symbols.ts | 3 + src/js/node/_http_agent.ts | 12 + src/js/node/net.ts | 14 +- .../node/http/node-http-agent-free-socket.test.ts | 251 +++++++++++++++++++++ 4 files changed, 279 insertions(+), 1 deletion(-) ``` </details> **gate history** · 3 passed · 2 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/net/symbols.ts 3 4 6 src/js/node/_http_agent.ts 5 10 8 src/js/node/net.ts 13 15 7 test/js/node/http/node-http-agent-free-socket.test.ts 0 0 2 ``` </details> <!-- robobun:evidence:end -->
|
This PR also fixes a second way to hit the same leaked cork: a Each case below has a listener that calls
On main the listener sees Checked on a debug build of main with this PR's
A test for this door is on branch Related: #42610 releases the same dispatcher cork at the CONNECT/Upgrade handoff only. I applied its |
|
Status of this PR against main at 36cd151:
What remains for this PR is the change to |
… connection iOS Safari sends its websocket handshake over a connection that has already loaded the page's assets. Bun 1.4.0's node:http corks a connection's socket after every request it dispatches and never uncorks it (oven-sh/bun#35664, open), so the socket serve.js is handed in 'upgrade' is still corked: the hub's 101 sat in its write buffer and never reached the phone. The dashboard socket stayed CONNECTING for good, refresh() never ran, and the board never filled in. Chromium and Playwright WebKit open a fresh connection for every websocket, so neither showed it. The upgrade handler uncorks the socket before it writes or pipes anything. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
1 participant
What
The
node:httpserver dispatch path corked the connection's socket Duplex after every'request'emit and never uncorked it. Bun'sServerResponsewrites through the native response handle rather than the Duplex, so the cork was a no-op for response bytes but leftsocket._writableState.corkedincrementing on every kept-alive request. When aCONNECTorUpgradelater arrived on the same connection and the handler wrote to the raw socket, those writes sat in the Writable's cork buffer and never reached the wire untilsocket.end()force-uncorked.Repro
Node prints
corked: 0 buffered: 0and the client receivestunnel-data. Bun printedcorked: 1 buffered: 50and the client never saw the writes (a latersocket.end()would flush them, but an open tunnel never ends).This is the failure behind #12213 (
proxy-chain+ puppeteer):anonymizeProxystarts a localhttp.Server; the first request on a connection goes throughforward(), andchain()'s outgoingCONNECTreuses the kept-alive socket to the upstream proxy. The upstream's'connect'handler writes200 Connection Establishedand then pipes, but the Duplex is corked so nothing reaches the client and Chrome reportsnet::ERR_TUNNEL_CONNECTION_FAILED. It is also the root of the "connection reset" write-side failure in #26553.Fix
socket.cork()inonNodeHTTPRequest. Bun'sServerResponsewrites through the native handle (handle.writeHeadAndEnd/handle.end), not the socket Duplex, so this cork was not coalescing any response bytes; it only leaked a cork count per request.ServerResponse.prototype.end, fully uncork the connection like Node'sOutgoingMessage.prototype.enddoes (_writableState.corked = 1; uncork()), reading the socket via the response's ownthis[fakeSocketSymbol](the storageres.cork()corked), so a user-issuedres.cork()is cleared when the response completes even ifreq.socketwas nulled by the stream destroyer.With this change Bun matches Node:
req.socket.writableCorkedis0at the start of every request and inside'connect'/'upgrade'handlers, and aproxy-chain-style CONNECT tunnel over a kept-alive socket delivers its writes immediately.Fixes #12213
Addresses the write-side portion of #26553
no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/http/node-http-connect.test.ts