moq-lite-04: Replace hop count with explicit OriginId list#1152
Merged
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds MoQ Lite DRAFT_04 support across JS and Rust: new ALPN/Version constants and connection branches for ALPN_04 (accept/connect/connectTransport/WebTransport). Converts announce hop representation from scalar counts to ordered origin-paths (JS bigint[] / Rust Vec) and adds an OriginId type with validation and wire encoding. Updates Announce, AnnounceInterest/AnnouncePlease (exclude hop), Subscribe, Fetch, Probe, SessionInfo, Publisher/Subscriber, Broadcast, Origin, and related encode/decode/version gating logic. Adjusts SetupVersion handling and adds an optional cluster origin_id config. 🚥 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 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
- Validate hop count <= 32 and non-zero OriginId in JS/TS encode/decode - Make OriginId::try_new fallible, rejecting zero and out-of-range values - Fill Lite03 decoded hops with OriginId::UNKNOWN placeholders for correct len() - Track sent suffixes in publisher to avoid sending Ended without prior Active - Fix loop detection to require strict prefix (not identical hop lists) - Use wildcard matching for forward-compatible version handling in setup.rs - Update probe.rs doc comment and add doc comments for JS announce fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # js/lite/src/connection/connect.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
js/lite/src/lite/announce.ts (1)
26-44: Extract the hop cap into a constant.The new
32limit is duplicated across encode and decode branches. A sharedMAX_HOPSkeeps the invariant obvious and reduces drift.♻️ Suggested cleanup
+const MAX_HOPS = 32; + export class Announce { @@ - if (this.hops.length > 32) { - throw new Error(`hop count ${this.hops.length} exceeds maximum of 32`); + if (this.hops.length > MAX_HOPS) { + throw new Error(`hop count ${this.hops.length} exceeds maximum of ${MAX_HOPS}`); } @@ - if (count > 32) { - throw new Error(`hop count ${count} exceeds maximum of 32`); + if (count > MAX_HOPS) { + throw new Error(`hop count ${count} exceeds maximum of ${MAX_HOPS}`); }As per coding guidelines, "Avoid using magic numbers; use named constants instead".
Also applies to: 58-82
🤖 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 26 - 44, Introduce a named constant (e.g., MAX_HOPS = 32) and replace the literal 32 checks with that constant wherever hop count is validated (both the encode branch checking this.hops.length and the default/DRAFT_04+ branch loop that currently uses 32), and update the corresponding decode logic referenced around lines handling hops (the other 58-82 section) to use MAX_HOPS as well so the invariant is centralized; ensure all thrown error messages and any uses of the numeric limit reference the constant for clarity.
🤖 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 105-123: Reject explicit 0n for withoutOrigin by validating the
constructor argument: in the constructor that assigns this.withoutOrigin
(symbol: constructor and field withoutOrigin on the Announce interest class),
throw a TypeError (or similar) if withoutOrigin === 0n so callers cannot pass
the wire-sentinel value; keep encoding in `#encode` using w.u62(this.withoutOrigin
?? 0n) unchanged (symbol: `#encode` and w.u62) because the sentinel will still be
used for "unset" when undefined, but explicit 0n must be treated as invalid
input.
In `@rs/moq-lite/src/lite/announce.rs`:
- Around line 39-54: The decoder currently accepts hop counts >32 by truncating
for Version::Lite03 or by allocating capacity but still reading all hops for
Version::Lite04+, which lets peers bypass the protocol limit; change the decode
logic in the announce handling so that when reading count via u64::decode for
Version::Lite03, Version::Lite04 and up you explicitly check if count > 32 and
return an error instead of truncating or proceeding to read extra entries
(update the branches around Version::Lite03 and the _ branch where
decode_origin_id is called to fail on count>32), and mirror this guard in the
encoder path by validating hops.len() <= 32 before writing hops.len() to the
output.
In `@rs/moq-lite/src/lite/publisher.rs`:
- Around line 211-225: When handling an Active broadcast that contains the
without_origin ID, instead of silently continuing, check if that suffix was
previously announced (sent_suffixes) and, if so, remove it and emit
lite::Announce::Ended via stream.writer.encode (mirroring the existing
"unannounce" branch). Concretely, inside the active branch where you currently
do if broadcast.info.hops.contains(id) { continue; }, replace that continue with
logic that does: if sent_suffixes.remove(&suffix) { tracing::debug!(broadcast =
%origin.absolute(&path), "unannounce"); let msg = lite::Announce::Ended {
suffix, hops: Vec::new() }; stream.writer.encode(&msg).await?; } and then
continue; ensuring you reference sent_suffixes, broadcast, without_origin,
lite::Announce::Ended, and stream.writer.encode in the updated flow.
In `@rs/moq-lite/src/model/origin.rs`:
- Around line 237-252: The early-reject branches in OriginNode::publish (the
hops-too-long and prefix-loop checks around existing.active.info.hops and
broadcast.info.hops.starts_with) only log but still let
OriginProducer::publish_broadcast() treat the publish as successful and schedule
remove(), which later hits assert failures; change OriginNode::publish() to
return a bool indicating whether it actually inserted/accepted the broadcast
(false for the new rejection paths), update OriginProducer::publish_broadcast()
to check that returned bool and only register the cleanup task (remove
scheduling) when publish returned true, and ensure all existing callers of
OriginNode::publish are updated to handle the new boolean result.
- Around line 957-961: The test helper test_hops currently returns fresh random
OriginId vectors on each call, so tests like test_hops_same_no_reannounce that
call test_hops twice end up comparing different hop lists; change the tests to
use a single generated hop vector and clone it for the “same” path (or add a new
helper fn test_hops_same(n: usize) -> Vec<OriginId> that generates one
Vec<OriginId> and returns clones where needed) instead of calling test_hops()
twice; update usages (e.g., test_hops_same_no_reannounce and any other places
flagged around lines 1026–1039) to use the cloned vector so the tests truly
compare identical hop sequences rather than just equal lengths.
---
Nitpick comments:
In `@js/lite/src/lite/announce.ts`:
- Around line 26-44: Introduce a named constant (e.g., MAX_HOPS = 32) and
replace the literal 32 checks with that constant wherever hop count is validated
(both the encode branch checking this.hops.length and the default/DRAFT_04+
branch loop that currently uses 32), and update the corresponding decode logic
referenced around lines handling hops (the other 58-82 section) to use MAX_HOPS
as well so the invariant is centralized; ensure all thrown error messages and
any uses of the numeric limit reference the constant for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9d9367b-4970-440f-88a5-3f14d9f6cb65
📒 Files selected for processing (9)
js/lite/src/connection/connect.tsjs/lite/src/lite/announce.tsjs/lite/src/lite/publisher.tsrs/moq-lite/src/lite/announce.rsrs/moq-lite/src/lite/probe.rsrs/moq-lite/src/lite/publisher.rsrs/moq-lite/src/model/origin.rsrs/moq-lite/src/setup.rsrs/moq-relay/src/cluster.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- js/lite/src/connection/connect.ts
- js/lite/src/lite/publisher.ts
- rs/moq-lite/src/lite/probe.rs
- rs/moq-relay/src/cluster.rs
Comment on lines
+105
to
+123
| /// Draft04+ only: filter out announces whose hops contain this origin ID. Uses 0n on the wire for "unset". | ||
| withoutOrigin?: bigint; | ||
|
|
||
| constructor(prefix: Path.Valid, withoutOrigin?: bigint) { | ||
| this.prefix = prefix; | ||
| this.withoutOrigin = withoutOrigin; | ||
| } | ||
|
|
||
| async #encode(w: Writer) { | ||
| async #encode(w: Writer, version: Version) { | ||
| await w.string(this.prefix); | ||
|
|
||
| switch (version) { | ||
| case Version.DRAFT_01: | ||
| case Version.DRAFT_02: | ||
| case Version.DRAFT_03: | ||
| break; | ||
| default: | ||
| // DRAFT_04+: encode withoutOrigin as varint (0 = no filter) | ||
| await w.u62(this.withoutOrigin ?? 0n); |
Contributor
There was a problem hiding this comment.
Reject explicit 0n for withoutOrigin.
0n is the wire sentinel for “unset”. Right now new AnnounceInterest(prefix, 0n) silently disables the filter instead of surfacing invalid input, which makes split-horizon easy to turn off by accident.
🔧 Suggested fix
constructor(prefix: Path.Valid, withoutOrigin?: bigint) {
+ if (withoutOrigin === 0n) {
+ throw new Error("withoutOrigin must be non-zero");
+ }
this.prefix = prefix;
this.withoutOrigin = withoutOrigin;
}🤖 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 105 - 123, Reject explicit 0n for
withoutOrigin by validating the constructor argument: in the constructor that
assigns this.withoutOrigin (symbol: constructor and field withoutOrigin on the
Announce interest class), throw a TypeError (or similar) if withoutOrigin === 0n
so callers cannot pass the wire-sentinel value; keep encoding in `#encode` using
w.u62(this.withoutOrigin ?? 0n) unchanged (symbol: `#encode` and w.u62) because
the sentinel will still be used for "unset" when undefined, but explicit 0n must
be treated as invalid input.
Comment on lines
+237
to
+252
| } else if broadcast.info.hops.len() > 32 { | ||
| // Cap max hops at 32 to prevent abuse. | ||
| tracing::debug!(broadcast = %full, hops = broadcast.info.hops.len(), "rejecting broadcast: too many hops"); | ||
| } else if let Some(existing) = &mut self.broadcast { | ||
| // This node is a leaf with an existing broadcast. | ||
| if broadcast.info.hops < existing.active.info.hops { | ||
|
|
||
| // Prefix check: if the existing broadcast's hops are a strict prefix of the new one, | ||
| // the new broadcast is routing through us (loop). Reject it. | ||
| // Identical hop lists are not treated as loops (could be a re-announcement). | ||
| 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; | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate publish rejection to the caller.
These new rejection branches only log locally. OriginProducer::publish_broadcast() still returns true and always schedules remove(), so when a rejected broadcast later closes under an existing entry, remove() falls through the backup lookup and hits assert!(entry.active.is_clone(&broadcast)). Have OriginNode::publish() report whether it actually inserted the broadcast, and only register cleanup when that is true.
🤖 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 237 - 252, The early-reject
branches in OriginNode::publish (the hops-too-long and prefix-loop checks around
existing.active.info.hops and broadcast.info.hops.starts_with) only log but
still let OriginProducer::publish_broadcast() treat the publish as successful
and schedule remove(), which later hits assert failures; change
OriginNode::publish() to return a bool indicating whether it actually
inserted/accepted the broadcast (false for the new rejection paths), update
OriginProducer::publish_broadcast() to check that returned bool and only
register the cleanup task (remove scheduling) when publish returned true, and
ensure all existing callers of OriginNode::publish are updated to handle the new
boolean result.
Comment on lines
+957
to
+961
| /// Create a hops vector with `n` random OriginIds for testing. | ||
| /// Each call returns a completely independent set of IDs. | ||
| fn test_hops(n: usize) -> Vec<OriginId> { | ||
| (0..n).map(|_| OriginId::random()).collect() | ||
| } |
Contributor
There was a problem hiding this comment.
This regression test no longer uses identical hop paths.
test_hops() returns fresh random IDs on every call, so test_hops_same_no_reannounce() now exercises “same length” rather than “same hops”. That won't catch starts_with() treating identical lists as loops again.
🔧 Suggested fix
- let b1 = Broadcast::new().with_hops(test_hops(2)).produce();
+ let hops = test_hops(2);
+ let b1 = Broadcast::new().with_hops(hops.clone()).produce();
let b1c = b1.consume();
@@
- let b2 = Broadcast::new().with_hops(test_hops(2)).produce();
+ let b2 = Broadcast::new().with_hops(hops).produce();
let _b2c = b2.consume();Also applies to: 1026-1039
🤖 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 957 - 961, The test helper
test_hops currently returns fresh random OriginId vectors on each call, so tests
like test_hops_same_no_reannounce that call test_hops twice end up comparing
different hop lists; change the tests to use a single generated hop vector and
clone it for the “same” path (or add a new helper fn test_hops_same(n: usize) ->
Vec<OriginId> that generates one Vec<OriginId> and returns clones where needed)
instead of calling test_hops() twice; update usages (e.g.,
test_hops_same_no_reannounce and any other places flagged around lines
1026–1039) to use the cloned vector so the tests truly compare identical hop
sequences rather than just equal lengths.
…t_origin - Change OriginId::UNKNOWN from max value to 0 - Replace OriginId::try_new with TryFrom<u64> - Make without_origin a plain OriginId (default 0) instead of Option - Add hop count > 32 validation on encode/decode paths - Fill draft-03 hops with 0n/UNKNOWN placeholders instead of discarding - Send Ended when filtering previously announced suffixes by without_origin - Use UNKNOWN for IETF/Lite01-02 remote broadcasts - Extract MAX_HOPS constant in JS, use props pattern for AnnounceInterest - Simplify AnnounceInit guard to use default for draft-03+ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-lite/src/lite/publisher.rs (1)
173-188: Lite01/02 init handling correctly filters bywithout_origin.The filtering logic mirrors the main announce loop. However, there's an inconsistency: when a race causes
activeto beNone(line 182-187), the code removes fromsent_suffixesbut the suffix may not have been added yet during this initial loop iteration.This is benign since
HashSet::removeis a no-op for non-existent keys, but for clarity:♻️ Optional: simplify the race handling
} else { // A potential race. tracing::debug!(broadcast = %origin.absolute(&path), "unannounce"); - let owned = suffix.to_owned(); - sent_suffixes.remove(&owned); init.retain(|path| path != &suffix); }The
sent_suffixes.removehere is unnecessary since we're in the initial sync loop before any Active messages have been sent.🤖 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 173 - 188, The initial-sync branch that handles the race when active is None needlessly calls sent_suffixes.remove(&owned) even though no announces have been sent yet; remove the sent_suffixes.remove(...) call from the else branch (inside the block that logs "unannounce") so only init.retain(...) is kept, leaving sent_suffixes untouched during the init loop; reference the variables/symbols active, without_origin, sent_suffixes, init, suffix, owned, and origin.absolute(&path) when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rs/moq-lite/src/lite/publisher.rs`:
- Around line 173-188: The initial-sync branch that handles the race when active
is None needlessly calls sent_suffixes.remove(&owned) even though no announces
have been sent yet; remove the sent_suffixes.remove(...) call from the else
branch (inside the block that logs "unannounce") so only init.retain(...) is
kept, leaving sent_suffixes untouched during the init loop; reference the
variables/symbols active, without_origin, sent_suffixes, init, suffix, owned,
and origin.absolute(&path) when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a7b1f4f-5c63-47a9-ae06-65fe88d6db41
📒 Files selected for processing (8)
js/lite/src/lite/announce.tsjs/lite/src/lite/subscriber.tsrs/moq-lite/src/ietf/subscriber.rsrs/moq-lite/src/lite/announce.rsrs/moq-lite/src/lite/publisher.rsrs/moq-lite/src/lite/subscriber.rsrs/moq-lite/src/model/origin.rsrs/moq-relay/src/cluster.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- js/lite/src/lite/subscriber.ts
- rs/moq-relay/src/cluster.rs
- rs/moq-lite/src/model/origin.rs
- js/lite/src/lite/announce.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/lite/src/lite/announce.ts (1)
35-49: MakeVersion.DRAFT_04explicit instead of usingdefault.These
defaultbranches currently assume every future version shares Draft04 framing. That removes exhaustiveness checking and makes the next version bump silently misencode/misdecode instead of failing fast. Prefer explicitcase Version.DRAFT_04branches and reservedefaultfor an unsupported-version error.Also applies to: 73-87, 125-142
🤖 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 35 - 49, The switch currently uses a default branch to implement DRAFT_04 framing which hides missing-case errors; replace the default branch with an explicit case Version.DRAFT_04 that contains the existing logic (checks on this.hops length vs MAX_HOPS, non-zero hop check, await w.u53(this.hops.length) and the await w.u62(hop) loop), and change the switch's default to throw an unsupported/unknown version error so future Version additions fail fast; apply the same change pattern to the other two occurrences noted (the blocks around the areas corresponding to lines 73-87 and 125-142) so all Draft04 handling is explicit via case Version.DRAFT_04 and default always errors.
🤖 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 12-13: The Announce type's hops array must not be populated with
synthetic 0n entries when decoding Draft03; update the Draft03 decode logic (the
code that currently converts the legacy hop count into 0n elements) to leave
Announce.hops as an empty array and instead preserve the legacy count in a
separate field (e.g., legacyHopCount) or retain the wire version on the object
so callers can distinguish legacy counts from real hops, and adjust the encoder
to consult that new field when re-encoding Draft03 wire format rather than
relying on zeros in hops.
---
Nitpick comments:
In `@js/lite/src/lite/announce.ts`:
- Around line 35-49: The switch currently uses a default branch to implement
DRAFT_04 framing which hides missing-case errors; replace the default branch
with an explicit case Version.DRAFT_04 that contains the existing logic (checks
on this.hops length vs MAX_HOPS, non-zero hop check, await
w.u53(this.hops.length) and the await w.u62(hop) loop), and change the switch's
default to throw an unsupported/unknown version error so future Version
additions fail fast; apply the same change pattern to the other two occurrences
noted (the blocks around the areas corresponding to lines 73-87 and 125-142) so
all Draft04 handling is explicit via case Version.DRAFT_04 and default always
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdfeaaf3-2866-4c76-ac51-e3ed8bbbf62e
📒 Files selected for processing (5)
js/lite/src/connection/connect.tsjs/lite/src/lite/announce.tsrs/moq-lite/src/lite/announce.rsrs/moq-lite/src/lite/publisher.rsrs/moq-lite/src/lite/subscriber.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- js/lite/src/connection/connect.ts
Comment on lines
+12
to
+13
| /// Ordered origin path for Draft04+. Empty when decoding Draft03 (which only carries a count). | ||
| hops: bigint[]; |
Contributor
There was a problem hiding this comment.
Don't surface Draft03 hop counts as fake 0n OriginIds.
Lines 60-67 turn a legacy count into synthetic 0n entries even though the Draft04 encode/decode paths reject 0n hop values, and the field docs on Lines 12-13 say Draft03 should decode to an empty list. Because Announce does not retain the wire version, callers can't tell placeholders from real path data, and re-encoding this object on a Draft04 connection will now fail the non-zero check. If the Draft03 count still matters, keep it in a separate field instead of overloading hops.
Also applies to: 59-67
🤖 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 12 - 13, The Announce type's hops
array must not be populated with synthetic 0n entries when decoding Draft03;
update the Draft03 decode logic (the code that currently converts the legacy hop
count into 0n elements) to leave Announce.hops as an empty array and instead
preserve the legacy count in a separate field (e.g., legacyHopCount) or retain
the wire version on the object so callers can distinguish legacy counts from
real hops, and adjust the encoder to consult that new field when re-encoding
Draft03 wire format rather than relying on zeros in hops.
…xplicit Remove the non-zero OriginId validation on encode/decode since 0 is valid (means UNKNOWN). Add explicit `case Version.DRAFT_04` before `default` in all switch blocks so the intent is clear while still defaulting forward. 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
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/lite/src/lite/announce.ts (1)
98-103:⚠️ Potential issue | 🟡 MinorDon't let explicit
0nsilently disable split-horizon filtering.Because
0nalready means “no filter”,new AnnounceInterest({ prefix, excludeHop: 0n })is indistinguishable from omitting the field and turns the filter off without any signal. Reject explicit0nhere, or keep the property optional in-memory and only mapundefinedto0nwhen encoding.🔧 Minimal guard
constructor(props: { prefix: Path.Valid; excludeHop?: bigint }) { this.prefix = props.prefix; + if (props.excludeHop === 0n) { + throw new Error("excludeHop must be non-zero; omit it to disable filtering"); + } this.excludeHop = props.excludeHop ?? 0n; }🤖 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 98 - 103, The constructor currently coerces an explicit 0n into the same “no-filter” state as omitted values by assigning props.excludeHop ?? 0n; change AnnounceInterest to keep excludeHop optional in-memory (declare excludeHop?: bigint and set this.excludeHop = props.excludeHop without defaulting) and instead perform the mapping to 0n only in the serialization/encoding path (where the AnnounceInterest is encoded for wire/bytes), converting undefined -> 0n there; alternatively, if you prefer rejecting explicit 0n, validate in the constructor and throw when props.excludeHop === 0n—use the class name AnnounceInterest, the constructor, and the excludeHop property to locate the changes.
🧹 Nitpick comments (1)
js/lite/src/lite/announce.ts (1)
25-43: Fail closed on unknown Lite versions.Both switches currently treat every non-01/02/03 value as Draft04 wire format. That works with today's enum, but the next
Versionaddition will silently inherit Lite04 encoding/decoding instead of forcing an explicit protocol decision. Add an explicitcase Version.DRAFT_04and throw indefault, likeAnnounceInit.#guard()already does.Also applies to: 52-76, 109-117, 124-131
🤖 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 25 - 43, The switch blocks that currently treat any unknown Version as Draft04 should be made explicit: add a case Version.DRAFT_04 that implements the current Draft04 behavior (check this.hops.length against MAX_HOPS, await w.u53(this.hops.length) and await w.u62(hop) for each hop) and replace the permissive default with a throw (e.g. throw new Error(`unsupported version ${version}`)); apply the same change to every switch(version) in this module (the blocks that currently use this.hops, MAX_HOPS, w.u53/w.u62) so unknown Version values fail closed (follow the pattern from AnnounceInit.#guard()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/lite/src/lite/announce.ts`:
- Around line 98-103: The constructor currently coerces an explicit 0n into the
same “no-filter” state as omitted values by assigning props.excludeHop ?? 0n;
change AnnounceInterest to keep excludeHop optional in-memory (declare
excludeHop?: bigint and set this.excludeHop = props.excludeHop without
defaulting) and instead perform the mapping to 0n only in the
serialization/encoding path (where the AnnounceInterest is encoded for
wire/bytes), converting undefined -> 0n there; alternatively, if you prefer
rejecting explicit 0n, validate in the constructor and throw when
props.excludeHop === 0n—use the class name AnnounceInterest, the constructor,
and the excludeHop property to locate the changes.
---
Nitpick comments:
In `@js/lite/src/lite/announce.ts`:
- Around line 25-43: The switch blocks that currently treat any unknown Version
as Draft04 should be made explicit: add a case Version.DRAFT_04 that implements
the current Draft04 behavior (check this.hops.length against MAX_HOPS, await
w.u53(this.hops.length) and await w.u62(hop) for each hop) and replace the
permissive default with a throw (e.g. throw new Error(`unsupported version
${version}`)); apply the same change to every switch(version) in this module
(the blocks that currently use this.hops, MAX_HOPS, w.u53/w.u62) so unknown
Version values fail closed (follow the pattern from AnnounceInit.#guard()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18455fc5-31e6-4775-9dbb-eb562df9661f
📒 Files selected for processing (1)
js/lite/src/lite/announce.ts
…sher Integrated dev's closure-based broadcast tracking with exclude_hop filtering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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