[robobun]

Related to #37717

Behaviour change: none

Problem

• node:http: the server follows Node (request body, framing, response finish, lifecycle) #43557 and node:http: second listen() throws, close() reaps pre-request sockets #44032 made the idle sweep of close() and closeIdleConnections() follow Node.js on the HTTP/1 fallback path.
• The tests for that path cover five states: nothing sent, a partial first head, a partial second head, a pending response, and idle.
• I found no test on that path for these:
  • a request body that still arrives after the response ended
  • a sweep in the tick of res.end()
  • a sweep from the request listener
  • closeAllConnections()
  • Http2SecureServer#close() with a partial head

Fix

• Add four tests to test/js/node/http/node-http-server-close-drain.test.ts and one test to test/js/node/http2/node-http2.test.js.
• The tests come from node:http: only treat HTTP/1 fallback connections that are between requests as idle #37717, which node:http: the server follows Node (request body, framing, response finish, lifecycle) #43557 and node:http: second listen() throws, close() reaps pre-request sockets #44032 supersede. One expectation is different: a connection that has sent nothing is closed, like in Node.js since nodejs/node@417aacbc365.
• Verified: the five tests pass on a debug build of main (5 of 5 runs). Four fail on canary 367d939, which is older than node:http: the server follows Node (request body, framing, response finish, lifecycle) #43557. The fifth guards a state that was already correct. The four node:http tests also pass, unchanged, on Node.js v26.5.0 and v26.10.0.

Background

• The HTTP/1 fallback is src/js/internal/http1_server_fallback.ts. It serves the sockets from server.emit("connection", socket) and the HTTP/1.1 clients of an http2 server with allowHTTP1. A socket that a listening http.Server accepts does not use it.
• The sweep (closeIdleHttp1Connections) destroys a connection when no request is open on it and no response is pending. A request is open from its first byte to its end.

Notes

Result for each state. "Closed" and "kept" are the destroyed flag of the server side of the connection, read directly after the call.

State of the connection	Node.js v26.5.0, v26.10.0	main 9f70da0	canary 367d939
Nothing sent	closed	closed	closed
Part of the first request head	kept, then served	kept, then served	closed
Response ended, request body still arrives	kept, then served	kept, then served	closed
Request complete, response not ended	kept, then served	kept, then served	kept
One exchange done, part of the next head	kept, then served	kept, then served	closed
One exchange done	closed	closed	closed
Sweep in the tick of res.end()	closed	closed	closed
Sweep from the request listener	kept, the next sweep closes it	kept, the next sweep closes it	closed
closeAllConnections(), connection with a partial head	destroyed	destroyed	closeIdleConnections() destroys it before
closeAllConnections(), upgraded socket	left	left	left
allowHTTP1, close(), nothing sent	closed	closed	closed
allowHTTP1, close(), part of a request head	kept, then served, then 'close'	kept, then served, then 'close'	closed
allowHTTP1, close(), one exchange done	closed	closed	closed

The new tests.

Test	main	canary 367d939
closeIdleConnections() with emit('connection'): a connection in each state, one sweep	pass	fail
closeIdleConnections() with emit('connection'): closes a connection in the tick of its res.end()	pass	pass
closeIdleConnections() with emit('connection'): called in the request listener, leaves the connection of that request	pass	fail
closeAllConnections() with emit('connection'): destroys a connection that holds a partial head, leaves an upgraded socket	pass	fail
Http2SecureServer#close() leaves an allowHTTP1 connection that has sent part of a request head	pass	fail

Node.js runs. node-http-server-close-drain.test.ts imports bun:test and harness. A resolve hook (module.registerHooks) maps those two imports to a small shim, and Node.js runs the unchanged file. Only the four new tests were selected. node-http2.test.js has more imports that are specific to Bun, so for that test the same steps and assertions ran as a plain script: 30 of 30 runs pass on Node.js v26.5.0 and on v26.10.0.

Node.js v26.3.0 is older than nodejs/node@417aacbc365. There, the two tests that have a connection with nothing sent fail on that row only.

Other runs on the debug build of main. The whole node-http-server-close-drain.test.ts: 32 pass, 0 fail. node-http2.test.js -t "Http2SecureServer#close": 4 pass, 0 fail.

Run time. On the debug build the four node:http tests take 0.07 to 0.6 s each and the http2 test takes 0.26 to 0.35 s. On Node.js the four node:http tests take 1 to 43 ms each.

When the server drops a connection that a test waits on, the test fails at once with a message. readResponseBody() rejects on 'end', 'close' and 'error' of the client half. beforeClose() rejects when the server half closes before a request head arrives or before a listener runs.

The 'close' check of the http2 test is before the last request head is complete, while one connection is open. It is not after the last response: Node.js v26.5.0 and v26.10.0 emit the server's 'close' before the client has that response (30 of 30 runs), and main emits it after (30 of 30 runs). The two events come from different sockets, so the test does not assert their order.

Not carried over from #37717. http2 allowHTTP1 connections leave the server's connection list when they close reads the ConnectionsList that #37717 added. Main keeps the sockets in a Set. With .size in place of .all() the test passes on main, but it tests an internal field, so it is not included.

