http: support client upgrade event#28828
Open
alii wants to merge 1 commit into
Open
Conversation
Collaborator
|
Updated 1:48 PM PT - May 30th, 2026
❌ @robobun, your commit 27eef2e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28828That installs a local version of the PR into your bun-28828 --bun |
Contributor
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds HTTP upgrade duplex flow: a new UpgradedSocket Duplex implementation, client-side upgrade buffering/backpressure with a fetch-body generator for Upgrade requests, and tests covering upgrade events, bidirectional IO, lifecycle behaviors, and a regression reproducer for Dockerode attach. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/js/internal/http/UpgradedSocket.ts`:
- Around line 46-52: The _write method currently calls callback() immediately
after this.#channel.push(buffer), allowing unbounded queue growth; change it to
respect backpressure by only acknowledging the write after the transport has
consumed the chunk: call this.#channel.push(buffer) and, if that push indicates
the channel is full (e.g., returns false or otherwise signals backpressure),
register a one-time drain/consumed event on the `#channel` to invoke callback(),
otherwise call callback() immediately; update the logic in _write (and any
related channel push API usage) to rely on the channel's backpressure signal
rather than always calling callback() right away.
In `@src/js/node/_http_client.ts`:
- Around line 453-469: After creating the UpgradedSocket instance in the 101
upgrade branch, assign the new socket to the request and response socket fields
before emitting: set this.socket = socket and res.socket = socket (and if
present, also set this.connection = socket) so the live upgraded connection
replaces the pre-upgrade placeholder and teardown paths like req.destroy()
operate on the actual upgraded socket; then emit "upgrade" as before. Use the
existing symbols: this, res, socket, and the UpgradedSocket instance created
from response.body.
- Around line 93-116: The upgrade body channel currently snapshots buffered
chunks (createUpgradeChannel(initialChunks) uses initialChunks.slice()), so
later req.write() appends to this[kBodyChunks] but the active upgrade stream
(upgradeBodyGenerator) reads only the snapshot and misses subsequent writes; fix
by making createUpgradeChannel reference the live buffer instead of copying it
(set chunks = initialChunks || [] rather than slice()) so the channel and
this[kBodyChunks] share the same array, and ensure push/end operate on that
shared array; apply the same change to the duplicate code path referenced around
lines 374-387.
🪄 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: 20bff52b-9ec9-4b61-b9ed-4284ee75b320
📒 Files selected for processing (4)
src/js/internal/http/UpgradedSocket.tssrc/js/node/_http_client.tstest/js/node/http/node-http-client-upgrade.test.tstest/regression/issue/18982.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Member
Author
|
@robobun adopt |
Collaborator
|
📝 Reading review feedback — need to address:
|
Collaborator
|
📝 New review feedback (4 items from claude[bot]):
Fixing... |
Contributor
There was a problem hiding this comment.
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/js/internal/http/UpgradedSocket.ts`:
- Around line 86-140: The address() method is returning the remote server
endpoint from `#parseAddress`() instead of the socket's local binding; change
address() to return the local endpoint (use the localAddress, localFamily, and
localPort getters or stored local info) so it matches net.Socket.address()
semantics, and modify localAddress/localPort/localFamily to derive or store real
local binding values (instead of hardcoded placeholders) when the socket is
bound/connected; update any places that relied on `#parseAddress`() for local info
to use the proper local fields and keep remoteAddress/remotePort/remoteFamily
returning `#parseAddress`() results.
In `@src/js/node/_http_client.ts`:
- Around line 527-529: The code currently always calls this.emit("upgrade", res,
socket, kEmptyBuffer) which leaves the upgraded socket open when no 'upgrade'
listeners exist; change the logic in the upgrade handling block to check for
listeners (e.g., use this.listenerCount?.("upgrade") or
this.listeners("upgrade").length) before emitting and only emit if a listener
exists; if there are no listeners, immediately destroy the socket (and/or call
socket.destroy()) and then call maybeEmitClose() to match Node.js behavior;
update the section around the this.emit("upgrade", ...) and maybeEmitClose()
calls accordingly so unhandled 101 upgrades are closed.
🪄 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: 652fb8fd-cca6-433a-a2b2-624a84f1037d
📒 Files selected for processing (3)
src/js/internal/http/UpgradedSocket.tssrc/js/node/_http_client.tstest/js/node/http/node-http-client-upgrade.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/js/internal/http/UpgradedSocket.ts`:
- Around line 94-149: `#parseAddress`() currently fabricates defaults ("", 0,
"IPv4") when this.#url is undefined or not a string; update it to detect
Unix-socket upgrades (when this.#url is undefined/null or not a valid URL
string) and cache a neutral address info object with address: undefined, port:
undefined, family: undefined in this.#addressInfo instead of the
empty-string/zero defaults so the getters remoteAddress, remotePort and
remoteFamily reflect "unset" rather than lying about a TCP peer; locate and
change logic in `#parseAddress` (and any use of this.#addressInfo/#url) to return
and cache those undefined fields.
- Around line 32-54: The socket timeout is set as a one-shot and never re-armed,
so activity in async `#pump`() or _write() won't reset the idle timer; update both
`#pump`() (inside the loop after receiving/pushing a chunk, e.g., just after
bytesRead update or after this.push(buf) succeeds) and the _write() handler
(when write activity occurs) to call this.setTimeout(this.timeout) or the
instance's configured timeout value to re-arm the idle timer on each read/write
event; ensure you only call setTimeout when a timeout value is configured and
preserve existing error/close behavior (refer to methods `#pump`() and _write()).
In `@src/js/node/_http_client.ts`:
- Around line 559-564: The upgrade-event emission currently happens inside the
nodeHttpClient(...).then(...) promise chain (the emit("upgrade", res, socket,
kEmptyBuffer) call), which lets listener exceptions reject the promise and
spuriously trigger the go(url, proxy, true).catch(iterate) retry path; move the
emit("upgrade", ...) call out of the fetch promise chain and schedule it via
process.nextTick (same pattern used for the "response" event) so listener
exceptions are thrown asynchronously and do not convert into promise rejections;
update the block around nodeHttpClient, the emit("upgrade", ...) call, and
ensure maybeEmitClose() behavior remains unchanged (keep maybeEmitClose() after
the scheduled emit).
🪄 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: f87d1e8f-8dc0-43ad-92b2-78af62fc62e8
📒 Files selected for processing (2)
src/js/internal/http/UpgradedSocket.tssrc/js/node/_http_client.ts
Contributor
There was a problem hiding this comment.
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/js/internal/http/UpgradedSocket.ts`:
- Around line 190-205: The setTimeout method in UpgradedSocket must validate its
arguments like ClientRequest.setTimeout: import and call
getTimerDuration(timeout) to validate/normalize the timeout and assign the
normalized value to this.timeout, and if a callback is provided call
validateFunction(callback, 'callback') before using it; keep the existing Node
semantics for timeout === 0 (remove listener) and for non-zero timeouts (once +
setTimeout), and ensure you throw the same validator error codes by reusing
internal/validators (getTimerDuration and validateFunction) and update the
top-level imports to include them.
- Around line 23-26: The constructor for UpgradedSocket currently sets
this.authorized = this.encrypted which incorrectly equates TLS encryption with
certificate verification; change the constructor/signature to accept an explicit
authorization flag or the underlying socket (e.g., a TLSSocket) and set
this.authorized from the real TLS verification result (socket.authorized) when
available, falling back to false if verification info is absent; update any
callers that instantiate UpgradedSocket to pass the socket or an authorized
boolean so authorized reflects actual certificate verification rather than
url.startsWith("https:").
🪄 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: c637bd3d-994b-41f4-9db4-7bfc2358431e
📒 Files selected for processing (2)
src/js/internal/http/UpgradedSocket.tssrc/js/node/_http_client.ts
Contributor
There was a problem hiding this comment.
The two outstanding issues from the prior review round — the missing this.destroyed guard in the nextTick callback and silent write-callback success on no-arg socket.destroy() — are both fixed in the final commits. All previously-reported bugs across this review cycle have been addressed. Given the size and production-criticality of this change, it warrants a human sign-off before merge.
Extended reasoning...
Overview\nPR #28828 adds client-side HTTP upgrade event support to Bun's http.ClientRequest. It introduces a new UpgradedSocket Duplex class (240 lines), significantly modifies _http_client.ts, and adds 600+ lines of tests. This is a substantial new feature touching production HTTP networking.\n\n### Security Risks\nNo authentication or authorization code is modified. The authorized field on UpgradedSocket correctly reflects the TLS verification state. The fetch-based implementation inherits Bun's existing TLS handling. No obvious injection or privilege-escalation vectors were introduced.\n\n### Level of Scrutiny\nHigh. This PR adds a complex async state machine (upgrade channel, body generator, happy-eyeballs integration) that bridges fetch internals with Node.js stream semantics. It touches the critical _http_client.ts path used by all HTTP requests. Multiple rounds of review found and fixed over 20 bugs; while the final state looks correct, the complexity and production impact justify human sign-off.\n\n### Other Factors\nAll bugs reported across multiple review rounds by both this bot and coderabbitai have been addressed in the commit history. The final diff includes the this.destroyed guard, the ERR_STREAM_DESTROYED error propagation fix, allowHalfOpen: false, keepOpen for the upgrade path, kBodyChunks clearing, backpressure fixes, and more. The test coverage is comprehensive (563 lines). No new bugs were found in this review pass.
Member
Author
|
@robobun rebase and get this mergable |
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
41214fc to
d1383b5
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
31cdf9c to
c9d9bf2
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
c9d9bf2 to
1be295f
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
1be295f to
639c09b
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
2 times, most recently
from
2bd42b3 to
19f2251
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
19f2251 to
326c404
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
326c404 to
60cbc2f
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
60cbc2f to
d68714d
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
2 times, most recently
from
1680bfc to
0ccec10
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
0ccec10 to
09c3125
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
09c3125 to
78bf763
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
78bf763 to
afdf6c3
Compare
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
afdf6c3 to
1749d5d
Compare
Emit 'upgrade' on http.ClientRequest when the server responds 101, with a usable Duplex socket bridging the underlying fetch streams. Gates duplex: "half" on the Upgrade header so non-upgrade requests keep their connection-reuse behavior. socket.destroy() aborts the underlying fetch so the TCP connection is torn down and the native request's event-loop ref is released — without this the process hangs after a socket is destroyed (the teardown path ws/playwright rely on). The idle-timeout timer is unref'd to match net.Socket. Fixes #18982.
robobun
force-pushed
the
ali/http-client-upgrade
branch
from
1749d5d to
27eef2e
Compare
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Emit
'upgrade'onhttp.ClientRequestwhen the server responds 101, with a usable Duplex socket bridging the underlying fetch streams. Gatesduplex: "half"on theUpgradeheader so non-upgrade requests keep their connection-reuse behavior.This makes the real
wsnpm package work (it does its handshake viahttp.request+req.on('upgrade')), which unblocks Playwright's bundled ws —chromium.connectOverCDP('ws://...')now connects and round-trips against a real Chrome (#9911). Thehttp://form (where Playwright first fetches/json/versionvia the same agent) still hangs at CDP response delivery; that's a separate interaction not addressed here.Supersedes #22412 and #28629.
Fixes #18982