From 142c4393be904b0d442dcb76abb654ef1052efa4 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 9 Jul 2026 06:51:05 +0000 Subject: [PATCH 1/4] node:http: defer ServerResponse.destroy() 'close' and stop rewriting completed requests as aborted ServerResponse.prototype.destroy() emitted 'close' synchronously from inside the call (so res.closed was already true when destroy() returned), and the socket _destroy path synchronously destroyed the request before its queued EOF could flip readableEnded/complete, firing 'aborted' on a fully-received request and dropping its pending 'end'. In Node.js res.destroy() destroys the socket and the socket's close callback (onServerResponseClose/abortIncoming) tears the request and response down on a later tick. Route the teardown through the same path: drop the synchronous 'close' emission from ServerResponse.destroy (the socket's #onClose schedules it, with a nextTick fallback when no socket is attached), drop the synchronous req.destroy() from #closeHandle (the same #onClose runs it after pending nextTicks), and keep the socket attached after a synchronous in-handler destroy so #onClose can still see it. With the fix, for a fully-received request res.destroy() matches Node.js exactly: destroy() returns with res.closed === false, 'end' is delivered, 'aborted' is not, req.complete stays true, and res 'close' follows req 'close' on a later tick. --- src/js/node/_http_server.ts | 32 ++++++++------ test/js/node/http/node-http.test.ts | 66 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 14 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 7b7893d31cfb..848cc2b55160 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -833,8 +833,13 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort if (handle.finished || didFinish) { handle = undefined; - http_res[kCloseCallback] = undefined; - http_res.detachSocket(socket); + // A destroyed-but-not-closed response: leave the socket attached so + // the socket close path (#onClose) can emit res 'close' and destroy + // the request (Node.js: socketOnClose → onServerResponseClose). + if (http_res._closed || !http_res.destroyed) { + http_res[kCloseCallback] = undefined; + http_res.detachSocket(socket); + } return; } if (http_res.socket) { @@ -1112,13 +1117,9 @@ const NodeHTTPServerSocket = class Socket extends Duplex { this[kHandle] = undefined; handle.onclose = this.#onCloseForDestroy.bind(this, callback, err); handle.close(); - // lets sync check and destroy the request if it's not complete - const message = this._httpMessage; - const req = message?.req; - if (req && !req.complete) { - // at this point the handle is not destroyed yet, lets destroy the request - req.destroy(); - } + // Do not req.destroy() here: #onClose (scheduled as a task from the native + // close path) does it after pending nextTicks, so a fully-received request + // is not rewritten as aborted (Node.js's socketOnClose → abortIncoming). } #onClose() { this[kHandle] = null; @@ -2416,11 +2417,14 @@ ServerResponse.prototype.destroy = function (err?: Error) { if (handle) { handle.abort(); } - this?.socket?.destroy(err); - if (!this._closed) { - // res.closed must already be true inside the 'close' listeners. - this._closed = true; - this.emit("close"); + // Writable.destroy semantics: 'close' is emitted on a later tick. The + // socket close path (#onClose → emitCloseNT) handles it when a socket is + // attached; otherwise schedule it here. + const socket = this.socket; + if (socket) { + socket.destroy(err); + } else { + process.nextTick(emitCloseNT, this); } return this; }; diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index 261e2007baa2..4b40f69874a1 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -3726,6 +3726,72 @@ it("http.Agent with proxyEnv does not write to a literal 'undefined' property", } }); +describe.each([ + ["before any write", (_res: ServerResponse) => {}, undefined], + [ + "after writeHead + partial body", + (res: ServerResponse) => { + res.writeHead(200, { "content-length": "50" }); + res.write("xx"); + }, + undefined, + ], + [ + "with an error argument", + (res: ServerResponse) => { + res.writeHead(200, { "content-length": "50" }); + res.write("xx"); + }, + Object.assign(new Error("boom"), { code: "EBOOM" }), + ], +])("ServerResponse.destroy() %s", (_name, setup, err) => { + it("defers 'close' and leaves a fully-received request complete (not aborted)", async () => { + const events: string[] = []; + let closedAtReturn: boolean | undefined; + let reqRef!: IncomingMessage; + + const { promise: resClosed, resolve: resolveResClose } = Promise.withResolvers(); + const { promise: reqClosed, resolve: resolveReqClose } = Promise.withResolvers(); + + await using server = createServer((req, res) => { + reqRef = req; + // Consume the (empty) body so 'end' is due on this request. + req.on("data", () => {}); + req.on("aborted", () => events.push("req.aborted")); + req.on("end", () => events.push("req.end")); + req.on("close", () => { + events.push("req.close"); + resolveReqClose(); + }); + res.on("close", () => { + events.push("res.close"); + resolveResClose(); + }); + setup(res); + events.push("call-destroy"); + res.destroy(err); + closedAtReturn = res.closed; + events.push("destroy-returned"); + }).listen(0, "127.0.0.1"); + await once(server, "listening"); + + const client = connect((server.address() as AddressInfo).port, "127.0.0.1", () => { + client.write("GET /x HTTP/1.1\r\nHost: h\r\n\r\n"); + }); + client.on("error", () => {}); + await once(client, "close"); + await Promise.all([resClosed, reqClosed]); + + // Writable.destroy semantics: 'close' is emitted on a later tick. + expect(closedAtReturn).toBe(false); + expect(events.indexOf("destroy-returned")).toBeLessThan(events.indexOf("res.close")); + // The server destroying its own response is not a client abort: the + // already-received request still gets 'end' and stays complete. + expect(events).toEqual(["call-destroy", "destroy-returned", "req.end", "req.close", "res.close"]); + expect({ aborted: reqRef.aborted, complete: reqRef.complete }).toEqual({ aborted: false, complete: true }); + }); +}); + it("OutgoingMessage outputData is per-instance and _flushOutput is defined", () => { expect(typeof OutgoingMessage.prototype._flushOutput).toBe("function"); From 7c32ef52f24d17eb3c28f2ab8bdd87a7550358d8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 9 Jul 2026 07:58:58 +0000 Subject: [PATCH 2/4] read socket storage directly in res.destroy; move tests to their own file The ServerResponse socket getter auto-creates a FakeSocket, so the no-socket fallback was unreachable and a standalone new ServerResponse() never emitted 'close' after destroy(). Read this[fakeSocketSymbol] directly (same pattern as end()/setTimeout()) and schedule emitCloseNT whenever there is no native handle, which covers the FakeSocket case. Also move the tests to test/js/node/http/node-http-server-response-destroy.test.ts so the gate runs them in isolation, and add coverage for the standalone response path. --- src/js/node/_http_server.ts | 14 +-- .../node-http-server-response-destroy.test.ts | 89 +++++++++++++++++++ test/js/node/http/node-http.test.ts | 66 -------------- 3 files changed, 98 insertions(+), 71 deletions(-) create mode 100644 test/js/node/http/node-http-server-response-destroy.test.ts diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 848cc2b55160..69aaaa757f89 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -2417,13 +2417,17 @@ ServerResponse.prototype.destroy = function (err?: Error) { if (handle) { handle.abort(); } - // Writable.destroy semantics: 'close' is emitted on a later tick. The - // socket close path (#onClose → emitCloseNT) handles it when a socket is - // attached; otherwise schedule it here. - const socket = this.socket; + // Writable.destroy semantics: 'close' is emitted on a later tick. Read the + // storage directly: the `socket` getter auto-creates a FakeSocket that does + // not route back to emitCloseNT. + const socket = this[fakeSocketSymbol]; if (socket) { socket.destroy(err); - } else { + } + // Native server path: the socket's #onClose schedules emitCloseNT(this). + // Without a handle (standalone response) schedule it here; emitCloseNT is + // guarded by _closed so a later socket close emission is a no-op. + if (!handle) { process.nextTick(emitCloseNT, this); } return this; diff --git a/test/js/node/http/node-http-server-response-destroy.test.ts b/test/js/node/http/node-http-server-response-destroy.test.ts new file mode 100644 index 000000000000..b1f262352c59 --- /dev/null +++ b/test/js/node/http/node-http-server-response-destroy.test.ts @@ -0,0 +1,89 @@ +/** + * ServerResponse.destroy() must follow Writable.destroy semantics: 'close' is + * emitted on a later tick (res.closed is still false when destroy() returns), + * and the server tearing down its own response must not retroactively rewrite + * an already fully-received request as a client abort. + * + * These tests also pass in Node.js. + */ +import { describe, expect, it } from "bun:test"; +import { once } from "node:events"; +import { createServer, IncomingMessage, ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; +import { connect } from "node:net"; + +describe.each([ + ["before any write", (_res: ServerResponse) => {}, undefined], + [ + "after writeHead + partial body", + (res: ServerResponse) => { + res.writeHead(200, { "content-length": "50" }); + res.write("xx"); + }, + undefined, + ], + [ + "with an error argument", + (res: ServerResponse) => { + res.writeHead(200, { "content-length": "50" }); + res.write("xx"); + }, + Object.assign(new Error("boom"), { code: "EBOOM" }), + ], +])("ServerResponse.destroy() %s", (_name, setup, err) => { + it("defers 'close' and leaves a fully-received request complete (not aborted)", async () => { + const events: string[] = []; + let closedAtReturn: boolean | undefined; + let reqRef!: IncomingMessage; + + const { promise: resClosed, resolve: resolveResClose } = Promise.withResolvers(); + const { promise: reqClosed, resolve: resolveReqClose } = Promise.withResolvers(); + + await using server = createServer((req, res) => { + reqRef = req; + // Consume the (empty) body so 'end' is due on this request. + req.on("data", () => {}); + req.on("aborted", () => events.push("req.aborted")); + req.on("end", () => events.push("req.end")); + req.on("close", () => { + events.push("req.close"); + resolveReqClose(); + }); + res.on("close", () => { + events.push("res.close"); + resolveResClose(); + }); + setup(res); + events.push("call-destroy"); + res.destroy(err); + closedAtReturn = res.closed; + events.push("destroy-returned"); + }).listen(0, "127.0.0.1"); + await once(server, "listening"); + + const client = connect((server.address() as AddressInfo).port, "127.0.0.1", () => { + client.write("GET /x HTTP/1.1\r\nHost: h\r\n\r\n"); + }); + client.on("error", () => {}); + await once(client, "close"); + await Promise.all([resClosed, reqClosed]); + + // Writable.destroy semantics: 'close' is emitted on a later tick. + expect(closedAtReturn).toBe(false); + expect(events.indexOf("destroy-returned")).toBeLessThan(events.indexOf("res.close")); + // The server destroying its own response is not a client abort: the + // already-received request still gets 'end' and stays complete. + expect(events).toEqual(["call-destroy", "destroy-returned", "req.end", "req.close", "res.close"]); + expect({ aborted: reqRef.aborted, complete: reqRef.complete }).toEqual({ aborted: false, complete: true }); + }); +}); + +it("standalone ServerResponse.destroy() defers 'close' to a later tick", async () => { + const res = new ServerResponse(new IncomingMessage(undefined as any)); + const { promise: closed, resolve } = Promise.withResolvers(); + res.on("close", resolve); + res.destroy(); + expect({ destroyed: res.destroyed, closed: res.closed }).toEqual({ destroyed: true, closed: false }); + await closed; + expect(res.closed).toBe(true); +}); diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index 4b40f69874a1..261e2007baa2 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -3726,72 +3726,6 @@ it("http.Agent with proxyEnv does not write to a literal 'undefined' property", } }); -describe.each([ - ["before any write", (_res: ServerResponse) => {}, undefined], - [ - "after writeHead + partial body", - (res: ServerResponse) => { - res.writeHead(200, { "content-length": "50" }); - res.write("xx"); - }, - undefined, - ], - [ - "with an error argument", - (res: ServerResponse) => { - res.writeHead(200, { "content-length": "50" }); - res.write("xx"); - }, - Object.assign(new Error("boom"), { code: "EBOOM" }), - ], -])("ServerResponse.destroy() %s", (_name, setup, err) => { - it("defers 'close' and leaves a fully-received request complete (not aborted)", async () => { - const events: string[] = []; - let closedAtReturn: boolean | undefined; - let reqRef!: IncomingMessage; - - const { promise: resClosed, resolve: resolveResClose } = Promise.withResolvers(); - const { promise: reqClosed, resolve: resolveReqClose } = Promise.withResolvers(); - - await using server = createServer((req, res) => { - reqRef = req; - // Consume the (empty) body so 'end' is due on this request. - req.on("data", () => {}); - req.on("aborted", () => events.push("req.aborted")); - req.on("end", () => events.push("req.end")); - req.on("close", () => { - events.push("req.close"); - resolveReqClose(); - }); - res.on("close", () => { - events.push("res.close"); - resolveResClose(); - }); - setup(res); - events.push("call-destroy"); - res.destroy(err); - closedAtReturn = res.closed; - events.push("destroy-returned"); - }).listen(0, "127.0.0.1"); - await once(server, "listening"); - - const client = connect((server.address() as AddressInfo).port, "127.0.0.1", () => { - client.write("GET /x HTTP/1.1\r\nHost: h\r\n\r\n"); - }); - client.on("error", () => {}); - await once(client, "close"); - await Promise.all([resClosed, reqClosed]); - - // Writable.destroy semantics: 'close' is emitted on a later tick. - expect(closedAtReturn).toBe(false); - expect(events.indexOf("destroy-returned")).toBeLessThan(events.indexOf("res.close")); - // The server destroying its own response is not a client abort: the - // already-received request still gets 'end' and stays complete. - expect(events).toEqual(["call-destroy", "destroy-returned", "req.end", "req.close", "res.close"]); - expect({ aborted: reqRef.aborted, complete: reqRef.complete }).toEqual({ aborted: false, complete: true }); - }); -}); - it("OutgoingMessage outputData is per-instance and _flushOutput is defined", () => { expect(typeof OutgoingMessage.prototype._flushOutput).toBe("function"); From e1597e76e92321ec03444650803f71a96514a9d6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 9 Jul 2026 09:06:59 +0000 Subject: [PATCH 3/4] ci: retrigger From bb231be76fc706e3054afbed542b083561e867ac Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 9 Jul 2026 09:26:20 +0000 Subject: [PATCH 4/4] test: swallow client ECONNRESET without relying on once() events.once() installs its own error listener that rejects the awaited promise; a separate no-op error listener does not suppress it. Use an explicit close resolver so an expected ECONNRESET from the force-closed connection cannot fail the test. --- .../js/node/http/node-http-server-response-destroy.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/js/node/http/node-http-server-response-destroy.test.ts b/test/js/node/http/node-http-server-response-destroy.test.ts index b1f262352c59..4cf4fa9ad892 100644 --- a/test/js/node/http/node-http-server-response-destroy.test.ts +++ b/test/js/node/http/node-http-server-response-destroy.test.ts @@ -61,12 +61,15 @@ describe.each([ }).listen(0, "127.0.0.1"); await once(server, "listening"); + const { promise: clientClosed, resolve: resolveClientClosed } = Promise.withResolvers(); const client = connect((server.address() as AddressInfo).port, "127.0.0.1", () => { client.write("GET /x HTTP/1.1\r\nHost: h\r\n\r\n"); }); + // The server force-closes the connection; an ECONNRESET here is expected + // and must not fail the test (once(client, "close") would reject on it). client.on("error", () => {}); - await once(client, "close"); - await Promise.all([resClosed, reqClosed]); + client.on("close", () => resolveClientClosed()); + await Promise.all([clientClosed, resClosed, reqClosed]); // Writable.destroy semantics: 'close' is emitted on a later tick. expect(closedAtReturn).toBe(false);