Repository navigation
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Checkpoint One characterization catches Required all-language interop is NOT RUN. Matching shared/QMux implementation is web-transport440 at13bd407, with full fix/check/test and real Rust/TS WebSocket interop passing. Compatible publication/adoption remains pending. No package release, version bump, deployment or merge occurred. (written by GPT-6) |
kixelated
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hook Drop the web-transport-trait Stats::streams plumbing and its git patch on moq-dev/web-transport#440. Native QUIC sessions in moq-tokio and moq-uring now hand moq-quic's snapshot to moq-net through the crate-private Grouped hook, next to wire(). The FFI and WASM adapters only wrap the browser backend, which has no counts, so they are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds bidirectional and unidirectional stream occupancy and available-credit statistics to QUIC and transport APIs. QUIC tracks locally initiated streams until full retirement and reports reset values after rejected 0-RTT or connection closure. Tokio and uring adapters pass stream statistics into session snapshots. Tests cover QUIC accounting, adapter propagation, and snapshots. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 32.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 14 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: exposing native stream occupancy and remaining peer opening credit. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description directly explains the added native transport stream occupancy and peer stream-credit statistics, the affected crates, implementation approach, scope limits, tests, and val… Full details: Description check
-
Explanation
The pull request description directly explains the added native transport stream occupancy and peer stream-credit statistics, the affected crates, implementation approach, scope limits, tests, and validation results.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 32.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 14 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bff2c394d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(super) recv: FxHashMap<StreamId, Option<StreamRecv>>, | ||
| pub(super) free_recv: Vec<StreamRecv>, | ||
| pub(super) next: [u64; 2], | ||
| local_open: [u64; 2], |
There was a problem hiding this comment.
This adds custom stream-accounting behavior directly to the hard-forked moq-quic, but rs/moq-quic/README.md has no corresponding entry under “Carried changes.” CONTRIBUTING.md:78-79 requires every carried fork change to be recorded with its upstream PR or the reason it has none; without that entry, future Quinn cherry-pick audits can overlook this custom state-management behavior. Add the inventory entry in this change.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 286c8fb: rs/moq-quic/README.md now lists the stream count under "Carried changes" (not offered upstream, since quinn exposes no stream counts), naming insert_local, stream_freed, and zero_rtt_rejected as the spots a cherry-pick must keep.
(Written by Claude Opus 5.5)
|
Closing: nothing consumes the stream snapshot. It isn't published on the stats tracks or exposed through moq-ffi or the bindings, and request-credit checks ask moq-quic directly. Per AGENTS.md we keep API private until a consumer needs it. If a stats or debugging need shows up, this branch is a good starting point. (moq-dev/web-transport#440 already shipped and stays as is; it's an additive default method.) (Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reopening: there is a consumer now. A downstream relay operator's metrics endpoint needs to graph a long-lived session's transport stream headroom ( The scope is unchanged: moq-net and moq-quic only, no moq-ffi or binding surface, and no stats-track or wire change. Back to draft while I merge (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 286c8fb8f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Locally initiated stream occupancy and remaining peer stream-ID credit. | ||
| pub streams: Option<crate::transport::Streams>, | ||
|
|
There was a problem hiding this comment.
This adds public moq-net stream statistics only on the Rust side, while a repo-wide search shows js/net/src/connection/stats.ts and doc/concept remain unchanged even though the JavaScript Stats interface says its fields match Rust's session::Stats. Update the JS surface and concept documentation, or explicitly document the native-only unsupported field so the cross-language APIs do not silently diverge.
AGENTS.md reference: AGENTS.md:L107-L110
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly fixed in 3e0353e: the JS Stats doc no longer claims a full match and names streams as Rust's native-only field, since W3C getStats() has no stream counts to report. Declining the rest: an always-absent JS field or a concept-doc section would publish surface no browser consumer needs, and doc/concept and the binding docs describe only what the stats tracks and moq-ffi carry, which this PR leaves unchanged.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Closing: moq-net only forwards the snapshot and nothing in moq reads it. The request-credit work needs an atomic non-waiting open, not a racy snapshot, so the one in-repo use of stream credit doesn't need this either. A single out-of-repo metrics consumer doesn't justify the new exported API across moq-quic, moq-net, moq-tokio, and moq-uring. The branch stays as a starting point if an in-repo consumer appears. (Written by Claude Opus 5.5) |
Problem
Transport users cannot tell locally initiated stream occupancy apart from exhausted peer stream-ID credit. Cumulative MAX_STREAMS is not a fixed concurrent capacity, so neither number can be derived from the other. A metrics endpoint that holds one long-lived session (a stats collector subscribed to many tracks, for example) needs both to graph its stream headroom; MoQ handle counts under-report it, since setup and track-info requests hold streams too and a stream stays open until the transport retires it.
Approach
Forward an optional structured snapshot through moq-net's existing session sampler.
availableis the peer's cumulative limit minus the next stream ID, so closing a stream never restores it on its own.transport::Stats::streams()through each crate's sealedGroupedhook, next towire(). Nothing goes throughweb-transport-trait, and no moq-dev/web-transport change or patch is needed.None, the trait default: qmux (WebSocket, TCP, WebRTC) and iroh in moq-tokio, and the browser backend behind the moq-ffi and moq-wasm adapters.rs/moq-quic/README.mdlists the stream count among the fork's carried changes, and the JSStatsdoc namesstreamsas native-only.Impact
transport::Streams { bidi, uni },transport::StreamState { open, available },transport::Stats::streams() -> Option<Streams>with a defaultNone, and asession::Stats::streamsfield (the struct is#[non_exhaustive]). The moq-net structs stay exhaustive because backend adapters in other crates construct them.StreamStats,StreamState(both#[non_exhaustive], like their sibling stats), and aConnectionStats::streamsfield.Grouped::streamshook is on a sealed, unnameable trait.MoqConnectionStatsdoes not gain the snapshot until a binding consumer needs it.doc/concept/stats.mdis unchanged because the stats tracks do not carry the snapshot.Alternatives
Settled during
/quest-iterate, picking the recommended option:Groupedhook moq-tokio and moq-uring already use forwire()and send groups. Native QUIC keeps the whole point of the PR; only the out-of-tree qmux crate loses it, and it gains it when qmux moves onto moq-quic (quest/m1/quic-qmux.md). When quest/m2/drop-web-transport-trait.md deletes the adapters, the hook body moves into each native session's owntransport::pollimpl.transport::polldirectly on the native sessions first. Rejected: that is the [L] drop-web-transport-trait quest, far past this PR.web_transport_trait::Stats::streams()(moq-dev/web-transport#440, now published asweb-transport-trait0.5.1 andqmux0.6.4) so qmux reports too. Rejected by the maintainer.MoqConnectionStats:rs/moq-cand the bindings.#[non_exhaustive]on the new moq-quic stats structs:CreditStatsand the other moq-quic stats, which only moq-quic constructs.quest/m1/rs-request-credit.md stays consistent: its credit check is a non-waiting open, not this stats snapshot, and its native path already asks moq-quic directly.
Follow-ups
Noneuntil quest/m1/quic-qmux.md moves it onto moq-quic.Validation
Grouped::streamshook reachesStats::streams(); moq-tokio's native QUIC and WebTransport integration tests assert the server session reportsSomestreams.just cion286c8fb8(merged withmainat2e7473d1): exit 0, 8,477 Rust tests passed plus every other selected check. On3e0353e6(a JS doc comment on top): fail-fast stopped at 7,095 of 8,477 on one failure,export::redundant_importers_switch_through_bufferbloat_on_lite_07, is the known flake in quest/m1/test-flakes-2/redundant-failover-drift.md (fix open in moq-dev/moq#6113) and passed rerun once alone.just test interop --allwas not run: no wire, moq-ffi or gateway change.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code