Conversation
Defines a PROBE bidirectional stream extension for bandwidth estimation. Subscribers request a target bitrate, publishers respond with measured bitrate and elapsed timestamps. Supports QUIC PING+PADDING or datagram fallback for padding. Addresses the application-limited problem where congestion controllers underestimate bandwidth for ABR media delivery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new informational draft specification for a PROBE extension mechanism within the Media over QUIC (MoQ) Transport protocol. The document defines negotiation procedures via a PROBE Setup Option, establishes a bidirectional PROBE stream type for bandwidth probing, and specifies message formats for PROBE_REQUEST and PROBE_RESPONSE exchanges. The mechanism enables subscribers to communicate target bitrates to publishers, who respond with measured bitrate and timing information. The specification includes guidance on per-hop bandwidth estimation, optional QUIC padding strategies for probing, congestion control integration, security considerations, rate-limiting recommendations, and IANA registry entries for the new Setup Option and Stream Type identifiers. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
draft-lcurley-moq-probe.md (2)
182-187: Consider adding guidance on PROBE_REQUEST rate limiting.The security considerations appropriately address excessive target values and padding amplification. However, there's no mention of rate-limiting the frequency of PROBE_REQUEST messages themselves.
A malicious or misbehaving subscriber could flood the publisher with frequent PROBE_REQUESTs (even with reasonable target values), causing processing overhead or rapid oscillation in padding behavior. Consider adding a recommendation that publishers MAY rate-limit or ignore PROBE_REQUESTs that arrive too frequently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@draft-lcurley-moq-probe.md` around lines 182 - 187, Add a short paragraph to the "Security Considerations" section recommending that publishers MAY rate-limit or ignore frequent PROBE_REQUEST messages to prevent flooding/oscillation: describe that publishers SHOULD enforce a minimum inter-request interval (or token-bucket/connection-based limits) for PROBE_REQUESTs from a given subscriber and MAY drop or delay requests that exceed that frequency, and suggest logging/metrics for monitoring; reference PROBE_REQUEST, publisher and subscriber when adding this guidance.
133-136: Consider clarifying bitrate measurement method.The spec states the Measured Bitrate is "across all streams on the connection" but doesn't specify the measurement methodology. Is this:
- Instantaneous rate at the moment?
- Average over the interval since the last PROBE_RESPONSE?
- Smoothed average over some window?
Specifying this would improve interoperability and ensure consistent behavior across implementations. Given the Elapsed field, averaging over the elapsed interval since the last response would be a natural choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@draft-lcurley-moq-probe.md` around lines 133 - 136, Clarify how the "Measured Bitrate" field is computed: update the description for Measured Bitrate to state whether it is an instantaneous value, an average over the elapsed interval, or a smoothed/windowed average, and explicitly tie it to the "Elapsed" field and PROBE_RESPONSE timing (e.g., "Measured Bitrate is the average kilobits-per-second across all streams on the connection over the elapsed interval since the previous PROBE_RESPONSE"); include any unit, rounding, and whether padding/other-data are included (already noted) so implementations produce consistent values for Measured Bitrate and anyone consuming PROBE_RESPONSE messages can interoperate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@draft-lcurley-moq-probe.md`:
- Around line 155-170: The spec defines a PROBE Datagram Type (Datagram Type
(vi64) = 0xPROBE_TODO) but never requests IANA registration; add a new IANA
subsection that registers this value in the MoQ Datagram Types registry.
Specifically, update the IANA Considerations section to include a "MOQT Datagram
Type" table registering Value = 0xPROBE_TODO, Name = PROBE, Reference = This
Document, and ensure the text instructs IANA to allocate/record that datagram
type for the protocol (referencing the PROBE Datagram and Datagram Type symbols
so reviewers can find the exact usage).
- Around line 103-108: The PROBE_REQUEST/PROBE_RESPONSE definitions lack
semantics for the Message Length field; update the definitions for Message
Length (and optionally PROBE_RESPONSE) to state explicitly whether it includes
itself or only subsequent bytes—for example: "Message Length (vi64) — number of
bytes that follow, encoded as specified in Section X.X of {{moqt}}." Reference
the exact encoding section in {{moqt}} (or state "excludes the Message Length
field") and apply the same wording to both PROBE_REQUEST and PROBE_RESPONSE to
keep them consistent.
---
Nitpick comments:
In `@draft-lcurley-moq-probe.md`:
- Around line 182-187: Add a short paragraph to the "Security Considerations"
section recommending that publishers MAY rate-limit or ignore frequent
PROBE_REQUEST messages to prevent flooding/oscillation: describe that publishers
SHOULD enforce a minimum inter-request interval (or
token-bucket/connection-based limits) for PROBE_REQUESTs from a given subscriber
and MAY drop or delay requests that exceed that frequency, and suggest
logging/metrics for monitoring; reference PROBE_REQUEST, publisher and
subscriber when adding this guidance.
- Around line 133-136: Clarify how the "Measured Bitrate" field is computed:
update the description for Measured Bitrate to state whether it is an
instantaneous value, an average over the elapsed interval, or a
smoothed/windowed average, and explicitly tie it to the "Elapsed" field and
PROBE_RESPONSE timing (e.g., "Measured Bitrate is the average
kilobits-per-second across all streams on the connection over the elapsed
interval since the previous PROBE_RESPONSE"); include any unit, rounding, and
whether padding/other-data are included (already noted) so implementations
produce consistent values for Measured Bitrate and anyone consuming
PROBE_RESPONSE messages can interoperate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22d0c095-4315-44fa-81c7-b53bf620505c
📒 Files selected for processing (1)
draft-lcurley-moq-probe.md
| ~~~ | ||
| PROBE_REQUEST { | ||
| Message Length (vi64) | ||
| Target Bitrate (vi64) | ||
| } | ||
| ~~~ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check moqt base spec for Message Length framing conventions
# Search for Message Length definitions in related documents
fd -e md -e txt . | xargs rg -n -C3 'Message Length' | head -50Repository: moq-dev/drafts
Length of output: 2161
🏁 Script executed:
# Check if draft-lcurley-moq-probe.md references moq-lite or explicitly defines Message Length semantics
rg -n -C5 'Message Length|moq-lite|moq\.lite' draft-lcurley-moq-probe.mdRepository: moq-dev/drafts
Length of output: 607
🏁 Script executed:
# Check the beginning of the probe document for YAML frontmatter and reference structure
head -50 draft-lcurley-moq-probe.mdRepository: moq-dev/drafts
Length of output: 2101
Add a reference or brief clarification for the Message Length field semantics.
The PROBE_REQUEST and PROBE_RESPONSE message definitions include a Message Length field but do not explain whether it includes itself or only the subsequent fields. While this should be defined in the normative {{moqt}} reference, adding a brief note (e.g., "following the encoding defined in Section X.X of {{moqt}}") or an explicit statement that it represents only the bytes that follow would improve clarity for implementers.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 103-103: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@draft-lcurley-moq-probe.md` around lines 103 - 108, The
PROBE_REQUEST/PROBE_RESPONSE definitions lack semantics for the Message Length
field; update the definitions for Message Length (and optionally PROBE_RESPONSE)
to state explicitly whether it includes itself or only subsequent bytes—for
example: "Message Length (vi64) — number of bytes that follow, encoded as
specified in Section X.X of {{moqt}}." Reference the exact encoding section in
{{moqt}} (or state "excludes the Message Length field") and apply the same
wording to both PROBE_REQUEST and PROBE_RESPONSE to keep them consistent.
- Add IANA registration for PROBE datagram type - Clarify measured bitrate is implementation-defined, with examples for pacing-based (BBR) vs window-based (CUBIC/Reno) controllers - Add rate-limiting guidance for PROBE_REQUEST flooding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Test plan
makebuilds successfully🤖 Generated with Claude Code