Conversation
jesup
requested review from
KershawChang,
larseggert,
martinthomson and
mxinden
as code owners
September 14, 2026 02:23
This was referenced Sep 14, 2026
This was referenced Sep 14, 2026
Member
Author
|
This PR is part of a stack of 14 bookmarks:
Created with jj-stack |
This was referenced Sep 14, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The deadline comparison does not match the queue’s strict expiry predicate.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Ensures expired WebTransport datagrams trigger HTTP/3 server connection processing and outcome accounting.
Changes:
- Detects due datagram expiry during server processing.
- Adds a test-only enqueue path and regression test.
File summaries
| File | Description |
|---|---|
neqo-http3/src/connection_server.rs |
Adds expired-datagram processing detection. |
neqo-http3/src/server.rs |
Supplies connection and time context. |
neqo-http3/src/webtransport.rs |
Adds a test-only datagram helper. |
neqo-http3/src/features/extended_connect/tests/webtransport/datagrams.rs |
Tests expiry as the sole pending work. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## users/jesup/watermark_priority_eviction #3988 +/- ##
===========================================================================
- Coverage 96.89% 96.89% -0.01%
===========================================================================
Files 119 119
Lines 41385 41411 +26
Branches 41385 41411 +26
===========================================================================
+ Hits 40099 40124 +25
- Misses 1260 1263 +3
+ Partials 26 24 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
jesup
force-pushed
the
users/jesup/watermark_priority_eviction
branch
from
September 14, 2026 05:31
af88cc2 to
288713f
Compare
jesup
force-pushed
the
users/jesup/process_when_only_a_datagram
branch
from
September 14, 2026 05:31
f4bbb65 to
d2a658d
Compare
jesup
force-pushed
the
users/jesup/watermark_priority_eviction
branch
from
September 14, 2026 17:56
288713f to
2053125
Compare
jesup
force-pushed
the
users/jesup/process_when_only_a_datagram
branch
from
September 14, 2026 17:56
d2a658d to
7057d30
Compare
jesup
force-pushed
the
users/jesup/watermark_priority_eviction
branch
from
September 16, 2026 03:45
2053125 to
3c0dfc6
Compare
jesup
force-pushed
the
users/jesup/process_when_only_a_datagram
branch
from
September 16, 2026 03:45
7057d30 to
e16793a
Compare
jesup
force-pushed
the
users/jesup/watermark_priority_eviction
branch
from
September 16, 2026 17:19
3c0dfc6 to
5c377c8
Compare
jesup
force-pushed
the
users/jesup/process_when_only_a_datagram
branch
from
September 16, 2026 17:19
e16793a to
29d2f7b
Compare
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
left a comment
mxinden
left a comment
Member
There was a problem hiding this comment.
I don't understand the need for this patch. Can you not react on OutgoingDatagramOutcome from neqo-transport?
jesup
force-pushed
the
users/jesup/watermark_priority_eviction
branch
from
September 20, 2026 19:34
026d19e to
bab27a4
Compare
jesup
force-pushed
the
users/jesup/process_when_only_a_datagram
branch
from
September 20, 2026 19:34
3d62c9b to
07ad55d
Compare
This was referenced Sep 20, 2026
Contributor
Performance profiles for profiler.firefox.comBenchmarks (14)
|
Contributor
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Contributor
Benchmark resultsNo significant performance differences relative to bab27a4. All resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: Change within noise threshold. time: [131.64 ms 131.82 ms 132.02 ms]
thrpt: [757.48 MiB/s 758.60 MiB/s 759.63 MiB/s]
change:
time: [-0.9855% -0.7719% -0.5486%] (p = 0.00 < 0.05)
thrpt: [+0.5516% +0.7779% +0.9953%]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1504: Change within noise threshold. time: [213.97 ms 214.26 ms 214.55 ms]
thrpt: [46.610 Kelem/s 46.672 Kelem/s 46.735 Kelem/s]
change:
time: [+0.2214% +0.4363% +0.6554%] (p = 0.00 < 0.05)
thrpt: [-0.6511% -0.4344% -0.2209%]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1504: Change within noise threshold. time: [7.4007 ms 7.4048 ms 7.4090 ms]
thrpt: [134.97 B/s 135.05 B/s 135.12 B/s]
change:
time: [+0.0675% +0.1459% +0.2231%] (p = 0.00 < 0.05)
thrpt: [-0.2226% -0.1457% -0.0675%]
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
2 (2.00%) high mildtransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: No change in performance detected. time: [136.58 ms 136.76 ms 136.98 ms]
thrpt: [730.02 MiB/s 731.23 MiB/s 732.18 MiB/s]
change:
time: [-0.3331% -0.1481% +0.0550%] (p = 0.15 > 0.05)
thrpt: [-0.0550% +0.1483% +0.3342%]
No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severestreams/walltime/1-streams/each-1000-bytes: No change in performance detected. time: [544.70 µs 546.63 µs 548.84 µs]
thrpt: [1.7376 MiB/s 1.7446 MiB/s 1.7508 MiB/s]
change:
time: [-0.4915% +0.0313% +0.5706%] (p = 0.90 > 0.05)
thrpt: [-0.5674% -0.0313% +0.4939%]
No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
1 (1.00%) high mild
11 (11.00%) high severestreams/walltime/1000-streams/each-1-bytes: No change in performance detected. time: [10.480 ms 10.494 ms 10.510 ms]
thrpt: [92.919 KiB/s 93.055 KiB/s 93.187 KiB/s]
change:
time: [-0.2921% -0.0726% +0.1285%] (p = 0.49 > 0.05)
thrpt: [-0.1283% +0.0727% +0.2930%]
No change in performance detected.streams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [34.129 ms 34.164 ms 34.199 ms]
thrpt: [27.886 MiB/s 27.915 MiB/s 27.944 MiB/s]
change:
time: [+1.0731% +1.2256% +1.3863%] (p = 0.00 < 0.05)
thrpt: [-1.3674% -1.2107% -1.0617%]
Change within noise threshold.streams-flow-controlled/walltime/1-streams/each-4194304-bytes: Change within noise threshold. time: [25.860 ms 25.892 ms 25.924 ms]
thrpt: [154.29 MiB/s 154.49 MiB/s 154.68 MiB/s]
change:
time: [+1.5826% +1.7700% +1.9548%] (p = 0.00 < 0.05)
thrpt: [-1.9174% -1.7392% -1.5580%]
Change within noise threshold.streams-flow-controlled/walltime/10-streams/each-1048576-bytes: Change within noise threshold. time: [71.571 ms 71.649 ms 71.728 ms]
thrpt: [139.42 MiB/s 139.57 MiB/s 139.72 MiB/s]
change:
time: [+1.0602% +1.2188% +1.3824%] (p = 0.00 < 0.05)
thrpt: [-1.3635% -1.2041% -1.0491%]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildtransfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [17.513 ms 17.526 ms 17.538 ms]
thrpt: [228.07 MiB/s 228.24 MiB/s 228.40 MiB/s]
change:
time: [+0.6748% +0.7704% +0.8709%] (p = 0.00 < 0.05)
thrpt: [-0.8634% -0.7645% -0.6702%]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildtransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [17.799 ms 17.810 ms 17.820 ms]
thrpt: [224.46 MiB/s 224.60 MiB/s 224.73 MiB/s]
change:
time: [+0.0190% +0.1157% +0.2119%] (p = 0.02 < 0.05)
thrpt: [-0.2114% -0.1155% -0.0190%]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [17.442 ms 17.455 ms 17.467 ms]
thrpt: [229.00 MiB/s 229.16 MiB/s 229.33 MiB/s]
change:
time: [+0.3058% +0.3880% +0.4811%] (p = 0.00 < 0.05)
thrpt: [-0.4788% -0.3865% -0.3049%]
Change within noise threshold.transfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [17.968 ms 17.978 ms 17.988 ms]
thrpt: [222.37 MiB/s 222.49 MiB/s 222.62 MiB/s]
change:
time: [+1.2666% +1.3460% +1.4251%] (p = 0.00 < 0.05)
thrpt: [-1.4051% -1.3281% -1.2508%]
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) low mild
1 (1.00%) high mildInstructions per cycleCriterion reported no significant timing changes. All benchmarks
Download data for |
Contributor
Client/server transfer resultsPerformance differences relative to bab27a4. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
DatagramQueue holds one session's outgoing datagrams: round-robin between send groups (equal turns, not equal bytes), highest send_order first within a group, max-age expiry inclusive at timestamp + max_age, and a byte budget that evicts the lowest-priority datagram rather than the newest. enqueue reports Ok/AboveWatermark/Rejected/Overflowed, and anything but Ok arms the resume signal resume_if_unblocked reports once the queue drains back below the high water mark - by a send or by expiry. DatagramQueue is exported only so this commit's lib target has a live user; the next one, which gives QuicDatagrams a queue per session, takes the export back out.
New Connection methods: enqueue_datagram, set_datagram_high_water_mark, set_datagram_max_age, datagram_queue_capacity, drop_session_datagrams. QuicDatagrams gains a BTreeMap<StreamId, DatagramQueue> and round-robins across sessions in write_frames one datagram at a time, so a datagram that doesn't fit is left queued (with its original timestamp) rather than round-tripped back through enqueue. Hooks the datagram-expiry deadline into next_delay and expiry itself into process_timer, the same way every other subsystem's timer works. Expiry - on the timer or from a shrunken max age - also fires OutgoingDatagramSpaceAvailable for any queue it unblocks: shedding by age is how one of these queues is expected to drain, and nothing else would ever revisit a queue that emptied without sending. The caller, not this layer, has to keep what it enqueues within the peer's max_datagram_frame_size: only the path MTU is applied at packet-build time. send_datagram is the checked path. QuicDatagrams also keeps a simpler count-bounded queue (send_datagram/remaining_datagram_queue_capacity) for callers that don't need per-session tracking, priority, or age.
…unts process_timer's own sweep already expires every session's queue on its own schedule but discards the result. This gives a caller (the HTTP3 layer) a way to run that same sweep on demand and get the expired IDs back, to report a per-datagram outcome. expire_session_datagrams is the same thing for one session. A caller holding a single session cannot tell which of a connection-wide sweep's IDs were its own, and a connection can carry several extended-CONNECT sessions at once.
Send datagrams through the new per-session queue instead of the legacy connection-wide FIFO, for both WebTransport and connect-udp, so send_datagram reports real backpressure and each session's queue is expired and torn down along with it. Tests cover send-order priority and byte-budget eviction through a live WebTransport connection, and that the queue's resume signal still reaches Http3ServerEvent::OutgoingDatagramSpaceAvailable. connect-udp has no outgoingHighWaterMark equivalent, so its old resume-signal test is renamed to what it now checks.
Connection::send_datagram, QuicDatagrams::add_datagram/datagrams/ max_queued_outgoing_datagrams, and ConnectionParameters:: outgoing_datagram_queue only ever had one production caller, Session::send_datagram, and that switched to the per-session enqueue_datagram queue in the previous commit. Firefox's WebTransport and MASQUE connect-udp stacks are the only two QUIC-datagram users, and both now go exclusively through the per-session path, so the plain, count-bounded FIFO has no caller left. connection/tests/datagram.rs: dropped the tests exercising the count-bounded queue's own backpressure contract (Ok(true)/Ok(false), blocking/resuming on a fixed slot count) - the per-session queue's own byte-budget/high-water-mark backpressure already has equivalent coverage further down in this file. Ported the tests exercising protocol-level mechanics that have no other coverage (frame encoding, ack/loss recovery tokens, stream/datagram priority interleaving, MTU-drop, packet-fill-gap edge cases) to enqueue_datagram instead. Requested by mxinden on PR 3984.
Implements Protocol::record_expired_outgoing_datagrams on the WebTransport
session, so SessionStats::datagrams_expired_outgoing actually increments;
the field existed but was hardcoded to stay zero, per its own doc comment
("populated once datagram expiry is wired up").
The test that checks it is also the first caller of the three test-only
ServerSession accessors added with the queues, so their #[expect(dead_code)]
markers come off here.
The sweep that produces those expiries runs from process_http3 on both the
client and server, added in the previous commit alongside the queues it
sweeps.
Connection::next_datagram_expiry() is a live query onto the same queue next_delay/process_timer already read, exposed so a caller can ask 'is anything due right now' independently of process_output's own callback-driven schedule. Http3ServerHandler::should_be_processed uses it: without this check, a server connection with nothing else pending never gets process_http3 called, so its per-session expiry sweep (stats, and any future outcome reporting) never runs for a datagram that goes stale with nothing else happening on the connection. process_timer still expires it either way - nothing gets sent late - but the http3-level bookkeeping for it was silently lost. Gated on the session being active, since a connection that has stopped being processed can't refresh a stale deadline, and an unguarded check would then return true forever. The client has no equivalent gap: Http3Client::process_http3 already runs unconditionally on every process_output call.
jesup
force-pushed
the
users/jesup/process_when_only_a_datagram
branch
from
September 23, 2026 06:27
07ad55d to
49a9494
Compare
jesup
changed the base branch from
users/jesup/watermark_priority_eviction
to
users/jesup/expire_datagrams
September 23, 2026 06:27
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
September 23, 2026 23:55
bcd4abd to
325e0d2
Compare
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
6 times, most recently
from
September 29, 2026 14:51
a0174a1 to
40e8b3a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.