Repository navigation
Conversation
A write() after end() errors the Duplex without destroying it, because autoDestroy is off like in node. An errored Duplex never emits 'finish' or 'end'. The streamEnd handlers deferred destroy() to one of those events, so the stream never emitted 'close'. A client stream also kept its maxConcurrentStreams slot. The streamEnd handlers now destroy an errored stream at once. An events.errorMonitor listener destroys a stream whose native close came before the error.
… in flight `stream.write(chunk); stream.end(); stream.write(late)` never sent END_STREAM, so the peer waited forever. A chunk dispatched before end() goes out without END_STREAM, and the flag then comes from _final. The late write errors the stream without destroying it, and the Writable never calls _final on an errored stream. The completion of such a write now sends the END_STREAM that _final would have sent. It does not send it when trailers are pending or when a reset is pending, so close(code) still reaches the peer as RST_STREAM.
…letion A peer RST_STREAM makes the native side drop the stream on the next read, and JS handles the reset a tick later. When both reads happen in one turn (a TLS socket over a JS Duplex), the completion of the last write runs in between, and writeStream throws "Invalid stream id". _final has the Writable's try/catch for this. The completion path now has its own. Tests: the _writev path, the peer reset, and a check that the write is still blocked in the later-turn test.
|
Updated 8:17 PM PT - Nov 10th, 2026
❌ @robobun, your commit c32f9d8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43424That installs a local version of the PR into your bun-43424 --bun |
|
Status: ready for review. The base is main, and main Reproduction, no END_STREAM ( import http2 from "node:http2";
const srv = http2.createServer();
srv.on("stream", st => { st.on("error", () => {}); st.respond({ ":status": 200 }); st.write("done"); st.end(); st.write("late", () => {}); });
srv.listen(0, "127.0.0.1", () => {
const c = http2.connect("http://127.0.0.1:" + srv.address().port); const r = c.request({ ":path": "/" });
let n = 0, ended = false; r.on("data", d => n += d.length); r.on("end", () => ended = true);
const t = setTimeout(() => { console.log("HANG body", n, "ended", ended); process.exit(1); }, 2000);
r.on("close", () => { clearTimeout(t); console.log("ok body", n, "ended", ended, "rst", r.rstCode); c.close(); srv.close(); }); r.end();
});
Reproduction, no Reproduction, no RST_STREAM after Tests: Not covered, as on main (details in the description): a write callback that throws leaves the stream without END_STREAM, and a reset with a code sends no END_STREAM first (from the late write's own callback, or a CI on |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the _final -> sendEndStream extraction (same writeStream(..., true, callback), flush(), and settled-code-5 bookkeeping as before, so behavior-preserving) and the onStreamWriteDone.bind(this, ...) wrapping, which matches the file's existing .bind(this usage rather than needing a $-intrinsic. The waitForTrailers early return is a deliberate exclusion that matches Node, not a missed sibling site.
Extended reasoning...
Findings were already reported inline, so this body only records what else was examined. The refactor moving the END_STREAM empty-DATA-frame path out of _final into sendEndStream was diffed line by line against the removed block (writeStream with endStream=true, native.flush(), and the settled === 5 handling now in onEndStreamSettled) and is equivalent. The .bind(this, callback) pattern was checked against tamper-resistance conventions: the file already uses .bind(this at 27 sites and there is no $bind intrinsic used anywhere in src/js/, so it follows local convention. The bunHTTP2WaitForTrailers return in the wrapper is documented in the PR as intentional and matches Node, so it is not an unhandled sibling of the fix.
When the request has already ended, the END_STREAM sent at write completion closes the native stream at once, before the 'error' of the late write is emitted. destroyClosedStream destroyed the stream right there, so the late write callback and the 'error' listeners got a destroyed stream with no session. It now leaves that destroy to streamOnErrored, which runs after 'error', as it already does when the close comes before the error.
There was a problem hiding this comment.
I re-reviewed after the latest push and found no new bugs; the follow-up commit resolves the destroyed-stream-under-listener issue I raised earlier, and because this touches Http2Stream teardown ordering for every natively closed errored stream, a human look is still worthwhile.
What was reviewed:
destroyClosedStreamnow gating on_writableState.errorEmitted: traced thatNativeClosedis set before the call on both the server (http2.ts:4105) and client (http2.ts:5106) paths and thatstreamOnErroredis an always-registered errorMonitor listener, so the deferred destroy cannot be skipped once the queued 'error' fires.onStreamWriteDoneguards:finalCalled/length === 0/EndStreamSent|NativeClosed/waitForTrailers/rstCodeeach map to a test in the new describe block; thebindper write matches this file's existingrstNextTick.bind/abortRequestStream.bindpattern.- The bare
catch {}aroundsendEndStream: mirrors the Writable's own try/catch on the_finalpath for the same "Invalid stream id" throw after a peer RST_STREAM, and is exercised by the raw-peer TLS test.
Extended reasoning...
Overview
The diff touches src/js/node/http2.ts (two new helpers sendEndStream and onStreamWriteDone, _final refactored to share sendEndStream, _write/_writev wrapping their completion callback when the chunk carries no END_STREAM, and a narrowed immediate-destroy in destroyClosedStream) plus ~240 lines of new tests in test/js/node/http2/node-http2.test.js. Since my previous review, one commit (525458a) landed that changes destroyClosedStream to skip the synchronous destroy when the 'error' is still queued, and adds the "server stream, request already ended" test asserting the late write callback and 'error' listener see destroyed === false and a defined session.
Security risks
None specific to this change. The new code path sends an empty DATA frame with END_STREAM on the existing native writeStream API; it does not parse peer data or widen any input surface. The raw-peer test builds HTTP/2 frames by hand but only on the test side.
Level of scrutiny
High. This is stream lifecycle code in a Node-compat module where event ordering relative to state mutation is the contract, and the destroyClosedStream change applies to every errored stream that closes natively, not only the new END_STREAM-at-write-completion path. I traced the deferred-destroy chain: NativeClosed is OR'd into the status before destroyClosedStream on both server and client close handlers; errorOrDestroy with autoDestroy off always schedules or runs emitErrorNT, which sets errorEmitted on both halves before emitting; the errorMonitor listener streamOnErrored is attached unconditionally in the constructor and schedules destroyIfNotDestroyedNT. I did not find a path where errored is set, errorEmitted stays false, and no 'error' emission is pending. The _final refactor into sendEndStream preserves the old behavior (flush, status 5 handling, push-stream close marking). I could not run the test suite in this environment (no debug build present), so the verification is by reading only.
Other factors
The PR carries a substantial test matrix (server/client, _write/_writev, flow-control-blocked, same-tick close(code)/close(), resets from the 'error' listener, peer RST that drops the native stream), and the author's description lists the remaining divergences from Node (JS-transport deferred completion ordering, client waitForTrailers now ending via 'wantTrailers'). The two ruled-out candidates this run were both the bare catch {}; that block mirrors the Writable's own handling of the identical throw on the _final path and is covered by a test, so I did not re-raise it. My earlier non-blocking note about a throwing user write callback preventing the END_STREAM from going out remains as-is and is pre-existing behavior on the _final path too.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
The completion path now sends the empty END_STREAM frame inline, so _final and onEndStreamSettled are unchanged again. Each added comment is one line.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline test nit, I also checked the new errorEmitted gate in destroyClosedStream (src/js/node/http2.ts:4917) for a stream that never gets destroyed: emitErrorNT sets kErrorEmitted on both the readable and writable state, and streamOnErrored is attached via errorMonitor in the constructor, so a queued 'error' on a NativeClosed stream (writable- or readable-side errored) still ends in the deferred destroy.
Extended reasoning...
The latest push (1aa955c) changed destroyClosedStream to skip the immediate destroy while 'error' is still queued and rely on streamOnErrored instead. The concern to rule out was a stream where errored is set but errorEmitted never becomes true, leaving nothing to destroy it. Traced: errorOrDestroy with autoDestroy: false always schedules emitErrorNT, which sets kErrorEmitted on both _readableState and _writableState before emitting, and streamOnErrored (registered on EventEmitter.errorMonitor at http2.ts:2337, so it fires even with no user 'error' listener) checks NativeClosed, which both state === 7 handlers set before calling destroyClosedStream. The readable-side-errored case is covered by the same emitErrorNT flag write. The onStreamWriteDone path is unchanged from the prior review apart from the guard set already discussed; the remaining inline finding is a test-only nit, so a human look is still worthwhile given this is HTTP/2 stream lifecycle code.
There was a problem hiding this comment.
I reviewed the latest pushes and found no new bugs; the errorEmitted gate in destroyClosedStream and the reject wiring in the peer-reset test cover the points raised earlier. A human look is still worthwhile: END_STREAM now goes out at write completion where Node decides on the tick after dispatch, and the PR lists behavioral divergences (JS-transport ordering, close(2) without a late write) that a maintainer should weigh.
What was reviewed:
onStreamWriteDoneguards against a second END_STREAM:prefinishsetsfinalCalledsynchronously before_final, and a chunk queued behind the completing one keepsstate.lengthnon-zero whileisFinalWritegives it the flag.destroyClosedStream:errored && !errorEmittedonly arises withemitErrorNTalready queued, andstreamOnErrored(errorMonitor, registered in the constructor) destroys on the next tick sinceNativeClosedis set before the call, so no stream is left undestroyed.- Native
writeStreamwith no callback:call_write_callbackno-ops on a non-callable value. The barecatch {}swallows any throw from that call, not only "Invalid stream id"; with constant arguments that is the only reachable one. - Tests were read, not executed (no debug build in this checkout).
Extended reasoning...
Overview
The diff touches src/js/node/http2.ts (26 lines) and adds ~250 lines of tests to test/js/node/http2/node-http2.test.js. The source change is (1) a new onStreamWriteDone wrapper bound around the Writable callback in _write and _writev for chunks that do not carry END_STREAM, which sends an empty END_STREAM DATA frame when the stream is ending, errored, not destroyed, _final was never called, nothing is queued, no trailers are pending, and no reset code is set; and (2) a change to destroyClosedStream so an errored stream is destroyed immediately only when its 'error' has already been emitted, leaving the queued emission plus streamOnErrored to destroy it afterwards. The four commits since my last review address the two inline findings I posted (destroyed stream observed inside the late write callback / 'error' listener; failure events not wired to reject in the peer-reset test).
Security risks
None identified. The change does not parse peer-supplied data; it only decides when to emit an empty DATA frame with END_STREAM on the local side. The catch {} around native.writeStream swallows a throw that a peer RST_STREAM can trigger (the native stream is dropped before JS observes the reset); the alternative is an uncaught exception reachable by a remote peer, so swallowing here is the safer direction. The arguments to that call are constant, so "Invalid stream id" is the only realistically reachable throw.
Level of scrutiny
Moderate to high. The source change is small but sits in the http2 stream lifecycle state machine, where event ordering (write callback, 'error', 'close'), rstCode, trailers, and native settle states interact. I traced: state.length is decremented synchronously inside onwrite before the wrapper's checks, and clearBuffer dispatches the next queued chunk synchronously so length stays non-zero while a chunk that will carry END_STREAM (via isFinalWrite, since ending is already set) is in flight; prefinish sets kFinalCalled synchronously before invoking _final, so a non-errored stream never reaches the new send; the EndStreamSent | NativeClosed bits and rstCode gate cover the close(code) path; native.writeStream with an undefined callback is safe because call_write_callback returns early on non-callable values. For destroyClosedStream, errorOrDestroy always queues emitErrorNT when it sets errored on a non-autoDestroy stream, and destroy(err) sets destroyed first, so the errored && !errorEmitted window always has a pending emission; streamOnErrored runs on errorMonitor and checks NativeClosed, which is set at the state-7 handler before destroyClosedStream is called. The PR itself documents divergences from Node (decision at write completion versus Node's nextTick-after-dispatch check, JS-transport ordering, close(2) with no late write) that are judgment calls for a maintainer, and the author is a bot account, so a human should confirm those trade-offs.
Other factors
The new tests are added inside the existing write() after end() describe with the shared serve/request/record helpers, wire client errors to reject, and pin the flow-control precondition (blockedLength === big.length) rather than assuming it. The two guard tests are labelled as passing without the fix. The peer-reset test builds raw HTTP/2 frames over a TLS socket through a Duplex proxy and now rejects on error/close from every emitter. I could not execute the tests in this checkout (no debug build present), so the "8 of 11 fail without the fix" claim in the description is unverified here. There are no CODEOWNERS entries covering these paths that I found, and no outstanding third-party objections in the timeline; the github-actions inline threads were resolved by the author after commit 1aa955c, which I read as the response.
|
Changes since the first review:
All review threads are resolved. One item stays open on purpose: a write callback that throws skips the END_STREAM here, as it skips |
|
@robobun When HEADERS(200) and RST_STREAM(INTERNAL_ERROR) arrive in one read through a JS Duplex transport, Node v26.3.0 queues 'response' with |
…tick later When HEADERS and RST_STREAM arrive in one read, node handles the reset synchronously (onStreamClose) and emits 'response' on the next tick, so the 'response' listeners get a stream that is already destroyed. Bun emitted 'response' inside the read and deferred the reset by a tick. - The client emits 'response', 'headers', 'push' and 'trailers' with process.nextTick, scheduled before the inbound half ends, like node. - A peer reset tears the stream down inside the dispatch, on client and server. For an error code the writable side ends first (node's closeStream), so 'finish' comes before 'error' and a later write gets ERR_STREAM_WRITE_AFTER_END. - JS now knows about the reset before the native side can drop the stream, so the completion path needs no try/catch for "Invalid stream id".
|
@cirospaciari Done in 7df43d0. Bun now matches node v26.3.0 for that read, and the test asserts node's sequence. What changed:
Your repro (a raw peer over a JS Duplex writes HEADERS(200) + RST_STREAM(2) in one chunk, and The test is now "peer reset in the same read as the response headers". It uses that transport and expects The reset is the root cause of the race that the old test drove. JS now knows about the reset before the native side can drop the stream, so the One existing expectation changed. The UAF fixture "re-entrant read() from a trailer-value toString" in Suites on the debug build: |
… the held write destroyClosedStream queues the destroy of an errored stream in every case: the 'error' event is then queued, in the middle of its emit, or done, and the tick comes after it. Tests: a held _writev, the mark of a held write that 'ready' clears, and an 'error' that user code emits on a stream that is not errored.
…2-end-stream-after-late-write
|
The base is main now, and this pull carries both halves (#43346 is closed). The description is rewritten for the pull as it stands. The per-write cost is gone. The completion function on each write is removed. The error itself now sends the END_STREAM: the What that needed:
Two cases stay as on main and differ from node:
Both have one cause: node submits END_STREAM one tick after the write, whether or not the stream errors later. An end check one tick after #44881 edits the same lines of |
There was a problem hiding this comment.
I reviewed the redesigned version of this PR (errorMonitor-driven endErroredStream replacing the per-write completion wrapper) and found no bugs; because it changes stream close/END_STREAM ordering across the JS and native layers of node:http2, a human look is still worthwhile.
What was reviewed:
endErroredStreamguards: traced NativeErrored, RequestWaitsForTrailers and WriteHeldUntilReady to every setter (both sessions' streamError, setupRequestEndAndSignal, the _write/_writev 'ready' wrappers) and to this one consumer; the no-callback emptywriteStreamis safe on the Rust side (non-callable callback is a no-op in dispatch, and the new early return keeps the queued frame's callback instead of concluding it before its bytes flush).- Ordering: errorMonitor runs before 'error' listeners, and a synchronous streamEnd(7) from the END_STREAM write lands in
destroyClosedStream's new errored branch, which defers the destroy a tick, so listeners see a live stream; the other three branches are byte-identical to the two blocks they replace. - Tests: every wait rejects on 'error'/'close', cleanup is in finally,
port: 0,Buffer.allocfor the large body, and the waitForTrailers client/server guards and the peer-reset-drops-native-stream case are now covered.
Extended reasoning...
The change touches src/js/node/http2.ts (new StreamState bits, an errorMonitor listener per stream, endErroredStream, the destroyClosedStream extraction shared by ServerHttp2Session and ClientHttp2Session), a four-line early return in Stream::queue_frame in src/runtime/api/bun/h2_frame_parser.rs, and about 640 lines of new tests. It touches no security-sensitive surface (no auth, crypto, or untrusted-input parsing beyond existing frame handling). It is not a simple or mechanical change: it alters when 'close' fires and when END_STREAM goes on the wire for errored streams, deliberately diverges from node in the native-close-first ordering, and the review history shows several rounds of ordering bugs found and fixed, with the earlier threads resolved by the author. The bug hunt ran dry with no findings and my own trace found nothing, which is why this is a defer rather than a finding, but the subtlety of the lifecycle semantics is why it is not an approve.
…behind another stream's queue With frames of another stream queued on the session, send_data queues the empty END_STREAM frame too. The peer in the test sends nothing more, so only the flush in the error path gets the frame out.
…2-end-stream-after-late-write
The Http2Stream constructor read EventEmitter.errorMonitor for each stream. That property of the events export is writable, so code that replaced it turned off the listener for every later stream. The constant is the registered symbol that events.ts uses.
…2-end-stream-after-late-write # Conflicts: # src/js/node/http2.ts
…the second text for --aot endErroredStream() called onEndStreamSettled(), which main no longer has. It now calls markWritableDone(), as _final does for a stream that settles to HALF_CLOSED_LOCAL. src/js/ahead-of-time/node/http2.ts gets the same change as src/js/node/http2.ts, and the hash of the new first text. The callback of writeStream() is optional in $Node.H2FrameParser: the END_STREAM write of an errored stream passes none, so that a queued frame keeps its own.
|
I merged main ( What the merge needed
Verified on a debug build of
Not verified here: macOS, Windows, and the tests under One point for review. On main, |
close() leaves the RST_STREAM of a NO_ERROR close to 'finish'. A write after close() errors the stream, and an errored stream never emits 'finish'. The peer got nothing more and the stream never closed. Node sends the RST_STREAM inside close(). close() now marks that it left the reset to 'finish' (ResetAtFinish). The 'error' listener of the stream sends the reset for a marked stream. It flushes first, so that a chunk that waits behind the frames of another stream goes out ahead of the reset. It sets ResetScheduled, so that _destroy() sends no second RST_STREAM. A request that had no id at close() still sends its reset at 'ready'. A stream that is already errored when close() ends it gets its reset on the next turn, like a close() with an error code. Like _final, the listener sends END_STREAM for a stream that close() ended only ahead of a CANCEL. A reset with another error code still takes the place of END_STREAM. A stream that the peer reset with NO_ERROR stays readable and waits for 'end' before it destroys itself. The listener now destroys that stream too. NativeErrored marks it, and the aborted handlers set the mark as the streamError handlers do. endErroredStream() no longer calls markWritableDone(). Nothing reads that bit on an errored stream.
There was a problem hiding this comment.
I reviewed the current push (through e0d81cf) and found no bugs; given how many interacting lifecycle paths this touches, a maintainer look is still worthwhile.
What was reviewed: the new ResetAtFinish path — close() sets it only on a non-pending stream, streamOnErrored sets ResetScheduled before rstStream so _destroy sends no second RST; endErroredStream's guards against the Writable (end() on an already-errored stream never sets ending, so the !state.ending early return is correct) and against _final's EndedByClose/CANCEL rule; the h2_frame_parser early return, whose only no-callback caller is the new empty END_STREAM write; the ahead-of-time mirror, whose hunks match and whose declared SHA-256 equals the current src/js/node/http2.ts. Candidate ruled out: close() with no code after the error has already been emitted waits on 'finish' — pre-existing on main, and END_STREAM already went out by then. I did not run the test suite.
Extended reasoning...
The change touches node:http2 stream lifecycle in src/js/node/http2.ts and its --aot mirror (four new StreamState bits, an errorMonitor listener that sends END_STREAM or the deferred RST_STREAM for an errored stream, a shared destroyClosedStream), a one-line early return in h2_frame_parser.rs's queue_frame, an optional callback in private.d.ts, and about 1250 lines of tests that wire failures to reject and clean up in finally. It touches no auth, crypto or input-parsing surface. It is not approved because the logic spans many interleaved paths (native close before or after the error, pending streams, close() with and without a code, client and server sessions) across 14 pushes, a maintainer already engaged on the thread, and the tests were not executed in this run.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
|
The point that the merge comment left for review is decided. Why it needed a change. The three
END_STREAM tells the peer that the body is complete, and What changed in
Verified on a debug build of |
…STREAM The error path sent END_STREAM ahead of a CANCEL reset, as _final does. Node does that only if close() runs while the session reads. From a timer, a 'connect' listener or right after request(), node sends the RST_STREAM alone, and so does main after a late write at every call site. END_STREAM also tells the peer that the body is complete, which a cancelled stream does not mean. The error path now sends END_STREAM only when no reset code is set, and a stream that close() ended gets only its RST_STREAM. After a late write, the frames of a stream with a reset code are the same as on main.
|
Correction to my comment above. What was wrong. After a late write, Now. After a late write, a stream with a reset code gets its RST_STREAM and no END_STREAM, as on main. The RST_STREAM(0) for a Verified on a debug build of |
…follows a late write After close() and a late write, the 'error' listener sends the RST_STREAM(NO_ERROR) that close() left to 'finish'. Under socket backpressure the native side still held DATA frames of the stream in its queue, and the reset dropped them. The peer got a short body and a clean close. Node sends its pending data ahead of an RST_STREAM, and the same close() with no late write delivers the data first. rstStream() takes a third argument. With it, the frames that flow control lets through go out ahead of the RST_STREAM, with socket backpressure too. The listener passes it and no longer calls flush().
There was a problem hiding this comment.
I re-reviewed the latest push (e105ce1) and found no new bugs; the native sendQueued path it adds addresses the backpressure truncation raised earlier. Given the native unsafe code and *.d.ts ownership, a human look is still worthwhile.
What was reviewed:
send_queued_framesin h2_frame_parser.rs: the stream is re-looked up after everyflush_queue(its write callback runs JS), and the loop terminates on flow-controlNoActionor when nothing was written and the queue did not shrink; frees stay deferred viapending_engine_stream_closes, so the&mut *streamreborrow matches the existingflush_stream_queuepattern.- The two-argument
rstStreamcallers (rstNextTick,sendRstOnReady) still pass through unchanged (undefinedcoerces to false);.bind.ts,private.d.tsand the AOT second text carry the same change as the main text. - Not run here: the new
close() under socket backpressuretest, theUSE_SYSTEM_BUN=1failure check, andtest/internal/second-texts-of-modules.test.tsfor the updated SHA-256 header.
Extended reasoning...
The latest commit adds a third sendQueued argument to the native rstStream (h2_frame_parser.rs, .bind.ts, private.d.ts) and a send_queued_frames loop that flushes a stream's queued DATA frames before end_stream, used from streamOnErrored in both http2.ts texts, plus a new backpressure test. It touches no auth, crypto or injection surface; the sensitive surface is unsafe Rust reborrowing *mut Stream across JS callbacks, which follows the existing flush-loop pattern and re-looks up the stream after each callback. The change is large (stream lifecycle across JS and native, ~1280 test lines), src/js/private.d.ts falls under the *.d.ts CODEOWNERS entry, and the test and SHA checks were not run, so approve is not warranted; the new commit substantively responds to a prior inline finding, which justifies a short note instead of silence.
…2-end-stream-after-late-write
…t serially Inside the concurrent block each of them shares the event loop with about 50 other tests. On a loaded machine the file response test took more than 5 s. The file is now two windows long (128 KB). The backpressure test writes 1 MB chunks, and its loop has a limit for a socket that never pushes back.
There was a problem hiding this comment.
I reviewed the current head (the November pushes on top of the merge with main) and found no bugs, but a human look is still worthwhile: the change rewires Http2Stream completion for errored streams on both server and client and adds a re-entrant native loop, which is beyond what an automated pass should sign off alone.
What was reviewed:
send_queued_framesinh2_frame_parser.rs: the loop re-looks up the stream after eachflush_queue(write callbacks run JS), breaks on NoAction and on a zero-progress Backpressure return, and a re-entrantrstStreamfrom a callback leaves the stream CLOSED so the outerend_streamreturns early.destroyClosedStreamagainst the two inline blocks it replaced: the readable-drain, 'finish' and plain-destroy branches are unchanged; only the newerroredbranch is added ahead of them.src/js/ahead-of-time/node/http2.ts: its diff matchessrc/js/node/http2.tsapart from type annotations, and its header SHA-256 matches the current first text.- Not run here: the new
describe("write() after end()")cases,test/internal/second-texts-of-modules.test.ts, andbun src/codegen/check-types-of-modules.ts.
Extended reasoning...
The change touches src/js/node/http2.ts and its ahead-of-time mirror (new StreamState bits, an errorMonitor listener per stream, endErroredStream, close()/ResetAtFinish handling, the destroyClosedStream extraction), the native h2_frame_parser.rs (optional callback on an empty END_STREAM write, a sendQueued argument to rstStream with a new send_queued_frames loop), the .bind.ts and private.d.ts declarations, and about 1,300 lines of new tests. It touches no auth, crypto or injection surface; the security-relevant exposure is stream lifecycle and native re-entrancy under peer-controlled timing. Approve was ruled out by size and complexity (stream completion semantics across server and client plus unsafe native pointer re-lookups) and by the long history of findings on this PR, all of which were resolved by the author rather than an independent reviewer; a maintainer comment exists in the thread but no review state. The bug hunt exited on dry_streak with no findings, and my own reading found the native loop termination, the refactor's behavior preservation and the mirror hash all in order.
Problem
write()that followsend()orclose(), anHttp2Streamnever emits'close'. For a server stream this is a regression of 1.4.0 (node:http2: rewritten inbound engine, batched write path, server push, +290 node v26.3.0 tests (79% passing) #31584, 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).end()and no RST_STREAM afterclose(), so the peer waits forever. Node v26.12.1 sends both.'finish'or'end'and never calls_final(needFinish,src/js/internal/streams/writable.ts:831).http2.tsrelied on all three, andautoDestroyis off.Fix
destroyClosedStream()destroys an errored stream when the native side reports the close. Anevents.errorMonitorlistener destroys one whose close came first._finalowed, or the RST_STREAM thatclose()left to'finish'.h2_frame_parser.rs): an empty END_STREAM write with no callback keeps the queued frame's callback.rstStream(id, code, true)sends the queued DATA that flow control allows ahead of the reset.describe("write() after end()"),test/js/node/http2/node-http2.test.js(36 fail on main). Alsotest/js/node/http2/, 283 vendored tests.Background
src/js/ahead-of-time/node/http2.ts, the module's second text forbun build --compile --aot, gets the same change.end(). Both cost a healthy stream.Downsides
Notes
This pull is the union of two changes. #43346 held the
'close'half and is closed. This pull targets main and carries both.Merged with main
98cda0d8b1(November). main changed this code after the October push (#44833, #46270). What the merge needed:enum StreamState. The four bits of main keep 9 to 12.NativeErrored,RequestWaitsForTrailersandWriteHeldUntilReadymove to 13 to 15, and the newResetAtFinishis 16.onEndStreamSettled(), which the error path called. The call is gone: nothing reads the bit it set on an errored stream.src/js/ahead-of-time/node/http2.tsis new on main. It is the second text of the module: the compiler ofbun build --compile --aotreads it in place ofsrc/js/node/http2.ts, andtest/internal/second-texts-of-modules.test.tsrequires the SHA-256 of the first text at its head. It gets every change of this pull. A program compiled with--aotfrom a script that makes a GET request and then callsreq.write()printserror ERR_STREAM_WRITE_AFTER_END, response 200, closewith this pull, andno close in 2 swith the two texts of main.writeStream()is now optional insrc/js/private.d.ts. The second text is checked against that declaration (bun src/codegen/check-types-of-modules.ts), and the END_STREAM of an errored stream has no callback (see "The native change").89452d2ac1is merged in too (1532ec4911). It touches none of the files of this pull.close()and_finalhave new rules on main. They meet the error path in two places: see "close()before the late write" and "NativeErroredandClosedByNative".What is a regression. Measured with the release binaries (the events of the stream, or what a plain client gets):
end("done"); write("late")finish error closeerrorerror closewrite("done"); end(); write("late")write("done"); close(); write("late")write(300KB); end(); write("late")req.end("body"); req.write("late")error, no responseerror responseerror response closeThe first three rows worked before 1.4.0. The last two did not.
Repro, no
'close'(server under test, the client half runs undernode):st.respond(); st.end("done"); st.write("late", cb).Repro, no END_STREAM (
bun file.mjs, compare withnode file.mjs):node:
ok body 4 ended true rst 0. main:HANG body 4 ended false. This PR:ok body 4 ended true rst 0.Stream events, END_STREAM already sent (
end(chunk)carries the flag). Node v26.3.0 gives thethis PRcolumn in every row except the last, where it never emits'close'.end("done"); write("late")in'stream'errorerror close'end'errorerror closereq.end("body"); req.write("late")error responseerror response closerequest()for a GET, thenreq.write()(the request ends itself)error responseerror response closeerror, slot helderror close, slot freedWith
maxConcurrentStreams: 2, two late-write requests make a third request pend forever on main. This PR answers it with 200.Where END_STREAM comes from. The last DATA frame carries END_STREAM when
end()comes before its dispatch. If not, the Writable calls_finalafter the last write completes, and_finalsends an empty DATA frame with the flag.What the client sees, last write in flight at
end()(server under test, plain client).lateiswrite("late").write("done"); end(); latewrite(300KB); end(); latewrite(300KB); end();thenlateon a later turnwrite(300KB), laterend(); latewrite(300KB); write("tail"); end(); late'close'cork(); write; write; uncork(); end(); late(_writev)write(""); end(); latewrite(300KB, () => destroy()); end(); latewrite("done"); end(); late; close(2)write("done"); end(); late; close()close(2)ordestroy(err)from an'error'listenerclose(2)ordestroy(err)from the late write's own callbackrespond({ waitForTrailers: true }), then the first rowrespondWithFile(), late write at'finish'write("a", () => { throw }),end(), with anuncaughtExceptionhandlerA client stream has the same bug and the same fix (
req.write("body"); req.end(); req.write("late")). The server stream events arewrite callback error,'error','close'in every row that ends, as in node.close()before the late write. On main,close()ends a stream thatend()did not end and marks it (EndedByClose)._finalthen sends no END_STREAM for that stream: like node, RST_STREAM ends it, unless the code is CANCEL. The RST_STREAM of aclose()with NO_ERROR waits for'finish'. An errored stream never emits'finish', so on main the peer gets nothing more and the stream never closes. Node submits the RST_STREAM insideclose().close()now marks that it left the reset to'finish'(ResetAtFinish), and the error listener sends the reset for a marked stream. The listener sends no END_STREAM for a stream thatclose()ended or that has a reset code. Frames that the server sends on the stream after HEADERS (raw client, the request has ended, andlateiswrite("late")):write("done"); close(); latewrite(300KB); close(); late, window never reopenedrespond({ waitForTrailers: true }), then the first rowwrite("done"); close(2); latewrite(300KB); close(8); latewrite("done"); close(8); lateclose(NGHTTP2_CANCEL)"write("done"); end(); late; close(), the request is still open'close''close'unshift()after'end'), thenclose()'close'In the rows that end with RST_STREAM(0) the stream emits
'error'and then'close', as in node. The rows with NO_ERROR and with code 2 give the same frames from a timer, and for a client request.close()waits for'finish'too, and the stream closes only when the peer ends its side. This pull does the same.close()then waits for a'finish'that cannot come.close()now treats that stream like a close with an error code and sends the reset on the next turn.'error'is emitted, one tick after the late write. Like node, the queued DATA frames of the stream that flow control lets through go out ahead of it: a chunk that waits for the socket, or behind the frames of another stream (rstStream(id, code, true)). A write that flow control still blocks loses its queued part, as in node, which sends only what fits the window.close()has run. The server writes 4 MB chunks until the native queue holds frames, thenclose(); late.8081725d96delivered 4,194,304 of 8,388,608 bytes and then RST_STREAM(0): the reset dropped every queued frame, and a node client reads that as a clean end.e105ce1287delivers all 8,388,608 bytes and then RST_STREAM(0), as main does for the sameclose()with no late write. Node v26.12.1 delivers its 4,194,304 of 4,194,304. A review comment on8081725d96found this.ResetScheduledwhen it sends the reset. If not, adestroy(err)from an'error'listener sends RST_STREAM(INTERNAL_ERROR) after it, once the native side has forgotten the stream.close()sends its RST_STREAM at'ready', as on main, after the write that it holds. The listener leaves that request alone, and the native close that follows now destroys it.'finish'in the trailers row. Bun does not.close(). There the late write of the first row goes out as a second DATA frame ahead of the RST_STREAM, and the stream emits no'error'.close(NGHTTP2_CANCEL). After a late write, a stream with a reset code gets its RST_STREAM and no END_STREAM, as on main.e0d81cf734sent END_STREAM ahead of a CANCEL, because_finaldoes that on main and node did it in the rows that I had measured. Those rows all ranclose()in a'stream'listener. Node v26.12.1 does it only ifclose()runs while the session reads:Http2Stream::SubmitRstStream(src/node_http2.cc) then puts a CANCEL on a list of pending resets, and END_STREAM goes out first.8081725d96takes it out. Frames after HEADERS forwrite("done"); close(8); late, and forwrite("done"); end(); late; close(8):close(8)runs ine0d81cf734'stream','data','response'or'remoteSettings'listener'connect'listener_final), and node only in the first row. This pull does not change_final.close()whether the session is reading. JS has no such signal today, and_finalwould need the same rule.Why the error sends END_STREAM. Three designs were built and run against the rows of the table of the client's view.
kWriteGeneric,lib/internal/http2/core.js), moved from each write toend(): oneprocess.nextTickfor each stream that callsend()while a write is in flight. In a run on an earlier base it matched node in every row above, including the last one and thelate write's own callbackrow. It costs a tick on a healthy stream, and it changes when END_STREAM goes out for a blocked last write and on a JS transport.errorMonitorlistener runs when'error'is emitted, before the'error'listeners. A stream that never errors does no new work:end()and_finalare the same as on main, and so are_writeand_writevfor a stream that has its id.The two rows that the third design leaves as on main:
'error', so itsclose(code)ordestroy(err)comes before the END_STREAM. Node already submitted END_STREAM one tick after the write.finishMaybe, so_finalnever runs, and no'error'is emitted on the stream. A plainWritablewith an asynchronous_writehas the same hang in node. Node's http2 stream ends only because of its end check. Covering this case needs that check, which is work on each healthy stream.The native change.
send_dataqueues an empty END_STREAM write when the stream still has queued frames (a write that flow control blocks).queue_framethen merges the flag into the last queued frame. Before this pull the merge also replaced the callback of that frame and ran the old callback at once, before its bytes were flushed. With the END_STREAM now sent from the error path, adestroy()in that callback cut the body short. The empty write from the error path has no callback, andqueue_framenow leaves the callback of the queued frame in place for such a write. The test "destroy() in the callback of the blocked write does not truncate the body" covers it. On a build of this head (e0d81cf734) without thequeue_framechange it is the one test of the block that fails (65535 bytes received, 131072 expected). The other 49 pass._finalpasses a callback and runs only after the last write completed, so it never reaches the merge with a frame queued.The native reset.
end_streamdrops the DATA frames that a stream still has in its queue. For the reset of the listener that lost data under socket backpressure.rstStream()has a third parameter now (sendQueued, declared inh2_frame_parser.bind.tsandsrc/js/private.d.ts). With it,send_queued_framesfirst writes the queued frames that the flow control windows allow, with socket backpressure too, and the RST_STREAM follows them. It looks the stream up again for each frame, because a write callback runs JS in between. Only the listener passes the argument. Every other reset is as on main.What the error path leaves alone. It sends END_STREAM only when the Writable would have called
_final: the stream is ending,_finaldid not run, and nothing is buffered behind the write in flight. Each exception has a test._finalalready ran, or a file response took its place (respondWithFileclears_finaland the file sends END_STREAM when it is done).bufferedRequestCount), not bytes:end("")leaves a chunk of length 0 behind a blocked write. With a byte comparison the error path sent END_STREAM too, the peer got two frames with the flag, and a node peer closed the session with a protocol error. The test reads the frames with a raw client.'ready'. A request gets its id one tick before'ready'dispatches the write it holds (a session over a socket that is already connected, or a request queued behindmaxConcurrentStreams). The'error'of a late write can fall in between. END_STREAM then went out ahead of the body and the native side dropped the body. The held write now sets a status bit (WriteHeldUntilReady), and it carries END_STREAM itself.close()ended the stream (above), or the native side already dropped the stream (below).The END_STREAM is flushed at once. With frames of another stream queued on the session,
send_dataqueues the empty END_STREAM frame too. A peer that sends nothing more then never gets it. So the error path callsnative.flush(), as_finaldoes. The test uses a raw peer that is silent after its second request.Two orderings of error and native close. The native close is dispatched when END_STREAM is written and the peer has already ended its side.
destroyClosedStream()destroys on the next tick.'error'is then still queued ahead of that tick, in the middle of its emit (the END_STREAM from the listener closed the stream), or done. In each case the late write callback and the'error'listeners get a live stream with its session.'finish'(mid-finish writable) or'end'(unread data), and neither fires after the error. TheerrorMonitorlistener destroys.'end'. It is the close-first ordering again:NativeErroredmarks the stream, and the listener destroys it one tick after'error'.Divergence from node v26, on purpose. In the close-first ordering node v26 never destroys the stream, because
onStreamClosealso waits for'end'. In node that ordering needs the late write to come at least one event loop turn after the response. In Bun it also happens in the same tick asend(chunk), and the client slot depends on'close'. So this PR destroys, one tick after'error'.read()still returns the buffered data, also after the destroy. Afor awaitthat starts inside the'error'listener yields the buffer and then throws the stream's error, as in node. One that starts on a later turn throws at once, where node v26 and main yield the buffer first. A'data'listener or apipe()added after the error gets nothing on node, main and this PR.Node main (v27). nodejs/node#63249 changed how a stream closes. Node v27.0.0-nightly20261018 destroys the stream in the close-first ordering too, and its
for awaiton a later turn then behaves as in this PR. Measured rows (v26.3.0 and v26.12.0 agree with each other):'close''close''close'write(300KB), laterend(); latewrite("done"); end(); late; close(2)write("done"); end(); close(2), no late writeNothing new on the wire from the destroy. Both destroy paths run after the native close, so
_destroysends no RST_STREAM. Measured with a TCP proxy that logs every frame: in the four late write flows whereend(chunk)carries END_STREAM (client and server, both orders) each direction carriesHEADERS, DATA END_STREAMon the stream, the same as the flow without the late write, on node v26.3.0, main and this PR.A stream that the native side already dropped. A peer RST_STREAM makes the native side drop the stream on the next read. JS destroys the stream one tick later, or at
'end'after a NO_ERROR reset. If the late write's'error'is emitted in between,writeStreamwould get an id the native side no longer knows (Invalid stream id). So thestreamErrorandabortedhandlers set a status bit in the same dispatch (NativeErrored), and the listener treats that stream like one that is natively closed (NativeClosed, set byonStreamEnd(7)). The tests "peer reset (INTERNAL_ERROR) that drops the native stream before the write completes", its CANCEL twin, and the two server twins drive this. Without the bit their runs reporterror: Invalid stream id.NativeErroredandClosedByNative. main has a bit with a similar name.emitStreamErrorNTsetsClosedByNativeimmediately before its owndestroy(), and_finalreads it to tell that destroy from adestroy()of user code.NativeErroredis set one tick earlier, in the native dispatch, and it stays set whenemitStreamErrorNTdoes not destroy (the NO_ERROR reset). One bit cannot do both jobs. Set in the dispatch,ClosedByNativewould make_finaltake adestroy()that user code calls in that tick for the native one, and it would change what_finaldoes when the NO_ERROR stream is destroyed at'end'.Costs for a stream with no late write.
'ready'dispatches the write.this.on(kErrorMonitor, streamOnErrored).kErrorMonitoris a module constant, the registered symbolevents.errorMonitor. Measured on a release build (1.4.3,367d939d9a):new Duplex()plus the twoon()calls the constructor already makes, with and without the thirdon(), 200,000 streams per round, 21 interleaved rounds, 3 runs: +2.8, +4.8 and +6.9 ns per stream (127 to 136 ns without). The spread between rounds of one variant (IQR) is 12 to 33 ns, so the delta is below the noise floor. 100,000 retained streams use 664 bytes per stream in both variants (heapStats().heapSize).options.waitForTrailers. The version with the completion function had the same line and measured no change per request.close(): aclose()with NO_ERROR on a stream that has an id and has not finished sets one status bit (ResetAtFinish). If the stream was not ending, it readserroredfirst.bundle-modules.ts --debug=OFF,InternalModuleRegistryConstants.bin) is 2,787,498 bytes at the merge base (98cda0d8b1) and 2,789,372 bytes with this pull (+1,874). The second text grows by 3,247 bytes of source. The native change is one branch in the merge path for an empty write, and one function that only the reset of the listener calls. I did not build a pair of release binaries to measure it.perfandvalgrindare not installed where this pull was built, so there are no instruction counts from me.Not in this PR: a reset in the same read as the response. A review comment here found that Bun and node differ when HEADERS and RST_STREAM(INTERNAL_ERROR) arrive in one read over a JS Duplex. Node handles the reset inside the read and emits
'response'one tick later:aborted,responseon a destroyed stream,finish,error:ERR_HTTP2_STREAM_ERROR,closeresponseon a live stream,aborted,error:ERR_HTTP2_STREAM_ERROR,closeTwo pushes of this PR tried to match node (7df43d0, 778f206). 9091109 reverts both. Review of them, and of a third version that was never pushed, found that the change reaches much more than this case. A design for it has to meet all of these:
on_stream_resetmay change its timing.streamErrorandabortedserve 12 native dispatch sites (teardown sweeps, local resets, six rejections inside nativerequest()). With all of them handled inside the call,session.destroy(err, NO_ERROR)no longer gave a request in flight its'error',request(headers, { parent: 0 })returned a destroyed stream, and a server stream gotERR_HTTP2_STREAM_ERRORin place ofERR_HTTP2_SESSION_ERRORafter a peer GOAWAY.duplexPair, a peer that answers inside the transport's write) a reset is read insiderequest(),end(),write()orpushStream(). A teardown there destroys the stream before the caller has it: no'error',rstCode0,end(cb)gets an error. Node never reads inside a write on the same session. A listener that'response'attaches must still get the error.maxConcurrentStreamsslot until it is read, no session teardown reaches it,respond()on it writes HEADERS for a reset stream,stateandendAfterHeadersthrowInvalid stream id, and a write that completes after the next read reaches the native side with a dropped id.'response'before'data', and Bun would not.'wantTrailers'. main emits it for NO_ERROR and CANCEL resets. Node never does.The third version, with tests for the first, third and fifth point, is on the branch
robobun/d1b8559a/backup-scoped-reset. Nothing else tracks this work yet.Since then main keeps a stream readable after a NO_ERROR reset (#44833), which is the start of point 3. The other points stand, and this pull does not touch them.
Related: #44881. That pull changes how the native side reports a completed write (no callback is kept in a queued frame). Its
queue_framenever runs a callback early, so thequeue_framechange here is not needed once it lands. The two pulls edit the same lines there.Also not in this PR.
'close'only when the session closes, with or without a late write. node:http2: release server push streams once their response ends #38082 covers that.'finish'was already scheduled emitsfinish error close, before and after this PR. Node emitserror close.write("done"); end(); close(2)with no late write is a clean end on main and with this PR. Node v26 sends RST_STREAM(2), node v27 nightly ends cleanly too. node:http2: send RST_STREAM when a server stream is reset #33380 covers that.close(NGHTTP2_CANCEL)". After a late write Bun never sends it. With no late write Bun always sends it. Node sends it in both cases, and only ifclose()runs while the session reads.close(), behind a write in flight: Bun sends them, and the last one carries END_STREAM, also forclose(2)andclose(8). Node drops them and sends the RST_STREAM after the first chunk. main and this pull send the same frames here, with a late write (measured for codes 0, 2 and 8) and with none (measured forclose()).respond({ waitForTrailers: true }), or a request made withwaitForTrailers) and a late write: the stream never ends, in node and in Bun.write(); end(); latethe stream stays open until the peer ends or resets it, in node v26.12.1 and in Bun. With no late write both close it with RST_STREAM(NO_ERROR) at'finish'.close(code)on a request that has no id yet puts its RST_STREAM on the wire twice, and the write that the request holds carries END_STREAM. This is so on main and with this PR, with or without a late write (measured with a raw server).removeAllListeners()with no event name removes the listener of this pull, as it removes the'pause'and'resume'listeners that main registers. The stream then behaves as on main.duplexPair) the callback of the write in flight can run after'close'. Node has both orders: after'close'on a server stream, before it on a client stream.stream.eventNames()now includes theevents.errorMonitorsymbol, andlistenerCount(events.errorMonitor)is 1.Tests. Removing any one of these makes at least one test of the block fail: the
endingcheck, thefinalCalledcheck, the check for chunks buffered behind the write in flight (also when it compares bytes), the mark for a write held until'ready'(its check, its set in_writeand in_writev, and its clear in both), either trailers clause, the record ofwaitForTrailerson a request, the reset check (also when it lets a CANCEL pass), theEndedByClosebranch,ResetAtFinish(its set inclose()and its check), the RST_STREAM of the listener, thesendQueuedargument of that reset (two tests), its set ofResetScheduled, theerroredclause ofclose()(also without its!endingpart),NativeErroredin the listener, its set in the four handlers, the deferred destroy in the listener and indestroyClosedStream(), theerroredcheck of the listener, the flush of the END_STREAM frame, and thequeue_framechange. On main 36 of the 53 tests fail and 17 pass (guards for a reset with an error code or with CANCEL, the trailers, the file response, the blocked write's callback, an error on a stream that has not ended, an'error'that user code emits, and the dropped native stream).Review of the merge. A review of the merge, and of the first version of the
close()change, raised 26 concerns. 15 led to a change.close(), ahead of the write that the request holds, and then a second time at'ready'. The reset overtook a chunk that waited behind the frames of another stream. Adestroy(err)from an'error'listener added RST_STREAM(INTERNAL_ERROR) after the RST_STREAM(NO_ERROR).close().'error'listener after a NO_ERROR reset, and a second RST_STREAM that the raw helpers could not see.end()on a later tick than the write in flight).removeAllListeners()removes the listener.close()dropped a queued chunk under socket backpressure. I listed that as a downside at first. A review comment on8081725d96rated it as a defect, ande105ce1287fixes it (see "The native reset").e0d81cf734, ranclose(NGHTTP2_CANCEL)at call sites that the first one did not cover. It found the END_STREAM ahead of a CANCEL where node and main send none.8081725d96removes that END_STREAM (see "close(NGHTTP2_CANCEL)"), and six guard tests hold the frames of main for codes 2 and 8.Suites on the debug+ASAN build (
c32f9d8581, linux x64, with main89452d2ac1merged in).test/js/node/http2/, 11 files: 756 pass, 7 skip, 0 fail. The 283 vendored tests pass (test-http2-*, sequential,test-diagnostics-channel-http2-*,test-stream-pipeline-http2,test-worker*-http2-*).serve-http2*,node-http2-ping-flood-staged,fetch-http2-client,fetch-http2-adversarialand six regression files: 473 pass, 0 fail. grpc-js: 323 pass, 5 fail, and the 5 need public DNS.test/internal/second-texts-of-modules.test.ts,test/internal/arms-of-modules-for-compiled-programs.test.ts,bun lintandbun fmt --checkpass.rustfmt --checkandcargo clippy -p bun_runtimepassed one105ce1287, which has the same sources for this pull.bun src/codegen/check-types-of-modules.tsreports nothing for the files of this pull. It reports 3 errors about engine sources that are not on my machine. The http2 directory was run with a longer per-test timeout. With the default 5 s, 22 runs ofnode-http2.test.jsalone (on the last four heads, with little else on the machine) had 0 to 9 failures each. Three runs had none. All are in tests of main: timeouts in tests that spawn a debug build ("http2 DATA payload survives its ArrayBuffer ...", "should receive goaway"), and "a session whose client is gone ends without an error on its streams, without TLS". Run alone, that test fails in half of its runs with main'shttp2.tstoo (15 of 30, and 16 of 30 with this pull, in alternating runs). With other work on the machine, one test of this pull passed 5 s ("late write during a file response"). It and the socket backpressure test now run serially, and the file is 128 KB.Not tested by me. macOS, Windows, and the suites under
test/aot(CI runs them). The second text was run in one program compiled with--aot(see the merge above).no test proof · iteration 6 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2.test.js