Found outside this change. On the fallback path, main does not apply headersTimeout, requestTimeout or keepAliveTimeout. The probe uses TCP sockets from emit('connection') and timeouts of 300, 600 and 300 ms. Node.js v26.5.0 and v26.10.0 close the three connections after 0.4, 0.7 and 1.3 s, the first one with a 408. Main closes none of them in 2.5 s. #37739 and #37724 are the open PRs for that. #37739 has the branch of #37717 as its base.

---

no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http-server-close-drain.test.ts

[robobun]

Status

This PR adds tests only. The five tests come from #37717, which is closed because #43557 and #44032 supersede it.

How I verified them (453a99a):

• Debug build of main (9f70da0): the five tests pass, 6 of 6 runs.
• Node.js v26.5.0 and v26.10.0: the four node:http tests pass, with the test file unchanged (3 of 3 runs). The steps of the http2 test pass as a plain script (30 of 30 runs).
• Canary 367d939, which is older than node:http: the server follows Node (request body, framing, response finish, lifecycle) #43557: four of the five tests fail. The fifth guards a state that was already correct.

CI: the diff is green. The one red test is not related to this PR.

• Build 121558, on 453a99a: the two changed test files passed at the first attempt on all 12 test lanes. These are Linux (glibc, musl and ASAN), macOS and Windows, x64 and aarch64.
• Builds 121538 and 121549, on the first version of the tests: the five new tests passed at the first attempt on all 11 test lanes.
• The red test of builds 121549 and 121558 is test/js/bun/spawn/spawn.test.ts on the ASAN lane. The red test of build 121538 is test/js/web/timers/setTimeout.test.js on the same lane. This PR touches neither file.

The PR is ready for a maintainer.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: Essentials

Run ID: 9a22736e-d4a1-46f3-bee0-cf97485408a5

📥 Commits

Reviewing files that changed from the base of the PR and between 9c328dd and 453a99a.

📒 Files selected for processing (2)

• test/js/node/http/node-http-server-close-drain.test.ts
• test/js/node/http2/node-http2.test.js

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

---

Walkthrough

Adds tests for HTTP server connection sweeps and server close behavior. The tests cover multiple request and response states, including partial request heads, pending responses, reused connections, and upgraded sockets. They also cover HTTP/1 connections accepted by an HTTP/2 secure server.

Changes

HTTP connection closing

Layer / File(s)	Summary
HTTP server connection sweeps test/js/node/http/node-http-server-close-drain.test.ts	Adds helpers and tests for sweep behavior across active, idle, reused, pending-response, and upgraded connections. The tests also cover sweep timing and closeAllConnections().
HTTP/1 connections on secure server close test/js/node/http2/node-http2.test.js	Adds a test for close() on an allowHTTP1 secure server. Fresh and completed connections close, while a connection with a partial request head completes its request before closing.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 453a9

This PR adds regression coverage for HTTP connection closing; no concrete user-facing or test-workflow failure is established, so it appears ready after normal checks.

🚥 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 identifies the main change: tests for idle-sweep behavior by connection state on the HTTP/1 fallback path. It does not mention the related HTTP/2 test, but it accurately summarizes t…
Description check	✅ Passed	The description explains the problem, test changes, verification results, background behavior, and test evidence. It does not use the exact template headings, but it contains the required information …

---

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

[robobun]

^{Updated 4:28 AM PT - Sep 29th, 2026}

❌ @robobun, your commit 453a99a has 1 failures in Build #121558 (All Failures):

• test/js/bun/spawn/spawn.test.ts - code 1: stdout reader of an unref'd child and process lifetime > an idle reader stopped at the highwater … on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 44203

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

bun-44203 --bun

[claude]

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

[claude]

LGTM — 453a99a addresses the three earlier inline notes, and this run found nothing new.

What was reviewed:

• readResponseBody now settles on 'end' as well as 'close'/'error', and beforeClose wraps every head-arrival and listener wait in the four node:http tests, so a wrongly destroyed duplexPair half rejects instead of hanging; listener cleanup in the finally avoids a late rejection escaping the race.
• The http2 test's partialHeadArrived rejects on 'close', and the onServerClose mock is checked after both destroyed clients report 'close', so an early server 'close' can now fail the test.
• All five tests assert one exact toEqual object per state and release every duplex/TLS socket and the server in finally; mock, connectOverHttp1 and readHttp1Response are already defined in node-http2.test.js.

Extended reasoning...

The diff is test-only (303 insertions) in test/js/node/http/node-http-server-close-drain.test.ts and test/js/node/http2/node-http2.test.js, adding four http.Server closeIdleConnections()/closeAllConnections() tests driven through emit('connection') with duplexPair() and one Http2SecureServer#close() allowHTTP1 test; no runtime source is touched and no security-sensitive surface is involved. The latest commit resolves every point raised in the prior review (hang-on-regression paths now reject, the 'close' ordering is asserted), neither file has a CODEOWNERS entry, no other reviewer has requested changes, and the bug hunt exited on dry_streak, which decided approve.