Repository navigation
Conversation
When session.destroy() (or any stream teardown) drops DATA frames still
queued behind flow control, the Writable _write callback was invoked
with no error. That looks like a successful write to the Writable state
machine, so it emits 'drain', and since _write/_writev fell through to
callback() once the native handle was gone, every subsequent write()
also reported success. A producer following the canonical
'if (!write(chunk)) once("drain", more)' idiom is woken by the
teardown and then never sees backpressure again, buffering its entire
source into a dead stream.
clean_queue now settles each dropped frame's callback with
ERR_HTTP2_INVALID_STREAM (node settles them with ECANCELED), and the
_write/_writev fallthrough does the same when the session/native handle
is gone. The Writable error path takes over: no 'drain', the stream is
marked errored so further write() returns false, and the user's write
callback receives the error.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adjusts HTTP/2 stream write completion handling and session destroy teardown in ChangesHTTP/2 Write and Destroy Semantics
Sequence Diagram(s)sequenceDiagram
participant Test
participant ClientSession
participant Stream
participant sessionDestroyStream
participant Callback
Test->>Stream: write large payload (backpressured)
Test->>ClientSession: destroy() on next tick
ClientSession->>ClientSession: compute streamRstCode
ClientSession->>sessionDestroyStream: forEachStream(sessionDestroyStream)
sessionDestroyStream->>Stream: emitStreamErrorNT
Stream->>Callback: writeStream completion via onWriteStreamDone
Callback-->>Test: error code (stream destroyed)
Compact Metadata
Related Issues: Not specified in provided information. Related PRs: Not specified in provided information. Suggested Labels: node:http2, bug, needs-tests Suggested Reviewers: Not specified in provided information. 🐰 A stream once blocked, now destroyed with care, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:14 AM PT - Jul 7th, 2026
❌ @robobun, your commit 5eb3eef has some failures in 🧪 To try this PR locally: bunx bun-pr 33606That installs a local version of the PR into your bun-33606 --bun |
node-http2.test.js has a pre-existing borderline-timeout test (maxSessionMemory, ~100s on debug+ASAN against a 150s limit) that flakes independently of this change. Moving the new test into its own file keeps the fail-before/pass-after proof deterministic.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ed writes The previous approach (native clean_queue passes an error to onwrite) reached errorOrDestroy on a not-yet-destroyed stream, emitting 'error' on streams without a listener (test-http2-cancel-while-client-reading, test-http2-respond-with-file-connection-abort). clean_queue is shared by every teardown path (stream.close(), received RST, socket abort), not only session.destroy(), so scoping it there is wrong. Instead: session.destroy() now runs emitStreamErrorNT synchronously for each still-open stream (via forEachStream) before emitErrorToAllStreams, so the dropped-frame Writable callback sees kDestroyed and afterWrite skips 'drain'. This is the same ordering node uses (destroy the Http2Stream before ClearOutgoing(UV_ECANCELED) settles the write req). onWriteStreamDone wraps the callback so a write that settles after the stream is destroyed is reported as ERR_HTTP2_INVALID_STREAM to the user's write callback (errorOrDestroy is a no-op on a destroyed stream). The _write/_writev fallthrough to callback() when native is gone is kept as an error for the same reason.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 2086-2090: The comment near the Writable onwrite wrapper in
http2.ts is too long for the repository’s 3-line limit. Shorten the explanatory
block around the writeStream/onwrite callback so it keeps only the durable,
non-obvious point about treating dropped writes during stream teardown as an
error and that onwriteError does not emit drain. Preserve the relevant context
in the existing comment, but remove the detailed bug narrative and extra
teardown mechanics.
- Around line 4111-4118: Condense the teardown comment near session.destroy() in
http2.ts to fit the 3-line limit and keep only durable behavior notes. Remove
the PR-history/style explanation and retain just the essential non-obvious
teardown semantics around destroying still-open streams, skipping already-closed
streams, and preserving the same error/rstCode flow used by emitStreamErrorNT.
In `@test/js/node/http2/node-http2-session-destroy-backpressure.test.ts`:
- Around line 4-10: Shorten the explanatory comment in the
node-http2-session-destroy-backpressure test to a brief, durable summary of the
behavior under test. Keep only the non-obvious point that session.destroy() must
fail the held write callback so the ClientHttp2Stream never emits drain and
later write() calls do not succeed; remove the historical regression story while
keeping the comment around the existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4a7ab11-fba7-4d75-94a3-df55a5bdf8b1
📒 Files selected for processing (2)
src/js/node/http2.tstest/js/node/http2/node-http2-session-destroy-backpressure.test.ts
|
CI status: the diff is green. The remaining red on build 69734 is unrelated infra:
No http2 failures anywhere. The previous build (69690) was green on every lane except a napi GC flake on Windows x64-baseline. All 194 expected-passing Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-07, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
A
ClientHttp2Streamwhose upload is blocked on flow control (the peer withholdsWINDOW_UPDATE) has a DATA frame queued in native with its Writable_writecallback held. Whensession.destroy()tears the stream down, that callback was invoked with no error. The Writable state machine treats that as a successful write, so it emits'drain'. And once the session's native handle is gone,_write/_writevfell through tocallback()on every call, sowrite()kept returningtrueforever.A producer following the canonical backpressure idiom:
is woken by the teardown itself and then never sees
write()returnfalseagain, so it drains its entire source into a destroyed stream.Reproduction
Before:
'drain'fires after destroy, 32 subsequentwrite()calls all returntrue:After: no
'drain', no further accepted writes, the write callback receivesERR_HTTP2_INVALID_STREAM:Node.js never emits
'drain'here and settles the pending write callbacks withECANCELED.Cause
session.destroy()calls nativeemitErrorToAllStreams, which drops each stream's queued DATA frames (clean_queue) and dispatchesonStreamError. The JSstreamErrorhandler defers the actual stream destroy viaprocess.nextTick(emitStreamErrorNT, ...), so whenclean_queueinvokes the held_writecallback the stream is still neither ended nor destroyed:afterWriteseeskNeedDrainwith nokEnding/kDestroyedand emits'drain'.Http2Stream._write/_writevalso fell through tocallback()whensession[bunHTTP2Native]isnull, which is the state aftersession.destroy()returns.Fix
session.destroy()(client and server) now runsemitStreamErrorNTsynchronously for each still-open stream (viaparser.forEachStream) beforeemitErrorToAllStreams, so whenclean_queuesettles the held callback the stream is already destroyed andafterWriteskips'drain'viakDestroyed. This matches Node.js, which destroys eachHttp2StreambeforeClearOutgoing(UV_ECANCELED)settles the write reqs. Streams already marked closed (completed normally) are skipped, matching nativeemitErrorToAllStreams._write/_writevwrap the callback handed to nativewriteStreamwithonWriteStreamDone: when the callback fires after the stream is destroyed, it reportsERR_HTTP2_INVALID_STREAMto the user's write callback (Node.js reportsECANCELED).errorOrDestroyis a no-op on a destroyed stream, so no'error'event is emitted and no'drain'._write/_writevfallthrough when the session/native handle is gone now passesERR_HTTP2_INVALID_STREAMinstead of reporting success.The other teardown paths that reach
clean_queue(stream.close(), received RST_STREAM, socket abort) already callend()ordestroy()before the callback fires, so they are unchanged.Verification
test/js/node/http2/node-http2-session-destroy-backpressure.test.ts: a raw TCP peer that withholdsWINDOW_UPDATE, one write larger than the 65535-byte initial window, thensession.destroy(). AssertsdrainsAfterDestroy === 0,writeOkAfterDestroy === 0, and the write callback received an error. Fails onmainwithdrainsAfterDestroy: 1, writeOkAfterDestroy: 32.