Skip to content

node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 82.3%, +182 vendored tests, client proxy support)#31587

Open
cirospaciari wants to merge 1 commit into
claude/port-node-net-tls-tests-2from
claude/http-client-node-port
Open

node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 82.3%, +182 vendored tests, client proxy support)#31587
cirospaciari wants to merge 1 commit into
claude/port-node-net-tls-tests-2from
claude/http-client-node-port

Conversation

@cirospaciari

@cirospaciari cirospaciari commented May 29, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Re-implements the node:http client as a port of Node.js's lib/_http_client.js, replacing the previous fetch-based ClientRequest. The client now runs on node:net / node:tls sockets, the llhttp HTTPParser binding (process.binding("http_parser")), and a real Agent socket pool, matching the upstream implementation as closely as possible (each ported file links to the upstream source it follows). It also includes the server-side fixes that were needed for the Node.js http test suite to pass against our server.

Stacked on claude/port-node-net-tls-tests-2 (uses the net/tls work as its base).

Test results

Measured on Node.js's complete upstream test-http-* + test-https-* suites (parallel + sequential, 447 tests, local run against a debug build):

fetch-based client (before) this PR
upstream test-http-* (402 tests) ~55% 331/402 (82.3%)
upstream test-https-* (45 vendored) 45/45 (100%)
upstream client-proxy suite (62 tests) 43/62 (69%)
combined 419/509 (82.3%)

182 newly passing upstream test files are vendored into test/js/node/test/ and enforced by CI (116 test-http-*, 23 test-https-*, 43 client-proxy). Two vendored tests that had been adapted to the old client's behavior are restored to their verbatim upstream versions. The port follows the latest Node.js release (each ported file links to the upstream source file it follows), not unreleased main behavior.

Client (ported from Node.js)

  • ClientRequest goes through Agent.addRequest/createConnection, OutgoingMessage._storeHeader/_writeRaw and the llhttp RESPONSE parser: upgrade/CONNECT, 1xx information/continue events, keep-alive socket reuse and pooling, AbortSignal, timeouts, createConnection, maxHeadersCount, insecureHTTPParser, joinDuplicateHeaders, setDefaultHeaders, auth, and proxy header rewriting follow upstream.
  • OutgoingMessage, IncomingMessage, _http_agent and _http_common are now the upstream implementations (kOutHeaders storage, outputData buffering, chunked encoding, trailers, headersDistinct/trailersDistinct, kSkipPendingData, etc.). The native Bun.serve-based server keeps working through explicit ServerResponse/server-side construction paths.
  • https.Agent gets the upstream createConnection over tls.connect with TLS session caching and the upstream cache key; NODE_TLS_REJECT_UNAUTHORIZED is read per connection like Node.js, so setting it at runtime works.
  • Client proxy support: NODE_USE_ENV_PROXY wires HTTP_PROXY/HTTPS_PROXY/NO_PROXY into the global agents, HTTPS requests through a proxy establish a CONNECT tunnel (establishTunnel, proxy-authorization, ERR_PROXY_TUNNEL with the proxy status code, tunnel timeouts), and http.setGlobalProxyFromEnv() swaps the global agents and returns a restore function.
  • http.globalAgent assignment now changes the agent used by http.request, and overriding https.globalAgent works the same way.
  • node:dns lookup errors carry the looked-up hostname (err.hostname, and in the message) like Node.js's DNSException.

Server fixes needed by the client tests

  • Request headers are built from rawHeaders with Node's duplicate-handling rules (joining, cookie/set-cookie special cases, joinDuplicateHeaders) and a null prototype; native-server requests expose req.client.
  • ServerResponse stores headers in Node's kOutHeaders format and renders them to the native writeHead as a flat name/value array, so repeated header names (multiple Set-Cookie, duplicate Content-Length, …) are written as separate lines, header names keep the user's casing, getHeader returns arrays as set, and mutating headers after writeHead throws ERR_HTTP_HEADERS_SENT. writeHead() locks the status line like Node.js. The auto-generated Date/Connection/Keep-Alive defaults never pollute getHeaders() introspection.
  • Default Date, Connection: keep-alive|close and Keep-Alive: timeout=N[, max=M] response headers honoring res.sendDate, removeHeader(), server.keepAliveTimeout (5s, matching the latest Node.js release) and server.maxRequestsPerSocket; HTTP/1.0 requests are answered with Connection: close since the transport always closes them.
  • Node.js stream semantics on ServerResponse and the server socket: destroy(err) records res.errored and forwards the error to the socket, socket errors don't crash as unhandled, a response still attached to a dying connection emits 'close', res.writable stays true after finish, and write() accounts bytes against the socket's platform-dependent writableHighWaterMark so write loops and oversized writes report backpressure with correct 'drain' timing.
  • Upgrade requests follow Node.js: detected only with both Upgrade and Connection: upgrade headers, gated on the new shouldUpgradeCallback server option (default: an 'upgrade' listener exists), the listener gets the raw socket plus any bytes after the request head, and the socket is destroyed when the upgrade is accepted but unhandled.
  • Informational responses work: writeEarlyHints/writeProcessing reach the wire and ServerResponse.writeInformation is implemented; raw writes to the request socket are no longer dropped outside CONNECT streaming mode.
  • The connection is released for the next keep-alive request even when the request body stream was torn down and req.socket was cleared (this is what Next.js route handlers do — fixes "Socket already assigned" / "failed to pipe response" with Next.js).
  • The CONNECT socket is handed to the 'connect' listener with no internal listeners attached, like Node.js.
  • HTTP/1.0 requests report httpVersion/httpVersionMajor/httpVersionMinor correctly.
  • Native: the parser binding decodes header names/values/status as latin1 like Node; uWS no longer auto-writes Content-Length when an explicit Transfer-Encoding header was set, frames one-shot res.end(body) responses as chunked when an explicit Transfer-Encoding header is present, and the native writeHead accepts the flat pairs array; net.Socket tolerates user-supplied handles without resume/pause (needed by an upstream agent test).

Test updates

A few existing tests encoded behavior of the old fetch-based client and were updated to the Node.js-verified behavior (event order, req.socket being null until the socket event, explicit Content-Length + Transfer-Encoding both being sent, AbortSignal surfacing an AbortError error event, https-proxy-agent constructor options only applying to the proxy hop). Two unix-socket abort tests were dropped from the vendored set because they cannot listen on named pipes on Windows CI.

Also included (server features the suite needed)

server.getConnections(), server.maxHeadersCount, the optimizeEmptyRequests and shouldUpgradeCallback server options, standalone ServerResponse + assignSocket(writable) output, Node.js's connections-checking interval lifecycle, 204/304 and close-delimited response framing (native), close-on-finish connection semantics, 'http' performance entries (HttpClient/HttpRequest), the NODE_DEBUG=http sensitive-data warning, and a child_process.spawnSync fix so runtime process.env mutations reach children.

Known remaining gaps (follow-ups)

  • The fetch() global-dispatcher half of setGlobalProxyFromEnv (no undici dispatcher to swap), and a few proxy error-path tests that assert Node.js's exact uncaught-error formatting.

  • Original header-name casing in req.rawHeaders (the uWS parser lowercases field names in place) and request/response trailers on the server.

  • Server headersTimeout/requestTimeout (408), accept-time 'connection' events, raw socket I/O after 'upgrade', and flushing already-written response data when res.socket.destroy() is called immediately after a write.

  • The native server rejects pipelined requests separated by stray CRLFs and requests that carry both Content-Length and Transfer-Encoding, where Node.js's parser is more lenient/explicit (affects test-http-keep-alive-max-requests).

  • A handful of upstream tests require Node internals (--expose-internals, V8 natives, node:domain async context) and are not portable.

