Skip to content

http3: split H3Client into h3_client/ + Alt-Svc upgrade flag#29863

Merged
Jarred-Sumner merged 7 commits into
mainfrom
claude/split-h3-client
Apr 28, 2026
Merged

http3: split H3Client into h3_client/ + Alt-Svc upgrade flag#29863
Jarred-Sumner merged 7 commits into
mainfrom
claude/split-h3-client

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Two related changes to the HTTP/3 client.

1. Split H3Client.zig into h3_client/ (no functional change)

Mirrors the h2_client/ layout so the file shape matches across the two multiplexed transports:

H3Client.zig            - thin entry point: re-exports + live counters
h3_client/
  Stream.zig            - one in-flight request
  ClientSession.zig     - one QUIC connection (pool entry, deliver loop)
  ClientContext.zig     - process-global lsquic engine + session registry
  encode.zig            - request header/body framing onto a quic.Stream
  callbacks.zig         - lsquic -> Zig glue (on_hsk_done / on_stream_*)
  PendingConnect.zig    - DNS-pending connect resolution (moved from h3/)

2. --experimental-http3-fetch / BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP3_CLIENT

Mirrors the --experimental-http2-fetch pair. When enabled, fetch() records Alt-Svc: h3 from HTTPS responses into a small HTTP-thread-local origin cache (h3_client/AltSvc.zig, RFC 7838) and routes the next request to that origin over QUIC instead of TCP.

  • AltSvc.parse() — field-value parser returning {port, ma} for the first same-host h3 alternative, or error.Clear for the clear token.
  • AltSvc.record/lookup() — origin-keyed cache with ma-based expiry.
  • canTryH3AltSvc() gate matching canOfferH2(); consulted in start_() before opening TCP and in handleResponseMetadata when reading the header.
  • fetchH3Internals.liveCounts exposed via bun:internal-for-testing so tests can prove the second fetch actually opened a QUIC session.

Tested locally:

  • fetch-http3-client.test.ts — 48 pass (incl. 3 new Alt-Svc tests covering env var, CLI flag, and off-by-default)
  • fetch-http3-adversarial.test.ts — 26 pass
  • serve-http3.test.ts — 44 pass
  • zig:check-all — all targets

Mirrors the h2_client/ layout so the file shape matches across the two
multiplexed transports:

  H3Client.zig            - thin entry point: re-exports + live counters
  h3_client/
    Stream.zig            - one in-flight request
    ClientSession.zig     - one QUIC connection (pool entry, deliver loop)
    ClientContext.zig     - process-global lsquic engine + session registry
    encode.zig            - request header/body framing onto a quic.Stream
    callbacks.zig         - lsquic -> Zig glue (on_hsk_done / on_stream_*)
    PendingConnect.zig    - DNS-pending connect resolution (moved from h3/)

No functional change; pure code motion plus the visibility tweaks needed
for the cross-file calls (fail/retryOrFail/deliver are now pub on
ClientSession, callback registration is bundled into callbacks.register).
@robobun

robobun commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator
Updated 4:45 PM PT - Apr 28th, 2026

@Jarred-Sumner, your commit a94aa99 has 2 failures in Build #48647 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29863

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

bun-29863 --bun

@coderabbitai

