moq-lite-04: Replace hop count with explicit OriginId list#1152
Open
moq-lite-04: Replace hop count with explicit OriginId list#1152
Conversation
Add OriginId(u64) newtype for unique origin identification, with random
non-zero 62-bit generation and varint encode/decode. Add Lite04 variant
to the Version enum with its own ALPN ("moq-lite-04") and wire code
(0xff0dad04), without advertising it in ALPNS or Versions::all() yet.
Update all exhaustive match arms on lite::Version to use wildcard for
newest behavior per the version convention.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the simple hop counter with an explicit list of origin IDs that tracks the path from the origin through relay nodes. This enables loop detection via prefix checks and origin-based filtering. Key changes: - Broadcast.hops: u64 -> Vec<OriginId> - Announce wire format: Lite04 encodes count + OriginId list - AnnouncePlease: add without_origin filter for loop avoidance - OriginProducer: add id field (random by default, configurable) - Subscriber: appends own OriginId when forwarding announces - Publisher: filters announces containing without_origin ID - OriginNode::publish: prefix check + 32-hop cap - ClusterConfig: optional --cluster-origin-id Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add DRAFT_04 version, ALPN_04 constant, and update all wire format encode/decode to handle the new version. Key changes: - Announce.hops changed from number to bigint[] (array of OriginId) - AnnounceInterest gains withoutOrigin field (encoded for DRAFT_04+) - All version switches updated to use default for newest behavior - ALPN negotiation includes moq-lite-04 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The connectTransport function had a second ALPN switch that was missing the DRAFT_04 case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis pull request adds support for MoQ Lite protocol version DRAFT_04 (Lite04) across both JavaScript and Rust implementations. The changes include: adding new ALPN negotiation for the protocol variant, introducing version-dependent serialization for multiple message types (Announce, Subscribe, AnnounceInterest), replacing hop count representation from single numeric values to vector-based origin path tracking via a new OriginId type, updating session handling to support the new version, and extending cluster configuration to allow explicit origin identifier assignment. Version-specific control flow is consolidated to treat DRAFT_04 alongside DRAFT_03 in several message handlers while maintaining backward compatibility with earlier versions. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/lite/probe.rs (1)
5-11:⚠️ Potential issue | 🟡 MinorUpdate outdated doc comment.
The comment says "Lite03 only" but the code now allows
Lite04and future versions via the wildcard pattern.📝 Suggested fix
/// Sent to probe the available bitrate. /// -/// Lite03 only. +/// Lite03+ only. #[derive(Clone, Debug)] pub struct Probe {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-lite/src/lite/probe.rs` around lines 5 - 11, The doc comment for struct Probe is outdated ("Lite03 only"); update the comment for Probe (the Probe struct in lite/probe.rs) to reflect that the probe applies to Lite03, Lite04 and future versions (or remove the version-specific note), e.g., replace "Lite03 only" with a version-agnostic phrase like "Lite03 and later" or remove the version mention entirely so the comment accurately matches the wildcard version matching in the code.
🧹 Nitpick comments (3)
js/lite/src/lite/publisher.ts (1)
85-100: Consider using default case for forward compatibility.Explicitly listing
DRAFT_03andDRAFT_04requires updating this switch whenDRAFT_05is added. A default case would make future versions automatically use the newer announce behavior.♻️ Suggested refactor
switch (this.version) { - case Version.DRAFT_03: - case Version.DRAFT_04: - // Draft03+: send individual Announce messages for initial state. - for (const suffix of active) { - const wire = new Announce({ suffix, active: true }); - await wire.encode(stream.writer, this.version); - } - break; case Version.DRAFT_01: case Version.DRAFT_02: { const init = new AnnounceInit([...active]); await init.encode(stream.writer, this.version); break; } + default: + // Draft03+: send individual Announce messages for initial state. + for (const suffix of active) { + const wire = new Announce({ suffix, active: true }); + await wire.encode(stream.writer, this.version); + } + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/lite/src/lite/publisher.ts` around lines 85 - 100, The switch over this.version in publisher.ts currently enumerates Version.DRAFT_03 and Version.DRAFT_04 for the "newer" announce behavior, which will require updates when future drafts are added; change the switch to treat the per-suffix Announce path as the default: keep the old-path handling for Version.DRAFT_01 and Version.DRAFT_02 (using AnnounceInit([...active]).encode(...)), and move the per-suffix Announce({ suffix, active: true }).encode(...) loop into the switch's default branch so any unknown/new Version values will automatically use the newer behavior; reference symbols: this.version, Version enum, Announce, AnnounceInit, and stream.writer.rs/moq-lite/src/setup.rs (1)
30-33: Consider using wildcard for forward compatibility.Currently,
Lite03andLite04are explicitly listed. Per coding guidelines, newer/future versions should default forward. IfLite05is added later, this match arm would need updating.♻️ Suggested refactor for forward compatibility
Version::Lite(lite::Version::Lite01) | Version::Lite(lite::Version::Lite02) => Self::LiteLegacy, - Version::Lite(lite::Version::Lite03 | lite::Version::Lite04) => Self::Unsupported, + Version::Lite(_) => Self::Unsupported, // Lite03+ use ALPN-only negotiationBased on learnings: "When matching on
Versionenums, default to the newest draft behavior so future versions default forward. Explicitly list older versions for old behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-lite/src/setup.rs` around lines 30 - 33, Replace the explicit match arm listing Lite03 and Lite04 with a wildcard so future Lite versions default to the newest behavior: in the match over Version (matching lite::Version), keep the explicit older cases (lite::Version::Lite01 and lite::Version::Lite02 => Self::LiteLegacy) and change the other arm to a catch-all (e.g. _ => Self::Unsupported) so any new lite::Version variants automatically map to Self::Unsupported/newest-draft behavior.js/lite/src/lite/announce.ts (1)
10-15: Document the new public origin fields.
hopsnow means an ordered origin path in Draft04, while decoded Draft03 announces cannot populate it because that wire format only carries a count.withoutOriginis also Draft04-only and uses0nas the wire sentinel for “unset”. A short doc comment on these public fields would make that contract clear to consumers.📝 Suggested docs
export class Announce { suffix: Path.Valid; active: boolean; + /** Ordered OriginId path for Draft04+. Draft03 only carries the hop count. */ hops: bigint[]; ... } export class AnnounceInterest { prefix: Path.Valid; + /** Draft04 split-horizon filter; encoded as 0n on the wire when unset. */ withoutOrigin?: bigint; ... }As per coding guidelines, "Document public APIs with clear docstrings or comments" and "Write comments that explain the 'why', not just the 'what'."
Also applies to: 81-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/lite/src/lite/announce.ts` around lines 10 - 15, Add concise doc comments to the public origin-related fields to document the Draft04/Draft03 contract: above the hops field (and in the constructor) explain that hops is an ordered origin path used in Draft04, that decoded Draft03 announces only carried a count and therefore cannot populate hops, and that consumers should treat hops as empty when decoding Draft03; likewise add a doc comment for withoutOrigin noting it is Draft04-only and uses the sentinel value 0n on the wire to represent “unset.” Place these comments next to the public fields (hops and withoutOrigin) and constructors/methods that initialize them so callers clearly understand why and how the fields differ between drafts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/lite/src/lite/announce.ts`:
- Around line 22-35: Validate and enforce hop-list limits and non-zero OriginId
values before encoding/decoding in the announce logic: in the switch handling
versions (the block that calls w.u53(this.hops.length) and w.u62(hop)) add a
guard that checks this.hops.length <= 32 and throw or return an error if
exceeded, and when iterating hops ensure each hop is non-zero (hop !== 0n) and
otherwise reject/throw; do the same validation on the decode/path that reads the
count from u53 and then reads u62 entries (validate the count <= 32, then
validate each decoded hop is non-zero) so malformed frames cannot be accepted or
emitted.
In `@rs/moq-lite/src/lite/announce.rs`:
- Around line 39-54: The Lite03 branch currently decodes the count but returns
Vec::new(), losing hop-length information; change the Version::Lite03 arm to
decode the count (u64::decode) and return a Vec whose length equals that count
(e.g., Vec::with_capacity/count then push placeholder OriginId entries) so that
broadcast.info.hops.len() reflects the advertised hop-count; use a suitable
placeholder/default for OriginId (e.g., OriginId::default(), OriginId::zero(),
or a defined OriginId::UNKNOWN) instead of calling decode_origin_id, so
subscriber logic that appends the local OriginId keeps correct path-length and
loop context.
In `@rs/moq-lite/src/lite/publisher.rs`:
- Around line 203-216: The bug: the publisher sends lite::Announce::Ended for a
suffix even when no prior Active was sent on that stream (e.g., Active was
suppressed by without_origin), causing subscriber producers map misses; fix by
tracking which suffixes were actually announced on each stream (e.g., attach a
per-stream HashSet of sent suffixes on the same Stream/Subscription structure
used in publisher.rs), only mark a suffix as "sent" when you successfully encode
an Active (in the branch where active is Some and not filtered), and before
encoding lite::Announce::Ended check that the suffix exists in that sent set and
remove it when you send Ended; do not send Ended for suffixes not in the set.
In `@rs/moq-lite/src/model/origin.rs`:
- Around line 11-55: OriginId currently allows any u64 but must be a non-zero
62-bit value; update constructors and decoding to validate and reject
out-of-range values: change OriginId::new and OriginId::decode_raw to a Fallible
constructor (e.g. try_new(value: u64) -> Result<OriginId, InvalidOriginId>) that
checks 1 <= value < (1u64 << 62), update impl Decode<Version> for OriginId to
validate the decoded u64 and return an appropriate DecodeError on violation,
replace the derived serde::Deserialize with a custom Deserialize impl that uses
the same validation, and update call sites (e.g. CLI parsing for
--cluster-origin-id and any places using OriginId::new/decode_raw) to use the
fallible constructor or convert errors accordingly; leave OriginId::random as-is
since it already produces valid IDs.
- Around line 215-220: The current loop-detection uses starts_with between
broadcast.info.hops and existing.active.info.hops, but starts_with treats
identical hop lists as a match; change the guard in the block that contains
existing.active.info.hops and broadcast.info.hops so it only rejects when the
existing hops are a strict prefix of the new hops (i.e., keep the
existing.is_empty() check and starts_with check but also require
broadcast.info.hops.len() > existing.active.info.hops.len()); update the
condition around starts_with(...) in the same function/block that references
existing and broadcast so identical hop lists are not treated as redundant
loops.
---
Outside diff comments:
In `@rs/moq-lite/src/lite/probe.rs`:
- Around line 5-11: The doc comment for struct Probe is outdated ("Lite03
only"); update the comment for Probe (the Probe struct in lite/probe.rs) to
reflect that the probe applies to Lite03, Lite04 and future versions (or remove
the version-specific note), e.g., replace "Lite03 only" with a version-agnostic
phrase like "Lite03 and later" or remove the version mention entirely so the
comment accurately matches the wildcard version matching in the code.
---
Nitpick comments:
In `@js/lite/src/lite/announce.ts`:
- Around line 10-15: Add concise doc comments to the public origin-related
fields to document the Draft04/Draft03 contract: above the hops field (and in
the constructor) explain that hops is an ordered origin path used in Draft04,
that decoded Draft03 announces only carried a count and therefore cannot
populate hops, and that consumers should treat hops as empty when decoding
Draft03; likewise add a doc comment for withoutOrigin noting it is Draft04-only
and uses the sentinel value 0n on the wire to represent “unset.” Place these
comments next to the public fields (hops and withoutOrigin) and
constructors/methods that initialize them so callers clearly understand why and
how the fields differ between drafts.
In `@js/lite/src/lite/publisher.ts`:
- Around line 85-100: The switch over this.version in publisher.ts currently
enumerates Version.DRAFT_03 and Version.DRAFT_04 for the "newer" announce
behavior, which will require updates when future drafts are added; change the
switch to treat the per-suffix Announce path as the default: keep the old-path
handling for Version.DRAFT_01 and Version.DRAFT_02 (using
AnnounceInit([...active]).encode(...)), and move the per-suffix Announce({
suffix, active: true }).encode(...) loop into the switch's default branch so any
unknown/new Version values will automatically use the newer behavior; reference
symbols: this.version, Version enum, Announce, AnnounceInit, and stream.writer.
In `@rs/moq-lite/src/setup.rs`:
- Around line 30-33: Replace the explicit match arm listing Lite03 and Lite04
with a wildcard so future Lite versions default to the newest behavior: in the
match over Version (matching lite::Version), keep the explicit older cases
(lite::Version::Lite01 and lite::Version::Lite02 => Self::LiteLegacy) and change
the other arm to a catch-all (e.g. _ => Self::Unsupported) so any new
lite::Version variants automatically map to Self::Unsupported/newest-draft
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a357e5a5-35fe-495c-a57d-9edc9c0c1cba
📒 Files selected for processing (25)
js/lite/src/connection/accept.tsjs/lite/src/connection/connect.tsjs/lite/src/lite/announce.tsjs/lite/src/lite/connection.tsjs/lite/src/lite/fetch.tsjs/lite/src/lite/probe.tsjs/lite/src/lite/publisher.tsjs/lite/src/lite/session.tsjs/lite/src/lite/subscribe.tsjs/lite/src/lite/subscriber.tsjs/lite/src/lite/version.tsrs/moq-lite/src/ietf/subscriber.rsrs/moq-lite/src/lite/announce.rsrs/moq-lite/src/lite/fetch.rsrs/moq-lite/src/lite/info.rsrs/moq-lite/src/lite/probe.rsrs/moq-lite/src/lite/publisher.rsrs/moq-lite/src/lite/subscribe.rsrs/moq-lite/src/lite/subscriber.rsrs/moq-lite/src/lite/version.rsrs/moq-lite/src/model/broadcast.rsrs/moq-lite/src/model/origin.rsrs/moq-lite/src/setup.rsrs/moq-lite/src/version.rsrs/moq-relay/src/cluster.rs
Comment on lines
22
to
+35
| switch (version) { | ||
| case Version.DRAFT_03: | ||
| await w.u53(this.hops); | ||
| await w.u53(this.hops.length); | ||
| break; | ||
| case Version.DRAFT_01: | ||
| case Version.DRAFT_02: | ||
| break; | ||
| default: | ||
| unreachable(version); | ||
| // DRAFT_04+: encode array of OriginId | ||
| await w.u53(this.hops.length); | ||
| for (const hop of this.hops) { | ||
| await w.u62(hop); | ||
| } | ||
| break; |
Contributor
There was a problem hiding this comment.
Validate the hop list before encoding/decoding it.
Draft03/04 currently trust the hop count, and Draft04 also trusts every u62 hop value. That skips the new 32-hop cap and allows 0n entries even though OriginId is non-zero, so JS can accept/emit invalid frames and a peer can force unbounded array growth here.
🛡️ Suggested guardrails
+const MAX_ANNOUNCE_HOPS = 32;
+
export class Announce {
...
async `#encode`(w: Writer, version: Version) {
await w.bool(this.active);
await w.string(this.suffix);
switch (version) {
case Version.DRAFT_03:
+ if (this.hops.length > MAX_ANNOUNCE_HOPS) {
+ throw new Error(`announce path exceeds ${MAX_ANNOUNCE_HOPS} hops`);
+ }
await w.u53(this.hops.length);
break;
case Version.DRAFT_01:
case Version.DRAFT_02:
break;
default:
- await w.u53(this.hops.length);
+ if (this.hops.length > MAX_ANNOUNCE_HOPS) {
+ throw new Error(`announce path exceeds ${MAX_ANNOUNCE_HOPS} hops`);
+ }
+ await w.u53(this.hops.length);
for (const hop of this.hops) {
+ if (hop === 0n) {
+ throw new Error("origin ids must be non-zero");
+ }
await w.u62(hop);
}
break;
}
}
...
static async `#decode`(r: Reader, version: Version): Promise<Announce> {
...
switch (version) {
case Version.DRAFT_03: {
- await r.u53();
+ const count = await r.u53();
+ if (count > MAX_ANNOUNCE_HOPS) {
+ throw new Error(`announce path exceeds ${MAX_ANNOUNCE_HOPS} hops`);
+ }
break;
}
...
default: {
const count = await r.u53();
+ if (count > MAX_ANNOUNCE_HOPS) {
+ throw new Error(`announce path exceeds ${MAX_ANNOUNCE_HOPS} hops`);
+ }
for (let i = 0; i < count; i++) {
- hops.push(await r.u62());
+ const hop = await r.u62();
+ if (hop === 0n) {
+ throw new Error("origin ids must be non-zero");
+ }
+ hops.push(hop);
}
break;
}
}Also applies to: 43-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/lite/src/lite/announce.ts` around lines 22 - 35, Validate and enforce
hop-list limits and non-zero OriginId values before encoding/decoding in the
announce logic: in the switch handling versions (the block that calls
w.u53(this.hops.length) and w.u62(hop)) add a guard that checks this.hops.length
<= 32 and throw or return an error if exceeded, and when iterating hops ensure
each hop is non-zero (hop !== 0n) and otherwise reject/throw; do the same
validation on the decode/path that reads the count from u53 and then reads u62
entries (validate the count <= 32, then validate each decoded hop is non-zero)
so malformed frames cannot be accepted or emitted.
Comment on lines
39
to
+54
| let hops = match version { | ||
| Version::Lite03 => u64::decode(r, version)?, | ||
| Version::Lite01 | Version::Lite02 => 0, | ||
| Version::Lite01 | Version::Lite02 => Vec::new(), | ||
| Version::Lite03 => { | ||
| // Lite03 sends a single varint count; decode but we don't know the actual IDs. | ||
| let _count = u64::decode(r, version)?; | ||
| Vec::new() | ||
| } | ||
| _ => { | ||
| // Lite04+: count followed by that many OriginId varints. | ||
| let count = u64::decode(r, version)?; | ||
| let mut ids = Vec::with_capacity(count.min(32) as usize); | ||
| for _ in 0..count { | ||
| ids.push(decode_origin_id(r, version)?); | ||
| } | ||
| ids | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve the Lite03 hop count instead of collapsing it to an empty path.
rs/moq-lite/src/model/origin.rs now ranks routes by broadcast.info.hops.len(), and rs/moq-lite/src/lite/subscriber.rs appends the local OriginId before storing a forwarded route. Returning Vec::new() here makes every Lite03 announce look like 0/1 hops regardless of the advertised count, so upgraded peers will prefer the wrong path and lose loop context as long as they still interop over Lite03.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-lite/src/lite/announce.rs` around lines 39 - 54, The Lite03 branch
currently decodes the count but returns Vec::new(), losing hop-length
information; change the Version::Lite03 arm to decode the count (u64::decode)
and return a Vec whose length equals that count (e.g., Vec::with_capacity/count
then push placeholder OriginId entries) so that broadcast.info.hops.len()
reflects the advertised hop-count; use a suitable placeholder/default for
OriginId (e.g., OriginId::default(), OriginId::zero(), or a defined
OriginId::UNKNOWN) instead of calling decode_origin_id, so subscriber logic that
appends the local OriginId keeps correct path-length and loop context.
Comment on lines
203
to
216
| if let Some(broadcast) = active { | ||
| tracing::debug!(broadcast = %origin.absolute(&path), hops = broadcast.info.hops, "announce"); | ||
| let msg = lite::Announce::Active { suffix, hops: broadcast.info.hops }; | ||
| // Skip if the broadcast's hops contain the without_origin ID. | ||
| if let Some(ref id) = without_origin { | ||
| if broadcast.info.hops.contains(id) { | ||
| continue; | ||
| } | ||
| } | ||
| tracing::debug!(broadcast = %origin.absolute(&path), hops = broadcast.info.hops.len(), "announce"); | ||
| let msg = lite::Announce::Active { suffix, hops: broadcast.info.hops.clone() }; | ||
| stream.writer.encode(&msg).await?; | ||
| } else { | ||
| tracing::debug!(broadcast = %origin.absolute(&path), "unannounce"); | ||
| let msg = lite::Announce::Ended { suffix, hops: 0 }; | ||
| let msg = lite::Announce::Ended { suffix, hops: Vec::new() }; | ||
| stream.writer.encode(&msg).await?; |
Contributor
There was a problem hiding this comment.
Don't emit Ended for suffixes that were never announced on this stream.
without_origin can suppress an Active, but this branch still sends Ended unconditionally. In rs/moq-lite/src/lite/subscriber.rs, Ended removes the suffix from the per-stream producers map and errors on a miss, so withdrawing a filtered path will tear down the entire announce stream. Track which suffixes were actually sent on this subscription and only encode Ended for those.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-lite/src/lite/publisher.rs` around lines 203 - 216, The bug: the
publisher sends lite::Announce::Ended for a suffix even when no prior Active was
sent on that stream (e.g., Active was suppressed by without_origin), causing
subscriber producers map misses; fix by tracking which suffixes were actually
announced on each stream (e.g., attach a per-stream HashSet of sent suffixes on
the same Stream/Subscription structure used in publisher.rs), only mark a suffix
as "sent" when you successfully encode an Active (in the branch where active is
Some and not filtered), and before encoding lite::Announce::Ended check that the
suffix exists in that sent set and remove it when you send Ended; do not send
Ended for suffixes not in the set.
Comment on lines
+11
to
+55
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| pub struct OriginId(u64); | ||
|
|
||
| impl OriginId { | ||
| /// Create an OriginId from a raw u64 value. | ||
| pub fn new(value: u64) -> Self { | ||
| Self(value) | ||
| } | ||
|
|
||
| /// Generate a random non-zero 62-bit origin ID. | ||
| pub fn random() -> Self { | ||
| let mut rng = rand::rng(); | ||
| let value = rng.random_range(1..(1u64 << 62)); | ||
| Self(value) | ||
| } | ||
|
|
||
| /// Get the inner u64 value. | ||
| pub fn into_inner(self) -> u64 { | ||
| self.0 | ||
| } | ||
|
|
||
| /// Construct from a raw u64 value (used when decoding from wire). | ||
| pub fn decode_raw(value: u64) -> Self { | ||
| Self(value) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for OriginId { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| self.0.fmt(f) | ||
| } | ||
| } | ||
|
|
||
| impl Encode<Version> for OriginId { | ||
| fn encode<W: bytes::BufMut>(&self, w: &mut W, version: Version) -> Result<(), EncodeError> { | ||
| self.0.encode(w, version) | ||
| } | ||
| } | ||
|
|
||
| impl Decode<Version> for OriginId { | ||
| fn decode<R: bytes::Buf>(r: &mut R, version: Version) -> Result<Self, DecodeError> { | ||
| let value = u64::decode(r, version)?; | ||
| Ok(Self(value)) | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce OriginId invariants at construction/decoding time.
The type is documented as a non-zero 62-bit identifier, but new(), decode_raw(), and Decode<Version> currently accept any u64, and the derived Deserialize does too. That makes OriginId(0) lossy because AnnouncePlease uses 0 as the on-wire sentinel for None, and it also lets --cluster-origin-id accept values outside the documented range. Reject bad IDs at the boundary instead of carrying them deeper into the protocol.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-lite/src/model/origin.rs` around lines 11 - 55, OriginId currently
allows any u64 but must be a non-zero 62-bit value; update constructors and
decoding to validate and reject out-of-range values: change OriginId::new and
OriginId::decode_raw to a Fallible constructor (e.g. try_new(value: u64) ->
Result<OriginId, InvalidOriginId>) that checks 1 <= value < (1u64 << 62), update
impl Decode<Version> for OriginId to validate the decoded u64 and return an
appropriate DecodeError on violation, replace the derived serde::Deserialize
with a custom Deserialize impl that uses the same validation, and update call
sites (e.g. CLI parsing for --cluster-origin-id and any places using
OriginId::new/decode_raw) to use the fallible constructor or convert errors
accordingly; leave OriginId::random as-is since it already produces valid IDs.
Comment on lines
+215
to
+220
| // Prefix check: if the existing broadcast's hops are a prefix of the new one, | ||
| // the new broadcast is routing through us (loop). Reject it. | ||
| if !existing.active.info.hops.is_empty() && broadcast.info.hops.starts_with(&existing.active.info.hops) { | ||
| tracing::debug!(broadcast = %full, "rejecting broadcast: hops are prefix of existing"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Only treat strictly longer prefix matches as redundant loops.
starts_with() also matches identical hop lists, so a second copy of the same route over another session is discarded instead of being kept as a same-cost backup. If the active session drops, the broadcast is spuriously unannounced even though the duplicate path is still live.
Suggested guard
- if !existing.active.info.hops.is_empty() && broadcast.info.hops.starts_with(&existing.active.info.hops) {
+ if !existing.active.info.hops.is_empty()
+ && broadcast.info.hops.len() > existing.active.info.hops.len()
+ && broadcast.info.hops.starts_with(&existing.active.info.hops)
+ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Prefix check: if the existing broadcast's hops are a prefix of the new one, | |
| // the new broadcast is routing through us (loop). Reject it. | |
| if !existing.active.info.hops.is_empty() && broadcast.info.hops.starts_with(&existing.active.info.hops) { | |
| tracing::debug!(broadcast = %full, "rejecting broadcast: hops are prefix of existing"); | |
| return; | |
| } | |
| // Prefix check: if the existing broadcast's hops are a prefix of the new one, | |
| // the new broadcast is routing through us (loop). Reject it. | |
| if !existing.active.info.hops.is_empty() | |
| && broadcast.info.hops.len() > existing.active.info.hops.len() | |
| && broadcast.info.hops.starts_with(&existing.active.info.hops) | |
| { | |
| tracing::debug!(broadcast = %full, "rejecting broadcast: hops are prefix of existing"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-lite/src/model/origin.rs` around lines 215 - 220, The current
loop-detection uses starts_with between broadcast.info.hops and
existing.active.info.hops, but starts_with treats identical hop lists as a
match; change the guard in the block that contains existing.active.info.hops and
broadcast.info.hops so it only rejects when the existing hops are a strict
prefix of the new hops (i.e., keep the existing.is_empty() check and starts_with
check but also require broadcast.info.hops.len() >
existing.active.info.hops.len()); update the condition around starts_with(...)
in the same function/block that references existing and broadcast so identical
hop lists are not treated as redundant loops.
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
1 participant
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.
Summary
OriginIdtype (u62 varint) andLite04version variant (not advertised yet)Broadcast.hops: u64withVec<OriginId>— each hop is an explicit relay identifierANNOUNCEencodes the full OriginId list;ANNOUNCE_PLEASEincludeswithout_originto prevent echo-back (BGP split-horizon style)OriginProducergets a randomOriginId(configurable via--cluster-origin-id), subscribers append it when forwardingwithout_originIDu62bigint encoding for full 62-bit OriginId supportTest plan
cargo check -p moq-lite -p moq-relaycompiles cleancargo test -p moq-lite— 235 tests passtsc --noEmitpasses for all packagesALPNSorVersions::all()(not advertised)🤖 Generated with Claude Code