[robobun]

Problem

• In a node:quic server, session.path read before session.opened caches two empty objects, not net.SocketAddress. A quic.session.created.server subscriber gets the same empty address and primes that cache.
• QuicSession::create read the addresses only from an lsquic connection (src/runtime/node/quic/session.rs:536). A session announced at the first packet (endpoint.rs:1726) and a version negotiation probe (endpoint.rs:2456) have none.
• onnewtoken(token, address) and quic.session.new.token got the same empty address: the dispatch passed only the token (session.rs:1093).

Fix

• create requires the peer address, like application since Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021, and reads the local address from its endpoint. The NewToken dispatch passes the same address (first commit).
• cache_sockaddrs, the later read from lsquic, is deleted. 721 of 721 such reads in the quic tests gave the same values.
• Verified: test/js/node/quic/quic-endpoint.test.ts (7 new tests, 5 fail on main), test/js/node/quic/, the 235 vendored test-quic-*.mjs.
• Self-reviewed: 11 concerns raised, 8 addressed in full. Not done: tracking issues, and three gaps outside session addresses.

Background

• Node 26.3.0, which Bun follows, announces a server session at the first Initial packet, before any lsquic connection exists.
• Node's Session::Config takes both addresses (session.cc:813-819). EmitNewToken passes remote_address() (session.cc:3694-3712). Source only: this Node has no QUIC.
• Considered writing both fields at the two callers with no connection. A fifth caller could skip it.

Downsides

• Before opened, session.path.remote is the unverified source of the first datagram. Bun sends no Retry, so a sender can forge it.
• An onnewtoken listener costs one more SocketAddress per client session (63 against 43 over 20). Release text: +512 B.
• After a migration, session.path still reports the creation-time addresses.

Notes

Repro

Run in the repo root (the fixture keys resolve from there):

import { listen, connect } from "node:quic";
import { createPrivateKey } from "node:crypto";
import { readFileSync } from "node:fs";
const dir = "test/js/node/test/fixtures/keys/";
const key = createPrivateKey(readFileSync(dir + "agent1-key.pem"));
const cert = readFileSync(dir + "agent1-cert.pem");
const done = Promise.withResolvers();
const server = await listen(async session => {
  const early = session.path;
  console.log("onsession:", early.remote.address, early.remote.port, early.local.address);
  await session.opened;
  console.log("opened:", session.path.remote.address, session.path === early);
  await session.close();
  done.resolve();
}, { sni: { "*": { keys: [key], certs: [cert] } }, alpn: ["quic-test"] });
const client = await connect(server.address, { alpn: "quic-test", verifyPeer: "manual" });
await client.opened;
await Promise.all([client.closed, done.promise]);
await server.close();

	onsession:	opened:
bun 1.4.3 and main	undefined undefined undefined	undefined true
this PR	127.0.0.1 <client port> 127.0.0.1	127.0.0.1 true

Found with this repro while reading the code. No user reported it.

The same empty objects show up in three more places on main:

• A diagnostics_channel subscriber on quic.session.created.server gets an empty address. The publish reads session.path, so the application then sees empty addresses after opened too.
• session.path read in onerror of a server session whose handshake fails.
• session.path.local of a client session that connect() opens with a version the server does not support.

The objects are empty and not undefined because bun's InternalSocketAddress shim (src/js/internal/quic/quic.ts:166) returns its argument, and new on a function that returns undefined gives a new empty object. That is also why the vendored test-quic-diagnostics-channel.mjs (ok(msg.address)) passes on main.

The four callers of QuicSession::create

Caller	Session	Addresses on main	Now
maybe_announce_provisional	server, at the first Initial	none until the handshake	the datagram's source, the endpoint's bound address
on_new_conn, no earlier announce	server, at handshake completion	read from the conn	the conn's peer, the endpoint's bound address
connect	client	read from the conn after lsquic_engine_connect	the connect() target, the endpoint's bound address
connect_verneg_probe	client, version negotiation probe	remote only	the connect() target, the endpoint's bound address

The local address is always the endpoint's bound address. That is the address on_data gives lsquic with every packet and connect gives lsquic_engine_connect, so it is what lsquic reported back.

Deleted

• QuicSession::cache_sockaddrs and its three calls (create, bind_conn, connect).
• The hand-written remote_addr.set(remote) in connect_verneg_probe.
• The Option on conn_peer_addr. lsquic_conn_get_sockaddr returns pointers into the conn's current path and has no failure case (vendor/lsquic/src/liblsquic/lsquic_conn.c).
• Cell and pub(super) on the two address fields. Nothing writes them after construction. A fix for the path change below will write them again.

Before the delete, a temporary build on this branch compared each cache_sockaddrs read with the value the constructor stored. 721 of 721 were equal: 591 over the 235 vendored test-quic-*.mjs and 130 over the three test/js/node/quic files. The runs include IPv6 and wildcard binds.

Tests

Seven tests in a new describe("session addresses") block. Each asserts instanceof SocketAddress and the exact address, port and family. An awaited event that does not come resolves to "closed" when the session ends, so the failure is a diff and not a timeout.

Test	main	PR
server session in onsession, then the same cached object after opened	fail	pass
quic.session.created.server message, and session.path after opened	fail	pass
server session in onerror after an ALPN mismatch	fail	pass
version negotiation probe	fail	pass
onnewtoken and quic.session.new.token	fail	pass
client session right after connect()	pass	pass
server session for a resumed 0-RTT connection	pass	pass

The last two guard the two callers that were already correct and now get their addresses from the constructor. A temporary build that passed a wrong address at one caller at a time gave:

Wrong value at	Tests that fail
the peer in on_new_conn (session created at handshake completion)	resumed 0-RTT
the remote in connect	client session, onnewtoken
the remote in the version negotiation probe	probe
the peer at the first Initial	onsession, channel message, failed handshake
the local address in create	all but onnewtoken

The resumed 0-RTT test reaches the on_new_conn caller because lsquic puts the 0-RTT packet ahead of the Initial in the first datagram, and the announce at the first packet looks only at the first header. In 28 of 28 runs the debug log had one announce provisional line for the two sessions. No assertion in the test tells that caller from the other.

The failed-handshake test covers Node 26.3.0 behaviour. Node 26.6.0 and later announce a server session only after the ClientHello, so a failed handshake never reaches onsession there (nodejs/node#64132). If bun ports that, the test fails with "no server session reported an error".

Results:

• Debug build (ASAN): quic-endpoint.test.ts 16 pass. On a debug build of main, 11 pass and these 5 fail. quic-sni.test.ts 11 pass, quic-stream.test.ts 11 pass.
• Under the ASAN lane's LeakSanitizer settings the file passes with no leak report (2 runs, and 4 runs pinned to one core beside two busy loops). One pinned run timed out in main's Worker test, which takes 22 to 29 s of its 30 s under that load on both builds.
• Vendored test-quic-*.mjs, release builds: 235 of 235 in 5 of 5 rounds on this PR and on main.
• Vendored, debug builds, 3 interleaved rounds: 232, 231, 230 pass here and 231, 231, 233 on main. test-quic-session-initial-rtt.mjs fails every debug run on both (its 10 ms RTT bound). The test-quic-callback-error-* files fail now and then on both.
• cargo clippy -p bun_runtime is clean.
• On CI the file passes on Linux ASAN (16 pass) and on alpine x64, Windows x64, Windows aarch64, macOS aarch64 and macOS x64 (15 pass, and the ASAN-only test of the file is skipped).

Measurements

Release builds of main at 0c864a0, without and with this diff, same toolchain. The vendored runs and the debug comparisons use the same pair. The branch was then rebased onto a later main with no conflict.

• Release binary text: +512 B (107,320,051 to 107,320,563, llvm-size). node::quic symbols: +499 B (93,928 to 94,427, llvm-nm sum).
• lsquic_conn_get_sockaddr calls per client and server connection: 3 to 1 (gdb hit count: 15 to 5 over 5 connections, 60 to 20 over 20).
• QuicSession::create per call: 213 to 222 instructions, 15 to 14 conditional branches, 9 to 9 direct calls, 0 to 0 indirect calls (gdb single-step, server and client call).
• on_data per dispatch on an established connection: 119 + 113 x packets on both builds (232 for 1 packet, 1023 for 8). The dispatch that creates a session: 326 to 328. The disassembly differs only by the 2 instructions that pass the new argument.
• getsockname syscalls for 5 and 20 connections: 4 and 4 on both builds.
• SocketAddress objects after 20 connections: 43 to 43 when nothing reads session.path, 43 to 83 when onsession reads it (heapStats).
• onnewtoken: 1 SocketAddress per client session with a listener (63 against 43 over 20 sessions), 0 without.
• sizeof(QuicSession): 1120 B to 1120 B.
• Host functions: 22 to 22 QuicSessionPrototype__ symbols. quic.classes.ts, src/js and bun-types are not touched.

Not changed here

• The early address is not verified. Bun announces a session at the first datagram that looks like an Initial and does not implement validateAddress. A 7-byte datagram from any UDP socket reaches onsession. Its source was an empty object before this PR and is a SocketAddress now. Node sends a Retry first by default, so the address it shows in onsession has passed address validation. Filter by address in endpoint.blockList, or read the address after opened.
• A path change after creation. on_path_switch does not update the two addresses, so the first session.path read after a migration returns the creation-time pair. Node updates its native addresses there (session.cc:2609-2618). This was the same before this PR.
• connect() does not send options.token yet (see the comment in endpoint.rs at the lsquic_engine_connect call). onnewtoken now reports the right address, but a saved token still has no effect.
• The version negotiation probe does not receive the per-session options.
• The InternalSocketAddress shim still maps undefined to an empty object. After this PR no live session returns undefined for an address.

Self-review

A review of this diff before it was pushed raised 11 concerns. Eight are addressed in full:

• The branch did not apply to main after Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021, and the body described the older tree. It is rebased, and every number here is from the rebased build.
• It stated no order with node:quic: make the session the server connection's context from its first packet #46226.
• The onnewtoken fix is independent. It is now the first commit.
• The body did not name the Node version it follows.
• The resumed 0-RTT test claimed a witness it did not have.
• Nothing had run on a tree that can merge, or under the ASAN lane's LeakSanitizer settings.
• The vendored results were stated for one build type.
• Awaited events in the tests could only time out.

Three are addressed in part. The body now says that no user reported the bug, that the early address is unverified, and what the onnewtoken address does not enable. The review also asked for a tracking issue for each gap under "Not changed here". None is filed with this PR. It also named three gaps outside session addresses. They are not listed here because they were not reproduced for this PR.

Order with other open PRs

• Draft node:quic: make the session the server connection's context from its first packet #46226 replaces the two server callers with one that runs at the first packet, and still fills the addresses with cache_sockaddrs. This PR is meant to land first: it is small and needs no lsquic patch. After it, node:quic: make the session the server connection's context from its first packet #46226 passes the conn's peer address to create and drops its cache_sockaddrs calls, and the resumed 0-RTT test becomes a plain address check. If node:quic: make the session the server connection's context from its first packet #46226 lands first, this PR keeps the create argument, the probe and the NewToken commit.
• node:quic: enforce QuicEndpoint maxConnectionsPerHost #35726 sets remote_addr on the announced session for its per-host count. That line becomes redundant here, and the count needs a getter because the field is now private.

Node references (v26.3.0)

• Session::Impl takes both addresses from its config
• Server config at the first Initial, client config
• Session::EmitNewToken passes remote_address()
• doc/api/quic.md: session.path is { local: net.SocketAddress, remote: net.SocketAddress }. OnNewTokenCallback gets address {SocketAddress}.
• Later Node keeps both of these. It moved only the time of onsession (quic: defer server session emit until TLS ClientHello is processed nodejs/node#64132, in 26.6.0).

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 0c3b0f64-5acd-4cca-8a60-292733116dcd

📥 Commits

Reviewing files that changed from the base of the PR and between 9634520 and 6874567.

📒 Files selected for processing: 3

• src/runtime/node/quic/endpoint.rs
• src/runtime/node/quic/session.rs
• test/js/node/quic/quic-endpoint.test.ts

• 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[robobun]

^{Updated 12:51 PM PT - Oct 21st, 2026}

✅ @robobun, your commit 6874567fe0cfead00a2d540fdea003bf2af01e7c passed in Build #130144! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 46343

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

bun-46343 --bun

[robobun]

Status

Reproduction on bun 1.4.3 and on a release build of main (0c864a0), with the script in the PR notes:

• A listen() callback that reads session.path before session.opened gets two empty objects that are not net.SocketAddress. They stay cached after opened.
• A quic.session.created.server subscriber gets the same empty address. The application's session.path is then empty too.
• connect() with a version the server does not support: session.path.local is empty.
• onnewtoken(token, address): address is empty.
• test/js/node/quic/quic-endpoint.test.ts: 5 of the 7 new tests fail on main. All 16 tests in the file pass with this PR.

PR: #46343

[claude]

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

Beyond the inline finding on post-migration addresses, I also checked that QuicEndpoint::local_addr() cannot hand QuicSession::create the default/unset value: connect and the verneg probe run after ensure_bound, and the two server call sites are reached only from datagrams on an already-bound socket, so the debug_assert! in create is not masking a release-build gap there.

Extended reasoning...

The change makes QuicSession's local/remote addresses immutable constructor inputs across all four create call sites in endpoint.rs, deletes cache_sockaddrs, drops the Option from conn_peer_addr, and adds the peer address as a second argument to onnewtoken, with seven new tests. It touches no auth or crypto surface, but the one confirmed finding (addresses never refreshed after path migration, which the author also lists as a downside) means a human should weigh whether that trade-off is acceptable before merging.