coderabbitai Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Refactors the in-file HTTP/3 client into new src/http/h3_client/* modules, re-exports core types from src/http/H3Client.zig, adds Alt‑Svc parsing/caching, CLI/env feature flags, JS testing hook for QUIC live counts, and subprocess tests to validate experimental HTTP/3 (QUIC) fetch behavior.

Changes

Cohort / File(s) Summary
Public re-export surface
src/http/H3Client.zig
Removed inline lsquic transport and local Stream/ClientSession/ClientContext/callbacks; now re-exports those symbols (and PendingConnect) from ./h3_client/, adds AltSvc, renames testing hook to TestingAPIs.quicLiveCounts, and drops unused imports.
Client lifecycle & registry
src/http/h3_client/ClientContext.zig
Adds process-global, lazily-initialized QUIC client context, session registry, get/getOrCreate, connect, unregister, and async-id routing helpers.
Session management & delivery
src/http/h3_client/ClientSession.zig
New ClientSession type with identity/state, pending stream FIFO, refcounting, enqueue/detach/fail/retry logic, async-id routing, and deliver response state machine.
Per-request stream model
src/http/h3_client/Stream.zig
New Stream type for in-flight H3 requests: owner/session, optional HTTPClient/qstream, header/body buffers, status flags, deinit and abort, and live-stream accounting.
Encoding & body transmission
src/http/h3_client/encode.zig
Adds QPACK header encoding, pseudo-header composition, header send semantics, and flow-control-aware body draining (writeRequest, drainSendBody).
lsquic callback glue
src/http/h3_client/callbacks.zig
Registers QUIC callbacks (handshake/GOAWAY/close/stream events); binds pending requests to streams, drives encoders, parses :status, forwards headers/body/writable/close events, and exposes register(qctx).
Alt‑Svc caching
src/http/h3_client/AltSvc.zig
Parses Alt-Svc values (including clear), records per-origin h3 port with expiry, and provides record() and lookup() backed by an unmanaged origin-keyed cache.
HTTP integration & feature flags
src/http.zig, src/env_var.zig
Adds experimental_http3_client_from_cli, canTryH3AltSvc() predicate, consults H3.AltSvc.lookup() before connect, records Alt-Svc from responses, and introduces BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP3_CLIENT.
CLI/boot/test wiring
src/bun.js.zig, src/cli.zig, src/cli/Arguments.zig, src/cli/test/parallel/runner.zig, src/cli/test_command.zig
Adds experimental_http3_fetch CLI flag, forwards it through runtime/worker/test boot paths, and wires it into test worker invocation.
JS testing exposure & tests
src/js/internal-for-testing.ts, test/js/web/fetch/fetch-http3-client.test.ts
Exports a JS test internals object (H3 live counts) and adds subprocess tests validating Alt‑Svc-driven experimental HTTP/3 upgrade behavior.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the two main structural changes: splitting H3Client into h3_client/ modules and adding an Alt-Svc upgrade flag, matching the actual changeset scope.
Description check ✅ Passed The description covers both objectives comprehensively with clear section headings, implementation details, module layout, feature behavior, and test results, following the template's required sections.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@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 — pure code motion splitting H3Client.zig into an h3_client/ module to mirror the h2_client/ layout.

Extended reasoning...

Overview

This PR splits the monolithic src/http/H3Client.zig (~660 lines) into a directory module src/http/h3_client/ with six files (Stream.zig, ClientSession.zig, ClientContext.zig, encode.zig, callbacks.zig, PendingConnect.zig), leaving H3Client.zig as a thin re-export façade plus the atomic live-object counters and TestingAPIs. PendingConnect.zig is a pure rename from src/http/h3/src/http/h3_client/ with no content change.

I diffed the moved bodies line-by-line against the deletions and they are byte-identical modulo: (a) fnpub fn on fail/retryOrFail/deliver/writeRequest/drainSendBody so they're reachable across the new file boundaries, (b) the eight qctx.onX(...) registrations folded into callbacks.register(qctx) which contains exactly the same eight calls in the same order, (c) ClientContext.instanceClientContext.get() in onConnClose (get() is a one-line accessor returning instance), (d) live_sessions/live_streamsH3.live_sessions/H3.live_streams since the counters stayed in the parent file, and (e) writeRequest's receiver renamed thissession with the single use (this.hostname) updated accordingly. A few doc comments were expanded (e.g. the RefCount holder list) but no logic changed.

Security risks

None. This is a file reorganization of the experimental HTTP/3 fetch client; no auth, crypto, parsing, or trust-boundary code is altered. The QPACK header building, body draining, and lsquic callback handling are moved verbatim.

Level of scrutiny

Low–medium. The change is mechanical and mirrors the already-landed h2_client/ split (#29809), so the target shape is established. The HTTP/3 client itself is an experimental feature gated behind protocol: "http3" (landed in #29795 a few commits ago). The author reports all three HTTP/3 test suites (115 tests) and zig:check-all pass; the bug-hunting system found nothing.

Other factors

No CODEOWNERS cover src/http/. No outstanding reviewer comments. The old src/http/h3/ directory is gone and nothing in the tree references the old h3/PendingConnect path. Public surface (H3.Stream, H3.ClientSession, H3.ClientContext, H3.PendingConnect) is preserved via re-exports, so external callers are unaffected.

Mirrors the --experimental-http2-fetch / BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT
pair. When enabled, fetch() records Alt-Svc: h3 from HTTPS responses into a
small HTTP-thread-local origin cache (h3_client/AltSvc.zig) and routes the
next request to that origin over QUIC instead of TCP.

- AltSvc.parse(): RFC 7838 field-value parser returning {port, ma} for the
  first same-host h3 alternative, or error.Clear for the 'clear' token.
- AltSvc.record/lookup(): origin-keyed cache with ma-based expiry.
- canTryH3AltSvc() gate in http.zig matching the canOfferH2() shape; consulted
  in start_() before opening TCP and in handleResponseMetadata when reading
  the header.
- fetchH3Internals.liveCounts exposed via bun:internal-for-testing so the
  test can prove the second fetch actually opened a QUIC session.
  (TestingAPIs.quicLiveCounts — distinct name needed because
  generate-js2native mangles digits, so H2Client/H3Client collide.)

Tests cover env var, CLI flag, and off-by-default.

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/http.zig`:
- Around line 1200-1210: H3 Alt-Svc lookup currently discards the returned
advertised port; update the logic around H3.AltSvc.lookup(this.url.hostname,
this.url.getPortAuto()) so that when it returns a port you store it (e.g., in a
local variable or a new field like alt_svc_port) and set this.flags.force_http3
as before, then use that cached port instead of this.url.getPortAuto() when
opening the QUIC connection (the connect call that initiates HTTP/3); ensure the
lookup is still conditional on canTryH3AltSvc() and that fallback semantics
remain unchanged.

In `@src/http/h3_client/AltSvc.zig`:
- Around line 123-125: Change the expiry check to expire entries on the boundary
by using >= instead of > when comparing std.time.timestamp() and rec.expires_at
in AltSvc.zig; update the conditional that currently reads "if
(std.time.timestamp() > rec.expires_at)" to use ">=" so that when the current
timestamp equals rec.expires_at (e.g., ma=0) the code will call
cache.fetchRemove(k) and free the key via bun.default_allocator.free(kv.key) and
then return null, ensuring immediate expiration; adjust only this comparison in
the block that references rec.expires_at, cache.fetchRemove, and
bun.default_allocator.free.

In `@src/http/H3Client.zig`:
- Around line 24-34: The comment above the TestingAPIs struct references
TestingAPIs.liveCounts but the exported function is named quicLiveCounts; update
the comment to reference quicLiveCounts (or "quicLiveCounts" /
"TestingAPIs.quicLiveCounts") so it correctly documents how JS reads
live_sessions and live_streams; locate the comment near the declarations of
live_sessions, live_streams and the TestingAPIs struct and replace
"TestingAPIs.liveCounts" with the correct identifier
"TestingAPIs.quicLiveCounts".
🪄 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: fdc529aa-4af9-414d-8c01-5a74dbeb7fc1

📥 Commits

Reviewing files that changed from the base of the PR and between e6670de and 1339fa5.

📒 Files selected for processing (11)
  • src/bun.js.zig
  • src/cli.zig
  • src/cli/Arguments.zig
  • src/cli/test/parallel/runner.zig
  • src/cli/test_command.zig
  • src/env_var.zig
  • src/http.zig
  • src/http/H3Client.zig
  • src/http/h3_client/AltSvc.zig
  • src/js/internal-for-testing.ts
  • test/js/web/fetch/fetch-http3-client.test.ts

Comment thread src/http.zig
Comment thread src/http/h3_client/AltSvc.zig Outdated
Comment thread src/http/H3Client.zig
- start_(): use the alt-authority port from AltSvc.lookup() for the QUIC
  connect instead of always dialing the origin port (RFC 7838 §3 allows the
  alternative to listen elsewhere; SNI/cert validation still use the origin
  hostname).
- AltSvc.lookup(): expire on >= so ma=0 is treated as immediately stale.
- H3Client.zig: doc comment now references quicLiveCounts.
@Jarred-Sumner Jarred-Sumner changed the title http3: split H3Client.zig into h3_client/ module http3: split H3Client into h3_client/ + Alt-Svc upgrade flag Apr 28, 2026
Comment thread src/http.zig

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/http.zig (1)

1211-1233: ⚠️ Potential issue | 🟠 Major

Keep Alt-Svc as a soft upgrade, not a hard protocol pin.

At Line 1213 the cache hit flips this.flags.force_http3 before we know the QUIC path is usable. That makes the failure branches at Lines 1228-1233 behave like an explicit protocol: "http3" request: a stale/unusable Alt-Svc entry now fails the fetch instead of falling back to the normal TCP connect. Please track the Alt-Svc attempt separately and only hard-fail when HTTP/3 was explicitly requested.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/http.zig` around lines 1211 - 1233, Currently a successful
H3.AltSvc.lookup flips this.flags.force_http3 before we know QUIC will work;
change this to keep Alt-Svc as a soft upgrade by not mutating
this.flags.force_http3 on lookup success. Instead, store the alt port in a local
(e.g., h3_alt_port) and a local bool (e.g., attempted_alt_svc) and use that to
try ctx.connect(this, this.url.hostname, h3_alt_port) without setting
force_http3; only set force_http3 (or hard-fail) when the request explicitly
asked for HTTP/3, or if connect succeeds and you want to persist the preference.
Ensure the existing failure branches that call this.fail(error.HTTP3Unsupported)
or this.fail(error.ConnectionRefused) only execute when the request explicitly
required HTTP/3, otherwise fall back to the normal TCP path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/http.zig`:
- Around line 1211-1233: Currently a successful H3.AltSvc.lookup flips
this.flags.force_http3 before we know QUIC will work; change this to keep
Alt-Svc as a soft upgrade by not mutating this.flags.force_http3 on lookup
success. Instead, store the alt port in a local (e.g., h3_alt_port) and a local
bool (e.g., attempted_alt_svc) and use that to try ctx.connect(this,
this.url.hostname, h3_alt_port) without setting force_http3; only set
force_http3 (or hard-fail) when the request explicitly asked for HTTP/3, or if
connect succeeds and you want to persist the preference. Ensure the existing
failure branches that call this.fail(error.HTTP3Unsupported) or
this.fail(error.ConnectionRefused) only execute when the request explicitly
required HTTP/3, otherwise fall back to the normal TCP path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 715ee397-5d2e-4273-9b50-631f6eb0b800

📥 Commits

Reviewing files that changed from the base of the PR and between 1339fa5 and eef6351.

📒 Files selected for processing (3)
  • src/http.zig
  • src/http/H3Client.zig
  • src/http/h3_client/AltSvc.zig

ban-words.test.ts forbids std.StringHashMapUnmanaged in favour of the
faster-eql wrapper.
Comment thread src/http.zig
Comment on lines +1211 to +1215
if (!this.flags.force_http3 and this.canTryH3AltSvc()) {
if (H3.AltSvc.lookup(this.url.hostname, h3_port)) |alt_port| {
this.flags.force_http3 = true;
h3_port = alt_port;
}

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.

🔴 Setting this.flags.force_http3 = true here for an opportunistic Alt-Svc upgrade persists across redirects: doRedirectMultiplexed() resets flags.protocol and flags.proxy_tunneling but never clears force_http3, so the redirect leg re-enters start_() with it still set. A cross-origin redirect target gets forced onto QUIC even though it never advertised h3 (plain fetch() fails with HTTP3HandshakeFailed), a same-origin alt-port redirect skips the !force_http3-gated lookup and dials QUIC on the origin port instead of the advertised alt-port, and an https→http redirect hard-fails with HTTP3Unsupported. Either clear force_http3 in doRedirectMultiplexed() (when it was set opportunistically rather than via protocol:"http3"), or use a one-shot local instead of persisting the decision into flags.

Extended reasoning...

What the bug is

The new Alt-Svc upgrade path at src/http.zig:1211-1215 writes the opportunistic h3 decision into this.flags.force_http3:

if (!this.flags.force_http3 and this.canTryH3AltSvc()) {
    if (H3.AltSvc.lookup(this.url.hostname, h3_port)) |alt_port| {
        this.flags.force_http3 = true;
        h3_port = alt_port;
    }
}

force_http3 is a persistent HTTPClient flag. Grepping src/http.zig for force_http3 shows only the field decl (line 612), the read at 1211, the write at 1213, and the read at 1219 — it is never cleared anywhere. doRedirectMultiplexed() (lines 2321-2348) resets flags.protocol = .http1_1 and flags.proxy_tunneling = false and then calls this.start(...), which re-enters start_() based on the new URL's isHTTPS(). start() (lines 1155-1166) does not touch force_http3 either. So once line 1213 fires, every subsequent redirect leg sees force_http3 == true.

The specific code path

On the redirect leg, start_() runs again with the flag still set:

  1. The Alt-Svc lookup at line 1211 is gated on !this.flags.force_http3, so it is skippedh3_port stays at this.url.getPortAuto() (the redirect target's origin port), and no per-origin check is done for the new host.
  2. Line 1219 if (this.flags.force_http3) is true, so the request is unconditionally routed onto the QUIC engine via ctx.connect(this, this.url.hostname, h3_port).
  3. If the redirect target is http://, start_(false) is called and lines 1220-1222 immediately fail(error.HTTP3Unsupported).

Why existing code doesn't prevent it

Before this PR, force_http3 was only ever set by the user via fetch(url, {protocol: "http3"}) — the field's doc comment still says exactly that. In that case, stickiness across redirects is the user's explicit choice (they asked for h3-or-fail). This PR repurposes the same flag for an opportunistic, per-origin decision derived from a cached Alt-Svc header. That decision is only valid for the origin it was looked up against (RFC 7838 §2: an alternative service is associated with a specific origin), not for an arbitrary redirect target. doRedirectMultiplexed() was never updated to account for the new write site.

Impact

Three concrete failure modes for a plain fetch() with --experimental-http3-fetch enabled and no protocol: hint:

  • Cross-origin redirect (very common: short-URL services, OAuth, CDN bounces): https://a.com previously advertised Alt-Svc: h3, so the request goes over QUIC and gets a 302 Location: https://b.com/.... The follow-up is forced onto QUIC against b.com:443 even though b.com never advertised h3. If b.com has no QUIC listener, the user's fetch() fails with HTTP3HandshakeFailed.
  • Same-origin redirect with alt-port ≠ origin-port: a.com:443 advertised Alt-Svc: h3=":8443". The first leg correctly dials QUIC on :8443 (per the eef6351 fix). The 302 → /other re-enters start_(), the !force_http3 gate skips the lookup, h3_port = getPortAuto() = 443, and ctx.connect dials QUIC on :443 where nothing is listening.
  • https→http redirect: start_(false) hits if (this.flags.force_http3) { if (!is_ssl) fail(HTTP3Unsupported) }, so an opportunistically-upgraded request hard-fails on a downgrade redirect that would have worked fine over HTTP/1.1.

Step-by-step proof (cross-origin case)

  1. --experimental-http3-fetch is on. A previous response from https://a.com carried Alt-Svc: h3=":443", so AltSvc.cache["a.com:443"] = { h3_port: 443, ... }.
  2. fetch("https://a.com/x")start_(true): force_http3 is false, canTryH3AltSvc() is true, lookup("a.com", 443) returns 443, so force_http3 = true and the request goes over QUIC.
  3. a.com responds 302 Location: https://b.com/y. ClientSession.deliverdoRedirectH3()doRedirectMultiplexed().
  4. doRedirectMultiplexed() sets flags.protocol = .http1_1, flags.proxy_tunneling = false, calls this.start(...). force_http3 is still true.
  5. start() sees isHTTPS() for https://b.com/ystart_(true).
  6. Line 1211: !this.flags.force_http3 is false → lookup skipped. h3_port = b.com's port = 443.
  7. Line 1219: force_http3 is true → ctx.connect(this, "b.com", 443) opens QUIC to b.com:443.
  8. b.com has no UDP/QUIC listener on 443 → handshake fails → fetch() rejects with HTTP3HandshakeFailed, even though the user never asked for h3 and b.com would have served the request fine over TCP.

How to fix

Don't persist the opportunistic decision into flags.force_http3. Either:

  • Use a one-shot local (e.g., var use_h3 = this.flags.force_http3; ... if (lookup(...)) |p| { use_h3 = true; h3_port = p; } and branch on use_h3 at line 1219), so the Alt-Svc choice is re-evaluated per start_() call; or
  • Add a separate flags.alt_svc_h3 bit that doRedirectMultiplexed() clears alongside protocol/proxy_tunneling, so the explicit protocol:"http3" semantics (sticky) stay distinct from the opportunistic ones (per-origin).

The first option also fixes the alt-port-on-redirect case automatically, since the lookup runs again against the redirect target.

flags.force_http3 is the user's explicit protocol:"http3" choice and
survives doRedirectMultiplexed(). An opportunistic Alt-Svc upgrade is
per-origin, so a cross-origin redirect must re-evaluate from h1 — set up
the QUIC connect inline instead of toggling the flag, so the redirected
start_() consults the cache for the new origin (and falls through to TCP
when there's no entry).

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/http/h3_client/AltSvc.zig`:
- Around line 85-87: The helper `key` currently masks formatting failures by
returning `catch buf`, which can yield an invalid/non-canonical slice; change
`fn key(buf: []u8, hostname: []const u8, port: u16) []const u8` to a fallible
signature (e.g., return an error union like `?[]const u8` or `![]const u8`) and
remove the `catch buf` fallback so `std.fmt.bufPrint` errors propagate. Then
update the call sites in `record` and `lookup` to handle the error (propagate it
upward or return a meaningful error) instead of relying on a bogus key.
Reference `std.fmt.bufPrint`, `key`, and the `record`/`lookup` functions when
making these changes so the formatting failure is surfaced and handled properly.
- Around line 83-113: The Alt-Svc cache (cache) grows unbounded because record()
always inserts new origins; modify record() to enforce a maximum size and
opportunistically sweep expired entries when inserting: implement a configurable
MAX_ALT_SVC_ENTRIES constant, check cache.count() (or track a simple counter)
before inserting and if the cap is exceeded run a short bounded sweep through
cache to remove expired entries (use the same expires_at logic as
lookup/expiry), and if still over cap evict the oldest/least-recent/any expired
entry (e.g., by checking timestamps on values) before calling getOrPut; keep all
changes inside record(), reusing key(), parse(), fetchRemove(), getOrPut() and
preserves existing clear behavior.
🪄 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: 5a9d60e3-11ec-4bba-9b48-9d4610b97b11

📥 Commits

Reviewing files that changed from the base of the PR and between eef6351 and d936557.

📒 Files selected for processing (1)
  • src/http/h3_client/AltSvc.zig

Comment thread src/http/h3_client/AltSvc.zig
Comment on lines +85 to +87
fn key(buf: []u8, hostname: []const u8, port: u16) []const u8 {
return std.fmt.bufPrint(buf, "{s}:{d}", .{ hostname, port }) catch buf;
}

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.

⚠️ Potential issue | 🟡 Minor

Avoid returning a fallback key slice from key() on formatting failure.

catch buf can return a non-canonical/undefined key slice if bufPrint ever fails. Make this helper fallible and handle the error at call sites (record/lookup) instead of producing a fallback key.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/http/h3_client/AltSvc.zig` around lines 85 - 87, The helper `key`
currently masks formatting failures by returning `catch buf`, which can yield an
invalid/non-canonical slice; change `fn key(buf: []u8, hostname: []const u8,
port: u16) []const u8` to a fallible signature (e.g., return an error union like
`?[]const u8` or `![]const u8`) and remove the `catch buf` fallback so
`std.fmt.bufPrint` errors propagate. Then update the call sites in `record` and
`lookup` to handle the error (propagate it upward or return a meaningful error)
instead of relying on a bogus key. Reference `std.fmt.bufPrint`, `key`, and the
`record`/`lookup` functions when making these changes so the formatting failure
is surfaced and handled properly.

- record() sweeps expired entries when the cap is hit and refuses the
  insert if still full, so memory stays bounded for long-lived processes
  hitting many distinct origins.
- key()'s bufPrint cannot overflow given the 256-byte hostname guard;
  catch unreachable instead of returning an undefined slice.

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/http.zig`:
- Around line 2845-2849: The code currently skips recording Alt-Svc when
canTryH3AltSvc() is false, which prevents learning H3 alternatives from HTTP/1.1
or HTTP/2 responses; change the hashHeaderConst("Alt-Svc") handler to call
H3.AltSvc.record(this.url.hostname, this.url.getPortAuto(), header.value)
whenever this.isHTTPS() is true (remove the canTryH3AltSvc() gate) so responses
always update the Alt-Svc cache even if the original request wouldn’t
immediately use H3; keep the existing hostname/port usage as-is to locate the
record call.
🪄 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: fff82ad2-f7b1-4c63-a217-23df1983ced3

📥 Commits

Reviewing files that changed from the base of the PR and between d936557 and d5051c6.

📒 Files selected for processing (1)
  • src/http.zig

Comment thread src/http.zig
Recording an Alt-Svc header shouldn't depend on whether *this* request
was h3-eligible (force_http1, proxy, sendfile, etc.) — the header is
authoritative for the origin and a future request may be eligible.
h3AltSvcEnabled() now checks only the feature flag; canTryH3AltSvc()
adds the per-request shape constraints and is used only on the lookup
side in start_().
@Jarred-Sumner Jarred-Sumner merged commit 189c1f0 into main Apr 28, 2026
69 of 70 checks passed
@Jarred-Sumner Jarred-Sumner deleted the claude/split-h3-client branch April 28, 2026 22:11
robobun added a commit that referenced this pull request May 1, 2026
Build #49698 failed on debian-13-x64-asan-test-bun with
AtomStringImpl::remove() assertion in fetch-http2-client.test.ts —
a pre-existing ASAN flake unrelated to these MySQL changes
(recent deflake PRs #29954, #29905, #29863, #29809 target the same test).

Empty commit to retrigger CI.
xhjkl pushed a commit to xhjkl/bun that referenced this pull request May 14, 2026
…#29863)

Two related changes to the HTTP/3 client.

### 1. Split `H3Client.zig` into `h3_client/` (no functional change)

Mirrors the `h2_client/` layout so the file shape matches across the two
multiplexed transports:

```
H3Client.zig            - thin entry point: re-exports + live counters
h3_client/
  Stream.zig            - one in-flight request
  ClientSession.zig     - one QUIC connection (pool entry, deliver loop)
  ClientContext.zig     - process-global lsquic engine + session registry
  encode.zig            - request header/body framing onto a quic.Stream
  callbacks.zig         - lsquic -> Zig glue (on_hsk_done / on_stream_*)
  PendingConnect.zig    - DNS-pending connect resolution (moved from h3/)
```

### 2. `--experimental-http3-fetch` /
`BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP3_CLIENT`

Mirrors the `--experimental-http2-fetch` pair. When enabled, `fetch()`
records `Alt-Svc: h3` from HTTPS responses into a small
HTTP-thread-local origin cache (`h3_client/AltSvc.zig`, RFC 7838) and
routes the next request to that origin over QUIC instead of TCP.

- `AltSvc.parse()` — field-value parser returning `{port, ma}` for the
first same-host `h3` alternative, or `error.Clear` for the `clear`
token.
- `AltSvc.record/lookup()` — origin-keyed cache with `ma`-based expiry.
- `canTryH3AltSvc()` gate matching `canOfferH2()`; consulted in
`start_()` before opening TCP and in `handleResponseMetadata` when
reading the header.
- `fetchH3Internals.liveCounts` exposed via `bun:internal-for-testing`
so tests can prove the second fetch actually opened a QUIC session.

Tested locally:
- `fetch-http3-client.test.ts` — 48 pass (incl. 3 new Alt-Svc tests
covering env var, CLI flag, and off-by-default)
- `fetch-http3-adversarial.test.ts` — 26 pass
- `serve-http3.test.ts` — 44 pass
- `zig:check-all` — all targets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants