Skip to content

node:http2: submit a queued client request in the async context of its request() caller - #46036

Open
robobun wants to merge 6 commits into
mainfrom
robobun/7753a71a/http2-queued-request-async-context
Open

robobun wants to merge 6 commits into
mainfrom
robobun/7753a71a/http2-queued-request-async-context

Conversation

@robobun

@robobun robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • The flush submits each request in the frame its stream captured in request() (node: reqAsync.bind(requestOnConnect)). close() while connecting rejects queued requests that way too.
  • The session arms the RST_STREAM of a parked close() and the write completion of a JS transport itself, so Bun.ModuleGraph.dispose() cannot drop them.
  • Verified: test/js/node/http2/node-http2-pending-request-async-context.test.ts (main fails 15 of 23, node passes its 11). Self-reviewed: 31 concerns raised, 24 addressed, 7 answered in Notes.

Background

  • ClientHttp2Session queues a request while the socket connects or the peer's SETTINGS_MAX_CONCURRENT_STREAMS leaves no slot. Its writes wait for 'ready'.
  • A frame is what AsyncLocalStorage reads. A tick queued under a frame runs under it. Bun.ModuleGraph.dispose() drops a graph's immediates.
  • Considered framing only the 'ready' tick: the start channel, header coercion and failure events stay wrong.

Downsides

  • A write() whose DATA frame queues in the session still runs its callback with no store.
  • A write() from another store during a slot wait completes in the request() caller's store (node: the write() caller's).
  • A waited request pays one more runInFrame call, a write over a JS transport two. request() is byte-identical. The bundled http2.js grows 604 bytes.
Notes

Found by a comparison with Node v26.3.0. No issue reports it. It completes #31825 (a stream's events run in its request() context) for the queue that #31584 and #32488 added.

Repro. One session, three callers, the peer allows one concurrent stream:

const http2 = require("node:http2");
const { AsyncLocalStorage } = require("node:async_hooks");
const als = new AsyncLocalStorage();
const server = http2.createServer({ settings: { maxConcurrentStreams: 1 } });
server.on("stream", stream => {
  stream.resume();
  stream.on("end", () => {
    stream.respond({ ":status": 200 });
    stream.end("ok");
  });
});
server.listen(0, () => {
  const client = http2.connect(`http://127.0.0.1:${server.address().port}`);
  client.on("remoteSettings", () => {
    let open = 0;
    for (const user of ["alice", "bob", "carol"]) {
      open++;
      als.run(user, () => {
        const req = client.request({ ":method": "POST", ":path": "/" });
        req.write("chunk", () => console.log(`${user}: upload callback ran as ${als.getStore()}`));
        req.end();
        req.resume();
        req.on("close", () => --open === 0 && (client.close(), server.close()));
      });
    }
  });
});
1.3.13 1.4.2, main node v26.3.0, this PR
alice ran as alice alice ran as alice alice ran as alice
bob ran as bob bob ran as alice bob ran as bob
carol ran as carol carol ran as bob carol ran as carol

1.3.13 had no queue: it sent past the peer's limit. git tag --contains puts #31584 in 1.4.0. Before the connect, the write() and end() callbacks and 'finish' were wrong on 1.3.13 too (the store of the connect() caller).

Who is affected. Code that reads the store in a write() or end() callback, in 'ready' or 'finish', or in a subscriber of http2.client.stream.start, bodyChunkSent or bodySent, on a request that waited. 'response', 'data', 'end' and 'close' were right already, and so is a request that did not wait. AsyncLocalStorage.bind() on the callback is a workaround.

What ran in the wrong store on main, for a request that waited:

  • The write() callback, the end() callback, 'ready', 'finish' and the three channels above.
  • The 'error' and 'close' of a request that fails at submission (ERR_HTTP2_OUT_OF_STREAMS, a header value that the encoder rejects).
  • The toString() of a header value that is not a string. Bun converts a queued request's header values at submission. The peer received x-user: alice on bob's request.

The store was that of the request whose stream closed and freed the slot, or the store of the connect() caller for a request made before the connect and after a SETTINGS frame that raised the limit.

The three flush callers are the 'close' listener of each submitted request, #onConnect, and the remoteSettings handler. They are not changed. They all reach the one call that the PR frames.

Node. request() makes new AsyncResource('PendingRequest') for each request (core.js#L1885) and submits through reqAsync.bind(requestOnConnect.bind(...)), deferred to 'connect' or at once (#L1912-L1926). requestOnConnect submits the request, or destroys it with ERR_HTTP2_GOAWAY_SESSION when the session was closed in the meantime (#L780-L786). At the stream limit node has no JS queue: it submits inside request() and nghttp2 holds the HEADERS frame. Bun queues in JS there, so the frame is needed for that wait too. The stream keeps the frame already, so the PR adds no AsyncResource, no field and no second holder of the store. The close() hunk is new parity, not part of the 1.4.0 regression.

Tests. The file uses node:test and node:assert. Its first 11 tests run unchanged under node, and a last test runs the file under node when the machine has node 26. 11 tests are for Bun.ModuleGraph and run under Bun only.

main this PR node v26.3.0
release build (1.4.3-canary 367d939) 15 fail, 8 pass
debug ASAN build with main's http2.js 15 fail, 8 pass
bun bd test (debug, ASAN), 7 runs 23 pass
node --test 11 pass

The 8 that pass on main pin what must not get worse: destroy() before the connect cancels a queued request in the store of the destroy() caller (as node does), the callbacks of a write over a Duplex transport run in the writer's store, 5 Bun.ModuleGraph cells that complete on main, and the node run. In the test for a header value that cannot be sent, the error code is not compared: node reports ERR_HTTP2_STREAM_ERROR from nghttp2, Bun reports ERR_HTTP2_INVALID_HEADER_VALUE. The stores are the same.

Each clause has a test that fails without it (checked on the bundled module): without the session-armed RST_STREAM 4 tests fail, without the session-armed write completion 3, without the writer's frame for that completion 1, with the frame left installed after a submission 6, without the frame on the close() rejection 1.

Bun.ModuleGraph. On main a waited request runs as the graph that drained the queue. Its callbacks see that graph as Bun.ModuleGraph.current, and its unhandled failure goes to that graph's onError. With the PR it runs as the graph that made it, also after dispose() stopped that graph. A request that did not wait does the same, and so does a queued node:http request on an Agent. Three tests pin that.

The swap alone made dispose() worse than main, because the session armed two immediates in the frame of the request, and dispose() drops the immediates of a graph:

  • sendRstOnReady, the RST_STREAM of a close() on a request that has no id yet. Without it the stream keeps its slot when the peer never finishes it.
  • The write completion of a JS transport (kDeferWriteCallback). Without it the next write of the upload never starts. A native socket completes a write on a tick, which dispose() does not drop.

The session arms both now. The write callback still runs in the frame of the writer.

A host session, the peer allows one stream, tenant B's request waits, B calls close() on it, the peer never answers it. Does a later request of the host get the slot?

disposed main swap only this PR
slot wait: the graph that holds the slot stalls completes completes
slot wait: B, while it waits completes stalls completes
slot wait: both stalls stalls completes
slot wait: B, right after the flush completes stalls completes
connect wait: B completes stalls completes
B uploads with write(chunk, () => end(chunk)) over a Duplex transport main swap only this PR
B disposed while its request waits completes stalls completes
B disposed in the middle of the upload, request waited completes stalls completes
B disposed in the middle of the upload, request did not wait stalls stalls completes

sql.begin() keeps main's behaviour for every caller inside a graph (inCallerContext, src/js/bun/sql.ts). That early-out was measured here: it leaves "bob ran as alice" and the other graph's onError for every caller inside a live graph. The first pushed shape kept a disposed graph's request in the drainer's frame. That guard is removed: with both immediates armed by the session no cell needs it, and the leftovers of a disposed graph no longer run as the host.

Measurements (debug ASAN build of this PR, and the same binary with main's http2.js):

  • request(), the path of a request that does not wait: bytecode identical, 945 instructions (JSC bytecode dump, cmp). _write is identical too, 151 instructions.
  • #flushPendingRequests: 177 -> 102 bytecode instructions, plus 86 in the new #submitPendingRequest.
  • AsyncContextFrame.run() calls per request over a native socket: 4.00 -> 4.00 for a request that does not wait, 4.00 -> 5.00 for one that waited. Frame swaps with no AsyncLocalStorage in use: 0.00.
  • Over a JS Duplex transport a completed write pays 2 run() calls, one to arm the immediate and one to run the callback. Measured with 200 requests that waited: 3.00 -> 6.00 calls per request with a body, 4.00 -> 6.00 without.
  • close() before the connect: +50 run() calls for 50 rejected requests.
  • Heap cells retained per queued request: 28.03 -> 28.03 (heapStats().objectCount, 10,000 requests).
  • Release-mode bundle of http2.js: 168,928 -> 169,532 bytes (+604). The binary embeds that text. The first pushed shape was measured with size on LTO release builds: bundle +619 bytes, text +619 bytes, data +0.
  • A GOAWAY (code 0 and code 2) with requests in the queue: output identical to main.

Not changed here.

  • Still open since 1.4.0: a write() from another store during a slot wait completes in the request() caller's store (node: the write() caller's, main: the drainer's). request() on a connected session leaves the stream corked for one tick (followed up separately). Header values are converted when the request leaves the queue, node converts them inside request() (node:http2: coerce outbound header values before taking a stream id #37568 is open for that).
  • Never right: a DATA frame that queues in the session runs its write() callback with no store, for a request that waited or not, on the client and on the server. It queues when the connection send window (65,535 bytes for all streams until the peer's WINDOW_UPDATE) or the stream's window is used up, or under backpressure. The callback is stored in the native session and called from the drain path. That is followed up with node:http2: fail a dropped write with ECANCELED, keep no write callback in native #44881, which rewrites that site. For a waited request, _write runs in the frame that this PR installs, so a fix that keeps the frame of _write needs this PR first.
  • Never right: session.settings(settings, callback) runs the callback in the session's store (followed up separately).
  • A GOAWAY rejects queued requests in the session's store, as on main. On node v26.3.0 a GOAWAY with an error code fails one waiting request in the session's store and another in its own (measured), so one frame for all of them in streamRejectedByGoawaySession would not match node either. That path needs its own change.
  • destroy() cancels queued requests in the store of its caller, as node does.
  • The session writes to a JS Duplex transport in the frame that is current. For a waited request that was the drainer's store on main and is the request's own store now. Node writes as the session.
  • As on main: close() on a stream that has an id arms its RST_STREAM immediate in the frame of the caller. A graph that closes such a stream and is disposed in the same turn keeps the slot (followed up separately).

Related open PRs on the same method. None of them frames the submission. After a merge, the lines they add to the per-request block go into #submitPendingRequest:

Self-review. 31 concerns before the first push. 24 are addressed in the diff, the tests or these notes: runInFrame in place of a hand-written frame swap, one-line comments, tests for the restore of the drainer's frame, the peer reset, the Bun.ModuleGraph rows, the node run, and the statements above. 7 are answered in another way:

  • "Copy the sql.begin() early-out" and "add no guard to the flush" contradict each other. The measured rows above decide it: no guard, and the two immediates that free a slot are armed by the session.
  • "Move every RST_STREAM immediate to a session owner frame here": only the one that a queued request uses is moved. The others stall on main for a request that did not wait, and they are lines that node:http2: fail a dropped write with ECANCELED, keep no write callback in native #44881 rewrites. Followed up separately.
  • "Frame the transport write at its own seam": not here. It is on the path of every write, and on main the write of a waited request already runs in the drainer's store.
  • "Pin the GOAWAY rejection with a test": not added. It pins code that the PR does not touch, and it costs 2 to 6 s on the debug ASAN lane.
  • "Add a Connect-ES test and todo rows for what stays wrong": not added. The rows are listed above.

Review after the first push: a graph disposed in the middle of a waited upload over a JS transport kept its stream slot, which main did not do. The session arms the write completion now. The guard for a disposed graph is removed, because it ran the leftovers of that graph as the host.

Suites. Debug ASAN build of this PR: the new file (7 runs), the 277 vendored test-http2-*, test-diagnostics-channel-http2-* and test-async-local-storage-http* files, and the 12 files in test/js/node/http2/. On this machine's debug build h2-conformance.test.ts (stream-reset floods, one GC straggler count) and node-http2.test.js (the 9 subprocess tests of "DATA payload survives") have tests that exceed the 5 s default or the straggler bound. They do so at the same rate with main's http2.js (6 and 2 interleaved runs), and the 9 pass on both with a 60 s timeout. Release build (LTO) of the first pushed shape: the 12 files in test/js/node/http2/ (node-http2.test.js 405 pass), the 277 vendored files, AsyncLocalStorage.test.ts 63, AsyncLocalStorage-tracking.test.ts 75, serve-http2.test.ts 93, fetch-http2-client.test.ts 83, module-graph.test.ts 292, module-graph-io.test.ts 28, module-graph-matrix.test.ts 893, grpc-js (27 of 29 files pass, test-resolver and test-tonic fail the same way on main: public DNS names and the tonic server).


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-pending-request-async-context.test.ts

…s request() caller

A request made before the session connects, or while the peer's
SETTINGS_MAX_CONCURRENT_STREAMS leaves no stream slot, waits in the
session's pending queue. The flush submitted it in the async context of
whatever drained the queue: the connect, a SETTINGS frame, or the 'close'
of another request. Its write and end callbacks, 'ready', 'finish', the
start and body diagnostics channels, its header value coercion and its
failure events ran in that context.

The flush now submits each request in the frame that the stream captured
in request(), as node binds requestOnConnect to the request's async
resource. The close()-while-connecting rejection does the same.

A request of a disposed Bun.ModuleGraph stays in the drainer's context,
and the RST_STREAM of a close() on a pending request is armed as the
session: dispose() drops what was armed in the graph, and the stream
would keep its slot.
@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

Reproduced with the script in the PR description on main 5900354 (release build and debug ASAN build) and on Bun 1.4.2: bob: upload callback ran as alice, carol: upload callback ran as bob. Node v26.3.0 and this branch print each caller's own store. Bun 1.3.13 also prints each caller's own store.

test/js/node/http2/node-http2-pending-request-async-context.test.ts fails 15 of 23 tests on main and passes on this branch. Its first 11 tests pass unchanged under node --test.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 402a758 and d76b053.

📒 Files selected for processing: 1
  • src/js/node/http2.ts

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

Walkthrough

HTTP/2 callback scheduling, stream resets, request rejections, and queued submissions now use captured async-context frames. The change adds a queued-submission helper and tests for context preservation, submission failures, cancellation, uploads, and module-graph disposal.

Changes

HTTP/2 async-context handling

Layer / File(s) Summary
Callback and reset context
src/js/node/http2.ts, test/js/node/http2/node-http2-pending-request-async-context.test.ts
Write callbacks, pending resets, and queued-request rejections use session or request frames. Tests check context across callbacks, events, stream-slot changes, header conversion, and uploads.
Queued request submission and failures
src/js/node/http2.ts, test/js/node/http2/node-http2-pending-request-async-context.test.ts
Queued submissions use a dedicated helper. Native submission failures release connection accounting, avoid resets for unannounced streams, destroy the request, and allow queue processing to continue. Tests cover submission errors, cancellation, and Node.js compatibility.
Module-graph disposal and queued uploads
test/js/node/http2/node-http2-pending-request-async-context.test.ts
Tests cover queued requests and uploads when waiting or holding module graphs are disposed. They also check error routing and async-local stores for callers sharing a graph.

Suggested reviewers: jarred-sumner, dylan-conway

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to d76b0

No actionable issue remains in the supplied review. The change is mergeable after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: submitting queued node:http2 client requests in the async context of request().
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.
Description check Passed The description provides a detailed explanation of the problem, implementation, scope, limitations, and verification results. It does not use the template headings exactly, but it includes the require…
Full details: Description check

Explanation

The description provides a detailed explanation of the problem, implementation, scope, limitations, and verification results. It does not use the template headings exactly, but it includes the required information, including how the code was verified.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:59 AM PT - Oct 22nd, 2026

@robobun, your commit d725951 is building: #130892

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 src/js/node/http2.ts — Hosts sharing one http2 session over a JS Duplex transport can lose a stream slot for good when a Bun.ModuleGraph is disposed mid-upload; on main that waited upload still finished. After this change 'ready' is queued in the request's own frame (http2.ts:6414), so the write completion that session[kDeferWriteCallback] arms with setImmediate at http2.ts:2957 belongs to the request's graph and dispose() drops it. END_STREAM is never sent, 'close' never fires and #activeRequestCount stays raised, so other tenants' requests queue forever once the peer's limit is hit. Fix: work that frees the shared session's slot must survive the request graph's dispose, as sendRstOnReady does at http2.ts:2257. [also at: src/js/node/http2.ts:6373 - A host sharing an http2 session with a Bun.ModuleGraph over a JS Duplex transport can lose a stream slot for good when the graph is disposed mid-upload.]

    Why this was flagged

    A host creates one ClientHttp2Session with createConnection returning a JS Duplex (so deferWriteCallbackForSocket at http2.ts:2083 returns setImmediate). A tenant Bun.ModuleGraph calls client.request() while the peer's maxConcurrentStreams leaves no slot, writes a body chunk and end()s; the host disposes the graph after the slot frees. On main the queued request was submitted in the drainer's frame, so the write immediate survived. Now process.nextTick(emitEventNT, req, "ready") at :6414 captures the tenant's frame; _write at :2934 continues on 'ready' in that frame and sessionkDeferWriteCallback at :2957 arms a setImmediate whose AsyncContextFrame is the disposed graph's; bindings.cpp:3305 drops it. The Writable callback never runs, so _final is never reached, no END_STREAM goes out and the once('close') from #trackActiveRequest at :6337 never decrements #activeRequestCount. The slot is taken until the session dies; every later request of the host and other tenants waits in #pendingRequests forever. The stopped-graph guard at :6368 only covers a graph disposed before submission, not one disposed during the upload.

    Verification: Line 6414 process.nextTick(emitEventNT, req, "ready") captures the request's frame. At http2.ts:2957 session[kDeferWriteCallback](callback) stays the class default setImmediate. jsc_hooks.rs:1652-1673: disposing the graph cancels every tracked timer, so the write callback is never invoked. On the base branch the immediate belonged to the host's context and survived the tenant's dispose().

  • 🟣 src/js/node/http2.ts — pre-existing: a Bun.ModuleGraph that calls close() on an already-submitted stream and is then disposed in the same tick never sends its RST_STREAM, so the stream keeps its slot. src/js/node/http2.ts:2601 arms setImmediate(rstNextTick...) in the close() caller's frame, and dispose() drops a disposed graph's immediates. The PR fixes exactly this for the parked (pending) sibling in sendRstOnReady at src/js/node/http2.ts:2257 but leaves the open-stream branch as the base. Fix: arm every RST_STREAM immediate of close() as the session, e.g. a shared helper used by both src/js/node/http2.ts:2257 and src/js/node/http2.ts:2601, so a stream's slot is freed whichever graph closed it.

    Why this was flagged

    A stream was submitted (has an id) from inside a Bun.ModuleGraph. That graph calls req.close() and then graph.dispose() before the next immediate runs, for example in a teardown routine. close() takes the branch at src/js/node/http2.ts:2600-2601 and calls setImmediate(rstNextTick.bind(session, this.#id, code)) with the graph's frame active. dispose() drops immediates armed as that graph (the reason the diff gives at src/js/node/http2.ts:2256), so rstNextTick at src/js/node/http2.ts:2235 never calls rstStream and the peer never sees the RST_STREAM. The stream stays open at the peer and counts against SETTINGS_MAX_CONCURRENT_STREAMS, so a later host request waits for a slot the peer never frees. The base branch behaves the same on this line; the diff fixes only the pending-stream sibling at src/js/node/http2.ts:2257 by arming via runInFrame(session[bunHTTP2AsyncContextFrame], setImmediate, ...), and the open-stream branch two lines below sendRstOnReady's caller is not given the same treatment.

    Verification: pre-existing — the base already fails by the same route; the diff fixes only the pending-stream sibling. Trigger: a Bun.ModuleGraph calls close() on a stream that already has an id, and the graph is disposed before the next immediate runs. src/js/node/http2.ts:2601 arms the RST_STREAM immediate in the close() caller's frame, so rstNextTick never calls rstStream and the slot is held.

  • 🟣 src/js/node/http2.ts — pre-existing: a caller whose queued request is refused by a peer GOAWAY still gets its 'error' and 'close' in the GOAWAY dispatch's store, not its request() store. The PR frames the pending-request rejection only for close()-while-connecting (http2.ts:5446); the peer-GOAWAY loop at http2.ts:5340 calls streamRejectedByGoawaySession bare, as do the forEachStream rejections at http2.ts:5333 and http2.ts:5449. Fix: run the destroy inside stream[bunHTTP2AsyncContextFrame] in streamRejectedByGoawaySession itself, which covers the 4 sites and lets the wrapper at 5446 go. Same pattern at 4 sites (http2.ts:5333, http2.ts:5340, http2.ts:5446, http2.ts:5449). [also at: src/js/node/http2.ts:5446 - pre-existing: a queued request refused by a GOAWAY the peer sends still gets its 'error' and 'close' in the wrong AsyncLocalStorage store, unlike Node.]

    Why this was flagged

    A client on a shared session has requests queued behind the peer's SETTINGS_MAX_CONCURRENT_STREAMS, each made inside als.run(user, ...). The peer sends GOAWAY. The native goaway handler at src/js/node/http2.ts:5320 runs in the parser dispatch context, and at http2.ts:5338-5342 it calls streamRejectedByGoawaySession(pendingRequests[i].req) with no runInFrame, so stream.destroy(err) at http2.ts:4975 and the nextTick'd 'error'/'close' run in that dispatch context, not the request() caller's frame. Node runs the equivalent refusal through the stream's AsyncWrap, so 'error'/'close' see the request() caller's store. The PR fixed the identical loop in #onConnect at http2.ts:5446 with runInFrame(req[bunHTTP2AsyncContextFrame], ...) but left this sibling; the forEachStream rejections at http2.ts:5333 and http2.ts:5449 call the same helper unframed for submitted streams. The base branch behaves the same way at these three sites, so this is pre-existing.

    Verification: pre-existing. The PR frames the close()-while-connecting rejection (line 5446), but the byte-identical loop in the native goaway handler is untouched: src/js/node/http2.ts:5338-5342 calls streamRejectedByGoawaySession bare. No test exercises a peer GOAWAY against queued requests. The base branch already fails this way by the same route (the 5340 loop is unchanged).

  • 🟣 src/js/node/http2.ts — A host that close()s an http2 session shared with a Bun.ModuleGraph can never see the session destroyed or 'close' when the last stream was a tenant's and that tenant is disposed first. The destroy immediate at src/js/node/http2.ts:5150 is armed inside streamEnd, which withStreamFrame runs under the finishing stream's frame; dispose() drops immediates of a stopped graph. This is the same class the PR fixes at src/js/node/http2.ts:2257 by arming the RST as the session, but the sibling site is left as is. Fix: arm every session-lifetime immediate (5150 and scheduleSettingsAckGraceNT at 5147) under session[bunHTTP2AsyncContextFrame], as sendRstOnReady now does.

    Why this was flagged

    Trigger: a host session, tenant graph B's stream is the last active one, the host calls session.close(), B's stream finishes (native streamEnd state 7), then B is disposed before the immediate phase. streamEnd is wrapped by withStreamFrame (src/js/node/http2.ts:3001-3002) so it runs under B's frame; setImmediate(destroyIfNotDestroyedNT, self) at src/js/node/http2.ts:5150 is therefore an AsyncContextFrame of B, and AsyncContextFrame::call drops it once B's graph is stopped (src/jsc/bindings/AsyncContextFrame.cpp:159). The host's close() never completes: no session 'close', the socket stays open, the process keeps running. The base branch behaves the same, but the PR explicitly fixes this exact mechanism for the parked RST_STREAM at src/js/node/http2.ts:2257 with the comment that dispose() drops an immediate armed as the request's graph, and leaves this sibling in the same function family untouched; REVIEW.md asks for the whole class in one PR. Remedy: run the setImmediate at 5150 (and 5147) via runInFrame(self[bunHTTP2AsyncContextFrame], ...).

    Verification: pre-existing. Trigger: a host closes a ClientHttp2Session whose last active stream was made by a Bun.ModuleGraph tenant, that stream reaches native state 7, and the tenant graph is disposed before the immediate phase. The session's only teardown is setImmediate(destroyIfNotDestroyedNT, self) at src/js/node/http2.ts:5150, inside streamEnd under stream[bunHTTP2AsyncContextFrame]; it never emits 'close'.

Comment thread src/js/node/http2.ts Outdated
dispose() of a Bun.ModuleGraph drops the immediates armed in it. A write
over a JS transport completes on an immediate, so a graph that was
disposed in the middle of an upload left its stream open, and the stream
kept a stream slot of the peer. The session arms that immediate now and
runs the callback in the async context of the writer.

The stopped-graph guard in the flush is removed. A request of a disposed
graph is submitted in its own context, and its failure goes to the
onError of its own graph.
Comment thread src/js/node/http2.ts
@robobun

robobun commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

Answers to the additional review findings, after 0d6ec09.

A graph disposed in the middle of an upload over a Duplex transport. Fixed. The session arms the write completion of a JS transport now, and the callback still runs in the frame of the writer. Three tests cover it ("over a Duplex transport"): the graph is disposed while the request waits, in the middle of the upload, and in the middle of an upload that did not wait. The last one stalls on main.

The guard for a disposed graph. Removed, see the review thread.

close() on a stream that has an id, then dispose() in the same turn. Confirmed, and main does the same. A host session with a stream limit of 1, the tenant's request gets the slot at once, close(NGHTTP2_CANCEL), dispose(): the host's next request never starts, on main and on this branch. It is not on the path of a queued request, and #44881 rewrites those lines, so it is not in this PR. It is followed up separately.

A GOAWAY that rejects queued requests. Not changed. I measured node v26.3.0 with three requests at a stream limit of 1 and a GOAWAY with code 2 and last stream id 1. The second request gets ERR_HTTP2_SESSION_ERROR and 'close' in the session's store. The third gets ERR_HTTP2_STREAM_ERROR in its own store. main and this branch reject both with ERR_HTTP2_GOAWAY_SESSION in the session's store. A frame inside streamRejectedByGoawaySession would move the second request away from node's store. With code 0, node emits nothing on the waiting requests in that setup. That path differs from node in the events and in the codes, so it needs its own change.

The session destroy immediate. I could not reproduce a hang. With the tenant disposed at its stream's 'end' or 'close' after the host called close(), the session still emits 'close', on main and on this branch. It is not changed here.

…2-queued-request-async-context

# Conflicts:
#	src/js/node/http2.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The second push drops the stopped-graph guard in the flush, so a disposed graph's queued request now submits in its own frame; that resolves what I raised on the first version. Beyond the inline finding, I also checked the new deferWriteCallbackAsSession for a wrong this: every call site invokes it as session[kDeferWriteCallback](callback), so the session frame it reads is always the right one, and native sockets still take the process.nextTick branch. isFrameOfStoppedModuleGraph has no remaining caller in http2.ts and its shared.ts export change is net-zero against base.

Extended reasoning...

The change touches src/js/node/http2.ts (pending-request flush, GOAWAY rejection, parked RST_STREAM arming, JS-transport write-callback deferral) plus a new node:test file; no auth, crypto, or input-parsing surface. The inline finding is pre-existing and the diff is otherwise consistent with the described Node behavior, but async-context framing in a compat module warrants a human look.

Comment thread src/js/node/http2.ts
@robobun

robobun commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main (9b4284f) to resolve the conflict. One hunk conflicted, in sendRstOnReady (src/js/node/http2.ts:2254):

The merge keeps both. Verified on the merge with the debug ASAN build:

  • test/js/node/http2/node-http2-pending-request-async-context.test.ts: 23 pass. With main's http2.js, 15 of the 23 fail, as before the merge.
  • test/js/node/http2/node-http2-client-close.test.ts, which has the tests of Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021 for that guard: 73 pass.
  • The 277 vendored test-http2-*, test-diagnostics-channel-http2-* and test-async-local-storage-http* files: all pass.

The review of 0d6ec09 has no blocking finding. Its inline finding is the behaviour on main: the callback of a write() whose DATA frame queues in the session runs with no store. I answered it in the thread.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

…2-queued-request-async-context

# Conflicts:
#	src/js/node/http2.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants