node:http: abort the in-flight request when an HTTP/1 fallback connection closes

[robobun]

Problem

• On the JS HTTP/1 fallback path (a socket handed to http.Server via server.emit("connection", socket), or an HTTP/1.1 connection on an allowHTTP1 http2 server), a request whose response has not finished is never told its connection went away. Node emits aborted, error (ECONNRESET) and close on req and marks it destroyed; bun emits only the response's close, so anything waiting on req waits forever.
• Reproduced with a reset mid-body, a peer FIN, closeAllConnections(), res.destroy() and a server-side socket.destroy(). Native http.Server connections already match node.
• Cause: the fallback's close handler freed the parser and stopped. Node's close path also destroys every request still waiting on its response.
• Smaller gap: a res.end() issued after the connection was destroyed or the peer hung up counted as the response finishing. It emitted finish and released the response before the close handler ran, and after a peer FIN also raised a clientError. Node writes nothing and never finishes such a response.

Fix

• When the connection closes, the request whose response is still assigned to the socket is destroyed with node's ConnResetException("aborted"). A response detaches when it finishes, so this is exactly node's set of unanswered requests, and an already answered request is left alone. The listener is registered ahead of each response's own, so req aborted precedes res close as in node. node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED #36991 rewrites this handler and includes the same abort; whichever lands second rebases one hunk.
• The fallback response handle now reports itself socket-closed as soon as the socket stops being writable, as the native handle does once its connection closed. ServerResponse already writes nothing and emits no finish in that state, so a late end() leaves the response assigned for the close handler to abort, and the stray finish and clientError go away.
• Two related cleanups: the Upgrade/CONNECT handoff also removes the close listener, so a tunnel closing later does not abort the upgrade request (node removes it too), and a req.destroy() in the socket end handler is deleted because the parser either rejects a truncated request or completes it first, so that branch never fired.
• Verification: new tests assert node v26's event sequences for reset, FIN, closeAllConnections(), res.destroy() and end-after-teardown (the last over real net and TLS sockets), plus two negatives (response already answered, upgraded tunnel). The abort tests fail on main with ["res-close"] as the only event. A scenario script printed the same results on node v26.3.0 and this build, apart from two pre-existing ordering differences noted in the original.

Background

• The HTTP/1 fallback: bun serves http.Server connections natively. Sockets bun did not accept itself (server.emit("connection", socket), or HTTP/1.1 chosen by ALPN on an allowHTTP1 http2 server) go through a JS port of node's connectionListener, with a JS object standing in for the native response handle.
• Node's abort-on-close contract: when a connection closes, every request still waiting on its response is destroyed with an ECONNRESET "aborted" error; IncomingMessage then emits aborted, error only if something listens, then close. Requests already answered are not touched.
• socket._httpMessage is the ServerResponse currently assigned to a socket; the fallback clears it when the response finishes. The native close path uses the same indicator to find the in-flight request.
• Response handle flags: ServerResponse reads a socket-closed bit from its handle and returns early from write()/end() when it is set, so the handle decides whether a late end() counts as finishing. The native handle sets the bit when its connection closes; the fallback handle never did.
• Close timing: net and TLS sockets emit close a turn after destroy(), so a handler can call res.end() in between. A duplexPair closes at once, which is why the end-after-teardown tests use real sockets.

Original description

Repro

On a connection served by src/js/internal/http1_server_fallback.ts (a socket fed to http.Server through server.emit("connection", socket), or an HTTP/1.1 connection on http2.createSecureServer({ allowHTTP1: true })), a request whose response has not finished is never told that its connection went away:

const http = require("http");
const { duplexPair } = require("stream");
const server = http.createServer((req, res) => {
  const events = [];
  req.on("aborted", () => events.push("aborted"));
  req.on("error", e => events.push("error:" + e.code));
  req.on("close", () => events.push("close"));
  res.on("close", () => events.push("res-close"));
  setImmediate(() => server.closeAllConnections());
  setTimeout(() => { console.log(JSON.stringify({ events, reqDestroyed: req.destroyed })); server.close(); }, 100);
});
server.listen(0, () => {
  const [client, conn] = duplexPair();
  server.emit("connection", conn);
  client.write("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 10\r\n\r\nabc");
});

node v26.3.0:       {"events":["aborted","res-close","error:ECONNRESET","close"],"reqDestroyed":true}
bun 1.4.0 / main:   {"events":["res-close"],"reqDestroyed":false}

Same result when the peer disconnects (TLS client destroyed mid-body on an allowHTTP1 server, FIN with the body cut short, FIN under an open long-poll/event-stream response), on res.destroy(), or on a server-side socket.destroy(): anything waiting on req ('aborted', 'error', 'close', stream.finished, req.destroyed) waits forever. Bun's native http.Server connections behave like node here; only the fallback path did not.

Cause

Node's socketOnClose (lib/_http_server.js) frees the parser and then runs abortIncoming(), which does req.destroy(new ConnResetException("aborted")) for every request still waiting for its response. IncomingMessage._destroy then emits 'aborted', and 'error' (ECONNRESET) only if something listens, then 'close'. The fallback's socket 'close' listener only did the first half (parser.close()).

A second, smaller gap showed up while reviewing the fix: the fallback's response handle treated every res.end() as the response finishing, even one issued after the connection had been destroyed or the peer had hung up, when nothing it writes can reach the wire. On a net or TLS socket 'close' arrives a turn after destroy(), so a res.end() in that window (a handler that tears the connection down and then ends the response, or one that ends it after the peer's FIN) emitted 'finish' and released the response, and an end() after the peer's FIN also surfaced as a 'clientError' from writing to the ended socket. Node never runs the finish callback for a response whose connection is gone, and the native NodeHTTPResponse reports itself closed in that state so ServerResponse returns early.

Fix

• The fallback's close listener destroys the in-flight request with a ConnResetException("aborted"), the call node's abortIncoming makes. Bun's IncomingMessage._destroy (the non-native branch this path uses) is a port of node's, so the event sequence, req.errored and the error-listener gating come out the same as node's. The request in flight is the one whose response is still assigned to the socket (socket._httpMessage.req): the fallback's responses detach on 'finish', where node's resOnFinish drops the request from state.incoming, and this is the same indicator the native socket's close path (NodeHTTPServerSocket) uses. A request whose response already went out is left alone (node does not abort it either), and res.destroy() leaves the response assigned, so that request is aborted like in node. The listener is registered when the connection is accepted, before any response's assignSocket() adds its own 'close' listener, so req 'aborted' precedes res 'close' as in node.
• The handle reports NodeHTTPResponseFlags.socket_closed (and, as the native handle derives it from the same bit, aborted) once the socket is no longer writable, like the native handle once its connection closed. ServerResponse's write()/end() already return without writing or emitting 'finish' in that state (end() still returns the response, as in node), so a response ended after the connection stopped being writable stays assigned and the close listener aborts its request; the stray 'finish' and the 'clientError' from the end-after-FIN case go away with it. aborted additionally requires the response not to have ended, so a normally completed response keeps reporting a later write()/end() as write-after-end.
• The Upgrade/CONNECT handoff removes the close listener along with the data/end/error listeners, as node's onParserExecuteCommon does, so a tunnel closing later does not abort the upgrade request (an 'upgrade' listener may have answered through a ServerResponse it assigned to the socket itself, which never detaches).
• The req.destroy() in the socket 'end' handler is removed: llhttp's finish() either reports HPE_INVALID_EOF_STATE for a request cut short or completes the message (EOF-delimited lenient bodies), so that branch was only ever reached with a complete request and never did anything. Node's socketOnEnd just ends the socket and lets the close path abort, which is what happens now.

Overlap note: #36991 (pipelining queue for the fallback) adds the in-flight abort inside its larger close-handler change, alongside the queued-response aborts it needs. This PR is the standalone fix for the bug on main; whichever lands second has a one-hunk rebase in that handler.

Verification

A scenario script run against node v26.3.0 and this build on the fallback path (events in order, then req.destroyed / req.aborted / req.errored) prints identical results for: a reset mid-body with and without an 'error' listener, a body-less request with a pending response, FIN with the body cut short, FIN under an open streaming response, res.end() issued after socket.destroy() / closeAllConnections() on a real net socket and after the peer's FIN, closeAllConnections(), a server-side socket.destroy(), req.destroy() from the handler, a response that finished before the connection died (not aborted), and a completed keep-alive exchange followed by an aborted second request. The remaining differences are pre-existing and unrelated to this change: res.destroy() emits the response's 'close' synchronously in bun, and the relative order of req 'end' vs res 'finish' on a normal exchange.

Tests (test/js/node/http/node-http.test.ts, next to the other connectionListener tests, and test/js/node/http2/node-http2.test.js next to the allowHTTP1 tests) assert node's event sequences for those scenarios. The end-after-teardown cases run over a real net socket pair and over an allowHTTP1 TLS connection, since a duplexPair emits 'close' too early to reach that window; they fail with the close listener alone and pass with the flag. Without the src change, every abort test fails with ["res-close"] as the only event; the two negative tests (response already finished, upgraded tunnel) pass both ways and guard the handoff change. The existing fallback coverage (test-http-generic-streams, test-http2-allow-http1, test-http2-https-fallback*, test-http-server-unconsume-consume, the *-per-stream tests, node-http-connect, and the full node-http / node-http2 files) still passes.

[coderabbitai]

Walkthrough

The HTTP/1 fallback now derives request state from socket state, cleans up socket closures through a named handler, aborts active requests with ECONNRESET, preserves upgraded connections, and avoids destroying incomplete requests during EOF. Tests cover HTTP and allowHTTP1 lifecycle behavior.

Changes

HTTP/1 request lifecycle

Layer / File(s)	Summary
Fallback socket cleanup src/js/internal/http1_server_fallback.ts	The fallback derives response flags from socket state, preserves incomplete requests during EOF, removes upgrade close listeners, and aborts active requests with ConnResetException("aborted") on socket close.
Node HTTP lifecycle tests test/js/node/http/node-http.test.ts	Tests cover connection resets, shutdown, peer closure, response destruction, completed responses, keep-alive requests, and upgraded requests.
allowHTTP1 disconnect coverage test/js/node/http2/node-http2.test.js	Tests verify event ordering, ECONNRESET, and request state when a TLS client disconnects during an incomplete HTTP/1.1 POST.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly and concisely describes the main HTTP/1 fallback request-abort fix.
Description check	✅ Passed	The description explains the problem, fix, scope, related behavior, and verification with extensive test coverage.

---

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

[robobun]

Status: reproduced on main with the script in the description (node v26.3.0 prints ["aborted","res-close","error:ECONNRESET","close"], main prints ["res-close"]), and on the allowHTTP1 TLS path with a client destroyed mid-body. Fix is in src/js/internal/http1_server_fallback.ts (close listener aborts the assigned request; the handle reports the connection closed once the socket stopped being writable, see the comments below). Tests in node-http.test.ts and node-http2.test.js fail without it and pass with it; the end-after-teardown cases additionally fail with only the close listener. CI is green on b163e75 (all 194 jobs); ready for review.

[claude]

LGTM — a focused port of Node's socketOnClose → abortIncoming() to the HTTP/1 fallback path.

What was reviewed:

• Verified ConnResetException is exported from internal/shared and socket._httpMessage.req is the right in-flight indicator (set by assignSocket, cleared by detachSocket on 'finish' — matches Node's state.incoming removal in resOnFinish).
• Checked listener ordering: onHttp1SocketClose is registered at connection time, before assignSocket()'s once('close', onServerResponseClose), so req 'aborted' precedes res 'close' as asserted.
• The removed req.destroy() in the 'end' handler was dead (llhttp finish() returns HPE_INVALID_EOF_STATE for a truncated body, taking the error branch first); the upgrade handoff removing the close listener mirrors Node's onParserExecuteCommon and is guarded by the new negative test.

Extended reasoning...

Overview

This PR makes the JS HTTP/1 server fallback (src/js/internal/http1_server_fallback.ts — used for server.emit('connection', duplex) and http2's allowHTTP1 ALPN fallback) abort the in-flight request when the connection closes, matching Node's socketOnClose → abortIncoming(). Previously anything waiting on req ('aborted', 'error', 'close', stream.finished, req.destroyed) would wait forever on this path when the peer disconnected mid-request.

The src change is ~15 lines: the anonymous close listener becomes a named onHttp1SocketClose that additionally does socket._httpMessage?.req.destroy(new ConnResetException('aborted')), the Upgrade/CONNECT handoff removes that listener along with the others, and a dead req.destroy() in the 'end' handler is dropped. ~220 lines of new tests in node-http.test.ts and ~40 in node-http2.test.js cover the abort scenarios (reset mid-body with/without error listener, closeAllConnections(), FIN cut short, body-less pending, res.destroy()), the negative cases (response already finished, upgraded tunnel, prior keep-alive request untouched), and the real TLS allowHTTP1 path.

Security risks

None. This is purely request-lifecycle event emission on server-side connection close. No parsing changes, no new user-controlled input paths, no auth/crypto. The ConnResetException is the same shared class the native path and _http_client already use.

Level of scrutiny

Medium. HTTP server request lifecycle is user-observable and event ordering matters, but the change is a direct port of Node's well-documented abortIncoming() behavior, only affects the fallback path (native http.Server connections already behave correctly per the PR description), and the native socket's #onClose already keys off the same _httpMessage indicator. I traced assignSocket/detachSocket in _http_server.ts to confirm socket._httpMessage is set/cleared exactly where Node's state.incoming is populated/drained, and confirmed the close-listener registration order guarantees the req 'aborted' → res 'close' sequence the tests assert.

Other factors

• The bug hunting system found no issues.
• Tests await observable conditions (dispatched promise, socket 'close', then one setImmediate for the deferred 'error'/'close' nextTick hops) rather than sleeping; the http2 test uses port: 0 and cleans up in finally.
• The PR description records byte-for-byte verification of the event sequences against Node v26.3.0 across nine scenarios, and states the negative tests pass both before and after (guarding the two secondary adjustments) while the three abort tests fail without the src change.
• The overlap with #36991 is called out; this is the standalone fix, and whichever lands second has a one-hunk rebase.

[github-actions]

This PR may be a duplicate of:

1. node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED #36991 - Its rewrite of the same socket close handler in src/js/internal/http1_server_fallback.ts already destroys the in-flight socket._httpMessage?.req with new ConnResetException("aborted"), the identical fix.
2. node:http: adopt external sockets fed in via server.emit('connection') #35285 - Ports Node's full connectionListener into _http_server.ts including socketOnClose → abortIncoming with ConnResetException("aborted"), giving the adopted-socket path the same abort-on-close behavior.

🤖 Generated with Claude Code

[robobun]

Not a duplicate of either, though the overlap with #36991 is real and is noted in the description.

• node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED #36991 fixes a different bug (the pipelining queue, ERR_HTTP_SOCKET_ASSIGNED on a second request) and, as part of rewriting the close handler for the queued responses, also destroys the in-flight request. That hunk is the overlap. It differs in two ways from this PR: it destroys without an error when no 'error' listener is attached (node passes the ConnResetException either way, so req.errored is ECONNRESET without a listener; the test here checks that), and its close listener stays anonymous, so it is still attached after an Upgrade/CONNECT handoff and a closing tunnel would abort the upgrade request; this PR removes it at the handoff like node does and has a test for it. This PR is the small standalone fix for the abort bug on main; whichever of the two lands second has a one-hunk rebase in that handler, and the tests here apply to both.
• node:http: adopt external sockets fed in via server.emit('connection') #35285 predates http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432, which added internal/http1_server_fallback.ts; it ports node's connectionListener into _http_server.ts instead and is currently conflicting with main. It does not touch the code path this PR fixes.

[robobun]

^{Updated 12:23 PM PT - Aug 12th, 2026}

✅ @robobun, your commit b163e75381b099e84b369152370bbaad94e5b513 passed in Build #93256! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37748

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

bun-37748 --bun

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/http/node-http.test.ts`:
- Around line 4192-4272: Split the six independent scenarios in
connectionListener aborts the in-flight request when the connection closes, like
Node into separate test cases, preserving each scenario’s setup, assertions, and
event expectations. Use distinct descriptive test names (or it.each with
scenario names) so failures identify the specific abort trigger and one failure
does not prevent the remaining scenarios from running.

🪄 Autofix

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: 179c042f-d70b-46d7-bd58-578ef7744c9c

📥 Commits

Reviewing files that changed from the base of the PR and between f59f705 and 577800f.

📒 Files selected for processing (3)

• src/js/internal/http1_server_fallback.ts
• test/js/node/http/node-http.test.ts
• test/js/node/http2/node-http2.test.js

[claude]

LGTM — focused Node-compat fix porting socketOnClose → abortIncoming into the HTTP/1 fallback path.

What was reviewed:

• Verified ConnResetException is exported from internal/shared and IncomingMessage._destroy's non-native branch emits 'aborted' + gates 'error' on listeners, matching Node.
• Confirmed socket._httpMessage?.req is the right in-flight indicator: assignSocket sets it, the existing res.on('finish', detachSocket) clears it, so finished responses are correctly excluded.
• Checked the removed req.destroy() in onHttp1SocketEnd was dead — llhttp finish() returns HPE_INVALID_EOF_STATE for a truncated body, taking the error branch first.
• Close-listener ordering (registered before every assignSocket's listener) and its removal at Upgrade/CONNECT handoff both trace correctly.

Extended reasoning...

Overview

The PR touches three files: a ~15-net-line change to src/js/internal/http1_server_fallback.ts (the JS-side HTTP/1 connection listener used for server.emit('connection', duplex) and http2's allowHTTP1 fallback), plus ~250 lines of new tests across node-http.test.ts and node-http2.test.js. The src change converts the anonymous socket 'close' listener into a named function that additionally destroys the in-flight request with new ConnResetException('aborted'), removes that listener at the Upgrade/CONNECT handoff alongside the other listeners, and drops a dead req.destroy() from the 'end' handler.

Security risks

None identified. This is a Node-compat behavioral fix in request-lifecycle event emission on connection close. No auth, crypto, path handling, or untrusted-input parsing is involved. The only new call is req.destroy(err) on an object the server itself constructed.

Level of scrutiny

Medium. It's a Node-compat change to a well-defined reference behavior (Node's lib/_http_server.js socketOnClose/abortIncoming), scoped to the fallback path only — the native http.Server path is untouched and already behaves correctly per the description. I traced the mechanism end-to-end: ServerResponse.prototype.assignSocket sets socket._httpMessage = this and adds its own 'close' listener; the fallback's existing res.on('finish', () => this.detachSocket(socket)) nulls _httpMessage, so socket._httpMessage?.req is exactly the request whose response hasn't finished — Node's state.incoming equivalent. The fallback registers its close listener at connection accept, before any assignSocket, so it fires first and req 'aborted' precedes res 'close', matching Node's order. IncomingMessage._destroy (non-native branch, _http_incoming.ts:416) sets aborted, emits 'aborted', and lets the stream machinery emit 'error' only when listened for — the same behavior the tests assert. The removed if (req && !req.complete) req.destroy() in the 'end' handler was unreachable with an incomplete request because parser.finish() returns an HPE_INVALID_EOF_STATE error on truncated bodies and the function returns early via onHttp1SocketError.

Other factors

Test coverage is thorough and follows the repo's review guidance: nine scenarios (three parametrized close triggers, no-error-listener, body-less GET with pending response, res.destroy(), keep-alive second-request abort, an allowHTTP1 TLS integration test) plus two negative guards (finished-response not aborted, upgraded tunnel not aborted). Tests await observable conditions (dispatched, serverSideClosed, then one setImmediate for the deferred nextTick hops) rather than sleeping, assert exact event orders taken from Node v26, and check req.destroyed/aborted/errored. All prior bot feedback (comment-cop on long comments, CodeRabbit on splitting scenarios) is resolved in the current head. The overlap with #36991 is acknowledged in the description and is a one-hunk rebase for whichever lands second.

[robobun]

Pushed 98ce1ca (plus a comment trim in 5087201) after a self-review pass over the first version turned up a gap in it.

The close listener keys the abort off the response still assigned to the socket, and the response releases itself on 'finish'. The fallback handle reported every res.end() as finishing the response, including one issued after the connection was already gone. On a net or TLS socket 'close' arrives a turn after destroy(), so a handler doing req.socket.destroy(); res.end(...) (or ending the response after the peer's FIN) released the response before the close listener ran and the request still was not aborted; the end() after a FIN also surfaced as a 'clientError' from writing to the ended socket. Node never runs the finish callback for such a response, and the native handle reports itself closed in that state. The handle now reports socket_closed once the socket is no longer writable, so ServerResponse's existing early return applies and the response stays assigned for the close listener.

The duplexPair-based tests could not see this (a duplexPair emits 'close' on the next tick, ahead of the deferred 'finish'), so the new cases run over a real net socket pair and over an allowHTTP1 TLS connection; they fail with the close listener alone and pass with the flag. Also added the graceful-FIN case (completed request, streaming response still open), which is the shape that goes through the socket 'end' handler this PR touched. Description updated accordingly.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes request-lifecycle semantics on the node:http fallback connection path (event ordering, the flags getter that now short-circuits res.end(), and the removed req.destroy() in the 'end' handler) and overlaps with #36991, a human look from someone familiar with the http server would still be worthwhile.

What was reviewed:

• Verified ConnResetException (internal/shared) and NodeHTTPResponseFlags.socket_closed (internal/http) exist and that ServerResponse in _http_server.ts reads handle.flags (never writes it), so the data-property → getter change is safe.
• Checked the close-handler abort keys off socket._httpMessage?.req, which the fallback's 'finish' listener detaches — matches Node's resOnFinish/abortIncoming shape.
• Confirmed the Upgrade/CONNECT handoff already removes data/error/end listeners; adding the close listener to that set matches Node's onParserExecuteCommon.
• Test imports (createNetServer, connect, duplexPair, AddressInfo) all present; each abort trigger is a separate it per the resolved CodeRabbit feedback.

Extended reasoning...

Overview

The PR modifies src/js/internal/http1_server_fallback.ts (the JS HTTP/1 server path used for server.emit('connection', socket) and http2.createSecureServer({ allowHTTP1: true })) so that a request whose response has not finished is aborted with ConnResetException('aborted') when the connection closes, matching Node's socketOnClose → abortIncoming. It also (a) turns the handle's flags data property into a getter that reports socket_closed once the socket is no longer writable so ServerResponse.end() on a dead connection does not emit 'finish', (b) removes the close listener at the Upgrade/CONNECT handoff, and (c) drops a req.destroy() in the socket 'end' handler that the description argues was unreachable. ~290 lines of new tests in node-http.test.ts and ~70 in node-http2.test.js cover a wide variant matrix (reset mid-body, closeAllConnections, FIN with truncated body, res.destroy(), end-after-destroy on real net/TLS sockets, keep-alive second request, and negative guards for finished responses and upgraded tunnels).

Security risks

None identified. This is server-side request lifecycle event emission; no parsing, validation, auth, or crypto changes. The ConnResetException is the same class Node uses.

Level of scrutiny

Moderate-to-high. The runtime diff is small (~25 lines) but it changes user-observable event semantics on a Node.js compat path — 'aborted'/'error'/'close' ordering, req.destroyed/req.errored, and whether res.end() after teardown emits 'finish'. The repo's review guidance calls out Node/Web compat as a situational area, and CodeRabbit suggested the http server owners as reviewers. The flags getter is a behavior change beyond the headline abort fix: any res.write()/end() on this fallback path now silently no-ops once socket.writable flips false. The description and tests argue this matches Node and the native handle, and I found no writes to handle.flags that the getter would break, but it is the kind of subtle change a maintainer should confirm.

Other factors

• All bot feedback (comment-cop on long comments, CodeRabbit on splitting scenarios) is resolved in the current revision.
• The PR description explicitly notes a one-hunk overlap with open PR #36991 in the same close handler; whichever lands second needs a rebase, so a maintainer should be aware of the coordination.
• The removed if (req && !req.complete) req.destroy() in onHttp1SocketEnd is argued to be dead because parser.finish() either errors (HPE_INVALID_EOF_STATE) on a truncated request or completes it; that reasoning is sound but is a behavior removal a maintainer may want to double-check.
• Tests await observable conditions (promises resolved from event handlers, one setImmediate hop after socket 'close' to let the deferred req 'error'/'close' fire) rather than sleeps, use port: 0 / local servers, and assert exact event orders against Node v26 — they meet the repo's test-quality bar.

Given the Node-compat sensitivity and the cross-PR coordination, deferring to a human reviewer rather than auto-approving.

[robobun]

One clarification on the flags getter's reach, since that is the part of this change that goes beyond the abort itself: it only affects a res.write() / res.end() issued after the connection is destroyed or the server side has ended it. Before this change nothing from such a call reached the wire either: on a destroyed socket the bytes were dropped by the stream, and on an ended socket they raised ERR_STREAM_WRITE_AFTER_END on the socket, which the listener turned into a 'clientError'. What changes is that the response no longer emits 'finish' for them (so it stays assigned and its request gets aborted) and the stray 'clientError' is gone. The early return it takes is the one ServerResponse already takes for a native response whose connection closed, so the two paths now report the same thing in that state (both also leave res.writableEnded false after such an end()). Responses on a live connection are unaffected; the existing allowHTTP1 framing tests (close-delimited, HEAD, keep-alive) still pass.

Follow-up in b163e75: while checking the above I noticed that with only flags derived from the connection, res.end() in that state returned true (the flags early return) where the native response returns the response itself through the aborted check that runs first. The native handle derives aborted from the same socket-closed bit, so the fallback handle now does too, and the net socket tests assert that end() still returns the response there.

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (1)

src/js/internal/http1_server_fallback.ts (1)

447-458: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Release the parser on socket close.

onHttp1SocketClose closes the parser but leaves socket.parser and the data/end listeners active. Remove the parser-related listeners, clear parser.socket, and set socket.parser = null so later events cannot use the closed parser.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js/internal/http1_server_fallback.ts` around lines 447 - 458, Update
onHttp1SocketClose to fully release the HTTP parser when the socket closes:
remove the parser’s data and end listeners, clear parser.socket, and set
socket.parser to null after closing it. Preserve the existing parser.close and
inflight request-abort behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/js/internal/http1_server_fallback.ts`:
- Around line 447-458: Update onHttp1SocketClose to fully release the HTTP
parser when the socket closes: remove the parser’s data and end listeners, clear
parser.socket, and set socket.parser to null after closing it. Preserve the
existing parser.close and inflight request-abort behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1fdd21f7-be17-4a22-ab25-61437fe84beb

📥 Commits

Reviewing files that changed from the base of the PR and between 577800f and b163e75.

📒 Files selected for processing (3)

• src/js/internal/http1_server_fallback.ts
• test/js/node/http/node-http.test.ts
• test/js/node/http2/node-http2.test.js

[claude]

I reviewed this PR and didn't find any bugs. Because the flags/aborted getter change alters when ServerResponse short-circuits writes on the fallback path (beyond just the abort itself), and because of the noted one-hunk overlap with #36991, a human look would still be worthwhile.

What was reviewed:

• onHttp1SocketClose vs Node's socketOnClose/abortIncoming: socket._httpMessage?.req matches the detach-on-'finish' bookkeeping, and the listener is registered before assignSocket() so event order holds.
• The flags/aborted getters against ServerResponse.prototype.end/write in _http_server.ts:3108,3191,3239,3360 — they gate the same early returns the native handle takes; no external code assigns handle.aborted so the getter conversion is safe.
• The removed req.destroy() in onHttp1SocketEnd: parser.finish() either errors (→ close path aborts) or completes the message, so the branch was dead.
• Tests: event-driven waits (no sleeps), negative cases (finished response, upgraded tunnel) guard against over-aborting, and the net/TLS variants cover the destroy-then-end window a duplexPair can't reach.

Extended reasoning...

Overview

The PR ports Node's socketOnClose → abortIncoming behavior to Bun's HTTP/1 fallback path (src/js/internal/http1_server_fallback.ts), used for server.emit('connection', socket) and http2's allowHTTP1. The source change is ~40 lines: the socket 'close' listener now destroys the in-flight request with ConnResetException('aborted'); the response handle's flags and aborted become getters derived from socket.writable so a res.end() after the connection is gone doesn't emit 'finish' (and so stays assigned for the abort); the Upgrade/CONNECT handoff removes the close listener; and a dead req.destroy() in the 'end' handler is dropped. ~360 lines of new tests cover reset/FIN/closeAllConnections/res.destroy() triggers, the no-error-listener case, the destroy-then-end window on real net and TLS sockets, and the two negative cases (response already finished, upgraded tunnel).

Security risks

None identified. This is server-side lifecycle-event plumbing on connections the application already accepted; no new parsing, no trust-boundary changes, no user-controlled input reaching new sinks. ConnResetException and NodeHTTPResponseFlags are existing internal helpers already used on the native path.

Level of scrutiny

Medium-high. The close-listener half is a straightforward port of Node's abortIncoming. The flags/aborted getter half is more subtle: it changes when ServerResponse.prototype.end/write (_http_server.ts:3108, 3191, 3239, 3360) take their early-return path on the fallback. I traced this and it only fires when socket.writable is false (destroyed or server-side ended), where writes were already dropped or raised ERR_STREAM_WRITE_AFTER_END; it aligns the fallback with the native NodeHTTPResponse's socket_closed bit. The !handle.ended guard on aborted correctly keeps a normally-completed response reporting write-after-end rather than silently returning this. Still, this is the part where a maintainer's eye on the equivalence claim vs the native handle would be valuable, since it reaches every fallback response, not just the abort scenario.

Other factors

• Test quality is high per REVIEW.md: event-driven awaits (Promise.withResolvers on 'close'), a single setImmediate hop to drain the known process.nextTick deferral of 'error'/'close', exact event-order assertions against Node v26, negative tests guarding the handoff removal and the finished-response exclusion, and variant coverage (duplexPair / net.Socket / TLS allowHTTP1).
• NodeHTTPResponseFlags is a const enum already require-imported the same way in _http_server.ts, so the bundler pattern is established.
• No external code writes handle.aborted, so converting it to a getter (and dropping this.aborted = true from abort()) is safe — abort() still destroys the socket, which flips the getter.
• All bot feedback (comment-cop on comment length, CodeRabbit on splitting scenarios) has been addressed and resolved.
• There is a known one-hunk overlap with #36991 in the same close handler; whichever lands second needs a small rebase, and the tests here would catch the two behavioral gaps in #36991's version noted in the thread.