Repository navigation
Conversation
… has left Node through v26 closes a server stream with RST_STREAM(NO_ERROR) when its response has finished and nobody tried to read its request (Http2Stream[kMaybeDestroy]). Bun did this only when unread request bytes were buffered. A stream whose client sent HEADERS without END_STREAM and nothing more stayed open: it kept its maxConcurrentStreams slot, and server.close() did not call back. The check now runs where the END_STREAM of the response leaves the native queue (markWritableDone) and at 'finish', whichever comes last, and it tests again one immediate later. This replaces the readableLength gate and the call per inbound DATA frame that #32488 added. A request that has ended is skipped, and a reset can no longer drop a queued END_STREAM. Node 27 drains the unread request instead of the reset (nodejs/node#63249).
|
Updated 4:42 PM PT - Oct 19th, 2026
✅ @robobun, your commit 8b4fc7720c00871a79864d4a07ffb38d56d94ffe passed in 🧪 To try this PR locally: bunx bun-pr 46058That installs a local version of the PR into your bun-46058 --bun |
|
Status: ready for review. One decision is open for the requester: close (node through v26, this PR) or drain (node main, nodejs/node#63249). Merge after #45801, #37985 and #43754. Reproduction: the four repro scripts of the request, unchanged, exit 1, 1, 1, 1 on bun 1.4.3-canary and 0, 0, 0, 0 on a release build of this branch. Node v26.3.0 and v26.12.0 exit 0, 0, 0, 0.
Tests: all 181 CI jobs pass on 8b4fc77, Windows and macOS included. The annotated tests are not http2 tests and passed on retry. On the debug build of my container one existing case of this file fails in each run, with main's file and main's build as well: a reset-flood case goes over the 5 s default, or a stream-release case fails (#42357). PR: #46058 |
…rooted streams The new describe ran ahead of the reset-flood cases and made them slower: 4.9 to 7.3 s against the 5 s default on a debug build, and 2.5 to 3.6 s with the describe at the end of the file. liveCount() takes the change of #42357. It stops when no more than GC_STRAGGLERS streams are left, and no longer when the count holds for 10 passes. Without it the stream-release cases fail in 7 of 8 runs on a debug build, with and without the change of this branch.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe HTTP/2 server now applies updated conditions when closing streams with unread requests. The conformance suite adds coverage for response completion, stream closure, transport variations, flow control, and request-reader timing. ChangesUnread server request streams
Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the two triggers (markWritableDone and the 'finish' hook) can send two RST_STREAM frames for one stream: close() sets StreamState.Closed synchronously via markStreamClosed, so the second queued closeUnreadServerStream returns on stream.closed. I also checked that dropping the streamData trigger does not reopen the original deadlock: END_STREAM-less uploads are now caught by the WritableClosed path in markWritableDone, and a request whose END_STREAM arrives later ends the readable before the immediate runs, so the re-check in closeUnreadServerStream skips it.
Extended reasoning...
The change rewrites the server-side auto-close for answered, unread node:http2 streams in src/js/node/http2.ts (two triggers, removed trailer and readableLength guards, removed streamData trigger) and adds ~700 lines of raw-frame conformance tests. It touches no auth or crypto surface, but it changes wire behavior (RST_STREAM emission) toward proxies and clients and deliberately diverges from node v26 in eight response shapes. The confirmed inline findings on late-DATA/trailer handling, the widened empty-body case for late readers, and the host-dependent heldBack assertion, plus the author's own request for a maintainer review of the removed Windows gate, mean a human look is needed.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| function closeUnreadServerStream(stream: ServerHttp2Stream) { | ||
| if (stream.destroyed || stream.closed) return; | ||
| // Tested again: 'finish' can come before the handler attaches its reader behind an await. | ||
| if (!stream.readableDidRead && stream.readableFlowing === null) stream.close(); |
There was a problem hiding this comment.
🔴 Operators running a Bun http2 server behind a proxy such as nginx grpc_pass can now get requests failed with "upstream rejected request with error 5" when a handler answers before the upload ends. The auto-close at src/js/node/http2.ts:3210 sends RST_STREAM(NO_ERROR) while the client still has DATA frames in flight, and the engine answers each late DATA frame with RST_STREAM(STREAM_CLOSED) instead of ignoring it (RFC 9113 §5.1). Fix: before this close runs routinely, make the engine ignore DATA and HEADERS on a stream it reset itself (keep the closed entry or a recently-reset set) so the only reset the peer sees is the NO_ERROR one, as node/nghttp2 do.
Why this was flagged
A client (grpc-js, nginx grpc_pass, any uploader) sends HEADERS without END_STREAM, the server handler responds and ends without reading the body (e.g. an auth rejection), and the client has more DATA frames in flight. closeUnreadServerStream at src/js/node/http2.ts:3210 calls stream.close(), which sends RST_STREAM(NO_ERROR) via rstNextTick (src/js/node/http2.ts:2598). Every DATA frame that then arrives hits src/runtime/api/bun/h2/connection.rs:1498 or :1504 and gets RST_STREAM(STREAM_CLOSED); the PR's own test comment at test/js/node/http2/h2-conformance.test.ts:2723 states this. On the base branch no reset is sent for a request with no buffered bytes, so the peer sees the whole response and no STREAM_CLOSED. nginx's grpc module treats a later RST_STREAM with a non-zero code as an upstream error (the author reports seeing 'upstream rejected request with error 5'), so the downstream client loses the answer. No safeguard prevents it: the engine only tracks streams in its map and has no record of streams it reset itself.
Verification: src/js/node/http2.ts:2108-2110 (new): markWritableDone now calls maybeCloseUnreadServerStream for any not-yet-Closed ServerHttp2Stream; :3210 closeUnreadServerStream calls stream.close() when nothing has read the request. Every later DATA frame for that id then hits the None arm: connection.rs:1637 → :1654 send_rst_stream for full frames, or :1498 for streamed frames.
There was a problem hiding this comment.
This is correct, and it is the second bullet of Downsides.
I measured it with a raw client: the answer first, then 3 late DATA frames. Node v26.3.0 sends RST_STREAM(0) and nothing more. This branch sends RST_STREAM(0), then RST_STREAM(STREAM_CLOSED) once per late DATA frame. Main gives the same reply on a stream that it reset itself (it resets when unread body bytes are buffered). So the defect is in the engine, and this PR makes it reachable for more requests.
The engine change that you describe is #45801 (late DATA) and #37985 (late HEADERS on a server). Both are open. This PR does not copy them. It has to merge after them, and the description says so.
I did not run nginx against this branch. The log line in the notes comes from a run with the rule emulated on a release binary.
| if (stream[kFileResponseFinal] !== undefined && (status & StreamState.WantTrailer) === 0) return; | ||
| if (!stream.readableDidRead && stream.readableFlowing === null) { | ||
| setImmediate(callStreamClose, stream); | ||
| setImmediate(closeUnreadServerStream, stream); |
There was a problem hiding this comment.
🔴 pre-existing, widened: Bun http2 clients that attach their response reader after an await now get an empty body from a Bun server whenever their request is still open when the answer finishes. The new close at src/js/node/http2.ts:3201 sends RST_STREAM(NO_ERROR) for every such stream, not only when body bytes were buffered. On the client that reset reaches emitStreamErrorNT, whose stream.resume() at src/js/node/http2.ts:3774 dumps the buffered response before any late 'data' listener or for-await exists; node's client keeps it. Fix: on a peer NO_ERROR reset keep the buffered readable and defer destroy to 'end', as the streamEnd(7) branch at src/js/node/http2.ts:5106-5110 already does, so both early and late readers get the body.
Why this was flagged
A Bun http2 client sends POST HEADERS, the body DATA+END_STREAM goes out in a later write. The Bun server handler answers without reading (404, 401, 413). After this change markWritableDone (src/js/node/http2.ts:2112-2114) and serverStreamOnFinish (src/js/node/http2.ts:3205) schedule closeUnreadServerStream, which calls stream.close() at src/js/node/http2.ts:3210; the base only did this when readableLength > 0 at 'finish', so a request with no body byte buffered was never reset. The client engine turns the RST_STREAM(NO_ERROR) into onStreamError(0); the ClientHttp2Session handler at src/js/node/http2.ts:5077-5082 runs emitStreamErrorNT, which at src/js/node/http2.ts:3773-3776 calls stream.resume() and push(null) and then destroy(). resume() flows the buffered response DATA on nextTick with no listener attached, so a handler that does await once(req, 'response'); await something; req.on('data', ...) or starts for-await later gets an empty body. The safeguard closeUnreadServerStream re-checks the server-side reader, not the client-side one, so it does not help.
Verification: src/js/node/http2.ts:3205 no longer gates serverStreamOnFinish on readableLength > 0, so :3210 stream.close() sends RST_STREAM(NO_ERROR) for an open request with no buffered body bytes, which the base never reset. In emitStreamErrorNT, :3773-3776 runs stream.resume() with no 'data' listener attached, so the buffered response bytes are discarded. A reader attached afterward sees an empty body and no error.
There was a problem hiding this comment.
Correct. I measured it with a 100 KiB response to a request that never ends:
- bun's client gets the whole body with a
datalistener attached at once, and withfor awaitstarted right afterresponse. - bun's client gets 0 bytes with a reader attached 50 ms after
response. - Against main all three readers get the body, because main sends no reset for a request without a buffered body byte.
- The client of node v26.3.0 gets the body in all three cases.
The client change that you describe is #43754. It is open and has conflicts. The description now says that a late reader can lose the response until #43754 lands. I keep the client change out of this PR, so that the two do not carry the same code.
The four "socket held the response back" rows required that one 8 MiB response is more than the socket takes. The buffers of the host decide that. The rows now send requests until the session reports a queued response, and they check that stream. The liveCount() comment is shorter and names its source. Correction to the message of the commit before this one: the place of the new describe in the file does not change how long the reset-flood cases take. Interleaved runs on a debug build give 3.4 to 7.5 s for them with either placement.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 3 findings from earlier reviews are still open above.
Still open from earlier reviews (3):
- 🔴
src/js/node/http2.ts:3201—pre-existing, widened: Bun http2 clients that attach their response reader after an await now get an empty body from a… - 🔴
src/js/node/http2.ts:3210—Operators running a Bun http2 server behind a proxy such as nginx grpc_pass can now get requests failed with "upstream… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
The change to liveCount() belongs to #42357, which has the heap snapshot for it. This branch does not touch the stream-release cases.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
src/js/node/http2.ts:3201—pre-existing, widened: Bun http2 clients that attach their response reader after an await now get an empty body from a… - 🔴
src/js/node/http2.ts:3210—Operators running a Bun http2 server behind a proxy such as nginx grpc_pass can now get requests failed with "upstream…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Merge after #45801, #37985 and #43754 (Downsides says why).
Problem
node:http2server stream stays open when the client sends no body byte and no END_STREAM. It keeps itsmaxConcurrentStreamsslot and blocksserver.close().finishonlyif (this.readableLength > 0)(src/js/node/http2.ts, from node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488).Fix
maybeCloseUnreadServerStreamnow runs frommarkWritableDone(the response's END_STREAM left the native queue) and fromfinish. The later call queues the close onesetImmediatelater.StreamState.WritableClosed, so a reset cannot drop a queued END_STREAM.test/js/node/http2/h2-conformance.test.ts(52 new tests, main fails 42). Alsonode-http2.test.js, 279 upstreamtest-http2-*, grpc-js.Background
finishcan fire before the handler resumes behind anawait, so the close tests again.Downsides
setImmediatewhen the answer precedes the request's END_STREAM, +214 bytes.Notes
Origin. No user reported this and there is no issue. A maintainer asked for it in team chat with four repro scripts.
Merge gates.
fetch-http2-leak.test.tsfailed on Windows withHTTP2StreamReseton its large responses, so these lanes are the check for its removal.What replaces the gate of #32488. bb67830 added
readableLength > 0because the END_STREAM of a request without a body could reach JS afterfinish, so nearly every such response got a reset. Now:WritableClosedcovers the response that is still queued. With the gate, main sends RST_STREAM in place of the END_STREAM of an 8 MiB compat response when 1000 unread body bytes are buffered (socket held back, request open or complete). This branch sends the whole response in those cases.Node versions. All of this ran in the build container.
node <script>: v26.3.0 exits 0, 0, 0, 0. v26.12.0 (the latest release) exits 0, 0, 0, 0. v27.0.0-nightly20261018f88a1d25e9 exits 1, 2, 2, 1: no RST_STREAM, noclose, noserver.close()callback, 0 of 4 fresh requests answered, the honest user of the gateway answered 0 of 5 times.autoDrainReadable(merge commit 5f9618049b27 of Fix HTTP/2 RST_STREAM behaviour, add auto-drain, deprecate 'aborted', fix related compat API issues nodejs/node#63249). It is afinishlistener of every server stream and callsresume()on an unread request. The close branch ofkMaybeDestroythen seesreadableFlowing !== null. The tags v26.12.0, v26.3.0 and v24.22.0 do not have the function. bun reportsprocess.versionv26.3.0.datalistener at once and once withfor awaitstarted 300 ms afterresponse. Servers that close (node v26.12.0, this branch) give the same rows: node v26.3.0 and v26.12.0 clients get the body with both readers and the late iterator throwsERR_STREAM_PREMATURE_CLOSE(http2: async iterator rejects complete buffered response after NO_ERROR reset with request side open nodejs/node#66693). The node nightly client and bun 1.4.3-canary get the body with the early reader and an empty body with the late one. Servers that do not close (node nightly, bun main): every client gets the body with both readers, and the stream stays open.respond()+end():finish,end,closewithrstCode0 andabortedfalse, on node v26.3.0 and on this branch.Where bun now differs from node v26 on purpose. For a request that never ends, node v26.3.0 and bun main send no reset in these 8 shapes, and this branch sends RST_STREAM(NO_ERROR): compat
end()without a body, compatstatusCode = 204, compatwriteHead(205), compatwriteHead(304), compatend()for a HEAD request,respond({ endStream: true, waitForTrailers: true }), a HEAD request withrespond()+end(), and aread()that returned null before the answer. Node holds these for trailers that never come. With one unread body byte buffered, main already resets all eight and node none. The table in the test file has node's frames beside bun's for 25 shapes. 17 are equal.What still holds a stream.
respondWithFile,respondWithFD). Node v26 keeps it open too.server.close(), main and this branch do not (one row run). node:http2: ignore a request that opens after the server's GOAWAY #43475 is about it.stream.setTimeoutis the session timer in bun (issue @fastify/http-proxy with HTTP/2 spuriously emits FST_REPLY_FROM_HTTP2_REQUEST_TIMEOUT after idle on Bun #30307, open PR http2: give Http2Stream its own per-stream idle timer #30308).So "
server.close()calls back" holds for answered, unread requests. It does not hold for the cases above.Frames after the reset. Measured with a raw client, answer first, then the rest of the request 3 s later:
grpc_passlogupstream rejected request with error 5for a late body, with the rule emulated on the release binary. I did not run that again.Measured. main is 5900354 (release 1.4.3-canary), PR is this branch. N=200 per class. The counts of the PR are the same on a debug build and on a release build of this branch.
setImmediateper request, in the order GET / POST with a 100-byte body and END_STREAM in the same write, unread / POST read then answered / one body byte, never ended / nothing more, never ended: main 0.00 / 1.00 / 0.00 / 3.00 / 0.00, PR 0.00 / 1.00 / 0.00 / 3.00 / 3.00. POST with an empty DATA+END_STREAM in the same write, answered at once: 0.00 -> 1.00.respond()+end():once('finish')registrations 1 -> 0. Calls of the check 0 -> 1 (it returns after one property read).setImmediate0 -> 0.perf,valgrindandstraceare not installed in the build container.end(chunk)or file, trailers at once or one immediate later, with and without 1000 upload bytes): node v26.3.0, main and this branch deliver the whole response in 8 of 8. The frames of this branch equal node's in 8 of 8. This is the probe that stands in for a test row.node/http2module, release codegen: 175,338 -> 175,552 bytes (+214). No host function and no codegen row is added.http2.ts:.text106,771,236 -> 106,771,464 bytes (+228). The file is 106,944,224 bytes in both builds.closeevents 0 -> 200, live stream objects after GC 200 -> 0.node:http2client against a bun server, a 100 KiB response to a request that never ends, three reader shapes: 3 of 3 get the whole body from main, 2 of 3 from this branch. The reader attached 50 ms afterresponsegets 0 bytes (node:http2: keep the data that arrived before a peer's RST_STREAM(NO_ERROR) #43754). The client of node v26.3.0 gets 3 of 3 from this branch.h2-conformance.test.ts0 fail of 136. Upstreamtest-http2-*: 0 fail of 279.node-http2.test.js: 0 fail, 405 pass, 6 skip (with--timeout 30000, see Tests).Tests.
headersSent,WritableClosed, the file rule, the second test in the immediate, the call inmarkWritableDone, thefinishhook, and the swaps ofsetImmediatefor a microtask ornextTickand ofclose()fordestroy(). They do not catch the removal of four guards that main already has (destroyed || closedtwice,writableFinished, the unread test before the immediate) or of the skip of thefinishhook for an ended request. Those change the count of immediates or registrations only.node-http2.test.jsgives 405 pass, 0 fail, 6 skip with--timeout 30000. With the default 5 s, 6 to 19 of its subprocess tests time out there, and 9 on the unfixed build. The new describe is the last one inh2-conformance.test.ts. The reset-flood cases of that file take 3.4 to 7.5 s on the debug build in the build container, against the 5 s default. Interleaved runs show the same range with the new describe ahead of them or behind them, and main's file with main's build is in that range as well.liveCount()) fail in 7 of 8 runs on a debug build in the build container, with and without this PR (4 to 8 of 16 streams alive on both builds). test: deflake the h2 stream-release cases on debug builds #42357 is about them. This PR does not change them.socket held the response backrows send requests until the session reports a queued response. They do not depend on the buffer sizes of the host.finish" cannot be built in bun: the empty END_STREAM frame leaves at once, andfinishwaits for DATA that flow control holds. Socket backpressure and late trailers reach theWritableClosedtest.Interim workaround on a released bun. It polls, and it cannot stop main's own cut.
Also seen, not changed here.
stream.end()withoutrespond()and without a chunk sends DATA+END_STREAM with no HEADERS frame. Node sends nothing. Main does the same.finishfires before a microtask that the handler queued (deferWriteCallbackForSocketcompletes a flushed write onprocess.nextTick). In node the microtask runs first. Main does the same.