Skip to content

node:quic: report a datagram's status under the id kept in its lsquic frame - #44680

Open
robobun wants to merge 6 commits into
mainfrom
robobun/a366d935/quic-datagram-status-by-id
Open

robobun wants to merge 6 commits into
mainfrom
robobun/a366d935/quic-datagram-status-by-id

Conversation

@robobun

@robobun robobun commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Known limitation of node:quic on lsquic — Node v26 compat, HTTP/3 #32602: ondatagramstatus(id, status) names the wrong id when a datagram is lost ahead of others. The network loses 1 of 6: bun reports 1 acknowledged and 2 lost.
  • lsquic reports (count, acked) for a packet, and QuicSession pops that many ids from a send-order queue (inflight_datagrams, src/runtime/node/quic/session.rs:1298).
  • A datagram with no report shifts every later id: a rejected 0-RTT datagram, or one that lsquic frames and then drops.

Fix

  • on_dg_write names the frame with the new lsquic_conn_set_datagram_id(). lsquic keeps the id in the frame record and reports (id, status) when the packet is acknowledged, lost, or dropped before the send ('lost').
  • Correct because the id stays with the frame, as dgram_id does in ngtcp2 under Node. QuicSession keeps no id queue.
  • Verified: test/js/node/quic/quic-datagram.test.ts (8 of 10 fail on main), the 235 vendored test-quic-* tests, the HTTP/3 suites. Self-reviewed: 21 concerns raised, 19 addressed. Not taken: a destroyed check for two other loops (not reproduced, separate change).

Background

  • A QUIC DATAGRAM frame (RFC 9221) is never retransmitted. lsquic keeps a record per frame of an outgoing packet, with a pointer-sized slot that DATAGRAM frames left unused.
  • Considered the packet-number map that node:quic on lsquic — Node v26 compat, HTTP/3 #32602 proposed: a second book that a dropped packet leaves stale.

Downsides

  • A datagram dropped before the send is 'lost'. Node keeps it queued. An app that resends on 'lost' got about 1000 callbacks in a 3 s outage (86 on main).
  • The last datagram of a quiet session still gets no status.
Notes

Other changes a user can see

  • stats.datagramsSent counts a datagram when lsquic frames it. On main the count came one event-loop pass later.
  • Each id that datagramsSent counts ends in datagramsAcknowledged or datagramsLost on a session that continues. On main a datagram that lsquic dropped before the send was counted in neither.
  • An ondatagramstatus that destroys the session (a throw does) ends the statuses of that pass. On main each further datagram of the same packet raised an uncaught ERR_INTERNAL_ASSERTION ("Unexpected datagram status event").
  • A datagram of a rejected 0-RTT flight gets no status, as in Node. The ids after it now keep their own statuses.
  • The diagnostics channel quic.session.receive.datagram.status gets the same corrected pairs.

The lsquic change

patches/lsquic/node-quic-accessors.patch is a patch file, so its diff here is a diff of a diff. This is what it changes in lsquic, condensed:

/* include/lsquic.h */
-    void (*on_datagram_status)(lsquic_conn_t *c, unsigned count, int acked);
+    void (*on_datagram_status)(lsquic_conn_t *c, uint64_t dgram_id, int status);
+void lsquic_conn_set_datagram_id (lsquic_conn_t *, uint64_t dgram_id);  /* only inside on_dg_write */
+enum lsquic_dg_status { LSQ_DG_ACKED, LSQ_DG_LOST, LSQ_DG_UNSENT };

/* lsquic_full_conn_ietf.c: struct ietf_full_conn */
+    uint64_t                    ifc_dg_id;
-    int                         ifc_ping_us_override_set;  /* also ifc_datagram_early, ifc_abort_silent */
+    unsigned char               ifc_ping_us_override_set;

/* lsquic_full_conn_ietf.c: write_datagram() */
+    conn->ifc_dg_id = 0;
     w = pf_gen_datagram_frame(..., on_dg_write, &conn->ifc_conn);
-    lsquic_packet_out_add_frame(packet_out, mm, 0, QUIC_FRAME_DATAGRAM, ...);
+    lsquic_packet_out_add_frame(packet_out, mm, conn->ifc_dg_id, QUIC_FRAME_DATAGRAM, ...);

/* lsquic_send_ctl.c */
+static void
+send_ctl_report_datagrams (struct lsquic_send_ctl *ctl,
+        struct lsquic_packet_out *packet_out, enum lsquic_dg_status status)
+{
+    struct packet_out_frec_iter pofi;
+    struct frame_rec *frec;
+
+    if (!(packet_out->po_frame_types & QUIC_FTBIT_DATAGRAM)
+            || !ctl->sc_enpub->enp_stream_if->on_datagram_status)
+        return;
+
+    for (frec = lsquic_pofi_first(&pofi, packet_out); frec;
+                                                frec = lsquic_pofi_next(&pofi))
+        if (QUIC_FRAME_DATAGRAM == frec->fe_frame_type)
+            ctl->sc_enpub->enp_stream_if->on_datagram_status(
+                ctl->sc_conn_pub->lconn, frec->fe_u.data, status);
+}
 send_ctl_handle_regular_lost_packet():  LSQ_DG_LOST    (replaces the count report)
 lsquic_send_ctl_got_ack():              LSQ_DG_ACKED   (replaces the count report)
 lsquic_send_ctl_next_packet_to_send():  LSQ_DG_UNSENT  (new, in the PO_REPACKNO block)
 lsquic_send_ctl_squeeze_sched():        LSQ_DG_UNSENT  (new, in the drop arm)
-lsquic_packet_out_count_datagram_frames()                (removed)
  • The two LSQ_DG_UNSENT sites are the places where lsquic discards a DATAGRAM frame that it framed and did not send. After a retransmission timeout lsquic sends at most two packets until an ACK arrives. It drops a waiting packet that holds only datagrams (lsquic_send_ctl_squeeze_sched). It cuts the DATAGRAM frame out of a waiting packet that it gives a new number (PO_REPACKNO).
  • No report is made for a rejected 0-RTT flight, for lsquic_send_ctl_drop_scheduled, or at connection cleanup. Node reports nothing there.
  • The patch file was regenerated from the patched tree. The later patches (coalesce-batch-drop, connection-close-pns, goaway-accept-uni-streams) apply on top of it unchanged.

Designs that were weighed

  • The frame record with only the two existing report sites (acknowledged, lost). It fixes the report in the title. It leaves 9 of 13 ids without a status in the outage probe below, and datagramsSent stays 13 against 3 acknowledged and 1 lost.
  • A map from packet number to ids in session.rs (the idea in node:quic on lsquic — Node v26 compat, HTTP/3 #32602). It is a second book beside the packet, and a packet that lsquic drops or renumbers leaves it stale.
  • One report site in send_ctl_destroy_packet. It adds a bit test for each destroyed packet on the ACK path of every connection, and it fires at close.
  • Not to frame a datagram that lsquic cannot send (Node's behaviour). This is the better end state and a larger lsquic change. It is listed under follow-ups.

Measurements

Release builds of the merge base 13a98b0 and of this branch, linux x64. perf, valgrind, strace and bloaty are not installed on this machine, so the instruction counts are static counts from objdump.

  • struct ietf_full_conn, release: 10232 -> 10232 bytes (the calloc immediate in lsquic_ietf_full_conn_client_new and _server_new). Debug: 10280 -> 10280. Bun.serve HTTP/3 and fetch use this struct too.
  • Instructions, main -> PR: lsquic_send_ctl_sent_packet 134 -> 134, squeeze_sched 129 -> 224, next_packet_to_send 266 -> 362, got_ack 963 -> 1001, write_datagram 99 -> 99. Each added instruction is behind the QUIC_FTBIT_DATAGRAM test.
  • Release text: 80,732,991 -> 80,732,767 bytes (-224, size). lsquic_send_ctl_got_ack 4123 -> 4367 bytes, write_datagram 405 -> 418 bytes.
  • QuicSession 880 -> 848 bytes. SessionEvent 136 -> 136 bytes.
  • Session events for 1000 datagrams sent 5 ms apart on a lossless path: 2000 -> 930. Id queue operations: 2000 -> 0. Burst of 2000 datagrams: 2000 sent events and 1013 status events -> 0 and 516.
  • Entries into the two new report sites in a lossless Bun.serve HTTP/3 run of 2,247 sent packets: 0.
  • Allocations for a datagram: none added (read from the code: the id uses a slot that exists). UDP packets for 1000 lossless datagrams: 1057 to 1061 -> 1055 to 1060. Outage probe: 4 -> 4 datagram packets on the wire.
  • Outage probe (13 datagrams around a retransmission timeout): ids without a status 9 -> 0, ids that contradict the server 2 -> 0, datagramsSent/Acknowledged/Lost 13/3/1 -> 13/3/10, in 3 of 3 runs.
  • Loss of datagram 1 of 6: 2 of 6 ids wrong -> 0.
  • An app that resends each datagram that is reported 'lost', in a 3 s outage. With one chain of resends: 2 -> 11 to 13 'lost' callbacks. With its own datagram each 10 ms too: 85 or 86 -> 805 to 1040 (5 runs). The count depends on timing: an earlier build of this branch gave 466 to 500. The packets on the wire are the same (88 to 90 datagram packets).

Tests

test/js/node/quic/quic-datagram.test.ts puts a UDP relay between a client and a server in one process. The relay loses or keeps packets that it selects by length. This is the loss-injection harness that #32602 said the fix needs.

test main PR
loss of datagram 1 of 6 under its id fail pass
loss of datagram 3 of 8 under its id fail pass
no loss: all acknowledged pass pass
three datagrams in one lost packet fail pass
an acknowledged datagram and the lost one after it stay apart pass pass
dropped before the send is counted as lost (with a listener) fail pass
dropped before the send is counted as lost (no listener) fail pass
cut out of a packet before the send fail pass
a callback that destroys the session ends the statuses fail pass
rejected 0-RTT datagram does not move later ids fail pass
  • The fifth test pins the new rule that joins consecutive ids into one event only when the status is the same. Without that rule it fails in 6 of 6 runs, and in 15 of 15 runs on a loaded machine.
  • Each failure on main takes less than 0.4 s. It is an assertion diff, or in the ninth test the uncaught ERR_INTERNAL_ASSERTION. No test fails by a timeout.
  • Runs of the whole file on this branch: release 100 of 100, and 60 of 60 with 24 busy loops on 12 cores. Debug (ASAN) 20 of 20, and 15 of 15 with the same load. The loaded runs use the per-test timeouts of CI (90 s, and 270 s on ASAN). With the default 5 s, the loaded debug build times out while it loads node:quic. The later commits change only the test file: how the tests fail, how they release what they open, and their types. After them: debug 12 of 12, and 10 of 10 with the load. The release runs are from before them.
  • A session that fails in the middle of a test rejects the wait that the test is in. Checked with a copy of the file that destroys the client or the server session: the test fails in about 0.55 s with that error. A handshake that fails releases the endpoints, the relay sockets and the sessions (checked with an ALPN mismatch: the test fails in less than 0.5 s with the transport error).
  • The file passes the type check of test/ (bun check js/node/quic/quic-datagram.test.ts in test/: no type errors).
  • The three "before the send" tests need a retransmission timeout of lsquic, which is a wall-clock timer of 200 ms or more. Their assertions hold on each path that lsquic can take, and no wait in them depends on the path. Whether they reach the two new report sites depends on timing: lsquic sends a NEW_CONNECTION_ID frame each second in the first seconds of a connection, and a slow run can leave the client with other retransmissions that hold back new frames. A stricter copy of the cut-out test, which fails if the datagram is not cut, passes 30 of 30 on the release build (idle and loaded), 20 of 20 on the debug build, and 13 of 20 on the debug build under the load above. gdb breakpoints on an idle machine: 2 hits at the drop site in each "dropped before the send" test, 1 hit at the renumber site in the cut-out test.
  • Other suites on this branch: the 235 vendored test/js/node/test/parallel/test-quic-*.mjs (same result on both release builds: 233 pass, test-quic-stream-idle-timeout.mjs and test-quic-h3-stream-idle-timeout.mjs fail on both), test/js/node/quic/ (30 pass, debug), serve-http3.test.ts (72 pass, 1 skip), fetch-http3-client.test.ts (65), fetch-http3-adversarial.test.ts (27), fetch-http3-cold-post.test.ts (1 pass, 1 skip).
  • Not run: macOS and Windows. The change has no platform-specific code.

Cases that stay as they are

  • lsquic arms no loss timer for a packet that holds only DATAGRAM frames. A lost datagram gets its status when a later packet of the session is acknowledged. The last datagram of a quiet session gets none. Same on main.
  • The retransmission timer of lsquic declares packets lost. It does not probe. A datagram that arrived can be reported 'lost' when its ACK is late. Same on main.
  • lsquic never reports an acknowledgement after a loss. Node can report both for one id.
  • 'abandoned' changes no counter. Node counts it in datagramsLost.

Follow-ups (not in this PR)

  • lsquic: keep a framed DATAGRAM packet that cannot leave yet, and do not drop it. Then LSQ_DG_UNSENT and the resend cycle above go away, and bun matches Node.
  • lsquic: a loss timer for packets that hold only DATAGRAM frames.
  • One dead peer stalls the datagram sends of every session on an endpoint. udp: stop setting IP_RECVERR on UDP sockets #35922 touches this path.
  • The session-ticket loop and the new-session loop have no destroyed check of their own. A stray callback from the ticket loop did not reproduce: the second ticket of a connection is delivered 500 ms after the first, in a pass that has the check. These are the two self-review concerns that this PR does not take.
  • maxDatagramSendAttempts is validated in quic.ts and read nowhere.

Open PRs that touch the same lines

… frame

`ondatagramstatus(id, status)` gave a status to the wrong id as soon as
a packet was declared lost ahead of acknowledged ones. lsquic reported
`(count, acked)` for a packet, and the session popped that many ids
from a send-order queue.

`on_dg_write` now names the frame with `lsquic_conn_set_datagram_id`.
`write_datagram` stores the id in the DATAGRAM frame record. The send
controller reports each frame as `(id, status)` when its packet is
acknowledged, when its packet is declared lost, and when lsquic drops
the frame before the send. The session reports a frame that was dropped
before the send as 'lost'.

The session keeps no queue of in-flight ids and no `sent` event for
each datagram.
@robobun

robobun commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

  • Reproduced: a UDP relay between a node:quic client and a server loses the packet of datagram 1 of 6. On main the client gets 1 acknowledged and 2 lost (3 of 3 runs). With the loss at datagram 6, or with no loss, each status is correct.
  • Fix: node:quic: report a datagram's status under the id kept in its lsquic frame #44680. test/js/node/quic/quic-datagram.test.ts holds the repro as a test.
  • CI: the diff is green. The build is red from two jobs that did not run a test. In build 127946, 179 jobs passed. The 2 failed jobs are darwin any aarch64 - test-bun, and both stop before the first test: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
  • The two jobs ran on the agents darwin-aarch64-26.6.1-1 and darwin-aarch64-26.6.2-1. In 14 recent builds of main these two agents failed 7 of their 14 test-bun jobs. The other macOS arm64 agents failed 0 of 42.
  • Builds 127935 and 126967 failed in the same way on the same agents. No test under test/js/node/quic/ failed on a lane. Each test that CI flagged passed on its retry.
  • The one CI retrigger is used. The PR needs a maintainer to merge it, or to retry the two macOS jobs.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e7977b12-4cca-4ca7-aa0f-fb50aabf3ec8
📥 Commits

Reviewing files that changed from the base of the PR and between 8e451fb and 44ae42c.

📒 Files selected for processing: 1
  • test/js/node/quic/quic-datagram.test.ts

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

Walkthrough

The LSQUIC interface gains connection, stream, and HTTP/3 controls. Datagram status callbacks now report IDs and statuses, and the QUIC runtime dispatches per-ID outcomes. New tests cover acknowledgements, loss, abandonment, and rejected 0-RTT data.

Changes

QUIC API and Datagram Status Updates

Layer / File(s) Summary
LSQUIC callbacks, settings, and APIs
patches/lsquic/node-quic-accessors.patch
The LSQUIC interface adds callbacks, settings, queries, and controls for connection, stream, PING, HTTP/3, and datagram behavior.
Connection lifecycle and transport controls
patches/lsquic/node-quic-accessors.patch
Connection handling changes include configurable idle timeouts and PING periods, preferred-address opt-in, stream creation when credit becomes available, and additional reset and close controls.
HTTP/3 and handshake handling
patches/lsquic/node-quic-accessors.patch
HTTP/3 SETTINGS and ORIGIN handling is extended. Optional callbacks expose handshake confirmation, path changes, and early-data failure; stream state records received early data.
Datagram IDs and status delivery
packages/bun-usockets/src/node_quic_shim.c, patches/lsquic/node-quic-accessors.patch, src/lsquic_sys/lib.rs, src/runtime/node/quic/session.rs, test/js/node/quic/quic-datagram.test.ts
LSQUIC reports each DATAGRAM frame’s ID and status. The Rust bindings and QUIC session pass per-ID outcomes to callbacks, and tests cover acknowledgements, loss, abandonment, callback errors, and rejected 0-RTT data.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 44ae4

No actionable issue remains from this review; the change is mergeable after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
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.
Title check Passed The title clearly and concisely describes the primary change: reporting QUIC datagram status using the ID stored in the lsquic frame.
Description check Passed The description explains the problem, fix, behavior changes, limitations, design choices, and verification results. It does not use the exact template headings, but it provides the required content in…
Full details: Description check

Explanation

The description explains the problem, fix, behavior changes, limitations, design choices, and verification results. It does not use the exact template headings, but it provides the required content in equivalent sections.

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

One part of this review's analysis that stopped early was run once more. It finished.

Comment thread src/runtime/node/quic/session.rs Resolved
Comment thread test/js/node/quic/quic-datagram.test.ts Resolved
…n fails

Each wait of the relay harness now races the `closed` promise and the
`onerror` event of both sessions. A session that fails rejects the wait,
and the test does not run into its timeout. The teardown destroys the
server session, so a client that failed does not leave it to the idle
timeout.

@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 nit, two more things were checked and ruled out: a lost packet that lsquic reschedules does not report its datagram 'lost' a second time through the new PO_REPACKNO site (the DATAGRAM frame sits in po_regen_sz and is chopped by update_for_resending before the packet is scheduled again), and on_datagram_status writing to session.events directly skips nothing (push_event is a plain push, and the drain pops from the front, so coalescing on the last event cannot touch an event being dispatched). 7426614 wires every relay-harness wait to reject on session failure or close.

Extended reasoning...

The change reworks per-datagram status reporting across the lsquic patch (frame-record id, three report sites in lsquic_send_ctl.c), the C shim and Rust FFI vtable signature, and the QuicSession event path, with a new test file; it touches no auth or injection surface but does touch native callback lifecycles and a vendored protocol library. A confirmed test-cleanup nit is posted inline, and earlier optional concerns about mapping LSQ_DG_UNSENT to 'lost' remain a design choice the author acknowledged in the description, so a human look at the lsquic semantics is still worthwhile rather than approval.

Still open from earlier reviews (2):

  • Unresolved: 2 minor or pre-existing.

Comment thread test/js/node/quic/quic-datagram.test.ts Resolved
@robobun

robobun commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:46 AM PT - Oct 18th, 2026

❌ @robobun, your commit 0840e16 has 1 failures in Build #127946 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 44680

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

bun-44680 --bun

…ests when a step fails

The relay harness and the 0-RTT test now keep what they open in an
`AsyncDisposableStack`. A handshake that fails releases the listening
endpoint, the relay sockets and the sessions, and the test fails with
the cause.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

`cd test && bun check` reports no type error on main. The new file
passes it too: `server.address!`, a `SocketAddress` for the relay, and
`as SessionOptions` for `verifyPeer`, as in the other files of this
directory.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants