http: emit 'upgrade' event on 101 and honor chunked body on upgrade by robobun · Pull Request #29015 · oven-sh/bun

robobun · 2026-04-08T16:54:58Z

Reproduction

`dockerode`'s `exec.start({ hijack: true, stdin: true })` hangs under Bun because `http.ClientRequest` never fires the `'upgrade'` event when the server answers `HTTP/1.1 101 Switching Protocols` on a request that was opened with `req.flushHeaders(); req.write(body)` (without `req.end()`).

dockerode's pattern:

```js
const req = http.request({
socketPath: "/var/run/docker.sock",
method: "POST",
path: "/exec/${id}/start",
headers: {
"Content-Type": "application/json",
"Connection": "Upgrade",
"Upgrade": "tcp",
"Transfer-Encoding": "chunked",
},
});
req.on("upgrade", (res, sock, head) => { /* hijacked socket */ });
req.flushHeaders();
req.write('{"Detach":false,"Tty":false}');
// req.end() is NOT called — openStdin keeps the upload half open.
```

Node fires `'upgrade'` immediately after the 101; Bun silently dropped it.

Root cause

Three layers were missing:

`FetchTasklet.skipChunkedFraming()` unconditionally skipped HTTP/1.1 chunked framing for any request that carried an `Upgrade:` header, even when the user had explicitly set `Transfer-Encoding: chunked`. The body was written as raw bytes while the headers still advertised chunked, so the server's chunked parser got garbage.
`HTTPClient.writeToStream()` refused to drain the streaming request-body buffer while `upgrade_state == .pending`. The buffered chunked body could never reach the server, and the server in turn would never send 101 — a deadlock. The original check was intended to hold back post-upgrade raw bytes for WebSocket-style handshakes.
`_http_client.ts` `ClientRequest` never turned a 101 fetch response into an `'upgrade'` event. With `isDuplex: true` (streaming body), it only called `handleResponse()` once the body generator finished — which never happens when the caller deliberately leaves the upload half open.

Fix

`FetchTasklet.skipChunkedFraming` honors an explicit `Transfer-Encoding: chunked` and returns `false`, matching Node.
`HTTPClient.writeToStream` drains the buffer in the pending-upgrade state as long as the request has explicit framing (Transfer-Encoding or Content-Length). Requests with no framing (WebSocket-style) still buffer until 101 arrives.
`ClientRequest` detects `response.status === 101`, builds a `Duplex` socket that reads from the fetch Response body (post-upgrade bytes) and routes writes back through `req.write`, and emits `'upgrade'` on `process.nextTick`. If no handler is attached, the hijacked socket is destroyed.

Verification

New test: `test/regression/issue/29012.test.ts` reproduces the dockerode flow with a Unix-socket server that speaks the same chunked-body → 101 → Docker stream framing sequence. The test hangs (5s timeout) on a binary built without the fix and passes with it; it also verifies the hijacked socket delivers the framed payload.

```
$ bun bd test test/regression/issue/29012.test.ts
(pass) http.request emits 'upgrade' on 101 without req.end() [1263ms]
(pass) http.request 'upgrade' delivers hijacked socket with writable Duplex [218ms]

2 pass
0 fail
10 expect() calls
```

robobun · 2026-04-08T16:55:13Z

^{Updated 4:09 PM PT - May 11th, 2026}

❌ @robobun, your commit 1d36c13 has 3 failures in Build #53479 (All Failures):

test/js/bun/s3/s3-storage-class.test.ts - code 1 on 🍎 14 aarch64
test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 2019 x64-baseline
test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 2019 x64
test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 11 aarch64
test/js/bun/jsc-stress/jsc-stress.test.ts - code 1 on 🪟 11 aarch64

🧪 To try this PR locally:

bunx bun-pr 29015

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

bun-29015 --bun

github-actions · 2026-04-08T16:58:28Z

Found 4 issues this PR may fix:

node:http and node:https strange behaviour with upgrade event #18945 - Client-side 'upgrade' event is not emitted on http.request and the connection is closed instead; directly fixed by emitting 'upgrade' on ClientRequest for 101 responses
Dockerode attach hangs #18982 - Dockerode container.attach() hangs because the client 'upgrade' event never fires; same root cause as dockerode exec.start({ hijack: true, stdin: true }) hangs under Bun but works under Node.js #29012
Playwright connectOverCDP() not working #9911 - Playwright connectOverCDP() fails because it relies on node:http client upgrade which never emits the 'upgrade' event
'websocket' package does not work #20547 - The websocket npm package's client uses node:http upgrade events internally; fixed by client-side upgrade event emission

If this is helpful, consider adding Fixes #<number> to the PR description to auto-close the issue on merge.

🤖 Generated with Claude Code

coderabbitai · 2026-04-08T17:04:31Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

Precompute a request-body framing flag during request build; change framing logic to consult upgrade signals and user headers; treat 101 responses as connection hijacks by exposing a Duplex hijacked socket; adjust streaming/drain behavior for pending upgrades; add regression tests for HTTP/1.1 upgrade hijacks.

Changes

Cohort / File(s)	Summary
Fetch framing logic `src/bun.js/webcore/fetch/FetchTasklet.zig`	`skipChunkedFraming()` now uses post-upgrade state from `this.signals.get(.upgraded)` and reads `Transfer-Encoding` from `request_headers` to decide chunked framing, reusing the computed `transfer_encoding` value. Added comments that `request_body_has_framing` is precomputed and that `skipChunkedFraming()` reads user headers directly.
HTTP request framing flag & streaming `src/http.zig`	Added `request_body_has_framing: bool` to `pub const Flags`. `buildRequest()` scans the finalized header buffer and sets the flag when `Content-Length` is present or `Transfer-Encoding` value contains `"chunked"` (case-insensitive). `writeToStream()` changes pending-upgrade behavior to proceed with draining when explicit framing is present.
Node HTTP client upgrade handling `src/js/node/_http_client.ts`	Added `createUpgradeSocket(req, res)` creating a hijacked `Duplex` that reads from `res` and writes into `req` with lazy bridging, EOF propagation, backpressure handling, `_final()` awaiting `req` finish, timeouts, and cleanup. `handleResponse()` treats `response.status === 101` as an upgrade, sets upgrade flag, assigns the hijacked socket, emits `'upgrade'` on nextTick if a listener exists, otherwise destroys the socket and drains the body.
Regression tests for hijack `test/regression/issue/29012.test.ts`	New tests spawn a local Unix-domain TCP upgrade server and verify `http.request()` upgrade behavior: `'upgrade'` event emission, receipt of `HTTP/1.1 101` with `Upgrade` header, that the hijacked object behaves like a writable `Duplex` (methods and `head` buffer), and cleanup/error handling.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the two main changes: emitting 'upgrade' event on HTTP 101 responses and honoring chunked body framing during upgrades.
Description check	✅ Passed	The PR description provides both required sections: 'What does this PR do?' (via Reproduction, Root cause, and Fix) and 'How did you verify your code works?' (via Verification with test results).
Linked Issues check	✅ Passed	All coding objectives from issue `#29012` are met: FetchTasklet honors explicit Transfer-Encoding: chunked [29012], HTTPClient drains buffered body during upgrade with explicit framing [29012], and ClientRequest emits 'upgrade' event on 101 with hijacked duplex socket [29012].
Out of Scope Changes check	✅ Passed	All changes directly address the three root causes from issue `#29012`. FetchTasklet, HTTPClient, ClientRequest, and test additions are all scoped to fixing the upgrade-event hang.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/http.zig`:
- Around line 648-659: hasExplicitBodyFraming currently inspects
this.header_entries (the raw header list) which can include user headers later
dropped by buildRequest (due to max_user_headers), causing writeToStream to
assume framing that won't be on the wire; change the logic so the decision is
made from the header set that actually survives buildRequest or is recorded
during buildRequest. Concretely: either have buildRequest compute and store a
boolean (e.g., HTTPClient.explicit_body_framing) based on the trimmed header
slice it constructs, or make hasExplicitBodyFraming operate on the finalized
header slice produced by buildRequest (not this.header_entries), ensuring
symbols involved are hasExplicitBodyFraming, buildRequest, header_entries,
max_user_headers, and writeToStream. Ensure writeToStream reads the preserved
flag/slice instead of the raw header_entries.

In `@src/js/node/_http_client.ts`:
- Around line 117-133: The wrapper currently calls the duplex write/final
callbacks synchronously; change write(chunk, encoding, callback) so that after
calling req.write(...) you only invoke callback immediately when req.write
returns true, but if it returns false attach a one-time 'drain' handler on req
to call callback when the request actually drains; likewise change
final(callback) so that instead of swallowing req.end() errors you call
req.end() and wait for the request to finish (attach one-time 'finish' or
'close' and 'error' handlers on req) and invoke callback after the request emits
finish/close or pass the error to callback on 'error' — ensure you remove any
listeners after firing to avoid leaks and still guard against
req.destroyed/req.finished as the original checks do.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d4e9b564-4a95-4944-bd9b-ca3e33f73cec

📥 Commits

Reviewing files that changed from the base of the PR and between 1afabdd and bbc9ff0.

📒 Files selected for processing (4)

src/bun.js/webcore/fetch/FetchTasklet.zig
src/http.zig
src/js/node/_http_client.ts
test/regression/issue/29012.test.ts

…essure, header truncation Fixes review comments on #29015: - FetchTasklet.skipChunkedFraming() now checks the upgraded signal. Post-101, writes bypass chunked framing AND writeEndRequest() skips the terminating 0-chunk, matching the raw-bytes semantics of a real hijacked TCP socket. Without this, dockerode stdin writes were being wrapped in HTTP/1.1 chunk headers after the upgrade. - createUpgradeSocket() no longer calls response.body.getReader() — that would permanently lock the ReadableStream, and any upgrade handler that touched res (res.resume(), res.pipe(), etc.) would trip 'ReadableStream is already locked'. The Duplex now bridges through the IncomingMessage's 'data'/'end'/'error' events on first _read, so res stays the single owner of the fetch body reader. - createUpgradeSocket()'s _write now honors backpressure: if req.write() returns false, we wait for 'drain' before acking. _final waits for 'finish'/'error'/'close' on the underlying request before calling back instead of silently swallowing req.end() errors. - duplex.setTimeout() now arms a real timer that emits 'timeout' after the configured interval (rearmed on every write/data event), instead of registering a listener for an event that never fired. - HTTPClient.hasExplicitBodyFraming() replaced with a Flags.request_body_has_framing bit set from buildRequest(), which already knows which user headers actually made it to the wire. A late Transfer-Encoding header dropped by max_user_headers no longer makes writeToStream() falsely enable pre-upgrade draining.

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/http.zig (1)
791-823: ⚠️ Potential issue | 🟠 Major

request_body_has_framing is still computed from the wrong predicate.

Line 791 skips this block for streamed GET/HEAD/OPTIONS requests, so the Node-compat body path in src/js/node/_http_client.ts never flips the flag even when a surviving Transfer-Encoding: chunked is already on the wire. But Line 808 goes the other direction and treats any Transfer-Encoding value as self-delimiting, even though this pre-101 drain gate should only unlock for Content-Length or a finalized TE that includes chunked. Please derive the flag from the finalized header slice itself, not from method.hasRequestBody() plus “any TE header”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/http.zig` around lines 791 - 823, The flag
this.flags.request_body_has_framing is being set from the wrong predicates
during header construction; instead of relying on method.hasRequestBody() or the
transient add_transfer_encoding branch, determine framing from the finalized
request_headers_buf slice after headers are assembled. Change the logic so code
still writes Content-Length or Transfer-Encoding (chunked) into
request_headers_buf as you currently do (symbols: request_headers_buf,
content_length_header_name, chunked_encoded_header, add_transfer_encoding,
this.flags.upgrade_state), but remove/avoid relying on method.hasRequestBody()
to set this.flags.request_body_has_framing inside those branches; after header
assembly (using header_count), scan the produced headers for either a
Content-Length header or a Transfer-Encoding header whose value contains
"chunked" and set this.flags.request_body_has_framing = true only if one of
those is present (otherwise leave false). Ensure this covers streamed
GET/HEAD/OPTIONS paths so the Node-compat path sees the finalized framing flag.

♻️ Duplicate comments (1)

src/bun.js/webcore/fetch/FetchTasklet.zig (1)
1201-1206: ⚠️ Potential issue | 🟡 Minor

Don’t re-infer chunk wrapping from the raw header bag.

Line 1201 still looks at this.request_headers, even though src/http.zig.buildRequest() can drop a user Transfer-Encoding: chunked past max_user_headers. In that case the HTTP thread correctly treats the request as unframed, but writeRequestData() has already buffered hex\r\n...\r\n wrappers here. If the server later switches protocols, those buffered chunk markers get flushed into the hijacked raw stream. Thread the finalized wire-framing decision down here instead of re-reading the original headers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/webcore/fetch/FetchTasklet.zig` around lines 1201 - 1206, The code
in FetchTasklet.zig currently re-checks this.request_headers for
Transfer-Encoding inside the logic that decides whether to wrap data as chunked;
instead it must use the finalized wire-framing decision produced by
src/http.zig.buildRequest() (the value already used when buffering chunk markers
in writeRequestData()). Replace the transfer_encoding lookups in the return
expression of the method (the block referencing this.request_headers and
transfer_encoding) with the finalized framing flag or enum propagated into the
FetchTasklet (e.g., a boolean like this.is_chunked or a wire_framing field), and
ensure writeRequestData() and the upgraded_connection check use that same
finalized value so no raw header bag re-inference occurs.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/http.zig`:
- Around line 791-823: The flag this.flags.request_body_has_framing is being set
from the wrong predicates during header construction; instead of relying on
method.hasRequestBody() or the transient add_transfer_encoding branch, determine
framing from the finalized request_headers_buf slice after headers are
assembled. Change the logic so code still writes Content-Length or
Transfer-Encoding (chunked) into request_headers_buf as you currently do
(symbols: request_headers_buf, content_length_header_name,
chunked_encoded_header, add_transfer_encoding, this.flags.upgrade_state), but
remove/avoid relying on method.hasRequestBody() to set
this.flags.request_body_has_framing inside those branches; after header assembly
(using header_count), scan the produced headers for either a Content-Length
header or a Transfer-Encoding header whose value contains "chunked" and set
this.flags.request_body_has_framing = true only if one of those is present
(otherwise leave false). Ensure this covers streamed GET/HEAD/OPTIONS paths so
the Node-compat path sees the finalized framing flag.

---

Duplicate comments:
In `@src/bun.js/webcore/fetch/FetchTasklet.zig`:
- Around line 1201-1206: The code in FetchTasklet.zig currently re-checks
this.request_headers for Transfer-Encoding inside the logic that decides whether
to wrap data as chunked; instead it must use the finalized wire-framing decision
produced by src/http.zig.buildRequest() (the value already used when buffering
chunk markers in writeRequestData()). Replace the transfer_encoding lookups in
the return expression of the method (the block referencing this.request_headers
and transfer_encoding) with the finalized framing flag or enum propagated into
the FetchTasklet (e.g., a boolean like this.is_chunked or a wire_framing field),
and ensure writeRequestData() and the upgraded_connection check use that same
finalized value so no raw header bag re-inference occurs.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f9c32673-b9f0-4bf6-8ba4-8a38b02883a5

📥 Commits

Reviewing files that changed from the base of the PR and between bbc9ff0 and c6a8c50.

📒 Files selected for processing (3)

src/bun.js/webcore/fetch/FetchTasklet.zig
src/http.zig
src/js/node/_http_client.ts

Addresses review feedback on #29015: - Attach res 'error'/'close' and req 'error' listeners eagerly in createUpgradeSocket(), not inside the lazy bridgeResData() closure. Previously, if the request aborted between the 'upgrade' event firing and the user's first socket.on('data') call, the error was silently swallowed by IncomingMessage's default error path and the duplex readable side hung forever. res 'close' now pushes EOF as a fallback. - _read() now checks res.destroyed / res.readableEnded and pushes null immediately so a consumer that starts reading after an early abort doesn't hang. - _write backpressure path now mirrors _final: the drain callback is paired with one-shot 'error'/'close' listeners behind a 'settled' guard. If req is destroyed while waiting for 'drain', callback still fires with the error instead of being orphaned in kWriting state.

claude

LGTM — all previously raised issues have been addressed in the follow-up commits.

Extended reasoning...

Overview

This PR fixes three layered bugs preventing http.ClientRequest from emitting 'upgrade' on HTTP 101 responses when the caller sends a chunked body without calling req.end() (the dockerode exec-hijack pattern). Changes span FetchTasklet.zig, http.zig, and _http_client.ts.

Previous review rounds

Multiple rounds of review surfaced issues: ReadableStream double-lock, post-upgrade chunked framing corruption, missing backpressure error handlers, lazy bridgeRes race window, spurious maybeEmitClose race, and incorrect setTimeout(0, cb) semantics. All were addressed in c6a8c50 and follow-on commits. All inline comments are resolved.

Current code assessment

skipChunkedFraming() correctly gates on signals.upgraded first, then honors explicit TE:chunked (pre-upgrade), and skips framing for Content-Length and post-upgrade writes.
request_body_has_framing is computed from the finalized wire-header slice in buildRequest(), not from the raw user-header list, so truncated headers cannot falsely gate pre-upgrade draining.
createUpgradeSocket eagerly wires res/req error and close propagation outside the lazy bridgeResData() closure, closing the race window.
_write() backpressure correctly handles all three termination scenarios (drain, error, close-with-error) via a single settle() guard.
maybeEmitClose() is intentionally suppressed in the upgrade-success path to prevent a lifecycle close from racing the req.once('close', onClose) backpressure handler.
duplex.setTimeout(0, cb) removes the listener rather than adding another, matching Node's net.Socket semantics.

Security risks

No security-sensitive code paths (auth, crypto, permissions) are touched. The change is scoped to HTTP upgrade/hijack plumbing.

Level of scrutiny

High-scrutiny area (HTTP client compatibility layer), but the fixes are well-reasoned, well-commented, and cover the edge cases I identified. Tests reproduce the exact dockerode pattern.

PR oven-sh/bun#29015 (filed 2026-04-08) is the upstream fix for ClientRequest not emitting 'upgrade' on 101; it is not yet merged into any Bun release. Our workaround remains necessary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…essure, header truncation Fixes review comments on #29015: - FetchTasklet.skipChunkedFraming() now checks the upgraded signal. Post-101, writes bypass chunked framing AND writeEndRequest() skips the terminating 0-chunk, matching the raw-bytes semantics of a real hijacked TCP socket. Without this, dockerode stdin writes were being wrapped in HTTP/1.1 chunk headers after the upgrade. - createUpgradeSocket() no longer calls response.body.getReader() — that would permanently lock the ReadableStream, and any upgrade handler that touched res (res.resume(), res.pipe(), etc.) would trip 'ReadableStream is already locked'. The Duplex now bridges through the IncomingMessage's 'data'/'end'/'error' events on first _read, so res stays the single owner of the fetch body reader. - createUpgradeSocket()'s _write now honors backpressure: if req.write() returns false, we wait for 'drain' before acking. _final waits for 'finish'/'error'/'close' on the underlying request before calling back instead of silently swallowing req.end() errors. - duplex.setTimeout() now arms a real timer that emits 'timeout' after the configured interval (rearmed on every write/data event), instead of registering a listener for an event that never fired. - HTTPClient.hasExplicitBodyFraming() replaced with a Flags.request_body_has_framing bit set from buildRequest(), which already knows which user headers actually made it to the wire. A late Transfer-Encoding header dropped by max_user_headers no longer makes writeToStream() falsely enable pre-upgrade draining.

Addresses review feedback on #29015: - Attach res 'error'/'close' and req 'error' listeners eagerly in createUpgradeSocket(), not inside the lazy bridgeResData() closure. Previously, if the request aborted between the 'upgrade' event firing and the user's first socket.on('data') call, the error was silently swallowed by IncomingMessage's default error path and the duplex readable side hung forever. res 'close' now pushes EOF as a fallback. - _read() now checks res.destroyed / res.readableEnded and pushes null immediately so a consumer that starts reading after an early abort doesn't hang. - _write backpressure path now mirrors _final: the drain callback is paired with one-shot 'error'/'close' listeners behind a 'settled' guard. If req is destroyed while waiting for 'drain', callback still fires with the error instead of being orphaned in kWriting state.

Addresses review comment on #29015: the hijacked socket's writable side was built around the dockerode pattern (POST + req.flushHeaders + req.write without req.end) and broke for the standard WebSocket / Playwright CDP / `websocket` npm package pattern (GET + req.end() BEFORE the 101). In that pattern req.finished becomes true synchronously inside req.end(), so every socket.write() in the 'upgrade' listener errored with ERR_STREAM_WRITE_AFTER_END (fixes #18945, #9911, #20547 which this PR originally claimed to fix). Solution: give upgrade requests a long-lived body generator whose exit condition is decoupled from req.finished. - hasUpgradeHeaders(req) detects a Connection: Upgrade + Upgrade: <proto> pair (ignoring h2/h2c, matching Bun__fetch_ in fetch.zig). - startFetch() forces isDuplex=true and forces the streaming-body branch for upgrade requests regardless of method or the req.finished state, funneling any pre-assembled body (from req.write before req.end) through kBodyChunks so the generator picks it up. - The generator's exit while-loop diverges: upgrade requests wait on a new 'upgradeBodyEnded' flag instead of self.finished. - kEndUpgradeBody (local Symbol) is installed on 'this' and flips the flag + resolves the pending chunk promise. The hijacked socket's _final() and _destroy() call it to release the upload half. - _write() no longer rejects when req.finished is true (that's expected for the WebSocket pattern). - req.end() no longer calls resolveNextChunk(true) for upgrade requests, so the generator doesn't exit before the 101 arrives. Adds a regression test using a GET + req.end() echo upgrade server that sends 101 on headers alone and echoes post-upgrade bytes. Fails (ERR_STREAM_WRITE_AFTER_END) without this commit, passes with it.

claude

All prior review feedback has been addressed and I found no new issues, but this introduces a substantial new code path (synthesizing a hijacked-socket Duplex on top of fetch, plus upgrade-aware changes to the body generator and http.zig/FetchTasklet framing logic) that's worth a maintainer's eyes before merge.

Extended reasoning...

Overview

This PR makes http.ClientRequest emit 'upgrade' on 101 responses and keeps the upload half of the connection live for post-upgrade writes. It touches three layers: FetchTasklet.skipChunkedFraming() (now consults signals.upgraded and explicit TE), http.zig (new Flags.request_body_has_framing precomputed in buildRequest(), gating writeToStream() draining during .pending), and ~300 net-new lines in _http_client.ts — hasUpgradeHeaders(), createUpgradeSocket() (a Duplex bridging IncomingMessage reads and req.write() writes with backpressure/timeout/error wiring), kEndUpgradeBody, and the upgrade-aware async-generator body in go(). A 500-line regression test covers dockerode (POST + flushHeaders + write), WebSocket/CDP (GET + req.end()), rejected-upgrade (400), and the req.end(body) double-request case.

Security risks

No injection/auth/crypto surface. The changes are protocol-state handling on outbound client connections; no user-controlled data flows into new sinks. The main risk class is correctness/leak (generator never exiting, FetchTasklet refcount, second-request re-entry), which the review cycles addressed.

Level of scrutiny

High. This is production-critical HTTP client code with cross-thread coordination (JS ↔ HTTP thread via ResumableSink), subtle stream-lifecycle invariants (when req.emit('close') is safe, when the generator may park), and Node.js-compat semantics that real packages (dockerode, ws, Playwright) depend on. The PR went through ~10 bot review rounds, each surfacing real bugs (locked ReadableStream, post-upgrade chunk framing, orphaned drain callback, non-101 generator leak, double-request on req.end(body), lifecycle-close vs. TCP-drop race) — all now fixed and resolved. That history is itself evidence the design is non-trivial.

Other factors

The overall approach — keeping the request-body generator alive past req.end() for upgrade requests and routing hijacked-socket writes back through it — is a meaningful architectural choice (vs. handing back a raw net.Socket). It works for the tested patterns, but a maintainer should sign off on it since it constrains how _http_client.ts can evolve. CI for the head commit (2aabb07) was building at the time of review; no failures noted in the thread.

…essure, header truncation Fixes review comments on #29015: - FetchTasklet.skipChunkedFraming() now checks the upgraded signal. Post-101, writes bypass chunked framing AND writeEndRequest() skips the terminating 0-chunk, matching the raw-bytes semantics of a real hijacked TCP socket. Without this, dockerode stdin writes were being wrapped in HTTP/1.1 chunk headers after the upgrade. - createUpgradeSocket() no longer calls response.body.getReader() — that would permanently lock the ReadableStream, and any upgrade handler that touched res (res.resume(), res.pipe(), etc.) would trip 'ReadableStream is already locked'. The Duplex now bridges through the IncomingMessage's 'data'/'end'/'error' events on first _read, so res stays the single owner of the fetch body reader. - createUpgradeSocket()'s _write now honors backpressure: if req.write() returns false, we wait for 'drain' before acking. _final waits for 'finish'/'error'/'close' on the underlying request before calling back instead of silently swallowing req.end() errors. - duplex.setTimeout() now arms a real timer that emits 'timeout' after the configured interval (rearmed on every write/data event), instead of registering a listener for an event that never fired. - HTTPClient.hasExplicitBodyFraming() replaced with a Flags.request_body_has_framing bit set from buildRequest(), which already knows which user headers actually made it to the wire. A late Transfer-Encoding header dropped by max_user_headers no longer makes writeToStream() falsely enable pre-upgrade draining.

Addresses review feedback on #29015: - Attach res 'error'/'close' and req 'error' listeners eagerly in createUpgradeSocket(), not inside the lazy bridgeResData() closure. Previously, if the request aborted between the 'upgrade' event firing and the user's first socket.on('data') call, the error was silently swallowed by IncomingMessage's default error path and the duplex readable side hung forever. res 'close' now pushes EOF as a fallback. - _read() now checks res.destroyed / res.readableEnded and pushes null immediately so a consumer that starts reading after an early abort doesn't hang. - _write backpressure path now mirrors _final: the drain callback is paired with one-shot 'error'/'close' listeners behind a 'settled' guard. If req is destroyed while waiting for 'drain', callback still fires with the error instead of being orphaned in kWriting state.

Addresses review comment on #29015: the hijacked socket's writable side was built around the dockerode pattern (POST + req.flushHeaders + req.write without req.end) and broke for the standard WebSocket / Playwright CDP / `websocket` npm package pattern (GET + req.end() BEFORE the 101). In that pattern req.finished becomes true synchronously inside req.end(), so every socket.write() in the 'upgrade' listener errored with ERR_STREAM_WRITE_AFTER_END (fixes #18945, #9911, #20547 which this PR originally claimed to fix). Solution: give upgrade requests a long-lived body generator whose exit condition is decoupled from req.finished. - hasUpgradeHeaders(req) detects a Connection: Upgrade + Upgrade: <proto> pair (ignoring h2/h2c, matching Bun__fetch_ in fetch.zig). - startFetch() forces isDuplex=true and forces the streaming-body branch for upgrade requests regardless of method or the req.finished state, funneling any pre-assembled body (from req.write before req.end) through kBodyChunks so the generator picks it up. - The generator's exit while-loop diverges: upgrade requests wait on a new 'upgradeBodyEnded' flag instead of self.finished. - kEndUpgradeBody (local Symbol) is installed on 'this' and flips the flag + resolves the pending chunk promise. The hijacked socket's _final() and _destroy() call it to release the upload half. - _write() no longer rejects when req.finished is true (that's expected for the WebSocket pattern). - req.end() no longer calls resolveNextChunk(true) for upgrade requests, so the generator doesn't exit before the 101 arrives. Adds a regression test using a GET + req.end() echo upgrade server that sends 101 on headers alone and echoes post-upgrade bytes. Fails (ERR_STREAM_WRITE_AFTER_END) without this commit, passes with it.

claude

All of my prior feedback has been addressed (latest in 7ccb4a6) and I found nothing further this pass — but this is a substantial change to the core HTTP client path across Zig and JS (new createUpgradeSocket Duplex, body-generator lifecycle, writeToStream/skipChunkedFraming framing decisions), so it warrants a human sign-off.

Extended reasoning...

Overview

This PR makes http.ClientRequest emit 'upgrade' on a 101 response and keeps the upload half of the connection alive for hijacked protocols (dockerode, WebSocket, CDP). It touches three layers:

src/http/http.zig — adds Flags.request_body_has_framing, computed in buildRequest() from the finalized header slice; writeToStream() uses it to drain the streaming request body during upgrade_state == .pending when explicit framing exists.
src/runtime/webcore/fetch/FetchTasklet.zig — skipChunkedFraming() now checks signals.upgraded first (raw bytes post-101) and honors an explicit Transfer-Encoding: chunked pre-101.
src/js/node/_http_client.ts — ~250 new lines: hasUpgradeHeaders(), createUpgradeSocket() (a Duplex emulating net.Socket over the fetch response body + req.write()), kEndUpgradeBody terminator, an upgrade-aware body-generator exit condition, and the 101 dispatch path in handleResponse(). The body-generator branch in go() is restructured to force streaming for all upgrade requests regardless of method/req.finished.
test/regression/issue/29012.test.ts — six Unix-socket tests covering the dockerode flow, the GET+req.end() WebSocket flow, rejected (non-101) upgrades, the req.end(body) double-request regression, and back-to-back synchronous writes.

Security risks

None identified. The change is client-side HTTP behavior; it does not touch auth, crypto, permissions, or input parsing in a way that expands attack surface. The new header scan in buildRequest() is bounded by the existing finalized header buffer.

Level of scrutiny

High. This is production-critical code on the hot path of every fetch()/http.request(). The PR went through ~15 rounds of bug fixes (stream-lock, post-upgrade chunked-framing corruption, backpressure orphans, generator leaks on non-101, double-request on req.end(body), dropped chunks on synchronous multi-write, etc.), which speaks to the subtlety of the interactions between the JS body generator, the Zig HTTP thread, and Node stream semantics. The createUpgradeSocket Duplex is a new abstraction that emulates net.Socket over a fetch Response — a design choice (vs. exposing the raw socket) with documented trade-offs (e.g. head is always empty). A human should validate that design and the body-generator restructuring.

Other factors

All of my prior inline findings are resolved; the diff reflects each fix (post-loop kBodyChunks drain, is101 rename, ERR_STREAM_DESTROYED, gated has_framing scan, eager res error/close wiring, etc.).
A human reviewer (alii) has already engaged on performance of hasUpgradeHeaders(), which was addressed by caching.
Test coverage is good for the targeted regressions but the surface area (every upgrade pattern × every body-delivery pattern × error/abort timing) is large enough that I would not be comfortable bot-approving.

http.request with Connection: Upgrade now dispatches the 'upgrade' event when the server returns HTTP/1.1 101 Switching Protocols, even when the caller never calls req.end(). This is the pattern dockerode uses for hijacked docker exec sessions: write the exec config via req.write, leave the upload half of the connection open, and use the hijacked socket that comes back via 'upgrade'. Three pieces were missing: 1. FetchTasklet.skipChunkedFraming() would skip chunked body framing for any upgraded connection, even when the user explicitly set Transfer-Encoding: chunked. Now an explicit chunked header wins, matching Node.js. 2. HTTPClient.writeToStream() refused to drain the request body buffer while upgrade_state == .pending, deadlocking requests where the server needs the chunked body before deciding to switch protocols. Drain when the request has explicit Transfer-Encoding or Content-Length framing; keep buffering for WebSocket-style handshakes with no pre-upgrade body. 3. _http_client.ClientRequest never turned a 101 fetch response into an 'upgrade' event. It now builds a Duplex around the response body (for reads) and routes writes back through req.write, then emits ('upgrade', res, socket, head) on process.nextTick. Fixes #29012

…essure, header truncation Fixes review comments on #29015: - FetchTasklet.skipChunkedFraming() now checks the upgraded signal. Post-101, writes bypass chunked framing AND writeEndRequest() skips the terminating 0-chunk, matching the raw-bytes semantics of a real hijacked TCP socket. Without this, dockerode stdin writes were being wrapped in HTTP/1.1 chunk headers after the upgrade. - createUpgradeSocket() no longer calls response.body.getReader() — that would permanently lock the ReadableStream, and any upgrade handler that touched res (res.resume(), res.pipe(), etc.) would trip 'ReadableStream is already locked'. The Duplex now bridges through the IncomingMessage's 'data'/'end'/'error' events on first _read, so res stays the single owner of the fetch body reader. - createUpgradeSocket()'s _write now honors backpressure: if req.write() returns false, we wait for 'drain' before acking. _final waits for 'finish'/'error'/'close' on the underlying request before calling back instead of silently swallowing req.end() errors. - duplex.setTimeout() now arms a real timer that emits 'timeout' after the configured interval (rearmed on every write/data event), instead of registering a listener for an event that never fired. - HTTPClient.hasExplicitBodyFraming() replaced with a Flags.request_body_has_framing bit set from buildRequest(), which already knows which user headers actually made it to the wire. A late Transfer-Encoding header dropped by max_user_headers no longer makes writeToStream() falsely enable pre-upgrade draining.

Addresses coderabbitai follow-up review: - buildRequest() now computes request_body_has_framing by scanning the finalized request_headers_buf slice after truncation, auto-added chunked, and all other decisions. Only a Content-Length header or a Transfer-Encoding header whose value actually contains 'chunked' flips the bit — a stray Transfer-Encoding: identity or a max_user_headers-dropped entry no longer lies about wire framing. - FetchTasklet pre-computes the same bit at setup time (main thread, before scheduling the HTTP task) so skipChunkedFraming() has the right answer the moment the first body chunk is yielded on the main thread, even though buildRequest() runs later on the HTTP thread. - FetchTasklet.skipChunkedFraming() now reads a single source of truth on HTTPClient.flags instead of re-inferring from the raw header bag, eliminating the mismatch between request_headers and the finalized wire header slice.

Addresses review feedback on #29015: - Attach res 'error'/'close' and req 'error' listeners eagerly in createUpgradeSocket(), not inside the lazy bridgeResData() closure. Previously, if the request aborted between the 'upgrade' event firing and the user's first socket.on('data') call, the error was silently swallowed by IncomingMessage's default error path and the duplex readable side hung forever. res 'close' now pushes EOF as a fallback. - _read() now checks res.destroyed / res.readableEnded and pushes null immediately so a consumer that starts reading after an early abort doesn't hang. - _write backpressure path now mirrors _final: the drain callback is paired with one-shot 'error'/'close' listeners behind a 'settled' guard. If req is destroyed while waiting for 'drain', callback still fires with the error instead of being orphaned in kWriting state.

The previous commit made FetchTasklet.skipChunkedFraming() read HTTPClient.flags.request_body_has_framing, but that single bit is set for BOTH Content-Length-delimited and chunked-encoded bodies. For a streaming request with a user-supplied Content-Length and no Transfer-Encoding, the wire carries only Content-Length — the body must go out as raw bytes — but the new logic returned !true = false and wrote the body wrapped in HTTP/1.1 chunk frames ({hex}\r\n…\r\n), corrupting the request. That was the debian-13-x64-asan-test-bun failure on build 44513. Revert skipChunkedFraming() to inspect the user headers directly (as it did before c61b57d), while keeping the post-upgrade signal check. http.zig's writeToStream() still reads the flag from buildRequest() — it's set on the HTTP thread before the body phase, no race with itself — so the pre-upgrade drain decision is unchanged. Also drop the now-unused pre-computation in FetchTasklet.get().

…omment Two small review follow-ups: - createUpgradeSocket()._write backpressure path: when req.write() returns false we wait for 'drain' and also watch 'error'/'close'. The onClose handler was calling settle() with no error argument, which made the duplex report a successful write even though the in-flight chunk was actually discarded when req tore down. Surface it as a ConnResetException('socket hang up') instead. _final()'s onClose is correct as-is: a clean close after req.end() legitimately signals completion there. - http.zig buildRequest() comment: the flag is only consumed by writeToStream(), not by FetchTasklet.skipChunkedFraming(). Update the comment to match.

Two review follow-ups: - createUpgradeSocket.setTimeout(0, cb) now calls removeListener('timeout', cb) instead of registering another 'once' listener, matching net.Socket.setTimeout(0, cb) semantics and ClientRequestPrototype .setTimeout's behavior further down in the same file. - The upgrade dispatch no longer calls maybeEmitClose() after emitting 'upgrade'. maybeEmitClose scheduled req.emit('close') on the very next tick, which raced the Duplex _write backpressure path: if the upgrade handler synchronously wrote > 1 MB to the hijacked socket, _write would register req.once('close', onClose) where onClose surfaces a ConnResetException('socket hang up'). The lifecycle close fired before 'drain', corrupting a live session with a spurious error. The hijacked socket is now the lifecycle owner — it destroys req from its own _destroy path, and 'close' on req fires at the right time. The no-upgrade-listener branch still calls maybeEmitClose() because it drops the hijacked socket immediately.

Addresses review comment on #29015: the hijacked socket's writable side was built around the dockerode pattern (POST + req.flushHeaders + req.write without req.end) and broke for the standard WebSocket / Playwright CDP / `websocket` npm package pattern (GET + req.end() BEFORE the 101). In that pattern req.finished becomes true synchronously inside req.end(), so every socket.write() in the 'upgrade' listener errored with ERR_STREAM_WRITE_AFTER_END (fixes #18945, #9911, #20547 which this PR originally claimed to fix). Solution: give upgrade requests a long-lived body generator whose exit condition is decoupled from req.finished. - hasUpgradeHeaders(req) detects a Connection: Upgrade + Upgrade: <proto> pair (ignoring h2/h2c, matching Bun__fetch_ in fetch.zig). - startFetch() forces isDuplex=true and forces the streaming-body branch for upgrade requests regardless of method or the req.finished state, funneling any pre-assembled body (from req.write before req.end) through kBodyChunks so the generator picks it up. - The generator's exit while-loop diverges: upgrade requests wait on a new 'upgradeBodyEnded' flag instead of self.finished. - kEndUpgradeBody (local Symbol) is installed on 'this' and flips the flag + resolves the pending chunk promise. The hijacked socket's _final() and _destroy() call it to release the upload half. - _write() no longer rejects when req.finished is true (that's expected for the WebSocket pattern). - req.end() no longer calls resolveNextChunk(true) for upgrade requests, so the generator doesn't exit before the 101 arrives. Adds a regression test using a GET + req.end() echo upgrade server that sends 101 on headers alone and echoes post-upgrade bytes. Fails (ERR_STREAM_WRITE_AFTER_END) without this commit, passes with it.

…ead code Addresses three issues from the follow-up review: 1. req.end(body) on upgrade fired a second nodeHttpClient request. When customBody was defined (body supplied via req.end(body) without prior flushHeaders), isDuplex was computed BEFORE customBody got cleared — so keepOpen stayed false, the .finally() reset fetching, and the first post-upgrade socket.write() re-entered startFetch and fired a second HTTP request to the same URL. Clear customBody BEFORE the isDuplex computation so keepOpen goes true. New test 'upgrade + req.end(body) without flushHeaders' catches this — fails with connectionCount === 2 without the fix. 2. Non-101 response on upgrade-headed request leaked the generator. The body generator's exit condition is !upgradeBodyEnded, and the flag is only flipped from createUpgradeSocket — which is only constructed on a 101 response. For a 400/404/etc. on an upgrade-headed request the generator parked forever, holding the ResumableSink / FetchTasklet / ClientRequest closure alive. Call this[kEndUpgradeBody]() in the fetch .then handler when the request had upgrade headers but got a non-101 response. 3. Dead code in _final(). kEndUpgradeBody is unconditionally set in the ClientRequest constructor, so the typeof guard is always true and the ~30-line fallback was unreachable. Delete it. Also reworded the comment in end() — dockerode never calls req.end(), it uses flushHeaders + write only.

… race Matches CLAUDE.md's rule against using setTimeout in tests. The double-request regression would fire a second TCP connection synchronously from socket.write(), so instead of sleeping 50ms and checking the count, race the echo round-trip against a promise that rejects on the server's 2nd 'connection' event. Fails immediately (with a descriptive error) if the bug recurs, and doesn't add a CI-load-dependent window when the fix is in place.

Two review follow-ups: - _http_client.ts: rename the inner 'const isUpgrade = response.status === 101' to 'is101' so it no longer shadows the outer 'const isUpgrade = hasUpgradeHeaders(this)' in go(). The two concepts (request carried Upgrade headers vs. server granted 101) diverge on a rejected upgrade — giving them different names makes the already- spelled-out distinction in the '!is101 && hasUpgradeHeaders(this)' guard self-documenting and protects against a future refactor silently capturing the wrong binding. - test(29012): the 'req.end(body) without flushHeaders' test's inline server discarded bytes after \r\n\r\n only on the FIRST 'data' event. If the kernel delivered the 4-byte 'init' body as a separate 'data' event (no coalescing with the headers), the server would echo it as hijacked-protocol data, the client's 'Buffer.concat(chunks) === "post"' check would never match, and the test would hang to timeout. Replace with a strict phase state machine (headers → consume exactly Content-Length bytes → upgraded) and relax the client assertion to endsWith('post').

Now that the inner 'const isUpgrade = response.status === 101' was renamed to 'is101' (commit 7f7fba7), the outer 'const isUpgrade = hasUpgradeHeaders(this)' is reachable from the .then handler. Use it directly instead of re-calling hasUpgradeHeaders(this) — same source of truth as the body generator's loop condition at line 613, so the guard cannot drift from the generator even if the user mutates 'Upgrade:' or 'Connection:' headers between go() and the .then() callback firing. Also avoids a redundant getHeader x2 + regex per response.

@alii

Review feedback from @alii: hasUpgradeHeaders() does two getHeader calls + regex test, and the PR was calling it 2x per request (once in this.end, once in go()). Cache the result in a closure variable set on the first startFetch() call (guarded by the 'fetching' flag, so it's set exactly once). Both this.end and go() now read the cached value; the .then leak-fix guard was already using the captured outer binding. this.end is guaranteed to see the cached value because it runs send() → startFetch() (which sets the flag) synchronously before the resolveNextChunk check. Subsequent re-entries into startFetch from pushChunk (writeCount > 1) are no-ops via the fetching guard, so the flag is written exactly once per request.

@claude

…rade_state Two review nits from @claude: - _http_client.ts: createUpgradeSocket._write gated on req.destroyed (deliberately not req.finished — see the comment block), so throwing ERR_STREAM_WRITE_AFTER_END here was miscategorizing a destroyed-socket write as a post-end() write. Use ERR_STREAM_DESTROYED('write') to match net.Socket / Writable semantics; ERR_STREAM_WRITE_AFTER_END stays for its current callsite in this.end. - http.zig: the request_body_has_framing scan in buildRequest() ran unconditionally on every HTTP request, but the flag is only read under 'upgrade_state == .pending' in writeToStream(). Gate the scan on 'upgrade_state != .none' — upgrade_state is set authoritatively by the first header loop above, so by the time we reach this block we already know whether we're an upgrade. Saves ~10-20 wyhash calls per non-upgrade request with zero correctness change (the consumer short-circuits on upgrade_state, so the flag value is unobservable for non-upgrade requests).

Claude review nit on the new 'upgrade' emit site. The fetch-routed architecture enqueues post-101 bytes into response.body before handleResponse runs; they flow through the IncomingMessage → Duplex bridge as the first socket.on('data') chunk instead of arriving synchronously in the 'head' argument. Standard consumers (ws, dockerode-modem, websocket npm) are unaffected because they do 'if (head.length) socket.unshift(head)' and then read from the socket. Document this as a known limitation.

Claude review 🔴: the body generator's main loop delivers one chunk per resolveNextChunk wake. Back-to-back synchronous writes followed by socket.end() on the hijacked Duplex — an idiomatic net.Socket pattern — used to leave later chunks stranded: socket.write(A) → pushChunk(A), resolveNextChunk(A), set to undefined socket.write(B) → pushChunk(B), resolveNextChunk?.() is no-op (undef) socket.end() → _final → kEndUpgradeBody → upgradeBodyEnded = true (generator resumes, yields A, loop check `!upgradeBodyEnded` is false, exits; B stranded in kBodyChunks) Fix: drain any remaining chunks after the main loop exits, before handleResponse?.(). Same shape applies to req.end() on non-upgrade streaming requests, so the drain is unconditional. Adds a regression test that writes three chunks back-to-back + ends — the echo server returns the full concatenated string only when every chunk reaches the wire.

Zig compiler ICE in Sema.handleExternLibName on Windows x64 build-zig for unrelated src/sys/windows/windows.zig (GetModuleFileNameW extern). This file is not touched by this PR — the ICE is a Zig 0.15.2 compiler flake.

robobun · 2026-05-11T22:13:29Z

Ready for a maintainer. CI on #1d36c13 fails only on chronic Windows flakes that hit main and every PR:

test/js/bun/jsc-stress/jsc-stress.test.ts — mprotect failed: 487 on Windows 11 aarch64 (also in builds 51401/51403/51405/51413/51415/51431)
test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts — timeout on Windows (13+ of last 25 builds across main and PRs)
test/js/bun/http/proxy.test.ts — 1-retry x64-asan flake

None exercise the http-upgrade code path. Used my one CI re-roll (previous build 53476 hit an unrelated Zig 0.15.2 compiler ICE in Sema.handleExternLibName while analyzing src/sys/windows/windows.zig's GetModuleFileNameW extern — also not touched by this PR; the re-roll cleared that). Local regression coverage is 9/9 green (6 tests for this issue + the 3 new tests from main's related #30377). All prior review threads resolved.

robobun · 2026-06-12T19:09:17Z

Superseded by #32204 for the client-side upgrade event. The native changes here target src/http/http.zig and FetchTasklet.zig, which are dormant reference files since the Rust port, so this branch no longer changes shipped behavior. The chunked-body-before-101 half of #29012 (writeToStream draining while Pending) still needs a native fix in src/http/lib.rs.

robobun · 2026-06-17T22:23:55Z

Closing as obsolete.

This PR made the old fetch-based ClientRequest emit 'upgrade' on a 101 response. Since then, #31587 rewrote the node:http client from scratch as a net/tls + llhttp port of Node.js's lib/_http_client.js. That client emits 'upgrade' natively with a real net/tls socket and the actual parser-overflow bytes in head, so the whole fetch-based code path this PR modified no longer exists.

Verified the original #29012 repro (dockerode exec.start({ hijack: true }): POST + Upgrade + Transfer-Encoding: chunked, no req.end()) against current main:

UPGRADE OK: {"status":101,"upgrade":"tcp","head":"ready"}

The 'upgrade' event fires, the 101 is delivered, and head carries the post-upgrade bytes (which this PR had documented as a known limitation of the fetch-based approach). Nothing here is worth rebasing onto the rewrite.

github-actions Bot added the claude label Apr 8, 2026

robobun mentioned this pull request Apr 8, 2026

dockerode exec.start({ hijack: true, stdin: true }) hangs under Bun but works under Node.js #29012

Open

coderabbitai Bot reviewed Apr 8, 2026

View reviewed changes

Comment thread src/http.zig Outdated

Comment thread src/js/node/_http_client.ts