(AI) Initial draft 16 support #938
Conversation
WalkthroughAdds IETF WebTransport Draft‑16 support across JS and Rust: new Version/ALPN entries for DRAFT_16 and negotiation paths updated to prefer/advertise draft‑16. Many encode/decode APIs were threaded with an IetfVersion parameter. Parameters/MessageParameters implement delta encoding for Draft‑16. RequestError/RequestError structs gain a retryInterval/retry_interval field used for Draft‑16. Broadcast default reload changed to false. Connection constructor made to require a version. Rust control code adds a timeout when waiting for next request id. Tests updated for versioned round‑trips. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-lite/src/ietf/request.rs (1)
113-136:⚠️ Potential issue | 🔴 CriticalCritical wire format mismatch between Rust and JS implementations in Draft16 REQUEST_ERROR encoding.
The JS decoder correctly implements the draft-ietf-moq-transport-16 spec ordering (Request ID → Error Code → Retry Interval → Reason Phrase), but both the JS encoder and the Rust encoder/decoder use an incorrect order (Request ID → Error Code → Reason Phrase → Retry Interval). This causes the JS decoder to fail when parsing Draft16 REQUEST_ERROR messages from either the Rust implementation or even its own encoder.
The Rust
encode_msg/decode_msgand JS#encodemethods must be updated to match the spec: encode and decode retry interval before reason phrase for Draft16 messages.
🤖 Fix all issues with AI agents
In `@js/hang/src/watch/broadcast.ts`:
- Line 43: The change made reload default to false at the Signal creation
(this.reload = Signal.from(props?.reload ?? false)) silently alters behavior;
either restore the previous behavior by making the default true (use
props?.reload ?? true in the Signal.from call) or, if the new default is
intentional for Draft 16, update the doc comment above (lines referencing the
reload behavior) to explicitly state "Reloading is disabled by default; pass
reload: true to wait for an announcement" so callers (and files like
js/lite/...subscriber.ts and js/hang/...element.ts) are not misled.
In `@js/lite/src/ietf/ietf.test.ts`:
- Around line 407-416: The test fails due to field-order mismatch in
RequestError serialization: RequestError.#encode currently writes reasonPhrase
before retryInterval while RequestError.#decode reads retryInterval then
reasonPhrase; make the wire order consistent by changing RequestError.#encode
(in request.ts) to write retryInterval before reasonPhrase so it matches the
existing RequestError.#decode order (or alternatively reorder
RequestError.#decode to match `#encode`) ensuring both encode and decode use the
exact same field sequence.
In `@js/lite/src/ietf/request.ts`:
- Around line 108-127: The private encoder `#encode` in RequestError writes fields
as requestId, errorCode, reasonPhrase, then retryInterval for Version.DRAFT_16,
but `#decode` expects requestId, errorCode, retryInterval, reasonPhrase; update
`#encode` (used by encode/Message.encode) so that when version ===
Version.DRAFT_16 it writes retryInterval (await w.u62(this.retryInterval))
immediately after errorCode and before writing reasonPhrase, ensuring the
Writer/Reader order matches the Reader in static `#decode`.
In `@rs/moq-lite/src/ietf/session.rs`:
- Around line 138-146: The Draft16 branch currently rejects ID 0x08 messages but
per the spec ID 0x08 is NAMESPACE in Draft16; replace the Version::Draft16 arm
that returns Err with code that decodes the message using
ietf::PublishNamespaceError::decode_msg(&mut data, version)? (same decoder used
above) or the appropriate ietf::Namespace decode helper, emit a
tracing::debug!(message = ?msg, "received control message"), and forward the
decoded message to the publisher (e.g., call publisher.recv_namespace(msg)? or
the existing subscriber method that handles namespace suffix responses) instead
of returning UnexpectedMessage so NAMESPACE responses are processed correctly.
🧹 Nitpick comments (12)
js/lite/src/connection/connect.ts (1)
103-107: Nested ternary is acceptable here but consider a comment or a small helper if more versions are added.Three levels of ternary is borderline. Currently readable, but if a Draft 17 is added this will become unwieldy. No change needed now.
js/lite/src/ietf/parameters.ts (2)
65-103: Duplicated delta-encoding logic betweenParametersandMessageParameters.The
encodemethods of both classes contain nearly identical Draft 16 delta-encoding logic (merge keys, sort, emit deltas). The same is true for thedecodemethods. Consider extracting a shared helper, e.g.:async function encodeDeltaParams( w: Writer, vars: Map<bigint, bigint>, bytes: Map<bigint, Uint8Array>, ): Promise<void> { /* shared delta logic */ }This would also reduce the risk of the two implementations drifting apart in future drafts.
Also applies to: 259-297
75-75: Misleading variable nameprevType— it tracks the previous key/id, not a type.
prevIdorprevKeywould better convey intent.Suggested rename
- let prevType = 0n; + let prevId = 0n; for (let i = 0; i < all.length; i++) { const { key, isVar } = all[i]; - const delta = i === 0 ? key : key - prevType; - prevType = key; + const delta = i === 0 ? key : key - prevId; + prevId = key;Apply the same rename in decode (lines 110 and 304).
As per coding guidelines, "Use clear and descriptive variable names that convey intent".
Also applies to: 269-269
rs/moq-lite/src/ietf/control.rs (1)
74-94: Hardcoded 10-second timeout forMAX_REQUEST_ID.The timeout is created once before the loop, so it acts as an absolute deadline — this is correct. However, the 10-second value is hardcoded. Consider making it configurable (e.g., a field on
Controlor a constant) so callers can tune it for different deployment scenarios.rs/moq-lite/src/ietf/fetch.rs (1)
332-422: Consider adding Draft16 round-trip tests forFetchandFetchOk.While Draft16 shares the Draft15 code path, adding a simple round-trip test for
Version::Draft16would confirm the routing works end-to-end and guard against future divergence. As per coding guidelines, "Rust tests should be integrated within source files".Example test to add
#[test] fn test_fetch_v16_round_trip() { let msg = Fetch { request_id: RequestId(1), subscriber_priority: 128, group_order: GroupOrder::Descending, fetch_type: FetchType::Standalone { namespace: Path::new("test"), track: "video".into(), start: Location { group: 0, object: 0 }, end: Location { group: 10, object: 5 }, }, }; let encoded = encode_message(&msg, Version::Draft16); let decoded: Fetch = decode_message(&encoded, Version::Draft16).unwrap(); assert_eq!(decoded.request_id, RequestId(1)); assert_eq!(decoded.subscriber_priority, 128); } #[test] fn test_fetch_ok_v16_round_trip() { let msg = FetchOk { request_id: RequestId(2), group_order: GroupOrder::Descending, end_of_track: false, end_location: Location { group: 5, object: 3 }, }; let encoded = encode_message(&msg, Version::Draft16); let decoded: FetchOk = decode_message(&encoded, Version::Draft16).unwrap(); assert_eq!(decoded.request_id, RequestId(2)); assert!(!decoded.end_of_track); assert_eq!(decoded.end_location, Location { group: 5, object: 3 }); }rs/moq-lite/src/ietf/track.rs (1)
96-142: Consider adding a Draft16 round-trip test forTrackStatus.Same reasoning as
fetch.rs— a quick test withVersion::Draft16would confirm routing and guard against future divergence. As per coding guidelines, "Rust tests should be integrated within source files".rs/moq-lite/src/ietf/publisher.rs (2)
137-137: Doc comment says "v15" but should say "v15+".The comment on Line 137 says "using RequestError for v15" but Draft16 now also uses this path.
📝 Suggested fix
- /// Send a subscribe error, using RequestError for v15. + /// Send a subscribe error, using RequestError for v15+.
429-429: Same doc comment inconsistency: "v15" → "v15+".📝 Suggested fix
- /// Send a fetch error, using RequestError for v15. + /// Send a fetch error, using RequestError for v15+.rs/moq-lite/src/ietf/publish.rs (1)
396-477: Consider adding Draft16 round-trip tests.Tests cover Draft14 and Draft15 but not Draft16. Since Draft16 uses delta encoding for
MessageParameters(as seen inparameters.rs), a Draft16 round-trip test here would exercise that integration path and catch any parameter encoding issues specific to Publish messages.rs/moq-lite/src/ietf/setup.rs (1)
117-171: Consider adding Draft16 round-trip tests for setup messages.Since Draft16 uses delta encoding for parameters, adding v16 round-trip tests would verify the end-to-end path through delta-encoded parameter serialization in the setup context.
rs/moq-lite/src/ietf/session.rs (1)
244-251: Exhaustive match is fine but could be simplified.Lines 246 and 250 match all three variants explicitly (
Draft14 | Draft15 | Draft16) and return the same error. This is functionally equivalent to a wildcard_but is more explicit about intent — fine either way.rs/moq-lite/src/ietf/parameters.rs (1)
82-129: Parameters Draft16 encode relies on HashMap iteration order consistency — correct but fragile.The encode collects keys and values into separate
Vecs and uses index-based cross-referencing (orig_idx). This works becauseHashMap::keys()andHashMap::values()iterate in matching order within a single access. However, this pattern is fragile and harder to reason about than iterating viaiter()to get key-value pairs together.Consider using
self.vars.iter()directly to collect(key, value)pairs, avoiding the index indirection:♻️ Suggested simplification
Version::Draft16 => { - // Delta encoding: collect all keys, sort, encode deltas - let mut all: Vec<(u64, bool, usize)> = Vec::new(); // (key, is_var, index) - let var_keys: Vec<_> = self.vars.keys().collect(); - let byte_keys: Vec<_> = self.bytes.keys().collect(); - for (i, k) in var_keys.iter().enumerate() { - all.push((u64::from(**k), true, i)); - } - for (i, k) in byte_keys.iter().enumerate() { - all.push((u64::from(**k), false, i)); - } - all.sort_by_key(|(k, _, _)| *k); - - let var_vals: Vec<_> = self.vars.values().collect(); - let byte_vals: Vec<_> = self.bytes.values().collect(); - - let mut prev_type: u64 = 0; - for (idx, (kind, is_var, orig_idx)) in all.iter().enumerate() { - let delta = if idx == 0 { *kind } else { kind - prev_type }; - prev_type = *kind; - delta.encode(w, version); - - if *is_var { - var_vals[*orig_idx].encode(w, version); - } else { - byte_vals[*orig_idx].encode(w, version); - } - } + enum ParamValue<'a> { + Var(&'a u64), + Bytes(&'a Vec<u8>), + } + let mut all: Vec<(u64, ParamValue)> = Vec::new(); + for (k, v) in self.vars.iter() { + all.push((u64::from(*k), ParamValue::Var(v))); + } + for (k, v) in self.bytes.iter() { + all.push((u64::from(*k), ParamValue::Bytes(v))); + } + all.sort_by_key(|(k, _)| *k); + + let mut prev_type: u64 = 0; + for (idx, (kind, val)) in all.iter().enumerate() { + let delta = if idx == 0 { *kind } else { kind - prev_type }; + prev_type = *kind; + delta.encode(w, version); + match val { + ParamValue::Var(v) => v.encode(w, version), + ParamValue::Bytes(v) => v.encode(w, version), + } + }This mirrors the approach already used in the
MessageParametersencode (Lines 207-230), reducing inconsistency between the two implementations.
| // 0x08: PublishNamespaceError in v14, NAMESPACE in v16, removed in v15 | ||
| ietf::PublishNamespaceError::ID => match version { | ||
| Version::Draft14 => { | ||
| let msg = ietf::PublishNamespaceError::decode_msg(&mut data, version)?; | ||
| tracing::debug!(message = ?msg, "received control message"); | ||
| publisher.recv_publish_namespace_error(msg)?; | ||
| } | ||
| Version::Draft15 => return Err(Error::UnexpectedMessage), | ||
| Version::Draft15 | Version::Draft16 => return Err(Error::UnexpectedMessage), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MoQ Transport draft-16 message ID 0x08 NAMESPACE specification
💡 Result:
In draft-ietf-moq-transport-16 (published 2026-01-13), the MOQT control message with Type / Message ID 0x08 is NAMESPACE. [1][2]
NAMESPACE (Type = 0x08) message format
NAMESPACE Message {
Type (i) = 0x8,
Length (16),
Track Namespace Suffix (..),
}
[1]
Semantics
NAMESPACEis likePUBLISH_NAMESPACE, but it is sent on the response stream of aSUBSCRIBE_NAMESPACErequest. [1]- Because every
NAMESPACEis in response toSUBSCRIBE_NAMESPACE, it carries only a Track Namespace Suffix:- Track Namespace Suffix = the track namespace after removing the namespace tuples included in the
SUBSCRIBE_NAMESPACEmessage’s Track Namespace Prefix. [1]
- Track Namespace Suffix = the track namespace after removing the namespace tuples included in the
How Track Namespace Suffix is encoded (Track Namespace structure)
Track Namespace Suffix uses the standard Track Namespace encoding:
- An ordered set of 1..32 Track Namespace Fields:
Track Namespace {
Number of Track Namespace Fields (i),
Track Namespace Field (..) ...
}
Track Namespace Field {
Track Namespace Field Length (i),
Track Namespace Field Value (..)
}
- Each field value MUST be at least 1 byte (length 0 is a protocol violation). [1]
Sources: IETF Internet-Draft Media over QUIC Transport (draft-ietf-moq-transport-16). [1][2]
🏁 Script executed:
# Search for NAMESPACE message handling in the codebase
rg -i "namespace" rs/moq-lite/src/ -A 3 -B 3
# Look for message ID definitions to understand the mapping
rg "0x08" rs/moq-lite/src/ -A 5 -B 5
# Check for any draft version specifications or enums
rg "Draft14|Draft15|Draft16" rs/moq-lite/src/ietf/ -lRepository: moq-dev/moq
Length of output: 50367
🏁 Script executed:
# Check if there's a NAMESPACE message type defined
rg "struct.*Namespace|enum.*Namespace" rs/moq-lite/src/
# Look for PublishNamespaceError definition
rg "PublishNamespaceError" rs/moq-lite/src/ -A 5 -B 2Repository: moq-dev/moq
Length of output: 5407
Implement NAMESPACE message handler for Draft16 instead of rejecting it.
The comment is correct: ID 0x08 is NAMESPACE in draft-16 per the MOQT spec. However, the code currently returns UnexpectedMessage for Draft16, which rejects all incoming NAMESPACE messages. A dedicated handler is required to decode and process NAMESPACE messages as specified in draft-16 (which carries a Track Namespace Suffix in response to SUBSCRIBE_NAMESPACE requests), rather than treating them as protocol violations.
🤖 Prompt for AI Agents
In `@rs/moq-lite/src/ietf/session.rs` around lines 138 - 146, The Draft16 branch
currently rejects ID 0x08 messages but per the spec ID 0x08 is NAMESPACE in
Draft16; replace the Version::Draft16 arm that returns Err with code that
decodes the message using ietf::PublishNamespaceError::decode_msg(&mut data,
version)? (same decoder used above) or the appropriate ietf::Namespace decode
helper, emit a tracing::debug!(message = ?msg, "received control message"), and
forward the decoded message to the publisher (e.g., call
publisher.recv_namespace(msg)? or the existing subscriber method that handles
namespace suffix responses) instead of returning UnexpectedMessage so NAMESPACE
responses are processed correctly.
1 participant
No description provided.