Skip to content

node:http: fix a process exit when a tunnel socket with a pending response is given to emit('connection') - #46421

Open
robobun wants to merge 1 commit into
mainfrom
robobun/b5ddfb28/http-emit-connection-inflight-crash
Open

robobun wants to merge 1 commit into
mainfrom
robobun/b5ddfb28/http-emit-connection-inflight-crash

Conversation

@robobun

@robobun robobun commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • An uncaught TypeError: inflight?.[kHttp1ResponseHandle]?.socketEnded is not a function ends the process. One client connection does it: a request with a pending response, a CONNECT behind it, and a 'connect' listener that gives the socket to a second server with emit('connection'). Node v26.3.0 stays up.
  • That server's 'drain', 'finish' and 'close' listeners call socketDrained(), socketEnded() and flushed() on the handle of the response that has the socket (src/js/internal/http1_server_fallback.ts:668, :677, :681, :687). Here that is the first server's pending response, whose native handle has none of them.

Fix

  • The four calls are optional (?.()).
  • Only the JS handle has these methods. They settle a response of the second server.
  • Verified: should leave the response ahead alone when a second server gets the socket in test/js/node/http/node-http-connect.test.ts. Main prints drain, then the TypeError, and exits with code 1. Each call, reverted alone, brings the TypeError back.
  • Self-reviewed: 2 concerns for this part, 2 addressed (Notes).

Background

  • A server parses accepted sockets natively. It parses a socket from emit('connection') in JS (internal/http1_server_fallback), where responses write through a JS handle.
  • A CONNECT parsed while a response is pending goes to 'connect' at once. The pending response keeps the socket until it ends.
  • Considered a marker on the JS handle. The later fix for pipelined requests on such a socket needs one, this crash does not.

Downsides

  • Requests on such a tunnel can still stay unanswered (a separate change).
  • +8 bytes of embedded JS. onHttp1SocketDrain has 22 instructions, was 21. Sockets from listen() have none of these listeners.
Notes

Repro (runs under Node and Bun). The response to /first never ends. The client closes when the tunnel is established.

const http = require("node:http"), net = require("node:net");
const second = http.createServer();
const server = http.createServer((req, res) => void res.write("first"));
server.on("connect", (req, socket) => {
  socket.on("close", () => { console.log("close"); server.close(); });
  socket.write("HTTP/1.1 200 Connection Established\r\n\r\n");
  second.emit("connection", socket);
});
server.listen(0, "127.0.0.1", () => {
  const client = net.connect(server.address().port, "127.0.0.1");
  client.on("data", chunk => String(chunk).includes("Established") && client.destroy());
  client.write("GET /first HTTP/1.1\r\nHost: a\r\n\r\nCONNECT a:1 HTTP/1.1\r\nHost: a:1\r\n\r\n");
});

Node v26.3.0 prints close and exits with code 0. Main throws the TypeError and exits with code 1.

What was run (debug build, Linux x64, base 491aaad. Main has moved 9 commits since, none changes these two files.)

  • The test, by name: it fails on main's source (stdout drain, the TypeError in stderr, exit code 1) and passes with the fix.
  • Each optional call reverted alone in the built module: the fixture exits with code 1 and the TypeError of that call (socketDrained, socketEnded in 'finish', flushed, socketEnded in 'close').
  • The fixture of the test under Node v26.3.0: drain, close, exit code 0.
  • A client that only sends its FIN also triggers it. With default options the second server ends the socket, and main throws the TypeError at 'finish' (3 of 3 runs). Node and the fix do not (3 of 3 each).
  • The README example of proxy-chain 3.0.1 for customConnectServer, which gives the tunnel socket to an in-process server this way. One connection that sends a proxied GET to an upstream that never answers, then a CONNECT: the process exits with code 1 on main's JS (2 of 2 runs) and stays up with the fix and on Node (2 of 2 each). The client was written to trigger it. How often an ordinary client does was not measured.
  • The whole of node-http-connect.test.ts does not pass on this debug build, with or without the fix: should handle backpressure and tests should run on bun time out at 5 s on main's source too. The new test passed in every run (3 of 3).
  • Size: the module grows from 18,615 to 18,623 bytes in the release bundle, and the blob that the binary embeds by the same 8 bytes (bundle-modules.ts --debug=OFF). No release binary was linked. The other three call sites are the same edit in anonymous listeners, and their instructions were not counted.

