Repository navigation
WebTransport: count a session's expired outgoing datagrams - #3986
Merged
Merged
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 6 bookmarks:
Created with jj-stack |
This was referenced Sep 14, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Wires outgoing WebTransport datagram expiry into HTTP/3 processing and records expiry statistics.
Changes:
- Expire datagram queues during client and server processing.
- Track expired outgoing datagrams in WebTransport stats.
- Add regression coverage and update documentation.
File summaries
| File | Description |
|---|---|
| neqo-http3/src/features/extended_connect/webtransport_session.rs | Updated as part of this pull request. |
| neqo-http3/src/features/extended_connect/tests/webtransport/datagrams.rs | Updated as part of this pull request. |
| neqo-http3/src/features/extended_connect/stats.rs | Updated as part of this pull request. |
| neqo-http3/src/connection_server.rs | Updated as part of this pull request. |
| neqo-http3/src/connection_client.rs | Updated as part of this pull request. |
Review details
Suppressed comments (4)
neqo-http3/src/connection_client.rs:750
- This sweep is inside the active-state arm, so it does not run once the client enters
Http3State::Closing. A peer-initiated close sets the HTTP/3 state toClosingwithout clearing the extended-CONNECT sessions, while the transport'sprocess_timeralso skips datagram expiry during closing; queued stale datagrams can therefore remain until the connection is dropped and are never counted. Run the sweep for every non-closed HTTP/3 tick, as the server path does.
self.base_handler
.expire_datagram_queues(&mut self.conn, now);
neqo-http3/src/connection_client.rs:750
- This activates a shared sweep whose accounting is not actually per session:
Session::expire_datagramscallsconn.expire_datagrams(now), which expires every queue, while this helper invokes it once for each active session. With the multiple-session case already supported bydatagrams_multiple_session, the first session visited consumes both sessions' stale datagrams and records both counts in its ownSessionStats; the other session records zero. Make the transport sweep session-scoped (or return per-session counts) before using this call.
self.base_handler
.expire_datagram_queues(&mut self.conn, now);
neqo-http3/src/connection_server.rs:198
- This sweep is not reached for every server connection at each
Http3Server::process_http3call: the outer server only selectsactive_connections()or handlers whoseshould_be_processed()is true, and that predicate does not include a pending QUIC datagram queue. If a send attempt leaves a datagram queued until its expiry timer, the transport'sConnection::process_timercan remove it before this handler is selected, so the WebTransport expiry statistic stays at zero. Select/sweep connections with a due datagram expiry before transport timer processing.
self.base_handler.expire_datagram_queues(conn, now);
neqo-http3/src/features/extended_connect/webtransport_session.rs:251
- This counter is intended to be per WebTransport session, but the sweep that calls this hook uses
Connection::expire_datagrams, which removes stale entries from every session queue on the connection. With two active WebTransport sessions, whichever session is visited first records all expired datagrams and the other records zero (or the first records datagrams belonging only to the other session). Scope the expiry/counting to the current session, or return per-session counts before updating this field.
fn record_expired_outgoing_datagrams(&mut self, count: u64) {
self.stats.datagrams_expired_outgoing += count;
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Merging this PR will regress 1 benchmark
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 14, 2026 05:31
1f3c5c1 to
40b0996
Compare
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
September 14, 2026 05:31
d74cf65 to
b3a3b72
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3986 +/- ##
==========================================
- Coverage 97.12% 97.00% -0.13%
==========================================
Files 113 118 +5
Lines 41313 41747 +434
Branches 41313 41747 +434
==========================================
+ Hits 40126 40497 +371
- Misses 1173 1226 +53
- Partials 14 24 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 14, 2026 17:56
40b0996 to
95b5b8d
Compare
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
September 14, 2026 17:56
b3a3b72 to
af3de7e
Compare
Member
|
@jesup I don't think the PR title matches the code you are proposing here. This pull request only adds the reporting, no? |
Member
Author
You're right, and I had already updated this locally - the commit is retitled here to "WebTransport: count a session's expired outgoing datagrams" since the |
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 16, 2026 03:45
95b5b8d to
6a29098
Compare
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
September 16, 2026 03:45
af3de7e to
579205f
Compare
mxinden
reviewed
Oct 5, 2026
Base automatically changed from
users/jesup/rename_max_buffered_datagrams
to
main
October 5, 2026 19:02
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
October 6, 2026 05:22
1f13246 to
da023cc
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found no defects worth posting. The code-review pass turned up a few points, and none survived filtering. One (a local close drops the queue without collecting its pending expiry count) was already raised in an earlier review thread. The rest were cleanup suggestions, stats gaps you can't observe once the session is removed, or doc nits.
Collaborator
|
@jesup merge conflicts prevent the stack from landing. |
mxinden
reviewed
Oct 6, 2026
Member
Author
|
From #3988:
Sorry for the slow answer - it didn't show as a "Resolve" question and I skipped over it. Reacting to OutgoingDatagramOutcome wouldn't work as things stand: the transport raises no event when process_timer drops an expired datagram. The outcome only covers Acked/Lost/DroppedTooBig, and only for datagrams enqueued with an id; adding an expiry outcome would bring back the per-datagram drop events you asked to remove in #3983. |
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
October 12, 2026 15:06
da023cc to
c21be6e
Compare
mxinden
approved these changes
Oct 13, 2026
mxinden
left a comment
mxinden
left a comment
Member
There was a problem hiding this comment.
One last suggestion. Otherwise ready to merge from my end.
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
October 14, 2026 17:16
c21be6e to
68648a9
Compare
mxinden
reviewed
Oct 14, 2026
mxinden
approved these changes
Oct 14, 2026
mxinden
left a comment
mxinden
left a comment
Member
There was a problem hiding this comment.
Minor cleanup, otherwise ready to merge.
Collaborator
|
@jesup clippy is unhappy |
Implement Protocol::record_expired_outgoing_datagrams on the WebTransport session, so SessionStats::datagrams_expired_outgoing counts datagrams shed for exceeding outgoingMaxAge; the field existed but stayed zero. The HTTP/3 layer picks the counts up in its per-session sweep, and close_session collects any still pending before returning the final stats. A session the peer closes drops its queue as soon as it leaves the active state; a close capsule without a FIN would otherwise leave the queue in place until the FIN arrives. Datagrams still queued when a session closes are dropped, not counted as expired.
jesup
force-pushed
the
users/jesup/expire_datagrams
branch
from
October 16, 2026 12:07
68648a9 to
c859f4f
Compare
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
Client/server transfer resultsPerformance differences relative to 59f731f. 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 |
Contributor
mxinden
approved these changes
Oct 16, 2026
mxinden-bot
pushed a commit
to mxinden-bot/neqo
that referenced
this pull request
Oct 20, 2026
User-visible changes since v0.33.0: - Expose the outgoingMaxBufferedDatagrams and outgoingMaxAge setters as production API: mozilla#3990 - Expose the outgoing WebTransport datagram queue capacity per session: mozilla#4037 - Expose the per-datagram overhead in the datagram queue capacity: mozilla#4067 - Report the fate of outgoing WebTransport datagrams via aggregate stats: mozilla#4091 - Count datagrams actually sent per session: mozilla#4030 - Count a session's expired outgoing WebTransport datagrams: mozilla#3986 - Return NotAvailable instead of TooMuchData when the peer has no datagram path: mozilla#3989 - Put ABE (RFC 8511) behind a pref: mozilla#4090 - Add a log-payload feature: mozilla#4046 - Remove the deprecated ConnectionError alias: mozilla#4089 - Answer every PATH_CHALLENGE received on a path: mozilla#4101 - Honor a peer's minimum ACK delay above 50ms: mozilla#4068 - Use version-specific key update labels: mozilla#4080 - Compute QPACK MaxEntries from the peer's capacity: mozilla#4033 - Stop connection ID generation when the generator is exhausted: mozilla#4082 - Pad to the PMTUD probe size only when sending a probe: mozilla#3964 - Ignore ACKs of stale PMTUD probes: mozilla#4088
mxinden-bot
pushed a commit
to mxinden-bot/neqo
that referenced
this pull request
Oct 20, 2026
User-visible changes since v0.33.0: - Expose the outgoingMaxBufferedDatagrams and outgoingMaxAge setters as production API: [mozilla#3990](mozilla#3990) - Expose the outgoing WebTransport datagram queue capacity per session: [mozilla#4037](mozilla#4037) - Expose the per-datagram overhead in the datagram queue capacity: [mozilla#4067](mozilla#4067) - Report the fate of outgoing WebTransport datagrams via aggregate stats: [mozilla#4091](mozilla#4091) - Count datagrams actually sent per session: [mozilla#4030](mozilla#4030) - Count a session's expired outgoing WebTransport datagrams: [mozilla#3986](mozilla#3986) - Return NotAvailable instead of TooMuchData when the peer has no datagram path: [mozilla#3989](mozilla#3989) - Put ABE (RFC 8511) behind a pref: [mozilla#4090](mozilla#4090) - Add a log-payload feature: [mozilla#4046](mozilla#4046) - Remove the deprecated ConnectionError alias: [mozilla#4089](mozilla#4089) - Answer every PATH_CHALLENGE received on a path: [mozilla#4101](mozilla#4101) - Honor a peer's minimum ACK delay above 50ms: [mozilla#4068](mozilla#4068) - Use version-specific key update labels: [mozilla#4080](mozilla#4080) - Compute QPACK MaxEntries from the peer's capacity: [mozilla#4033](mozilla#4033) - Stop connection ID generation when the generator is exhausted: [mozilla#4082](mozilla#4082) - Pad to the PMTUD probe size only when sending a probe: [mozilla#3964](mozilla#3964) - Ignore ACKs of stale PMTUD probes: [mozilla#4088](mozilla#4088)
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.
Uh oh!
There was an error while loading. Please reload this page.