How did you verify your code works?

  • Ran the full vendored test/js/node/test/parallel + sequential test-http-* and test-https-* suites, the hand-written suites in test/js/node/http/, test/js/bun/test/parallel/test-http-*, test/js/bun/http/proxy.test.ts, and the Next.js (next-auth) integration test against a debug build; compared raw wire output against Node.js for the affected behaviors (keep-alive headers, duplicate header lines, HEAD + Transfer-Encoding, latin1 headers, informational responses, explicit Content-Length + Transfer-Encoding, client timeout and abort semantics, event ordering, backpressure/drain accounting).
  • The newly vendored tests were each verified to pass repeatedly before being added.

@robobun

robobun commented May 29, 2026

Copy link
Copy Markdown
Collaborator
Updated 1:07 PM PT - Jun 15th, 2026

@cirospaciari, your commit 80e6e38 has some failures in Build #62634 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 31587

That installs a local version of the PR into your bun-31587 executable, so you can run:

bun-31587 --bun

Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/_http_server.ts Outdated
Comment thread test/regression/issue/27061.test.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread packages/bun-uws/src/HttpResponse.h
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from 83618ad to 036fff3 Compare May 30, 2026 21:33
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from f9114ea to 5df4936 Compare May 30, 2026 23:46
Comment thread src/js/node/https.ts
Comment thread src/js/node/_http_incoming.ts Outdated
Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/_http_server.ts Outdated
@cirospaciari cirospaciari changed the title node:http: rewrite the client on net/tls sockets and the llhttp parser node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 80.5%, +124 vendored tests) Jun 1, 2026
Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/_http_server.ts
@cirospaciari cirospaciari changed the title node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 80.5%, +124 vendored tests) node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 82.8%, +135 vendored tests) Jun 1, 2026
Comment thread packages/bun-uws/src/HttpResponseData.h
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread test/js/node/test/parallel/test-http-unix-socket-keep-alive.js
Comment thread src/js/node/_http_server.ts Outdated
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from b4b0da6 to 3598acd Compare June 2, 2026 00:44
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from 635ac06 to 7a0dca3 Compare June 2, 2026 16:53
Comment thread src/js/node/_http_client.ts Outdated
Comment thread src/which/lib.rs Outdated
Comment thread packages/bun-uws/src/HttpParser.h
Comment thread packages/bun-uws/src/HttpResponse.h
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/which/lib.rs Outdated
Comment thread packages/bun-uws/src/HttpParser.h
@cirospaciari cirospaciari changed the title node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 82.8%, +135 vendored tests) node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 82.3%, +182 vendored tests, client proxy support) Jun 2, 2026
Comment thread src/js/node/https.ts
Comment thread src/js/node/https.ts
Comment thread src/js/node/_http_incoming.ts
Comment thread src/js/node/_http_incoming.ts Outdated
@robobun

robobun commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

This PR fixes #31795 (tunnel.httpsOverHttp() being ignored) — the ported client goes through Agent.addRequest/onSocket, which is the exact path the tunnel package uses for its CONNECT tunnel. I closed #31798 (a narrower targeted fix) in favor of this.

One gap: I didn't find an explicit tunnel-package regression test on this branch. If useful, the two tests I wrote for #31795 exercise the exact issue scenario and could be vendored here as a guard — a 502 CONNECT failure surfacing tunneling socket could not be established, statusCode=502, and a full httpOverHttp tunnel success path asserting the proxy's connect handler actually ran. Happy to open a small PR adding just those against this branch if you want.

Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts
Comment thread src/js/node/https.ts
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from a8653be to a0fa094 Compare June 8, 2026 18:59
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch 2 times, most recently from 40136d1 to c4e7222 Compare June 8, 2026 19:09
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from a0fa094 to 0123e91 Compare June 10, 2026 00:18
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from c4e7222 to e4da32a Compare June 10, 2026 00:23
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch 2 times, most recently from 51b18bb to ef5a776 Compare June 10, 2026 21:52
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from 2fb3ab4 to b4e3f68 Compare June 10, 2026 22:25
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from ef5a776 to 9f68962 Compare June 10, 2026 22:45
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch 2 times, most recently from 4c199fe to 62478b7 Compare June 11, 2026 00:12
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from 9f68962 to af336b9 Compare June 11, 2026 00:12
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch 2 times, most recently from ca6ba19 to 2ec08e3 Compare June 11, 2026 01:38
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from af336b9 to e64402a Compare June 11, 2026 02:19
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from 2ec08e3 to 048abb3 Compare June 11, 2026 02:19
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from e64402a to 8c5c085 Compare June 15, 2026 18:51
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from 048abb3 to 293652e Compare June 15, 2026 18:55
Comment on lines +868 to +871
OutgoingMessage.prototype.addTrailers = function addTrailers(headers) {
if (this.finished) {
throw $ERR_HTTP_HEADERS_SENT("set trailing");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The if (this.finished) throw $ERR_HTTP_HEADERS_SENT("set trailing") guard at the top of addTrailers() is in Node.js main but not in v26.3.0, which the file header at line 3 pins this port to — so req.end(); req.addTrailers({...}) now throws synchronously where v26.3.0 (and pre-PR Bun) silently no-op. Same policy class as the resolved keepAliveTimeout 65s→5s revert (commit e568057, "track the latest Node.js release, not main") and the PR description's explicit "follows the latest Node.js release ... not unreleased main behavior". Low impact (calling addTrailers() after end() is a user error and the trailers cannot reach the wire either way); one-line fix per the stated policy: drop the if (this.finished) guard.

Extended reasoning...

What the bug is

OutgoingMessage.prototype.addTrailers at src/js/node/_http_outgoing.ts:868-871 opens with:

OutgoingMessage.prototype.addTrailers = function addTrailers(headers) {
  if (this.finished) {
    throw $ERR_HTTP_HEADERS_SENT("set trailing");
  }
  this._trailer = "";
  ...

The file header at line 3 says this file is a port of https://github.com/nodejs/node/blob/v26.3.0/lib/_http_outgoing.js. But the if (this.finished) guard is not in Node.js v26.3.0 — it only exists on unreleased Node.js main. Verified by fetching both:

  • v26.3.0, line 1025-1026: OutgoingMessage.prototype.addTrailers = function addTrailers(headers) { this._trailer = ''; ... — starts directly with this._trailer = '', no finished check.
  • Node.js main, line 1025-1028: if (this.finished) { throw new ERR_HTTP_HEADERS_SENT('set trailing'); } — the guard is present.

So in v26.3.0, req.end(); req.addTrailers({...}) is a silent no-op (it sets this._trailer to a string that is never read, since end() already wrote the chunked terminator). In Bun it now throws ERR_HTTP_HEADERS_SENT synchronously.

Why this matters per the PR's own policy

The PR description states explicitly: "The port follows the latest Node.js release (each ported file links to the upstream source file it follows), not unreleased main behavior." The file header link at line 3 pins to v26.3.0. And there is direct precedent on this PR: review thread #3327493294 flagged the keepAliveTimeout default of 65s as Node.js-main-only (v26.3.0 has 5s), and the author reverted it in commit e568057 ("node:http: track the latest Node.js release, not main"), updating the PR description to match. The accepted doc nit #3366298904 reinforced the same policy.

The pre-PR addTrailers (visible in the diff's removed OutgoingMessagePrototype.addTrailers lines) also had no finished check — it started directly with this._trailer = "". So this PR introduced the divergence; it is not pre-existing.

Why nothing catches it

The vendored test-http-outgoing-proto.js only calls addTrailers() on fresh new OutgoingMessage() instances where this.finished === false, so the guard is never reached. No test (vendored or hand-written) calls addTrailers() after end().

Step-by-step proof

  1. const req = http.request({...}); req.end();OutgoingMessage.prototype.end() sets this.finished = true (_http_outgoing.ts:992).
  2. req.addTrailers({ "X-Trailer": "value" })_http_outgoing.ts:869: this.finished is truethrows ERR_HTTP_HEADERS_SENT("set trailing") synchronously.
  3. Node.js v26.3.0 for the same code: addTrailers line 1025 falls straight to this._trailer = '', builds the trailer string, returns. The trailer never reaches the wire (the 0\r\n + trailer + \r\n write in end() already happened with the previous _trailer value), but no throw.
  4. Pre-PR Bun: the removed OutgoingMessagePrototype.addTrailers had no finished check either — same silent no-op as v26.3.0.

So this is a behavioral change introduced by this PR: code with a defensive/late addTrailers() in a cleanup path (e.g. inside a 'finish' listener, or after an early end()) that ran fine on v26.3.0 and on pre-PR Bun now crashes.

Impact

Low. Calling addTrailers() after end() is a user error — the trailers cannot reach the wire either way, since the chunked terminator has already been written. Throwing is arguably more helpful behavior (it's why Node.js main added it). But it contradicts the file's own version pin and the PR's explicitly stated and previously enforced policy. Same severity bucket as #3366298904 (the keepAliveTimeout description fix) and the e568057 revert.

Fix

Per the stated policy, drop the guard so addTrailers matches v26.3.0:

 OutgoingMessage.prototype.addTrailers = function addTrailers(headers) {
-  if (this.finished) {
-    throw $ERR_HTTP_HEADERS_SENT("set trailing");
-  }
-
   this._trailer = "";

Alternatively, if the main behavior is intentionally preferred here, update the file-header link or add a note that this one method tracks main — but that would be inconsistent with the e568057 precedent.

Comment thread src/js/node/https.ts
Comment on lines +350 to +356
options = { __proto__: null, ...options };
options.defaultPort ??= 443;
options.protocol ??= "https:";
http.Agent.$call(this, options);

this.maxCachedSessions = this.options.maxCachedSessions;
if (this.maxCachedSessions === undefined) this.maxCachedSessions = 100;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Heads up: resolved comment #3359624794 says "the injected keys are deleted from agent.options afterwards, so introspection matches Node again. User-supplied values are left alone" — but the constructor at lines 350-353 contains no delete this.options.defaultPort / delete this.options.protocol (grep confirms zero matches in src/js/node/), so new https.Agent().options still carries both keys. The fix described in that resolution appears to have been lost in the squash to 293652e. Same introspection-only severity as the two prior rounds (#3329489544, #3345078775); flagging only because the author's own resolution text now describes a code change that is absent — either restore the two delete lines (gated on the user not having supplied them, per the comment) or correct the resolved status.

Extended reasoning...

What this is

Resolved review comment #3359624794 on this file says:

Took the refinement in 4f64b34: protocol/defaultPort are still injected before the super call (the proxy config needs them), but the injected keys are deleted from agent.options afterwards, so introspection matches Node again. User-supplied values are left alone.

But the current https.Agent constructor (lines 347-362) is:

function Agent(options) {
  if (!(this instanceof Agent)) return new Agent(options);

  options = { __proto__: null, ...options };
  options.defaultPort ??= 443;
  options.protocol ??= "https:";
  http.Agent.$call(this, options);

  this.maxCachedSessions = this.options.maxCachedSessions;
  if (this.maxCachedSessions === undefined) this.maxCachedSessions = 100;

  this._sessionCache = { map: {}, list: [] };
}

There is no delete this.options.defaultPort / delete this.options.protocol. A grep for delete this.options across src/js/node/ returns zero matches. http.Agent (_http_agent.ts:27) does this.options = { __proto__: null, ...options } and never filters these keys, so new https.Agent().options still carries defaultPort: 443 and protocol: 'https:' — exactly the introspection divergence the resolved comment claims was fixed.

Step-by-step proof

  1. new https.Agent() → line 350: options = { __proto__: null }. Lines 351-352: options.defaultPort = 443, options.protocol = 'https:'.
  2. Line 353: http.Agent.$call(this, options)_http_agent.ts:27: this.options = { __proto__: null, ...options }this.options = { __proto__: null, defaultPort: 443, protocol: 'https:' }.
  3. Lines 355-361: read maxCachedSessions, set _sessionCache. No delete this.options.defaultPort / delete this.options.protocol anywhere.
  4. Result: new https.Agent().options.defaultPort === 443 and .protocol === 'https:'. Node.js: both undefined.
  5. The resolved comment explicitly says these keys are deleted after the super call. They are not.

Why the fix is verifiably absent

  • grep -rn 'delete this\.options' src/js/node/zero matches.
  • git log -p --all -G 'delete this\.options\.(defaultPort|protocol)' -- src/js/node/https.ts src/js/node/_http_agent.ts returns nothing — the deletion was either never committed to a pushed ref or was lost before the squash to 293652e. Either way it is not in the PR.
  • The branch has been squashed into a single commit (293652e), so 4f64b34 is no longer individually inspectable; but the result of that commit is what's on the branch, and it does not contain the claimed change.

Why flag this (a third time)

This is the third comment on the same underlying introspection nit (#3329489544 → #3345078775 → this), which is borderline review-fatigue territory. The distinguishing factor: the prior two rounds were "this divergence exists" / "this divergence was reintroduced (deliberately, for proxy parsing)". This round is "the author's own resolution text describes a code change that is verifiably absent from the code" — which is a different actionable concern. If the two delete lines were lost in the squash, the author would want to know; if they were intentionally dropped, the resolved-comment text should be corrected so the record matches the code.

There is direct precedent on this PR for accepting "resolved status is stale / claims a fix that isn't present" reports: #3345078775 was exactly this kind of note on the same issue (round 2), and the author accepted it; #3366298904 (PR description stale) and #3366532336 (code comment stale) were also accepted.

Impact

Zero functional impact — same as the original #3329489544 explicitly said: agent.defaultPort/agent.protocol instance properties are correct, tls.connect doesn't read these keys from the merged options, and getName() doesn't reference them. The only observable is that new https.Agent().options carries two extra keys Node.js doesn't. The injection-before-super itself is necessary and correct (the base http.Agent constructor calls parseProxyConfigFromEnv(proxyEnv, this.protocol, …), so post-super assignment would parse against HTTP_PROXY instead of HTTPS_PROXY — see #3345078775's reasoning). Only the post-super cleanup the author said they applied is missing.

Fix

If the intent in #3359624794 stands, restore the cleanup after the super call (gated on the user not having supplied the values, per "User-supplied values are left alone"):

function Agent(options) {
  if (!(this instanceof Agent)) return new Agent(options);

  options = { __proto__: null, ...options };
  const injectedDefaultPort = options.defaultPort === undefined;
  const injectedProtocol = options.protocol === undefined;
  options.defaultPort ??= 443;
  options.protocol ??= "https:";
  http.Agent.$call(this, options);
  // http.Agent already copied these onto `this`; drop the injected keys
  // from agent.options so introspection matches Node.js's lib/https.js.
  if (injectedDefaultPort) delete this.options.defaultPort;
  if (injectedProtocol) delete this.options.protocol;

  this.maxCachedSessions = this.options.maxCachedSessions;
  ...
}

Otherwise no code change needed — just noting the resolved status on #3359624794 describes a state that does not match the squashed result.

@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from 8c5c085 to e914de7 Compare June 15, 2026 19:36
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from 293652e to 80e6e38 Compare June 15, 2026 19:36
@cirospaciari cirospaciari force-pushed the claude/port-node-net-tls-tests-2 branch from e914de7 to 6aac31a Compare June 15, 2026 20:06
Re-implement the node:http client as a faithful port of Node's
lib/_http_client.js on net/tls sockets and the llhttp parser binding,
replacing the fetch-based client. Node http suite: ~55% -> 82.3%, +182
vendored tests. Adds client proxy support (NODE_USE_ENV_PROXY, https
CONNECT tunneling, http.setGlobalProxyFromEnv), raw header wire casing,
server upgrade raw socket I/O, standalone ServerResponse, perf_hooks
'http' entries, and close-on-finish/close-delimited response framing.
@cirospaciari cirospaciari force-pushed the claude/http-client-node-port branch from 80e6e38 to cacf99d Compare June 15, 2026 20:06
Comment on lines 1468 to 1469
}
$toClass(ServerResponse, "ServerResponse", OutgoingMessage);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 🟡 Follow-up to the eight resolved standalone-path comments: the ServerResponse constructor is missing Node's if (req.httpVersionMajor < 1 || req.httpVersionMinor < 1) { this.useChunkedEncodingByDefault = chunkExpression.test(req.headers.te); this.shouldKeepAlive = false; } branch (the Node.js source the line-1448 comment links to has it directly above the HEAD check). On the native path this is introspection-only (renderNativeHeaders never reads useChunkedEncodingByDefault, and requestShouldKeepAlive already handles HTTP/1.0); on the standalone path new ServerResponse(new IncomingMessage(null)) — where httpVersionMajor/Minor === null and null < 1 is true — Bun chunk-encodes with Connection: keep-alive while Node.js close-delimits with Connection: close, and several of this PR's own regression tests in node-http.test.ts encode the Bun output (they would fail under Node.js; the vendored test-http-server-response-standalone.js only passes because it explicitly sets httpVersionMajor: 1, httpVersionMinor: 1). Same severity bucket as the eight prior accepted standalone findings (#3337696357 through #3366890847); fix: add the version check after the HEAD check using req.headers?.te, and update the standalone tests to pass an HTTP/1.1 req like the vendored test does.

Extended reasoning...

What the bug is

Node.js's ServerResponse constructor (lib/_http_server.js — the file the line-1448 comment links to) has, immediately above the HEAD check:

if (req.httpVersionMajor < 1 || req.httpVersionMinor < 1) {
  this.useChunkedEncodingByDefault = chunkExpression.test(req.headers.te);
  this.shouldKeepAlive = false;
}

Bun's constructor at line 1431 unconditionally does this.useChunkedEncodingByDefault = true and never derives shouldKeepAlive from the request version. Two consequences:

(1) Native-server path — introspection only. For an HTTP/1.0 request (onNodeHTTPRequest sets http_req.httpVersionMinor = 0 at line 592), res.useChunkedEncodingByDefault === true and res.shouldKeepAlive === true (via the kShouldKeepAlive ?? true getter at line 2341, since the constructor never wrote false to it), while in Node.js both are false. Wire output is unaffected: renderNativeHeaders() does not read useChunkedEncodingByDefault anywhere, and requestShouldKeepAlive(res.req) already returns false for HTTP/1.0 (line 1643), so the response correctly advertises Connection: close.

(2) Standalone path — output divergence. For new ServerResponse(new IncomingMessage(null)), the socket-based IncomingMessage constructor branch (this PR's diff) sets httpVersionMajor = null / httpVersionMinor = null as instance properties. null < 1 coerces to 0 < 1true, so in Node.js this enters the branch: useChunkedEncodingByDefault = chunkExpression.test(undefined)false, and shouldKeepAlive = false. Then _storeHeader's else if (!this.useChunkedEncodingByDefault) { this._last = true; } branch fires (no TE header, body close-delimited), and shouldSendKeepAlive = this.shouldKeepAlive && (…) is falseConnection: close. In Bun, useChunkedEncodingByDefault stays true so _storeHeader falls through to Transfer-Encoding: chunked (or Content-Length when _contentLength is known via end()'s fromEnd path), and shouldKeepAlive stays true so shouldSendKeepAlive is trueConnection: keep-alive.

Step-by-step proof (the PR's own test encodes the divergence)

The regression test "standalone ServerResponse buffers writes made before assignSocket and flushes them on assignment" in node-http.test.ts (added in 6987495) does:

const res = new ServerResponse(new IncomingMessage(null as any));
res.write('hello');
res.assignSocket(ws);
res.end();

expect(out).toContain('Transfer-Encoding: chunked');
expect(out).toContain('5\\r\\nhello\\r\\n');
expect(out).toEndWith('0\\r\\n\\r\\n');
  1. new IncomingMessage(null) → socket-based else branch → this.httpVersionMajor = null, this.httpVersionMinor = null.
  2. new ServerResponse(req) line 1431: useChunkedEncodingByDefault = true unconditionally. Node.js would evaluate null < 1 || null < 1trueuseChunkedEncodingByDefault = false, shouldKeepAlive = false.
  3. res.write('hello') → no handle → OutgoingMessage.writewrite_()_implicitHeader()writeHead(200) → standalone _storeHeader. Bun: !state.contLen && !state.teuseChunkedEncodingByDefault true → falls through to header += 'Transfer-Encoding: chunked'; this.chunkedEncoding = true. shouldSendKeepAlive = true && (false || true || undefined)Connection: keep-alive. Node.js: else if (!this.useChunkedEncodingByDefault) { this._last = true; } fires (no TE header). shouldSendKeepAlive = false && (…) → else → Connection: close.
  4. Bun output: HTTP/1.1 200 OK\\r\\n…Connection: keep-alive\\r\\n…Transfer-Encoding: chunked\\r\\n\\r\\n5\\r\\nhello\\r\\n0\\r\\n\\r\\n — the test's assertions pass. Node.js output: HTTP/1.1 200 OK\\r\\n…Connection: close\\r\\n\\r\\nhello (close-delimited, no TE, no chunk framing) — every one of those three assertions fails.

The same applies to the sibling tests "standalone ServerResponse flushHeaders pushes the header block immediately" (asserts 2\\r\\nhi\\r\\n0\\r\\n\\r\\n), "standalone ServerResponse writeContinue reaches the assigned socket", and others that construct from new IncomingMessage(null as any). The vendored test-http-server-response-standalone.js (line 12-16) passes only because it explicitly constructs the request as { method: 'GET', httpVersionMajor: 1, httpVersionMinor: 1 } — i.e. it knows the version matters.

Why this is a nit / why nothing catches it

  • Native path is introspection-only: the wire output is correct because renderNativeHeaders ignores useChunkedEncodingByDefault entirely and requestShouldKeepAlive already handles the HTTP/1.0 close case. Only code reading res.useChunkedEncodingByDefault / res.shouldKeepAlive directly on an HTTP/1.0 native response would observe the wrong values.
  • Standalone path is niche: new ServerResponse(req) + assignSocket(writable) is a test-harness / serverless-adapter pattern, and the form that diverges (new IncomingMessage(null) rather than an explicit {httpVersionMajor: 1, httpVersionMinor: 1} req) is rarer still.
  • Pre-existing constructor gap: the constructor body itself is unchanged context (not a + line in the diff). But this PR added the standalone _storeHeader path in writeHead (line ~2237), the OutgoingMessagePrototype delegation in write/end, the shouldKeepAlive accessor backed by kShouldKeepAlive (6542f11), and the regression tests — all of which make this divergence newly observable. So this fits the "pre-existing constructor gap that the PR's standalone path now exposes" pattern of #3337696357 (the chunkedEncoding accessor) and #3366708485 (the shouldKeepAlive accessor), both accepted as nits.
  • Not a duplicate of any open or resolved comment on this PR — none address the constructor's HTTP-version-derived useChunkedEncodingByDefault/shouldKeepAlive initialization.

Same severity bucket as the eight prior accepted standalone-ServerResponse findings (#3337696357, #3337810012, #3363075086, #3365977895, #3366298897, #3366596979, #3366708485, #3366890847) the author fixed.

Fix

Add the version check after the HEAD check at line 1450, using req.headers?.te so a plain-object req without .headers (as some of the PR's tests pass) does not throw:

if (req.method === 'HEAD') this._hasBody = false;

if (req.httpVersionMajor < 1 || req.httpVersionMinor < 1) {
  this.useChunkedEncodingByDefault = chunkExpression.test(req.headers?.te);
  this.shouldKeepAlive = false;
}

(chunkExpression is already exported from node:_http_common.) The standalone regression tests in node-http.test.ts would then need to construct the request as { method: 'GET', httpVersionMajor: 1, httpVersionMinor: 1, headers: {} } (like the vendored upstream test does) instead of new IncomingMessage(null), so they keep asserting chunk-framed output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants