Skip to content

node:quic h3: deliver response trailers when only ontrailers is set - #35731

Open
robobun wants to merge 5 commits into
mainfrom
claude/farm/df68052d/quic-h3-ontrailers-wantsheaders
Open

robobun wants to merge 5 commits into
mainfrom
claude/farm/df68052d/quic-h3-ontrailers-wantsheaders

Conversation

@robobun

@robobun robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A node:quic HTTP/3 client that sets only ontrailers (no onheaders) never receives trailing headers: the body reads fine, ontrailers never fires, no error.

const st = await client.createBidirectionalStream({
  headers: { ":method": "GET", ":path": "/", ":scheme": "https", ":authority": "localhost" },
});
st.ontrailers = t => console.log("TRAILERS", t);   // never called
for await (const c of st) { ... }                  // body reads fine

This is the common shape for gRPC-over-h3 and any client that cares about trailers but not the initial :status block.

Cause

The native StreamHeaders dispatch is gated on stream.state.wantsHeaders, which is the one flag for every HEADERS kind (initial, trailing, hints). Only the onheaders setter wrote it. Setting ontrailers or oninfo alone left wantsHeaders = 0, so the native side dropped every decoded block.

Separately, on_stream_read claimed lsquic_stream_get_hset() once per callback. stream->uh is a linked list, and when a peer emits HEADERS + DATA + HEADERS(trailers) + FIN in one STREAM frame, lsquic's HQ filter decodes the trailing block as a side effect of draining DATA; lsquic_stream_read then returns -1 ("header set not claimed: cannot read from stream") instead of 0 for the FIN.

Fix

  • set ontrailers / set oninfo / set onheaders now compute wantsHeaders from the OR of all three handlers.
  • [kHeaders] no longer asserts onheaders on an initial block: the block is stashed to stream.headers (unchanged) and the callback is optional, matching the trailing/hints cases.
  • on_stream_read drains stream->uh as a list and re-checks it when lsquic_stream_read returns -1; if a new block appeared it claims it and re-reads for the FIN behind it.

Verification

# fail-before (src/ stashed)
(fail) HTTP/3 response trailers > are delivered when only ontrailers is set
(fail) HTTP/3 response trailers > are delivered when decoded in the same read callback as the body

# pass-after
(pass) HTTP/3 response trailers > are delivered when only ontrailers is set
(pass) HTTP/3 response trailers > are delivered when decoded in the same read callback as the body

Existing test/js/node/quic/*.test.ts and test/js/node/test/parallel/test-quic-h3-* all pass.


[review] gate passed · iteration 2 · 3 files touched

fails on main (without fix)
ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/quic/quic-stream.test.ts
bun test v1.4.0 (8224b855b)

test/js/node/quic/quic-stream.test.ts:
ExperimentalWarning: quic is an experimental feature and might change at any time
      at node:quic (node:quic:16:20)

55 |     // dispatched or never will be.
56 |     const result = await Promise.race([trailers.promise, stream.closed.then(() => "dropped")]);
57 | 
58 |     client.close();
59 |     expect(body).toBe("body");
60 |     expect(result).toEqual({ "x-tr-a": "1", "x-tr-b": "two" });
                        ^
error: expect(received).toEqual(expected)

- {
-   "x-tr-a": "1",
-   "x-tr-b": "two",
- }
+ "dropped"

- Expected  - 4
+ Received  + 1

      at <anonymous> (/workspace/bun/test/js/node/quic/quic-stream.test.ts:60:20)
(fail) HTTP/3 response trailers > are delivered when only ontrailers is set [1063.37ms]
105 |     for await (const _ of stream) {
106 |     }
107 |     const result = await Promise.race([trailers.promise, stream.closed.then(() => "dropped")]);
108 | 
109 |     client.close();
110 |     expect(result).
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (a353fd654)

test/js/node/quic/quic-stream.test.ts:
ExperimentalWarning: quic is an experimental feature and might change at any time
      at node:quic (node:quic:15:20)

(pass) HTTP/3 response trailers > are delivered when only ontrailers is set [56.43ms]
(pass) HTTP/3 response trailers > are delivered when decoded in the same read callback as the body [7.97ms]
(pass) HTTP/3 1xx interim responses are delivered when only oninfo is set [8.59ms]
(pass) QuicStream.destroy after the app ended the send side > delivers the committed response instead of retracting it with RESET_STREAM [10.08ms]
(pass) HTTP/3 header encoding > round-trips non-ASCII header values byte-for-byte [8.64ms]
(pass) HTTP/3 header encoding > rejects a value whose latin1 encoding splices in an extra header [1008.33ms]

 6 pass
 0 fail
 13 expect() calls
Ran 6 tests across 1 file. [1413.00ms]
__F:0:S:0
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/quic/quic-stream.test.ts
bun test v1.4.0 (8224b855b)

test/js/node/quic/quic-stream.test.ts:
ExperimentalWarning: quic is an experimental feature and might change at any time
      at node:quic (node:quic:16:20)

(pass) HTTP/3 response trailers > are delivered when only ontrailers is set [892.24ms]
(pass) HTTP/3 response trailers > are delivered when decoded in the same read callback as the body [192.30ms]
(pass) HTTP/3 1xx interim responses are delivered when only oninfo is set [209.72ms]
(pass) QuicStream.destroy after the app ended the send side > delivers the committed response instead of retracting it with RESET_STREAM [257.58ms]
(pass) HTTP/3 header encoding > round-trips non-ASCII header values byte-for-byte [203.11ms]
(pass) HTTP/3 header encoding > rejects a value whose latin1 encoding splices in an extra header [1187.25ms]

 6 pass
 0 fail
 13 expect() calls
Ran 6 tests across 1 file. [9.80s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1385ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/22] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited
[2/22] gen JS modules (bundle-modules)
Preprocess modules (22369ms)
Bundle modules (829ms)
Postprocesss modules (806ms)
Bundle Functions (2028ms)
Generate Code (76ms)

[26.18s] Bundled "src/js" for production
  2571 kb
  193 internal modules
  13 native modules
  90 internal functions across 19 files
[2/7] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zl
... (truncated)
diff hotspot
src/js/internal/quic/quic.ts          |  16 +++-
 src/runtime/node/quic/stream.rs       | 107 ++++++++++++++----------
 test/js/node/quic/quic-stream.test.ts | 151 +++++++++++++++++++++++++++++++++-
 3 files changed, 225 insertions(+), 49 deletions(-)

gate history · 2 passed · 1 rejected · iteration 2

evidence per changed file
file                                   reads  edits  tests
src/js/internal/quic/quic.ts               5      7      0
src/runtime/node/quic/stream.rs            4     12      0
test/js/node/quic/quic-stream.test.ts      2      4      0

The wantsHeaders state flag gates the native StreamHeaders dispatch for
every kind (initial, trailing, hints), but only the onheaders setter
wrote it. A client that set only ontrailers had wantsHeaders=0, so the
native side dropped every HEADERS block including the trailing one:
body read fine, ontrailers never fired, no error.

The ontrailers and oninfo setters now drive wantsHeaders too, and
[kHeaders] no longer asserts onheaders on an initial block (the block
is stashed to stream.headers and the callback is optional).

on_stream_read now drains stream->uh as a list and re-checks it after
lsquic_stream_read returns -1: the HQ filter can decode the trailing
block while draining DATA, and read() then refuses to proceed past it
('header set not claimed') before returning the FIN behind it.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c687b104-978b-4005-9c4b-394ff7bd1936

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and 8224b85.

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

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

@robobun

robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:08 PM PT - Jul 25th, 2026

@robobun, your commit 8224b85 is building: #81200

Comment thread src/js/internal/quic/quic.ts Outdated
Comment thread src/runtime/node/quic/stream.rs Outdated
Comment thread src/runtime/node/quic/stream.rs
Comment thread src/runtime/node/quic/stream.rs
Comment thread src/runtime/node/quic/stream.rs
Comment thread src/runtime/node/quic/stream.rs Outdated
Comment thread src/js/internal/quic/quic.ts Outdated
Comment thread test/js/node/quic/quic-stream.test.ts
Also trim a few comments flagged by comment-cop; the H3_MESSAGE_ERROR
block is unchanged code that moved into the claim_hsets closure.
Comment thread src/runtime/node/quic/stream.rs

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

Both earlier nits are addressed in 701b69b — syncWantsHeaders is extracted and the oninfo-only path now has its own test. No new issues found; deferring only because the on_stream_read change reworks the lsquic -1 handling into a claim-and-retry loop, which is native protocol read-path logic worth a maintainer glance.

What was reviewed:

  • claim_hsets loop termination — Claimed::Some requires take_header_set() to have popped at least one entry, so the continue cannot spin on an unchanged stream->uh.
  • The Malformed case inside the -1 arm falls through to break (not continue), so a mid-read reset still exits the loop.
  • [kHeaders] initial-block path now guards onheaders optionally while still stashing inner.headers, matching the trailing/hints arms.
  • New tests cover ontrailers-only, same-callback decode, and oninfo-only; each races against stream.closed rather than sleeping.
Extended reasoning...

Overview

The PR fixes two related node:quic HTTP/3 issues: (1) wantsHeaders was only written by the onheaders setter, so a client setting only ontrailers or oninfo never received any HEADERS block; (2) on_stream_read popped stream->uh once per callback, but lsquic's HQ filter can decode a trailing HEADERS block as a side effect of draining DATA, after which lsquic_stream_read returns -1 ("header set not claimed") instead of 0 for FIN. The fix extracts a syncWantsHeaders helper called from all three setters, makes the [kHeaders] initial-block dispatch tolerate a missing onheaders, and refactors the native header claim into a draining closure that is re-invoked when read() returns -1.

Prior feedback

My earlier pass left two nits: extract the repeated wantsHeaders recomputation into a helper, and add coverage for the oninfo-only arm. Commit 701b69b addresses both — syncWantsHeaders(inner) now exists at module scope and is called from each setter, and a third test exercises a 103 Early Hints response with only stream.oninfo set. The remaining unresolved comment-cop bot flags on stream.rs point at RFC-citation comments (H3_MESSAGE_ERROR constant, STOP_SENDING rationale) that were pre-existing and merely re-indented into the new closure, plus one three-line note explaining lsquic's -1 semantics; none read as workaround justifications to me.

Security risks

None identified. The change is on the receive side of an experimental node:quic module; it does not relax any validation (the malformed-request :status check is preserved verbatim inside the closure) and does not touch TLS, credentials, or path handling.

Level of scrutiny

Medium-high. on_stream_read is an unsafe extern "C" lsquic callback on the hot read path; the new -1 → claim_hsets → continue loop changes control flow in a way that depends on lsquic-internal semantics (stream->uh being a finite linked list, -1 meaning either EWOULDBLOCK or unclaimed-hset). I traced termination — Claimed::Some requires at least one successful pop, so each continue strictly shrinks stream->uh and the loop cannot spin — but someone who knows lsquic's HQ filter should confirm the -1 interpretation matches upstream.

Other factors

Test coverage is good: three new tests covering both entry points (setter-after-create and option-at-create), the same-callback-decode edge, and the oninfo sibling; all await real events and race against stream.closed instead of sleeping. Existing test/js/node/quic/* and test-quic-h3-* are reported passing. Given the native read-path refactor, I'm deferring rather than approving.

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

Both earlier nits are addressed in 701b69b (syncWantsHeaders helper extracted; oninfo-only test added), and I found nothing further. Deferring to a human for the on_stream_read refactor: the claim_hsets loop and the -1 → re-claim → continue path change how lsquic's "header set not claimed" contract is handled, which is protocol-sensitive enough to warrant a look from someone familiar with the QUIC subsystem.

Checked: the re-read loop terminates (Claimed::None breaks when no new hset appears); Claimed::Malformed at the second call site still resets the stream via mark_reset before breaking; the JS [kHeaders] initial case now stashes stream.headers and skips the callback when onheaders is unset, matching the trailing/hints arms.

Extended reasoning...

Overview

Three files: src/js/internal/quic/quic.ts (adds syncWantsHeaders helper called from all three header-handler setters; makes the onheaders callback optional in the [kHeaders] initial dispatch), src/runtime/node/quic/stream.rs (refactors on_stream_read to drain stream->uh as a list via a claim_hsets closure and re-invoke it when lsquic_stream_read returns -1), and test/js/node/quic/quic-stream.test.ts (three new tests: ontrailers-only, same-callback trailer decode, oninfo-only).

Security risks

None identified. The change is in the node:quic experimental module's HTTP/3 header dispatch. No new user-controlled parsing; the malformed-request :status guard and H3_MESSAGE_ERROR reset are preserved verbatim inside the new closure. The -1 retry loop is bounded by lsquic's finite stream->uh list, so no DoS via infinite re-read.

Level of scrutiny

Medium-high. The JS changes are mechanical and low-risk. The Rust change is a real control-flow modification in an unsafe extern "C" lsquic callback: it changes a single if let into a draining while let, and changes the -1 arm of the read loop from an unconditional break into a conditional continue. The reasoning (lsquic's HQ filter appends trailers to stream->uh while draining DATA, then lsquic_stream_read returns -1 for "header set not claimed") is well-documented in the PR body and cited to stream_readf in lsquic_stream.c, but verifying that contract and confirming there's no other -1 condition that would spuriously match Claimed::Some is best done by someone who knows the lsquic integration.

Other factors

Both nits from my earlier pass were addressed. The PR body includes fail-before/pass-after evidence for both debug-ASAN and release builds. All comment-cop threads are resolved (three were pre-existing comments that moved via re-indent; one was justified and kept). The one edge I traced but couldn't fully rule out: at the second claim_hsets call site (inside the read loop), a Claimed::Malformed result falls through to break rather than early-returning as the first site does — mark_reset has already fired so the stream is in the errored state, and the subsequent StreamWake is harmless, but it's a slightly different exit shape than the top-level malformed path.

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

LGTM — both earlier nits addressed (extracted syncWantsHeaders, added the oninfo-only test).

What was reviewed:

  • [kHeaders] "initial" case now matches the existing "trailing"/"hints" shape (stash to stream.headers, optional callback) — no assert to trip when only ontrailers/oninfo is set.
  • on_stream_read refactor: if let → while let drain of take_header_set() inside a closure, plus the -1 re-claim path — no new lifetimes/allocations, the loop terminates (finite stream->uh list; Claimed::None breaks), and the Malformed arm in the retry falls through to break after mark_reset, which is fine.
  • Three new tests await real conditions (Promise.race against stream.closed), no sleeps; fail-before/pass-after gated.
Extended reasoning...

Overview

Fixes node:quic HTTP/3 clients that set only ontrailers (or only oninfo) never receiving those HEADERS blocks. Two independent causes: (1) state.wantsHeaders was only written by the onheaders setter, so the native dispatch gate stayed off; (2) on_stream_read claimed lsquic_stream_get_hset() once per callback, but stream->uh is a linked list and lsquic's HQ filter can decode trailers as a side-effect of draining DATA, after which lsquic_stream_read returns -1 ("header set not claimed") instead of 0 for FIN.

Touches src/js/internal/quic/quic.ts (+16/-4), src/runtime/node/quic/stream.rs (+65/-42, mostly re-indent of existing logic into a closure), and test/js/node/quic/quic-stream.test.ts (+151/-3).

Security risks

None identified. node:quic is experimental; no auth/crypto/permission surfaces touched. The header-block validation (server rejecting :status in a request with H3_MESSAGE_ERROR) is preserved verbatim inside the new closure. The -1 retry path only re-reads after successfully claiming a new header set, so it can't spin.

Level of scrutiny

Moderate. The Rust change is in an lsquic FFI callback, but it's a control-flow refactor (if → while, extract to closure, add retry on -1) with no new raw pointers, allocations, or lifetime changes. take_header_set() returns an owned HeaderSet whose .pairs() is consumed into the session event queue exactly as before. peer_is_client is hoisted but computed identically. The JS change is straightforward: a shared syncWantsHeaders helper called from all three setters, and the "initial" dispatch case brought in line with the existing "trailing"/"hints" cases (optional callback, still stashes to inner.headers and publishes to the diagnostic channel).

Other factors

  • Both nits from my previous review were addressed in 701b69b: the three-way duplicated predicate is now one syncWantsHeaders(inner) helper, and a third test covers the oninfo-only arm.
  • The comment-cop bot flags were resolved (comments trimmed or explained as pre-existing text moved into the closure via re-indent).
  • The gate evidence shows all three new tests fail on main and pass with the fix; the existing quic-stream.test.ts tests continue to pass.
  • Tests await observable conditions (Promise.race([handler, stream.closed])) rather than sleeping, and are bounded by maxIdleTimeout: 2.
  • I checked the Claimed::Malformed case in the loop's _ => arm: it breaks (not continues), mark_reset has already fired, and any subsequent StreamWake just surfaces PULL_STATUS_ERROR to the reader — correct.

@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the diff itself is green. On build #81200, 145 jobs passed including every lane that ran test/js/node/quic/quic-stream.test.ts. The failures are unrelated infra:

  • Three build-bun jobs timed out at the 60-minute cap (linux x64-musl, linux x64-android, freebsd aarch64), so their dependent test lanes never ran.
  • Four known-flaky tests (filter-workspace, test-fs-read-stream-pos, 20144, webview-chrome) tripped once and passed on retry; none touch node:quic.
  • The previous build #81049 had the same shape: linux x64-asan build-bun timed out, no quic test failures.

Ready for review; a maintainer can retry the timed-out build lanes if a full-green run is needed.

Jarred-Sumner pushed a commit that referenced this pull request Sep 16, 2026
…is read (#42860)

### Problem
- Since the lsquic 4.9.4 update (#42523), `fetch()` over HTTP/3 rejects
with `HTTP3StreamReset` when two 1xx blocks precede a final response
with no body. A `node:quic` client gets no `onheaders`, and a
`node:quic` server loses trailers that follow the request headers. Bun
1.4.2 passes. No release has it yet.
- `lsquic_stream_read` now decodes a header block that follows another
one, and returns 0 at the end of the stream with that block still
queued. `us_quic_on_read` (`packages/bun-usockets/src/quic.c:676`) and
`on_stream_read` (`src/runtime/node/quic/stream.rs:1084`) claim blocks
only before they read. They deliver the FIN and lose the block.
- A `node:quic` client gets the body before `onheaders`. It queues two
decoded blocks, so one read decodes the final response behind a claimed
1xx and returns its DATA.

### Fix
- Both readers claim every queued block before the FIN.
- `us_nq_settings_init` sets `es_max_header_sets` to 1, as a server has.
lsquic then parses nothing past an unclaimed block, as in 1.4.2.
- Verified: `test/js/node/quic/quic-stream.test.ts` (2 new) and
`test/js/web/fetch/fetch-http3-client.test.ts` (`/no-body`) fail on main
and pass on 1.4.2. Also `test/js/node/quic/`, `serve-http3`,
`fetch-http3-*`, `serve-protocols`.
- Self-reviewed: 8 concerns raised, 7 addressed. Open: no upstream
lsquic report (Notes).

### Background
- A header set is one decoded HEADERS block (1xx, final response,
trailers). lsquic queues them, and `lsquic_stream_get_hset` claims the
next one.
- `es_max_header_sets` bounds the queue. Parsing stops while it is full.
- On receipt lsquic parses a stream only until a block is queued. It
parses the rest inside `lsquic_stream_read`, so a block that shares a
STREAM frame with an earlier one is decoded during a read.

<details><summary>Notes</summary>

**Found by a read of the code, not from a user report.** It came up
while I worked on the late `100 Continue` of `Bun.serve({ http3: true
})`: a server that sends its 100 early puts it in its own STREAM frame,
and that exposes these readers. The flush is #42867, stacked on this PR.

**Versions.** 1.4.2 (744846f, lsquic 3181911) against canary 09bb546
and main (lsquic 4.9.4), same scripts:

| peer sends in one write | 1.4.2 | 09bb546 |
| --- | --- | --- |
| 100, 103, 204 to `fetch()` | `204` | `HTTP3StreamReset` |
| 100, 103, 204 to a `node:quic` client | `info 100, info 103, headers
204` | `info 100, info 103` |
| 100, 103, 200 + body to a `node:quic` client | `headers 200` before
the data | `headers 200` after the stream ended |
| request headers + trailers to a `node:quic` server | `ontrailers`
fires | no `ontrailers` |

**The exact window.** The block is lost only when one
`lsquic_stream_read` call decodes it and also reaches the FIN:
- three blocks plus FIN in one frame to a client. On receipt lsquic
decodes two (the client's queue bound) and stops. The reader claims
them, and the third is decoded inside the read.
- a 1xx in one STREAM frame and the final HEADERS plus FIN in the next,
both received before `on_read` runs. lsquic skips its parser on receipt
while a decoded block is queued.
- two blocks plus FIN in one frame to a server, whose queue bound is 1.

A 1xx and a final response that arrive in different event loop turns
pass on every build, because `on_read` runs between them. Trailers that
follow DATA were already delivered: the read returns the bytes first,
the next read fails on the unclaimed block, and lsquic calls `on_read`
again.

**lsquic.** At the pinned commit, `stream_readf` returns 0 when it read
nothing and reached the FIN, also when `uh` is not empty
([`lsquic_stream.c#L1650-L1667`](https://github.com/litespeedtech/lsquic/blob/f812ceb9529e26717df78c6064a2ff648b949f3e/src/liblsquic/lsquic_stream.c#L1650-L1667)).
`lsquic_stream_readf` guards the same state only on entry
([`#L1693-L1703`](https://github.com/litespeedtech/lsquic/blob/f812ceb9529e26717df78c6064a2ff648b949f3e/src/liblsquic/lsquic_stream.c#L1693-L1703))
and lets the next call fail with "header set not claimed". The queue
bound defaults to 1 for a server and 2 for a client
([`lsquic.h#L330-L335`](https://github.com/litespeedtech/lsquic/blob/f812ceb9529e26717df78c6064a2ff648b949f3e/include/lsquic.h#L330-L335)).
I did not report this upstream. The two loops are correct with and
without an upstream change.

**Why not a `patches/lsquic` patch.** A few lines in `stream_readf`
would cover both readers. The loops need no patch to carry across lsquic
updates, and they are also correct against a queue bound above 1.

**`es_max_header_sets` for `fetch()`.** The `quic.c` client keeps
lsquic's default of 2. `ClientSession::deliver` holds body bytes until
the final headers arrive, so DATA ahead of its HEADERS is not observable
there. I found no test that can tell the two values apart for `fetch()`,
so I left it.

**Each part is live.** With the `es_max_header_sets` line removed, only
the `/body` order assertion of the client test fails. With `origin/main`
sources, both `node:quic` tests and the `fetch()` `/no-body` case fail
on the debug build.

**Not asserted.** The order of `ontrailers` against the end of the
request body on a server. Both events are queued by one lsquic callback,
and the body iteration can finish in the microtasks between them. 1.4.2
has the same order.

**Excluded.** `packages/h3blast/src/h3blast.c:435` has the same single
claim. It is a standalone load generator, it does not link until #42406,
and it never sends `Expect`.

**Overlap.** #35731 hoists the same claim block of `on_stream_read` into
a loop and changes the would-block arm. It leaves the end-of-stream arm
alone and predates #42523, so it does not fix this. It needs a rebase
onto this change: the closure here has the same shape.

**Other runs.** The 25 `test-quic-h3-*.mjs` files: 24 pass.
`test-quic-h3-stream-idle-timeout.mjs` fails the same way on canary
09bb546.

</details>

<!-- robobun:evidence:begin -->

---

**[human-review]** gate passed · iteration 0 · 5 files touched

<details><summary>fails on main (without fix)</summary>

```console
ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/quic/quic-stream.test.ts "test/js/web/fetch/fetch-http3-client.test.ts"
bun test v1.4.3 (09bb546)

test/js/web/fetch/fetch-http3-client.test.ts:
(node:380852) ExperimentalWarning: quic is an experimental feature and might change at any time
(Use `bun-debug --trace-warnings ...` to show where the warning was created)
(pass) fetch protocol: http3 > GET text [40.56ms]
(pass) fetch protocol: http3 > 'h3' alias [14.27ms]
(pass) fetch protocol: http3 > POST echo with headers [33.47ms]
(pass) fetch protocol: http3 > JSON + query string [31.69ms]
(pass) fetch protocol: http3 > route params [17.44ms]
(pass) fetch protocol: http3 > large response body (multi-packet) [21.82ms]
(pass) fetch protocol: http3 > large request body [59.43ms]
(pass) fetch protocol: http3 > compress: gzip request body — small (shared-buffer fast path) [32.80ms]
(pass) fetch protocol: http3 > compress: gzip request body — large (zlib-streaming spill path) [17.73ms]
(pass) fetch protocol: http3 > status 200 [19.93ms]
(pass) fetch protocol: http3 > status 
... (truncated)

release without fix: 5 FAILED
bun test v1.4.3-canary.1 (09bb546)

test/js/web/fetch/fetch-http3-client.test.ts:
(node:380939) ExperimentalWarning: quic is an experimental feature and might change at any time
(Use `bun --trace-warnings ...` to show where the warning was created)
(pass) fetch protocol: http3 > GET text [4.63ms]
(pass) fetch protocol: http3 > 'h3' alias [0.38ms]
(pass) fetch protocol: http3 > POST echo with headers [0.84ms]
(pass) fetch protocol: http3 > JSON + query string [0.42ms]
(pass) fetch protocol: http3 > route params [0.33ms]
(pass) fetch protocol: http3 > large response body (multi-packet) [1.96ms]
(pass) fetch protocol: http3 > large request body [2.05ms]
(pass) fetch protocol: http3 > compress: gzip request body — small (shared-buffer fast path) [1.05ms]
(pass) fetch protocol: http3 > compress: gzip request body — large (zlib-streaming spill path) [2.33ms]
(pass) fetch protocol: http3 > status 200 [0.54ms]
(pass) fetch protocol: http3 > status 204 [0.18ms]
(pass) fetch protocol: http3 > status 404 [0.14ms]
(pass) fetch protocol: http3 > status 500 [0.12ms]
(pass) fetch protocol: http3 > HEAD has no body [0.36ms]
(pass) fetch protocol: http3 > the response to a 204
... (truncated)
```

</details>

<details><summary>passes on PR (with fix)</summary>

```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/quic/quic-stream.test.ts "test/js/web/fetch/fetch-http3-client.test.ts"
bun test v1.4.3 (09bb546)

test/js/web/fetch/fetch-http3-client.test.ts:
(node:382834) ExperimentalWarning: quic is an experimental feature and might change at any time
(Use `bun-debug --trace-warnings ...` to show where the warning was created)
(pass) fetch protocol: http3 > GET text [58.80ms]
(pass) fetch protocol: http3 > 'h3' alias [19.68ms]
(pass) fetch protocol: http3 > POST echo with headers [57.54ms]
(pass) fetch protocol: http3 > JSON + query string [29.50ms]
(pass) fetch protocol: http3 > route params [25.71ms]
(pass) fetch protocol: http3 > large response body (multi-packet) [38.49ms]
(pass) fetch protocol: http3 > large request body [34.48ms]
(pass) fetch protocol: http3 > compress: gzip request body — small (shared-buffer fast path) [45.96ms]
(pass) fetch protocol: http3 > compress: gzip request body — large (zlib-streaming spill path) [25.14ms]
(pass) fetch protocol: http3 > status 200 [27.31ms]
(pass) fetch protocol: http3 > status 
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     c446afd
  features     lto, baseline

23 deps, 131 codegen, 1176 objects in 822ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1254] mkdir codegen
[2/1254] mkdir stamps
[3/1254] mkdir pch
[4/1254] mkdir obj
[5/1254] install /workspace/bun
bun install v1.4.3-canary.1 (09bb546)

Checked 22 installs across 61 packages (no changes) [24.00ms]
[6/1254] install /workspace/bun/packages/bun-error
bun install v1.4.3-canary.1 (09bb546)

Checked 1 install across 2 packages (no changes) [1.00ms]
[7/1254] install /workspace/bun/src/node-fallbacks
bun install v1.4.3-canary.1 (09bb546)

Checked 111 installs across 104 packages (no changes) [5.00ms]
[8/1254] gen ErrorCode+*.h
[9/1254] gen node-fallbacks/react-refresh.js
Bundled 1 module in 9ms

  react-refresh.js  4.81 KB  (entry point)

[10/1254] fetch picohttpparser
[picohttpparser] up to date
[11/1254] gen bindgenv2
[12/1254] fetch tinycc
[tinycc] up to date
[13/1254] fetch zlib
[zlib] up to date
[14/1254] gen JSBuffer.lut.h

... (truncated)
```

</details>

<details><summary>diff hotspot</summary>

```
packages/bun-usockets/src/node_quic_shim.c   |   3 +
 packages/bun-usockets/src/quic.c             |  36 ++++++----
 src/runtime/node/quic/stream.rs              |  93 +++++++++++++-----------
 test/js/node/quic/quic-stream.test.ts        | 104 +++++++++++++++++++++++++++
 test/js/web/fetch/fetch-http3-client.test.ts |  46 ++++++++++++
 5 files changed, 226 insertions(+), 56 deletions(-)
```

</details>

**gate history** · 1 passed · 0 rejected · iteration 0

<details><summary>evidence per changed file</summary>

```
file                                          reads  edits  tests
packages/bun-usockets/src/node_quic_shim.c        2      3     48
packages/bun-usockets/src/quic.c                 17     21     51
src/runtime/node/quic/stream.rs                   2      3     49
test/js/node/quic/quic-stream.test.ts             2      6     22
test/js/web/fetch/fetch-http3-client.test.ts      3      8     40
```

</details>

<!-- robobun:evidence:end -->

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