Repository navigation
Conversation
…ted, not a clean 'end' When req.destroy() is called while an EOF-delimited response body (no Content-Length, no Transfer-Encoding) is streaming, the socket teardown delivers an 'end' to socketOnEnd, which calls parser.finish(). For an EOF-delimited body that runs parserOnMessageComplete, so res.complete is already true by the time socketCloseListener checks it and the locally aborted, truncated download is handed to the user as a clean 'end' with no 'aborted' or ECONNRESET. Chunked bodies were already correct because parser.finish() does not complete a mid-chunk message. Node never reaches socketOnEnd after req.destroy() (its socket.destroy() emits only 'close'), so parser.finish() runs at the bottom of socketCloseListener, after the "!res.complete" abort check. Guard socketOnEnd on !req.destroyed so a teardown-driven 'end' defers parser finalisation to socketCloseListener the same way.
|
Updated 8:54 AM PT - Jul 16th, 2026
❌ @robobun, your commit 83ef475 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34360That installs a local version of the PR into your bun-34360 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesHTTP client teardown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/js/node/_http_client.ts`:
- Around line 602-608: Update the comment above the req.destroyed check in
socketCloseListener to state that this is local teardown after req.destroy(),
and that the response remains incomplete and is reported as aborted with
ECONNRESET rather than receiving a clean end event.
In `@test/js/node/http/node-http.test.ts`:
- Around line 3905-3908: Replace the parameterized it.each block around the HTTP
response scenarios with describe.each(), placing a normal it() test inside each
generated case. Preserve both EOF-delimited and chunked scenario inputs and
their expectedComplete values.
🪄 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: f0244a43-76ff-4b4f-989c-55cf1bbfbe33
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/js/node/http/node-http.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes event-emission ordering in the node:http client teardown path, which interacts with keep-alive/agent reuse and the recent #32488 lifecycle work — worth a quick human look.
What was reviewed:
- Traced every setter of
req.destroyed— the other three (socketCloseListener,responseKeepAlive, upgrade path) either run after'close'or removesocketOnEndfirst, so the guard only trips onClientRequest.prototype.destroy. - Confirmed the skipped
freeParseris still reached at the bottom ofsocketCloseListener, so no parser leak. - Checked the legitimate-server-FIN path (no
req.destroy()):req.destroyedis false, existing behavior unchanged. - Test's
expectedComplete: truefor EOF-delimited matches Node (parser.finish() runs after the abort check insocketCloseListener).
Extended reasoning...
Overview
Adds an early-return guard at the top of socketOnEnd in src/js/node/_http_client.ts: when req.destroyed is already true, skip parser.finish() / freeParser() and just socket.destroy(), deferring finalization to socketCloseListener. Paired with an it.each test in test/js/node/http/node-http.test.ts covering EOF-delimited and chunked framing, asserting the exact event sequence (data, aborted, error:ECONNRESET) and res.complete against Node v26.3.0.
Security risks
None. No untrusted-input parsing, no auth/crypto, no new external surface. The guard reads a boolean already set by user-initiated req.destroy().
Level of scrutiny
Medium-high. The diff is 8 lines, but it sits in the node:http client socket lifecycle — a state machine where 'end'/'close' ordering, res.complete, keep-alive pooling, and agent socket reuse all interact. This file was just heavily reworked in #32488 for Node v26.3.0 compat, and the PR itself notes the fix is compensating for a Bun-specific net.Socket behavior (spurious 'end' on destroy, tracked separately in #33256). A human familiar with that work should confirm this guard doesn't regress any of the keep-alive / half-close / agent-free paths that #32488 stabilized.
Other factors
- I verified the PR's central claim that
req.destroyedcan only be true insocketOnEndviaClientRequest.prototype.destroy:responseKeepAliveand the upgrade handler bothremoveListener('end', socketOnEnd)before setting it, andsocketCloseListenerruns on'close'which fires after'end'. - The early return skips
freeParser, butsocketCloseListenerunconditionally callsparser.finish()+freeParser()at the end, so there's no leak — and this matches the Node ordering the PR is restoring (finish after the!res.completeabort check). - Test imports (
once,AddressInfo,createNetServer) are already present in the file;it.eachmatches existing local convention. Both CodeRabbit threads are resolved. - The PR states all 67
test/js/node/test/parallel/test-http-client-*.jsplus abort/keep-alive/response-close tests pass; CI build #73920 is in progress.
node-http.test.ts has an unrelated environment-dependent failure (the issue#4295 proxy test's server listens on ::1 while the client resolves localhost to 127.0.0.1 in some containers); keep the new coverage in a self-contained file so it runs clean everywhere.
There was a problem hiding this comment.
I didn't find any bugs. The guard is narrow and the root-cause analysis holds up, but this is node:http client socket/parser lifecycle — event-ordering-sensitive code where regressions are subtle — so it's worth a human sign-off.
Checked that req.destroyed can only be true ahead of socketOnEnd via ClientRequest.prototype.destroy (the other four setters either run on 'close', remove the 'end' listener first, or never attach it). Verified the skipped parser.finish()/freeParser() are still run at the tail of socketCloseListener, so no parser leak. The test's expectedComplete: true for the EOF-delimited case matches Node because parser.finish() in socketCloseListener runs after the abort check.
Extended reasoning...
Overview
Adds an early-return guard in socketOnEnd (src/js/node/_http_client.ts) so that a socket 'end' arriving after req.destroy() skips parser.finish() and defers finalization to socketCloseListener. Without this, an EOF-delimited body is marked complete before the close listener's abort check, so a locally-aborted download surfaces as a clean 'end' instead of 'aborted' + ECONNRESET. A new test file covers both EOF-delimited and chunked framing and asserts the exact event sequence and res.complete against Node v26.3.0.
Security risks
None. No new input parsing, no auth/TLS/permissions surface. The guard only reorders which existing teardown path runs the parser finalizer.
Level of scrutiny
Medium-high. The diff is 8 source lines, but it sits inside the node:http client socket-event lifecycle, where the ordering of 'end'/'close', parser finalization, and res.complete interacts with keep-alive reuse, upgrade/CONNECT, informational responses, and abort paths. The PR itself notes this is compensating for a Bun-specific net.Socket divergence (spurious 'end' on destroy(), tracked separately in #33256). Someone familiar with the recent _http_client.ts refactors should confirm this doesn't regress any of the adjacent lifecycle paths.
Other factors
- Traced every write to
req.destroyedin the file:socketCloseListener(runs on'close', after'end'), the upgrade branch andresponseKeepAlive(both remove thesocketOnEndlistener before setting it), andonSocketNT(never attaches the listener on that branch). So the guard is only reachable via explicitClientRequest.prototype.destroy, matching the PR's claim. - The skipped
parser.finish()/freeParser()are not leaked —socketCloseListenerruns them unconditionally at its tail aftersocket.destroy()fires'close'. - Author reports the full
test/js/node/test/parallel/test-http-client-*.jssuite plus abort/keep-alive/response-close tests pass. - CodeRabbit's two nits (comment wording,
it.eachvsdescribe.each) were addressed/resolved; no outstanding review threads.
|
CI on 83ef475 (build 73925, 283/286 passed):
None of these touch |
|
Closing this in favor of #33256. The diagnosis here is good and the test is unusually careful — it matches Node exactly, including the counterintuitive Verified against a real node v26.5.0 binary and against the CI builds of both PRs ( 1. "independent of #33256" doesn't hold — #33256 alone already fixes thisRunning this PR's own test against the CI build of #33256, with none of this PR's Byte-identical to Node on both framings. That follows from the diagnosis in the description: once the socket stops synthesizing an EOF on local destroy, 2. On the one path where the guard is live, it diverges from Node
import { request } from "node:http";
import { Duplex } from "node:stream";
const HEAD = "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n\r\npart1-";
class FakeSocket extends Duplex {
constructor() { super(); this.setNoDelay = this.setTimeout = this.ref = this.unref = () => this; }
_read() {}
_write(_c, _e, cb) { setImmediate(() => this.push(HEAD)); cb(); }
destroy(err) { if (!this.destroyed) this.push(null); return super.destroy(err); } // emits 'end' after destroy
}
const log = [];
const req = request({ createConnection: () => new FakeSocket(), host: "x", port: 80 }, res => {
res.on("data", () => { log.push("data"); req.destroy(); });
res.on("end", () => log.push("end"));
res.on("aborted", () => log.push("aborted"));
res.on("error", e => log.push("error:" + e.code));
res.on("close", () => console.log(log.join(",") + " | complete=" + res.complete));
});
req.end();Whatever one thinks of Node's behavior there, main and #33256 both match it and this PR doesn't. So the guard is either unreachable (real sockets, post-#33256) or wrong (custom sockets). Worth salvaging
|
… case req.destroy() while an EOF-delimited response body is streaming must surface as 'aborted' + ECONNRESET on the response, never a clean 'end'. Before this change the spurious socket 'end' reached socketOnEnd and finished the parser, so socketCloseListener saw the body as complete and delivered 'end' for a locally aborted, truncated download. The chunked case is the node-identical control. Moved from #34360 per review; fails on main, passes with the net-layer destroyed guard here, and matches Node v26.5.0 exactly (including res.complete === true on the EOF-delimited case).
|
Agreed on both points. The custom- Pushed |
… case req.destroy() while an EOF-delimited response body is streaming must surface as 'aborted' + ECONNRESET on the response, never a clean 'end'. Before this change the spurious socket 'end' reached socketOnEnd and finished the parser, so socketCloseListener saw the body as complete and delivered 'end' for a locally aborted, truncated download. The chunked case is the node-identical control. Moved from #34360 per review; fails on main, passes with the net-layer destroyed guard here, and matches Node v26.5.0 exactly (including res.complete === true on the EOF-delimited case).
2 participants
What
Calling
req.destroy()while an EOF-delimited response body (noContent-Length, noTransfer-Encoding; RFC 9112 6.3 rule 8) is streaming delivers a clean'end'on the response and never'aborted'/ECONNRESET. A locally aborted, truncated download is indistinguishable from a complete one. The same destroy on a chunked body is already Node-identical.This matters for abort/cancel plumbing (timeouts,
AbortSignal-driven destroys, user cancellation) on EOF-delimited origins: caches store the truncated prefix as the full body, retry logic never retries, metrics count the abort as a completed transfer.Why
req.destroy()destroys the socket, and the socket's'end'reachessocketOnEndbefore'close'reachessocketCloseListener.socketOnEndcallsparser.finish(), which for an EOF-delimited body runsparserOnMessageComplete(setsres.complete = trueand pushes EOF). By the timesocketCloseListenerchecksif (!res.complete)the response already looks complete, so theres.destroy(ConnResetException("aborted"))path is skipped andres.push(null)delivers'end'.Node never reaches
socketOnEndafterreq.destroy()because itssocket.destroy()emits only'close', soparser.finish()runs at the bottom ofsocketCloseListener, after the abort check. (#33256 fixes the underlyingnet.Socketspurious'end'; this change is independent of it.)Fix
Guard
socketOnEndon!req.destroyed: an'end'that arrives after the request was already destroyed is our own teardown, not a server FIN, so defer parser finalisation tosocketCloseListener.req.destroyedis only set ahead ofsocketOnEndbyClientRequest.prototype.destroy(the other setters remove the listener first or run after'close').Verification
New parametrised test in
test/js/node/http/node-http.test.tscovers both EOF-delimited and chunked framing and asserts the full event sequence plusres.completeagainst Node v26.3.0. Fails on bun 1.4.0 (["data","end"]), passes with this change. All 67test/js/node/test/parallel/test-http-client-*.jsand the abort/keep-alive/response-close tests pass.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file