Self-review. The crash was first part of a larger change for pipelined requests on such a socket. The review asked for it as a separate pull request, and for a 'drain' trigger in the test that does not depend on how much one write the kernel takes. The test now writes until write() returns false.

Left alone, the same on main. With httpAllowHalfOpen set on the second server, the tunnel socket stays open after the client's FIN and the end of the first server's response. Node v26.3.0 closes it. Main does the same as this branch (3 of 3 runs each) and does not exit in that flow. The _last mark of the second server's 'end' listener goes to the queue entry that the pipelined CONNECT leaves behind, and nothing reads it there.

Not checked. Windows and macOS were not run. No issue reports this crash (tracker search for the three TypeError texts and for customConnectServer: 0 results). The path exists since #43557.

Open pull requests that edit the same listeners: #46014, #46141.

…ponse is given to emit('connection')

The listeners that internal/http1_server_fallback puts on a socket call
socketDrained(), socketEnded() and flushed() on the handle of the response
that has the socket. That response can be one of the listening server: a
CONNECT that was parsed behind it is handed to 'connect' at once, and the
listener can give the socket to a second server with emit('connection').
A native handle has none of these methods, so 'drain', 'finish' and 'close'
on the socket threw an uncaught TypeError. The four calls are now optional.
@robobun

robobun commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Status: open, waiting for CI and review.

How it was reproduced, on a debug build of main at 491aaad (Linux x64):

  1. A client sends GET /first and a CONNECT in one write. The server never ends the response to /first.
  2. The 'connect' listener answers and gives the socket to a second server with emit('connection').
  3. The client closes, or the socket drains.

Main then throws TypeError: inflight?.[kHttp1ResponseHandle]?.socketEnded is not a function (or the one for socketDrained or flushed) and exits with code 1. Node v26.3.0 and this branch exit with code 0. The script is in the Notes of the description.

The new test fails on main's source and passes with the fix. The rest of node-http-connect.test.ts has tests that time out at 5 s on a local debug build, with and without this change.

@robobun

robobun commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:14 PM PT - Oct 21st, 2026

✅ @robobun, your commit 15e647827ab39d933cfdd81bcaeb6df7620e19af passed in Build #130532! 🎉


🧪   To try this PR locally:

bunx bun-pr 46421

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

bun-46421 --bun

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 75551826-bfa7-4696-85f8-08b8c0410799
📥 Commits

Reviewing files that changed from the base of the PR and between f2de590 and 15e6478.

📒 Files selected for processing: 2
  • src/js/internal/http1_server_fallback.ts
  • test/js/node/http/node-http-connect.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

HTTP/1 fallback socket lifecycle callbacks now tolerate response handles without fallback-specific methods. A concurrent subprocess regression test covers CONNECT socket handoff to a second HTTP server while an earlier response remains open.

Changes

HTTP/1 socket handoff

Layer / File(s) Summary
Fallback lifecycle callbacks and regression test
src/js/internal/http1_server_fallback.ts, test/js/node/http/node-http-connect.test.ts
Drain, finish, and close handling optionally call socketDrained, socketEnded, and flushed when those methods exist. The regression test checks the drain and close scenarios during CONNECT socket handoff.

Possibly related issues

  • oven-sh/bun#43381 — Covers Node-compatible socket teardown, including node:http request sockets. This change addresses a teardown crash during pending-response socket handoff.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 15e64

No merge-blocking issue is identified; merge after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the HTTP module, the process-exit bug, and the tunnel socket handoff scenario. It accurately summarizes the primary change.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is complete and relevant. It explains the problem, the fix, scope, downsides, and verification results. It does not use the exact template headings, but it provides the required inform…
Full details: Description check

Explanation

The description is complete and relevant. It explains the problem, the fix, scope, downsides, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/js/internal/http1_server_fallback.ts — A proxy whose second server has httpAllowHalfOpen set leaks the tunnel connection after the client sends FIN: the first server's response ends later, but the socket is never closed. The 'end' listener at src/js/internal/http1_server_fallback.ts:656-658 sets _last = true on socket._httpMessage, which after a handoff is the first server's native response. Nothing in src/js/node/_http_server.ts reads _last, so the flag is dropped and the fallback's own close-on-finish at :519 never runs for that response. Fix: when socket._httpMessage is a native response (no fallback handle), end the socket after that response finishes (listen for its 'finish' and call socket.end()), or mark it with the native close flag the native finish path honours.

    Why this was flagged

    _last is read only by fallback responses (src/js/internal/http1_server_fallback.ts:519 reads this._last). The native ServerResponse in src/js/node/_http_server.ts never reads _last. Trigger: the same scenario the PR fixes (GET /first pending, CONNECT behind it, 'connect' hands the socket to second via emit('connection')) with second.httpAllowHalfOpen = true. The client sends FIN; the second server's onHttp1SocketEnd (:645) runs parser.finish() then, because httpAllowHalfOpen is true, sets _last on the native /first response at :658 instead of ending the socket. When /first later calls res.end(), the native finish path (onResponseFinishHandleSocket :2713) checks only kMustCloseConnection, so the socket stays open with the peer gone; Node's resOnFinish would socket.destroySoon() because _last is set. Before this PR the process died on the next drain/finish/close anyway, so the leak was masked; with the fix the process lives and accumulates half-open sockets.

    Verification: onHttp1SocketEnd with httpAllowHalfOpen true sets httpMessage._last = true (src/js/internal/http1_server_fallback.ts:656-658) on the first server's native ServerResponse. The only reader of _last is the fallback's own finish handler (fallback :519); the native finish path reads only kMustCloseConnection (src/js/node/_http_server.ts:2714). The socket is never ended or destroyed.

  • 🟣 src/js/internal/http1_server_fallback.ts — Proxies that hand a tunnel socket to an in-process server now stay up but every request the client sends on that tunnel is never answered while the first server's response is pending. The second server's parser queues its response behind socket._httpMessage (src/js/internal/http1_server_fallback.ts:504-505), and that queue is advanced only by the first server's native finish path. The PR fixes only the crash that hid this hang, so the proxy-chain customConnectServer flow goes from exit code 1 to a silent timeout. Fix: either assign the socket to the second server's response when the in-flight response belongs to another server, or land the deferred pipelined-tunnel change together with this one so the unblocked path has a defined outcome.

    Why this was flagged

    If a 'connect' listener on a native-listener server hands the socket to a second http.Server with emit("connection") while the first response is still pending (e.g. proxy-chain's customConnectServer), then any HTTP request the client writes into the tunnel is parsed by the fallback and reaches src/js/internal/http1_server_fallback.ts:504-505, where it is queued behind socket._httpMessage, the first server's native response. The queue is only advanced by advanceResponsePipeline (src/js/node/_http_server.ts:2923) from the first server's finish, which checks socket[kHandle]?.response and then dispatches via the native pipeline, not the fallback's; the fallback's own advance never runs. Before this PR the process crashed on the first drain/close so the hang was unreachable; after it the request hangs with no error, no timeout and no 'close' until the client gives up. Remedy: land the deferred queue-advance for foreign in-flight responses with this fix, or at least add a test pinning the hang so the follow-up has a failing test.

    Verification: The fallback parser hits src/js/internal/http1_server_fallback.ts:504-505, so the second server's response is parked behind the first server's native response. The only advance is advanceResponsePipeline at src/js/node/_http_server.ts:2706, whose native branch at :2976-2990 passes the fallback's JS handle to startPipelinedResponse, which returns false, so :2985-2988 destroys the response.

@robobun

robobun commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Both optional findings are about behaviour outside this diff. I ran them on a debug build of main at 491aaad, on this branch, and on Node v26.3.0.

Requests on the tunnel that stay unanswered. This is the first bullet of Downsides. While the first server's response is pending, Node does not answer them either. A second pull request for the requests on such a socket is in work. This one stays limited to the crash.

The socket that stays open with httpAllowHalfOpen on the second server. Confirmed: after the client's FIN and the end of the first response, the socket stays open (3 of 3 runs). Node closes it (3 of 3). Main without this change behaves like this branch (3 of 3). It does not exit in that flow: no 'drain', 'finish' or 'close' reaches the socket while the response is pending, so the TypeError does not fire. This change does not add the leak and does not unmask it.

One detail differs from the analysis. _last is not set on the response. lastPipelinedResponse() returns the queue entry that the pipelined CONNECT leaves behind, and the mark goes there: _last reads true on that entry and false on the response, on main and on this branch.

I added the case to the Notes of the description as left alone. The same runs showed a simpler trigger for the crash, which is now in the Notes too: with default options, a client that only sends its FIN makes main throw the TypeError at 'finish' (3 of 3).

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant