Skip to content

node:quic: send headers queued before the stream is bound - #42869

Merged
Jarred-Sumner merged 1 commit into
mainfrom
robobun/78340612/quic-flush-pending-headers
Sep 16, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
robobun/78340612/quic-flush-pending-headers

Conversation

@robobun

@robobun robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • A node:quic client that opens an HTTP/3 stream before the handshake completes and calls sendHeaders(..) without a terminal flag does not send the request. The HEADERS frame stays in the lsquic stream buffer until the client writes to stream.writer or ends it. A client that waits for the server (for example for 100 Continue) waits until the idle timeout.
  • The cause is bind_raw in src/runtime/node/quic/stream.rs:235. It hands the queued pending_headers to lsquic_stream_send_headers, which only buffers the block. The flush runs in drain_outbound, which runs from the on_write callback. bind_raw asked for a write event only when outbound.started was already true, so no on_write ran.

Fix

  • bind_raw now asks for a write event (s.want_write(true)) when it queued at least one non-terminal header block, whatever the state of the writer.
  • This matches the path for a stream that already exists: send_headers calls kick_write() after s.send_headers(..). A terminal block does not need it, because s.shutdown(1) makes lsquic flush on its own.
  • The resulting on_write runs drain_outbound with an empty outbound queue. It clears the write interest and calls s.flush(), so the headers leave. Latency after sendHeaders is the same as for a stream created after client.opened (about 30 ms on a debug build).
  • Verified: test/js/node/quic/quic-stream.test.ts (new test, fails on stock bun with the idle timeout close). Also test/js/node/quic/, test/js/bun/http/serve-http3.test.ts and test/js/web/fetch/fetch-http3-client.test.ts.

Background

  • QuicStream wraps one lsquic stream. createBidirectionalStream before the handshake creates the JS object first, with raw null. lsquic calls on_new_stream later, and bind_raw attaches the lsquic stream and replays what JS queued in the meantime (pending_headers).
  • lsquic is event driven: a stream gets an on_write callback only while it has write interest (lsquic_stream_wantwrite). drain_outbound is the on_write handler. It writes queued body bytes, then calls lsquic_stream_flush.
  • outbound.started becomes true when JS takes stream.writer or passes a body. That is why taking the writer before the handshake hid the bug.
Notes

Timing probe against Bun.serve({ http3: true }) on a debug build, from sendHeaders to the server handler: writer taken before the handshake 59 ms, await client.opened first 27 ms, neither (this fix) 38 ms.

PR #42860 and #42867 change the quic.c header writers and on_stream_read. They do not touch bind_raw.

… is bound

A stream created before the handshake completes queues its header
block in pending_headers. bind_raw hands the block to
lsquic_stream_send_headers, which only buffers it. The flush in
drain_outbound runs from on_write, and bind_raw asked for a write
event only when the JS side had already started the writer. Ask for
one whenever a non-terminal header block was queued.

Fixes #42866
@robobun

robobun commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:09 PM PT - Sep 15th, 2026

✅ @robobun, your commit dad2a6a4e94f8d6e54f05070a0c6dae9f3c159c2 passed in Build #116318! 🎉


🧪   To try this PR locally:

bunx bun-pr 42869

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

bun-42869 --bun

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: a4ea80b2-471e-4d9b-bba7-725af2f2c775

📥 Commits

Reviewing files that changed from the base of the PR and between 877a3f9 and dad2a6a.

📒 Files selected for processing (2)
  • src/runtime/node/quic/stream.rs
  • test/js/node/quic/quic-stream.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


Walkthrough

QuicStream::bind_raw now requests write interest when queued headers require flushing. A regression test verifies that headers sent before handshake completion reach the server without stream data writes.

Changes

QUIC queued header flushing

Layer / File(s) Summary
Pending header write handling
src/runtime/node/quic/stream.rs, test/js/node/quic/quic-stream.test.ts
bind_raw tracks successful non-terminal header submission and includes pending headers in write-interest decisions. The test verifies queued POST /queued headers reach the server before client closure.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to dad2a

The queued-header flushing change has no identified unresolved defect and is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: sending headers queued before the QUIC stream is bound.
Description check ✅ Passed The description explains the problem, fix, technical context, and verification. It does not use the template headings exactly, but it provides the required information and is complete.
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.

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

@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.

Beyond the inline finding, I also checked whether the new write event from bind_raw changes JS-visible behavior: the resulting drain_outbound run with an empty queue (clear write interest, flush, possible StreamDrain push since has_outbound is already set) is the same sequence the already-bound sendHeaders path produces via kick_write, so it is parity rather than new behavior. The restructured loop still sets wrote_to_lsquic and handles the EOS branch exactly as before.

Extended reasoning...

The diff is a small, focused change to QuicStream::bind_raw in /home/claude/bun/src/runtime/node/quic/stream.rs plus one new test. I traced the new want_write flag to its consumer (s.want_write(true) at line 268-270), then followed the resulting on_write into drain_outbound (lines 480-544): with an empty outbound queue it hits the empty branch, clears write interest, calls s.flush(), and may push StreamDrain because has_outbound was set to 1 when the headers were queued (line 946). That is the same sequence the bound-stream send_headers path already produces through kick_write (lines 950-964), so the candidate concern about a premature drain event is not introduced by this PR. The refactor of the send_headers == 0 && eos condition into nested ifs preserves the old EOS behavior and the unconditional wrote_to_lsquic.set(true); the non-zero return path remains a silent no-op, which is the inline finding already posted. The single inline finding means a human still needs to look, so this note only records what else was examined.

Comment thread src/runtime/node/quic/stream.rs
@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

The inline finding about a rejected header block in bind_raw is pre-existing and out of scope here. I replied in the thread. No code change in this PR for it.

@Jarred-Sumner
Jarred-Sumner merged commit 403f36d into main Sep 16, 2026
11 of 12 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the robobun/78340612/quic-flush-pending-headers branch September 16, 2026 05:17
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